diff --git a/app/controllers/users/webauthn_setup_controller.rb b/app/controllers/users/webauthn_setup_controller.rb index 43fbc570331..ec1719c102a 100644 --- a/app/controllers/users/webauthn_setup_controller.rb +++ b/app/controllers/users/webauthn_setup_controller.rb @@ -134,7 +134,7 @@ def process_valid_webauthn(form) success: true, ) handle_remember_device_preference(params[:remember_device]) - if form.platform_authenticator? + if form.setup_as_platform_authenticator? handle_valid_verification_for_confirmation_context( auth_method: TwoFactorAuthenticatable::AuthMethod::WEBAUTHN_PLATFORM, ) @@ -144,7 +144,7 @@ def process_valid_webauthn(form) analytics, threatmetrix_attrs, ) - flash[:success] = t('notices.webauthn_platform_configured') + flash[:success] = t('notices.webauthn_platform_configured') if !form.transports_mismatch? else handle_valid_verification_for_confirmation_context( auth_method: TwoFactorAuthenticatable::AuthMethod::WEBAUTHN, @@ -155,9 +155,15 @@ def process_valid_webauthn(form) analytics, threatmetrix_attrs, ) - flash[:success] = t('notices.webauthn_configured') + flash[:success] = t('notices.webauthn_configured') if !form.transports_mismatch? + end + + if form.transports_mismatch? + user_session[:webauthn_mismatch_id] = form.webauthn_configuration.id + redirect_to webauthn_setup_mismatch_path + else + redirect_to next_setup_path || after_mfa_setup_path end - redirect_to next_setup_path || after_mfa_setup_path end def analytics_properties diff --git a/app/forms/webauthn_setup_form.rb b/app/forms/webauthn_setup_form.rb index 727e6f60c39..ea95f712bc5 100644 --- a/app/forms/webauthn_setup_form.rb +++ b/app/forms/webauthn_setup_form.rb @@ -12,7 +12,7 @@ class WebauthnSetupForm validate :name_is_unique validate :validate_attestation_response - attr_reader :attestation_response + attr_reader :attestation_response, :webauthn_configuration def initialize(user:, user_session:, device_name:) @user = user @@ -48,6 +48,18 @@ def platform_authenticator? !!@platform_authenticator end + def setup_as_platform_authenticator? + if transports.present? + platform_authenticator_transports? + else + platform_authenticator? + end + end + + def transports_mismatch? + transports.present? && platform_authenticator_transports? != platform_authenticator? + end + def generic_error_message if platform_authenticator? I18n.t('errors.webauthn_platform_setup.general_error') @@ -134,11 +146,11 @@ def create_webauthn_configuration credential = attestation_response.credential public_key = Base64.strict_encode64(credential.public_key) id = Base64.strict_encode64(credential.id) - user.webauthn_configurations.create( + @webauthn_configuration = user.webauthn_configurations.create( credential_public_key: public_key, credential_id: id, name: name, - platform_authenticator: platform_authenticator, + platform_authenticator: setup_as_platform_authenticator?, transports: transports.presence, authenticator_data_flags: authenticator_data_flags, aaguid: aaguid, @@ -166,20 +178,29 @@ def aaguid nil end + def platform_authenticator_transports? + (transports & WebauthnConfiguration::PLATFORM_AUTHENTICATOR_TRANSPORTS.to_a).present? + end + + def multi_factor_auth_method + if setup_as_platform_authenticator? + 'webauthn_platform' + else + 'webauthn' + end + end + def extra_analytics_attributes - auth_method = if platform_authenticator? - 'webauthn_platform' - else - 'webauthn' - end { mfa_method_counts: mfa_user.enabled_two_factor_configuration_counts_hash, enabled_mfa_methods_count: mfa_user.enabled_mfa_methods_count, - multi_factor_auth_method: auth_method, + multi_factor_auth_method:, pii_like_keypaths: [[:mfa_method_counts, :phone]], authenticator_data_flags: authenticator_data_flags, unknown_transports: invalid_transports.presence, aaguid: aaguid, + transports: transports, + transports_mismatch: transports_mismatch?, } end end diff --git a/app/models/webauthn_configuration.rb b/app/models/webauthn_configuration.rb index 5973745835f..7d0acdb2a3e 100644 --- a/app/models/webauthn_configuration.rb +++ b/app/models/webauthn_configuration.rb @@ -7,6 +7,12 @@ class WebauthnConfiguration < ApplicationRecord validates :credential_public_key, presence: true validate :valid_transports + # https://w3c.github.io/webauthn/#enum-transport + PLATFORM_AUTHENTICATOR_TRANSPORTS = %w[ + hybrid + internal + ].to_set.freeze + # https://w3c.github.io/webauthn/#enum-transport VALID_TRANSPORTS = %w[ usb diff --git a/app/services/analytics_events.rb b/app/services/analytics_events.rb index 8b84d05100a..b52f97caf0c 100644 --- a/app/services/analytics_events.rb +++ b/app/services/analytics_events.rb @@ -5834,6 +5834,10 @@ def multi_factor_auth_phone_setup( # @param [String, nil] aaguid AAGUID value of WebAuthn device # @param [String[], nil] unknown_transports Array of unrecognized WebAuthn transports, intended to # be used in case of future specification changes. + # @param [String[], nil] transports WebAuthn transports associated with registration. + # @param [Boolean, nil] transports_mismatch Whether the WebAuthn transports associated with + # registration contradict the authenticator attachment for user setup. For example, a user can + # set up a platform authenticator through the Security Key setup flow. # @param [:authentication, :account_creation, nil] webauthn_platform_recommended A/B test for # recommended Face or Touch Unlock setup, if applicable. def multi_factor_auth_setup( @@ -5859,6 +5863,8 @@ def multi_factor_auth_setup( attempts: nil, aaguid: nil, unknown_transports: nil, + transports: nil, + transports_mismatch: nil, webauthn_platform_recommended: nil, **extra ) @@ -5886,6 +5892,8 @@ def multi_factor_auth_setup( attempts:, aaguid:, unknown_transports:, + transports:, + transports_mismatch:, webauthn_platform_recommended:, **extra, ) @@ -7753,9 +7761,12 @@ def webauthn_setup_mismatch_visited( ) end - # @param [Boolean] platform_authenticator - # @param [Boolean] success - # @param [Hash, nil] errors + # @param [Boolean] platform_authenticator Whether submission is for setting up a platform + # authenticator. This aligns to what the user experienced in setting up the authenticator. + # However, if `transports_mismatch` is true, the authentication method is created as the + # opposite of this value. + # @param [Boolean] success Whether the submission was successful + # @param [Hash, nil] errors Errors resulting from form validation, or nil if successful. # @param [Boolean] in_account_creation_flow Whether user is going through account creation flow # Tracks whether or not Webauthn setup was successful def webauthn_setup_submitted( @@ -7767,10 +7778,10 @@ def webauthn_setup_submitted( ) track_event( :webauthn_setup_submitted, - platform_authenticator: platform_authenticator, - success: success, + platform_authenticator:, + success:, + errors:, in_account_creation_flow:, - errors: errors, **extra, ) end diff --git a/spec/controllers/users/webauthn_setup_controller_spec.rb b/spec/controllers/users/webauthn_setup_controller_spec.rb index 7f9886240bc..afa5fc4a37d 100644 --- a/spec/controllers/users/webauthn_setup_controller_spec.rb +++ b/spec/controllers/users/webauthn_setup_controller_spec.rb @@ -34,7 +34,7 @@ end end - describe 'when signed in and not account creation' do + context 'when signed in and not account creation' do let(:user) { create(:user, :fully_registered, :with_authentication_app) } before do @@ -42,7 +42,7 @@ stub_sign_in(user) end - describe 'GET new' do + describe '#new' do it 'tracks page visit' do stub_sign_in stub_analytics @@ -77,7 +77,9 @@ end end - describe 'patch confirm' do + describe '#confirm' do + subject(:response) { patch :confirm, params: params } + let(:params) do { attestation_object: attestation_object, @@ -103,6 +105,16 @@ controller.user_session[:webauthn_challenge] = webauthn_challenge end + it 'should flash a success message after successfully creating' do + response + + expect(flash[:success]).to eq(t('notices.webauthn_configured')) + end + + it 'redirects to next setup path' do + expect(response).to redirect_to(account_url) + end + it 'tracks the submission' do Funnel::Registration::AddMfa.call(user.id, 'phone', @analytics, threatmetrix_attrs) @@ -130,6 +142,8 @@ ed: false, }, attempts: 1, + transports: ['usb'], + transports_mismatch: false, ) expect(@analytics).to have_logged_event( :webauthn_setup_submitted, @@ -139,6 +153,74 @@ ) end + context 'with transports mismatch' do + let(:params) { super().merge(transports: 'internal') } + + it 'handles as successful setup for platform authenticator' do + expect(controller).to receive(:handle_valid_verification_for_confirmation_context).with( + auth_method: TwoFactorAuthenticatable::AuthMethod::WEBAUTHN_PLATFORM, + ) + + response + end + + it 'does not flash success message' do + response + + expect(flash[:success]).to be_nil + end + + it 'redirects to mismatch confirmation' do + expect(response).to redirect_to(webauthn_setup_mismatch_url) + end + + it 'sets session value for mismatched configuration id' do + response + + expect(controller.user_session[:webauthn_mismatch_id]) + .to eq(user.webauthn_configurations.last.id) + end + end + + context 'with platform authenticator set up' do + let(:params) { super().merge(platform_authenticator: true, transports: 'internal') } + + it 'should flash a success message after successfully creating' do + response + + expect(flash[:success]).to eq(t('notices.webauthn_platform_configured')) + end + + context 'with transports mismatch' do + let(:params) { super().merge(transports: 'usb') } + + it 'handles as successful setup for cross-platform authenticator' do + expect(controller).to receive(:handle_valid_verification_for_confirmation_context).with( + auth_method: TwoFactorAuthenticatable::AuthMethod::WEBAUTHN, + ) + + response + end + + it 'does not flash success message' do + response + + expect(flash[:success]).to be_nil + end + + it 'redirects to mismatch confirmation' do + expect(response).to redirect_to(webauthn_setup_mismatch_url) + end + + it 'sets session value for mismatched configuration id' do + response + + expect(controller.user_session[:webauthn_mismatch_id]) + .to eq(user.webauthn_configurations.last.id) + end + end + end + context 'with setup from sms recommendation' do before do controller.user_session[:webauthn_platform_recommended] = :authentication @@ -283,6 +365,8 @@ multi_factor_auth_method: 'webauthn', success: true, attempts: 1, + transports: ['usb'], + transports_mismatch: false, ) expect(@analytics).to have_logged_event( :webauthn_setup_submitted, @@ -344,6 +428,8 @@ multi_factor_auth_method: 'webauthn_platform', success: true, attempts: 1, + transports: ['internal', 'hybrid'], + transports_mismatch: false, ) end @@ -380,18 +466,18 @@ expect(@analytics).to have_logged_event( 'Multi-Factor Authentication Setup', - { - enabled_mfa_methods_count: 0, - errors: { - attestation_object: [I18n.t('errors.webauthn_platform_setup.general_error')], - }, - error_details: { attestation_object: { invalid: true } }, - in_account_creation_flow: false, - mfa_method_counts: {}, - multi_factor_auth_method: 'webauthn_platform', - success: false, - attempts: 1, + enabled_mfa_methods_count: 0, + errors: { + attestation_object: [I18n.t('errors.webauthn_platform_setup.general_error')], }, + error_details: { attestation_object: { invalid: true } }, + in_account_creation_flow: false, + mfa_method_counts: {}, + multi_factor_auth_method: 'webauthn_platform', + success: false, + attempts: 1, + transports: ['internal', 'hybrid'], + transports_mismatch: false, ) end end @@ -448,6 +534,8 @@ ed: false, }, attempts: 2, + transports: ['usb'], + transports_mismatch: false, ) expect(@analytics).to have_logged_event( :webauthn_setup_submitted, diff --git a/spec/forms/webauthn_setup_form_spec.rb b/spec/forms/webauthn_setup_form_spec.rb index 21b02f6a56c..1e78a2a96bd 100644 --- a/spec/forms/webauthn_setup_form_spec.rb +++ b/spec/forms/webauthn_setup_form_spec.rb @@ -19,13 +19,31 @@ protocol:, } end - let(:subject) { WebauthnSetupForm.new(user:, user_session:, device_name:) } + subject(:form) { WebauthnSetupForm.new(user:, user_session:, device_name:) } before do allow(IdentityConfig.store).to receive(:domain_name).and_return(domain_name) end + describe '#webauthn_configuration' do + subject(:webauthn_configuration) { form.webauthn_configuration } + + it { is_expected.to be_nil } + + context 'after successful submission' do + before do + form.submit(params) + end + + it 'returns the created configuration' do + expect(webauthn_configuration).to eq(user.reload.webauthn_configurations.take) + end + end + end + describe '#submit' do + subject(:result) { form.submit(params) } + context 'when the input is valid' do context 'security key' do it 'returns FormResponse with success: true and creates a webauthn configuration' do @@ -46,9 +64,11 @@ pii_like_keypaths: [[:mfa_method_counts, :phone]], } - expect(subject.submit(params).to_h).to eq( + expect(result.to_h).to eq( success: true, errors: nil, + transports: ['usb'], + transports_mismatch: false, **extra_attributes, ) @@ -63,11 +83,10 @@ expect(PushNotification::HttpPush).to receive(:deliver) .with(PushNotification::RecoveryInformationChangedEvent.new(user: user)) - subject.submit(params) + result end it 'does not contains uuid' do - result = subject.submit(params) expect(result.extra[:aaguid]).to eq nil end end @@ -79,7 +98,6 @@ end it 'creates a platform authenticator' do - result = subject.submit(params) expect(result.extra[:multi_factor_auth_method]).to eq 'webauthn_platform' user.reload @@ -94,8 +112,6 @@ let(:params) { super().merge(authenticator_data_value: '65') } it 'includes data flags with bs set as false ' do - result = subject.submit(params) - expect(result.to_h[:authenticator_data_flags]).to eq( up: true, uv: false, @@ -111,8 +127,6 @@ let(:params) { super().merge(authenticator_data_value: 'bad_error') } it 'should not include authenticator data flag' do - result = subject.submit(params) - expect(result.to_h[:authenticator_data_flags]).to be_nil end end @@ -121,14 +135,11 @@ let(:params) { super().merge(authenticator_data_value: nil) } it 'should not include authenticator data flag' do - result = subject.submit(params) - expect(result.to_h[:authenticator_data_flags]).to be_nil end end it 'contains uuid' do - result = subject.submit(params) expect(result.extra[:aaguid]).to eq aaguid end end @@ -137,7 +148,7 @@ let(:params) { super().merge(transports: 'wrong') } it 'creates a webauthn configuration without transports' do - subject.submit(params) + result user.reload @@ -145,8 +156,6 @@ end it 'includes unknown transports in extra analytics' do - result = subject.submit(params) - expect(result.to_h).to eq( success: true, errors: nil, @@ -164,6 +173,8 @@ pii_like_keypaths: [[:mfa_method_counts, :phone]], unknown_transports: ['wrong'], aaguid: nil, + transports: [], + transports_mismatch: false, ) end end @@ -190,7 +201,7 @@ pii_like_keypaths: [[:mfa_method_counts, :phone]], } - expect(subject.submit(params).to_h).to eq( + expect(result.to_h).to eq( success: false, errors: { attestation_object: [ @@ -201,6 +212,8 @@ ], }, error_details: { attestation_object: { invalid: true } }, + transports: ['usb'], + transports_mismatch: false, **extra_attributes, ) end @@ -210,7 +223,7 @@ let(:params) { super().except(:transports) } it 'creates a webauthn configuration without transports' do - subject.submit(params) + result user.reload @@ -242,7 +255,7 @@ pii_like_keypaths: [[:mfa_method_counts, :phone]], } - expect(subject.submit(params).to_h).to eq( + expect(result.to_h).to eq( success: false, errors: { attestation_object: [ @@ -253,11 +266,129 @@ ], }, error_details: { attestation_object: { invalid: true } }, + transports: ['usb'], + transports_mismatch: false, **extra_attributes, ) end end + + context 'with transports mismatch' do + let(:params) { super().merge(transports: 'internal') } + + it 'returns setup as mismatched type' do + expect(result.to_h).to eq( + success: true, + errors: nil, + enabled_mfa_methods_count: 1, + mfa_method_counts: { webauthn_platform: 1 }, + multi_factor_auth_method: 'webauthn_platform', + authenticator_data_flags: { + up: true, + uv: false, + be: true, + bs: true, + at: false, + ed: true, + }, + unknown_transports: nil, + aaguid: nil, + transports: ['internal'], + transports_mismatch: true, + pii_like_keypaths: [[:mfa_method_counts, :phone]], + ) + end + end + end + + describe '#setup_as_platform_authenticator?' do + subject(:setup_as_platform_authenticator?) { form.setup_as_platform_authenticator? } + + it { is_expected.to eq(false) } + + context 'after successful submission' do + before do + form.submit(params) + end + + it { is_expected.to eq(false) } + + context 'without transports' do + let(:params) { super().merge(transports: nil) } + + it { is_expected.to eq(false) } + end + + context 'with platform authenticator transports' do + let(:params) { super().merge(transports: 'internal') } + + it { is_expected.to eq(true) } + end + + context 'when setup as platform authenticator' do + let(:params) { super().merge(platform_authenticator: true, transports: 'internal') } + + it { is_expected.to eq(true) } + + context 'without transports' do + let(:params) { super().merge(transports: nil) } + + it { is_expected.to eq(true) } + end + + context 'without platform authenticator transports' do + let(:params) { super().merge(transports: 'usb') } + + it { is_expected.to eq(false) } + end + end + end end + + describe '#transports_mismatch?' do + subject(:transports_mismatch?) { form.transports_mismatch? } + + it { is_expected.to eq(false) } + + context 'after successful submission' do + before do + form.submit(params) + end + + it { is_expected.to eq(false) } + + context 'without transports' do + let(:params) { super().merge(transports: nil) } + + it { is_expected.to eq(false) } + end + + context 'with platform authenticator transports' do + let(:params) { super().merge(transports: 'internal') } + + it { is_expected.to eq(true) } + end + + context 'when setup as platform authenticator' do + let(:params) { super().merge(platform_authenticator: true, transports: 'internal') } + + it { is_expected.to eq(false) } + + context 'without transports' do + let(:params) { super().merge(transports: nil) } + + it { is_expected.to eq(false) } + end + + context 'without platform authenticator transports' do + let(:params) { super().merge(transports: 'usb') } + + it { is_expected.to eq(true) } + end + end + end + end + describe '.name_is_unique' do context 'webauthn' do let(:user) do @@ -266,7 +397,7 @@ user end it 'checks for unique device on a webauthn device' do - result = subject.submit(params) + result = form.submit(params) expect(result.extra[:multi_factor_auth_method]).to eq 'webauthn' expect(result.errors[:name]).to eq( [I18n.t( @@ -295,7 +426,7 @@ end it 'adds a new platform device with the same existing name and appends a (1)' do - result = subject.submit(params) + result = form.submit(params) expect(result.extra[:multi_factor_auth_method]).to eq 'webauthn_platform' expect(user.webauthn_configurations.platform_authenticators.count).to eq(2) expect( @@ -321,7 +452,7 @@ end it 'adds a second new platform device with the same existing name and appends a (2)' do - result = subject.submit(params) + result = form.submit(params) expect(result.success?).to eq(true) expect(user.webauthn_configurations.platform_authenticators.count).to eq(3) diff --git a/spec/models/webauthn_configuration_spec.rb b/spec/models/webauthn_configuration_spec.rb index e1c84e1cad4..33d8cfcc037 100644 --- a/spec/models/webauthn_configuration_spec.rb +++ b/spec/models/webauthn_configuration_spec.rb @@ -10,6 +10,24 @@ let(:subject) { create(:webauthn_configuration) } + describe '.PLATFORM_AUTHENTICATOR_TRANSPORTS' do + subject(:transports) { WebauthnConfiguration::PLATFORM_AUTHENTICATOR_TRANSPORTS } + + it 'is a frozen array of strings' do + expect(transports).to all be_a(String) + expect(transports.frozen?).to eq(true) + end + end + + describe '.VALID_TRANSPORTS' do + subject(:transports) { WebauthnConfiguration::VALID_TRANSPORTS } + + it 'is a frozen array of strings' do + expect(transports).to all be_a(String) + expect(transports.frozen?).to eq(true) + end + end + describe '#selection_presenters' do context 'for a roaming authenticator' do it 'returns a WebauthnSelectionPresenter in an array' do