Merge branch 'if-webauthn_conversion_logging' into 'master'

Improve logging around U2F to WebAuthn conversion

See merge request gitlab-org/gitlab!51447

app/models/u2f_registration.rb

@@ -4,11 +4,19 @@
 
 class U2fRegistration < ApplicationRecord
   belongs_to :user
-  after_commit :schedule_webauthn_migration, on: :create
-  after_commit :update_webauthn_registration, on: :update, if: :counter_changed?
 
-  def schedule_webauthn_migration
-    BackgroundMigrationWorker.perform_async('MigrateU2fWebauthn', [id, id])
+  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::AppJsonLogger.error(
+      event: 'u2f_migration',
+      error: ex.class.name,
+      backtrace: ::Gitlab::BacktraceCleaner.clean_backtrace(ex.backtrace),
+      message: "U2F to WebAuthn conversion failed")
   end
 
   def update_webauthn_registration

lib/gitlab/auth/u2f_webauthn_converter.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Auth
+    class U2fWebauthnConverter
+      def initialize(u2f_registration)
+        @u2f_registration = u2f_registration
+      end
+
+      def convert
+        now = Time.current
+
+        converted_credential = WebAuthn::U2fMigrator.new(
+          app_id: Gitlab.config.gitlab.url,
+          certificate: u2f_registration.certificate,
+          key_handle: u2f_registration.key_handle,
+          public_key: u2f_registration.public_key,
+          counter: u2f_registration.counter
+        ).credential
+
+        {
+          credential_xid: Base64.strict_encode64(converted_credential.id),
+          public_key: Base64.strict_encode64(converted_credential.public_key),
+          counter: u2f_registration.counter || 0,
+          name: u2f_registration.name || '',
+          user_id: u2f_registration.user_id,
+          u2f_registration_id: u2f_registration.id,
+          created_at: now,
+          updated_at: now
+        }
+      end
+
+      private
+
+      attr_reader :u2f_registration
+    end
+  end
+end

lib/gitlab/background_migration/migrate_u2f_webauthn.rb

@@ -16,26 +16,9 @@ module Gitlab
       def perform(start_id, end_id)
         old_registrations = U2fRegistration.where(id: start_id..end_id)
         old_registrations.each_slice(100) do |slice|
-          now = Time.now
           values = slice.map do |u2f_registration|
-            converted_credential = WebAuthn::U2fMigrator.new(
-              app_id: Gitlab.config.gitlab.url,
-              certificate: u2f_registration.certificate,
-              key_handle: u2f_registration.key_handle,
-              public_key: u2f_registration.public_key,
-              counter: u2f_registration.counter
-            ).credential
-
-            {
-                credential_xid: Base64.strict_encode64(converted_credential.id),
-                public_key: Base64.strict_encode64(converted_credential.public_key),
-                counter: u2f_registration.counter || 0,
-                name: u2f_registration.name || '',
-                user_id: u2f_registration.user_id,
-                u2f_registration_id: u2f_registration.id,
-                created_at: now,
-                updated_at: now
-            }
+            converter = Gitlab::Auth::U2fWebauthnConverter.new(u2f_registration)
+            converter.convert
           end
 
           WebauthnRegistration.insert_all(values, unique_by: :credential_xid, returning: false)

spec/factories/u2f_registrations.rb

@@ -2,6 +2,8 @@
 
 FactoryBot.define do
   factory :u2f_registration do
+    user
+
     certificate { FFaker::BaconIpsum.characters(728) }
     key_handle { FFaker::BaconIpsum.characters(86) }
     public_key { FFaker::BaconIpsum.characters(88) }

spec/features/u2f_spec.rb

@@ -39,6 +39,11 @@ RSpec.describe 'Using U2F (Universal 2nd Factor) Devices for Authentication', :j
     end
 
     it 'allows the same device to be registered for multiple users' do
+      # U2f specs will be removed after WebAuthn migration completed
+      pending('FakeU2fDevice has static key handle, '\
+              'leading to duplicate credential_xid for WebAuthn during migration, '\
+              'resulting in unique constraint violation')
+
       # First user
       visit profile_account_path
       manage_two_factor_authentication
@@ -148,6 +153,11 @@ RSpec.describe 'Using U2F (Universal 2nd Factor) Devices for Authentication', :j
 
       describe "and also the current user" do
         it "allows logging in with that particular device" do
+          # U2f specs will be removed after WebAuthn migration completed
+          pending('FakeU2fDevice has static key handle, '\
+                  'leading to duplicate credential_xid for WebAuthn during migration, '\
+                  'resulting in unique constraint violation')
+
           # Register current user with the same U2F device
           current_user = gitlab_sign_in(:user)
           current_user.update_attribute(:otp_required_for_login, true)

spec/features/webauthn_spec.rb

@@ -129,6 +129,10 @@ RSpec.describe 'Using WebAuthn Devices for Authentication', :js do
       end
 
       it 'falls back to U2F' do
+        # WebAuthn registration is automatically created with the U2fRegistration because of the after_create callback
+        # so we need to delete it
+        WebauthnRegistration.delete_all
+
         gitlab_sign_in(user)
 
         u2f_device.respond_to_u2f_authentication

spec/lib/gitlab/auth/u2f_webauthn_converter_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::Auth::U2fWebauthnConverter do
+  let_it_be(:u2f_registration) do
+    device = U2F::FakeU2F.new(FFaker::BaconIpsum.characters(5))
+    create(:u2f_registration, name: 'u2f_device',
+                              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
+
+  it 'converts u2f registration' do
+    webauthn_credential = WebAuthn::U2fMigrator.new(
+      app_id: Gitlab.config.gitlab.url,
+      certificate: u2f_registration.certificate,
+      key_handle: u2f_registration.key_handle,
+      public_key: u2f_registration.public_key,
+      counter: u2f_registration.counter
+    ).credential
+
+    converted_webauthn = described_class.new(u2f_registration).convert
+
+    expect(converted_webauthn).to(
+      include(user_id: u2f_registration.user_id,
+              credential_xid: Base64.strict_encode64(webauthn_credential.id)))
+  end
+end

spec/models/u2f_registration_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe U2fRegistration do
+  let_it_be(:user) { create(:user) }
+  let(:u2f_registration) do
+    device = U2F::FakeU2F.new(FFaker::BaconIpsum.characters(5))
+    create(:u2f_registration, name: 'u2f_device',
+                              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
+      it 'creates webauthn registration' do
+        u2f_registration.save!
+
+        webauthn_registration = WebauthnRegistration.where(u2f_registration_id: u2f_registration.id)
+        expect(webauthn_registration).to exist
+      end
+
+      it 'logs error' do
+        allow(Gitlab::Auth::U2fWebauthnConverter).to receive(:new).and_raise('boom!')
+        expect(Gitlab::AppJsonLogger).to(
+          receive(:error).with(a_hash_including(event: 'u2f_migration',
+                                                error: 'RuntimeError',
+                                                message: 'U2F to WebAuthn conversion failed'))
+        )
+
+        u2f_registration.save!
+      end
+    end
+  end
+end