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..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,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.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 43cc0937b97..2f57b2d6286 100644 --- a/app/controllers/two_factor_authentication/otp_verification_controller.rb +++ b/app/controllers/two_factor_authentication/otp_verification_controller.rb @@ -126,7 +126,7 @@ def form_params end def post_analytics(result) - properties = result.to_h.merge(analytics_properties) + 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 c30e83ecc86..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,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: 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 52555750d54..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,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: 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 d55520cfdec..0860c2b8c32 100644 --- a/app/controllers/two_factor_authentication/totp_verification_controller.rb +++ b/app/controllers/two_factor_authentication/totp_verification_controller.rb @@ -18,8 +18,7 @@ def show def create result = TotpVerificationForm.new(current_user, params.require(:code).strip).submit - - analytics.track_mfa_submit_event(result.to_h) + 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 39b1e0a42ec..df9c15076d9 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: 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..7b019aa33da 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?(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 7ded8bf2af1..0aa69c5795e 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -400,6 +400,10 @@ def has_devices? !recent_devices.empty? end + 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` # event. # diff --git a/app/services/analytics_events.rb b/app/services/analytics_events.rb index 3559dd34b24..3a7efb8b678 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] 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, 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..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 @@ -25,7 +25,6 @@ it 'tracks the valid authentication event' do freeze_time do sign_in_before_2fa(user) - stub_analytics stub_attempts_tracker analytics_hash = { @@ -33,6 +32,7 @@ errors: {}, multi_factor_auth_method: 'backup_code', multi_factor_auth_method_created_at: Time.zone.now.strftime('%s%L'), + new_device: nil, } expect(@analytics).to receive(:track_mfa_submit_event). @@ -86,7 +86,6 @@ it 'tracks the valid authentication event when there are exisitng codes' do freeze_time do stub_sign_in_before_2fa(user) - stub_analytics stub_attempts_tracker @@ -96,6 +95,7 @@ errors: {}, multi_factor_auth_method: 'backup_code', multi_factor_auth_method_created_at: Time.zone.now.strftime('%s%L'), + new_device: nil, }) expect(@irs_attempts_api_tracker).to receive(:track_event). @@ -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 } } @@ -156,6 +190,7 @@ errors: {}, multi_factor_auth_method: 'backup_code', multi_factor_auth_method_created_at: nil, + 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 71bea1af56f..608a57dc45f 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: nil, 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: nil, 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: nil, phone_configuration_id: controller.current_user.default_phone_configuration.id, area_code: parsed_phone.area_code, country_code: parsed_phone.country, @@ -312,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: { @@ -462,6 +499,7 @@ context: 'confirmation', multi_factor_auth_method: 'sms', multi_factor_auth_method_created_at: phone_configuration_created_at.strftime('%s%L'), + new_device: nil, phone_configuration_id: phone_id, area_code: parsed_phone.area_code, country_code: parsed_phone.country, @@ -552,6 +590,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, area_code: parsed_phone.area_code, country_code: parsed_phone.country, phone_fingerprint: Pii::Fingerprinter.fingerprint(parsed_phone.e164), @@ -634,6 +673,7 @@ 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, 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..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 @@ -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: nil, } expect(@analytics).to receive(:track_mfa_submit_event). @@ -108,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 @@ -187,6 +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: 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 442f3ca293a..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 @@ -106,6 +106,7 @@ attributes = { context: 'authentication', multi_factor_auth_method: 'piv_cac', + new_device: nil, piv_cac_configuration_id: nil, } @@ -117,6 +118,7 @@ errors: {}, context: 'authentication', multi_factor_auth_method: 'piv_cac', + new_device: nil, multi_factor_auth_method_created_at: cfg.created_at.strftime('%s%L'), piv_cac_configuration_id: cfg.id, } @@ -133,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 @@ -203,6 +229,7 @@ attributes = { context: 'authentication', multi_factor_auth_method: 'piv_cac', + new_device: nil, piv_cac_configuration_id: nil, } @@ -220,6 +247,7 @@ context: 'authentication', multi_factor_auth_method: 'piv_cac', multi_factor_auth_method_created_at: nil, + 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 f711408c57f..bfe42ca4446 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: nil, auth_app_configuration_id: controller.current_user.auth_app_configurations.first.id, } expect(@analytics).to receive(:track_mfa_submit_event). @@ -64,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 @@ -131,6 +154,7 @@ errors: {}, multi_factor_auth_method: 'totp', multi_factor_auth_method_created_at: nil, + 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 8f1de36b8a6..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,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: nil, } end @@ -211,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, @@ -218,7 +255,6 @@ credential_id: credential_id, credential_public_key: credential_public_key, ) - webauthn_configuration = controller.current_user.webauthn_configurations.first result = { context: 'authentication', multi_factor_auth_method: 'webauthn', @@ -226,7 +262,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: nil } expect(@analytics).to receive(:track_mfa_submit_event). with(result) @@ -290,6 +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: nil, webauthn_configuration_id: nil, frontend_error: webauthn_error, ) diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 74851f239d0..d77201be2c6 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?(cookie_uuid: cookies[:device]) + 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?(cookie_uuid: cookies[:device]) + expect(device_present).to eq(true) + end + end + end + describe '#password_reset_profile' do let(:user) { create(:user) }