From 76dcf0850270368dfd2d96008219e955b952375f Mon Sep 17 00:00:00 2001 From: kevinsmaster5 Date: Tue, 24 Sep 2024 08:56:43 -0400 Subject: [PATCH 01/45] changelog: Internal, MFA setup, Add attempt count to MFA setup analytics event --- app/controllers/users/webauthn_setup_controller.rb | 3 +++ app/services/analytics_events.rb | 3 +++ 2 files changed, 6 insertions(+) diff --git a/app/controllers/users/webauthn_setup_controller.rb b/app/controllers/users/webauthn_setup_controller.rb index 6a80cfda56b..f92a109bef7 100644 --- a/app/controllers/users/webauthn_setup_controller.rb +++ b/app/controllers/users/webauthn_setup_controller.rb @@ -51,6 +51,8 @@ def new end flash_error(result.errors) unless result.success? + session[:webauthn_attempts] = 0 if session[:webauthn_attempts].nil? + session[:webauthn_attempts] += 1 end def confirm @@ -141,6 +143,7 @@ def process_valid_webauthn(form) def analytics_properties { in_account_creation_flow: user_session[:in_account_creation_flow] || false, + webauthn_attempts: session[:webauthn_attempts] || nil, } end diff --git a/app/services/analytics_events.rb b/app/services/analytics_events.rb index d410f516c4e..bb3c0d29cfa 100644 --- a/app/services/analytics_events.rb +++ b/app/services/analytics_events.rb @@ -5287,6 +5287,7 @@ def multi_factor_auth_phone_setup( # @param [String, nil] key_id PIV/CAC key_id from PKI service # @param [Hash] mfa_method_counts Hash of MFA method with the number of that method on the account # @param [Hash] authenticator_data_flags WebAuthn authenticator data flags + # @param [Integer] webauthn_attempts number of setup attempts # @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. @@ -5310,6 +5311,7 @@ def multi_factor_auth_setup( key_id: nil, mfa_method_counts: nil, authenticator_data_flags: nil, + webauthn_attempts: nil, aaguid: nil, unknown_transports: nil, **extra @@ -5335,6 +5337,7 @@ def multi_factor_auth_setup( key_id:, mfa_method_counts:, authenticator_data_flags:, + webauthn_attempts:, aaguid:, unknown_transports:, **extra, From c6402f980089dd12aa919789f29b9f34f71da407 Mon Sep 17 00:00:00 2001 From: kevinsmaster5 Date: Wed, 25 Sep 2024 10:59:17 -0400 Subject: [PATCH 02/45] add piv attempts, use common name in analytics --- .../users/piv_cac_authentication_setup_controller.rb | 4 ++++ app/controllers/users/webauthn_setup_controller.rb | 2 +- app/services/analytics_events.rb | 6 +++--- 3 files changed, 8 insertions(+), 4 deletions(-) diff --git a/app/controllers/users/piv_cac_authentication_setup_controller.rb b/app/controllers/users/piv_cac_authentication_setup_controller.rb index 9b32434f741..c721beabf38 100644 --- a/app/controllers/users/piv_cac_authentication_setup_controller.rb +++ b/app/controllers/users/piv_cac_authentication_setup_controller.rb @@ -66,6 +66,8 @@ def piv_cac_service_url_with_redirect end def process_piv_cac_setup + session[:piv_cac_attempts] = 0 if session[:piv_cac_attempts].nil? + session[:piv_cac_attempts] += 1 result = user_piv_cac_form.submit properties = result.to_h.merge(analytics_properties) analytics.multi_factor_auth_setup(**properties) @@ -91,6 +93,7 @@ def process_valid_submission auth_method: TwoFactorAuthenticatable::AuthMethod::PIV_CAC, ) flash[:success] = t('notices.piv_cac_configured') + @piv_cac_attempts = session[:piv_cac_attempts] save_piv_cac_information( subject: user_piv_cac_form.x509_dn, issuer: user_piv_cac_form.x509_issuer, @@ -126,6 +129,7 @@ def analytics_properties { in_account_creation_flow: user_session[:in_account_creation_flow] || false, enabled_mfa_methods_count: mfa_context.enabled_mfa_methods_count, + mfa_attempts: session[:piv_cac_attempts] || nil, } end diff --git a/app/controllers/users/webauthn_setup_controller.rb b/app/controllers/users/webauthn_setup_controller.rb index f92a109bef7..aafe8587eca 100644 --- a/app/controllers/users/webauthn_setup_controller.rb +++ b/app/controllers/users/webauthn_setup_controller.rb @@ -143,7 +143,7 @@ def process_valid_webauthn(form) def analytics_properties { in_account_creation_flow: user_session[:in_account_creation_flow] || false, - webauthn_attempts: session[:webauthn_attempts] || nil, + mfa_attempts: session[:webauthn_attempts] || nil, } end diff --git a/app/services/analytics_events.rb b/app/services/analytics_events.rb index bb3c0d29cfa..1d65d0efe42 100644 --- a/app/services/analytics_events.rb +++ b/app/services/analytics_events.rb @@ -5287,7 +5287,7 @@ def multi_factor_auth_phone_setup( # @param [String, nil] key_id PIV/CAC key_id from PKI service # @param [Hash] mfa_method_counts Hash of MFA method with the number of that method on the account # @param [Hash] authenticator_data_flags WebAuthn authenticator data flags - # @param [Integer] webauthn_attempts number of setup attempts + # @param [Integer] mfa_attempts number of MFA setup attempts # @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. @@ -5311,7 +5311,7 @@ def multi_factor_auth_setup( key_id: nil, mfa_method_counts: nil, authenticator_data_flags: nil, - webauthn_attempts: nil, + mfa_attempts: nil, aaguid: nil, unknown_transports: nil, **extra @@ -5337,7 +5337,7 @@ def multi_factor_auth_setup( key_id:, mfa_method_counts:, authenticator_data_flags:, - webauthn_attempts:, + mfa_attempts:, aaguid:, unknown_transports:, **extra, From ca53ef12737c7de83e59a50da9a6a52c92832829 Mon Sep 17 00:00:00 2001 From: kevinsmaster5 Date: Thu, 26 Sep 2024 12:12:30 -0400 Subject: [PATCH 03/45] add logging to otp and totp setup --- .../two_factor_authentication/otp_verification_controller.rb | 3 +++ app/controllers/users/totp_setup_controller.rb | 4 +++- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/app/controllers/two_factor_authentication/otp_verification_controller.rb b/app/controllers/two_factor_authentication/otp_verification_controller.rb index 57b3d5b6012..122d9b026ce 100644 --- a/app/controllers/two_factor_authentication/otp_verification_controller.rb +++ b/app/controllers/two_factor_authentication/otp_verification_controller.rb @@ -21,6 +21,8 @@ def show end def create + session[:otp_attempts] = 0 if session[:otp_attempts].nil? + session[:otp_attempts] += 1 result = otp_verification_form.submit post_analytics(result) @@ -156,6 +158,7 @@ def analytics_properties phone_configuration_id: phone_configuration&.id, in_account_creation_flow: user_session[:in_account_creation_flow] || false, enabled_mfa_methods_count: mfa_context.enabled_mfa_methods_count, + mfa_attempts: session[:otp_attempts] || nil, } end diff --git a/app/controllers/users/totp_setup_controller.rb b/app/controllers/users/totp_setup_controller.rb index fb9524a8be1..652eada2222 100644 --- a/app/controllers/users/totp_setup_controller.rb +++ b/app/controllers/users/totp_setup_controller.rb @@ -26,7 +26,8 @@ def new def confirm result = totp_setup_form.submit - + session[:totp_attempts] = 0 if session[:totp_attempts].nil? + session[:totp_attempts] += 1 properties = result.to_h.merge(analytics_properties) analytics.multi_factor_auth_setup(**properties) @@ -118,6 +119,7 @@ def analytics_properties { in_account_creation_flow: in_account_creation_flow?, pii_like_keypaths: [[:mfa_method_counts, :phone]], + mfa_attempts: session[:totp_attempts] || nil, } end end From 6542fe3dc482ffa54f7336cff37af73b38350f42 Mon Sep 17 00:00:00 2001 From: kevinsmaster5 Date: Thu, 26 Sep 2024 14:39:55 -0400 Subject: [PATCH 04/45] reset attempts count on success --- .../two_factor_authentication/otp_verification_controller.rb | 1 + app/controllers/users/piv_cac_authentication_setup_controller.rb | 1 + app/controllers/users/webauthn_setup_controller.rb | 1 + 3 files changed, 3 insertions(+) diff --git a/app/controllers/two_factor_authentication/otp_verification_controller.rb b/app/controllers/two_factor_authentication/otp_verification_controller.rb index 122d9b026ce..4e14de3832b 100644 --- a/app/controllers/two_factor_authentication/otp_verification_controller.rb +++ b/app/controllers/two_factor_authentication/otp_verification_controller.rb @@ -143,6 +143,7 @@ def form_params def post_analytics(result) properties = result.to_h.merge(analytics_properties) analytics.multi_factor_auth_setup(**properties) if context == 'confirmation' + session[:otp_attempts] = nil if result.success? end def analytics_properties diff --git a/app/controllers/users/piv_cac_authentication_setup_controller.rb b/app/controllers/users/piv_cac_authentication_setup_controller.rb index c721beabf38..2e246503e34 100644 --- a/app/controllers/users/piv_cac_authentication_setup_controller.rb +++ b/app/controllers/users/piv_cac_authentication_setup_controller.rb @@ -73,6 +73,7 @@ def process_piv_cac_setup analytics.multi_factor_auth_setup(**properties) if result.success? process_valid_submission + session[:piv_cac_attempts] = nil else process_invalid_submission end diff --git a/app/controllers/users/webauthn_setup_controller.rb b/app/controllers/users/webauthn_setup_controller.rb index aafe8587eca..83cf635f4e6 100644 --- a/app/controllers/users/webauthn_setup_controller.rb +++ b/app/controllers/users/webauthn_setup_controller.rb @@ -73,6 +73,7 @@ def confirm ) properties = result.to_h.merge(analytics_properties) analytics.multi_factor_auth_setup(**properties) + session[:webauthn_attempts] = nil if result.success? if result.success? process_valid_webauthn(form) From 587da36c1309653823af523b67b1dfd102e79dd9 Mon Sep 17 00:00:00 2001 From: kevinsmaster5 Date: Thu, 26 Sep 2024 14:44:17 -0400 Subject: [PATCH 05/45] add totp reset and refactor reset on webauthn --- app/controllers/users/totp_setup_controller.rb | 1 + app/controllers/users/webauthn_setup_controller.rb | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/app/controllers/users/totp_setup_controller.rb b/app/controllers/users/totp_setup_controller.rb index 652eada2222..42524bbe21b 100644 --- a/app/controllers/users/totp_setup_controller.rb +++ b/app/controllers/users/totp_setup_controller.rb @@ -33,6 +33,7 @@ def confirm if result.success? process_valid_code + session[:totp_attempts] = nil else process_invalid_code end diff --git a/app/controllers/users/webauthn_setup_controller.rb b/app/controllers/users/webauthn_setup_controller.rb index 83cf635f4e6..65b6dc70e1a 100644 --- a/app/controllers/users/webauthn_setup_controller.rb +++ b/app/controllers/users/webauthn_setup_controller.rb @@ -73,10 +73,10 @@ def confirm ) properties = result.to_h.merge(analytics_properties) analytics.multi_factor_auth_setup(**properties) - session[:webauthn_attempts] = nil if result.success? if result.success? process_valid_webauthn(form) + session[:webauthn_attempts] = nil else flash.now[:error] = result.first_error_message render :new From ce6927afdadeb4d36d1a727003528b2044df1686 Mon Sep 17 00:00:00 2001 From: Kevin Masters <135744319+kevinsmaster5@users.noreply.github.com> Date: Thu, 26 Sep 2024 16:21:52 -0400 Subject: [PATCH 06/45] update otp specs --- app/services/analytics_events.rb | 6 ++++++ .../otp_verification_controller_spec.rb | 5 +++++ 2 files changed, 11 insertions(+) diff --git a/app/services/analytics_events.rb b/app/services/analytics_events.rb index 1d65d0efe42..3dc809112f5 100644 --- a/app/services/analytics_events.rb +++ b/app/services/analytics_events.rb @@ -4945,6 +4945,7 @@ def logout_initiated( # @param [Boolean] new_device Whether the user is authenticating from a new device # @param [String] multi_factor_auth_method Authentication method used # @param [String] multi_factor_auth_method_created_at When the authentication method was created + # @param [Integer] mfa_attempts number of MFA setup attempts # @param [Integer] auth_app_configuration_id Database ID of authentication app configuration # @param [Integer] piv_cac_configuration_id Database ID of PIV/CAC configuration # @param [String] piv_cac_configuration_dn_uuid PIV/CAC X509 distinguished name UUID @@ -4968,6 +4969,7 @@ def multi_factor_auth( errors: nil, error_details: nil, context: nil, + mfa_attempts: nil, multi_factor_auth_method_created_at: nil, auth_app_configuration_id: nil, piv_cac_configuration_id: nil, @@ -4991,6 +4993,7 @@ def multi_factor_auth( error_details:, context:, new_device:, + mfa_attempts:, multi_factor_auth_method:, multi_factor_auth_method_created_at:, auth_app_configuration_id:, @@ -5088,6 +5091,7 @@ def multi_factor_auth_enter_backup_code_visit(context:, **extra) end # @param ["authentication", "reauthentication", "confirmation"] context User session context + # @param [Integer] mfa_attempts number of MFA setup attempts # @param [String] multi_factor_auth_method # @param [Boolean] confirmation_for_add_phone # @param [Integer] phone_configuration_id @@ -5107,11 +5111,13 @@ def multi_factor_auth_enter_otp_visit( phone_fingerprint:, in_account_creation_flow:, enabled_mfa_methods_count:, + mfa_attempts: nil, **extra ) track_event( 'Multi-Factor Authentication: enter OTP visited', context:, + mfa_attempts:, multi_factor_auth_method:, confirmation_for_add_phone:, phone_configuration_id:, diff --git a/spec/controllers/two_factor_authentication/otp_verification_controller_spec.rb b/spec/controllers/two_factor_authentication/otp_verification_controller_spec.rb index cf02c712fd8..bd3c1e2bd04 100644 --- a/spec/controllers/two_factor_authentication/otp_verification_controller_spec.rb +++ b/spec/controllers/two_factor_authentication/otp_verification_controller_spec.rb @@ -152,6 +152,7 @@ phone_fingerprint: Pii::Fingerprinter.fingerprint(parsed_phone.e164), enabled_mfa_methods_count: 1, in_account_creation_flow: false, + mfa_attempts: 1, ) end @@ -234,6 +235,7 @@ phone_fingerprint: Pii::Fingerprinter.fingerprint(parsed_phone.e164), enabled_mfa_methods_count: 1, in_account_creation_flow: false, + mfa_attempts: 1, ) expect(@analytics).to have_logged_event('Multi-Factor Authentication: max attempts reached') end @@ -535,6 +537,7 @@ phone_fingerprint: Pii::Fingerprinter.fingerprint(parsed_phone.e164), enabled_mfa_methods_count: 1, in_account_creation_flow: true, + mfa_attempts: 1, ) end @@ -604,6 +607,7 @@ phone_fingerprint: Pii::Fingerprinter.fingerprint(parsed_phone.e164), enabled_mfa_methods_count: 1, in_account_creation_flow: false, + mfa_attempts: 1, ) end @@ -669,6 +673,7 @@ phone_fingerprint: Pii::Fingerprinter.fingerprint(parsed_phone.e164), enabled_mfa_methods_count: 0, in_account_creation_flow: false, + mfa_attempts: 1, ) expect(controller).to have_received(:create_user_event).with(:phone_confirmed) From a5b882513f4b9a2100ada89695eccc2a1660c31b Mon Sep 17 00:00:00 2001 From: Kevin Masters <135744319+kevinsmaster5@users.noreply.github.com> Date: Thu, 26 Sep 2024 16:30:04 -0400 Subject: [PATCH 07/45] update totp specs --- spec/controllers/users/totp_setup_controller_spec.rb | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/spec/controllers/users/totp_setup_controller_spec.rb b/spec/controllers/users/totp_setup_controller_spec.rb index 0d865fc2e1f..974a4e7bac1 100644 --- a/spec/controllers/users/totp_setup_controller_spec.rb +++ b/spec/controllers/users/totp_setup_controller_spec.rb @@ -108,6 +108,7 @@ multi_factor_auth_method: 'totp', enabled_mfa_methods_count: 0, in_account_creation_flow: false, + mfa_attempts: 1, ) end end @@ -136,6 +137,7 @@ auth_app_configuration_id: next_auth_app_id, enabled_mfa_methods_count: 2, in_account_creation_flow: false, + mfa_attempts: 1, ) end end @@ -164,6 +166,7 @@ multi_factor_auth_method: 'totp', enabled_mfa_methods_count: 1, in_account_creation_flow: false, + mfa_attempts: 1, ) end end @@ -193,6 +196,7 @@ multi_factor_auth_method: 'totp', enabled_mfa_methods_count: 1, in_account_creation_flow: false, + mfa_attempts: 1, ) end end @@ -221,6 +225,7 @@ multi_factor_auth_method: 'totp', enabled_mfa_methods_count: 0, in_account_creation_flow: false, + mfa_attempts: 1, ) end end @@ -252,6 +257,7 @@ auth_app_configuration_id: next_auth_app_id, enabled_mfa_methods_count: 1, in_account_creation_flow: true, + mfa_attempts: 1, ) end end @@ -271,6 +277,7 @@ auth_app_configuration_id: next_auth_app_id, enabled_mfa_methods_count: 1, in_account_creation_flow: true, + mfa_attempts: 1, ) end end @@ -297,6 +304,7 @@ multi_factor_auth_method: 'totp', enabled_mfa_methods_count: 0, in_account_creation_flow: false, + mfa_attempts: 1, ) end end From afb95c5970780531120616e7ef0ce19801002981 Mon Sep 17 00:00:00 2001 From: Kevin Masters <135744319+kevinsmaster5@users.noreply.github.com> Date: Thu, 26 Sep 2024 16:38:59 -0400 Subject: [PATCH 08/45] update piv_cac_setup specs --- app/services/analytics_events.rb | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/app/services/analytics_events.rb b/app/services/analytics_events.rb index 3dc809112f5..c39bb917716 100644 --- a/app/services/analytics_events.rb +++ b/app/services/analytics_events.rb @@ -5041,10 +5041,12 @@ def multi_factor_auth_added_phone( # @param [Integer] enabled_mfa_methods_count Number of enabled MFA methods on the account # @param [Boolean] in_account_creation_flow whether user is going through creation flow # @param ['piv_cac'] method_name Authentication method added + # @param [Integer] mfa_attempts number of MFA setup attempts def multi_factor_auth_added_piv_cac( enabled_mfa_methods_count:, in_account_creation_flow:, method_name: :piv_cac, + mfa_attempts: nil, **extra ) track_event( @@ -5052,6 +5054,7 @@ def multi_factor_auth_added_piv_cac( method_name:, enabled_mfa_methods_count:, in_account_creation_flow:, + mfa_attempts:, **extra, ) end @@ -5993,11 +5996,18 @@ def piv_cac_recommended_visited # Tracks when user's piv cac setup # @param [Boolean] in_account_creation_flow Whether user is going through account creation # @param [Integer] enabled_mfa_methods_count Number of enabled MFA methods on the account - def piv_cac_setup_visited(in_account_creation_flow:, enabled_mfa_methods_count: nil, **extra) + # @param [Integer] mfa_attempts number of MFA setup attempts + def piv_cac_setup_visited( + in_account_creation_flow:, + enabled_mfa_methods_count: nil, + mfa_attempts: nil, + **extra + ) track_event( :piv_cac_setup_visited, in_account_creation_flow:, enabled_mfa_methods_count:, + mfa_attempts:, **extra, ) end From f9e035dc36c0f6017b8e084c72bc47a39fb48e60 Mon Sep 17 00:00:00 2001 From: Kevin Masters <135744319+kevinsmaster5@users.noreply.github.com> Date: Fri, 27 Sep 2024 11:47:49 -0400 Subject: [PATCH 09/45] add otp spec that confirms incremented mfa_attempts analytics log --- .../otp_verification_controller_spec.rb | 27 +++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/spec/controllers/two_factor_authentication/otp_verification_controller_spec.rb b/spec/controllers/two_factor_authentication/otp_verification_controller_spec.rb index bd3c1e2bd04..97047181008 100644 --- a/spec/controllers/two_factor_authentication/otp_verification_controller_spec.rb +++ b/spec/controllers/two_factor_authentication/otp_verification_controller_spec.rb @@ -614,6 +614,13 @@ context 'user enters in valid code after invalid entry' do before do expect(subject.current_user.reload.second_factor_attempts_count).to eq 1 + post( + :create, + params: { + code: '999', + otp_delivery_preference: 'sms', + }, + ) post( :create, params: { @@ -625,6 +632,26 @@ it 'resets second_factor_attempts_count' do expect(subject.current_user.reload.second_factor_attempts_count).to eq 0 end + + it 'tracks an event' do + expect(@analytics).to have_logged_event( + 'Multi-Factor Authentication Setup', + success: false, + error_details: { code: { wrong_length: true, incorrect: true } }, + confirmation_for_add_phone: true, + context: 'confirmation', + multi_factor_auth_method: 'sms', + phone_configuration_id: controller.current_user.default_phone_configuration.id, + multi_factor_auth_method_created_at: controller.current_user. + default_phone_configuration.created_at.strftime('%s%L'), + area_code: parsed_phone.area_code, + country_code: parsed_phone.country, + phone_fingerprint: Pii::Fingerprinter.fingerprint(parsed_phone.e164), + enabled_mfa_methods_count: 1, + in_account_creation_flow: false, + mfa_attempts: 2, + ) + end end end From 563173a8cd6686d9c454dd3c189460545ab71d7c Mon Sep 17 00:00:00 2001 From: Kevin Masters <135744319+kevinsmaster5@users.noreply.github.com> Date: Fri, 27 Sep 2024 12:40:31 -0400 Subject: [PATCH 10/45] add specs for piv and totp --- ...ac_authentication_setup_controller_spec.rb | 27 ++++++++++++++++ .../users/totp_setup_controller_spec.rb | 31 +++++++++++++++++++ 2 files changed, 58 insertions(+) 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 3656a2d72ac..b2e4c30ee8f 100644 --- a/spec/controllers/users/piv_cac_authentication_setup_controller_spec.rb +++ b/spec/controllers/users/piv_cac_authentication_setup_controller_spec.rb @@ -109,8 +109,35 @@ context 'with no additional MFAs chosen on setup' do let(:mfa_selections) { ['piv_cac'] } it 'redirects to suggest 2nd MFA page' do + stub_analytics get :new, params: { token: good_token } expect(response).to redirect_to(auth_method_confirmation_url) + + expect(@analytics).to have_logged_event( + 'Multi-Factor Authentication Setup', + enabled_mfa_methods_count: 1, + errors: {}, + multi_factor_auth_method: 'piv_cac', + in_account_creation_flow: false, + success: true, + mfa_attempts: 1, + ) + end + + it 'logs mfa attempts commensurate to number of attempts' do + stub_analytics + get :new, params: { token: bad_token } + get :new, params: { token: good_token } + + expect(@analytics).to have_logged_event( + 'Multi-Factor Authentication Setup', + enabled_mfa_methods_count: 1, + errors: {}, + multi_factor_auth_method: 'piv_cac', + in_account_creation_flow: false, + success: true, + mfa_attempts: 2, + ) end it 'sets the piv/cac session information' do diff --git a/spec/controllers/users/totp_setup_controller_spec.rb b/spec/controllers/users/totp_setup_controller_spec.rb index 974a4e7bac1..07d34a58759 100644 --- a/spec/controllers/users/totp_setup_controller_spec.rb +++ b/spec/controllers/users/totp_setup_controller_spec.rb @@ -142,6 +142,37 @@ end end + context 'when user presents correct code after submitting an incorrect code' do + before do + user = create(:user, :fully_registered) + secret = ROTP::Base32.random_base32 + stub_sign_in(user) + stub_analytics + + subject.user_session[:new_totp_secret] = 'abcdehij' + + patch :confirm, params: { name: name, code: 123 } + + subject.user_session[:new_totp_secret] = secret + + patch :confirm, params: { name: name, code: generate_totp_code(secret) } + end + + it 'logs correct events' do + expect(@analytics).to have_logged_event( + 'Multi-Factor Authentication Setup', + success: true, + errors: {}, + totp_secret_present: true, + multi_factor_auth_method: 'totp', + auth_app_configuration_id: next_auth_app_id, + enabled_mfa_methods_count: 2, + in_account_creation_flow: false, + mfa_attempts: 2, + ) + end + end + context 'when user presents nil code' do before do user = create(:user, :fully_registered) From c93c566402d86c20e8b3969b22f6e9a39ebbcff6 Mon Sep 17 00:00:00 2001 From: Kevin Masters <135744319+kevinsmaster5@users.noreply.github.com> Date: Mon, 30 Sep 2024 09:29:12 -0400 Subject: [PATCH 11/45] refactor slightly session key setter --- .../two_factor_authentication/otp_verification_controller.rb | 2 +- .../users/piv_cac_authentication_setup_controller.rb | 2 +- app/controllers/users/totp_setup_controller.rb | 2 +- app/controllers/users/webauthn_setup_controller.rb | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/app/controllers/two_factor_authentication/otp_verification_controller.rb b/app/controllers/two_factor_authentication/otp_verification_controller.rb index 4e14de3832b..a74035ff45e 100644 --- a/app/controllers/two_factor_authentication/otp_verification_controller.rb +++ b/app/controllers/two_factor_authentication/otp_verification_controller.rb @@ -21,7 +21,7 @@ def show end def create - session[:otp_attempts] = 0 if session[:otp_attempts].nil? + session[:otp_attempts] ||= 0 session[:otp_attempts] += 1 result = otp_verification_form.submit post_analytics(result) diff --git a/app/controllers/users/piv_cac_authentication_setup_controller.rb b/app/controllers/users/piv_cac_authentication_setup_controller.rb index 2e246503e34..69a7c04e79f 100644 --- a/app/controllers/users/piv_cac_authentication_setup_controller.rb +++ b/app/controllers/users/piv_cac_authentication_setup_controller.rb @@ -66,7 +66,7 @@ def piv_cac_service_url_with_redirect end def process_piv_cac_setup - session[:piv_cac_attempts] = 0 if session[:piv_cac_attempts].nil? + session[:piv_cac_attempts] ||= 0 session[:piv_cac_attempts] += 1 result = user_piv_cac_form.submit properties = result.to_h.merge(analytics_properties) diff --git a/app/controllers/users/totp_setup_controller.rb b/app/controllers/users/totp_setup_controller.rb index 42524bbe21b..e7835e098ee 100644 --- a/app/controllers/users/totp_setup_controller.rb +++ b/app/controllers/users/totp_setup_controller.rb @@ -26,7 +26,7 @@ def new def confirm result = totp_setup_form.submit - session[:totp_attempts] = 0 if session[:totp_attempts].nil? + session[:totp_attempts] ||= 0 session[:totp_attempts] += 1 properties = result.to_h.merge(analytics_properties) analytics.multi_factor_auth_setup(**properties) diff --git a/app/controllers/users/webauthn_setup_controller.rb b/app/controllers/users/webauthn_setup_controller.rb index 65b6dc70e1a..eba5b3eb884 100644 --- a/app/controllers/users/webauthn_setup_controller.rb +++ b/app/controllers/users/webauthn_setup_controller.rb @@ -51,7 +51,7 @@ def new end flash_error(result.errors) unless result.success? - session[:webauthn_attempts] = 0 if session[:webauthn_attempts].nil? + session[:webauthn_attempts] ||= 0 session[:webauthn_attempts] += 1 end From 34cc1783e62f21a85236fce9e1dad0d97ab762be Mon Sep 17 00:00:00 2001 From: Kevin Masters <135744319+kevinsmaster5@users.noreply.github.com> Date: Mon, 30 Sep 2024 10:20:21 -0400 Subject: [PATCH 12/45] add testing for attempts on webauthn setup --- .../users/webauthn_setup_controller_spec.rb | 52 +++++++++++++++++++ 1 file changed, 52 insertions(+) diff --git a/spec/controllers/users/webauthn_setup_controller_spec.rb b/spec/controllers/users/webauthn_setup_controller_spec.rb index b801a775bdc..db80abc4d29 100644 --- a/spec/controllers/users/webauthn_setup_controller_spec.rb +++ b/spec/controllers/users/webauthn_setup_controller_spec.rb @@ -369,5 +369,57 @@ end end end + + context 'sign in and confirm' do + let(:params) do + { + attestation_object: attestation_object, + client_data_json: setup_client_data_json, + name: 'mykey', + transports: 'usb', + authenticator_data_value: '65', + } + end + + before do + controller.user_session[:in_account_creation_flow] = true + allow(IdentityConfig.store).to receive(:domain_name).and_return('localhost:3000') + request.host = 'localhost:3000' + controller.user_session[:webauthn_challenge] = webauthn_challenge + session[:webauthn_attempts] = 1 + end + + it 'tracks the submission' do + Funnel::Registration::AddMfa.call(user.id, 'phone', @analytics) + + patch :confirm, params: params + + expect(@analytics).to have_logged_event( + 'Multi-Factor Authentication Setup', + enabled_mfa_methods_count: 1, + mfa_method_counts: { + webauthn: 1, + }, + multi_factor_auth_method: 'webauthn', + success: true, + errors: {}, + in_account_creation_flow: true, + authenticator_data_flags: { + up: true, + uv: false, + be: false, + bs: false, + at: true, + ed: false, + }, + mfa_attempts: 1, + ) + expect(@analytics).to have_logged_event( + :webauthn_setup_submitted, + platform_authenticator: false, + success: true, + ) + end + end end end From 34f553c8ea2a4bdd16348cd1dcbe28e724b3dcf3 Mon Sep 17 00:00:00 2001 From: Kevin Masters <135744319+kevinsmaster5@users.noreply.github.com> Date: Thu, 3 Oct 2024 12:04:06 -0400 Subject: [PATCH 13/45] refactor pulling commonly use function into mfa setup concern --- app/controllers/concerns/mfa_setup_concern.rb | 9 +++++++++ .../otp_verification_controller.rb | 7 +++---- .../users/piv_cac_authentication_setup_controller.rb | 8 +++----- app/controllers/users/totp_setup_controller.rb | 7 +++---- app/controllers/users/webauthn_setup_controller.rb | 7 +++---- spec/controllers/users/webauthn_setup_controller_spec.rb | 2 +- 6 files changed, 22 insertions(+), 18 deletions(-) diff --git a/app/controllers/concerns/mfa_setup_concern.rb b/app/controllers/concerns/mfa_setup_concern.rb index fb23d795c6a..7ffbdfd837c 100644 --- a/app/controllers/concerns/mfa_setup_concern.rb +++ b/app/controllers/concerns/mfa_setup_concern.rb @@ -64,6 +64,15 @@ def in_account_creation_flow? user_session[:in_account_creation_flow] || false end + def mfa_selection_attempt_count + session[:mfa_attempts] ||= 0 + session[:mfa_attempts] += 1 + end + + def reset_mfa_selection_attempt_count + session[:mfa_attempts] = nil + end + def mfa_selection_count user_session[:mfa_selections]&.count || 0 end diff --git a/app/controllers/two_factor_authentication/otp_verification_controller.rb b/app/controllers/two_factor_authentication/otp_verification_controller.rb index a74035ff45e..43d56b46c13 100644 --- a/app/controllers/two_factor_authentication/otp_verification_controller.rb +++ b/app/controllers/two_factor_authentication/otp_verification_controller.rb @@ -21,8 +21,7 @@ def show end def create - session[:otp_attempts] ||= 0 - session[:otp_attempts] += 1 + mfa_selection_attempt_count result = otp_verification_form.submit post_analytics(result) @@ -143,7 +142,7 @@ def form_params def post_analytics(result) properties = result.to_h.merge(analytics_properties) analytics.multi_factor_auth_setup(**properties) if context == 'confirmation' - session[:otp_attempts] = nil if result.success? + reset_mfa_selection_attempt_count if result.success? end def analytics_properties @@ -159,7 +158,7 @@ def analytics_properties phone_configuration_id: phone_configuration&.id, in_account_creation_flow: user_session[:in_account_creation_flow] || false, enabled_mfa_methods_count: mfa_context.enabled_mfa_methods_count, - mfa_attempts: session[:otp_attempts] || nil, + mfa_attempts: session[:mfa_attempts] || nil, } end diff --git a/app/controllers/users/piv_cac_authentication_setup_controller.rb b/app/controllers/users/piv_cac_authentication_setup_controller.rb index 69a7c04e79f..304c78ac150 100644 --- a/app/controllers/users/piv_cac_authentication_setup_controller.rb +++ b/app/controllers/users/piv_cac_authentication_setup_controller.rb @@ -66,14 +66,13 @@ def piv_cac_service_url_with_redirect end def process_piv_cac_setup - session[:piv_cac_attempts] ||= 0 - session[:piv_cac_attempts] += 1 + mfa_selection_attempt_count result = user_piv_cac_form.submit properties = result.to_h.merge(analytics_properties) analytics.multi_factor_auth_setup(**properties) if result.success? process_valid_submission - session[:piv_cac_attempts] = nil + reset_mfa_selection_attempt_count else process_invalid_submission end @@ -94,7 +93,6 @@ def process_valid_submission auth_method: TwoFactorAuthenticatable::AuthMethod::PIV_CAC, ) flash[:success] = t('notices.piv_cac_configured') - @piv_cac_attempts = session[:piv_cac_attempts] save_piv_cac_information( subject: user_piv_cac_form.x509_dn, issuer: user_piv_cac_form.x509_issuer, @@ -130,7 +128,7 @@ def analytics_properties { in_account_creation_flow: user_session[:in_account_creation_flow] || false, enabled_mfa_methods_count: mfa_context.enabled_mfa_methods_count, - mfa_attempts: session[:piv_cac_attempts] || nil, + mfa_attempts: session[:mfa_attempts] || nil, } end diff --git a/app/controllers/users/totp_setup_controller.rb b/app/controllers/users/totp_setup_controller.rb index e7835e098ee..c793852679a 100644 --- a/app/controllers/users/totp_setup_controller.rb +++ b/app/controllers/users/totp_setup_controller.rb @@ -26,14 +26,13 @@ def new def confirm result = totp_setup_form.submit - session[:totp_attempts] ||= 0 - session[:totp_attempts] += 1 + mfa_selection_attempt_count properties = result.to_h.merge(analytics_properties) analytics.multi_factor_auth_setup(**properties) if result.success? process_valid_code - session[:totp_attempts] = nil + reset_mfa_selection_attempt_count else process_invalid_code end @@ -120,7 +119,7 @@ def analytics_properties { in_account_creation_flow: in_account_creation_flow?, pii_like_keypaths: [[:mfa_method_counts, :phone]], - mfa_attempts: session[:totp_attempts] || nil, + mfa_attempts: session[:mfa_attempts] || nil, } end end diff --git a/app/controllers/users/webauthn_setup_controller.rb b/app/controllers/users/webauthn_setup_controller.rb index eba5b3eb884..5180419add5 100644 --- a/app/controllers/users/webauthn_setup_controller.rb +++ b/app/controllers/users/webauthn_setup_controller.rb @@ -51,8 +51,7 @@ def new end flash_error(result.errors) unless result.success? - session[:webauthn_attempts] ||= 0 - session[:webauthn_attempts] += 1 + mfa_selection_attempt_count end def confirm @@ -76,7 +75,7 @@ def confirm if result.success? process_valid_webauthn(form) - session[:webauthn_attempts] = nil + reset_mfa_selection_attempt_count else flash.now[:error] = result.first_error_message render :new @@ -144,7 +143,7 @@ def process_valid_webauthn(form) def analytics_properties { in_account_creation_flow: user_session[:in_account_creation_flow] || false, - mfa_attempts: session[:webauthn_attempts] || nil, + mfa_attempts: session[:mfa_attempts] || nil, } end diff --git a/spec/controllers/users/webauthn_setup_controller_spec.rb b/spec/controllers/users/webauthn_setup_controller_spec.rb index db80abc4d29..64457048af5 100644 --- a/spec/controllers/users/webauthn_setup_controller_spec.rb +++ b/spec/controllers/users/webauthn_setup_controller_spec.rb @@ -386,7 +386,7 @@ allow(IdentityConfig.store).to receive(:domain_name).and_return('localhost:3000') request.host = 'localhost:3000' controller.user_session[:webauthn_challenge] = webauthn_challenge - session[:webauthn_attempts] = 1 + session[:mfa_attempts] = 1 end it 'tracks the submission' do From 73ecdc86149e4c3d3193f128b723deaaad2640b3 Mon Sep 17 00:00:00 2001 From: Kevin Masters <135744319+kevinsmaster5@users.noreply.github.com> Date: Fri, 4 Oct 2024 15:26:48 -0400 Subject: [PATCH 14/45] move mfa attempts to 2fa methods concern, change to user_session --- app/controllers/concerns/mfa_setup_concern.rb | 9 --------- .../concerns/two_factor_authenticatable_methods.rb | 9 +++++++++ .../otp_verification_controller.rb | 2 +- .../users/piv_cac_authentication_setup_controller.rb | 2 +- app/controllers/users/totp_setup_controller.rb | 2 +- app/controllers/users/webauthn_setup_controller.rb | 2 +- 6 files changed, 13 insertions(+), 13 deletions(-) diff --git a/app/controllers/concerns/mfa_setup_concern.rb b/app/controllers/concerns/mfa_setup_concern.rb index 7ffbdfd837c..fb23d795c6a 100644 --- a/app/controllers/concerns/mfa_setup_concern.rb +++ b/app/controllers/concerns/mfa_setup_concern.rb @@ -64,15 +64,6 @@ def in_account_creation_flow? user_session[:in_account_creation_flow] || false end - def mfa_selection_attempt_count - session[:mfa_attempts] ||= 0 - session[:mfa_attempts] += 1 - end - - def reset_mfa_selection_attempt_count - session[:mfa_attempts] = nil - end - def mfa_selection_count user_session[:mfa_selections]&.count || 0 end diff --git a/app/controllers/concerns/two_factor_authenticatable_methods.rb b/app/controllers/concerns/two_factor_authenticatable_methods.rb index bcfee78cc87..2076d13ac03 100644 --- a/app/controllers/concerns/two_factor_authenticatable_methods.rb +++ b/app/controllers/concerns/two_factor_authenticatable_methods.rb @@ -124,6 +124,15 @@ def handle_remember_device_preference(remember_device_preference) save_remember_device_preference(remember_device_preference) end + def mfa_selection_attempt_count + user_session[:mfa_attempts] ||= 0 + user_session[:mfa_attempts] += 1 + end + + def reset_mfa_selection_attempt_count + user_session.delete(:mfa_attempts) + end + # Method will be renamed in the next refactor. # You can pass in any "type" with a corresponding I18n key in # two_factor_authentication.invalid_#{type} diff --git a/app/controllers/two_factor_authentication/otp_verification_controller.rb b/app/controllers/two_factor_authentication/otp_verification_controller.rb index 43d56b46c13..c2d01ce2eed 100644 --- a/app/controllers/two_factor_authentication/otp_verification_controller.rb +++ b/app/controllers/two_factor_authentication/otp_verification_controller.rb @@ -158,7 +158,7 @@ def analytics_properties phone_configuration_id: phone_configuration&.id, in_account_creation_flow: user_session[:in_account_creation_flow] || false, enabled_mfa_methods_count: mfa_context.enabled_mfa_methods_count, - mfa_attempts: session[:mfa_attempts] || nil, + mfa_attempts: user_session[:mfa_attempts] || nil, } end diff --git a/app/controllers/users/piv_cac_authentication_setup_controller.rb b/app/controllers/users/piv_cac_authentication_setup_controller.rb index 304c78ac150..b854e192832 100644 --- a/app/controllers/users/piv_cac_authentication_setup_controller.rb +++ b/app/controllers/users/piv_cac_authentication_setup_controller.rb @@ -128,7 +128,7 @@ def analytics_properties { in_account_creation_flow: user_session[:in_account_creation_flow] || false, enabled_mfa_methods_count: mfa_context.enabled_mfa_methods_count, - mfa_attempts: session[:mfa_attempts] || nil, + mfa_attempts: user_session[:mfa_attempts] || nil, } end diff --git a/app/controllers/users/totp_setup_controller.rb b/app/controllers/users/totp_setup_controller.rb index c793852679a..5956d61776a 100644 --- a/app/controllers/users/totp_setup_controller.rb +++ b/app/controllers/users/totp_setup_controller.rb @@ -119,7 +119,7 @@ def analytics_properties { in_account_creation_flow: in_account_creation_flow?, pii_like_keypaths: [[:mfa_method_counts, :phone]], - mfa_attempts: session[:mfa_attempts] || nil, + mfa_attempts: user_session[:mfa_attempts] || nil, } end end diff --git a/app/controllers/users/webauthn_setup_controller.rb b/app/controllers/users/webauthn_setup_controller.rb index 5180419add5..7c17ece08a2 100644 --- a/app/controllers/users/webauthn_setup_controller.rb +++ b/app/controllers/users/webauthn_setup_controller.rb @@ -143,7 +143,7 @@ def process_valid_webauthn(form) def analytics_properties { in_account_creation_flow: user_session[:in_account_creation_flow] || false, - mfa_attempts: session[:mfa_attempts] || nil, + mfa_attempts: user_session[:mfa_attempts] || nil, } end From 45fd9756942f26eb0afa9088f706a6ffeeecef93 Mon Sep 17 00:00:00 2001 From: Kevin Masters <135744319+kevinsmaster5@users.noreply.github.com> Date: Mon, 7 Oct 2024 10:22:47 -0400 Subject: [PATCH 15/45] equip otp with mfa attempt logging at authentication --- .../otp_verification_controller.rb | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/app/controllers/two_factor_authentication/otp_verification_controller.rb b/app/controllers/two_factor_authentication/otp_verification_controller.rb index c2d01ce2eed..dc63e8d8d36 100644 --- a/app/controllers/two_factor_authentication/otp_verification_controller.rb +++ b/app/controllers/two_factor_authentication/otp_verification_controller.rb @@ -14,6 +14,7 @@ class OtpVerificationController < ApplicationController helper_method :in_multi_mfa_selection_flow? def show + mfa_selection_attempt_count unless UserSessionContext.confirmation_context?(context) analytics.multi_factor_auth_enter_otp_visit(**analytics_properties) @landline_alert = landline_warning? @@ -21,7 +22,7 @@ def show end def create - mfa_selection_attempt_count + mfa_selection_attempt_count if UserSessionContext.confirmation_context?(context) result = otp_verification_form.submit post_analytics(result) @@ -142,7 +143,9 @@ def form_params def post_analytics(result) properties = result.to_h.merge(analytics_properties) analytics.multi_factor_auth_setup(**properties) if context == 'confirmation' - reset_mfa_selection_attempt_count if result.success? + if result.success? && UserSessionContext.confirmation_context?(context) + reset_mfa_selection_attempt_count + end end def analytics_properties From 8dd9cd65948a9215ad1281209ce8e01aa2148f76 Mon Sep 17 00:00:00 2001 From: Kevin Masters <135744319+kevinsmaster5@users.noreply.github.com> Date: Mon, 7 Oct 2024 12:51:08 -0400 Subject: [PATCH 16/45] update spec because of session token change, add attempt count to webauthn verification --- .../webauthn_verification_controller.rb | 2 ++ .../otp_verification_controller_spec.rb | 3 +-- spec/controllers/users/webauthn_setup_controller_spec.rb | 2 +- 3 files changed, 4 insertions(+), 3 deletions(-) diff --git a/app/controllers/two_factor_authentication/webauthn_verification_controller.rb b/app/controllers/two_factor_authentication/webauthn_verification_controller.rb index c58a1967315..f649e89f091 100644 --- a/app/controllers/two_factor_authentication/webauthn_verification_controller.rb +++ b/app/controllers/two_factor_authentication/webauthn_verification_controller.rb @@ -10,6 +10,7 @@ class WebauthnVerificationController < ApplicationController before_action :confirm_webauthn_enabled, only: :show def show + mfa_selection_attempt_count save_challenge_in_session analytics.multi_factor_auth_enter_webauthn_visit(**analytics_properties) @presenter = presenter_for_two_factor_authentication_method @@ -30,6 +31,7 @@ def handle_webauthn_result(result) **analytics_properties, multi_factor_auth_method_created_at: webauthn_configuration_or_latest.created_at.strftime('%s%L'), + mfa_attempts: user_session[:mfa_attempts] || nil, }, ) diff --git a/spec/controllers/two_factor_authentication/otp_verification_controller_spec.rb b/spec/controllers/two_factor_authentication/otp_verification_controller_spec.rb index 97047181008..46939b5fa73 100644 --- a/spec/controllers/two_factor_authentication/otp_verification_controller_spec.rb +++ b/spec/controllers/two_factor_authentication/otp_verification_controller_spec.rb @@ -62,6 +62,7 @@ country_code: parsed_phone.country, phone_fingerprint: Pii::Fingerprinter.fingerprint(parsed_phone.e164), enabled_mfa_methods_count: 1, + mfa_attempts: 1, in_account_creation_flow: false, ) end @@ -152,7 +153,6 @@ phone_fingerprint: Pii::Fingerprinter.fingerprint(parsed_phone.e164), enabled_mfa_methods_count: 1, in_account_creation_flow: false, - mfa_attempts: 1, ) end @@ -235,7 +235,6 @@ phone_fingerprint: Pii::Fingerprinter.fingerprint(parsed_phone.e164), enabled_mfa_methods_count: 1, in_account_creation_flow: false, - mfa_attempts: 1, ) expect(@analytics).to have_logged_event('Multi-Factor Authentication: max attempts reached') end diff --git a/spec/controllers/users/webauthn_setup_controller_spec.rb b/spec/controllers/users/webauthn_setup_controller_spec.rb index 64457048af5..ac8530cfd31 100644 --- a/spec/controllers/users/webauthn_setup_controller_spec.rb +++ b/spec/controllers/users/webauthn_setup_controller_spec.rb @@ -386,7 +386,7 @@ allow(IdentityConfig.store).to receive(:domain_name).and_return('localhost:3000') request.host = 'localhost:3000' controller.user_session[:webauthn_challenge] = webauthn_challenge - session[:mfa_attempts] = 1 + controller.user_session[:mfa_attempts] = 1 end it 'tracks the submission' do From 961f06bc89cc2ccaecf158245d4b63f6370e22d5 Mon Sep 17 00:00:00 2001 From: Kevin Masters <135744319+kevinsmaster5@users.noreply.github.com> Date: Mon, 7 Oct 2024 13:39:56 -0400 Subject: [PATCH 17/45] add mfa attempt count for totp authentication --- app/controllers/concerns/two_factor_authenticatable_methods.rb | 1 + .../two_factor_authentication/totp_verification_controller.rb | 3 ++- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/app/controllers/concerns/two_factor_authenticatable_methods.rb b/app/controllers/concerns/two_factor_authenticatable_methods.rb index 2076d13ac03..3e2773e1085 100644 --- a/app/controllers/concerns/two_factor_authenticatable_methods.rb +++ b/app/controllers/concerns/two_factor_authenticatable_methods.rb @@ -18,6 +18,7 @@ def handle_verification_for_authentication_context(result:, auth_method:, extra_ enabled_mfa_methods_count: mfa_context.enabled_mfa_methods_count, new_device: new_device?, **extra_analytics.to_h, + mfa_attempts: user_session[:mfa_attempts] || nil, ) if result.success? diff --git a/app/controllers/two_factor_authentication/totp_verification_controller.rb b/app/controllers/two_factor_authentication/totp_verification_controller.rb index 72e0983d558..e06625660cc 100644 --- a/app/controllers/two_factor_authentication/totp_verification_controller.rb +++ b/app/controllers/two_factor_authentication/totp_verification_controller.rb @@ -9,6 +9,7 @@ class TotpVerificationController < ApplicationController before_action :confirm_totp_enabled def show + mfa_selection_attempt_count analytics.multi_factor_auth_enter_totp_visit(context: context) @presenter = presenter_for_two_factor_authentication_method @@ -26,7 +27,7 @@ def create result:, auth_method: TwoFactorAuthenticatable::AuthMethod::TOTP, ) - + mfa_selection_attempt_count if result.success? handle_remember_device_preference(params[:remember_device]) redirect_to after_sign_in_path_for(current_user) From ff1564ce00eb310267865ec1c3003df81bc5220e Mon Sep 17 00:00:00 2001 From: Kevin Masters <135744319+kevinsmaster5@users.noreply.github.com> Date: Mon, 7 Oct 2024 13:52:12 -0400 Subject: [PATCH 18/45] add mfa count for piv authentication --- .../piv_cac_verification_controller.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/controllers/two_factor_authentication/piv_cac_verification_controller.rb b/app/controllers/two_factor_authentication/piv_cac_verification_controller.rb index 27ec7494354..4f87e99bff9 100644 --- a/app/controllers/two_factor_authentication/piv_cac_verification_controller.rb +++ b/app/controllers/two_factor_authentication/piv_cac_verification_controller.rb @@ -29,6 +29,7 @@ def redirect_to_piv_cac_service private def process_token + mfa_selection_attempt_count result = piv_cac_verification_form.submit session[:sign_in_flow] = :sign_in @@ -104,6 +105,7 @@ def analytics_properties multi_factor_auth_method: 'piv_cac', piv_cac_configuration_id: piv_cac_verification_form&.piv_cac_configuration&.id, new_device: new_device?, + mfa_attempts: user_session[:mfa_attempts] || nil, } end end From c5d3fbe52388ec45687d955063658d21970d8a4f Mon Sep 17 00:00:00 2001 From: Kevin Masters <135744319+kevinsmaster5@users.noreply.github.com> Date: Mon, 7 Oct 2024 14:12:46 -0400 Subject: [PATCH 19/45] clear user_session token when changing mfa after a failed attempt --- app/controllers/two_factor_authentication/options_controller.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/app/controllers/two_factor_authentication/options_controller.rb b/app/controllers/two_factor_authentication/options_controller.rb index fec6acb3a8d..1cb0eacccc8 100644 --- a/app/controllers/two_factor_authentication/options_controller.rb +++ b/app/controllers/two_factor_authentication/options_controller.rb @@ -26,6 +26,7 @@ class OptionsController < ApplicationController }.freeze def index + reset_mfa_selection_attempt_count @two_factor_options_form = TwoFactorLoginOptionsForm.new(current_user) @presenter = two_factor_options_presenter analytics.multi_factor_auth_option_list_visit From 0572f73f9b06e3ca1a1ca114a5810d51f618f0c7 Mon Sep 17 00:00:00 2001 From: Kevin Masters <135744319+kevinsmaster5@users.noreply.github.com> Date: Mon, 7 Oct 2024 14:29:30 -0400 Subject: [PATCH 20/45] add params to piv analytics event --- app/services/analytics_events.rb | 3 +++ 1 file changed, 3 insertions(+) diff --git a/app/services/analytics_events.rb b/app/services/analytics_events.rb index c39bb917716..4582c842c7b 100644 --- a/app/services/analytics_events.rb +++ b/app/services/analytics_events.rb @@ -5146,6 +5146,7 @@ def multi_factor_auth_enter_personal_key_visit(context:, **extra) # @identity.idp.previous_event_name 'Multi-Factor Authentication: enter PIV CAC visited' # @param ["authentication", "reauthentication", "confirmation"] context User session context # @param ["piv_cac"] multi_factor_auth_method + # @param [Integer] mfa_attempts number of MFA setup attempts # @param [Integer, nil] piv_cac_configuration_id PIV/CAC configuration database ID # @param [Boolean] new_device Whether the user is authenticating from a new device # User used a PIV/CAC as their mfa @@ -5154,6 +5155,7 @@ def multi_factor_auth_enter_piv_cac( multi_factor_auth_method:, piv_cac_configuration_id:, new_device:, + mfa_attempts:, **extra ) track_event( @@ -5162,6 +5164,7 @@ def multi_factor_auth_enter_piv_cac( multi_factor_auth_method: multi_factor_auth_method, piv_cac_configuration_id: piv_cac_configuration_id, new_device:, + mfa_attempts:, **extra, ) end From d5a4d2088e0cfc043c7ea5c894386575bbc84eb6 Mon Sep 17 00:00:00 2001 From: Kevin Masters <135744319+kevinsmaster5@users.noreply.github.com> Date: Mon, 7 Oct 2024 16:01:43 -0400 Subject: [PATCH 21/45] fix piv verification spec. reset mfa account for setup failure --- .../users/two_factor_authentication_setup_controller.rb | 1 + .../piv_cac_verification_controller_spec.rb | 2 ++ 2 files changed, 3 insertions(+) diff --git a/app/controllers/users/two_factor_authentication_setup_controller.rb b/app/controllers/users/two_factor_authentication_setup_controller.rb index 0ef866928b7..dbe1503782b 100644 --- a/app/controllers/users/two_factor_authentication_setup_controller.rb +++ b/app/controllers/users/two_factor_authentication_setup_controller.rb @@ -12,6 +12,7 @@ class TwoFactorAuthenticationSetupController < ApplicationController delegate :enabled_mfa_methods_count, to: :mfa_context def index + reset_mfa_selection_attempt_count two_factor_options_form @presenter = two_factor_options_presenter analytics.user_registration_2fa_setup_visit( diff --git a/spec/controllers/two_factor_authentication/piv_cac_verification_controller_spec.rb b/spec/controllers/two_factor_authentication/piv_cac_verification_controller_spec.rb index d2c04d1ce67..ea163b730f7 100644 --- a/spec/controllers/two_factor_authentication/piv_cac_verification_controller_spec.rb +++ b/spec/controllers/two_factor_authentication/piv_cac_verification_controller_spec.rb @@ -135,6 +135,7 @@ piv_cac_configuration_id: cfg.id, piv_cac_configuration_dn_uuid: cfg.x509_dn_uuid, key_id: 'foo', + mfa_attempts: 1, ) expect(@analytics).to have_logged_event( 'User marked authenticated', @@ -257,6 +258,7 @@ new_device: true, key_id: 'foo', piv_cac_configuration_dn_uuid: 'bad-uuid', + mfa_attempts: 1, ) expect(@analytics).to have_logged_event('Multi-Factor Authentication: max attempts reached') end From 54f296b1b1f2939a12210adbe15bae1c54801624 Mon Sep 17 00:00:00 2001 From: Kevin Masters <135744319+kevinsmaster5@users.noreply.github.com> Date: Tue, 8 Oct 2024 11:45:19 -0400 Subject: [PATCH 22/45] express auth attempts as a hash consisting of attempt count and method --- .../two_factor_authenticatable_methods.rb | 10 ++++- .../otp_verification_controller.rb | 2 +- .../piv_cac_verification_controller.rb | 2 +- .../webauthn_verification_controller.rb | 2 +- ...piv_cac_authentication_setup_controller.rb | 2 +- .../users/totp_setup_controller.rb | 2 +- .../users/webauthn_setup_controller.rb | 2 +- app/services/analytics_events.rb | 12 ++--- .../otp_verification_controller_spec.rb | 25 ++++++++--- .../piv_cac_verification_controller_spec.rb | 10 ++++- ...ac_authentication_setup_controller_spec.rb | 10 ++++- .../users/totp_setup_controller_spec.rb | 45 +++++++++++++++---- .../users/webauthn_setup_controller_spec.rb | 5 ++- 13 files changed, 97 insertions(+), 32 deletions(-) diff --git a/app/controllers/concerns/two_factor_authenticatable_methods.rb b/app/controllers/concerns/two_factor_authenticatable_methods.rb index 3e2773e1085..c872e41f8ed 100644 --- a/app/controllers/concerns/two_factor_authenticatable_methods.rb +++ b/app/controllers/concerns/two_factor_authenticatable_methods.rb @@ -18,7 +18,7 @@ def handle_verification_for_authentication_context(result:, auth_method:, extra_ enabled_mfa_methods_count: mfa_context.enabled_mfa_methods_count, new_device: new_device?, **extra_analytics.to_h, - mfa_attempts: user_session[:mfa_attempts] || nil, + mfa_attempts: mfa_attempts_hash(auth_method), ) if result.success? @@ -134,6 +134,14 @@ def reset_mfa_selection_attempt_count user_session.delete(:mfa_attempts) end + def mfa_attempts_hash(auth_method) + return nil if user_session[:mfa_attempts].nil? + { + attempts: user_session[:mfa_attempts], + auth_method: auth_method, + } + end + # Method will be renamed in the next refactor. # You can pass in any "type" with a corresponding I18n key in # two_factor_authentication.invalid_#{type} diff --git a/app/controllers/two_factor_authentication/otp_verification_controller.rb b/app/controllers/two_factor_authentication/otp_verification_controller.rb index dc63e8d8d36..e465bc0eccb 100644 --- a/app/controllers/two_factor_authentication/otp_verification_controller.rb +++ b/app/controllers/two_factor_authentication/otp_verification_controller.rb @@ -161,7 +161,7 @@ def analytics_properties phone_configuration_id: phone_configuration&.id, in_account_creation_flow: user_session[:in_account_creation_flow] || false, enabled_mfa_methods_count: mfa_context.enabled_mfa_methods_count, - mfa_attempts: user_session[:mfa_attempts] || nil, + mfa_attempts: mfa_attempts_hash('otp'), } end diff --git a/app/controllers/two_factor_authentication/piv_cac_verification_controller.rb b/app/controllers/two_factor_authentication/piv_cac_verification_controller.rb index 4f87e99bff9..ea41a3402ec 100644 --- a/app/controllers/two_factor_authentication/piv_cac_verification_controller.rb +++ b/app/controllers/two_factor_authentication/piv_cac_verification_controller.rb @@ -105,7 +105,7 @@ def analytics_properties multi_factor_auth_method: 'piv_cac', piv_cac_configuration_id: piv_cac_verification_form&.piv_cac_configuration&.id, new_device: new_device?, - mfa_attempts: user_session[:mfa_attempts] || nil, + mfa_attempts: mfa_attempts_hash('piv_cac'), } end end diff --git a/app/controllers/two_factor_authentication/webauthn_verification_controller.rb b/app/controllers/two_factor_authentication/webauthn_verification_controller.rb index f649e89f091..71cd2a4784b 100644 --- a/app/controllers/two_factor_authentication/webauthn_verification_controller.rb +++ b/app/controllers/two_factor_authentication/webauthn_verification_controller.rb @@ -31,7 +31,7 @@ def handle_webauthn_result(result) **analytics_properties, multi_factor_auth_method_created_at: webauthn_configuration_or_latest.created_at.strftime('%s%L'), - mfa_attempts: user_session[:mfa_attempts] || nil, + mfa_attempts: mfa_attempts_hash('webauthn'), }, ) diff --git a/app/controllers/users/piv_cac_authentication_setup_controller.rb b/app/controllers/users/piv_cac_authentication_setup_controller.rb index b854e192832..1da8ee73dcd 100644 --- a/app/controllers/users/piv_cac_authentication_setup_controller.rb +++ b/app/controllers/users/piv_cac_authentication_setup_controller.rb @@ -128,7 +128,7 @@ def analytics_properties { in_account_creation_flow: user_session[:in_account_creation_flow] || false, enabled_mfa_methods_count: mfa_context.enabled_mfa_methods_count, - mfa_attempts: user_session[:mfa_attempts] || nil, + mfa_attempts: mfa_attempts_hash('piv_cac'), } end diff --git a/app/controllers/users/totp_setup_controller.rb b/app/controllers/users/totp_setup_controller.rb index 5956d61776a..52bf36de35c 100644 --- a/app/controllers/users/totp_setup_controller.rb +++ b/app/controllers/users/totp_setup_controller.rb @@ -119,7 +119,7 @@ def analytics_properties { in_account_creation_flow: in_account_creation_flow?, pii_like_keypaths: [[:mfa_method_counts, :phone]], - mfa_attempts: user_session[:mfa_attempts] || nil, + mfa_attempts: mfa_attempts_hash('totp'), } end end diff --git a/app/controllers/users/webauthn_setup_controller.rb b/app/controllers/users/webauthn_setup_controller.rb index 7c17ece08a2..a919548e592 100644 --- a/app/controllers/users/webauthn_setup_controller.rb +++ b/app/controllers/users/webauthn_setup_controller.rb @@ -143,7 +143,7 @@ def process_valid_webauthn(form) def analytics_properties { in_account_creation_flow: user_session[:in_account_creation_flow] || false, - mfa_attempts: user_session[:mfa_attempts] || nil, + mfa_attempts: mfa_attempts_hash('webauthn'), } end diff --git a/app/services/analytics_events.rb b/app/services/analytics_events.rb index 4582c842c7b..03a25ab2a74 100644 --- a/app/services/analytics_events.rb +++ b/app/services/analytics_events.rb @@ -4945,7 +4945,7 @@ def logout_initiated( # @param [Boolean] new_device Whether the user is authenticating from a new device # @param [String] multi_factor_auth_method Authentication method used # @param [String] multi_factor_auth_method_created_at When the authentication method was created - # @param [Integer] mfa_attempts number of MFA setup attempts + # @param [Hash] mfa_attempts number of MFA setup attempts # @param [Integer] auth_app_configuration_id Database ID of authentication app configuration # @param [Integer] piv_cac_configuration_id Database ID of PIV/CAC configuration # @param [String] piv_cac_configuration_dn_uuid PIV/CAC X509 distinguished name UUID @@ -5041,7 +5041,7 @@ def multi_factor_auth_added_phone( # @param [Integer] enabled_mfa_methods_count Number of enabled MFA methods on the account # @param [Boolean] in_account_creation_flow whether user is going through creation flow # @param ['piv_cac'] method_name Authentication method added - # @param [Integer] mfa_attempts number of MFA setup attempts + # @param [Hash] mfa_attempts number of MFA setup attempts def multi_factor_auth_added_piv_cac( enabled_mfa_methods_count:, in_account_creation_flow:, @@ -5094,7 +5094,7 @@ def multi_factor_auth_enter_backup_code_visit(context:, **extra) end # @param ["authentication", "reauthentication", "confirmation"] context User session context - # @param [Integer] mfa_attempts number of MFA setup attempts + # @param [Hash] mfa_attempts number of MFA setup attempts # @param [String] multi_factor_auth_method # @param [Boolean] confirmation_for_add_phone # @param [Integer] phone_configuration_id @@ -5146,7 +5146,7 @@ def multi_factor_auth_enter_personal_key_visit(context:, **extra) # @identity.idp.previous_event_name 'Multi-Factor Authentication: enter PIV CAC visited' # @param ["authentication", "reauthentication", "confirmation"] context User session context # @param ["piv_cac"] multi_factor_auth_method - # @param [Integer] mfa_attempts number of MFA setup attempts + # @param [Hash] mfa_attempts number of MFA setup attempts # @param [Integer, nil] piv_cac_configuration_id PIV/CAC configuration database ID # @param [Boolean] new_device Whether the user is authenticating from a new device # User used a PIV/CAC as their mfa @@ -5299,7 +5299,7 @@ def multi_factor_auth_phone_setup( # @param [String, nil] key_id PIV/CAC key_id from PKI service # @param [Hash] mfa_method_counts Hash of MFA method with the number of that method on the account # @param [Hash] authenticator_data_flags WebAuthn authenticator data flags - # @param [Integer] mfa_attempts number of MFA setup attempts + # @param [Hash] mfa_attempts number of MFA setup attempts # @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. @@ -5999,7 +5999,7 @@ def piv_cac_recommended_visited # Tracks when user's piv cac setup # @param [Boolean] in_account_creation_flow Whether user is going through account creation # @param [Integer] enabled_mfa_methods_count Number of enabled MFA methods on the account - # @param [Integer] mfa_attempts number of MFA setup attempts + # @param [Hash] mfa_attempts number of MFA setup attempts def piv_cac_setup_visited( in_account_creation_flow:, enabled_mfa_methods_count: nil, diff --git a/spec/controllers/two_factor_authentication/otp_verification_controller_spec.rb b/spec/controllers/two_factor_authentication/otp_verification_controller_spec.rb index 46939b5fa73..db42a74dc88 100644 --- a/spec/controllers/two_factor_authentication/otp_verification_controller_spec.rb +++ b/spec/controllers/two_factor_authentication/otp_verification_controller_spec.rb @@ -62,7 +62,10 @@ country_code: parsed_phone.country, phone_fingerprint: Pii::Fingerprinter.fingerprint(parsed_phone.e164), enabled_mfa_methods_count: 1, - mfa_attempts: 1, + mfa_attempts: { + attempts: 1, + auth_method: 'otp', + }, in_account_creation_flow: false, ) end @@ -536,7 +539,10 @@ phone_fingerprint: Pii::Fingerprinter.fingerprint(parsed_phone.e164), enabled_mfa_methods_count: 1, in_account_creation_flow: true, - mfa_attempts: 1, + mfa_attempts: { + attempts: 1, + auth_method: 'otp', + }, ) end @@ -606,7 +612,10 @@ phone_fingerprint: Pii::Fingerprinter.fingerprint(parsed_phone.e164), enabled_mfa_methods_count: 1, in_account_creation_flow: false, - mfa_attempts: 1, + mfa_attempts: { + attempts: 1, + auth_method: 'otp', + }, ) end @@ -648,7 +657,10 @@ phone_fingerprint: Pii::Fingerprinter.fingerprint(parsed_phone.e164), enabled_mfa_methods_count: 1, in_account_creation_flow: false, - mfa_attempts: 2, + mfa_attempts: { + attempts: 2, + auth_method: 'otp', + }, ) end end @@ -699,7 +711,10 @@ phone_fingerprint: Pii::Fingerprinter.fingerprint(parsed_phone.e164), enabled_mfa_methods_count: 0, in_account_creation_flow: false, - mfa_attempts: 1, + mfa_attempts: { + attempts: 1, + auth_method: 'otp', + }, ) expect(controller).to have_received(:create_user_event).with(:phone_confirmed) diff --git a/spec/controllers/two_factor_authentication/piv_cac_verification_controller_spec.rb b/spec/controllers/two_factor_authentication/piv_cac_verification_controller_spec.rb index ea163b730f7..a9af723559b 100644 --- a/spec/controllers/two_factor_authentication/piv_cac_verification_controller_spec.rb +++ b/spec/controllers/two_factor_authentication/piv_cac_verification_controller_spec.rb @@ -135,7 +135,10 @@ piv_cac_configuration_id: cfg.id, piv_cac_configuration_dn_uuid: cfg.x509_dn_uuid, key_id: 'foo', - mfa_attempts: 1, + mfa_attempts: { + attempts: 1, + auth_method: 'piv_cac', + }, ) expect(@analytics).to have_logged_event( 'User marked authenticated', @@ -258,7 +261,10 @@ new_device: true, key_id: 'foo', piv_cac_configuration_dn_uuid: 'bad-uuid', - mfa_attempts: 1, + mfa_attempts: { + attempts: 1, + auth_method: 'piv_cac', + }, ) expect(@analytics).to have_logged_event('Multi-Factor Authentication: max attempts reached') end 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 b2e4c30ee8f..39d33d16ce6 100644 --- a/spec/controllers/users/piv_cac_authentication_setup_controller_spec.rb +++ b/spec/controllers/users/piv_cac_authentication_setup_controller_spec.rb @@ -120,7 +120,10 @@ multi_factor_auth_method: 'piv_cac', in_account_creation_flow: false, success: true, - mfa_attempts: 1, + mfa_attempts: { + attempts: 1, + auth_method: 'piv_cac', + }, ) end @@ -136,7 +139,10 @@ multi_factor_auth_method: 'piv_cac', in_account_creation_flow: false, success: true, - mfa_attempts: 2, + mfa_attempts: { + attempts: 2, + auth_method: 'piv_cac', + }, ) end diff --git a/spec/controllers/users/totp_setup_controller_spec.rb b/spec/controllers/users/totp_setup_controller_spec.rb index 07d34a58759..b5e83dd98c0 100644 --- a/spec/controllers/users/totp_setup_controller_spec.rb +++ b/spec/controllers/users/totp_setup_controller_spec.rb @@ -108,7 +108,10 @@ multi_factor_auth_method: 'totp', enabled_mfa_methods_count: 0, in_account_creation_flow: false, - mfa_attempts: 1, + mfa_attempts: { + attempts: 1, + auth_method: 'totp', + }, ) end end @@ -137,7 +140,10 @@ auth_app_configuration_id: next_auth_app_id, enabled_mfa_methods_count: 2, in_account_creation_flow: false, - mfa_attempts: 1, + mfa_attempts: { + attempts: 1, + auth_method: 'totp', + }, ) end end @@ -168,7 +174,10 @@ auth_app_configuration_id: next_auth_app_id, enabled_mfa_methods_count: 2, in_account_creation_flow: false, - mfa_attempts: 2, + mfa_attempts: { + attempts: 2, + auth_method: 'totp', + }, ) end end @@ -197,7 +206,10 @@ multi_factor_auth_method: 'totp', enabled_mfa_methods_count: 1, in_account_creation_flow: false, - mfa_attempts: 1, + mfa_attempts: { + attempts: 1, + auth_method: 'totp', + }, ) end end @@ -227,7 +239,10 @@ multi_factor_auth_method: 'totp', enabled_mfa_methods_count: 1, in_account_creation_flow: false, - mfa_attempts: 1, + mfa_attempts: { + attempts: 1, + auth_method: 'totp', + }, ) end end @@ -256,7 +271,10 @@ multi_factor_auth_method: 'totp', enabled_mfa_methods_count: 0, in_account_creation_flow: false, - mfa_attempts: 1, + mfa_attempts: { + attempts: 1, + auth_method: 'totp', + }, ) end end @@ -288,7 +306,10 @@ auth_app_configuration_id: next_auth_app_id, enabled_mfa_methods_count: 1, in_account_creation_flow: true, - mfa_attempts: 1, + mfa_attempts: { + attempts: 1, + auth_method: 'totp', + }, ) end end @@ -308,7 +329,10 @@ auth_app_configuration_id: next_auth_app_id, enabled_mfa_methods_count: 1, in_account_creation_flow: true, - mfa_attempts: 1, + mfa_attempts: { + attempts: 1, + auth_method: 'totp', + }, ) end end @@ -335,7 +359,10 @@ multi_factor_auth_method: 'totp', enabled_mfa_methods_count: 0, in_account_creation_flow: false, - mfa_attempts: 1, + mfa_attempts: { + attempts: 1, + auth_method: 'totp', + }, ) end end diff --git a/spec/controllers/users/webauthn_setup_controller_spec.rb b/spec/controllers/users/webauthn_setup_controller_spec.rb index ac8530cfd31..f0d941d5ecd 100644 --- a/spec/controllers/users/webauthn_setup_controller_spec.rb +++ b/spec/controllers/users/webauthn_setup_controller_spec.rb @@ -412,7 +412,10 @@ at: true, ed: false, }, - mfa_attempts: 1, + mfa_attempts: { + attempts: 1, + auth_method: 'webauthn', + }, ) expect(@analytics).to have_logged_event( :webauthn_setup_submitted, From f57e9cd79f4419d3b7a093bc060eab512864794c Mon Sep 17 00:00:00 2001 From: Kevin Masters <135744319+kevinsmaster5@users.noreply.github.com> Date: Thu, 10 Oct 2024 15:27:18 -0400 Subject: [PATCH 23/45] group all attempts into a hash --- .../concerns/two_factor_authenticatable_methods.rb | 11 +++++++---- .../two_factor_authentication/options_controller.rb | 1 - .../otp_verification_controller.rb | 9 +++------ .../piv_cac_verification_controller.rb | 2 -- .../totp_verification_controller.rb | 2 -- .../webauthn_verification_controller.rb | 2 -- .../users/piv_cac_authentication_setup_controller.rb | 6 +++--- app/controllers/users/totp_setup_controller.rb | 6 +++--- .../two_factor_authentication_setup_controller.rb | 1 - app/controllers/users/webauthn_setup_controller.rb | 6 +++--- app/services/analytics_events.rb | 3 --- 11 files changed, 19 insertions(+), 30 deletions(-) diff --git a/app/controllers/concerns/two_factor_authenticatable_methods.rb b/app/controllers/concerns/two_factor_authenticatable_methods.rb index c872e41f8ed..44bef50559d 100644 --- a/app/controllers/concerns/two_factor_authenticatable_methods.rb +++ b/app/controllers/concerns/two_factor_authenticatable_methods.rb @@ -12,17 +12,19 @@ def auth_methods_session end def handle_verification_for_authentication_context(result:, auth_method:, extra_analytics: nil) + mfa_selection_attempt_count(auth_method) analytics.multi_factor_auth( **result.to_h, multi_factor_auth_method: auth_method, enabled_mfa_methods_count: mfa_context.enabled_mfa_methods_count, new_device: new_device?, **extra_analytics.to_h, - mfa_attempts: mfa_attempts_hash(auth_method), + mfa_attempts: user_session[:mfa_attempts], ) if result.success? handle_valid_verification_for_authentication_context(auth_method:) + user_session.delete(:mfa_attempts) else handle_invalid_verification_for_authentication_context end @@ -125,9 +127,10 @@ def handle_remember_device_preference(remember_device_preference) save_remember_device_preference(remember_device_preference) end - def mfa_selection_attempt_count - user_session[:mfa_attempts] ||= 0 - user_session[:mfa_attempts] += 1 + def mfa_selection_attempt_count(auth_method) + user_session[:mfa_attempts] ||= {} + attempt = { auth_method.to_sym => 1 } + user_session[:mfa_attempts].merge!(attempt) { |_key, old_val, new_val| old_val + new_val } end def reset_mfa_selection_attempt_count diff --git a/app/controllers/two_factor_authentication/options_controller.rb b/app/controllers/two_factor_authentication/options_controller.rb index 1cb0eacccc8..fec6acb3a8d 100644 --- a/app/controllers/two_factor_authentication/options_controller.rb +++ b/app/controllers/two_factor_authentication/options_controller.rb @@ -26,7 +26,6 @@ class OptionsController < ApplicationController }.freeze def index - reset_mfa_selection_attempt_count @two_factor_options_form = TwoFactorLoginOptionsForm.new(current_user) @presenter = two_factor_options_presenter analytics.multi_factor_auth_option_list_visit diff --git a/app/controllers/two_factor_authentication/otp_verification_controller.rb b/app/controllers/two_factor_authentication/otp_verification_controller.rb index e465bc0eccb..8d21e41e88a 100644 --- a/app/controllers/two_factor_authentication/otp_verification_controller.rb +++ b/app/controllers/two_factor_authentication/otp_verification_controller.rb @@ -14,7 +14,6 @@ class OtpVerificationController < ApplicationController helper_method :in_multi_mfa_selection_flow? def show - mfa_selection_attempt_count unless UserSessionContext.confirmation_context?(context) analytics.multi_factor_auth_enter_otp_visit(**analytics_properties) @landline_alert = landline_warning? @@ -22,7 +21,7 @@ def show end def create - mfa_selection_attempt_count if UserSessionContext.confirmation_context?(context) + mfa_selection_attempt_count('otp') result = otp_verification_form.submit post_analytics(result) @@ -44,6 +43,7 @@ def create end reset_otp_session_data + user_session.delete(:mfa_attempts) else handle_invalid_otp(type: 'otp') end @@ -143,9 +143,6 @@ def form_params def post_analytics(result) properties = result.to_h.merge(analytics_properties) analytics.multi_factor_auth_setup(**properties) if context == 'confirmation' - if result.success? && UserSessionContext.confirmation_context?(context) - reset_mfa_selection_attempt_count - end end def analytics_properties @@ -161,7 +158,7 @@ def analytics_properties phone_configuration_id: phone_configuration&.id, in_account_creation_flow: user_session[:in_account_creation_flow] || false, enabled_mfa_methods_count: mfa_context.enabled_mfa_methods_count, - mfa_attempts: mfa_attempts_hash('otp'), + mfa_attempts: user_session[:mfa_attempts], } end diff --git a/app/controllers/two_factor_authentication/piv_cac_verification_controller.rb b/app/controllers/two_factor_authentication/piv_cac_verification_controller.rb index ea41a3402ec..27ec7494354 100644 --- a/app/controllers/two_factor_authentication/piv_cac_verification_controller.rb +++ b/app/controllers/two_factor_authentication/piv_cac_verification_controller.rb @@ -29,7 +29,6 @@ def redirect_to_piv_cac_service private def process_token - mfa_selection_attempt_count result = piv_cac_verification_form.submit session[:sign_in_flow] = :sign_in @@ -105,7 +104,6 @@ def analytics_properties multi_factor_auth_method: 'piv_cac', piv_cac_configuration_id: piv_cac_verification_form&.piv_cac_configuration&.id, new_device: new_device?, - mfa_attempts: mfa_attempts_hash('piv_cac'), } end end diff --git a/app/controllers/two_factor_authentication/totp_verification_controller.rb b/app/controllers/two_factor_authentication/totp_verification_controller.rb index e06625660cc..96aafe9965a 100644 --- a/app/controllers/two_factor_authentication/totp_verification_controller.rb +++ b/app/controllers/two_factor_authentication/totp_verification_controller.rb @@ -9,7 +9,6 @@ class TotpVerificationController < ApplicationController before_action :confirm_totp_enabled def show - mfa_selection_attempt_count analytics.multi_factor_auth_enter_totp_visit(context: context) @presenter = presenter_for_two_factor_authentication_method @@ -27,7 +26,6 @@ def create result:, auth_method: TwoFactorAuthenticatable::AuthMethod::TOTP, ) - mfa_selection_attempt_count if result.success? handle_remember_device_preference(params[:remember_device]) redirect_to after_sign_in_path_for(current_user) diff --git a/app/controllers/two_factor_authentication/webauthn_verification_controller.rb b/app/controllers/two_factor_authentication/webauthn_verification_controller.rb index 71cd2a4784b..c58a1967315 100644 --- a/app/controllers/two_factor_authentication/webauthn_verification_controller.rb +++ b/app/controllers/two_factor_authentication/webauthn_verification_controller.rb @@ -10,7 +10,6 @@ class WebauthnVerificationController < ApplicationController before_action :confirm_webauthn_enabled, only: :show def show - mfa_selection_attempt_count save_challenge_in_session analytics.multi_factor_auth_enter_webauthn_visit(**analytics_properties) @presenter = presenter_for_two_factor_authentication_method @@ -31,7 +30,6 @@ def handle_webauthn_result(result) **analytics_properties, multi_factor_auth_method_created_at: webauthn_configuration_or_latest.created_at.strftime('%s%L'), - mfa_attempts: mfa_attempts_hash('webauthn'), }, ) diff --git a/app/controllers/users/piv_cac_authentication_setup_controller.rb b/app/controllers/users/piv_cac_authentication_setup_controller.rb index 1da8ee73dcd..2ff52ccdbae 100644 --- a/app/controllers/users/piv_cac_authentication_setup_controller.rb +++ b/app/controllers/users/piv_cac_authentication_setup_controller.rb @@ -66,13 +66,13 @@ def piv_cac_service_url_with_redirect end def process_piv_cac_setup - mfa_selection_attempt_count + mfa_selection_attempt_count('piv_cac') result = user_piv_cac_form.submit properties = result.to_h.merge(analytics_properties) analytics.multi_factor_auth_setup(**properties) if result.success? process_valid_submission - reset_mfa_selection_attempt_count + user_session.delete(:mfa_attempts) else process_invalid_submission end @@ -128,7 +128,7 @@ def analytics_properties { in_account_creation_flow: user_session[:in_account_creation_flow] || false, enabled_mfa_methods_count: mfa_context.enabled_mfa_methods_count, - mfa_attempts: mfa_attempts_hash('piv_cac'), + mfa_attempts: user_session[:mfa_attempts], } end diff --git a/app/controllers/users/totp_setup_controller.rb b/app/controllers/users/totp_setup_controller.rb index 52bf36de35c..6732fac8899 100644 --- a/app/controllers/users/totp_setup_controller.rb +++ b/app/controllers/users/totp_setup_controller.rb @@ -26,13 +26,13 @@ def new def confirm result = totp_setup_form.submit - mfa_selection_attempt_count + mfa_selection_attempt_count('totp') properties = result.to_h.merge(analytics_properties) analytics.multi_factor_auth_setup(**properties) if result.success? process_valid_code - reset_mfa_selection_attempt_count + user_session.delete(:mfa_attempts) else process_invalid_code end @@ -119,7 +119,7 @@ def analytics_properties { in_account_creation_flow: in_account_creation_flow?, pii_like_keypaths: [[:mfa_method_counts, :phone]], - mfa_attempts: mfa_attempts_hash('totp'), + mfa_attempts: user_session[:mfa_attempts], } 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 dbe1503782b..0ef866928b7 100644 --- a/app/controllers/users/two_factor_authentication_setup_controller.rb +++ b/app/controllers/users/two_factor_authentication_setup_controller.rb @@ -12,7 +12,6 @@ class TwoFactorAuthenticationSetupController < ApplicationController delegate :enabled_mfa_methods_count, to: :mfa_context def index - reset_mfa_selection_attempt_count two_factor_options_form @presenter = two_factor_options_presenter analytics.user_registration_2fa_setup_visit( diff --git a/app/controllers/users/webauthn_setup_controller.rb b/app/controllers/users/webauthn_setup_controller.rb index a919548e592..78eafcf5acc 100644 --- a/app/controllers/users/webauthn_setup_controller.rb +++ b/app/controllers/users/webauthn_setup_controller.rb @@ -51,7 +51,7 @@ def new end flash_error(result.errors) unless result.success? - mfa_selection_attempt_count + mfa_selection_attempt_count('webauthn') end def confirm @@ -75,7 +75,7 @@ def confirm if result.success? process_valid_webauthn(form) - reset_mfa_selection_attempt_count + user_session.delete(:mfa_attempts) else flash.now[:error] = result.first_error_message render :new @@ -143,7 +143,7 @@ def process_valid_webauthn(form) def analytics_properties { in_account_creation_flow: user_session[:in_account_creation_flow] || false, - mfa_attempts: mfa_attempts_hash('webauthn'), + mfa_attempts: user_session[:mfa_attempts], } end diff --git a/app/services/analytics_events.rb b/app/services/analytics_events.rb index 03a25ab2a74..3cf1c999126 100644 --- a/app/services/analytics_events.rb +++ b/app/services/analytics_events.rb @@ -5146,7 +5146,6 @@ def multi_factor_auth_enter_personal_key_visit(context:, **extra) # @identity.idp.previous_event_name 'Multi-Factor Authentication: enter PIV CAC visited' # @param ["authentication", "reauthentication", "confirmation"] context User session context # @param ["piv_cac"] multi_factor_auth_method - # @param [Hash] mfa_attempts number of MFA setup attempts # @param [Integer, nil] piv_cac_configuration_id PIV/CAC configuration database ID # @param [Boolean] new_device Whether the user is authenticating from a new device # User used a PIV/CAC as their mfa @@ -5155,7 +5154,6 @@ def multi_factor_auth_enter_piv_cac( multi_factor_auth_method:, piv_cac_configuration_id:, new_device:, - mfa_attempts:, **extra ) track_event( @@ -5164,7 +5162,6 @@ def multi_factor_auth_enter_piv_cac( multi_factor_auth_method: multi_factor_auth_method, piv_cac_configuration_id: piv_cac_configuration_id, new_device:, - mfa_attempts:, **extra, ) end From a692ecfdb433be40e4877840cd0b878af2a02d4f Mon Sep 17 00:00:00 2001 From: Kevin Masters <135744319+kevinsmaster5@users.noreply.github.com> Date: Thu, 10 Oct 2024 17:00:18 -0400 Subject: [PATCH 24/45] sync rspec up with changes made --- .../two_factor_authenticatable_methods.rb | 2 +- .../otp_verification_controller_spec.rb | 32 ++++++++++++------- .../piv_cac_verification_controller_spec.rb | 6 ++-- ...ac_authentication_setup_controller_spec.rb | 6 ++-- .../users/totp_setup_controller_spec.rb | 27 ++++++---------- .../users/webauthn_setup_controller_spec.rb | 3 +- 6 files changed, 35 insertions(+), 41 deletions(-) diff --git a/app/controllers/concerns/two_factor_authenticatable_methods.rb b/app/controllers/concerns/two_factor_authenticatable_methods.rb index 44bef50559d..5e142bb156c 100644 --- a/app/controllers/concerns/two_factor_authenticatable_methods.rb +++ b/app/controllers/concerns/two_factor_authenticatable_methods.rb @@ -129,7 +129,7 @@ def handle_remember_device_preference(remember_device_preference) def mfa_selection_attempt_count(auth_method) user_session[:mfa_attempts] ||= {} - attempt = { auth_method.to_sym => 1 } + attempt = { auth_method => 1 } user_session[:mfa_attempts].merge!(attempt) { |_key, old_val, new_val| old_val + new_val } end diff --git a/spec/controllers/two_factor_authentication/otp_verification_controller_spec.rb b/spec/controllers/two_factor_authentication/otp_verification_controller_spec.rb index db42a74dc88..90bd7697433 100644 --- a/spec/controllers/two_factor_authentication/otp_verification_controller_spec.rb +++ b/spec/controllers/two_factor_authentication/otp_verification_controller_spec.rb @@ -62,10 +62,6 @@ country_code: parsed_phone.country, phone_fingerprint: Pii::Fingerprinter.fingerprint(parsed_phone.e164), enabled_mfa_methods_count: 1, - mfa_attempts: { - attempts: 1, - auth_method: 'otp', - }, in_account_creation_flow: false, ) end @@ -156,6 +152,10 @@ phone_fingerprint: Pii::Fingerprinter.fingerprint(parsed_phone.e164), enabled_mfa_methods_count: 1, in_account_creation_flow: false, + mfa_attempts: { + 'otp' => 1, + 'sms' => 1, + }, ) end @@ -238,6 +238,10 @@ phone_fingerprint: Pii::Fingerprinter.fingerprint(parsed_phone.e164), enabled_mfa_methods_count: 1, in_account_creation_flow: false, + mfa_attempts: { + 'otp' => 1, + 'sms' => 1, + }, ) expect(@analytics).to have_logged_event('Multi-Factor Authentication: max attempts reached') end @@ -308,6 +312,10 @@ phone_fingerprint: Pii::Fingerprinter.fingerprint(parsed_phone.e164), enabled_mfa_methods_count: 1, in_account_creation_flow: false, + mfa_attempts: { + 'otp' => 1, + 'sms' => 1, + }, ) expect(@analytics).to have_logged_event( 'User marked authenticated', @@ -355,6 +363,10 @@ phone_fingerprint: Pii::Fingerprinter.fingerprint(parsed_phone.e164), enabled_mfa_methods_count: 1, in_account_creation_flow: false, + mfa_attempts: { + 'otp' => 1, + 'sms' => 1, + }, ) expect(@analytics).to have_logged_event( 'User marked authenticated', @@ -540,8 +552,7 @@ enabled_mfa_methods_count: 1, in_account_creation_flow: true, mfa_attempts: { - attempts: 1, - auth_method: 'otp', + 'otp' => 1, }, ) end @@ -613,8 +624,7 @@ enabled_mfa_methods_count: 1, in_account_creation_flow: false, mfa_attempts: { - attempts: 1, - auth_method: 'otp', + 'otp' => 1, }, ) end @@ -658,8 +668,7 @@ enabled_mfa_methods_count: 1, in_account_creation_flow: false, mfa_attempts: { - attempts: 2, - auth_method: 'otp', + 'otp' => 3, }, ) end @@ -712,8 +721,7 @@ enabled_mfa_methods_count: 0, in_account_creation_flow: false, mfa_attempts: { - attempts: 1, - auth_method: 'otp', + 'otp' => 1, }, ) diff --git a/spec/controllers/two_factor_authentication/piv_cac_verification_controller_spec.rb b/spec/controllers/two_factor_authentication/piv_cac_verification_controller_spec.rb index a9af723559b..a3ad89d5994 100644 --- a/spec/controllers/two_factor_authentication/piv_cac_verification_controller_spec.rb +++ b/spec/controllers/two_factor_authentication/piv_cac_verification_controller_spec.rb @@ -136,8 +136,7 @@ piv_cac_configuration_dn_uuid: cfg.x509_dn_uuid, key_id: 'foo', mfa_attempts: { - attempts: 1, - auth_method: 'piv_cac', + 'piv_cac' => 1, }, ) expect(@analytics).to have_logged_event( @@ -262,8 +261,7 @@ key_id: 'foo', piv_cac_configuration_dn_uuid: 'bad-uuid', mfa_attempts: { - attempts: 1, - auth_method: 'piv_cac', + 'piv_cac' => 1, }, ) expect(@analytics).to have_logged_event('Multi-Factor Authentication: max attempts reached') 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 39d33d16ce6..a35df9ea97e 100644 --- a/spec/controllers/users/piv_cac_authentication_setup_controller_spec.rb +++ b/spec/controllers/users/piv_cac_authentication_setup_controller_spec.rb @@ -121,8 +121,7 @@ in_account_creation_flow: false, success: true, mfa_attempts: { - attempts: 1, - auth_method: 'piv_cac', + 'piv_cac' => 1, }, ) end @@ -140,8 +139,7 @@ in_account_creation_flow: false, success: true, mfa_attempts: { - attempts: 2, - auth_method: 'piv_cac', + 'piv_cac' => 2, }, ) end diff --git a/spec/controllers/users/totp_setup_controller_spec.rb b/spec/controllers/users/totp_setup_controller_spec.rb index b5e83dd98c0..c7cd2fa54e3 100644 --- a/spec/controllers/users/totp_setup_controller_spec.rb +++ b/spec/controllers/users/totp_setup_controller_spec.rb @@ -109,8 +109,7 @@ enabled_mfa_methods_count: 0, in_account_creation_flow: false, mfa_attempts: { - attempts: 1, - auth_method: 'totp', + 'totp' => 1, }, ) end @@ -141,8 +140,7 @@ enabled_mfa_methods_count: 2, in_account_creation_flow: false, mfa_attempts: { - attempts: 1, - auth_method: 'totp', + 'totp' => 1, }, ) end @@ -175,8 +173,7 @@ enabled_mfa_methods_count: 2, in_account_creation_flow: false, mfa_attempts: { - attempts: 2, - auth_method: 'totp', + 'totp' => 2, }, ) end @@ -207,8 +204,7 @@ enabled_mfa_methods_count: 1, in_account_creation_flow: false, mfa_attempts: { - attempts: 1, - auth_method: 'totp', + 'totp' => 1, }, ) end @@ -240,8 +236,7 @@ enabled_mfa_methods_count: 1, in_account_creation_flow: false, mfa_attempts: { - attempts: 1, - auth_method: 'totp', + 'totp' => 1, }, ) end @@ -272,8 +267,7 @@ enabled_mfa_methods_count: 0, in_account_creation_flow: false, mfa_attempts: { - attempts: 1, - auth_method: 'totp', + 'totp' => 1, }, ) end @@ -307,8 +301,7 @@ enabled_mfa_methods_count: 1, in_account_creation_flow: true, mfa_attempts: { - attempts: 1, - auth_method: 'totp', + 'totp' => 1, }, ) end @@ -330,8 +323,7 @@ enabled_mfa_methods_count: 1, in_account_creation_flow: true, mfa_attempts: { - attempts: 1, - auth_method: 'totp', + 'totp' => 1, }, ) end @@ -360,8 +352,7 @@ enabled_mfa_methods_count: 0, in_account_creation_flow: false, mfa_attempts: { - attempts: 1, - auth_method: 'totp', + 'totp' => 1, }, ) end diff --git a/spec/controllers/users/webauthn_setup_controller_spec.rb b/spec/controllers/users/webauthn_setup_controller_spec.rb index f0d941d5ecd..c3568d3c065 100644 --- a/spec/controllers/users/webauthn_setup_controller_spec.rb +++ b/spec/controllers/users/webauthn_setup_controller_spec.rb @@ -413,8 +413,7 @@ ed: false, }, mfa_attempts: { - attempts: 1, - auth_method: 'webauthn', + 'webauthn' => 1, }, ) expect(@analytics).to have_logged_event( From ab1fe0ec35ea65d543ab7b0dc2b24f856ae40f02 Mon Sep 17 00:00:00 2001 From: Kevin Masters <135744319+kevinsmaster5@users.noreply.github.com> Date: Thu, 10 Oct 2024 17:07:18 -0400 Subject: [PATCH 25/45] add mfa attempt to event expectation --- .../totp_verification_controller_spec.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/spec/controllers/two_factor_authentication/totp_verification_controller_spec.rb b/spec/controllers/two_factor_authentication/totp_verification_controller_spec.rb index 8d273b1df37..5fa7b9476d8 100644 --- a/spec/controllers/two_factor_authentication/totp_verification_controller_spec.rb +++ b/spec/controllers/two_factor_authentication/totp_verification_controller_spec.rb @@ -59,6 +59,7 @@ multi_factor_auth_method_created_at: cfg.created_at.strftime('%s%L'), new_device: true, auth_app_configuration_id: controller.current_user.auth_app_configurations.first.id, + mfa_attempts: { 'totp' => 1 }, ) expect(@analytics).to have_logged_event( 'User marked authenticated', @@ -177,6 +178,7 @@ enabled_mfa_methods_count: 2, multi_factor_auth_method: 'totp', new_device: true, + mfa_attempts: { 'totp' => 1 }, ) expect(@analytics).to have_logged_event('Multi-Factor Authentication: max attempts reached') end From 5d92095bc8fababbdd69fa1d8a6dda7fd888d031 Mon Sep 17 00:00:00 2001 From: Kevin Masters <135744319+kevinsmaster5@users.noreply.github.com> Date: Thu, 10 Oct 2024 17:15:18 -0400 Subject: [PATCH 26/45] add mfa attempt to event expectation --- .../webauthn_verification_controller_spec.rb | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/spec/controllers/two_factor_authentication/webauthn_verification_controller_spec.rb b/spec/controllers/two_factor_authentication/webauthn_verification_controller_spec.rb index 0c8732018ef..d2854bc6576 100644 --- a/spec/controllers/two_factor_authentication/webauthn_verification_controller_spec.rb +++ b/spec/controllers/two_factor_authentication/webauthn_verification_controller_spec.rb @@ -158,6 +158,7 @@ webauthn_configuration_id: webauthn_configuration.id, multi_factor_auth_method_created_at: webauthn_configuration.created_at.strftime('%s%L'), new_device: true, + mfa_attempts: { 'webauthn' => 1 }, ) expect(@analytics).to have_logged_event( 'User marked authenticated', @@ -218,6 +219,7 @@ multi_factor_auth_method_created_at: webauthn_configuration.created_at. strftime('%s%L'), new_device: true, + mfa_attempts: { 'webauthn_platform' => 1 }, ) expect(@analytics).to have_logged_event( 'User marked authenticated', @@ -249,6 +251,7 @@ webauthn_configuration_id: webauthn_configuration.id, multi_factor_auth_method_created_at: webauthn_configuration.created_at.strftime('%s%L'), new_device: true, + mfa_attempts: { 'webauthn' => 1 }, ) end @@ -315,6 +318,7 @@ second_webauthn_platform_configuration.created_at.strftime('%s%L'), new_device: true, frontend_error: webauthn_error, + mfa_attempts: { 'webauthn_platform' => 1 }, ) end end From 25a70ff0118079ee18e9d3ac247d2954881fc33a Mon Sep 17 00:00:00 2001 From: Kevin Masters <135744319+kevinsmaster5@users.noreply.github.com> Date: Fri, 11 Oct 2024 13:12:12 -0400 Subject: [PATCH 27/45] fixes specs to catch missing events --- app/controllers/concerns/two_factor_authenticatable_methods.rb | 1 + .../concerns/two_factor_authenticatable_methods_spec.rb | 2 ++ .../backup_code_verification_controller_spec.rb | 3 +++ .../personal_key_verification_controller_spec.rb | 2 ++ spec/controllers/users/webauthn_setup_controller_spec.rb | 2 +- 5 files changed, 9 insertions(+), 1 deletion(-) diff --git a/app/controllers/concerns/two_factor_authenticatable_methods.rb b/app/controllers/concerns/two_factor_authenticatable_methods.rb index 5e142bb156c..7a2f4da30c6 100644 --- a/app/controllers/concerns/two_factor_authenticatable_methods.rb +++ b/app/controllers/concerns/two_factor_authenticatable_methods.rb @@ -129,6 +129,7 @@ def handle_remember_device_preference(remember_device_preference) def mfa_selection_attempt_count(auth_method) user_session[:mfa_attempts] ||= {} + auth_method = auth_method.gsub('personal_key', 'personal-key') attempt = { auth_method => 1 } user_session[:mfa_attempts].merge!(attempt) { |_key, old_val, new_val| old_val + new_val } end diff --git a/spec/controllers/concerns/two_factor_authenticatable_methods_spec.rb b/spec/controllers/concerns/two_factor_authenticatable_methods_spec.rb index f14f9ec9c32..2e283f6e50b 100644 --- a/spec/controllers/concerns/two_factor_authenticatable_methods_spec.rb +++ b/spec/controllers/concerns/two_factor_authenticatable_methods_spec.rb @@ -36,6 +36,7 @@ multi_factor_auth_method: TwoFactorAuthenticatable::AuthMethod::REMEMBER_DEVICE, enabled_mfa_methods_count: 0, new_device: true, + mfa_attempts: { 'remember_device' => 1 }, ) end @@ -189,6 +190,7 @@ multi_factor_auth_method: TwoFactorAuthenticatable::AuthMethod::SMS, enabled_mfa_methods_count: 1, new_device: true, + mfa_attempts: { 'sms' => 1 }, ) end diff --git a/spec/controllers/two_factor_authentication/backup_code_verification_controller_spec.rb b/spec/controllers/two_factor_authentication/backup_code_verification_controller_spec.rb index 2b474fdf885..2b8a6e20cbb 100644 --- a/spec/controllers/two_factor_authentication/backup_code_verification_controller_spec.rb +++ b/spec/controllers/two_factor_authentication/backup_code_verification_controller_spec.rb @@ -41,6 +41,7 @@ multi_factor_auth_method_created_at: Time.zone.now.strftime('%s%L'), enabled_mfa_methods_count: 1, new_device: true, + mfa_attempts: { 'backup_code' => 1 }, ) expect(subject.user_session[:auth_events]).to eq( @@ -97,6 +98,7 @@ multi_factor_auth_method_created_at: Time.zone.now.strftime('%s%L'), enabled_mfa_methods_count: 1, new_device: true, + mfa_attempts: { 'backup_code' => 1 }, ) expect(@analytics).to have_logged_event( 'User marked authenticated', @@ -175,6 +177,7 @@ multi_factor_auth_method: 'backup_code', enabled_mfa_methods_count: 1, new_device: true, + mfa_attempts: { 'backup_code' => 1 }, ) expect(@analytics).to have_logged_event('Multi-Factor Authentication: max attempts reached') end diff --git a/spec/controllers/two_factor_authentication/personal_key_verification_controller_spec.rb b/spec/controllers/two_factor_authentication/personal_key_verification_controller_spec.rb index d376cc825b9..14971363673 100644 --- a/spec/controllers/two_factor_authentication/personal_key_verification_controller_spec.rb +++ b/spec/controllers/two_factor_authentication/personal_key_verification_controller_spec.rb @@ -76,6 +76,7 @@ multi_factor_auth_method: 'personal-key', multi_factor_auth_method_created_at:, new_device: true, + mfa_attempts: { 'personal_key' => 1 }, ) expect(@analytics).to have_logged_event( 'Personal key: Alert user about sign in', @@ -218,6 +219,7 @@ multi_factor_auth_method: 'personal-key', multi_factor_auth_method_created_at: personal_key_generated_at.strftime('%s%L'), new_device: true, + mfa_attempts: { 'personal_key' => 1 }, ) expect(@analytics).to have_logged_event('Multi-Factor Authentication: max attempts reached') end diff --git a/spec/controllers/users/webauthn_setup_controller_spec.rb b/spec/controllers/users/webauthn_setup_controller_spec.rb index c3568d3c065..df537c5e422 100644 --- a/spec/controllers/users/webauthn_setup_controller_spec.rb +++ b/spec/controllers/users/webauthn_setup_controller_spec.rb @@ -390,7 +390,7 @@ end it 'tracks the submission' do - Funnel::Registration::AddMfa.call(user.id, 'phone', @analytics) + Funnel::Registration::AddMfa.call(user.id, 'webauthn', @analytics) patch :confirm, params: params From c6f08f0ff8c7e407e7fa79f00b29820655b3be05 Mon Sep 17 00:00:00 2001 From: Kevin Masters <135744319+kevinsmaster5@users.noreply.github.com> Date: Fri, 11 Oct 2024 14:30:14 -0400 Subject: [PATCH 28/45] fix webauthn spec to correct user flow --- spec/controllers/users/webauthn_setup_controller_spec.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/spec/controllers/users/webauthn_setup_controller_spec.rb b/spec/controllers/users/webauthn_setup_controller_spec.rb index df537c5e422..6e1d5ba5729 100644 --- a/spec/controllers/users/webauthn_setup_controller_spec.rb +++ b/spec/controllers/users/webauthn_setup_controller_spec.rb @@ -382,13 +382,15 @@ end before do + get :new controller.user_session[:in_account_creation_flow] = true allow(IdentityConfig.store).to receive(:domain_name).and_return('localhost:3000') request.host = 'localhost:3000' controller.user_session[:webauthn_challenge] = webauthn_challenge - controller.user_session[:mfa_attempts] = 1 end + let(:mfa_selections) { ['webauthn'] } + it 'tracks the submission' do Funnel::Registration::AddMfa.call(user.id, 'webauthn', @analytics) From a97a0ac0a1c442d6e82761f7812e87ff91b5ad35 Mon Sep 17 00:00:00 2001 From: Kevin Masters <135744319+kevinsmaster5@users.noreply.github.com> Date: Fri, 11 Oct 2024 14:50:47 -0400 Subject: [PATCH 29/45] fix mfa label in spec --- .../personal_key_verification_controller_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/controllers/two_factor_authentication/personal_key_verification_controller_spec.rb b/spec/controllers/two_factor_authentication/personal_key_verification_controller_spec.rb index 14971363673..2c787a59339 100644 --- a/spec/controllers/two_factor_authentication/personal_key_verification_controller_spec.rb +++ b/spec/controllers/two_factor_authentication/personal_key_verification_controller_spec.rb @@ -76,7 +76,7 @@ multi_factor_auth_method: 'personal-key', multi_factor_auth_method_created_at:, new_device: true, - mfa_attempts: { 'personal_key' => 1 }, + mfa_attempts: { 'personal-key' => 1 }, ) expect(@analytics).to have_logged_event( 'Personal key: Alert user about sign in', @@ -219,7 +219,7 @@ multi_factor_auth_method: 'personal-key', multi_factor_auth_method_created_at: personal_key_generated_at.strftime('%s%L'), new_device: true, - mfa_attempts: { 'personal_key' => 1 }, + mfa_attempts: { 'personal-key' => 1 }, ) expect(@analytics).to have_logged_event('Multi-Factor Authentication: max attempts reached') end From 02ec7f7534a29a67648c059095fa5a5884d645e5 Mon Sep 17 00:00:00 2001 From: Kevin Masters <135744319+kevinsmaster5@users.noreply.github.com> Date: Tue, 15 Oct 2024 12:29:24 -0400 Subject: [PATCH 30/45] remove private_key gsub --- .../concerns/two_factor_authenticatable_methods.rb | 1 - .../personal_key_verification_controller_spec.rb | 4 ++-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/app/controllers/concerns/two_factor_authenticatable_methods.rb b/app/controllers/concerns/two_factor_authenticatable_methods.rb index 7a2f4da30c6..5e142bb156c 100644 --- a/app/controllers/concerns/two_factor_authenticatable_methods.rb +++ b/app/controllers/concerns/two_factor_authenticatable_methods.rb @@ -129,7 +129,6 @@ def handle_remember_device_preference(remember_device_preference) def mfa_selection_attempt_count(auth_method) user_session[:mfa_attempts] ||= {} - auth_method = auth_method.gsub('personal_key', 'personal-key') attempt = { auth_method => 1 } user_session[:mfa_attempts].merge!(attempt) { |_key, old_val, new_val| old_val + new_val } end diff --git a/spec/controllers/two_factor_authentication/personal_key_verification_controller_spec.rb b/spec/controllers/two_factor_authentication/personal_key_verification_controller_spec.rb index 2c787a59339..14971363673 100644 --- a/spec/controllers/two_factor_authentication/personal_key_verification_controller_spec.rb +++ b/spec/controllers/two_factor_authentication/personal_key_verification_controller_spec.rb @@ -76,7 +76,7 @@ multi_factor_auth_method: 'personal-key', multi_factor_auth_method_created_at:, new_device: true, - mfa_attempts: { 'personal-key' => 1 }, + mfa_attempts: { 'personal_key' => 1 }, ) expect(@analytics).to have_logged_event( 'Personal key: Alert user about sign in', @@ -219,7 +219,7 @@ multi_factor_auth_method: 'personal-key', multi_factor_auth_method_created_at: personal_key_generated_at.strftime('%s%L'), new_device: true, - mfa_attempts: { 'personal-key' => 1 }, + mfa_attempts: { 'personal_key' => 1 }, ) expect(@analytics).to have_logged_event('Multi-Factor Authentication: max attempts reached') end From 097a11458ceb99bb1654d885163b5d0f1a2f7360 Mon Sep 17 00:00:00 2001 From: Kevin Masters <135744319+kevinsmaster5@users.noreply.github.com> Date: Tue, 15 Oct 2024 14:34:58 -0400 Subject: [PATCH 31/45] leverage symbols for mfa methods, remove no longer needed method from prev iteration --- .../concerns/two_factor_authenticatable_methods.rb | 12 ------------ .../otp_verification_controller.rb | 2 +- .../users/piv_cac_authentication_setup_controller.rb | 2 +- app/controllers/users/totp_setup_controller.rb | 2 +- app/controllers/users/webauthn_setup_controller.rb | 2 +- .../otp_verification_controller_spec.rb | 8 ++++---- 6 files changed, 8 insertions(+), 20 deletions(-) diff --git a/app/controllers/concerns/two_factor_authenticatable_methods.rb b/app/controllers/concerns/two_factor_authenticatable_methods.rb index 5e142bb156c..5df37c6ba7f 100644 --- a/app/controllers/concerns/two_factor_authenticatable_methods.rb +++ b/app/controllers/concerns/two_factor_authenticatable_methods.rb @@ -133,18 +133,6 @@ def mfa_selection_attempt_count(auth_method) user_session[:mfa_attempts].merge!(attempt) { |_key, old_val, new_val| old_val + new_val } end - def reset_mfa_selection_attempt_count - user_session.delete(:mfa_attempts) - end - - def mfa_attempts_hash(auth_method) - return nil if user_session[:mfa_attempts].nil? - { - attempts: user_session[:mfa_attempts], - auth_method: auth_method, - } - end - # Method will be renamed in the next refactor. # You can pass in any "type" with a corresponding I18n key in # two_factor_authentication.invalid_#{type} diff --git a/app/controllers/two_factor_authentication/otp_verification_controller.rb b/app/controllers/two_factor_authentication/otp_verification_controller.rb index 8d21e41e88a..ad131611920 100644 --- a/app/controllers/two_factor_authentication/otp_verification_controller.rb +++ b/app/controllers/two_factor_authentication/otp_verification_controller.rb @@ -21,7 +21,7 @@ def show end def create - mfa_selection_attempt_count('otp') + mfa_selection_attempt_count(:otp) result = otp_verification_form.submit post_analytics(result) diff --git a/app/controllers/users/piv_cac_authentication_setup_controller.rb b/app/controllers/users/piv_cac_authentication_setup_controller.rb index 2ff52ccdbae..fa26e22377d 100644 --- a/app/controllers/users/piv_cac_authentication_setup_controller.rb +++ b/app/controllers/users/piv_cac_authentication_setup_controller.rb @@ -66,7 +66,7 @@ def piv_cac_service_url_with_redirect end def process_piv_cac_setup - mfa_selection_attempt_count('piv_cac') + mfa_selection_attempt_count(:piv_cac) result = user_piv_cac_form.submit properties = result.to_h.merge(analytics_properties) analytics.multi_factor_auth_setup(**properties) diff --git a/app/controllers/users/totp_setup_controller.rb b/app/controllers/users/totp_setup_controller.rb index 6732fac8899..f1f3915a348 100644 --- a/app/controllers/users/totp_setup_controller.rb +++ b/app/controllers/users/totp_setup_controller.rb @@ -26,7 +26,7 @@ def new def confirm result = totp_setup_form.submit - mfa_selection_attempt_count('totp') + mfa_selection_attempt_count(:totp) properties = result.to_h.merge(analytics_properties) analytics.multi_factor_auth_setup(**properties) diff --git a/app/controllers/users/webauthn_setup_controller.rb b/app/controllers/users/webauthn_setup_controller.rb index 78eafcf5acc..d03e1f70814 100644 --- a/app/controllers/users/webauthn_setup_controller.rb +++ b/app/controllers/users/webauthn_setup_controller.rb @@ -51,7 +51,7 @@ def new end flash_error(result.errors) unless result.success? - mfa_selection_attempt_count('webauthn') + mfa_selection_attempt_count(:webauthn) end def confirm diff --git a/spec/controllers/two_factor_authentication/otp_verification_controller_spec.rb b/spec/controllers/two_factor_authentication/otp_verification_controller_spec.rb index 90bd7697433..75847193f88 100644 --- a/spec/controllers/two_factor_authentication/otp_verification_controller_spec.rb +++ b/spec/controllers/two_factor_authentication/otp_verification_controller_spec.rb @@ -552,7 +552,7 @@ enabled_mfa_methods_count: 1, in_account_creation_flow: true, mfa_attempts: { - 'otp' => 1, + otp: 1, }, ) end @@ -624,7 +624,7 @@ enabled_mfa_methods_count: 1, in_account_creation_flow: false, mfa_attempts: { - 'otp' => 1, + otp: 1, }, ) end @@ -668,7 +668,7 @@ enabled_mfa_methods_count: 1, in_account_creation_flow: false, mfa_attempts: { - 'otp' => 3, + otp: 3, }, ) end @@ -721,7 +721,7 @@ enabled_mfa_methods_count: 0, in_account_creation_flow: false, mfa_attempts: { - 'otp' => 1, + otp: 1, }, ) From 1dcea8147bc0315546726a52b3c297b72612666f Mon Sep 17 00:00:00 2001 From: Kevin Masters <135744319+kevinsmaster5@users.noreply.github.com> Date: Tue, 15 Oct 2024 15:11:34 -0400 Subject: [PATCH 32/45] revise spec to use symbol --- ...cac_authentication_setup_controller_spec.rb | 4 ++-- .../users/totp_setup_controller_spec.rb | 18 +++++++++--------- 2 files changed, 11 insertions(+), 11 deletions(-) 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 a35df9ea97e..7f6bf4de733 100644 --- a/spec/controllers/users/piv_cac_authentication_setup_controller_spec.rb +++ b/spec/controllers/users/piv_cac_authentication_setup_controller_spec.rb @@ -121,7 +121,7 @@ in_account_creation_flow: false, success: true, mfa_attempts: { - 'piv_cac' => 1, + piv_cac: 1, }, ) end @@ -139,7 +139,7 @@ in_account_creation_flow: false, success: true, mfa_attempts: { - 'piv_cac' => 2, + piv_cac: 2, }, ) end diff --git a/spec/controllers/users/totp_setup_controller_spec.rb b/spec/controllers/users/totp_setup_controller_spec.rb index c7cd2fa54e3..556be8f3bd1 100644 --- a/spec/controllers/users/totp_setup_controller_spec.rb +++ b/spec/controllers/users/totp_setup_controller_spec.rb @@ -109,7 +109,7 @@ enabled_mfa_methods_count: 0, in_account_creation_flow: false, mfa_attempts: { - 'totp' => 1, + totp: 1, }, ) end @@ -140,7 +140,7 @@ enabled_mfa_methods_count: 2, in_account_creation_flow: false, mfa_attempts: { - 'totp' => 1, + totp: 1, }, ) end @@ -173,7 +173,7 @@ enabled_mfa_methods_count: 2, in_account_creation_flow: false, mfa_attempts: { - 'totp' => 2, + totp: 2, }, ) end @@ -204,7 +204,7 @@ enabled_mfa_methods_count: 1, in_account_creation_flow: false, mfa_attempts: { - 'totp' => 1, + totp: 1, }, ) end @@ -236,7 +236,7 @@ enabled_mfa_methods_count: 1, in_account_creation_flow: false, mfa_attempts: { - 'totp' => 1, + totp: 1, }, ) end @@ -267,7 +267,7 @@ enabled_mfa_methods_count: 0, in_account_creation_flow: false, mfa_attempts: { - 'totp' => 1, + totp: 1, }, ) end @@ -301,7 +301,7 @@ enabled_mfa_methods_count: 1, in_account_creation_flow: true, mfa_attempts: { - 'totp' => 1, + totp: 1, }, ) end @@ -323,7 +323,7 @@ enabled_mfa_methods_count: 1, in_account_creation_flow: true, mfa_attempts: { - 'totp' => 1, + totp: 1, }, ) end @@ -352,7 +352,7 @@ enabled_mfa_methods_count: 0, in_account_creation_flow: false, mfa_attempts: { - 'totp' => 1, + totp: 1, }, ) end From e1ae5320237e465f42bf47f05786e61a1269c620 Mon Sep 17 00:00:00 2001 From: Kevin Masters <135744319+kevinsmaster5@users.noreply.github.com> Date: Tue, 15 Oct 2024 16:57:53 -0400 Subject: [PATCH 33/45] address keypath warnings from spec --- .../concerns/two_factor_authenticatable_methods.rb | 5 +++++ .../otp_verification_controller_spec.rb | 1 + 2 files changed, 6 insertions(+) diff --git a/app/controllers/concerns/two_factor_authenticatable_methods.rb b/app/controllers/concerns/two_factor_authenticatable_methods.rb index 5df37c6ba7f..dc536a36f64 100644 --- a/app/controllers/concerns/two_factor_authenticatable_methods.rb +++ b/app/controllers/concerns/two_factor_authenticatable_methods.rb @@ -20,6 +20,11 @@ def handle_verification_for_authentication_context(result:, auth_method:, extra_ new_device: new_device?, **extra_analytics.to_h, mfa_attempts: user_session[:mfa_attempts], + pii_like_keypaths: [ + [:mfa_attempts, :otp], + [:errors, :personal_key], + [:error_details, :personal_key], + ], ) if result.success? diff --git a/spec/controllers/two_factor_authentication/otp_verification_controller_spec.rb b/spec/controllers/two_factor_authentication/otp_verification_controller_spec.rb index 75847193f88..8115238f309 100644 --- a/spec/controllers/two_factor_authentication/otp_verification_controller_spec.rb +++ b/spec/controllers/two_factor_authentication/otp_verification_controller_spec.rb @@ -554,6 +554,7 @@ mfa_attempts: { otp: 1, }, + pii_like_keypaths: [[:mfa_attempts, :otp], [:errors, :personal_key]], ) end From 9500700f1248bfca4dc51e10680c49ee0fe14d79 Mon Sep 17 00:00:00 2001 From: Kevin Masters <135744319+kevinsmaster5@users.noreply.github.com> Date: Tue, 15 Oct 2024 17:51:12 -0400 Subject: [PATCH 34/45] fix how otp verification controller generates the attempts count, update specs to sync with that --- .../concerns/two_factor_authenticatable_methods.rb | 4 ++-- .../otp_verification_controller.rb | 7 ++++++- .../otp_verification_controller_spec.rb | 5 ----- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/app/controllers/concerns/two_factor_authenticatable_methods.rb b/app/controllers/concerns/two_factor_authenticatable_methods.rb index dc536a36f64..365fbbc337d 100644 --- a/app/controllers/concerns/two_factor_authenticatable_methods.rb +++ b/app/controllers/concerns/two_factor_authenticatable_methods.rb @@ -134,8 +134,8 @@ def handle_remember_device_preference(remember_device_preference) def mfa_selection_attempt_count(auth_method) user_session[:mfa_attempts] ||= {} - attempt = { auth_method => 1 } - user_session[:mfa_attempts].merge!(attempt) { |_key, old_val, new_val| old_val + new_val } + user_session[:mfa_attempts][auth_method] ||= 0 + user_session[:mfa_attempts][auth_method] += 1 end # Method will be renamed in the next refactor. diff --git a/app/controllers/two_factor_authentication/otp_verification_controller.rb b/app/controllers/two_factor_authentication/otp_verification_controller.rb index ad131611920..95b2ae8cce9 100644 --- a/app/controllers/two_factor_authentication/otp_verification_controller.rb +++ b/app/controllers/two_factor_authentication/otp_verification_controller.rb @@ -21,7 +21,7 @@ def show end def create - mfa_selection_attempt_count(:otp) + mfa_selection_attempt_count(:otp) if UserSessionContext.confirmation_context?(context) result = otp_verification_form.submit post_analytics(result) @@ -159,6 +159,11 @@ def analytics_properties in_account_creation_flow: user_session[:in_account_creation_flow] || false, enabled_mfa_methods_count: mfa_context.enabled_mfa_methods_count, mfa_attempts: user_session[:mfa_attempts], + pii_like_keypaths: [ + [:mfa_attempts, :otp], + [:errors, :personal_key], + [:error_details, :personal_key], + ], } end diff --git a/spec/controllers/two_factor_authentication/otp_verification_controller_spec.rb b/spec/controllers/two_factor_authentication/otp_verification_controller_spec.rb index 8115238f309..5fca28f9f41 100644 --- a/spec/controllers/two_factor_authentication/otp_verification_controller_spec.rb +++ b/spec/controllers/two_factor_authentication/otp_verification_controller_spec.rb @@ -153,7 +153,6 @@ enabled_mfa_methods_count: 1, in_account_creation_flow: false, mfa_attempts: { - 'otp' => 1, 'sms' => 1, }, ) @@ -239,7 +238,6 @@ enabled_mfa_methods_count: 1, in_account_creation_flow: false, mfa_attempts: { - 'otp' => 1, 'sms' => 1, }, ) @@ -313,7 +311,6 @@ enabled_mfa_methods_count: 1, in_account_creation_flow: false, mfa_attempts: { - 'otp' => 1, 'sms' => 1, }, ) @@ -364,7 +361,6 @@ enabled_mfa_methods_count: 1, in_account_creation_flow: false, mfa_attempts: { - 'otp' => 1, 'sms' => 1, }, ) @@ -554,7 +550,6 @@ mfa_attempts: { otp: 1, }, - pii_like_keypaths: [[:mfa_attempts, :otp], [:errors, :personal_key]], ) end From 1800e9b774998221362e16b67b64578f480c6c0f Mon Sep 17 00:00:00 2001 From: Kevin Masters <135744319+kevinsmaster5@users.noreply.github.com> Date: Thu, 17 Oct 2024 08:21:53 -0400 Subject: [PATCH 35/45] gsub personal_key for mfa_attempts --- app/controllers/concerns/two_factor_authenticatable_methods.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/app/controllers/concerns/two_factor_authenticatable_methods.rb b/app/controllers/concerns/two_factor_authenticatable_methods.rb index 365fbbc337d..76dd15b409d 100644 --- a/app/controllers/concerns/two_factor_authenticatable_methods.rb +++ b/app/controllers/concerns/two_factor_authenticatable_methods.rb @@ -134,6 +134,7 @@ def handle_remember_device_preference(remember_device_preference) def mfa_selection_attempt_count(auth_method) user_session[:mfa_attempts] ||= {} + auth_method = auth_method.gsub('personal_key', 'personal-key') user_session[:mfa_attempts][auth_method] ||= 0 user_session[:mfa_attempts][auth_method] += 1 end From 39b18a9cae75b6232f719c32a068f5fff0db31bc Mon Sep 17 00:00:00 2001 From: Kevin Masters <135744319+kevinsmaster5@users.noreply.github.com> Date: Thu, 17 Oct 2024 09:19:22 -0400 Subject: [PATCH 36/45] convert to sym --- app/controllers/concerns/two_factor_authenticatable_methods.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/concerns/two_factor_authenticatable_methods.rb b/app/controllers/concerns/two_factor_authenticatable_methods.rb index 76dd15b409d..69b42d9ed9e 100644 --- a/app/controllers/concerns/two_factor_authenticatable_methods.rb +++ b/app/controllers/concerns/two_factor_authenticatable_methods.rb @@ -134,7 +134,7 @@ def handle_remember_device_preference(remember_device_preference) def mfa_selection_attempt_count(auth_method) user_session[:mfa_attempts] ||= {} - auth_method = auth_method.gsub('personal_key', 'personal-key') + auth_method = auth_method.to_s.gsub('personal_key', 'personal-key') user_session[:mfa_attempts][auth_method] ||= 0 user_session[:mfa_attempts][auth_method] += 1 end From 6528f703649ddfb62d52c02bfa9c574fd3828a7f Mon Sep 17 00:00:00 2001 From: Kevin Masters <135744319+kevinsmaster5@users.noreply.github.com> Date: Thu, 17 Oct 2024 09:37:48 -0400 Subject: [PATCH 37/45] convert key to sym with correct method --- app/controllers/concerns/two_factor_authenticatable_methods.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/concerns/two_factor_authenticatable_methods.rb b/app/controllers/concerns/two_factor_authenticatable_methods.rb index 69b42d9ed9e..bc775e406fe 100644 --- a/app/controllers/concerns/two_factor_authenticatable_methods.rb +++ b/app/controllers/concerns/two_factor_authenticatable_methods.rb @@ -134,7 +134,7 @@ def handle_remember_device_preference(remember_device_preference) def mfa_selection_attempt_count(auth_method) user_session[:mfa_attempts] ||= {} - auth_method = auth_method.to_s.gsub('personal_key', 'personal-key') + auth_method = auth_method.to_s.gsub('personal_key', 'personal-key').to_sym user_session[:mfa_attempts][auth_method] ||= 0 user_session[:mfa_attempts][auth_method] += 1 end From 1f9ae4c17204f3b95cb36c8e17ccad94cc23ca63 Mon Sep 17 00:00:00 2001 From: Kevin Masters <135744319+kevinsmaster5@users.noreply.github.com> Date: Thu, 17 Oct 2024 10:12:39 -0400 Subject: [PATCH 38/45] rename incrementing method param. repair rspec to correct exptected mfa --- .../concerns/two_factor_authenticatable_methods.rb | 8 ++++---- .../otp_verification_controller_spec.rb | 8 ++++---- .../personal_key_verification_controller_spec.rb | 4 ++-- .../users/piv_cac_authentication_setup_controller_spec.rb | 4 ++-- 4 files changed, 12 insertions(+), 12 deletions(-) diff --git a/app/controllers/concerns/two_factor_authenticatable_methods.rb b/app/controllers/concerns/two_factor_authenticatable_methods.rb index bc775e406fe..554eec2f600 100644 --- a/app/controllers/concerns/two_factor_authenticatable_methods.rb +++ b/app/controllers/concerns/two_factor_authenticatable_methods.rb @@ -132,11 +132,11 @@ def handle_remember_device_preference(remember_device_preference) save_remember_device_preference(remember_device_preference) end - def mfa_selection_attempt_count(auth_method) + def mfa_selection_attempt_count(increment_auth_method) user_session[:mfa_attempts] ||= {} - auth_method = auth_method.to_s.gsub('personal_key', 'personal-key').to_sym - user_session[:mfa_attempts][auth_method] ||= 0 - user_session[:mfa_attempts][auth_method] += 1 + increment_auth_method = increment_auth_method.to_s.gsub('personal_key', 'personal-key') + user_session[:mfa_attempts][increment_auth_method] ||= 0 + user_session[:mfa_attempts][increment_auth_method] += 1 end # Method will be renamed in the next refactor. diff --git a/spec/controllers/two_factor_authentication/otp_verification_controller_spec.rb b/spec/controllers/two_factor_authentication/otp_verification_controller_spec.rb index 5fca28f9f41..abb25986473 100644 --- a/spec/controllers/two_factor_authentication/otp_verification_controller_spec.rb +++ b/spec/controllers/two_factor_authentication/otp_verification_controller_spec.rb @@ -548,7 +548,7 @@ enabled_mfa_methods_count: 1, in_account_creation_flow: true, mfa_attempts: { - otp: 1, + 'otp' => 1, }, ) end @@ -620,7 +620,7 @@ enabled_mfa_methods_count: 1, in_account_creation_flow: false, mfa_attempts: { - otp: 1, + 'otp' => 1, }, ) end @@ -664,7 +664,7 @@ enabled_mfa_methods_count: 1, in_account_creation_flow: false, mfa_attempts: { - otp: 3, + 'otp' => 3, }, ) end @@ -717,7 +717,7 @@ enabled_mfa_methods_count: 0, in_account_creation_flow: false, mfa_attempts: { - otp: 1, + 'otp' => 1, }, ) diff --git a/spec/controllers/two_factor_authentication/personal_key_verification_controller_spec.rb b/spec/controllers/two_factor_authentication/personal_key_verification_controller_spec.rb index 14971363673..2c787a59339 100644 --- a/spec/controllers/two_factor_authentication/personal_key_verification_controller_spec.rb +++ b/spec/controllers/two_factor_authentication/personal_key_verification_controller_spec.rb @@ -76,7 +76,7 @@ multi_factor_auth_method: 'personal-key', multi_factor_auth_method_created_at:, new_device: true, - mfa_attempts: { 'personal_key' => 1 }, + mfa_attempts: { 'personal-key' => 1 }, ) expect(@analytics).to have_logged_event( 'Personal key: Alert user about sign in', @@ -219,7 +219,7 @@ multi_factor_auth_method: 'personal-key', multi_factor_auth_method_created_at: personal_key_generated_at.strftime('%s%L'), new_device: true, - mfa_attempts: { 'personal_key' => 1 }, + mfa_attempts: { 'personal-key' => 1 }, ) expect(@analytics).to have_logged_event('Multi-Factor Authentication: max attempts reached') end 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 7f6bf4de733..a35df9ea97e 100644 --- a/spec/controllers/users/piv_cac_authentication_setup_controller_spec.rb +++ b/spec/controllers/users/piv_cac_authentication_setup_controller_spec.rb @@ -121,7 +121,7 @@ in_account_creation_flow: false, success: true, mfa_attempts: { - piv_cac: 1, + 'piv_cac' => 1, }, ) end @@ -139,7 +139,7 @@ in_account_creation_flow: false, success: true, mfa_attempts: { - piv_cac: 2, + 'piv_cac' => 2, }, ) end From da9afdf38806619ffa14dc5aba0cbdae0b25be9c Mon Sep 17 00:00:00 2001 From: Kevin Masters <135744319+kevinsmaster5@users.noreply.github.com> Date: Thu, 17 Oct 2024 11:05:07 -0400 Subject: [PATCH 39/45] fix rspec expected mfa types --- spec/controllers/users/totp_setup_controller_spec.rb | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/spec/controllers/users/totp_setup_controller_spec.rb b/spec/controllers/users/totp_setup_controller_spec.rb index 556be8f3bd1..3e8bdb018e7 100644 --- a/spec/controllers/users/totp_setup_controller_spec.rb +++ b/spec/controllers/users/totp_setup_controller_spec.rb @@ -204,7 +204,7 @@ enabled_mfa_methods_count: 1, in_account_creation_flow: false, mfa_attempts: { - totp: 1, + 'totp' => 1, }, ) end @@ -267,7 +267,7 @@ enabled_mfa_methods_count: 0, in_account_creation_flow: false, mfa_attempts: { - totp: 1, + 'totp' => 1, }, ) end @@ -301,7 +301,7 @@ enabled_mfa_methods_count: 1, in_account_creation_flow: true, mfa_attempts: { - totp: 1, + 'totp' => 1, }, ) end @@ -323,7 +323,7 @@ enabled_mfa_methods_count: 1, in_account_creation_flow: true, mfa_attempts: { - totp: 1, + 'totp' => 1, }, ) end @@ -352,7 +352,7 @@ enabled_mfa_methods_count: 0, in_account_creation_flow: false, mfa_attempts: { - totp: 1, + 'totp' => 1, }, ) end From 65f323299732d5382ef7b91526637542627376a1 Mon Sep 17 00:00:00 2001 From: Kevin Masters <135744319+kevinsmaster5@users.noreply.github.com> Date: Thu, 17 Oct 2024 17:01:12 -0400 Subject: [PATCH 40/45] place gsub behind a conditional --- .../concerns/two_factor_authenticatable_methods.rb | 7 ++++++- .../otp_verification_controller_spec.rb | 8 ++++---- .../users/piv_cac_authentication_setup_controller_spec.rb | 4 ++-- spec/controllers/users/totp_setup_controller_spec.rb | 8 ++++---- 4 files changed, 16 insertions(+), 11 deletions(-) diff --git a/app/controllers/concerns/two_factor_authenticatable_methods.rb b/app/controllers/concerns/two_factor_authenticatable_methods.rb index 554eec2f600..afc7f65f4ac 100644 --- a/app/controllers/concerns/two_factor_authenticatable_methods.rb +++ b/app/controllers/concerns/two_factor_authenticatable_methods.rb @@ -134,7 +134,12 @@ def handle_remember_device_preference(remember_device_preference) def mfa_selection_attempt_count(increment_auth_method) user_session[:mfa_attempts] ||= {} - increment_auth_method = increment_auth_method.to_s.gsub('personal_key', 'personal-key') + if increment_auth_method == 'personal_key' + increment_auth_method = increment_auth_method.gsub( + 'personal_key', + 'personal-key', + ) + end user_session[:mfa_attempts][increment_auth_method] ||= 0 user_session[:mfa_attempts][increment_auth_method] += 1 end diff --git a/spec/controllers/two_factor_authentication/otp_verification_controller_spec.rb b/spec/controllers/two_factor_authentication/otp_verification_controller_spec.rb index abb25986473..5fca28f9f41 100644 --- a/spec/controllers/two_factor_authentication/otp_verification_controller_spec.rb +++ b/spec/controllers/two_factor_authentication/otp_verification_controller_spec.rb @@ -548,7 +548,7 @@ enabled_mfa_methods_count: 1, in_account_creation_flow: true, mfa_attempts: { - 'otp' => 1, + otp: 1, }, ) end @@ -620,7 +620,7 @@ enabled_mfa_methods_count: 1, in_account_creation_flow: false, mfa_attempts: { - 'otp' => 1, + otp: 1, }, ) end @@ -664,7 +664,7 @@ enabled_mfa_methods_count: 1, in_account_creation_flow: false, mfa_attempts: { - 'otp' => 3, + otp: 3, }, ) end @@ -717,7 +717,7 @@ enabled_mfa_methods_count: 0, in_account_creation_flow: false, mfa_attempts: { - 'otp' => 1, + otp: 1, }, ) 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 a35df9ea97e..7f6bf4de733 100644 --- a/spec/controllers/users/piv_cac_authentication_setup_controller_spec.rb +++ b/spec/controllers/users/piv_cac_authentication_setup_controller_spec.rb @@ -121,7 +121,7 @@ in_account_creation_flow: false, success: true, mfa_attempts: { - 'piv_cac' => 1, + piv_cac: 1, }, ) end @@ -139,7 +139,7 @@ in_account_creation_flow: false, success: true, mfa_attempts: { - 'piv_cac' => 2, + piv_cac: 2, }, ) end diff --git a/spec/controllers/users/totp_setup_controller_spec.rb b/spec/controllers/users/totp_setup_controller_spec.rb index 3e8bdb018e7..ac6d1dab3cb 100644 --- a/spec/controllers/users/totp_setup_controller_spec.rb +++ b/spec/controllers/users/totp_setup_controller_spec.rb @@ -267,7 +267,7 @@ enabled_mfa_methods_count: 0, in_account_creation_flow: false, mfa_attempts: { - 'totp' => 1, + totp: 1, }, ) end @@ -301,7 +301,7 @@ enabled_mfa_methods_count: 1, in_account_creation_flow: true, mfa_attempts: { - 'totp' => 1, + totp: 1, }, ) end @@ -323,7 +323,7 @@ enabled_mfa_methods_count: 1, in_account_creation_flow: true, mfa_attempts: { - 'totp' => 1, + totp: 1, }, ) end @@ -352,7 +352,7 @@ enabled_mfa_methods_count: 0, in_account_creation_flow: false, mfa_attempts: { - 'totp' => 1, + totp: 1, }, ) end From b92b9a6dc13826fac9a66729308d999c18a17100 Mon Sep 17 00:00:00 2001 From: Kevin Masters <135744319+kevinsmaster5@users.noreply.github.com> Date: Fri, 18 Oct 2024 11:05:37 -0400 Subject: [PATCH 41/45] set up a programmatic way of protecting pii keys in sessions --- .../two_factor_authenticatable_methods.rb | 7 +---- lib/session_encryptor.rb | 26 ++++++++++++------- 2 files changed, 17 insertions(+), 16 deletions(-) diff --git a/app/controllers/concerns/two_factor_authenticatable_methods.rb b/app/controllers/concerns/two_factor_authenticatable_methods.rb index afc7f65f4ac..815a59f9f28 100644 --- a/app/controllers/concerns/two_factor_authenticatable_methods.rb +++ b/app/controllers/concerns/two_factor_authenticatable_methods.rb @@ -134,14 +134,9 @@ def handle_remember_device_preference(remember_device_preference) def mfa_selection_attempt_count(increment_auth_method) user_session[:mfa_attempts] ||= {} - if increment_auth_method == 'personal_key' - increment_auth_method = increment_auth_method.gsub( - 'personal_key', - 'personal-key', - ) - end user_session[:mfa_attempts][increment_auth_method] ||= 0 user_session[:mfa_attempts][increment_auth_method] += 1 + user_session['pii_like_key'] = ['mfa_attempts'] end # Method will be renamed in the next refactor. diff --git a/lib/session_encryptor.rb b/lib/session_encryptor.rb index 57974db862b..94b998d417c 100644 --- a/lib/session_encryptor.rb +++ b/lib/session_encryptor.rb @@ -172,16 +172,22 @@ def alert_or_raise_if_contains_sensitive_value!(string, hash) def alert_or_raise_if_contains_sensitive_keys!(hash) hash.deep_transform_keys do |key| - if SENSITIVE_KEYS.include?(key.to_s) - exception = SensitiveKeyError.new("#{key} unexpectedly appeared in session") - if IdentityConfig.store.session_encryptor_alert_enabled - NewRelic::Agent.notice_error( - exception, custom_params: { - session_structure: hash.deep_transform_values { |_v| '' }, - } - ) - else - raise exception + if key == 'warden.user.user.session' && hash['warden.user.user.session']&.dig('pii_like_key'). + present? + unless hash['warden.user.user.session']['pii_like_key']. + any? { |key| hash['warden.user.user.session'].key?(key) } + if SENSITIVE_KEYS.include?(key.to_s) + exception = SensitiveKeyError.new("#{key} unexpectedly appeared in session") + if IdentityConfig.store.session_encryptor_alert_enabled + NewRelic::Agent.notice_error( + exception, custom_params: { + session_structure: hash.deep_transform_values { |_v| '' }, + } + ) + else + raise exception + end + end end end end From b9b2843ba4c5936ec3f71de1a4e2c0f92d3c6829 Mon Sep 17 00:00:00 2001 From: Kevin Masters <135744319+kevinsmaster5@users.noreply.github.com> Date: Fri, 18 Oct 2024 11:12:11 -0400 Subject: [PATCH 42/45] refactor increment verb --- .../concerns/two_factor_authenticatable_methods.rb | 8 ++++---- .../otp_verification_controller.rb | 4 +++- .../users/piv_cac_authentication_setup_controller.rb | 2 +- app/controllers/users/totp_setup_controller.rb | 2 +- app/controllers/users/webauthn_setup_controller.rb | 2 +- 5 files changed, 10 insertions(+), 8 deletions(-) diff --git a/app/controllers/concerns/two_factor_authenticatable_methods.rb b/app/controllers/concerns/two_factor_authenticatable_methods.rb index 815a59f9f28..fee8ddf02d2 100644 --- a/app/controllers/concerns/two_factor_authenticatable_methods.rb +++ b/app/controllers/concerns/two_factor_authenticatable_methods.rb @@ -12,7 +12,7 @@ def auth_methods_session end def handle_verification_for_authentication_context(result:, auth_method:, extra_analytics: nil) - mfa_selection_attempt_count(auth_method) + increment_mfa_selection_attempt_count(auth_method) analytics.multi_factor_auth( **result.to_h, multi_factor_auth_method: auth_method, @@ -132,10 +132,10 @@ def handle_remember_device_preference(remember_device_preference) save_remember_device_preference(remember_device_preference) end - def mfa_selection_attempt_count(increment_auth_method) + def increment_mfa_selection_attempt_count(auth_method) user_session[:mfa_attempts] ||= {} - user_session[:mfa_attempts][increment_auth_method] ||= 0 - user_session[:mfa_attempts][increment_auth_method] += 1 + user_session[:mfa_attempts][auth_method] ||= 0 + user_session[:mfa_attempts][auth_method] += 1 user_session['pii_like_key'] = ['mfa_attempts'] end diff --git a/app/controllers/two_factor_authentication/otp_verification_controller.rb b/app/controllers/two_factor_authentication/otp_verification_controller.rb index 95b2ae8cce9..f0e96b486dd 100644 --- a/app/controllers/two_factor_authentication/otp_verification_controller.rb +++ b/app/controllers/two_factor_authentication/otp_verification_controller.rb @@ -21,7 +21,9 @@ def show end def create - mfa_selection_attempt_count(:otp) if UserSessionContext.confirmation_context?(context) + if UserSessionContext.confirmation_context?(context) + increment_mfa_selection_attempt_count(:otp) + end result = otp_verification_form.submit post_analytics(result) diff --git a/app/controllers/users/piv_cac_authentication_setup_controller.rb b/app/controllers/users/piv_cac_authentication_setup_controller.rb index fa26e22377d..012d5112d76 100644 --- a/app/controllers/users/piv_cac_authentication_setup_controller.rb +++ b/app/controllers/users/piv_cac_authentication_setup_controller.rb @@ -66,7 +66,7 @@ def piv_cac_service_url_with_redirect end def process_piv_cac_setup - mfa_selection_attempt_count(:piv_cac) + increment_mfa_selection_attempt_count(:piv_cac) result = user_piv_cac_form.submit properties = result.to_h.merge(analytics_properties) analytics.multi_factor_auth_setup(**properties) diff --git a/app/controllers/users/totp_setup_controller.rb b/app/controllers/users/totp_setup_controller.rb index f1f3915a348..a61e4126f87 100644 --- a/app/controllers/users/totp_setup_controller.rb +++ b/app/controllers/users/totp_setup_controller.rb @@ -26,7 +26,7 @@ def new def confirm result = totp_setup_form.submit - mfa_selection_attempt_count(:totp) + increment_mfa_selection_attempt_count(:totp) properties = result.to_h.merge(analytics_properties) analytics.multi_factor_auth_setup(**properties) diff --git a/app/controllers/users/webauthn_setup_controller.rb b/app/controllers/users/webauthn_setup_controller.rb index d03e1f70814..753bcff0c61 100644 --- a/app/controllers/users/webauthn_setup_controller.rb +++ b/app/controllers/users/webauthn_setup_controller.rb @@ -51,7 +51,7 @@ def new end flash_error(result.errors) unless result.success? - mfa_selection_attempt_count(:webauthn) + increment_mfa_selection_attempt_count(:webauthn) end def confirm From ee762d5fb1e6e512ea02f154d01ce2f7f1ccea8d Mon Sep 17 00:00:00 2001 From: Kevin Masters <135744319+kevinsmaster5@users.noreply.github.com> Date: Fri, 18 Oct 2024 11:54:18 -0400 Subject: [PATCH 43/45] fix broken logic --- lib/session_encryptor.rb | 27 +++++++++++++-------------- 1 file changed, 13 insertions(+), 14 deletions(-) diff --git a/lib/session_encryptor.rb b/lib/session_encryptor.rb index 94b998d417c..2fc4d96bfcd 100644 --- a/lib/session_encryptor.rb +++ b/lib/session_encryptor.rb @@ -172,21 +172,20 @@ def alert_or_raise_if_contains_sensitive_value!(string, hash) def alert_or_raise_if_contains_sensitive_keys!(hash) hash.deep_transform_keys do |key| - if key == 'warden.user.user.session' && hash['warden.user.user.session']&.dig('pii_like_key'). - present? - unless hash['warden.user.user.session']['pii_like_key']. - any? { |key| hash['warden.user.user.session'].key?(key) } - if SENSITIVE_KEYS.include?(key.to_s) - exception = SensitiveKeyError.new("#{key} unexpectedly appeared in session") - if IdentityConfig.store.session_encryptor_alert_enabled - NewRelic::Agent.notice_error( - exception, custom_params: { - session_structure: hash.deep_transform_values { |_v| '' }, - } - ) - else - raise exception + if SENSITIVE_KEYS.include?(key.to_s) + unless hash['warden.user.user.session']&.dig('pii_like_key'). + present? && hash['warden.user.user.session']['pii_like_key'].any? do |key| + hash['warden.user.user.session'].key?(key) end + exception = SensitiveKeyError.new("#{key} unexpectedly appeared in session") + if IdentityConfig.store.session_encryptor_alert_enabled + NewRelic::Agent.notice_error( + exception, custom_params: { + session_structure: hash.deep_transform_values { |_v| '' }, + } + ) + else + raise exception end end end From 7a58848ac73f6733ff2b11684690eedd38dea14c Mon Sep 17 00:00:00 2001 From: Kevin Masters <135744319+kevinsmaster5@users.noreply.github.com> Date: Fri, 18 Oct 2024 12:33:13 -0400 Subject: [PATCH 44/45] put expected attempt key back --- .../personal_key_verification_controller_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/controllers/two_factor_authentication/personal_key_verification_controller_spec.rb b/spec/controllers/two_factor_authentication/personal_key_verification_controller_spec.rb index 2c787a59339..14971363673 100644 --- a/spec/controllers/two_factor_authentication/personal_key_verification_controller_spec.rb +++ b/spec/controllers/two_factor_authentication/personal_key_verification_controller_spec.rb @@ -76,7 +76,7 @@ multi_factor_auth_method: 'personal-key', multi_factor_auth_method_created_at:, new_device: true, - mfa_attempts: { 'personal-key' => 1 }, + mfa_attempts: { 'personal_key' => 1 }, ) expect(@analytics).to have_logged_event( 'Personal key: Alert user about sign in', @@ -219,7 +219,7 @@ multi_factor_auth_method: 'personal-key', multi_factor_auth_method_created_at: personal_key_generated_at.strftime('%s%L'), new_device: true, - mfa_attempts: { 'personal-key' => 1 }, + mfa_attempts: { 'personal_key' => 1 }, ) expect(@analytics).to have_logged_event('Multi-Factor Authentication: max attempts reached') end From 2f17d793dcf953aedfa1a938846a4566c47eecbe Mon Sep 17 00:00:00 2001 From: Kevin Masters <135744319+kevinsmaster5@users.noreply.github.com> Date: Fri, 18 Oct 2024 16:05:04 -0400 Subject: [PATCH 45/45] add testing for change to session_encryptor --- spec/lib/session_encryptor_spec.rb | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/spec/lib/session_encryptor_spec.rb b/spec/lib/session_encryptor_spec.rb index 048351aa390..a298f034994 100644 --- a/spec/lib/session_encryptor_spec.rb +++ b/spec/lib/session_encryptor_spec.rb @@ -125,6 +125,21 @@ subject.dump(session) end + it 'does not raise for session personal_key key for counting mfa attempts' do + nested_session = { + 'warden.user.user.session': { + mfa_attempts: { personal_key: 1 }, + pii_like_key: ['mfa_attempts'], + }, + } + + expect do + subject.dump(nested_session) + end.not_to raise_error( + SessionEncryptor::SensitiveKeyError, 'personal_key unexpectedly appeared in session' + ) + end + it 'raises if sensitive value is not KMS encrypted' do session = { 'new_key' => Idp::Constants::MOCK_IDV_APPLICANT[:last_name],