From df1c686209537e04d6037074875dfdfccfe99456 Mon Sep 17 00:00:00 2001 From: Malick Diarra Date: Mon, 5 Aug 2024 09:29:01 -0400 Subject: [PATCH 1/6] changelog: User-Facing Improvements, Authentication, Auth setup 2nd mfa lista all options regardless of SP --- app/controllers/concerns/mfa_setup_concern.rb | 3 +++ .../two_factor_authentication_setup_controller.rb | 14 ++++++++++++-- 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/app/controllers/concerns/mfa_setup_concern.rb b/app/controllers/concerns/mfa_setup_concern.rb index e999c4c1376..5a42ce8dfcb 100644 --- a/app/controllers/concerns/mfa_setup_concern.rb +++ b/app/controllers/concerns/mfa_setup_concern.rb @@ -75,6 +75,9 @@ def mfa_selection_index def set_mfa_selections(selections) user_session[:mfa_selections] = selections end + def user_already_has_mfa? + mfa_context.enabled_mfa_methods_count > 0 + end def show_skip_additional_mfa_link? !(mfa_context.enabled_mfa_methods_count == 1 && diff --git a/app/controllers/users/two_factor_authentication_setup_controller.rb b/app/controllers/users/two_factor_authentication_setup_controller.rb index 7804b188d15..6920a0a9925 100644 --- a/app/controllers/users/two_factor_authentication_setup_controller.rb +++ b/app/controllers/users/two_factor_authentication_setup_controller.rb @@ -60,14 +60,24 @@ def two_factor_options_presenter TwoFactorOptionsPresenter.new( user_agent: request.user_agent, user: current_user, - phishing_resistant_required: service_provider_mfa_policy.phishing_resistant_required?, - piv_cac_required: service_provider_mfa_policy.piv_cac_required?, + phishing_resistant_required: phishing_resistant?, + piv_cac_required: piv_cac_required?, show_skip_additional_mfa_link: show_skip_additional_mfa_link?, after_mfa_setup_path:, return_to_sp_cancel_path:, ) end + def phishing_resistant? + return false if user_already_has_mfa? + service_provider_mfa_policy.phishing_resistant_required? + end + + def piv_cac_required? + return false if user_already_has_mfa? + service_provider_mfa_policy.piv_cac_required? + end + def process_valid_form user_session[:mfa_selections] = @two_factor_options_form.selection redirect_to(first_mfa_selection_path || after_mfa_setup_path) From 936855c2c67caf487bc687ab5ee475fc270f0c70 Mon Sep 17 00:00:00 2001 From: Malick Diarra Date: Mon, 5 Aug 2024 13:23:42 -0400 Subject: [PATCH 2/6] add multiple mfa sign up spec --- app/controllers/concerns/mfa_setup_concern.rb | 5 +- .../multiple_mfa_sign_up_spec.rb | 52 +++++++++++++++++++ 2 files changed, 55 insertions(+), 2 deletions(-) diff --git a/app/controllers/concerns/mfa_setup_concern.rb b/app/controllers/concerns/mfa_setup_concern.rb index 5a42ce8dfcb..8ada8061b43 100644 --- a/app/controllers/concerns/mfa_setup_concern.rb +++ b/app/controllers/concerns/mfa_setup_concern.rb @@ -75,8 +75,9 @@ def mfa_selection_index def set_mfa_selections(selections) user_session[:mfa_selections] = selections end - def user_already_has_mfa? - mfa_context.enabled_mfa_methods_count > 0 + + def user_already_has_mfa? + mfa_context.enabled_mfa_methods_count > 0 end def show_skip_additional_mfa_link? 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..2a1a388d34f 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,57 @@ end end + context 'User with phishing resistant service provider' do + it 'should only show phishing resistant options for first 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')) + end + + it 'should show 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 From 04ee3d6c3d042eeff82fda1944213bac6b83c989 Mon Sep 17 00:00:00 2001 From: Malick Diarra Date: Mon, 5 Aug 2024 15:31:34 -0400 Subject: [PATCH 3/6] take into accounts users who already signed in --- app/controllers/concerns/mfa_setup_concern.rb | 4 --- ..._factor_authentication_setup_controller.rb | 8 ++--- .../multiple_mfa_sign_up_spec.rb | 34 +++++-------------- 3 files changed, 13 insertions(+), 33 deletions(-) diff --git a/app/controllers/concerns/mfa_setup_concern.rb b/app/controllers/concerns/mfa_setup_concern.rb index 8ada8061b43..e999c4c1376 100644 --- a/app/controllers/concerns/mfa_setup_concern.rb +++ b/app/controllers/concerns/mfa_setup_concern.rb @@ -76,10 +76,6 @@ def set_mfa_selections(selections) user_session[:mfa_selections] = selections end - def user_already_has_mfa? - mfa_context.enabled_mfa_methods_count > 0 - end - def show_skip_additional_mfa_link? !(mfa_context.enabled_mfa_methods_count == 1 && mfa_context.webauthn_platform_configurations.count == 1) diff --git a/app/controllers/users/two_factor_authentication_setup_controller.rb b/app/controllers/users/two_factor_authentication_setup_controller.rb index 6920a0a9925..be3367b3b1c 100644 --- a/app/controllers/users/two_factor_authentication_setup_controller.rb +++ b/app/controllers/users/two_factor_authentication_setup_controller.rb @@ -69,13 +69,13 @@ def two_factor_options_presenter end def phishing_resistant? - return false if user_already_has_mfa? - service_provider_mfa_policy.phishing_resistant_required? + service_provider_mfa_policy.phishing_resistant_required? && + !mfa_context.phishing_resistant_configurations.present? end def piv_cac_required? - return false if user_already_has_mfa? - service_provider_mfa_policy.piv_cac_required? + service_provider_mfa_policy.piv_cac_required? && + !mfa_context.piv_cac_configurations.present? end def process_valid_form 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 2a1a388d34f..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 @@ -293,23 +293,7 @@ end context 'User with phishing resistant service provider' do - it 'should only show phishing resistant options for first 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')) - end - - it 'should show all mfa options for second mfa' 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) @@ -332,14 +316,14 @@ 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')) + 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 From 4391b56dd6c4c2f2a3328d9bebf1af9b347e124e Mon Sep 17 00:00:00 2001 From: Malick Diarra Date: Mon, 5 Aug 2024 16:19:22 -0400 Subject: [PATCH 4/6] rubocop --- .../users/two_factor_authentication_setup_controller.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/controllers/users/two_factor_authentication_setup_controller.rb b/app/controllers/users/two_factor_authentication_setup_controller.rb index be3367b3b1c..a3e7613e0bf 100644 --- a/app/controllers/users/two_factor_authentication_setup_controller.rb +++ b/app/controllers/users/two_factor_authentication_setup_controller.rb @@ -69,12 +69,12 @@ def two_factor_options_presenter end def phishing_resistant? - service_provider_mfa_policy.phishing_resistant_required? && + service_provider_mfa_policy.phishing_resistant_required? && !mfa_context.phishing_resistant_configurations.present? end def piv_cac_required? - service_provider_mfa_policy.piv_cac_required? && + service_provider_mfa_policy.piv_cac_required? && !mfa_context.piv_cac_configurations.present? end From 6a8dafb74405127bd7daa6ac5bce590395c69c95 Mon Sep 17 00:00:00 2001 From: Malick Diarra Date: Tue, 6 Aug 2024 12:22:14 -0400 Subject: [PATCH 5/6] refactor to put in options presenter --- ..._factor_authentication_setup_controller.rb | 14 +----- app/policies/mfa_policy.rb | 4 ++ .../two_factor_options_presenter.rb | 13 +++-- .../two_factor_options_presenter_spec.rb | 50 +++++++++++++++++++ 4 files changed, 66 insertions(+), 15 deletions(-) diff --git a/app/controllers/users/two_factor_authentication_setup_controller.rb b/app/controllers/users/two_factor_authentication_setup_controller.rb index a3e7613e0bf..7804b188d15 100644 --- a/app/controllers/users/two_factor_authentication_setup_controller.rb +++ b/app/controllers/users/two_factor_authentication_setup_controller.rb @@ -60,24 +60,14 @@ def two_factor_options_presenter TwoFactorOptionsPresenter.new( user_agent: request.user_agent, user: current_user, - phishing_resistant_required: phishing_resistant?, - piv_cac_required: piv_cac_required?, + phishing_resistant_required: service_provider_mfa_policy.phishing_resistant_required?, + piv_cac_required: service_provider_mfa_policy.piv_cac_required?, show_skip_additional_mfa_link: show_skip_additional_mfa_link?, after_mfa_setup_path:, return_to_sp_cancel_path:, ) end - def phishing_resistant? - service_provider_mfa_policy.phishing_resistant_required? && - !mfa_context.phishing_resistant_configurations.present? - end - - def piv_cac_required? - service_provider_mfa_policy.piv_cac_required? && - !mfa_context.piv_cac_configurations.present? - end - def process_valid_form user_session[:mfa_selections] = @two_factor_options_form.selection redirect_to(first_mfa_selection_path || after_mfa_setup_path) 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/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) From 7e0eeae60cb7cde908abdc7fa681b67a9ee92fd1 Mon Sep 17 00:00:00 2001 From: Malick Diarra Date: Tue, 6 Aug 2024 13:40:11 -0400 Subject: [PATCH 6/6] add check for piv mfa policy --- spec/policies/user_mfa_policy_spec.rb | 14 ++++++++++++++ 1 file changed, 14 insertions(+) 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) }