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 4e8c05c55e3..28befc982e4 100644 --- a/app/controllers/two_factor_authentication/backup_code_verification_controller.rb +++ b/app/controllers/two_factor_authentication/backup_code_verification_controller.rb @@ -3,6 +3,7 @@ module TwoFactorAuthentication class BackupCodeVerificationController < ApplicationController include TwoFactorAuthenticatable + include NewDeviceConcern prepend_before_action :authenticate_user before_action :check_sp_required_mfa @@ -22,7 +23,7 @@ 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.merge(new_device: user_session[:new_device]), + result.to_h.merge(new_device: new_device?), ) 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 31d836e8c88..3bf40093f41 100644 --- a/app/controllers/two_factor_authentication/otp_verification_controller.rb +++ b/app/controllers/two_factor_authentication/otp_verification_controller.rb @@ -4,6 +4,7 @@ module TwoFactorAuthentication class OtpVerificationController < ApplicationController include TwoFactorAuthenticatable include MfaSetupConcern + include NewDeviceConcern before_action :check_sp_required_mfa before_action :confirm_multiple_factors_enabled @@ -132,7 +133,7 @@ def form_params end def post_analytics(result) - properties = result.to_h.merge(analytics_properties, new_device: user_session[:new_device]) + properties = result.to_h.merge(analytics_properties, new_device: 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 64b0a8c85c8..85302e11991 100644 --- a/app/controllers/two_factor_authentication/personal_key_verification_controller.rb +++ b/app/controllers/two_factor_authentication/personal_key_verification_controller.rb @@ -3,6 +3,7 @@ module TwoFactorAuthentication class PersonalKeyVerificationController < ApplicationController include TwoFactorAuthenticatable + include NewDeviceConcern prepend_before_action :authenticate_user before_action :check_personal_key_enabled @@ -28,7 +29,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: user_session[:new_device], + new_device: 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 39b89f881fa..56151472bfa 100644 --- a/app/controllers/two_factor_authentication/piv_cac_verification_controller.rb +++ b/app/controllers/two_factor_authentication/piv_cac_verification_controller.rb @@ -4,6 +4,7 @@ module TwoFactorAuthentication class PivCacVerificationController < ApplicationController include TwoFactorAuthenticatable include PivCacConcern + include NewDeviceConcern before_action :confirm_piv_cac_enabled, only: :show before_action :reset_attempt_count_if_user_no_longer_locked_out, only: :show @@ -105,7 +106,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: user_session[:new_device], + new_device: 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 eaed664380a..111c8a52c1b 100644 --- a/app/controllers/two_factor_authentication/totp_verification_controller.rb +++ b/app/controllers/two_factor_authentication/totp_verification_controller.rb @@ -3,6 +3,7 @@ module TwoFactorAuthentication class TotpVerificationController < ApplicationController include TwoFactorAuthenticatable + include NewDeviceConcern before_action :check_sp_required_mfa before_action :confirm_totp_enabled @@ -20,7 +21,7 @@ def show def create result = TotpVerificationForm.new(current_user, params.require(:code).strip).submit - analytics.track_mfa_submit_event(result.to_h.merge(new_device: user_session[:new_device])) + analytics.track_mfa_submit_event(result.to_h.merge(new_device: 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 57072bccfc6..458ebf650a8 100644 --- a/app/controllers/two_factor_authentication/webauthn_verification_controller.rb +++ b/app/controllers/two_factor_authentication/webauthn_verification_controller.rb @@ -4,6 +4,7 @@ module TwoFactorAuthentication # The WebauthnVerificationController class is responsible webauthn verification at sign in class WebauthnVerificationController < ApplicationController include TwoFactorAuthenticatable + include NewDeviceConcern before_action :check_sp_required_mfa before_action :check_if_device_supports_platform_auth, only: :show @@ -22,7 +23,7 @@ def confirm **analytics_properties, multi_factor_auth_method_created_at: webauthn_configuration_or_latest.created_at.strftime('%s%L'), - new_device: user_session[:new_device], + new_device: new_device?, ) if analytics_properties[:multi_factor_auth_method] == 'webauthn_platform' 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 8c2cb22cff7..a1f1118228b 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 @@ -32,7 +32,7 @@ errors: {}, multi_factor_auth_method: 'backup_code', multi_factor_auth_method_created_at: Time.zone.now.strftime('%s%L'), - new_device: nil, + new_device: true, } expect(@analytics).to receive(:track_mfa_submit_event). @@ -99,7 +99,7 @@ errors: {}, multi_factor_auth_method: 'backup_code', multi_factor_auth_method_created_at: Time.zone.now.strftime('%s%L'), - new_device: nil, + new_device: true, }) expect(@irs_attempts_api_tracker).to receive(:track_event). @@ -113,37 +113,19 @@ 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) + context 'with existing device' do + before do + allow(controller).to receive(:new_device?).and_return(false) + end - expect(@irs_attempts_api_tracker).to receive(:track_event). - with(:mfa_login_backup_code, success: true) + it 'tracks new device value' do + stub_analytics + stub_sign_in_before_2fa(user) - post :create, params: payload + expect(@analytics).to receive(:track_mfa_submit_event). + with(hash_including(new_device: false)) - 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 + post :create, params: payload end end @@ -194,7 +176,7 @@ errors: {}, multi_factor_auth_method: 'backup_code', multi_factor_auth_method_created_at: nil, - new_device: 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 331c859a614..2cc1a037dfa 100644 --- a/spec/controllers/two_factor_authentication/otp_verification_controller_spec.rb +++ b/spec/controllers/two_factor_authentication/otp_verification_controller_spec.rb @@ -140,7 +140,7 @@ context: 'authentication', multi_factor_auth_method: 'sms', multi_factor_auth_method_created_at: phone_configuration_created_at.strftime('%s%L'), - new_device: nil, + new_device: true, phone_configuration_id: controller.current_user.default_phone_configuration.id, area_code: parsed_phone.area_code, country_code: parsed_phone.country, @@ -220,7 +220,7 @@ context: 'authentication', multi_factor_auth_method: 'sms', multi_factor_auth_method_created_at: phone_configuration_created_at.strftime('%s%L'), - new_device: nil, + new_device: true, phone_configuration_id: controller.current_user.default_phone_configuration.id, area_code: parsed_phone.area_code, country_code: parsed_phone.country, @@ -287,7 +287,7 @@ context: 'authentication', multi_factor_auth_method: 'sms', multi_factor_auth_method_created_at: phone_configuration_created_at.strftime('%s%L'), - new_device: nil, + new_device: true, phone_configuration_id: controller.current_user.default_phone_configuration.id, area_code: parsed_phone.area_code, country_code: parsed_phone.country, @@ -329,37 +329,21 @@ 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, - } + context 'with existing device' do + before do + allow(controller).to receive(:new_device?).and_return(false) + end + it 'tracks new device value' do stub_analytics expect(@analytics).to receive(:track_mfa_submit_event). - with(properties) + with(hash_including(new_device: false)) - freeze_time do - post :create, params: { - code: subject.current_user.reload.direct_otp, - otp_delivery_preference: 'sms', - } - end + post :create, params: { + code: subject.current_user.reload.direct_otp, + otp_delivery_preference: 'sms', + } end end @@ -512,7 +496,7 @@ context: 'confirmation', multi_factor_auth_method: 'sms', multi_factor_auth_method_created_at: phone_configuration_created_at.strftime('%s%L'), - new_device: nil, + new_device: true, phone_configuration_id: phone_id, area_code: parsed_phone.area_code, country_code: parsed_phone.country, @@ -603,7 +587,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: nil, + new_device: true, area_code: parsed_phone.area_code, country_code: parsed_phone.country, phone_fingerprint: Pii::Fingerprinter.fingerprint(parsed_phone.e164), @@ -685,7 +669,7 @@ context: 'confirmation', multi_factor_auth_method: 'sms', multi_factor_auth_method_created_at: nil, - new_device: 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 0a3a6b46ad1..2baef2c7369 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 @@ -56,7 +56,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: nil, + new_device: true, } expect(@analytics).to receive(:track_mfa_submit_event). @@ -112,31 +112,19 @@ expect(response).to redirect_to(account_path) end 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 } } + context 'with existing device' do + before do + allow(controller).to receive(:new_device?).and_return(false) + end - 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, - } + it 'tracks new device value' do + stub_analytics + stub_sign_in_before_2fa(user) - expect(@analytics).to receive(:track_mfa_submit_event). - with(analytics_hash) + expect(@analytics).to receive(:track_mfa_submit_event). + with(hash_including(new_device: false)) - freeze_time do post :create, params: payload end end @@ -221,7 +209,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: nil, + 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 520d84c26f3..e62ea8697cd 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 @@ -108,7 +108,7 @@ attributes = { context: 'authentication', multi_factor_auth_method: 'piv_cac', - new_device: nil, + new_device: true, piv_cac_configuration_id: nil, } @@ -120,7 +120,7 @@ errors: {}, context: 'authentication', multi_factor_auth_method: 'piv_cac', - new_device: nil, + new_device: true, multi_factor_auth_method_created_at: cfg.created_at.strftime('%s%L'), piv_cac_configuration_id: cfg.id, piv_cac_configuration_dn_uuid: cfg.x509_dn_uuid, @@ -144,27 +144,17 @@ get :show, params: { token: 'good-token' } end - context 'with new device session value' do + context 'with existing device' do before do - subject.user_session[:new_device] = false + allow(controller).to receive(:new_device?).and_return(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, - piv_cac_configuration_dn_uuid: cfg.x509_dn_uuid, - key_id: 'foo', - } + stub_sign_in_before_2fa(user) + expect(@analytics).to receive(:track_mfa_submit_event). - with(submit_attributes) + with(hash_including(new_device: false)) get :show, params: { token: 'good-token' } end @@ -253,7 +243,7 @@ attributes = { context: 'authentication', multi_factor_auth_method: 'piv_cac', - new_device: nil, + new_device: true, piv_cac_configuration_id: nil, } @@ -271,7 +261,7 @@ context: 'authentication', multi_factor_auth_method: 'piv_cac', multi_factor_auth_method_created_at: nil, - new_device: nil, + new_device: true, key_id: 'foo', piv_cac_configuration_dn_uuid: 'bad-uuid', 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 617078830ea..5cc253b1c51 100644 --- a/spec/controllers/two_factor_authentication/totp_verification_controller_spec.rb +++ b/spec/controllers/two_factor_authentication/totp_verification_controller_spec.rb @@ -50,7 +50,7 @@ errors: {}, multi_factor_auth_method: 'totp', multi_factor_auth_method_created_at: cfg.created_at.strftime('%s%L'), - new_device: nil, + new_device: true, auth_app_configuration_id: controller.current_user.auth_app_configurations.first.id, } expect(@analytics).to receive(:track_mfa_submit_event). @@ -66,23 +66,16 @@ post :create, params: { code: generate_totp_code(@secret) } end - context 'with new device session value' do + context 'with existing device' do before do - subject.user_session[:new_device] = false + allow(controller).to receive(:new_device?).and_return(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, - } + stub_analytics + expect(@analytics).to receive(:track_mfa_submit_event). - with(attributes) + with(hash_including(new_device: false)) post :create, params: { code: generate_totp_code(@secret) } end @@ -167,7 +160,7 @@ errors: {}, multi_factor_auth_method: 'totp', multi_factor_auth_method_created_at: nil, - new_device: 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 8fc1ce922e5..802b619982b 100644 --- a/spec/controllers/two_factor_authentication/webauthn_verification_controller_spec.rb +++ b/spec/controllers/two_factor_authentication/webauthn_verification_controller_spec.rb @@ -140,7 +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: nil, + new_device: true, } end @@ -175,6 +175,21 @@ end end + context 'with existing device' do + before do + allow(controller).to receive(:new_device?).and_return(false) + end + + it 'tracks new device value' do + stub_analytics + + expect(@analytics).to receive(:track_mfa_submit_event). + with(hash_including(new_device: false)) + + patch :confirm, params: params + end + end + context 'with platform authenticator' do let!(:webauthn_configuration) do create( @@ -215,42 +230,6 @@ 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, @@ -266,7 +245,7 @@ webauthn_configuration_id: webauthn_configuration.id, multi_factor_auth_method_created_at: webauthn_configuration.created_at. strftime('%s%L'), - new_device: nil } + new_device: true } expect(@analytics).to receive(:track_mfa_submit_event). with(result) expect(controller).to receive(:create_user_event).with(:sign_in_unsuccessful_2fa) @@ -331,7 +310,7 @@ multi_factor_auth_method: 'webauthn_platform', multi_factor_auth_method_created_at: second_webauthn_platform_configuration.created_at.strftime('%s%L'), - new_device: nil, + new_device: true, webauthn_configuration_id: nil, frontend_error: webauthn_error, )