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

@@ -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

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