diff --git a/app/controllers/users/mfa_selection_controller.rb b/app/controllers/users/mfa_selection_controller.rb index 03cb90b25f7..0fc7013d2e5 100644 --- a/app/controllers/users/mfa_selection_controller.rb +++ b/app/controllers/users/mfa_selection_controller.rb @@ -22,12 +22,9 @@ def update if result.success? process_valid_form else - flash[:error] = t('errors.two_factor_auth_setup.must_select_additional_option') - redirect_back(fallback_location: second_mfa_setup_path, allow_other_host: false) + flash[:error] = result.first_error_message + redirect_to second_mfa_setup_path end - rescue ActionController::ParameterMissing - flash[:error] = t('errors.two_factor_auth_setup.must_select_option') - redirect_back(fallback_location: two_factor_options_path, allow_other_host: false) end # @api private @@ -67,6 +64,8 @@ def process_valid_form def two_factor_options_form_params params.require(:two_factor_options_form).permit(:selection, selection: []) + rescue ActionController::ParameterMissing + ActionController::Parameters.new(selection: []) end end end diff --git a/app/controllers/users/two_factor_authentication_setup_controller.rb b/app/controllers/users/two_factor_authentication_setup_controller.rb index db1dd43a107..2f5fd7bab98 100644 --- a/app/controllers/users/two_factor_authentication_setup_controller.rb +++ b/app/controllers/users/two_factor_authentication_setup_controller.rb @@ -25,13 +25,10 @@ def create if result.success? process_valid_form else - flash[:error] = t('errors.two_factor_auth_setup.must_select_option') + flash.now[:error] = result.first_error_message @presenter = two_factor_options_presenter render :index end - rescue ActionController::ParameterMissing - flash[:error] = t('errors.two_factor_auth_setup.must_select_option') - redirect_back(fallback_location: authentication_methods_setup_path, allow_other_host: false) end # @api private @@ -71,6 +68,8 @@ def confirm_user_needs_2fa_setup def two_factor_options_form_params params.require(:two_factor_options_form).permit(:selection, selection: []) + rescue ActionController::ParameterMissing + ActionController::Parameters.new(selection: []) end end end diff --git a/app/forms/two_factor_options_form.rb b/app/forms/two_factor_options_form.rb index 491c2344ade..6328293f72f 100644 --- a/app/forms/two_factor_options_form.rb +++ b/app/forms/two_factor_options_form.rb @@ -1,5 +1,6 @@ class TwoFactorOptionsForm include ActiveModel::Model + include ActionView::Helpers::TranslationHelper attr_accessor :selection, :user, :phishing_resistant_required, :piv_cac_required @@ -7,7 +8,7 @@ class TwoFactorOptionsForm webauthn webauthn_platform backup_code] } - validates :selection, length: { minimum: 1 }, if: :has_no_mfa_or_in_required_flow? + validate :validate_selection_present def initialize(user:, phishing_resistant_required:, piv_cac_required:) self.user = user @@ -16,7 +17,7 @@ def initialize(user:, phishing_resistant_required:, piv_cac_required:) end def submit(params) - self.selection = Array(params[:selection]).filter(&:present?) + self.selection = params[:selection] success = valid? update_otp_delivery_preference_for_user if success && user_needs_updating? @@ -25,6 +26,11 @@ def submit(params) private + def validate_selection_present + return if !has_no_mfa_or_in_required_flow? || selection.present? + errors.add(:selection, missing_selection_error_message, type: :missing_selection) + end + def mfa_user @mfa_user ||= MfaContext.new(user) end @@ -66,7 +72,16 @@ def platform_auth_only_option? end def has_no_mfa_or_in_required_flow? - has_no_configured_mfa? || in_phishing_resistant_or_piv_cac_required_flow? || + has_no_configured_mfa? || + in_phishing_resistant_or_piv_cac_required_flow? || platform_auth_only_option? end + + def missing_selection_error_message + if has_no_configured_mfa? || in_phishing_resistant_or_piv_cac_required_flow? + t('errors.two_factor_auth_setup.must_select_option') + elsif platform_auth_only_option? + t('errors.two_factor_auth_setup.must_select_additional_option') + end + end end diff --git a/app/views/users/mfa_selection/index.html.erb b/app/views/users/mfa_selection/index.html.erb index be32cbfd6fa..1a2bff4fdb4 100644 --- a/app/views/users/mfa_selection/index.html.erb +++ b/app/views/users/mfa_selection/index.html.erb @@ -11,7 +11,6 @@
<%= @presenter.intro %> - <%= hidden_field_tag 'two_factor_options_form[selection][]', nil %> <% @presenter.options.each do |option| %> <%= render(option) do %> <%= render partial: 'partials/multi_factor_authentication/mfa_selection', diff --git a/app/views/users/two_factor_authentication_setup/index.html.erb b/app/views/users/two_factor_authentication_setup/index.html.erb index 74c49fa5625..d44c9de7fdb 100644 --- a/app/views/users/two_factor_authentication_setup/index.html.erb +++ b/app/views/users/two_factor_authentication_setup/index.html.erb @@ -23,7 +23,6 @@ <%= t('two_factor_authentication.form_legend') %> - <%= hidden_field_tag 'two_factor_options_form[selection][]', nil %> <% @presenter.options.each do |option| %> <%= render(option) do %> <%= render partial: 'partials/multi_factor_authentication/mfa_selection', diff --git a/spec/controllers/users/mfa_selection_controller_spec.rb b/spec/controllers/users/mfa_selection_controller_spec.rb index 6c095a6583f..346977825a1 100644 --- a/spec/controllers/users/mfa_selection_controller_spec.rb +++ b/spec/controllers/users/mfa_selection_controller_spec.rb @@ -27,7 +27,7 @@ voice_params = { two_factor_options_form: { - selection: 'voice', + selection: ['voice'], }, } @@ -51,7 +51,7 @@ patch :update, params: { two_factor_options_form: { - selection: 'phone', + selection: ['phone'], }, } @@ -91,7 +91,7 @@ patch :update, params: { two_factor_options_form: { - selection: 'auth_app', + selection: ['auth_app'], }, } @@ -105,7 +105,7 @@ patch :update, params: { two_factor_options_form: { - selection: 'webauthn', + selection: ['webauthn'], }, } @@ -119,7 +119,7 @@ patch :update, params: { two_factor_options_form: { - selection: 'webauthn_platform', + selection: ['webauthn_platform'], }, } @@ -133,7 +133,7 @@ patch :update, params: { two_factor_options_form: { - selection: 'piv_cac', + selection: ['piv_cac'], }, } @@ -149,15 +149,15 @@ end context 'with no active MFA' do + let(:user) { build(:user) } + it 'redirects to the index page with a flash error' do patch :update, params: { two_factor_options_form: {}, } - expect(response).to redirect_to two_factor_options_path - expect(flash[:error]).to eq( - t('errors.two_factor_auth_setup.must_select_option'), - ) + expect(response).to redirect_to second_mfa_setup_path + expect(flash[:error]).to eq(t('errors.two_factor_auth_setup.must_select_option')) end end @@ -166,9 +166,7 @@ create(:phone_configuration, user: user) patch :update, params: { - two_factor_options_form: { - selection: [''], - }, + two_factor_options_form: {}, } expect(response).to redirect_to account_path @@ -182,7 +180,7 @@ patch :update, params: { two_factor_options_form: { - selection: 'foo', + selection: ['foo'], }, } diff --git a/spec/controllers/users/two_factor_authentication_setup_controller_spec.rb b/spec/controllers/users/two_factor_authentication_setup_controller_spec.rb index 9ea15dee085..825c14cddd7 100644 --- a/spec/controllers/users/two_factor_authentication_setup_controller_spec.rb +++ b/spec/controllers/users/two_factor_authentication_setup_controller_spec.rb @@ -61,7 +61,7 @@ voice_params = { two_factor_options_form: { - selection: 'voice', + selection: ['voice'], }, } @@ -149,7 +149,7 @@ patch :create, params: { two_factor_options_form: { - selection: 'auth_app', + selection: ['auth_app'], }, } @@ -163,7 +163,7 @@ patch :create, params: { two_factor_options_form: { - selection: 'webauthn', + selection: ['webauthn'], }, } @@ -177,7 +177,7 @@ patch :create, params: { two_factor_options_form: { - selection: 'webauthn_platform', + selection: ['webauthn_platform'], }, } @@ -191,7 +191,7 @@ patch :create, params: { two_factor_options_form: { - selection: 'piv_cac', + selection: ['piv_cac'], }, } @@ -205,7 +205,7 @@ patch :create, params: { two_factor_options_form: { - selection: 'foo', + selection: ['foo'], }, } 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 bb67f6d5a03..39e566c8d4e 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 @@ -151,71 +151,82 @@ expect(current_path).to eq account_path end - scenario 'user can select 1 MFA methods and skips selecting second mfa' do - sign_in_before_2fa + describe 'skipping second mfa' do + context 'with skippable mfa method' do + it 'allows user to skip using skip link' do + sign_in_before_2fa + click_2fa_option('backup_code') - expect(current_path).to eq authentication_methods_setup_path + click_continue + expect(current_path).to eq backup_code_setup_path - click_2fa_option('backup_code') - - click_continue - - expect(current_path).to eq backup_code_setup_path - - click_continue + click_continue + expect(page).to have_link(t('components.download_button.label')) - expect(page).to have_link(t('components.download_button.label')) + click_continue + expect(page).to have_content(t('notices.backup_codes_configured')) + expect(page).to have_current_path(auth_method_confirmation_path) - click_continue + click_link t('mfa.add') + expect(page).to have_current_path(second_mfa_setup_path) - expect(page).to have_content(t('notices.backup_codes_configured')) + click_link t('mfa.skip') + expect(page).to have_current_path(account_path) + end - expect(page).to have_current_path( - auth_method_confirmation_path, - ) + it 'allows user to skip by clicking continue without selection' do + sign_in_before_2fa + click_2fa_option('backup_code') - click_link t('mfa.add') + click_continue + expect(current_path).to eq backup_code_setup_path - expect(page).to have_current_path(second_mfa_setup_path) + click_continue + expect(page).to have_link(t('components.download_button.label')) - click_link t('mfa.skip') + click_continue + expect(page).to have_content(t('notices.backup_codes_configured')) + expect(page).to have_current_path(auth_method_confirmation_path) - expect(page).to have_current_path(account_path) - end + click_link t('mfa.add') + expect(page).to have_current_path(second_mfa_setup_path) - scenario 'user cannot select Platform Auth as only MFA and must select second mfa setup' do - allow(IdentityConfig.store).to receive(:platform_auth_set_up_enabled).and_return(true) - allow(IdentityConfig.store). - to receive(:show_unsupported_passkey_platform_authentication_setup). - and_return(true) - mock_webauthn_setup_challenge - user = sign_up_and_set_password - user.password = Features::SessionHelper::VALID_PASSWORD - expect(current_path).to eq authentication_methods_setup_path - # webauthn option is hidden in browsers that don't support it - select_2fa_option('webauthn_platform', visible: :all) - - click_continue - - expect(page).to have_current_path webauthn_setup_path(platform: true) - - fill_in_nickname_and_click_continue - mock_press_button_on_hardware_key_on_setup - - expect(page).to have_current_path( - auth_method_confirmation_path, - ) - - expect(page).to_not have_button(t('mfa.skip')) - - click_link t('mfa.add') - - expect(page).to have_current_path(second_mfa_setup_path) - - click_continue + click_continue + expect(page).to have_current_path(account_path) + end + end - expect(page).to have_current_path(second_mfa_setup_path) - expect(page).to have_content(t('errors.two_factor_auth_setup.must_select_additional_option')) + context 'with platform authenticator as the first mfa' do + it 'does not allow the user to skip selecting second mfa' do + allow(IdentityConfig.store).to receive(:platform_auth_set_up_enabled).and_return(true) + allow(IdentityConfig.store). + to receive(:show_unsupported_passkey_platform_authentication_setup). + and_return(true) + mock_webauthn_setup_challenge + user = sign_up_and_set_password + user.password = Features::SessionHelper::VALID_PASSWORD + expect(current_path).to eq authentication_methods_setup_path + # webauthn option is hidden in browsers that don't support it + select_2fa_option('webauthn_platform', visible: :all) + + click_continue + expect(page).to have_current_path webauthn_setup_path(platform: true) + + fill_in_nickname_and_click_continue + mock_press_button_on_hardware_key_on_setup + expect(page).to have_current_path(auth_method_confirmation_path) + expect(page).to_not have_button(t('mfa.skip')) + + click_link t('mfa.add') + expect(page).to have_current_path(second_mfa_setup_path) + + click_continue + expect(page).to have_current_path(second_mfa_setup_path) + expect(page).to have_content( + t('errors.two_factor_auth_setup.must_select_additional_option'), + ) + end + end end end diff --git a/spec/features/users/sign_up_spec.rb b/spec/features/users/sign_up_spec.rb index df7f6be12f5..dec3ac13da7 100644 --- a/spec/features/users/sign_up_spec.rb +++ b/spec/features/users/sign_up_spec.rb @@ -382,10 +382,13 @@ def clipboard_text it 'redirects back with an error if the user does not select 2FA option' do sign_in_user - visit authentication_methods_setup_path - click_on 'Continue' + click_continue expect(page).to have_content(t('errors.two_factor_auth_setup.must_select_option')) + + select_2fa_option('phone') + expect(page).to have_current_path(phone_setup_path) + expect(page).not_to have_content(t('errors.two_factor_auth_setup.must_select_option')) end it 'does not show the remember device option as the default when the SP is AAL2' do diff --git a/spec/forms/two_factor_options_form_spec.rb b/spec/forms/two_factor_options_form_spec.rb index 5fb12016609..237a6b67870 100644 --- a/spec/forms/two_factor_options_form_spec.rb +++ b/spec/forms/two_factor_options_form_spec.rb @@ -13,13 +13,13 @@ end describe '#submit' do - let(:submit_phone) { subject.submit(selection: 'phone') } + let(:submit_phone) { subject.submit(selection: ['phone']) } let(:enabled_mfa_methods_count) { 0 } let(:mfa_selection) { ['sms'] } it 'is successful if the selection is valid' do %w[auth_app piv_cac webauthn webauthn_platform].each do |selection| - result = subject.submit(selection: selection) + result = subject.submit(selection: [selection]) expect(result.success?).to eq true end @@ -27,7 +27,7 @@ it 'is unsuccessful if the selection is invalid' do %w[!!!!].each do |selection| - result = subject.submit(selection: selection) + result = subject.submit(selection: [selection]) expect(result.success?).to eq false expect(result.errors).to include :selection @@ -56,7 +56,7 @@ end it 'includes analytics hash with a methods count of zero' do - result = subject.submit(selection: 'piv_cac') + result = subject.submit(selection: ['piv_cac']) expect(result.success?).to eq(true) expect(result.to_h).to include(enabled_mfa_methods_count: 0) @@ -82,7 +82,7 @@ it "does not update the user's otp_delivery_preference" do expect(UpdateUser).to_not receive(:new) - subject.submit(selection: 'sms') + subject.submit(selection: ['sms']) end end @@ -90,7 +90,7 @@ it "does not update the user's otp_delivery_preference" do expect(UpdateUser).to_not receive(:new) - subject.submit(selection: 'auth_app') + subject.submit(selection: ['auth_app']) end end @@ -117,19 +117,19 @@ let(:phishing_resistant_required) { true } let(:piv_cac_required) { false } - context 'when user is didnt select an mfa' do - let(:mfa_selection) { nil } + context 'when user did not select an mfa' do + let(:mfa_selection) { [] } - it 'does not submits the form' do + it 'is unsuccessful' do submission = subject.submit(selection: mfa_selection) - expect(submission.success?).to be_falsey + expect(submission.success?).to eq(false) end end context 'when user selects an mfa' do - it 'submits the form' do + it 'is successful' do submission = subject.submit(selection: mfa_selection) - expect(submission.success?).to be_truthy + expect(submission.success?).to eq(true) end end end