diff --git a/app/controllers/concerns/mfa_setup_concern.rb b/app/controllers/concerns/mfa_setup_concern.rb index f0d4bd6d0dc..d22df51a6c0 100644 --- a/app/controllers/concerns/mfa_setup_concern.rb +++ b/app/controllers/concerns/mfa_setup_concern.rb @@ -1,6 +1,25 @@ module MfaSetupConcern extend ActiveSupport::Concern + def user_next_authentication_setup_path!(final_path = nil) + case user_session[:selected_mfa_options]&.shift + when 'voice', 'sms', 'phone' + phone_setup_url + when 'auth_app' + authenticator_setup_url + when 'piv_cac' + setup_piv_cac_url + when 'webauthn' + webauthn_setup_url + when 'webauthn_platform' + webauthn_setup_url(platform: true) + when 'backup_code' + backup_code_setup_url + else + final_path + end + end + def confirm_user_authenticated_for_2fa_setup authenticate_user!(force: true) return if user_fully_authenticated? diff --git a/app/controllers/two_factor_authentication/otp_verification_controller.rb b/app/controllers/two_factor_authentication/otp_verification_controller.rb index 5cd8f7ddf37..6c170fc3d3e 100644 --- a/app/controllers/two_factor_authentication/otp_verification_controller.rb +++ b/app/controllers/two_factor_authentication/otp_verification_controller.rb @@ -1,6 +1,7 @@ module TwoFactorAuthentication class OtpVerificationController < ApplicationController include TwoFactorAuthenticatable + include MfaSetupConcern before_action :check_sp_required_mfa_bypass before_action :confirm_multiple_factors_enabled @@ -16,7 +17,11 @@ def create result = OtpVerificationForm.new(current_user, sanitized_otp_code).submit post_analytics(result) if result.success? - handle_valid_otp + next_url = nil + if UserSessionContext.confirmation_context?(context) + next_url = user_next_authentication_setup_path! + end + handle_valid_otp(next_url) else handle_invalid_otp end diff --git a/app/controllers/users/backup_code_setup_controller.rb b/app/controllers/users/backup_code_setup_controller.rb index a23d8dd332b..d844d6d1157 100644 --- a/app/controllers/users/backup_code_setup_controller.rb +++ b/app/controllers/users/backup_code_setup_controller.rb @@ -25,7 +25,7 @@ def edit; end def continue flash[:success] = t('notices.backup_codes_configured') - redirect_to after_mfa_setup_path + redirect_to user_next_authentication_setup_path!(after_mfa_setup_path) end def download diff --git a/app/controllers/users/piv_cac_authentication_setup_controller.rb b/app/controllers/users/piv_cac_authentication_setup_controller.rb index 735f6bf8b3c..ee21c20ff7d 100644 --- a/app/controllers/users/piv_cac_authentication_setup_controller.rb +++ b/app/controllers/users/piv_cac_authentication_setup_controller.rb @@ -103,7 +103,8 @@ def process_valid_submission create_user_event(:piv_cac_enabled) Funnel::Registration::AddMfa.call(current_user.id, 'piv_cac') session[:needs_to_setup_piv_cac_after_sign_in] = false - redirect_to after_sign_in_path_for(current_user) + final_path = after_sign_in_path_for(current_user) + redirect_to user_next_authentication_setup_path!(final_path) end def piv_cac_enabled? diff --git a/app/controllers/users/totp_setup_controller.rb b/app/controllers/users/totp_setup_controller.rb index e1dd7cfb875..1790e3de945 100644 --- a/app/controllers/users/totp_setup_controller.rb +++ b/app/controllers/users/totp_setup_controller.rb @@ -78,8 +78,8 @@ def process_valid_code mark_user_as_fully_authenticated handle_remember_device flash[:success] = t('notices.totp_configured') - redirect_to after_mfa_setup_path user_session.delete(:new_totp_secret) + redirect_to user_next_authentication_setup_path!(after_mfa_setup_path) end def handle_remember_device diff --git a/app/controllers/users/two_factor_authentication_setup_controller.rb b/app/controllers/users/two_factor_authentication_setup_controller.rb index 8c678faa4a2..2db72b6c6e4 100644 --- a/app/controllers/users/two_factor_authentication_setup_controller.rb +++ b/app/controllers/users/two_factor_authentication_setup_controller.rb @@ -43,20 +43,8 @@ def two_factor_options_presenter end def process_valid_form - case @two_factor_options_form.selection - when 'voice', 'sms', 'phone' - redirect_to phone_setup_url - when 'auth_app' - redirect_to authenticator_setup_url - when 'piv_cac' - redirect_to setup_piv_cac_url - when 'webauthn' - redirect_to webauthn_setup_url - when 'webauthn_platform' - redirect_to webauthn_setup_url(platform: true) - when 'backup_code' - redirect_to backup_code_setup_url - end + user_session[:selected_mfa_options] = @two_factor_options_form.selection + redirect_to user_next_authentication_setup_path!(user_session[:selected_mfa_options].first) end def handle_empty_selection @@ -73,7 +61,7 @@ def confirm_user_needs_2fa_setup end def two_factor_options_form_params - params.require(:two_factor_options_form).permit(:selection) + params.require(:two_factor_options_form).permit(:selection, selection: []) end end end diff --git a/app/controllers/users/webauthn_setup_controller.rb b/app/controllers/users/webauthn_setup_controller.rb index 516955b3577..1fd64e6a13c 100644 --- a/app/controllers/users/webauthn_setup_controller.rb +++ b/app/controllers/users/webauthn_setup_controller.rb @@ -138,7 +138,8 @@ def process_valid_webauthn(form) flash[:success] = t('notices.webauthn_configured') end user_session[:auth_method] = 'webauthn' - redirect_to after_mfa_setup_path + + redirect_to user_next_authentication_setup_path!(after_mfa_setup_path) end def handle_remember_device diff --git a/app/forms/two_factor_options_form.rb b/app/forms/two_factor_options_form.rb index 37e5937bae0..e25258d8676 100644 --- a/app/forms/two_factor_options_form.rb +++ b/app/forms/two_factor_options_form.rb @@ -13,7 +13,7 @@ def initialize(user) end def submit(params) - self.selection = params[:selection] + self.selection = Array(params[:selection]) success = valid? update_otp_delivery_preference_for_user if success && user_needs_updating? @@ -33,11 +33,13 @@ def extra_analytics_attributes end def user_needs_updating? - %w[voice sms].include?(selection) && selection != user.otp_delivery_preference + (%w[voice sms] & selection).present? && + !selection.include?(user.otp_delivery_preference) end def update_otp_delivery_preference_for_user - user_attributes = { otp_delivery_preference: selection } + user_attributes = { otp_delivery_preference: + selection.find { |element| %w[voice sms].include?(element) } } UpdateUser.new(user: user, attributes: user_attributes).call end end diff --git a/spec/controllers/users/backup_code_setup_controller_spec.rb b/spec/controllers/users/backup_code_setup_controller_spec.rb index 75b874f155e..0c0b15daf30 100644 --- a/spec/controllers/users/backup_code_setup_controller_spec.rb +++ b/spec/controllers/users/backup_code_setup_controller_spec.rb @@ -13,7 +13,7 @@ end it 'creates backup codes' do - user = create(:user, :signed_up) + user = build(:user, :signed_up) stub_sign_in(user) expect(PushNotification::HttpPush).to receive(:deliver). with(PushNotification::RecoveryInformationChangedEvent.new(user: user)) @@ -24,7 +24,7 @@ end it 'deletes backup codes' do - user = create(:user, :signed_up) + user = build(:user, :signed_up) stub_sign_in(user) expect(PushNotification::HttpPush).to receive(:deliver). with(PushNotification::RecoveryInformationChangedEvent.new(user: user)) @@ -34,6 +34,31 @@ expect(user.backup_code_configurations.length).to eq 0 end + context 'when user selects multiple mfas on account creation' do + it 'redirects to phone setup page' do + user = build(:user, :signed_up) + stub_sign_in(user) + codes = BackupCodeGenerator.new(user).create + controller.user_session[:backup_codes] = codes + + controller.user_session[:selected_mfa_options] = ['voice'] + post :continue + + expect(response).to redirect_to(phone_setup_url) + end + end + + context 'when user only selects backup code on account creation' do + it 'redirects to account page' do + user = build(:user, :signed_up) + stub_sign_in(user) + codes = BackupCodeGenerator.new(user).create + controller.user_session[:backup_codes] = codes + post :continue + expect(response).to redirect_to(account_url) + end + end + describe '#refreshed' do render_views diff --git a/spec/controllers/users/piv_cac_authentication_setup_controller_spec.rb b/spec/controllers/users/piv_cac_authentication_setup_controller_spec.rb index 013c440b166..55d89d59d44 100644 --- a/spec/controllers/users/piv_cac_authentication_setup_controller_spec.rb +++ b/spec/controllers/users/piv_cac_authentication_setup_controller_spec.rb @@ -94,26 +94,55 @@ end context 'when redirected with a good token' do - it 'redirects to account page' do - get :new, params: { token: good_token } - expect(response).to redirect_to(account_url) + context 'with no additional MFAs chosen on setup' do + it 'redirects to account page' do + get :new, params: { token: good_token } + expect(response).to redirect_to(account_url) + end + + it 'sets the piv/cac session information' do + get :new, params: { token: good_token } + json = { + 'subject' => 'some dn', + 'issuer' => nil, + 'presented' => true, + }.to_json + + expect(subject.user_session[:decrypted_x509]).to eq json + end + + it 'sets the session to not require piv setup upon sign-in' do + get :new, params: { token: good_token } + + expect(subject.session[:needs_to_setup_piv_cac_after_sign_in]).to eq false + end end - it 'sets the piv/cac session information' do - get :new, params: { token: good_token } - json = { - 'subject' => 'some dn', - 'issuer' => nil, - 'presented' => true, - }.to_json - - expect(subject.user_session[:decrypted_x509]).to eq json - end - - it 'sets the session to not require piv setup upon sign-in' do - get :new, params: { token: good_token } - - expect(subject.session[:needs_to_setup_piv_cac_after_sign_in]).to eq false + context 'with additional MFAs leftover' do + before do + subject.user_session[:selected_mfa_options] = ['voice'] + end + it 'redirects to phone setup page' do + get :new, params: { token: good_token } + expect(response).to redirect_to(phone_setup_url) + end + + it 'sets the piv/cac session information' do + get :new, params: { token: good_token } + json = { + 'subject' => 'some dn', + 'issuer' => nil, + 'presented' => true, + }.to_json + + expect(subject.user_session[:decrypted_x509]).to eq json + end + + it 'sets the session to not require piv setup upon sign-in' do + get :new, params: { token: good_token } + + expect(subject.session[:needs_to_setup_piv_cac_after_sign_in]).to eq false + end end end diff --git a/spec/controllers/users/totp_setup_controller_spec.rb b/spec/controllers/users/totp_setup_controller_spec.rb index 0b7e91c4dfe..8ea3ccb3402 100644 --- a/spec/controllers/users/totp_setup_controller_spec.rb +++ b/spec/controllers/users/totp_setup_controller_spec.rb @@ -194,30 +194,51 @@ end context 'when user presents correct code' do + let(:selected_mfa_options) { nil } before do secret = ROTP::Base32.random_base32 stub_sign_in_before_2fa stub_analytics allow(@analytics).to receive(:track_event) subject.user_session[:new_totp_secret] = secret + subject.user_session[:selected_mfa_options] = selected_mfa_options patch :confirm, params: { code: generate_totp_code(secret) } end + context 'when user selected only one method on account creation' do + it 'redirects to account_path with a success message' do + expect(response).to redirect_to(account_path) + expect(subject.user_session[:new_totp_secret]).to be_nil + + result = { + success: true, + errors: {}, + totp_secret_present: true, + multi_factor_auth_method: 'totp', + auth_app_configuration_id: next_auth_app_id, + } + + expect(@analytics).to have_received(:track_event). + with(Analytics::MULTI_FACTOR_AUTH_SETUP, result) + end + end - it 'redirects to account_path with a success message' do - expect(response).to redirect_to(account_path) - expect(subject.user_session[:new_totp_secret]).to be_nil - - result = { - success: true, - errors: {}, - totp_secret_present: true, - multi_factor_auth_method: 'totp', - auth_app_configuration_id: next_auth_app_id, - } - - expect(@analytics).to have_received(:track_event). - with(Analytics::MULTI_FACTOR_AUTH_SETUP, result) + context 'when user has multiple MFA methods left in user session' do + let(:selected_mfa_options) { ['voice'] } + it 'redirects to phone_setup_path with a success message and still logs analytics' do + expect(response).to redirect_to(phone_setup_path) + + result = { + success: true, + errors: {}, + totp_secret_present: true, + multi_factor_auth_method: 'totp', + auth_app_configuration_id: next_auth_app_id, + } + + expect(@analytics).to have_received(:track_event). + with(Analytics::MULTI_FACTOR_AUTH_SETUP, result) + end end end 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 bdd7d7a115b..96c11d000d8 100644 --- a/spec/controllers/users/two_factor_authentication_setup_controller_spec.rb +++ b/spec/controllers/users/two_factor_authentication_setup_controller_spec.rb @@ -64,14 +64,14 @@ }, } params = ActionController::Parameters.new(voice_params) - response = FormResponse.new(success: true, errors: {}, extra: { selection: 'voice' }) + response = FormResponse.new(success: true, errors: {}, extra: { selection: ['voice'] }) form = instance_double(TwoFactorOptionsForm) allow(TwoFactorOptionsForm).to receive(:new).with(user).and_return(form) expect(form).to receive(:submit). with(params.require(:two_factor_options_form).permit(:selection)). and_return(response) - expect(form).to receive(:selection).and_return('voice') + expect(form).to receive(:selection).and_return(['voice']) patch :create, params: voice_params end @@ -81,7 +81,7 @@ stub_analytics result = { - selection: 'voice', + selection: ['voice'], success: true, errors: {}, } @@ -110,6 +110,32 @@ end end + context 'when multi selection with phone first' do + it 'redirects properly' do + stub_sign_in_before_2fa + patch :create, params: { + two_factor_options_form: { + selection: ['phone', 'auth_app'], + }, + } + + expect(response).to redirect_to phone_setup_url + end + end + + context 'when multi selection with auth app first' do + it 'redirects properly' do + stub_sign_in_before_2fa + patch :create, params: { + two_factor_options_form: { + selection: ['auth_app', 'phone', 'webauthn'], + }, + } + + expect(response).to redirect_to authenticator_setup_url + end + end + context 'when the selection is auth_app' do it 'redirects to authentication app setup page' do stub_sign_in_before_2fa diff --git a/spec/controllers/users/webauthn_setup_controller_spec.rb b/spec/controllers/users/webauthn_setup_controller_spec.rb index f5a2c890a99..75fe4ef17dc 100644 --- a/spec/controllers/users/webauthn_setup_controller_spec.rb +++ b/spec/controllers/users/webauthn_setup_controller_spec.rb @@ -134,4 +134,42 @@ end end end + + describe 'when signed in and account creation' do + let(:user) { create(:user, :signed_up) } + let(:params) do + { + attestation_object: attestation_object, + client_data_json: setup_client_data_json, + name: 'mykey', + } + end + + before do + stub_analytics + stub_sign_in(user) + allow(IdentityConfig.store).to receive(:domain_name).and_return('localhost:3000') + controller.user_session[:webauthn_challenge] = webauthn_challenge + end + + context 'with multiple MFA methods chosen on account creation' do + before do + controller.user_session[:selected_mfa_options] = ['voice'] + end + + it 'should direct user to phone page' do + patch :confirm, params: params + + expect(response).to redirect_to(phone_setup_url) + end + end + + context 'with a single MFA method chosen on account creation' do + it 'should direct user to account page' do + patch :confirm, params: params + + expect(response).to redirect_to(account_url) + end + end + end end