From bb2d50c65a4b0c55e5630c51f26becbc70d20a14 Mon Sep 17 00:00:00 2001 From: kevinsmaster5 Date: Fri, 15 Dec 2023 12:25:58 -0500 Subject: [PATCH 01/21] changelog: Internal, Analytics, add new device property to Multi-Factor Authentication event --- .../backup_code_verification_controller.rb | 4 ++- .../otp_verification_controller.rb | 1 + .../personal_key_verification_controller.rb | 1 + .../piv_cac_verification_controller.rb | 1 + .../totp_verification_controller.rb | 5 ++-- .../webauthn_verification_controller.rb | 1 + app/services/device_cookie.rb | 8 +++++ app/services/user_event_creator.rb | 30 +++++++++---------- 8 files changed, 33 insertions(+), 18 deletions(-) create mode 100644 app/services/device_cookie.rb diff --git a/app/controllers/two_factor_authentication/backup_code_verification_controller.rb b/app/controllers/two_factor_authentication/backup_code_verification_controller.rb index 231ca2b780d..809e7ee363e 100644 --- a/app/controllers/two_factor_authentication/backup_code_verification_controller.rb +++ b/app/controllers/two_factor_authentication/backup_code_verification_controller.rb @@ -19,7 +19,9 @@ def show def create @backup_code_form = BackupCodeVerificationForm.new(current_user) result = @backup_code_form.submit(backup_code_params) - analytics.track_mfa_submit_event(result.to_h) + analytics_result = result.to_h + analytics_result[:new_device] = DeviceCookie.check_for_new_device(cookies, current_user).nil? + analytics.track_mfa_submit_event(analytics_result) irs_attempts_api_tracker.mfa_login_backup_code(success: result.success?) handle_result(result) end diff --git a/app/controllers/two_factor_authentication/otp_verification_controller.rb b/app/controllers/two_factor_authentication/otp_verification_controller.rb index 43cc0937b97..037eb88537a 100644 --- a/app/controllers/two_factor_authentication/otp_verification_controller.rb +++ b/app/controllers/two_factor_authentication/otp_verification_controller.rb @@ -127,6 +127,7 @@ def form_params def post_analytics(result) properties = result.to_h.merge(analytics_properties) + properties[:new_device] = DeviceCookie.check_for_new_device(cookies, current_user).nil? analytics.multi_factor_auth_setup(**properties) if context == 'confirmation' analytics.track_mfa_submit_event(properties) diff --git a/app/controllers/two_factor_authentication/personal_key_verification_controller.rb b/app/controllers/two_factor_authentication/personal_key_verification_controller.rb index c30e83ecc86..f758aeb4c64 100644 --- a/app/controllers/two_factor_authentication/personal_key_verification_controller.rb +++ b/app/controllers/two_factor_authentication/personal_key_verification_controller.rb @@ -26,6 +26,7 @@ def track_analytics(result) analytics_hash = result.to_h.merge( multi_factor_auth_method: 'personal-key', multi_factor_auth_method_created_at: mfa_created_at&.strftime('%s%L'), + new_device: DeviceCookie.check_for_new_device(cookies, current_user).nil?, ) analytics.track_mfa_submit_event(analytics_hash) 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 52555750d54..1fa89e6e12f 100644 --- a/app/controllers/two_factor_authentication/piv_cac_verification_controller.rb +++ b/app/controllers/two_factor_authentication/piv_cac_verification_controller.rb @@ -102,6 +102,7 @@ def analytics_properties context: context, multi_factor_auth_method: 'piv_cac', piv_cac_configuration_id: piv_cac_verification_form&.piv_cac_configuration&.id, + new_device: DeviceCookie.check_for_new_device(cookies, current_user).nil?, } 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 d55520cfdec..2ad87fa0157 100644 --- a/app/controllers/two_factor_authentication/totp_verification_controller.rb +++ b/app/controllers/two_factor_authentication/totp_verification_controller.rb @@ -18,8 +18,9 @@ def show def create result = TotpVerificationForm.new(current_user, params.require(:code).strip).submit - - analytics.track_mfa_submit_event(result.to_h) + analytics_result = result.to_h + analytics_result[:new_device] = DeviceCookie.check_for_new_device(cookies, current_user).nil? + analytics.track_mfa_submit_event(analytics_result) irs_attempts_api_tracker.mfa_login_totp(success: result.success?) if result.success? diff --git a/app/controllers/two_factor_authentication/webauthn_verification_controller.rb b/app/controllers/two_factor_authentication/webauthn_verification_controller.rb index 39b1e0a42ec..5e1f4262a77 100644 --- a/app/controllers/two_factor_authentication/webauthn_verification_controller.rb +++ b/app/controllers/two_factor_authentication/webauthn_verification_controller.rb @@ -19,6 +19,7 @@ def confirm **analytics_properties, multi_factor_auth_method_created_at: webauthn_configuration_or_latest.created_at.strftime('%s%L'), + new_device: DeviceCookie.check_for_new_device(cookies, current_user).nil?, ) if analytics_properties[:multi_factor_auth_method] == 'webauthn_platform' diff --git a/app/services/device_cookie.rb b/app/services/device_cookie.rb new file mode 100644 index 00000000000..6303e2947dc --- /dev/null +++ b/app/services/device_cookie.rb @@ -0,0 +1,8 @@ +class DeviceCookie + def self.check_for_new_device(cookies, current_user) + cookies[:device] && Device.find_by( + user_id: current_user.id, + cookie_uuid: cookies[:device], + ) + end +end diff --git a/app/services/user_event_creator.rb b/app/services/user_event_creator.rb index 922eb54c60c..1350b00dece 100644 --- a/app/services/user_event_creator.rb +++ b/app/services/user_event_creator.rb @@ -11,10 +11,7 @@ def initialize(current_user:, request: nil) # @return [Array(Event, String)] an (event, disavowal_token) tuple def create_user_event(event_type, user = current_user, disavowal_token = nil) return unless user&.id - existing_device = cookies[:device] && Device.find_by( - user_id: user.id, - cookie_uuid: cookies[:device], - ) + existing_device = DeviceCookie.check_for_new_device(cookies, current_user) if existing_device.present? create_event_for_existing_device( event_type: event_type, user: user, device: existing_device, @@ -77,19 +74,22 @@ def build_disavowal_token # @return [Array(Event, String)] an (event, disavowal_token) tuple def create_event_for_new_device(event_type:, user:, disavowal_token:) if user.fully_registered? && user.has_devices? && disavowal_token.nil? - device, event, disavowal_token = Device.transaction do - device = create_device_for_user(user) - event, disavowal_token = create_user_event_with_disavowal( - event_type, user, device + if event_type != :sign_in_before_2fa + device, event, disavowal_token = Device.transaction do + device = create_device_for_user(user) + event, disavowal_token = create_user_event_with_disavowal( + event_type, user, device + ) + [device, event, disavowal_token] + end + send_new_device_notification( + user: user, + device: device, + disavowal_token: disavowal_token, ) - [device, event, disavowal_token] + [event, disavowal_token] end - send_new_device_notification( - user: user, - device: device, - disavowal_token: disavowal_token, - ) - [event, disavowal_token] + else Device.transaction do device = create_device_for_user(user) From 0fa9d438fd614e5b2938ad2f29c2d66cfd3f0670 Mon Sep 17 00:00:00 2001 From: kevinsmaster5 Date: Mon, 18 Dec 2023 10:49:39 -0500 Subject: [PATCH 02/21] update test coverage to account for new device analytics property --- .../backup_code_verification_controller_spec.rb | 3 +++ .../otp_verification_controller_spec.rb | 6 ++++++ .../personal_key_verification_controller_spec.rb | 2 ++ .../piv_cac_verification_controller_spec.rb | 4 ++++ .../totp_verification_controller_spec.rb | 2 ++ .../webauthn_verification_controller_spec.rb | 5 ++++- 6 files changed, 21 insertions(+), 1 deletion(-) 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 ca4b86a68ff..cd775224eb3 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 @@ -33,6 +33,7 @@ errors: {}, multi_factor_auth_method: 'backup_code', multi_factor_auth_method_created_at: Time.zone.now.strftime('%s%L'), + new_device: true, } expect(@analytics).to receive(:track_mfa_submit_event). @@ -96,6 +97,7 @@ errors: {}, multi_factor_auth_method: 'backup_code', multi_factor_auth_method_created_at: Time.zone.now.strftime('%s%L'), + new_device: true, }) expect(@irs_attempts_api_tracker).to receive(:track_event). @@ -156,6 +158,7 @@ errors: {}, multi_factor_auth_method: 'backup_code', multi_factor_auth_method_created_at: nil, + new_device: true, } stub_analytics 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 71bea1af56f..839454f4d93 100644 --- a/spec/controllers/two_factor_authentication/otp_verification_controller_spec.rb +++ b/spec/controllers/two_factor_authentication/otp_verification_controller_spec.rb @@ -138,6 +138,7 @@ context: 'authentication', multi_factor_auth_method: 'sms', multi_factor_auth_method_created_at: phone_configuration_created_at.strftime('%s%L'), + new_device: true, phone_configuration_id: controller.current_user.default_phone_configuration.id, area_code: parsed_phone.area_code, country_code: parsed_phone.country, @@ -209,6 +210,7 @@ context: 'authentication', multi_factor_auth_method: 'sms', multi_factor_auth_method_created_at: phone_configuration_created_at.strftime('%s%L'), + new_device: true, phone_configuration_id: controller.current_user.default_phone_configuration.id, area_code: parsed_phone.area_code, country_code: parsed_phone.country, @@ -275,6 +277,7 @@ context: 'authentication', multi_factor_auth_method: 'sms', multi_factor_auth_method_created_at: phone_configuration_created_at.strftime('%s%L'), + new_device: true, phone_configuration_id: controller.current_user.default_phone_configuration.id, area_code: parsed_phone.area_code, country_code: parsed_phone.country, @@ -462,6 +465,7 @@ context: 'confirmation', multi_factor_auth_method: 'sms', multi_factor_auth_method_created_at: phone_configuration_created_at.strftime('%s%L'), + new_device: true, phone_configuration_id: phone_id, area_code: parsed_phone.area_code, country_code: parsed_phone.country, @@ -552,6 +556,7 @@ multi_factor_auth_method: 'sms', phone_configuration_id: controller.current_user.default_phone_configuration.id, multi_factor_auth_method_created_at: phone_configuration_created_at.strftime('%s%L'), + new_device: true, area_code: parsed_phone.area_code, country_code: parsed_phone.country, phone_fingerprint: Pii::Fingerprinter.fingerprint(parsed_phone.e164), @@ -634,6 +639,7 @@ context: 'confirmation', multi_factor_auth_method: 'sms', multi_factor_auth_method_created_at: nil, + new_device: true, confirmation_for_add_phone: false, phone_configuration_id: nil, area_code: parsed_phone.area_code, 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 fbcdfef9455..aec8dc0cba5 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 @@ -55,6 +55,7 @@ multi_factor_auth_method: 'personal-key', multi_factor_auth_method_created_at: user.reload. encrypted_recovery_code_digest_generated_at.strftime('%s%L'), + new_device: true, } expect(@analytics).to receive(:track_mfa_submit_event). @@ -187,6 +188,7 @@ error_details: { personal_key: { personal_key_incorrect: true } }, multi_factor_auth_method: 'personal-key', multi_factor_auth_method_created_at: personal_key_generated_at.strftime('%s%L'), + new_device: true, } expect(@analytics).to receive(:track_mfa_submit_event). 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 442f3ca293a..0c21a15db32 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 @@ -106,6 +106,7 @@ attributes = { context: 'authentication', multi_factor_auth_method: 'piv_cac', + new_device: true, piv_cac_configuration_id: nil, } @@ -117,6 +118,7 @@ errors: {}, context: 'authentication', multi_factor_auth_method: 'piv_cac', + new_device: true, multi_factor_auth_method_created_at: cfg.created_at.strftime('%s%L'), piv_cac_configuration_id: cfg.id, } @@ -203,6 +205,7 @@ attributes = { context: 'authentication', multi_factor_auth_method: 'piv_cac', + new_device: true, piv_cac_configuration_id: nil, } @@ -220,6 +223,7 @@ context: 'authentication', multi_factor_auth_method: 'piv_cac', multi_factor_auth_method_created_at: nil, + new_device: true, key_id: nil, piv_cac_configuration_id: nil, } 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 f711408c57f..e4124c45fd8 100644 --- a/spec/controllers/two_factor_authentication/totp_verification_controller_spec.rb +++ b/spec/controllers/two_factor_authentication/totp_verification_controller_spec.rb @@ -53,6 +53,7 @@ errors: {}, multi_factor_auth_method: 'totp', 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, } expect(@analytics).to receive(:track_mfa_submit_event). @@ -131,6 +132,7 @@ errors: {}, multi_factor_auth_method: 'totp', multi_factor_auth_method_created_at: nil, + new_device: true, auth_app_configuration_id: nil, } 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 8f1de36b8a6..f87ca2154b6 100644 --- a/spec/controllers/two_factor_authentication/webauthn_verification_controller_spec.rb +++ b/spec/controllers/two_factor_authentication/webauthn_verification_controller_spec.rb @@ -140,6 +140,7 @@ success: true, webauthn_configuration_id: webauthn_configuration.id, multi_factor_auth_method_created_at: webauthn_configuration.created_at.strftime('%s%L'), + new_device: true, } end @@ -226,7 +227,8 @@ error_details: { authenticator_data: { invalid_authenticator_data: true } }, webauthn_configuration_id: webauthn_configuration.id, multi_factor_auth_method_created_at: webauthn_configuration.created_at. - strftime('%s%L') } + strftime('%s%L'), + new_device: true } expect(@analytics).to receive(:track_mfa_submit_event). with(result) @@ -290,6 +292,7 @@ multi_factor_auth_method: 'webauthn_platform', multi_factor_auth_method_created_at: second_webauthn_platform_configuration.created_at.strftime('%s%L'), + new_device: true, webauthn_configuration_id: nil, frontend_error: webauthn_error, ) From f05856f55fc9686f6200802cd178415d0f623ebf Mon Sep 17 00:00:00 2001 From: kevinsmaster5 Date: Mon, 18 Dec 2023 13:04:06 -0500 Subject: [PATCH 03/21] remove unneeded changes --- app/services/device_cookie.rb | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/app/services/device_cookie.rb b/app/services/device_cookie.rb index 6303e2947dc..0895128f548 100644 --- a/app/services/device_cookie.rb +++ b/app/services/device_cookie.rb @@ -1,7 +1,8 @@ class DeviceCookie - def self.check_for_new_device(cookies, current_user) + def self.check_for_new_device(cookies, user) + return unless user&.id cookies[:device] && Device.find_by( - user_id: current_user.id, + user_id: user.id, cookie_uuid: cookies[:device], ) end From 949b6125c6adb1a1d8c8fc75d7aeb543f5256a0b Mon Sep 17 00:00:00 2001 From: kevinsmaster5 Date: Tue, 19 Dec 2023 10:01:35 -0500 Subject: [PATCH 04/21] change device tracking sign in to account for 2FA submit --- spec/features/new_device_tracking_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/features/new_device_tracking_spec.rb b/spec/features/new_device_tracking_spec.rb index d4a35e0f83a..5a5b9755b93 100644 --- a/spec/features/new_device_tracking_spec.rb +++ b/spec/features/new_device_tracking_spec.rb @@ -11,7 +11,7 @@ end it 'sends a user notification on signin' do - sign_in_user(user) + sign_in_live_with_2fa(user) expect(user.reload.devices.length).to eq 2 From b4dc743e8f94fc7c3551aaf79e1163197a245a6f Mon Sep 17 00:00:00 2001 From: kevinsmaster5 Date: Tue, 19 Dec 2023 10:25:44 -0500 Subject: [PATCH 05/21] change disavawal second sign in to account for 2FA submit --- spec/features/event_disavowal_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/features/event_disavowal_spec.rb b/spec/features/event_disavowal_spec.rb index 661bedc7130..e73d2910030 100644 --- a/spec/features/event_disavowal_spec.rb +++ b/spec/features/event_disavowal_spec.rb @@ -27,7 +27,7 @@ signin(user.email, user.password) Capybara.reset_session! visit root_path - signin(user.email, user.password) + sign_in_live_with_2fa(user) disavow_last_action_and_reset_password end From accc29c28fae3b97efb38610d7577f0e864a4991 Mon Sep 17 00:00:00 2001 From: kevinsmaster5 Date: Wed, 20 Dec 2023 14:24:05 -0500 Subject: [PATCH 06/21] add test for device cookie service --- spec/services/device_cookie_spec.rb | 36 +++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) create mode 100644 spec/services/device_cookie_spec.rb diff --git a/spec/services/device_cookie_spec.rb b/spec/services/device_cookie_spec.rb new file mode 100644 index 00000000000..15a416be6d3 --- /dev/null +++ b/spec/services/device_cookie_spec.rb @@ -0,0 +1,36 @@ +require 'rails_helper' + +RSpec.describe DeviceCookie do + let(:user_agent) { 'A computer on the internet' } + let(:ip_address) { '4.4.4.4' } + let(:existing_device_cookie) { 'existing_device_cookie' } + let(:cookie_jar) do + { + device: existing_device_cookie, + }.with_indifferent_access.tap do |cookie_jar| + allow(cookie_jar).to receive(:permanent).and_return({}) + end + end + let(:request) do + double( + remote_ip: ip_address, + user_agent: user_agent, + cookie_jar: cookie_jar, + ) + end + let(:user) { create(:user, :fully_registered) } + let(:device) { create(:device, user: user, cookie_uuid: existing_device_cookie) } + + before do + # Memoize user and device before specs run + user + device + end + describe '.device_cookie' do + it 'returns true if a matching device cookie is present' do + cookies = request.cookie_jar + device_present = DeviceCookie.check_for_new_device(cookies, user).present? + expect(device_present).to eq(true) + end + end +end From aa315801964cc5083f22fa63813ba502227a98a0 Mon Sep 17 00:00:00 2001 From: kevinsmaster5 Date: Wed, 20 Dec 2023 15:01:20 -0500 Subject: [PATCH 07/21] add new_device event to mfa properties hash --- 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 3559dd34b24..8be1ea53c6d 100644 --- a/app/services/analytics_events.rb +++ b/app/services/analytics_events.rb @@ -3003,6 +3003,7 @@ def logout_initiated( # @param [Boolean] success Whether authentication was successful # @param [Hash] errors Authentication error reasons, if unsuccessful # @param [String] context + # @param [Boolean] true if authenticated using a new device # @param [String] multi_factor_auth_method # @param [DateTime] multi_factor_auth_method_created_at time auth method was created # @param [Integer] auth_app_configuration_id @@ -3020,6 +3021,7 @@ def multi_factor_auth( success:, errors: nil, context: nil, + new_device: nil, multi_factor_auth_method: nil, multi_factor_auth_method_created_at: nil, auth_app_configuration_id: nil, @@ -3040,6 +3042,7 @@ def multi_factor_auth( success: success, errors: errors, context: context, + new_device: new_device, multi_factor_auth_method: multi_factor_auth_method, multi_factor_auth_method_created_at: multi_factor_auth_method_created_at, auth_app_configuration_id: auth_app_configuration_id, From e9ebcef7a90ac0bbfb6a730317adfd393dcc0f2c Mon Sep 17 00:00:00 2001 From: kevinsmaster5 Date: Wed, 20 Dec 2023 15:41:34 -0500 Subject: [PATCH 08/21] fix lint --- app/services/analytics_events.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/services/analytics_events.rb b/app/services/analytics_events.rb index 8be1ea53c6d..3a7efb8b678 100644 --- a/app/services/analytics_events.rb +++ b/app/services/analytics_events.rb @@ -3003,7 +3003,7 @@ def logout_initiated( # @param [Boolean] success Whether authentication was successful # @param [Hash] errors Authentication error reasons, if unsuccessful # @param [String] context - # @param [Boolean] true if authenticated using a new device + # @param [Boolean] new_device # @param [String] multi_factor_auth_method # @param [DateTime] multi_factor_auth_method_created_at time auth method was created # @param [Integer] auth_app_configuration_id From 7e3301f8092030800741174e5308e7b4e36788f4 Mon Sep 17 00:00:00 2001 From: kevinsmaster5 Date: Tue, 2 Jan 2024 09:20:13 -0500 Subject: [PATCH 09/21] remove DeviceCookie service. put as function in User controller. update dependencies --- .../backup_code_verification_controller.rb | 2 +- .../otp_verification_controller.rb | 2 +- .../personal_key_verification_controller.rb | 2 +- .../piv_cac_verification_controller.rb | 2 +- .../totp_verification_controller.rb | 2 +- .../webauthn_verification_controller.rb | 2 +- app/models/user.rb | 8 +++++ app/services/device_cookie.rb | 9 ----- app/services/user_event_creator.rb | 5 ++- spec/services/device_cookie_spec.rb | 36 ------------------- 10 files changed, 18 insertions(+), 52 deletions(-) delete mode 100644 app/services/device_cookie.rb delete mode 100644 spec/services/device_cookie_spec.rb diff --git a/app/controllers/two_factor_authentication/backup_code_verification_controller.rb b/app/controllers/two_factor_authentication/backup_code_verification_controller.rb index 809e7ee363e..0115ffc83c0 100644 --- a/app/controllers/two_factor_authentication/backup_code_verification_controller.rb +++ b/app/controllers/two_factor_authentication/backup_code_verification_controller.rb @@ -20,7 +20,7 @@ def create @backup_code_form = BackupCodeVerificationForm.new(current_user) result = @backup_code_form.submit(backup_code_params) analytics_result = result.to_h - analytics_result[:new_device] = DeviceCookie.check_for_new_device(cookies, current_user).nil? + analytics_result[:new_device] = current_user.new_device(cookies) analytics.track_mfa_submit_event(analytics_result) irs_attempts_api_tracker.mfa_login_backup_code(success: result.success?) handle_result(result) diff --git a/app/controllers/two_factor_authentication/otp_verification_controller.rb b/app/controllers/two_factor_authentication/otp_verification_controller.rb index 037eb88537a..ad7616af219 100644 --- a/app/controllers/two_factor_authentication/otp_verification_controller.rb +++ b/app/controllers/two_factor_authentication/otp_verification_controller.rb @@ -127,7 +127,7 @@ def form_params def post_analytics(result) properties = result.to_h.merge(analytics_properties) - properties[:new_device] = DeviceCookie.check_for_new_device(cookies, current_user).nil? + properties[:new_device] = current_user.new_device(cookies) analytics.multi_factor_auth_setup(**properties) if context == 'confirmation' analytics.track_mfa_submit_event(properties) diff --git a/app/controllers/two_factor_authentication/personal_key_verification_controller.rb b/app/controllers/two_factor_authentication/personal_key_verification_controller.rb index f758aeb4c64..b7785c3ed0a 100644 --- a/app/controllers/two_factor_authentication/personal_key_verification_controller.rb +++ b/app/controllers/two_factor_authentication/personal_key_verification_controller.rb @@ -26,7 +26,7 @@ def track_analytics(result) analytics_hash = result.to_h.merge( multi_factor_auth_method: 'personal-key', multi_factor_auth_method_created_at: mfa_created_at&.strftime('%s%L'), - new_device: DeviceCookie.check_for_new_device(cookies, current_user).nil?, + new_device: current_user.new_device(cookies), ) analytics.track_mfa_submit_event(analytics_hash) 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 1fa89e6e12f..206c9148e65 100644 --- a/app/controllers/two_factor_authentication/piv_cac_verification_controller.rb +++ b/app/controllers/two_factor_authentication/piv_cac_verification_controller.rb @@ -102,7 +102,7 @@ def analytics_properties context: context, multi_factor_auth_method: 'piv_cac', piv_cac_configuration_id: piv_cac_verification_form&.piv_cac_configuration&.id, - new_device: DeviceCookie.check_for_new_device(cookies, current_user).nil?, + new_device: current_user.new_device(cookies), } 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 2ad87fa0157..ca7d5f86bb1 100644 --- a/app/controllers/two_factor_authentication/totp_verification_controller.rb +++ b/app/controllers/two_factor_authentication/totp_verification_controller.rb @@ -19,7 +19,7 @@ def show def create result = TotpVerificationForm.new(current_user, params.require(:code).strip).submit analytics_result = result.to_h - analytics_result[:new_device] = DeviceCookie.check_for_new_device(cookies, current_user).nil? + analytics_result[:new_device] = current_user.new_device(cookies) analytics.track_mfa_submit_event(analytics_result) irs_attempts_api_tracker.mfa_login_totp(success: result.success?) diff --git a/app/controllers/two_factor_authentication/webauthn_verification_controller.rb b/app/controllers/two_factor_authentication/webauthn_verification_controller.rb index 5e1f4262a77..936e0928e21 100644 --- a/app/controllers/two_factor_authentication/webauthn_verification_controller.rb +++ b/app/controllers/two_factor_authentication/webauthn_verification_controller.rb @@ -19,7 +19,7 @@ def confirm **analytics_properties, multi_factor_auth_method_created_at: webauthn_configuration_or_latest.created_at.strftime('%s%L'), - new_device: DeviceCookie.check_for_new_device(cookies, current_user).nil?, + new_device: current_user.new_device(cookies), ) if analytics_properties[:multi_factor_auth_method] == 'webauthn_platform' diff --git a/app/models/user.rb b/app/models/user.rb index 7ded8bf2af1..6ebd4b3da63 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -400,6 +400,14 @@ def has_devices? !recent_devices.empty? end + def new_device(cookies) + existing_device = cookies[:device] && Device.find_by( + user_id: id, + cookie_uuid: cookies[:device], + ) + return existing_device.nil? + end + # Returns the number of times the user has signed in, corresponding to the `sign_in_before_2fa` # event. # diff --git a/app/services/device_cookie.rb b/app/services/device_cookie.rb deleted file mode 100644 index 0895128f548..00000000000 --- a/app/services/device_cookie.rb +++ /dev/null @@ -1,9 +0,0 @@ -class DeviceCookie - def self.check_for_new_device(cookies, user) - return unless user&.id - cookies[:device] && Device.find_by( - user_id: user.id, - cookie_uuid: cookies[:device], - ) - end -end diff --git a/app/services/user_event_creator.rb b/app/services/user_event_creator.rb index 1350b00dece..5c9b9ef6862 100644 --- a/app/services/user_event_creator.rb +++ b/app/services/user_event_creator.rb @@ -11,7 +11,10 @@ def initialize(current_user:, request: nil) # @return [Array(Event, String)] an (event, disavowal_token) tuple def create_user_event(event_type, user = current_user, disavowal_token = nil) return unless user&.id - existing_device = DeviceCookie.check_for_new_device(cookies, current_user) + existing_device = cookies[:device] && Device.find_by( + user_id: user.id, + cookie_uuid: cookies[:device], + ) if existing_device.present? create_event_for_existing_device( event_type: event_type, user: user, device: existing_device, diff --git a/spec/services/device_cookie_spec.rb b/spec/services/device_cookie_spec.rb deleted file mode 100644 index 15a416be6d3..00000000000 --- a/spec/services/device_cookie_spec.rb +++ /dev/null @@ -1,36 +0,0 @@ -require 'rails_helper' - -RSpec.describe DeviceCookie do - let(:user_agent) { 'A computer on the internet' } - let(:ip_address) { '4.4.4.4' } - let(:existing_device_cookie) { 'existing_device_cookie' } - let(:cookie_jar) do - { - device: existing_device_cookie, - }.with_indifferent_access.tap do |cookie_jar| - allow(cookie_jar).to receive(:permanent).and_return({}) - end - end - let(:request) do - double( - remote_ip: ip_address, - user_agent: user_agent, - cookie_jar: cookie_jar, - ) - end - let(:user) { create(:user, :fully_registered) } - let(:device) { create(:device, user: user, cookie_uuid: existing_device_cookie) } - - before do - # Memoize user and device before specs run - user - device - end - describe '.device_cookie' do - it 'returns true if a matching device cookie is present' do - cookies = request.cookie_jar - device_present = DeviceCookie.check_for_new_device(cookies, user).present? - expect(device_present).to eq(true) - end - end -end From c446ea6a0f9dd63db42ecabb0503071a0a70eea6 Mon Sep 17 00:00:00 2001 From: kevinsmaster5 Date: Tue, 2 Jan 2024 11:42:56 -0500 Subject: [PATCH 10/21] add #new_device to user spec --- spec/models/user_spec.rb | 49 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+) diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 74851f239d0..c286499a6f6 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -1495,6 +1495,55 @@ def it_should_not_send_survey end end + describe '#new_device' do + let(:user_agent) { 'A computer on the internet' } + let(:ip_address) { '4.4.4.4' } + let(:existing_device_cookie) { 'existing_device_cookie' } + let(:cookie_jar) do + { + device: existing_device_cookie, + }.with_indifferent_access.tap do |cookie_jar| + allow(cookie_jar).to receive(:permanent).and_return({}) + end + end + let(:request) do + double( + remote_ip: ip_address, + user_agent: user_agent, + cookie_jar: cookie_jar, + ) + end + let(:user) { create(:user, :fully_registered) } + let(:device) { create(:device, user: user, cookie_uuid: existing_device_cookie) } + + context 'with existing device' do + before do + # Memoize user and device before specs run + user + device + end + it 'does not expect a device to be new' do + cookies = request.cookie_jar + device_present = user.new_device(cookies) + expect(device_present).to eq(false) + end + end + + context 'with new device' do + let(:device) { create(:device, user: user, cookie_uuid: 'non_existing_device_cookie') } + before do + # Memoize user and device before specs run + user + device + end + it 'expects a new device' do + cookies = request.cookie_jar + device_present = user.new_device(cookies) + expect(device_present).to eq(true) + end + end + end + describe '#password_reset_profile' do let(:user) { create(:user) } From 20cdea897faa65ab33959d2cb5fc3e43053f4c09 Mon Sep 17 00:00:00 2001 From: kevinsmaster5 Date: Tue, 2 Jan 2024 14:35:38 -0500 Subject: [PATCH 11/21] leverage user session to store new device, fix some style inconsistencies --- .../backup_code_verification_controller.rb | 6 ++--- .../otp_verification_controller.rb | 3 +-- .../personal_key_verification_controller.rb | 2 +- .../piv_cac_verification_controller.rb | 2 +- .../totp_verification_controller.rb | 4 +-- .../webauthn_verification_controller.rb | 2 +- app/controllers/users/sessions_controller.rb | 1 + app/models/user.rb | 8 ++---- app/services/user_event_creator.rb | 25 ++++++++----------- 9 files changed, 22 insertions(+), 31 deletions(-) diff --git a/app/controllers/two_factor_authentication/backup_code_verification_controller.rb b/app/controllers/two_factor_authentication/backup_code_verification_controller.rb index 0115ffc83c0..d7818e091e1 100644 --- a/app/controllers/two_factor_authentication/backup_code_verification_controller.rb +++ b/app/controllers/two_factor_authentication/backup_code_verification_controller.rb @@ -19,9 +19,9 @@ def show def create @backup_code_form = BackupCodeVerificationForm.new(current_user) result = @backup_code_form.submit(backup_code_params) - analytics_result = result.to_h - analytics_result[:new_device] = current_user.new_device(cookies) - analytics.track_mfa_submit_event(analytics_result) + analytics.track_mfa_submit_event( + result.to_h.merge(new_device: user_session[:new_device]), + ) irs_attempts_api_tracker.mfa_login_backup_code(success: result.success?) handle_result(result) end diff --git a/app/controllers/two_factor_authentication/otp_verification_controller.rb b/app/controllers/two_factor_authentication/otp_verification_controller.rb index ad7616af219..2f57b2d6286 100644 --- a/app/controllers/two_factor_authentication/otp_verification_controller.rb +++ b/app/controllers/two_factor_authentication/otp_verification_controller.rb @@ -126,8 +126,7 @@ def form_params end def post_analytics(result) - properties = result.to_h.merge(analytics_properties) - properties[:new_device] = current_user.new_device(cookies) + properties = result.to_h.merge(analytics_properties, new_device: user_session[:new_device]) analytics.multi_factor_auth_setup(**properties) if context == 'confirmation' analytics.track_mfa_submit_event(properties) diff --git a/app/controllers/two_factor_authentication/personal_key_verification_controller.rb b/app/controllers/two_factor_authentication/personal_key_verification_controller.rb index b7785c3ed0a..1f46304ab10 100644 --- a/app/controllers/two_factor_authentication/personal_key_verification_controller.rb +++ b/app/controllers/two_factor_authentication/personal_key_verification_controller.rb @@ -26,7 +26,7 @@ def track_analytics(result) analytics_hash = result.to_h.merge( multi_factor_auth_method: 'personal-key', multi_factor_auth_method_created_at: mfa_created_at&.strftime('%s%L'), - new_device: current_user.new_device(cookies), + new_device: user_session[:new_device], ) analytics.track_mfa_submit_event(analytics_hash) 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 206c9148e65..78dd80fb34d 100644 --- a/app/controllers/two_factor_authentication/piv_cac_verification_controller.rb +++ b/app/controllers/two_factor_authentication/piv_cac_verification_controller.rb @@ -102,7 +102,7 @@ def analytics_properties context: context, multi_factor_auth_method: 'piv_cac', piv_cac_configuration_id: piv_cac_verification_form&.piv_cac_configuration&.id, - new_device: current_user.new_device(cookies), + new_device: user_session[:new_device], } 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 ca7d5f86bb1..0860c2b8c32 100644 --- a/app/controllers/two_factor_authentication/totp_verification_controller.rb +++ b/app/controllers/two_factor_authentication/totp_verification_controller.rb @@ -18,9 +18,7 @@ def show def create result = TotpVerificationForm.new(current_user, params.require(:code).strip).submit - analytics_result = result.to_h - analytics_result[:new_device] = current_user.new_device(cookies) - analytics.track_mfa_submit_event(analytics_result) + analytics.track_mfa_submit_event(result.to_h.merge(new_device: user_session[:new_device])) irs_attempts_api_tracker.mfa_login_totp(success: result.success?) if result.success? diff --git a/app/controllers/two_factor_authentication/webauthn_verification_controller.rb b/app/controllers/two_factor_authentication/webauthn_verification_controller.rb index 936e0928e21..df9c15076d9 100644 --- a/app/controllers/two_factor_authentication/webauthn_verification_controller.rb +++ b/app/controllers/two_factor_authentication/webauthn_verification_controller.rb @@ -19,7 +19,7 @@ def confirm **analytics_properties, multi_factor_auth_method_created_at: webauthn_configuration_or_latest.created_at.strftime('%s%L'), - new_device: current_user.new_device(cookies), + new_device: user_session[:new_device], ) if analytics_properties[:multi_factor_auth_method] == 'webauthn_platform' diff --git a/app/controllers/users/sessions_controller.rb b/app/controllers/users/sessions_controller.rb index e807997d4e6..2ef6cef5c70 100644 --- a/app/controllers/users/sessions_controller.rb +++ b/app/controllers/users/sessions_controller.rb @@ -115,6 +115,7 @@ def process_locked_out_user def handle_valid_authentication sign_in(resource_name, resource) cache_profiles(auth_params[:password]) + user_session[:new_device] = current_user.new_device?(device_cookie: cookies[:device]) create_user_event(:sign_in_before_2fa) EmailAddress.update_last_sign_in_at_on_user_id_and_email( user_id: current_user.id, diff --git a/app/models/user.rb b/app/models/user.rb index 6ebd4b3da63..e1fe81585ed 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -400,12 +400,8 @@ def has_devices? !recent_devices.empty? end - def new_device(cookies) - existing_device = cookies[:device] && Device.find_by( - user_id: id, - cookie_uuid: cookies[:device], - ) - return existing_device.nil? + def new_device?(device_cookie:) + !device_cookie || !devices.exists?(cookie_uuid: device_cookie) end # Returns the number of times the user has signed in, corresponding to the `sign_in_before_2fa` diff --git a/app/services/user_event_creator.rb b/app/services/user_event_creator.rb index 5c9b9ef6862..922eb54c60c 100644 --- a/app/services/user_event_creator.rb +++ b/app/services/user_event_creator.rb @@ -77,22 +77,19 @@ def build_disavowal_token # @return [Array(Event, String)] an (event, disavowal_token) tuple def create_event_for_new_device(event_type:, user:, disavowal_token:) if user.fully_registered? && user.has_devices? && disavowal_token.nil? - if event_type != :sign_in_before_2fa - device, event, disavowal_token = Device.transaction do - device = create_device_for_user(user) - event, disavowal_token = create_user_event_with_disavowal( - event_type, user, device - ) - [device, event, disavowal_token] - end - send_new_device_notification( - user: user, - device: device, - disavowal_token: disavowal_token, + device, event, disavowal_token = Device.transaction do + device = create_device_for_user(user) + event, disavowal_token = create_user_event_with_disavowal( + event_type, user, device ) - [event, disavowal_token] + [device, event, disavowal_token] end - + send_new_device_notification( + user: user, + device: device, + disavowal_token: disavowal_token, + ) + [event, disavowal_token] else Device.transaction do device = create_device_for_user(user) From d52cad734e3721a0811e0f521f361b736b649bcb Mon Sep 17 00:00:00 2001 From: kevinsmaster5 Date: Tue, 2 Jan 2024 16:23:56 -0500 Subject: [PATCH 12/21] update tests to include session value for new device --- .../backup_code_verification_controller_spec.rb | 7 +++++-- .../otp_verification_controller_spec.rb | 6 ++++++ .../personal_key_verification_controller_spec.rb | 4 +++- .../piv_cac_verification_controller_spec.rb | 2 ++ .../totp_verification_controller_spec.rb | 2 ++ .../webauthn_verification_controller_spec.rb | 4 +++- spec/models/user_spec.rb | 4 ++-- 7 files changed, 23 insertions(+), 6 deletions(-) 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 cd775224eb3..8a4fc849a92 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 @@ -22,10 +22,12 @@ describe '#create' do context 'when the user enters a valid backup code' do + it 'tracks the valid authentication event' do + freeze_time do sign_in_before_2fa(user) - + subject.user_session[:new_device] = true stub_analytics stub_attempts_tracker analytics_hash = { @@ -87,7 +89,7 @@ it 'tracks the valid authentication event when there are exisitng codes' do freeze_time do stub_sign_in_before_2fa(user) - + subject.user_session[:new_device] = true stub_analytics stub_attempts_tracker @@ -153,6 +155,7 @@ user.second_factor_attempts_count = IdentityConfig.store.login_otp_confirmation_max_attempts - 1 user.save + subject.user_session[:new_device] = true properties = { success: false, errors: {}, 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 839454f4d93..80204b92245 100644 --- a/spec/controllers/two_factor_authentication/otp_verification_controller_spec.rb +++ b/spec/controllers/two_factor_authentication/otp_verification_controller_spec.rb @@ -127,6 +127,7 @@ before do sign_in_before_2fa controller.user_session[:mfa_selections] = ['sms'] + subject.user_session[:new_device] = true expect(controller.current_user.reload.second_factor_attempts_count).to eq 0 phone_configuration_created_at = controller.current_user. default_phone_configuration.created_at @@ -200,6 +201,7 @@ ) sign_in_before_2fa(user) controller.user_session[:mfa_selections] = ['sms'] + subject.user_session[:new_device] = true phone_configuration_created_at = controller.current_user. default_phone_configuration.created_at @@ -246,6 +248,7 @@ before do sign_in_before_2fa subject.user_session[:mfa_selections] = ['sms'] + subject.user_session[:new_device] = true expect(subject.current_user.reload.second_factor_attempts_count).to eq 0 end @@ -452,6 +455,7 @@ before do subject.user_session[:mfa_selections] = ['sms'] subject.user_session[:in_account_creation_flow] = true + subject.user_session[:new_device] = true phone_configuration = MfaContext.new(subject.current_user).phone_configurations.last phone_id = phone_configuration.id parsed_phone = Phonelib.parse(phone_configuration.phone) @@ -509,6 +513,7 @@ context 'user enters an invalid code' do before do + subject.user_session[:new_device] = true expect(@irs_attempts_api_tracker).to receive(:mfa_enroll_phone_otp_submitted). with(success: false) @@ -618,6 +623,7 @@ context 'when given valid code' do before do + subject.user_session[:new_device] = true post( :create, params: { 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 aec8dc0cba5..5a37e2bb34d 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 @@ -3,7 +3,7 @@ RSpec.describe TwoFactorAuthentication::PersonalKeyVerificationController do let(:personal_key) { { personal_key: 'foo' } } let(:payload) { { personal_key_form: personal_key } } - + describe '#show' do context 'when there is no session (signed out or locked out), and the user reloads the page' do it 'redirects to the home page' do @@ -47,6 +47,7 @@ it 'tracks the valid authentication event' do personal_key sign_in_before_2fa(user) + subject.user_session[:new_device] = true stub_analytics analytics_hash = { @@ -179,6 +180,7 @@ user.save personal_key_generated_at = controller.current_user. encrypted_recovery_code_digest_generated_at + subject.user_session[:new_device] = true stub_analytics stub_attempts_tracker 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 0c21a15db32..da000efc8a7 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 @@ -61,6 +61,7 @@ context 'when the user presents a valid PIV/CAC' do before(:each) do stub_sign_in_before_2fa(user) + subject.user_session[:new_device] = true end it 'redirects to the profile' do @@ -198,6 +199,7 @@ IdentityConfig.store.login_otp_confirmation_max_attempts - 1 user.save stub_sign_in_before_2fa(user) + subject.user_session[:new_device] = true stub_analytics stub_attempts_tracker 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 e4124c45fd8..ba80229cfc7 100644 --- a/spec/controllers/two_factor_authentication/totp_verification_controller_spec.rb +++ b/spec/controllers/two_factor_authentication/totp_verification_controller_spec.rb @@ -10,6 +10,7 @@ context 'when the user enters a valid TOTP' do before do sign_in_before_2fa + subject.user_session[:new_device] = true @secret = subject.current_user.generate_totp_secret user = subject.current_user Db::AuthAppConfiguration.create(user, @secret, nil, 'foo') @@ -124,6 +125,7 @@ IdentityConfig.store.login_otp_confirmation_max_attempts - 1, ) sign_in_before_2fa(user) + subject.user_session[:new_device] = true @secret = user.generate_totp_secret Db::AuthAppConfiguration.create(user, @secret, nil, 'foo') 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 f87ca2154b6..827ffd28b43 100644 --- a/spec/controllers/two_factor_authentication/webauthn_verification_controller_spec.rb +++ b/spec/controllers/two_factor_authentication/webauthn_verification_controller_spec.rb @@ -146,6 +146,7 @@ before do allow(WebauthnVerificationForm).to receive(:domain_name).and_return('localhost:3000') + subject.user_session[:new_device] = true end it 'tracks a valid submission' do @@ -219,7 +220,7 @@ credential_id: credential_id, credential_public_key: credential_public_key, ) - + subject.user_session[:new_device] = true webauthn_configuration = controller.current_user.webauthn_configurations.first result = { context: 'authentication', multi_factor_auth_method: 'webauthn', @@ -250,6 +251,7 @@ before do controller.user_session[:webauthn_challenge] = webauthn_challenge + subject.user_session[:new_device] = true end let(:view_context) { ActionController::Base.new.view_context } diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index c286499a6f6..e0d33a6a62c 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -1524,7 +1524,7 @@ def it_should_not_send_survey end it 'does not expect a device to be new' do cookies = request.cookie_jar - device_present = user.new_device(cookies) + device_present = user.new_device?(device_cookie: cookies[:device]) expect(device_present).to eq(false) end end @@ -1538,7 +1538,7 @@ def it_should_not_send_survey end it 'expects a new device' do cookies = request.cookie_jar - device_present = user.new_device(cookies) + device_present = user.new_device?(device_cookie: cookies[:device]) expect(device_present).to eq(true) end end From c4d396788acf252fe1eae4db6fc8270276d55b5b Mon Sep 17 00:00:00 2001 From: kevinsmaster5 Date: Tue, 2 Jan 2024 16:25:40 -0500 Subject: [PATCH 13/21] fix lint --- .../backup_code_verification_controller_spec.rb | 2 -- .../personal_key_verification_controller_spec.rb | 2 +- 2 files changed, 1 insertion(+), 3 deletions(-) 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 8a4fc849a92..1cda99673e3 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 @@ -22,9 +22,7 @@ describe '#create' do context 'when the user enters a valid backup code' do - it 'tracks the valid authentication event' do - freeze_time do sign_in_before_2fa(user) subject.user_session[:new_device] = true 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 5a37e2bb34d..d1837811a8d 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 @@ -3,7 +3,7 @@ RSpec.describe TwoFactorAuthentication::PersonalKeyVerificationController do let(:personal_key) { { personal_key: 'foo' } } let(:payload) { { personal_key_form: personal_key } } - + describe '#show' do context 'when there is no session (signed out or locked out), and the user reloads the page' do it 'redirects to the home page' do From 4b614bb9045e048d19fbd537f9393976a25965d0 Mon Sep 17 00:00:00 2001 From: kevinsmaster5 Date: Tue, 2 Jan 2024 16:42:45 -0500 Subject: [PATCH 14/21] revert changes to spec features --- spec/features/event_disavowal_spec.rb | 2 +- spec/features/new_device_tracking_spec.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/features/event_disavowal_spec.rb b/spec/features/event_disavowal_spec.rb index e73d2910030..661bedc7130 100644 --- a/spec/features/event_disavowal_spec.rb +++ b/spec/features/event_disavowal_spec.rb @@ -27,7 +27,7 @@ signin(user.email, user.password) Capybara.reset_session! visit root_path - sign_in_live_with_2fa(user) + signin(user.email, user.password) disavow_last_action_and_reset_password end diff --git a/spec/features/new_device_tracking_spec.rb b/spec/features/new_device_tracking_spec.rb index 5a5b9755b93..d4a35e0f83a 100644 --- a/spec/features/new_device_tracking_spec.rb +++ b/spec/features/new_device_tracking_spec.rb @@ -11,7 +11,7 @@ end it 'sends a user notification on signin' do - sign_in_live_with_2fa(user) + sign_in_user(user) expect(user.reload.devices.length).to eq 2 From ea2f7e5054d3143dbfb73896504931e18995540b Mon Sep 17 00:00:00 2001 From: kevinsmaster5 Date: Wed, 3 Jan 2024 13:20:57 -0500 Subject: [PATCH 15/21] fix spec label --- spec/models/user_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index e0d33a6a62c..6ddffedb2d3 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -1495,7 +1495,7 @@ def it_should_not_send_survey end end - describe '#new_device' do + describe '#new_device?' do let(:user_agent) { 'A computer on the internet' } let(:ip_address) { '4.4.4.4' } let(:existing_device_cookie) { 'existing_device_cookie' } From 5ae8e42661dab739fe6fcf664c4bfb9962689a4c Mon Sep 17 00:00:00 2001 From: kevinsmaster5 Date: Wed, 3 Jan 2024 13:58:51 -0500 Subject: [PATCH 16/21] rename keyword for new_device function --- app/controllers/users/sessions_controller.rb | 2 +- app/models/user.rb | 4 ++-- .../webauthn_verification_controller_spec.rb | 4 ++-- spec/models/user_spec.rb | 4 ++-- 4 files changed, 7 insertions(+), 7 deletions(-) diff --git a/app/controllers/users/sessions_controller.rb b/app/controllers/users/sessions_controller.rb index 2ef6cef5c70..7b019aa33da 100644 --- a/app/controllers/users/sessions_controller.rb +++ b/app/controllers/users/sessions_controller.rb @@ -115,7 +115,7 @@ def process_locked_out_user def handle_valid_authentication sign_in(resource_name, resource) cache_profiles(auth_params[:password]) - user_session[:new_device] = current_user.new_device?(device_cookie: cookies[:device]) + user_session[:new_device] = current_user.new_device?(cookie_uuid: cookies[:device]) create_user_event(:sign_in_before_2fa) EmailAddress.update_last_sign_in_at_on_user_id_and_email( user_id: current_user.id, diff --git a/app/models/user.rb b/app/models/user.rb index e1fe81585ed..0aa69c5795e 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -400,8 +400,8 @@ def has_devices? !recent_devices.empty? end - def new_device?(device_cookie:) - !device_cookie || !devices.exists?(cookie_uuid: device_cookie) + def new_device?(cookie_uuid:) + !cookie_uuid || !devices.exists?(cookie_uuid:) end # Returns the number of times the user has signed in, corresponding to the `sign_in_before_2fa` 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 827ffd28b43..25714864e64 100644 --- a/spec/controllers/two_factor_authentication/webauthn_verification_controller_spec.rb +++ b/spec/controllers/two_factor_authentication/webauthn_verification_controller_spec.rb @@ -140,13 +140,13 @@ success: true, webauthn_configuration_id: webauthn_configuration.id, multi_factor_auth_method_created_at: webauthn_configuration.created_at.strftime('%s%L'), - new_device: true, + new_device: false, } end before do allow(WebauthnVerificationForm).to receive(:domain_name).and_return('localhost:3000') - subject.user_session[:new_device] = true + # subject.user_session[:new_device] = true end it 'tracks a valid submission' do diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 6ddffedb2d3..d77201be2c6 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -1524,7 +1524,7 @@ def it_should_not_send_survey end it 'does not expect a device to be new' do cookies = request.cookie_jar - device_present = user.new_device?(device_cookie: cookies[:device]) + device_present = user.new_device?(cookie_uuid: cookies[:device]) expect(device_present).to eq(false) end end @@ -1538,7 +1538,7 @@ def it_should_not_send_survey end it 'expects a new device' do cookies = request.cookie_jar - device_present = user.new_device?(device_cookie: cookies[:device]) + device_present = user.new_device?(cookie_uuid: cookies[:device]) expect(device_present).to eq(true) end end From 6c782ac66661147b60fe7f1f3e8574cd46b0776f Mon Sep 17 00:00:00 2001 From: kevinsmaster5 Date: Wed, 3 Jan 2024 16:18:51 -0500 Subject: [PATCH 17/21] update spec to account for nil device and also false --- ...ackup_code_verification_controller_spec.rb | 9 ++-- .../otp_verification_controller_spec.rb | 18 +++----- ...rsonal_key_verification_controller_spec.rb | 34 ++++++++++++-- .../piv_cac_verification_controller_spec.rb | 34 +++++++++++--- .../totp_verification_controller_spec.rb | 28 ++++++++++-- .../webauthn_verification_controller_spec.rb | 45 ++++++++++++++++--- 6 files changed, 130 insertions(+), 38 deletions(-) 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 1cda99673e3..56831a1464d 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 @@ -25,7 +25,6 @@ it 'tracks the valid authentication event' do freeze_time do sign_in_before_2fa(user) - subject.user_session[:new_device] = true stub_analytics stub_attempts_tracker analytics_hash = { @@ -33,7 +32,7 @@ errors: {}, multi_factor_auth_method: 'backup_code', multi_factor_auth_method_created_at: Time.zone.now.strftime('%s%L'), - new_device: true, + new_device: nil, } expect(@analytics).to receive(:track_mfa_submit_event). @@ -87,7 +86,6 @@ it 'tracks the valid authentication event when there are exisitng codes' do freeze_time do stub_sign_in_before_2fa(user) - subject.user_session[:new_device] = true stub_analytics stub_attempts_tracker @@ -97,7 +95,7 @@ errors: {}, multi_factor_auth_method: 'backup_code', multi_factor_auth_method_created_at: Time.zone.now.strftime('%s%L'), - new_device: true, + new_device: nil, }) expect(@irs_attempts_api_tracker).to receive(:track_event). @@ -153,13 +151,12 @@ user.second_factor_attempts_count = IdentityConfig.store.login_otp_confirmation_max_attempts - 1 user.save - subject.user_session[:new_device] = true properties = { success: false, errors: {}, multi_factor_auth_method: 'backup_code', multi_factor_auth_method_created_at: nil, - new_device: true, + new_device: nil, } stub_analytics 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 80204b92245..19a36c60bf8 100644 --- a/spec/controllers/two_factor_authentication/otp_verification_controller_spec.rb +++ b/spec/controllers/two_factor_authentication/otp_verification_controller_spec.rb @@ -127,7 +127,6 @@ before do sign_in_before_2fa controller.user_session[:mfa_selections] = ['sms'] - subject.user_session[:new_device] = true expect(controller.current_user.reload.second_factor_attempts_count).to eq 0 phone_configuration_created_at = controller.current_user. default_phone_configuration.created_at @@ -139,7 +138,7 @@ context: 'authentication', multi_factor_auth_method: 'sms', multi_factor_auth_method_created_at: phone_configuration_created_at.strftime('%s%L'), - new_device: true, + new_device: nil, phone_configuration_id: controller.current_user.default_phone_configuration.id, area_code: parsed_phone.area_code, country_code: parsed_phone.country, @@ -201,7 +200,6 @@ ) sign_in_before_2fa(user) controller.user_session[:mfa_selections] = ['sms'] - subject.user_session[:new_device] = true phone_configuration_created_at = controller.current_user. default_phone_configuration.created_at @@ -212,7 +210,7 @@ context: 'authentication', multi_factor_auth_method: 'sms', multi_factor_auth_method_created_at: phone_configuration_created_at.strftime('%s%L'), - new_device: true, + new_device: nil, phone_configuration_id: controller.current_user.default_phone_configuration.id, area_code: parsed_phone.area_code, country_code: parsed_phone.country, @@ -248,7 +246,6 @@ before do sign_in_before_2fa subject.user_session[:mfa_selections] = ['sms'] - subject.user_session[:new_device] = true expect(subject.current_user.reload.second_factor_attempts_count).to eq 0 end @@ -280,7 +277,7 @@ context: 'authentication', multi_factor_auth_method: 'sms', multi_factor_auth_method_created_at: phone_configuration_created_at.strftime('%s%L'), - new_device: true, + new_device: nil, phone_configuration_id: controller.current_user.default_phone_configuration.id, area_code: parsed_phone.area_code, country_code: parsed_phone.country, @@ -455,7 +452,6 @@ before do subject.user_session[:mfa_selections] = ['sms'] subject.user_session[:in_account_creation_flow] = true - subject.user_session[:new_device] = true phone_configuration = MfaContext.new(subject.current_user).phone_configurations.last phone_id = phone_configuration.id parsed_phone = Phonelib.parse(phone_configuration.phone) @@ -469,7 +465,7 @@ context: 'confirmation', multi_factor_auth_method: 'sms', multi_factor_auth_method_created_at: phone_configuration_created_at.strftime('%s%L'), - new_device: true, + new_device: nil, phone_configuration_id: phone_id, area_code: parsed_phone.area_code, country_code: parsed_phone.country, @@ -513,7 +509,6 @@ context 'user enters an invalid code' do before do - subject.user_session[:new_device] = true expect(@irs_attempts_api_tracker).to receive(:mfa_enroll_phone_otp_submitted). with(success: false) @@ -561,7 +556,7 @@ multi_factor_auth_method: 'sms', phone_configuration_id: controller.current_user.default_phone_configuration.id, multi_factor_auth_method_created_at: phone_configuration_created_at.strftime('%s%L'), - new_device: true, + new_device: nil, area_code: parsed_phone.area_code, country_code: parsed_phone.country, phone_fingerprint: Pii::Fingerprinter.fingerprint(parsed_phone.e164), @@ -623,7 +618,6 @@ context 'when given valid code' do before do - subject.user_session[:new_device] = true post( :create, params: { @@ -645,7 +639,7 @@ context: 'confirmation', multi_factor_auth_method: 'sms', multi_factor_auth_method_created_at: nil, - new_device: true, + new_device: nil, confirmation_for_add_phone: false, phone_configuration_id: nil, area_code: parsed_phone.area_code, 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 d1837811a8d..465c864fbae 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 @@ -47,7 +47,6 @@ it 'tracks the valid authentication event' do personal_key sign_in_before_2fa(user) - subject.user_session[:new_device] = true stub_analytics analytics_hash = { @@ -56,7 +55,7 @@ multi_factor_auth_method: 'personal-key', multi_factor_auth_method_created_at: user.reload. encrypted_recovery_code_digest_generated_at.strftime('%s%L'), - new_device: true, + new_device: nil, } expect(@analytics).to receive(:track_mfa_submit_event). @@ -110,6 +109,34 @@ end end + context 'with new device session value' do + let(:user) { create(:user, :with_phone) } + let(:personal_key) { { personal_key: PersonalKeyGenerator.new(user).create } } + let(:payload) { { personal_key_form: personal_key } } + + it 'tracks new device value' do + personal_key + sign_in_before_2fa(user) + stub_analytics + subject.user_session[:new_device] = false + analytics_hash = { + success: true, + errors: {}, + multi_factor_auth_method: 'personal-key', + multi_factor_auth_method_created_at: user.reload. + encrypted_recovery_code_digest_generated_at.strftime('%s%L'), + new_device: false, + } + + expect(@analytics).to receive(:track_mfa_submit_event). + with(analytics_hash) + + freeze_time do + post :create, params: payload + end + end + end + it 'does generate a new personal key after the user signs in with their old one' do user = create(:user) raw_key = PersonalKeyGenerator.new(user).create @@ -180,7 +207,6 @@ user.save personal_key_generated_at = controller.current_user. encrypted_recovery_code_digest_generated_at - subject.user_session[:new_device] = true stub_analytics stub_attempts_tracker @@ -190,7 +216,7 @@ error_details: { personal_key: { personal_key_incorrect: true } }, multi_factor_auth_method: 'personal-key', multi_factor_auth_method_created_at: personal_key_generated_at.strftime('%s%L'), - new_device: true, + new_device: nil, } expect(@analytics).to receive(:track_mfa_submit_event). 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 da000efc8a7..fd500029671 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 @@ -61,7 +61,6 @@ context 'when the user presents a valid PIV/CAC' do before(:each) do stub_sign_in_before_2fa(user) - subject.user_session[:new_device] = true end it 'redirects to the profile' do @@ -107,7 +106,7 @@ attributes = { context: 'authentication', multi_factor_auth_method: 'piv_cac', - new_device: true, + new_device: nil, piv_cac_configuration_id: nil, } @@ -119,7 +118,7 @@ errors: {}, context: 'authentication', multi_factor_auth_method: 'piv_cac', - new_device: true, + new_device: nil, multi_factor_auth_method_created_at: cfg.created_at.strftime('%s%L'), piv_cac_configuration_id: cfg.id, } @@ -136,6 +135,30 @@ get :show, params: { token: 'good-token' } end + + context 'with new device session value' do + before do + subject.user_session[:new_device] = false + end + it 'tracks new device value' do + stub_analytics + cfg = controller.current_user.piv_cac_configurations.first + + submit_attributes = { + success: true, + errors: {}, + context: 'authentication', + multi_factor_auth_method: 'piv_cac', + new_device: false, + multi_factor_auth_method_created_at: cfg.created_at.strftime('%s%L'), + piv_cac_configuration_id: cfg.id, + } + expect(@analytics).to receive(:track_mfa_submit_event). + with(submit_attributes) + + get :show, params: { token: 'good-token' } + end + end end context 'when the user presents an invalid piv/cac' do @@ -199,7 +222,6 @@ IdentityConfig.store.login_otp_confirmation_max_attempts - 1 user.save stub_sign_in_before_2fa(user) - subject.user_session[:new_device] = true stub_analytics stub_attempts_tracker @@ -207,7 +229,7 @@ attributes = { context: 'authentication', multi_factor_auth_method: 'piv_cac', - new_device: true, + new_device: nil, piv_cac_configuration_id: nil, } @@ -225,7 +247,7 @@ context: 'authentication', multi_factor_auth_method: 'piv_cac', multi_factor_auth_method_created_at: nil, - new_device: true, + new_device: nil, key_id: nil, piv_cac_configuration_id: nil, } 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 ba80229cfc7..e7893d5c255 100644 --- a/spec/controllers/two_factor_authentication/totp_verification_controller_spec.rb +++ b/spec/controllers/two_factor_authentication/totp_verification_controller_spec.rb @@ -10,7 +10,6 @@ context 'when the user enters a valid TOTP' do before do sign_in_before_2fa - subject.user_session[:new_device] = true @secret = subject.current_user.generate_totp_secret user = subject.current_user Db::AuthAppConfiguration.create(user, @secret, nil, 'foo') @@ -54,7 +53,7 @@ errors: {}, multi_factor_auth_method: 'totp', multi_factor_auth_method_created_at: cfg.created_at.strftime('%s%L'), - new_device: true, + new_device: nil, auth_app_configuration_id: controller.current_user.auth_app_configurations.first.id, } expect(@analytics).to receive(:track_mfa_submit_event). @@ -66,6 +65,28 @@ post :create, params: { code: generate_totp_code(@secret) } end + + context 'with new device session value' do + before do + subject.user_session[:new_device] = false + end + it 'tracks new device value' do + cfg = controller.current_user.auth_app_configurations.first + + attributes = { + success: true, + errors: {}, + multi_factor_auth_method: 'totp', + multi_factor_auth_method_created_at: cfg.created_at.strftime('%s%L'), + new_device: false, + auth_app_configuration_id: controller.current_user.auth_app_configurations.first.id, + } + expect(@analytics).to receive(:track_mfa_submit_event). + with(attributes) + + post :create, params: { code: generate_totp_code(@secret) } + end + end end context 'with multiple TOTP configurations' do @@ -125,7 +146,6 @@ IdentityConfig.store.login_otp_confirmation_max_attempts - 1, ) sign_in_before_2fa(user) - subject.user_session[:new_device] = true @secret = user.generate_totp_secret Db::AuthAppConfiguration.create(user, @secret, nil, 'foo') @@ -134,7 +154,7 @@ errors: {}, multi_factor_auth_method: 'totp', multi_factor_auth_method_created_at: nil, - new_device: true, + new_device: nil, auth_app_configuration_id: nil, } 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 25714864e64..668b48b6eb2 100644 --- a/spec/controllers/two_factor_authentication/webauthn_verification_controller_spec.rb +++ b/spec/controllers/two_factor_authentication/webauthn_verification_controller_spec.rb @@ -140,13 +140,12 @@ success: true, webauthn_configuration_id: webauthn_configuration.id, multi_factor_auth_method_created_at: webauthn_configuration.created_at.strftime('%s%L'), - new_device: false, + new_device: nil, } end before do allow(WebauthnVerificationForm).to receive(:domain_name).and_return('localhost:3000') - # subject.user_session[:new_device] = true end it 'tracks a valid submission' do @@ -213,6 +212,42 @@ end end + context 'with new device session value' do + let!(:webauthn_configuration) do + create( + :webauthn_configuration, + user: controller.current_user, + credential_id: credential_id, + credential_public_key: credential_public_key, + ) + controller.current_user.webauthn_configurations.first + end + let(:result) do + { + context: 'authentication', + multi_factor_auth_method: 'webauthn', + success: true, + webauthn_configuration_id: webauthn_configuration.id, + multi_factor_auth_method_created_at: webauthn_configuration.created_at.strftime('%s%L'), + new_device: false, + } + end + + before do + allow(WebauthnVerificationForm).to receive(:domain_name).and_return('localhost:3000') + subject.user_session[:new_device] = false + end + + it 'tracks new device value' do + expect(@analytics).to receive(:track_mfa_submit_event). + with(result) + + freeze_time do + patch :confirm, params: params + end + end + end + it 'tracks an invalid submission' do create( :webauthn_configuration, @@ -220,7 +255,6 @@ credential_id: credential_id, credential_public_key: credential_public_key, ) - subject.user_session[:new_device] = true webauthn_configuration = controller.current_user.webauthn_configurations.first result = { context: 'authentication', multi_factor_auth_method: 'webauthn', @@ -229,7 +263,7 @@ webauthn_configuration_id: webauthn_configuration.id, multi_factor_auth_method_created_at: webauthn_configuration.created_at. strftime('%s%L'), - new_device: true } + new_device: nil } expect(@analytics).to receive(:track_mfa_submit_event). with(result) @@ -251,7 +285,6 @@ before do controller.user_session[:webauthn_challenge] = webauthn_challenge - subject.user_session[:new_device] = true end let(:view_context) { ActionController::Base.new.view_context } @@ -294,7 +327,7 @@ multi_factor_auth_method: 'webauthn_platform', multi_factor_auth_method_created_at: second_webauthn_platform_configuration.created_at.strftime('%s%L'), - new_device: true, + new_device: nil, webauthn_configuration_id: nil, frontend_error: webauthn_error, ) From 2e22cb271c2fc6695de11bb58a7c435e82502ce2 Mon Sep 17 00:00:00 2001 From: kevinsmaster5 Date: Wed, 3 Jan 2024 16:21:00 -0500 Subject: [PATCH 18/21] fix lint --- .../personal_key_verification_controller_spec.rb | 2 +- .../piv_cac_verification_controller_spec.rb | 4 ++-- .../totp_verification_controller_spec.rb | 4 ++-- 3 files changed, 5 insertions(+), 5 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 465c864fbae..e39edbf07e9 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 @@ -113,7 +113,7 @@ let(:user) { create(:user, :with_phone) } let(:personal_key) { { personal_key: PersonalKeyGenerator.new(user).create } } let(:payload) { { personal_key_form: personal_key } } - + it 'tracks new device value' do personal_key sign_in_before_2fa(user) 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 fd500029671..0bb7aa5bf03 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 @@ -143,7 +143,7 @@ it 'tracks new device value' do stub_analytics cfg = controller.current_user.piv_cac_configurations.first - + submit_attributes = { success: true, errors: {}, @@ -155,7 +155,7 @@ } expect(@analytics).to receive(:track_mfa_submit_event). with(submit_attributes) - + get :show, params: { token: 'good-token' } end end 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 e7893d5c255..bfe42ca4446 100644 --- a/spec/controllers/two_factor_authentication/totp_verification_controller_spec.rb +++ b/spec/controllers/two_factor_authentication/totp_verification_controller_spec.rb @@ -72,7 +72,7 @@ end it 'tracks new device value' do cfg = controller.current_user.auth_app_configurations.first - + attributes = { success: true, errors: {}, @@ -83,7 +83,7 @@ } expect(@analytics).to receive(:track_mfa_submit_event). with(attributes) - + post :create, params: { code: generate_totp_code(@secret) } end end From 9d85956f79d0fed2390a4fbd88553eac2edd2ff9 Mon Sep 17 00:00:00 2001 From: kevinsmaster5 Date: Wed, 3 Jan 2024 16:49:50 -0500 Subject: [PATCH 19/21] add new device false tests --- ...ackup_code_verification_controller_spec.rb | 34 ++++++++++++++++++ .../otp_verification_controller_spec.rb | 35 +++++++++++++++++++ 2 files changed, 69 insertions(+) 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 56831a1464d..8df4dc200b2 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 @@ -109,6 +109,40 @@ end end + context 'with new device session value' do + it 'tracks new device value' do + freeze_time do + sign_in_before_2fa(user) + subject.user_session[:new_device] = false + stub_analytics + stub_attempts_tracker + analytics_hash = { + success: true, + errors: {}, + multi_factor_auth_method: 'backup_code', + multi_factor_auth_method_created_at: Time.zone.now.strftime('%s%L'), + new_device: false, + } + + expect(@analytics).to receive(:track_mfa_submit_event). + with(analytics_hash) + + expect(@irs_attempts_api_tracker).to receive(:track_event). + with(:mfa_login_backup_code, success: true) + + post :create, params: payload + + expect(subject.user_session[:auth_events]).to eq( + [ + auth_method: TwoFactorAuthenticatable::AuthMethod::BACKUP_CODE, + at: Time.zone.now, + ], + ) + expect(subject.user_session[TwoFactorAuthenticatable::NEED_AUTHENTICATION]).to eq false + end + end + end + context 'when the backup code field is empty' do let(:backup_code) { { backup_code: '' } } let(:payload) { { backup_code_verification_form: backup_code } } 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 19a36c60bf8..1925dbc161e 100644 --- a/spec/controllers/two_factor_authentication/otp_verification_controller_spec.rb +++ b/spec/controllers/two_factor_authentication/otp_verification_controller_spec.rb @@ -659,6 +659,41 @@ it 'resets context to authentication' do expect(subject.user_session[:context]).to eq 'authentication' end + + context 'with new device session value' do + before do + subject.user_session[:new_device] = false + post( + :create, + params: { + code: subject.current_user.direct_otp, + otp_delivery_preference: 'sms', + }, + ) + end + it 'tracks new device value' do + subject.user_session[:new_device] = false + parsed_phone = Phonelib.parse('+1 (703) 555-5555') + properties = { + success: true, + errors: nil, + context: 'confirmation', + multi_factor_auth_method: 'sms', + multi_factor_auth_method_created_at: nil, + new_device: nil, + confirmation_for_add_phone: false, + phone_configuration_id: nil, + area_code: parsed_phone.area_code, + country_code: parsed_phone.country, + phone_fingerprint: Pii::Fingerprinter.fingerprint(parsed_phone.e164), + enabled_mfa_methods_count: 0, + in_account_creation_flow: false, + } + + expect(@analytics).to have_received(:track_event). + with('Multi-Factor Authentication Setup', properties) + end + end end describe 'multiple MFA handling' do From be65b95ceb92fb849738a0c1ec5248022fc01329 Mon Sep 17 00:00:00 2001 From: kevinsmaster5 Date: Thu, 4 Jan 2024 11:53:29 -0500 Subject: [PATCH 20/21] remove unneeded user_session override --- .../otp_verification_controller_spec.rb | 1 - 1 file changed, 1 deletion(-) 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 1925dbc161e..0d0e5f491a7 100644 --- a/spec/controllers/two_factor_authentication/otp_verification_controller_spec.rb +++ b/spec/controllers/two_factor_authentication/otp_verification_controller_spec.rb @@ -672,7 +672,6 @@ ) end it 'tracks new device value' do - subject.user_session[:new_device] = false parsed_phone = Phonelib.parse('+1 (703) 555-5555') properties = { success: true, From b093dc869a7f62d1b40ce66586bdd8e2f90e3ffc Mon Sep 17 00:00:00 2001 From: kevinsmaster5 Date: Thu, 4 Jan 2024 12:54:38 -0500 Subject: [PATCH 21/21] debug otp spec --- .../otp_verification_controller_spec.rb | 68 +++++++++---------- 1 file changed, 34 insertions(+), 34 deletions(-) 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 0d0e5f491a7..608a57dc45f 100644 --- a/spec/controllers/two_factor_authentication/otp_verification_controller_spec.rb +++ b/spec/controllers/two_factor_authentication/otp_verification_controller_spec.rb @@ -315,6 +315,40 @@ end end + context 'with new device session value' do + it 'tracks new device value' do + subject.user_session[:new_device] = false + phone_configuration_created_at = controller.current_user. + default_phone_configuration.created_at + properties = { + success: true, + confirmation_for_add_phone: false, + context: 'authentication', + multi_factor_auth_method: 'sms', + multi_factor_auth_method_created_at: phone_configuration_created_at.strftime('%s%L'), + new_device: false, + phone_configuration_id: controller.current_user.default_phone_configuration.id, + 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, + } + + stub_analytics + + expect(@analytics).to receive(:track_mfa_submit_event). + with(properties) + + freeze_time do + post :create, params: { + code: subject.current_user.reload.direct_otp, + otp_delivery_preference: 'sms', + } + end + end + end + context "with a leading '#' sign" do it 'redirects to the profile' do post :create, params: { @@ -659,40 +693,6 @@ it 'resets context to authentication' do expect(subject.user_session[:context]).to eq 'authentication' end - - context 'with new device session value' do - before do - subject.user_session[:new_device] = false - post( - :create, - params: { - code: subject.current_user.direct_otp, - otp_delivery_preference: 'sms', - }, - ) - end - it 'tracks new device value' do - parsed_phone = Phonelib.parse('+1 (703) 555-5555') - properties = { - success: true, - errors: nil, - context: 'confirmation', - multi_factor_auth_method: 'sms', - multi_factor_auth_method_created_at: nil, - new_device: nil, - confirmation_for_add_phone: false, - phone_configuration_id: nil, - area_code: parsed_phone.area_code, - country_code: parsed_phone.country, - phone_fingerprint: Pii::Fingerprinter.fingerprint(parsed_phone.e164), - enabled_mfa_methods_count: 0, - in_account_creation_flow: false, - } - - expect(@analytics).to have_received(:track_event). - with('Multi-Factor Authentication Setup', properties) - end - end end describe 'multiple MFA handling' do