Merge branch 'if-webauthn-avoid_falling_back_to_u2f' into 'master'

Avoid falling back to U2F registrations when WebAuthn enabled

See merge request gitlab-org/gitlab!75748

app/controllers/concerns/authenticates_with_two_factor.rb

@@ -25,7 +25,7 @@ module AuthenticatesWithTwoFactor
     session[:user_password_hash] = Digest::SHA256.hexdigest(user.encrypted_password)
     push_frontend_feature_flag(:webauthn)
 
-    if user.two_factor_webauthn_enabled?
+    if Feature.enabled?(:webauthn)
       setup_webauthn_authentication(user)
     else
       setup_u2f_authentication(user)

app/models/user.rb

@@ -917,6 +917,8 @@ class User < ApplicationRecord
   end
 
   def two_factor_u2f_enabled?
+    return false if Feature.enabled?(:webauthn)
+
     if u2f_registrations.loaded?
       u2f_registrations.any?
     else

ee/spec/controllers/ee/admin/sessions_controller_spec.rb

@@ -26,6 +26,7 @@ RSpec.describe Admin::SessionsController, :do_not_mock_admin_mode do
 
       context 'when U2F authentication fails' do
         before do
+          stub_feature_flags(webauthn: false)
           allow(U2fRegistration).to receive(:authenticate).and_return(false)
         end
 

ee/spec/controllers/ee/sessions_controller_spec.rb

@@ -104,6 +104,7 @@ RSpec.describe SessionsController, :geo do
 
       context 'when U2F authentication fails' do
         before do
+          stub_feature_flags(webauthn: false)
           allow(U2fRegistration).to receive(:authenticate).and_return(false)
         end
 

spec/features/webauthn_spec.rb

@@ -113,124 +113,94 @@ RSpec.describe 'Using WebAuthn Devices for Authentication', :js do
   describe 'authentication' do
     let(:otp_required_for_login) { true }
     let(:user) { create(:user, webauthn_xid: WebAuthn.generate_user_id, otp_required_for_login: otp_required_for_login) }
+    let!(:webauthn_device) do
+      add_webauthn_device(app_id, user)
+    end
 
-    describe 'when there is only an U2F device' do
-      let!(:u2f_device) do
-        fake_device = U2F::FakeU2F.new(app_id) # "Client"
-        u2f = U2F::U2F.new(app_id) # "Server"
+    describe 'when 2FA via OTP is disabled' do
+      let(:otp_required_for_login) { false }
 
-        challenges = u2f.registration_requests.map(&:challenge)
-        device_response = fake_device.register_response(challenges[0])
-        device_registration_params = { device_response: device_response,
-                                       name: 'My device' }
+      it 'allows logging in with the WebAuthn device' do
+        gitlab_sign_in(user)
 
-        U2fRegistration.register(user, app_id, device_registration_params, challenges)
-        FakeU2fDevice.new(page, 'My device', fake_device)
-      end
+        webauthn_device.respond_to_webauthn_authentication
 
-      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
+        expect(page).to have_css('.sign-out-link', visible: false)
+      end
+    end
 
+    describe 'when 2FA via OTP is enabled' do
+      it 'allows logging in with the WebAuthn device' do
         gitlab_sign_in(user)
 
-        u2f_device.respond_to_u2f_authentication
+        webauthn_device.respond_to_webauthn_authentication
 
         expect(page).to have_css('.sign-out-link', visible: false)
       end
     end
 
-    describe 'when there is a WebAuthn device' do
-      let!(:webauthn_device) do
-        add_webauthn_device(app_id, user)
-      end
+    describe 'when a given WebAuthn device has already been registered by another user' do
+      describe 'but not the current user' do
+        let(:other_user) { create(:user, webauthn_xid: WebAuthn.generate_user_id, otp_required_for_login: otp_required_for_login) }
 
-      describe 'when 2FA via OTP is disabled' do
-        let(:otp_required_for_login) { false }
+        it 'does not allow logging in with that particular device' do
+          # Register other user with a different WebAuthn device
+          other_device = add_webauthn_device(app_id, other_user)
 
-        it 'allows logging in with the WebAuthn device' do
+          # Try authenticating user with the old WebAuthn device
           gitlab_sign_in(user)
-
-          webauthn_device.respond_to_webauthn_authentication
-
-          expect(page).to have_css('.sign-out-link', visible: false)
+          other_device.respond_to_webauthn_authentication
+          expect(page).to have_content('Authentication via WebAuthn device failed')
         end
       end
 
-      describe 'when 2FA via OTP is enabled' do
-        it 'allows logging in with the WebAuthn device' do
-          gitlab_sign_in(user)
-
+      describe "and also the current user" do
+        # TODO Uncomment once WebAuthn::FakeClient supports passing credential options
+        # (especially allow_credentials, as this is needed to specify which credential the
+        # fake client should use. Currently, the first credential is always used).
+        # There is an issue open for this: https://github.com/cedarcode/webauthn-ruby/issues/259
+        it "allows logging in with that particular device" do
+          pending("support for passing credential options in FakeClient")
+          # Register current user with the same WebAuthn device
+          current_user = gitlab_sign_in(:user)
+          visit profile_account_path
+          manage_two_factor_authentication
+          register_webauthn_device(webauthn_device)
+          gitlab_sign_out
+
+          # Try authenticating user with the same WebAuthn device
+          gitlab_sign_in(current_user)
           webauthn_device.respond_to_webauthn_authentication
 
           expect(page).to have_css('.sign-out-link', visible: false)
         end
       end
+    end
 
-      describe 'when a given WebAuthn device has already been registered by another user' do
-        describe 'but not the current user' do
-          let(:other_user) { create(:user, webauthn_xid: WebAuthn.generate_user_id, otp_required_for_login: otp_required_for_login) }
-
-          it 'does not allow logging in with that particular device' do
-            # Register other user with a different WebAuthn device
-            other_device = add_webauthn_device(app_id, other_user)
-
-            # Try authenticating user with the old WebAuthn device
-            gitlab_sign_in(user)
-            other_device.respond_to_webauthn_authentication
-            expect(page).to have_content('Authentication via WebAuthn device failed')
-          end
-        end
-
-        describe "and also the current user" do
-          # TODO Uncomment once WebAuthn::FakeClient supports passing credential options
-          # (especially allow_credentials, as this is needed to specify which credential the
-          # fake client should use. Currently, the first credential is always used).
-          # There is an issue open for this: https://github.com/cedarcode/webauthn-ruby/issues/259
-          it "allows logging in with that particular device" do
-            pending("support for passing credential options in FakeClient")
-            # Register current user with the same WebAuthn device
-            current_user = gitlab_sign_in(:user)
-            visit profile_account_path
-            manage_two_factor_authentication
-            register_webauthn_device(webauthn_device)
-            gitlab_sign_out
-
-            # Try authenticating user with the same WebAuthn device
-            gitlab_sign_in(current_user)
-            webauthn_device.respond_to_webauthn_authentication
-
-            expect(page).to have_css('.sign-out-link', visible: false)
-          end
-        end
-      end
-
-      describe 'when a given WebAuthn device has not been registered' do
-        it 'does not allow logging in with that particular device' do
-          unregistered_device = FakeWebauthnDevice.new(page, 'My device')
-          gitlab_sign_in(user)
-          unregistered_device.respond_to_webauthn_authentication
+    describe 'when a given WebAuthn device has not been registered' do
+      it 'does not allow logging in with that particular device' do
+        unregistered_device = FakeWebauthnDevice.new(page, 'My device')
+        gitlab_sign_in(user)
+        unregistered_device.respond_to_webauthn_authentication
 
-          expect(page).to have_content('Authentication via WebAuthn device failed')
-        end
+        expect(page).to have_content('Authentication via WebAuthn device failed')
       end
+    end
 
-      describe 'when more than one device has been registered by the same user' do
-        it 'allows logging in with either device' do
-          first_device = add_webauthn_device(app_id, user)
-          second_device = add_webauthn_device(app_id, user)
+    describe 'when more than one device has been registered by the same user' do
+      it 'allows logging in with either device' do
+        first_device = add_webauthn_device(app_id, user)
+        second_device = add_webauthn_device(app_id, user)
 
-          # Authenticate as both devices
-          [first_device, second_device].each do |device|
-            gitlab_sign_in(user)
-            # register_webauthn_device(device)
-            device.respond_to_webauthn_authentication
+        # Authenticate as both devices
+        [first_device, second_device].each do |device|
+          gitlab_sign_in(user)
+          # register_webauthn_device(device)
+          device.respond_to_webauthn_authentication
 
-            expect(page).to have_css('.sign-out-link', visible: false)
+          expect(page).to have_css('.sign-out-link', visible: false)
 
-            gitlab_sign_out
-          end
+          gitlab_sign_out
         end
       end
     end

spec/models/user_spec.rb

@@ -1726,6 +1726,52 @@ RSpec.describe User do
     end
   end
 
+  context 'two_factor_u2f_enabled?' do
+    let_it_be(:user) { create(:user, :two_factor) }
+
+    context 'when webauthn feature flag is enabled' do
+      context 'user has no U2F registration' do
+        it { expect(user.two_factor_u2f_enabled?).to eq(false) }
+      end
+
+      context 'user has existing U2F registration' do
+        it 'returns false' do
+          device = U2F::FakeU2F.new(FFaker::BaconIpsum.characters(5))
+          create(:u2f_registration, name: 'my 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))
+
+          expect(user.two_factor_u2f_enabled?).to eq(false)
+        end
+      end
+    end
+
+    context 'when webauthn feature flag is disabled' do
+      before do
+        stub_feature_flags(webauthn: false)
+      end
+
+      context 'user has no U2F registration' do
+        it { expect(user.two_factor_u2f_enabled?).to eq(false) }
+      end
+
+      context 'user has existing U2F registration' do
+        it 'returns true' do
+          device = U2F::FakeU2F.new(FFaker::BaconIpsum.characters(5))
+          create(:u2f_registration, name: 'my 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))
+
+          expect(user.two_factor_u2f_enabled?).to eq(true)
+        end
+      end
+    end
+  end
+
   describe 'projects' do
     before do
       @user = create(:user)