From 0374323c6b2e6080cf6bcc2502b267354e11365f Mon Sep 17 00:00:00 2001
From: Jessie Young <jessieyoung@gitlab.com>
Date: Thu, 28 Jul 2022 18:04:43 +0000
Subject: [PATCH] Fix bug in U2fRegistration after_update callback
* In a pipeline for a separate MR, rspec undercover flagged that
this was not tested: https://gitlab.com/gitlab-org/gitlab/-/jobs/2778676259
* Specs for the existing behavior revealed that this callback was not
called as expected due to the activerecord dirty condition
* In ActiveRecord 5.1, the following deprecation message was added:
DEPRECATION WARNING: The behavior of `attribute_changed?` inside of
after callbacks will be changing in the next version of Rails. The new
return value will reflect the behavior of calling the method after
`save` returned (e.g. the opposite of what it returns now). To maintain
the current behavior, use `saved_change_to_attribute?` instead.
* Starting with Rails 5.2 (we are currently on Rails 6.1.6.1), within an
`after_update` callback, `attribute_changed?` now returns `false` even
if the attribute was changed.
* So in this case, changes to the `counter` attribute were not
triggering this callback.
* Updating the callback to `saved_change_to_<attribute>` fixes the
issue: https://github.com/rails/rails/blob/04972d9b9ef60796dc8f0917817b5392d61fcf09/activerecord/lib/active_record/attribute_methods/dirty.rb#L86-L92
---
app/models/u2f_registration.rb | 34 +++++++++++-----------
spec/models/u2f_registration_spec.rb | 43 +++++++++++++++++++++-------
2 files changed, 49 insertions(+), 28 deletions(-)
diff --git a/app/models/u2f_registration.rb b/app/models/u2f_registration.rb
index 7c01aa7a42099..b4b24ea1486f4 100644
--- a/app/models/u2f_registration.rb
+++ b/app/models/u2f_registration.rb
@@ -6,21 +6,7 @@ class U2fRegistration < ApplicationRecord
belongs_to :user
after_create :create_webauthn_registration
- after_update :update_webauthn_registration, if: :counter_changed?
-
- def create_webauthn_registration
- converter = Gitlab::Auth::U2fWebauthnConverter.new(self)
- WebauthnRegistration.create!(converter.convert)
- rescue StandardError => ex
- Gitlab::ErrorTracking.track_exception(ex, u2f_registration_id: self.id)
- end
-
- def update_webauthn_registration
- # When we update the sign count of this registration
- # we need to update the sign count of the corresponding webauthn registration
- # as well if it exists already
- WebauthnRegistration.find_by_credential_xid(webauthn_credential_xid)&.update_attribute(:counter, counter)
- end
+ after_update :update_webauthn_registration, if: :saved_change_to_counter?
def self.register(user, app_id, params, challenges)
u2f = U2F::U2F.new(app_id)
@@ -60,10 +46,22 @@ def self.authenticate(user, app_id, json_response, challenges)
private
+ def create_webauthn_registration
+ converter = Gitlab::Auth::U2fWebauthnConverter.new(self)
+ WebauthnRegistration.create!(converter.convert)
+ rescue StandardError => ex
+ Gitlab::ErrorTracking.track_exception(ex, u2f_registration_id: self.id)
+ end
+
+ def update_webauthn_registration
+ # When we update the sign count of this registration
+ # we need to update the sign count of the corresponding webauthn registration
+ # as well if it exists already
+ WebauthnRegistration.find_by_credential_xid(webauthn_credential_xid)
+ &.update_attribute(:counter, counter)
+ end
+
def webauthn_credential_xid
- # To find the corresponding webauthn registration, we use that
- # the key handle of the u2f reg corresponds to the credential xid of the webauthn reg
- # (with some base64 back and forth)
Base64.strict_encode64(Base64.urlsafe_decode64(key_handle))
end
end
diff --git a/spec/models/u2f_registration_spec.rb b/spec/models/u2f_registration_spec.rb
index 6bb9ccfcf35cd..027d26d965732 100644
--- a/spec/models/u2f_registration_spec.rb
+++ b/spec/models/u2f_registration_spec.rb
@@ -6,23 +6,22 @@
let_it_be(:user) { create(:user) }
let(:u2f_registration_name) { 'u2f_device' }
-
let(:u2f_registration) do
device = U2F::FakeU2F.new(FFaker::BaconIpsum.characters(5))
- create(:u2f_registration, name: u2f_registration_name,
- user: user,
- certificate: Base64.strict_encode64(device.cert_raw),
- key_handle: U2F.urlsafe_encode64(device.key_handle_raw),
- public_key: Base64.strict_encode64(device.origin_public_key_raw))
+ create(
+ :u2f_registration, name: u2f_registration_name,
+ user: user,
+ certificate: Base64.strict_encode64(device.cert_raw),
+ key_handle: U2F.urlsafe_encode64(device.key_handle_raw),
+ public_key: Base64.strict_encode64(device.origin_public_key_raw)
+ )
end
describe 'callbacks' do
- describe '#create_webauthn_registration' do
+ describe 'after create' do
shared_examples_for 'creates webauthn registration' do
it 'creates webauthn registration' do
- created_record = u2f_registration
-
- webauthn_registration = WebauthnRegistration.where(u2f_registration_id: created_record.id)
+ webauthn_registration = WebauthnRegistration.where(u2f_registration_id: u2f_registration.id)
expect(webauthn_registration).to exist
end
end
@@ -55,5 +54,29 @@
u2f_registration
end
end
+
+ describe 'after update' do
+ context 'when counter is updated' do
+ it 'updates the webauthn registration counter to be the same value' do
+ new_counter = u2f_registration.counter + 1
+ webauthn_registration = WebauthnRegistration.find_by(u2f_registration_id: u2f_registration.id)
+
+ u2f_registration.update!(counter: new_counter)
+
+ expect(u2f_registration.reload.counter).to eq(new_counter)
+ expect(webauthn_registration.reload.counter).to eq(new_counter)
+ end
+ end
+
+ context 'when sign count of registration is not updated' do
+ it 'does not update the counter' do
+ webauthn_registration = WebauthnRegistration.find_by(u2f_registration_id: u2f_registration.id)
+
+ expect do
+ u2f_registration.update!(name: 'a new name')
+ end.not_to change { webauthn_registration.counter }
+ end
+ end
+ end
end
end
--
GitLab