diff --git a/app/policies/mfa_policy.rb b/app/policies/mfa_policy.rb index 6c217e3bb07..41440f3c699 100644 --- a/app/policies/mfa_policy.rb +++ b/app/policies/mfa_policy.rb @@ -15,6 +15,10 @@ def phishing_resistant_mfa_enabled? mfa_user.webauthn_configurations.present? end + def piv_cac_mfa_enabled? + mfa_user.piv_cac_configurations.present? + end + def multiple_factors_enabled? mfa_user.enabled_mfa_methods_count > 1 end diff --git a/app/presenters/two_factor_options_presenter.rb b/app/presenters/two_factor_options_presenter.rb index 21f7200b8da..6c97445e661 100644 --- a/app/presenters/two_factor_options_presenter.rb +++ b/app/presenters/two_factor_options_presenter.rb @@ -43,7 +43,12 @@ def all_options_sorted TwoFactorAuthentication::SetUpPivCacSelectionPresenter, TwoFactorAuthentication::SetUpBackupCodeSelectionPresenter, ].map do |klass| - klass.new(user:, piv_cac_required:, phishing_resistant_required:, user_agent:) + klass.new( + user:, + piv_cac_required: piv_cac_required?, + phishing_resistant_required: phishing_resistant_only?, + user_agent:, + ) end. partition(&:recommended?). flatten @@ -106,11 +111,13 @@ def skip_label private def piv_cac_required? - @piv_cac_required + @piv_cac_required && + !mfa_policy.piv_cac_mfa_enabled? end def phishing_resistant_only? - @phishing_resistant_required && !mfa_policy.phishing_resistant_mfa_enabled? + @phishing_resistant_required && + !mfa_policy.phishing_resistant_mfa_enabled? end def mfa_policy diff --git a/spec/features/two_factor_authentication/multiple_mfa_sign_up_spec.rb b/spec/features/two_factor_authentication/multiple_mfa_sign_up_spec.rb index 189baf01f77..6394a7a4c3f 100644 --- a/spec/features/two_factor_authentication/multiple_mfa_sign_up_spec.rb +++ b/spec/features/two_factor_authentication/multiple_mfa_sign_up_spec.rb @@ -2,6 +2,7 @@ RSpec.feature 'Multi Two Factor Authentication', allowed_extra_analytics: [:*] do include WebAuthnHelper + include OidcAuthHelper describe 'When the user has not set up 2FA' do let(:fake_analytics) { FakeAnalytics.new } @@ -291,6 +292,41 @@ end end + context 'User with phishing resistant service provider' do + it 'should show phishing option first then all mfa options for second mfa' do + allow(IdentityConfig.store). + to receive(:show_unsupported_passkey_platform_authentication_setup). + and_return(true) + + visit_idp_from_ial1_oidc_sp_requesting_phishing_resistant(prompt: 'select_account') + sign_up_and_set_password + mock_webauthn_setup_challenge + expect(page). + to have_content(t('two_factor_authentication.two_factor_choice_options.webauthn')) + expect(page). + to have_content(t('two_factor_authentication.two_factor_choice_options.piv_cac')) + expect(page). + to_not have_content(t('two_factor_authentication.two_factor_choice_options.auth_app')) + expect(page). + to_not have_content(t('two_factor_authentication.two_factor_choice_options.phone')) + select_2fa_option('webauthn_platform', visible: :all) + + click_continue + + mock_press_button_on_hardware_key_on_setup + + click_link t('mfa.add') + expect(page). + to have_content(t('two_factor_authentication.two_factor_choice_options.webauthn')) + expect(page). + to have_content(t('two_factor_authentication.two_factor_choice_options.piv_cac')) + expect(page). + to have_content(t('two_factor_authentication.two_factor_choice_options.auth_app')) + expect(page). + to have_content(t('two_factor_authentication.two_factor_choice_options.phone')) + end + end + def click_2fa_option(option) find("label[for='two_factor_options_form_selection_#{option}']").click end diff --git a/spec/policies/user_mfa_policy_spec.rb b/spec/policies/user_mfa_policy_spec.rb index ce4951a52da..784877283e0 100644 --- a/spec/policies/user_mfa_policy_spec.rb +++ b/spec/policies/user_mfa_policy_spec.rb @@ -24,6 +24,20 @@ it { expect(subject.multiple_factors_enabled?).to eq true } end + describe '#piv_cac_mfa_enabled?' do + context 'with piv configuration' do + let(:user) { create(:user, :with_piv_or_cac) } + + it { expect(subject.piv_cac_mfa_enabled?).to eq true } + end + + context 'with non PIV MFA option' do + let(:user) { create(:user, :fully_registered) } + + it { expect(subject.piv_cac_mfa_enabled?).to eq false } + end + end + describe '#unphishable?' do context 'with unphishable configuration' do let(:user) { create(:user, :with_piv_or_cac, :with_webauthn) } diff --git a/spec/presenters/two_factor_options_presenter_spec.rb b/spec/presenters/two_factor_options_presenter_spec.rb index bde99540686..5816a8da51d 100644 --- a/spec/presenters/two_factor_options_presenter_spec.rb +++ b/spec/presenters/two_factor_options_presenter_spec.rb @@ -51,6 +51,56 @@ end end + context 'when a phishing-resistant SP but already has phishing-resistant mfa' do + let(:user) do + create( + :user, :fully_registered, :with_webauthn + ) + end + let(:presenter) do + described_class.new( + user_agent: user_agent, user: user, + phishing_resistant_required: true + ) + end + + it 'displays all options' do + expect(presenter.options.map(&:class)).to eq [ + TwoFactorAuthentication::SetUpWebauthnPlatformSelectionPresenter, + TwoFactorAuthentication::SetUpAuthAppSelectionPresenter, + TwoFactorAuthentication::SetUpPhoneSelectionPresenter, + TwoFactorAuthentication::SetUpWebauthnSelectionPresenter, + TwoFactorAuthentication::SetUpPivCacSelectionPresenter, + TwoFactorAuthentication::SetUpBackupCodeSelectionPresenter, + ] + end + end + + context 'with a PIV only SP but already has PIV mfa' do + let(:user) do + create( + :user, :fully_registered, :with_piv_or_cac + ) + end + let(:presenter) do + described_class.new( + user_agent: user_agent, user: user, + piv_cac_required: true + ) + end + + it 'displays all options' do + expect(presenter.options.map(&:class)).to eq [ + TwoFactorAuthentication::SetUpWebauthnPlatformSelectionPresenter, + TwoFactorAuthentication::SetUpAuthAppSelectionPresenter, + TwoFactorAuthentication::SetUpPhoneSelectionPresenter, + TwoFactorAuthentication::SetUpWebauthnSelectionPresenter, + TwoFactorAuthentication::SetUpPivCacSelectionPresenter, + TwoFactorAuthentication::SetUpBackupCodeSelectionPresenter, + ] + end + end + context 'when hide_phone_mfa_signup is enabled' do before do allow(IdentityConfig.store).to receive(:hide_phone_mfa_signup).and_return(true)