diff --git a/app/controllers/concerns/two_factor_authenticatable_methods.rb b/app/controllers/concerns/two_factor_authenticatable_methods.rb index a7e37560ef0..6f0b9ecda47 100644 --- a/app/controllers/concerns/two_factor_authenticatable_methods.rb +++ b/app/controllers/concerns/two_factor_authenticatable_methods.rb @@ -10,6 +10,26 @@ def auth_methods_session @auth_methods_session ||= AuthMethodsSession.new(user_session:) end + def handle_valid_verification_for_authentication_context(auth_method:) + mark_user_session_authenticated(auth_method:, authentication_type: :valid_2fa) + disavowal_event, disavowal_token = create_user_event_with_disavowal(:sign_in_after_2fa) + + if IdentityConfig.store.feature_new_device_alert_aggregation_enabled && + user_session[:new_device] != false + if current_user.sign_in_new_device_at.blank? + current_user.update(sign_in_new_device_at: disavowal_event.created_at) + end + + UserAlerts::AlertUserAboutNewDevice.send_alert( + user: current_user, + disavowal_event:, + disavowal_token:, + ) + end + + reset_second_factor_attempts_count + end + private def authenticate_user @@ -163,13 +183,6 @@ def handle_valid_verification_for_confirmation_context(auth_method:) reset_second_factor_attempts_count end - def handle_valid_verification_for_authentication_context(auth_method:) - mark_user_session_authenticated(auth_method:, authentication_type: :valid_2fa) - create_user_event(:sign_in_after_2fa) - - reset_second_factor_attempts_count - end - def reset_second_factor_attempts_count UpdateUser.new(user: current_user, attributes: { second_factor_attempts_count: 0 }).call end diff --git a/app/controllers/users/piv_cac_login_controller.rb b/app/controllers/users/piv_cac_login_controller.rb index 13fb27a9ad0..a4db6b9f510 100644 --- a/app/controllers/users/piv_cac_login_controller.rb +++ b/app/controllers/users/piv_cac_login_controller.rb @@ -74,6 +74,7 @@ def process_valid_submission presented: true, ) + user_session[:new_device] = current_user.new_device?(cookie_uuid: cookies[:device]) handle_valid_verification_for_authentication_context( auth_method: TwoFactorAuthenticatable::AuthMethod::PIV_CAC, ) diff --git a/app/models/event.rb b/app/models/event.rb index 596bf9d74ca..db39c7a6657 100644 --- a/app/models/event.rb +++ b/app/models/event.rb @@ -28,6 +28,7 @@ class Event < ApplicationRecord phone_added: 21, password_invalidated: 22, sign_in_unsuccessful_2fa: 23, + sign_in_notification_timeframe_expired: 24, } validates :event_type, presence: true diff --git a/app/services/create_new_device_alert.rb b/app/services/create_new_device_alert.rb index e038aec5f04..f10df5aa20f 100644 --- a/app/services/create_new_device_alert.rb +++ b/app/services/create_new_device_alert.rb @@ -9,7 +9,7 @@ def perform(now) sql_query_for_users_with_new_device, tvalue: now - IdentityConfig.store.new_device_alert_delay_in_minutes.minutes, ).each do |user| - emails_sent += 1 if clear_new_device_and_send_email(user) + emails_sent += 1 if expire_sign_in_notification_timeframe_and_send_alert(user) end emails_sent @@ -24,9 +24,10 @@ def sql_query_for_users_with_new_device SQL end - def clear_new_device_and_send_email(user) - UserAlerts::AlertUserAboutNewDevice.send_alert(user) + def expire_sign_in_notification_timeframe_and_send_alert(user) + disavowal_event, disavowal_token = UserEventCreator.new(current_user: user). + create_out_of_band_user_event_with_disavowal(:sign_in_notification_timeframe_expired) - true + UserAlerts::AlertUserAboutNewDevice.send_alert(user:, disavowal_event:, disavowal_token:) end end diff --git a/app/services/user_alerts/alert_user_about_new_device.rb b/app/services/user_alerts/alert_user_about_new_device.rb index f1ae603625e..e4a8bfb4483 100644 --- a/app/services/user_alerts/alert_user_about_new_device.rb +++ b/app/services/user_alerts/alert_user_about_new_device.rb @@ -2,17 +2,17 @@ module UserAlerts class AlertUserAboutNewDevice - def self.call(user, device, disavowal_token) + def self.call(event:, device:, disavowal_token:) if IdentityConfig.store.feature_new_device_alert_aggregation_enabled - user.sign_in_new_device_at ||= Time.zone.now - user.save + event.user.sign_in_new_device_at ||= event.created_at + event.user.save else device_decorator = DeviceDecorator.new(device) login_location = device_decorator.last_sign_in_location_and_ip device_name = device_decorator.nice_name - user.confirmed_email_addresses.each do |email_address| - UserMailer.with(user: user, email_address: email_address).new_device_sign_in( + event.user.confirmed_email_addresses.each do |email_address| + UserMailer.with(user: event.user, email_address: email_address).new_device_sign_in( date: device.last_used_at.in_time_zone('Eastern Time (US & Canada)'). strftime('%B %-d, %Y %H:%M Eastern Time'), location: login_location, @@ -23,17 +23,31 @@ def self.call(user, device, disavowal_token) end end - def self.send_alert(user) - user.update(sign_in_new_device_at: nil) - # Stub out for possible email in follow-up work - # disavowal_token = SecureRandom.urlsafe_base64(32) + def self.send_alert(user:, disavowal_event:, disavowal_token:) + return false unless user.sign_in_new_device_at + + events = user.events.where( + created_at: user.sign_in_new_device_at.., + event_type: [ + 'sign_in_before_2fa', + 'sign_in_unsuccessful_2fa', + 'sign_in_after_2fa', + ], + ).order(:created_at).includes(:device) + + user.confirmed_email_addresses.each do |email_address| + mailer = UserMailer.with(user:, email_address:) + mail = case disavowal_event.event_type + when 'sign_in_notification_timeframe_expired' + mailer.new_device_sign_in_before_2fa(events:, disavowal_token:) + when 'sign_in_after_2fa' + mailer.new_device_sign_in_after_2fa(events:, disavowal_token:) + end + mail.deliver_now_or_later + end - # user.confirmed_email_addresses.each do |email_address| - # UserMailer.with(user: user, email_address: email_address).new_device_sign_in( - # events: events, - # disavowal_token: disavowal_token, - # ).deliver_now_or_later - # end + user.update(sign_in_new_device_at: nil) + true end end end diff --git a/app/services/user_event_creator.rb b/app/services/user_event_creator.rb index 180ef6d0a73..0db96a2d2b3 100644 --- a/app/services/user_event_creator.rb +++ b/app/services/user_event_creator.rb @@ -81,16 +81,10 @@ 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 - ) + 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, - ) + send_new_device_notification(event:, device:, disavowal_token:) [event, disavowal_token] else Device.transaction do @@ -123,8 +117,8 @@ def assign_device_cookie(device_cookie) cookies.permanent[:device] = device_cookie unless device_cookie == cookies[:device] end - def send_new_device_notification(user:, device:, disavowal_token:) - UserAlerts::AlertUserAboutNewDevice.call(user, device, disavowal_token) + def send_new_device_notification(event:, device:, disavowal_token:) + UserAlerts::AlertUserAboutNewDevice.call(event:, device:, disavowal_token:) end # @return [Array(Event, String)] an (event, disavowal_token) tuple diff --git a/config/locales/event_types/en.yml b/config/locales/event_types/en.yml index cfa6adea444..a208c846517 100644 --- a/config/locales/event_types/en.yml +++ b/config/locales/event_types/en.yml @@ -24,6 +24,7 @@ en: piv_cac_enabled: PIV/CAC card associated sign_in_after_2fa: Signed in with second factor sign_in_before_2fa: Signed in with password + sign_in_notification_timeframe_expired: Expired notification timeframe for sign-in from new device sign_in_unsuccessful_2fa: Failed to authenticate webauthn_key_added: Hardware security key added webauthn_key_removed: Hardware security key removed diff --git a/config/locales/event_types/es.yml b/config/locales/event_types/es.yml index 727f2581fd8..4740687c944 100644 --- a/config/locales/event_types/es.yml +++ b/config/locales/event_types/es.yml @@ -24,6 +24,8 @@ es: piv_cac_enabled: Tarjeta PIV/CAC asociada sign_in_after_2fa: Inicia sesión con segundo factor sign_in_before_2fa: Inicia sesión con contraseña + sign_in_notification_timeframe_expired: Plazo de notificación expirado para el + inicio de sesión desde un nuevo dispositivo sign_in_unsuccessful_2fa: Error al autenticar webauthn_key_added: Clave de seguridad de hardware añadido webauthn_key_removed: Clave de seguridad de hardware eliminada diff --git a/config/locales/event_types/fr.yml b/config/locales/event_types/fr.yml index 568c692b19b..69a697c171f 100644 --- a/config/locales/event_types/fr.yml +++ b/config/locales/event_types/fr.yml @@ -24,6 +24,8 @@ fr: piv_cac_enabled: Carte PIV/CAC associée sign_in_after_2fa: Signé avec deuxième facteur sign_in_before_2fa: Connecté avec mot de passe + sign_in_notification_timeframe_expired: Délai de notification pour la connexion + à partir d’un nouveau dispositif expiré sign_in_unsuccessful_2fa: Échec de l’authentification webauthn_key_added: Clé de sécurité ajoutée webauthn_key_removed: Clé de sécurité retirée diff --git a/config/locales/user_mailer/en.yml b/config/locales/user_mailer/en.yml index 0803ee4201d..0c3d1f9f205 100644 --- a/config/locales/user_mailer/en.yml +++ b/config/locales/user_mailer/en.yml @@ -239,7 +239,9 @@ en: one: Your %{app_name} email and password were used to sign in from a new device but failed to authenticate. other: Your %{app_name} email and password were used to sign in from a new - device but failed to authenticate %{count} times + device but failed to authenticate %{count} times. + zero: Your %{app_name} email and password were used to sign in from a new device + but failed to authenticate. info_p2: If you recognize this activity, you don’t need to do anything. info_p3_html: Two-factor authentication protects your account from unauthorized access. If this wasn’t you, %{reset_password_link_html} immediately. diff --git a/config/locales/user_mailer/es.yml b/config/locales/user_mailer/es.yml index bed4f08e957..12540d650c8 100644 --- a/config/locales/user_mailer/es.yml +++ b/config/locales/user_mailer/es.yml @@ -254,7 +254,10 @@ es: error. other: Su correo electrónico y su contraseña de %{app_name} se usaron para ingresar desde un nuevo dispositivo, pero error al autenticar - %{count} veces + %{count} veces. + zero: Su correo electrónico y su contraseña de %{app_name} se usaron para + ingresar desde un nuevo dispositivo, pero la autenticación dio + error. info_p2: Si reconoce esta actividad, no tiene que hacer nada. info_p3_html: La autenticación de dos factores protege su cuenta de accesos no autorizados. Si no fue usted, %{reset_password_link_html} diff --git a/config/locales/user_mailer/fr.yml b/config/locales/user_mailer/fr.yml index 6eefb89ad04..b1e075c8cea 100644 --- a/config/locales/user_mailer/fr.yml +++ b/config/locales/user_mailer/fr.yml @@ -261,6 +261,9 @@ fr: other: Votre adresse électronique et votre mot de passe %{app_name} ont été utilisés pour vous connecter à partir d’un nouvel appareil, mais l’authentification a échoué %{count} reprises. + zero: Votre adresse électronique et votre mot de passe %{app_name} ont été + utilisés pour vous connecter à partir d’un nouvel appareil, mais + l’authentification a échoué. info_p2: Si vous reconnaissez cette activité, vous n’avez rien à faire. info_p3_html: L’authentification à deux facteurs protège votre compte contre tout accès non autorisé. Si ce n’est pas vous, diff --git a/spec/controllers/concerns/two_factor_authenticatable_methods_spec.rb b/spec/controllers/concerns/two_factor_authenticatable_methods_spec.rb new file mode 100644 index 00000000000..82f6d0b25f6 --- /dev/null +++ b/spec/controllers/concerns/two_factor_authenticatable_methods_spec.rb @@ -0,0 +1,147 @@ +require 'rails_helper' + +RSpec.describe TwoFactorAuthenticatableMethods, type: :controller do + controller ApplicationController do + include TwoFactorAuthenticatableMethods + end + + describe '#handle_valid_verification_for_authentication_context' do + let(:user) { create(:user) } + let(:auth_method) { TwoFactorAuthenticatable::AuthMethod::REMEMBER_DEVICE } + + subject(:result) do + controller.handle_valid_verification_for_authentication_context(auth_method:) + end + + before do + stub_sign_in_before_2fa(user) + end + + it 'tracks authentication event' do + stub_analytics + + result + + expect(@analytics).to have_logged_event( + 'User marked authenticated', + authentication_type: :valid_2fa, + ) + end + + it 'authenticates user session auth methods' do + expect(controller.auth_methods_session).to receive(:authenticate!).with(auth_method) + + result + end + + it 'creates a new user event with disavowal' do + expect { result }.to change { user.reload.events.count }.from(0).to(1) + expect(user.events.last.event_type).to eq('sign_in_after_2fa') + expect(user.events.last.disavowal_token_fingerprint).to be_present + end + + context 'when authenticating without new device sign in' do + let(:user) { create(:user) } + + context 'when alert aggregation feature is disabled' do + before do + allow(IdentityConfig.store).to receive(:feature_new_device_alert_aggregation_enabled). + and_return(false) + end + + it 'does not send an alert' do + expect(UserAlerts::AlertUserAboutNewDevice).to_not receive(:send_alert) + + result + end + end + + context 'when alert aggregation feature is enabled' do + before do + allow(IdentityConfig.store).to receive(:feature_new_device_alert_aggregation_enabled). + and_return(true) + end + + context 'with an existing device' do + before do + controller.user_session[:new_device] = false + end + + it 'does not send an alert' do + expect(UserAlerts::AlertUserAboutNewDevice).to_not receive(:send_alert) + + result + end + end + + context 'with a new device' do + before do + controller.user_session[:new_device] = true + end + + it 'sends the new device alert using 2fa event date' do + expect(UserAlerts::AlertUserAboutNewDevice).to receive(:send_alert) do |**args| + expect(user.reload.sign_in_new_device_at.change(usec: 0)).to eq( + args[:disavowal_event].created_at.change(usec: 0), + ) + expect(args[:user]).to eq(user) + expect(args[:disavowal_event]).to be_kind_of(Event) + expect(args[:disavowal_token]).to be_kind_of(String) + end + + result + end + end + end + end + + context 'when authenticating with new device sign in' do + let(:user) { create(:user, sign_in_new_device_at: Time.zone.now) } + + context 'when alert aggregation feature is disabled' do + before do + allow(IdentityConfig.store).to receive(:feature_new_device_alert_aggregation_enabled). + and_return(false) + end + + it 'does not send an alert' do + expect(UserAlerts::AlertUserAboutNewDevice).to_not receive(:send_alert) + + result + end + end + + context 'when alert aggregation feature is enabled' do + before do + allow(IdentityConfig.store).to receive(:feature_new_device_alert_aggregation_enabled). + and_return(true) + end + + context 'with an existing device' do + before do + controller.user_session[:new_device] = false + end + + it 'does not send an alert' do + expect(UserAlerts::AlertUserAboutNewDevice).to_not receive(:send_alert) + + result + end + end + + context 'with a new device' do + before do + controller.user_session[:new_device] = true + end + + it 'sends the new device alert' do + expect(UserAlerts::AlertUserAboutNewDevice).to receive(:send_alert). + with(user:, disavowal_event: kind_of(Event), disavowal_token: kind_of(String)) + + result + end + end + end + end + end +end diff --git a/spec/controllers/two_factor_authentication/backup_code_verification_controller_spec.rb b/spec/controllers/two_factor_authentication/backup_code_verification_controller_spec.rb index 5d353308aff..8c2cb22cff7 100644 --- a/spec/controllers/two_factor_authentication/backup_code_verification_controller_spec.rb +++ b/spec/controllers/two_factor_authentication/backup_code_verification_controller_spec.rb @@ -41,6 +41,10 @@ expect(@irs_attempts_api_tracker).to receive(:track_event). with(:mfa_login_backup_code, success: true) + expect(controller).to receive(:handle_valid_verification_for_authentication_context). + with(auth_method: TwoFactorAuthenticatable::AuthMethod::BACKUP_CODE). + and_call_original + post :create, params: payload expect(subject.user_session[:auth_events]).to eq( 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 56c9d199f4e..72d13471eac 100644 --- a/spec/controllers/two_factor_authentication/otp_verification_controller_spec.rb +++ b/spec/controllers/two_factor_authentication/otp_verification_controller_spec.rb @@ -307,6 +307,10 @@ expect(@irs_attempts_api_tracker).to receive(:mfa_login_phone_otp_submitted). with(reauthentication: false, success: true) + expect(controller).to receive(:handle_valid_verification_for_authentication_context). + with(auth_method: TwoFactorAuthenticatable::AuthMethod::SMS). + and_call_original + freeze_time do post :create, params: { code: subject.current_user.reload.direct_otp, 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 a10da783e3f..0a3a6b46ad1 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 @@ -70,6 +70,10 @@ expect(@analytics).to receive(:track_event). with('User marked authenticated', authentication_type: :valid_2fa) + expect(controller).to receive(:handle_valid_verification_for_authentication_context). + with(auth_method: TwoFactorAuthenticatable::AuthMethod::PERSONAL_KEY). + and_call_original + freeze_time do post :create, params: payload 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 9612e4f5a08..3d516fa69d1 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 @@ -134,6 +134,10 @@ expect(@analytics).to receive(:track_event). with('User marked authenticated', authentication_type: :valid_2fa) + expect(controller).to receive(:handle_valid_verification_for_authentication_context). + with(auth_method: TwoFactorAuthenticatable::AuthMethod::PIV_CAC). + and_call_original + get :show, params: { token: 'good-token' } 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 ea71d27e16e..9775ae9ca05 100644 --- a/spec/controllers/two_factor_authentication/totp_verification_controller_spec.rb +++ b/spec/controllers/two_factor_authentication/totp_verification_controller_spec.rb @@ -62,6 +62,9 @@ with('User marked authenticated', authentication_type: :valid_2fa) expect(@irs_attempts_api_tracker).to receive(:track_event). with(:mfa_login_totp, success: true) + expect(controller).to receive(:handle_valid_verification_for_authentication_context). + with(auth_method: TwoFactorAuthenticatable::AuthMethod::TOTP). + and_call_original post :create, params: { code: generate_totp_code(@secret) } end 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 4e0940a508d..8fc1ce922e5 100644 --- a/spec/controllers/two_factor_authentication/webauthn_verification_controller_spec.rb +++ b/spec/controllers/two_factor_authentication/webauthn_verification_controller_spec.rb @@ -158,6 +158,9 @@ :mfa_login_webauthn_roaming, success: true, ) + expect(controller).to receive(:handle_valid_verification_for_authentication_context). + with(auth_method: TwoFactorAuthenticatable::AuthMethod::WEBAUTHN). + and_call_original freeze_time do patch :confirm, params: params diff --git a/spec/controllers/users/piv_cac_login_controller_spec.rb b/spec/controllers/users/piv_cac_login_controller_spec.rb index 9ed2668a552..f4d5597f197 100644 --- a/spec/controllers/users/piv_cac_login_controller_spec.rb +++ b/spec/controllers/users/piv_cac_login_controller_spec.rb @@ -123,6 +123,7 @@ end it 'sets the session correctly' do + expect(controller.user_session[:new_device]).to eq(true) expect(controller.user_session[TwoFactorAuthenticatable::NEED_AUTHENTICATION]). to eq false @@ -152,6 +153,16 @@ expect(controller.user_session[:decrypted_x509]).to eq session_info.to_json end + context 'from existing device' do + before do + allow(user).to receive(:new_device?).and_return(false) + end + + it 'sets the session correctly' do + expect(controller.user_session[:new_device]).to eq(true) + end + end + context 'when the user has not accepted the most recent terms of use' do let(:user) do build(:user, accepted_terms_at: IdentityConfig.store.rules_of_use_updated_at - 1.year) diff --git a/spec/features/event_disavowal_spec.rb b/spec/features/event_disavowal_spec.rb index 982ac843f36..fc045be82c5 100644 --- a/spec/features/event_disavowal_spec.rb +++ b/spec/features/event_disavowal_spec.rb @@ -3,6 +3,68 @@ RSpec.feature 'disavowing an action', allowed_extra_analytics: [:*] do let(:user) { create(:user, :fully_registered, :with_personal_key) } + context 'with aggregated sign-in notifications enabled' do + before do + allow(IdentityConfig.store).to receive(:feature_new_device_alert_aggregation_enabled). + and_return(true) + user.devices << create(:device) + end + + scenario 'disavowing a sign-in after 2fa' do + sign_in_live_with_2fa(user) + Capybara.reset_session! + + disavow_last_action_and_reset_password + end + + scenario 'disavowing a sign-in before 2fa' do + travel_to (IdentityConfig.store.new_device_alert_delay_in_minutes + 1).minutes.ago do + sign_in_user(user) + end + + Capybara.reset_session! + CreateNewDeviceAlert.new.perform(Time.zone.now) + + disavow_last_action_and_reset_password + end + + scenario 'disavowing a sign-in after 2fa after new device timeframe expired' do + travel_to (IdentityConfig.store.new_device_alert_delay_in_minutes + 1).minutes.ago do + sign_in_user(user) + end + + CreateNewDeviceAlert.new.perform(Time.zone.now) + + expect_delivered_email_count(1) + expect_delivered_email( + to: [user.email_addresses.first.email], + subject: t('user_mailer.new_device_sign_in_before_2fa.subject', app_name: APP_NAME), + ) + + fill_in_code_with_last_phone_otp + click_submit_default + + expect_delivered_email_count(2) + expect_delivered_email( + to: [user.email_addresses.first.email], + subject: t('user_mailer.new_device_sign_in_after_2fa.subject', app_name: APP_NAME), + ) + + disavow_last_action_and_reset_password + end + + context 'user with piv or cac' do + let(:user) { create(:user, :fully_registered, :with_piv_or_cac) } + + scenario 'disavowing a sign-in after 2fa using piv or cac' do + signin_with_piv(user) + Capybara.reset_session! + + disavow_last_action_and_reset_password + end + end + end + scenario 'disavowing a password reset' do perform_disavowable_password_reset disavow_last_action_and_reset_password @@ -190,7 +252,11 @@ def disavow_last_action_and_reset_password signin(user.email, 'NewVal!dPassw0rd') # We should be on the MFA screen because we logged in with the new password - expect(page).to have_content(t('two_factor_authentication.header_text')) - expect(page.current_path).to eq(login_two_factor_path(otp_delivery_preference: :sms)) + if user.piv_cac_configurations.any? + expect(page).to have_current_path(login_two_factor_piv_cac_path) + else + expect(page).to have_content(t('two_factor_authentication.header_text')) + expect(page.current_path).to eq(login_two_factor_path(otp_delivery_preference: :sms)) + end end end diff --git a/spec/features/new_device_tracking_spec.rb b/spec/features/new_device_tracking_spec.rb index 909f01e074f..7f300f26f1f 100644 --- a/spec/features/new_device_tracking_spec.rb +++ b/spec/features/new_device_tracking_spec.rb @@ -12,8 +12,9 @@ ).and_return(false) create(:device, user: user) 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 @@ -28,6 +29,20 @@ 'From 127.0.0.1 (IP address potentially located in United States)'], ) end + + context 'from existing device' do + before do + Capybara.current_session.driver.browser.current_session.cookie_jar[:device] = + user.devices.first.cookie_uuid + end + + it 'does not send a user notification on sign in' do + sign_in_live_with_2fa(user) + + expect(user.reload.devices.length).to eq 1 + expect_delivered_email_count(0) + end + end end context 'user has existing devices and aggregated new device alerts is enabled' do @@ -37,10 +52,30 @@ ).and_return(true) create(:device, user: user) 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 + expect_delivered_email_count(1) + expect_delivered_email( + to: [user.email_addresses.first.email], + subject: t('user_mailer.new_device_sign_in_after_2fa.subject', app_name: APP_NAME), + ) + end + + context 'from existing device' do + before do + Capybara.current_session.driver.browser.current_session.cookie_jar[:device] = + user.devices.first.cookie_uuid + end + + it 'does not send a user notification on sign in' do + sign_in_live_with_2fa(user) + + expect(user.reload.devices.length).to eq 1 + expect_delivered_email_count(0) + end end end diff --git a/spec/features/two_factor_authentication/sign_in_via_personal_key_spec.rb b/spec/features/two_factor_authentication/sign_in_via_personal_key_spec.rb index 9622afd5071..a1a4d50657c 100644 --- a/spec/features/two_factor_authentication/sign_in_via_personal_key_spec.rb +++ b/spec/features/two_factor_authentication/sign_in_via_personal_key_spec.rb @@ -20,11 +20,15 @@ expect(last_message.body).to eq t('telephony.personal_key_sign_in_notice', app_name: APP_NAME) expect(last_message.to).to eq user.phone_configurations.take.phone - expect_delivered_email_count(1) + expect_delivered_email_count(2) expect_delivered_email( to: [user.email_addresses.first.email], subject: t('user_mailer.personal_key_sign_in.subject'), ) + expect_delivered_email( + to: [user.email_addresses.first.email], + subject: t('user_mailer.new_device_sign_in_after_2fa.subject', app_name: APP_NAME), + ) end context 'user enters incorrect personal key' do diff --git a/spec/services/user_alerts/alert_user_about_new_device_spec.rb b/spec/services/user_alerts/alert_user_about_new_device_spec.rb index 9df3d84b522..f8d72f3587f 100644 --- a/spec/services/user_alerts/alert_user_about_new_device_spec.rb +++ b/spec/services/user_alerts/alert_user_about_new_device_spec.rb @@ -1,35 +1,40 @@ require 'rails_helper' RSpec.describe UserAlerts::AlertUserAboutNewDevice do - describe '#call' do - let(:user) { create(:user, :fully_registered) } - let(:disavowal_token) { 'the_disavowal_token' } - let(:device) { create(:device, user: user) } + let(:user) { create(:user, :fully_registered) } + let(:event) { create(:event, user:) } + let(:disavowal_token) { 'the_disavowal_token' } + let(:device) { create(:device, user: user) } + describe '.call' do context 'aggregated new device alerts enabled' do before do allow(IdentityConfig.store).to receive( :feature_new_device_alert_aggregation_enabled, ).and_return(true) end - it 'sets the user sign_in_new_device_at value to current datetime' do - described_class.call(user, device, disavowal_token) - expect(user.sign_in_new_device_at).not_to be_nil + + it 'sets the user sign_in_new_device_at value to time of the given event' do + described_class.call(event:, device:, disavowal_token:) + + expect(user.sign_in_new_device_at.change(usec: 0)).to be_present. + and eq(event.created_at.change(usec: 0)) end end - context 'aggregated new device alerts disenabled' do + context 'aggregated new device alerts disabled' do before do allow(IdentityConfig.store).to receive( :feature_new_device_alert_aggregation_enabled, ).and_return(false) end + it 'sends an email to all confirmed email addresses' do user.email_addresses.destroy_all confirmed_email_addresses = create_list(:email_address, 2, user: user) create(:email_address, user: user, confirmed_at: nil) - described_class.call(user, device, disavowal_token) + described_class.call(event:, device:, disavowal_token:) expect_delivered_email_count(2) expect_delivered_email( @@ -45,4 +50,137 @@ end end end + + describe '.send_alert' do + let(:sign_in_new_device_at) { 10.minutes.ago } + let(:user) { create(:user, :fully_registered, sign_in_new_device_at:) } + let(:disavowal_event) do + create(:event, user:, event_type: :sign_in_after_2fa, created_at: 5.minutes.ago) + end + + subject(:result) do + UserAlerts::AlertUserAboutNewDevice.send_alert(user:, disavowal_event:, disavowal_token:) + end + + it 'returns true' do + expect(result).to eq(true) + end + + it 'unsets sign_in_new_device_at on the user' do + expect { result }.to change { user.reload.sign_in_new_device_at&.change(usec: 0) }. + from(sign_in_new_device_at.change(usec: 0)). + to(nil) + end + + context 'with sign in notification expired disavowal event' do + let(:disavowal_event) do + create( + :event, + user:, + event_type: :sign_in_notification_timeframe_expired, + created_at: 5.minutes.ago, + ) + end + + it 'sends mailer for authentication events within the time window' do + # 1. Exclude events outside the timeframe, e.g. previous sign-in + create(:event, user:, event_type: :sign_in_after_2fa, created_at: 11.minutes.ago) + + # 2. Include authentication events inside the timeframe, inclusive + + # 2.1 Include sign-in before 2FA + sign_in_before_2fa_event = create( + :event, + user:, + event_type: :sign_in_before_2fa, + created_at: sign_in_new_device_at, + ) + + # 2.2 Include sign-in unsuccessful 2FA + sign_in_unsuccessful_2fa_event = create( + :event, + user:, + event_type: :sign_in_unsuccessful_2fa, + created_at: 8.minutes.ago, + ) + + # 3. Exclude sign in notification timeframe expired event + disavowal_event + + delivery = instance_double(ActionMailer::MessageDelivery) + expect(delivery).to receive(:deliver_now_or_later) + mailer = instance_double(UserMailer) + expect(UserMailer).to receive(:with).and_return(mailer) + expect(mailer).to receive(:new_device_sign_in_before_2fa).with( + events: [ + sign_in_before_2fa_event, + sign_in_unsuccessful_2fa_event, + ], + disavowal_token:, + ).and_return(delivery) + + result + end + end + + context 'with sign in after 2fa disavowal event' do + let(:disavowal_event) do + create(:event, user:, event_type: :sign_in_after_2fa, created_at: 5.minutes.ago) + end + + it 'sends mailer for authentication events within the time window' do + # 1. Exclude events outside the timeframe, e.g. previous sign-in + create(:event, user:, event_type: :sign_in_after_2fa, created_at: 11.minutes.ago) + + # 2. Include authentication events inside the timeframe, inclusive + + # 2.1 Include sign-in before 2FA + sign_in_before_2fa_event = create( + :event, + user:, + event_type: :sign_in_before_2fa, + created_at: sign_in_new_device_at, + ) + + # 2.2 Include sign-in unsuccessful 2FA + sign_in_unsuccessful_2fa_event = create( + :event, + user:, + event_type: :sign_in_unsuccessful_2fa, + created_at: 8.minutes.ago, + ) + + # 2.3 Include sign-in after 2FA + sign_in_after_2fa_event = disavowal_event + + # 3. Exclude events not related to authentication, e.g. actions after sign-in + create(:event, user:, event_type: :password_changed, created_at: 4.minutes.ago) + + delivery = instance_double(ActionMailer::MessageDelivery) + expect(delivery).to receive(:deliver_now_or_later) + mailer = instance_double(UserMailer) + expect(UserMailer).to receive(:with).and_return(mailer) + expect(mailer).to receive(:new_device_sign_in_after_2fa).with( + events: [ + sign_in_before_2fa_event, + sign_in_unsuccessful_2fa_event, + sign_in_after_2fa_event, + ], + disavowal_token:, + ).and_return(delivery) + + result + end + end + + context 'without new device timestamp' do + let(:sign_in_new_device_at) { nil } + + it 'returns false and does not send email' do + expect(UserMailer).not_to receive(:with) + + expect(result).to eq(false) + end + end + end end diff --git a/spec/services/user_event_creator_spec.rb b/spec/services/user_event_creator_spec.rb index 91b3d7758c2..7d9b6eb8f02 100644 --- a/spec/services/user_event_creator_spec.rb +++ b/spec/services/user_event_creator_spec.rb @@ -65,8 +65,11 @@ event, _disavowal_token = subject.create_user_event(event_type, user) expect(event).to be_a(Event) - expect(UserAlerts::AlertUserAboutNewDevice).to have_received(:call). - with(user, user.events.first.device, instance_of(String)) + expect(UserAlerts::AlertUserAboutNewDevice).to have_received(:call).with( + event: user.events.first, + device: user.events.first.device, + disavowal_token: instance_of(String), + ) end end @@ -101,8 +104,11 @@ event, _disavowal_token = subject.create_user_event(event_type, user) expect(event).to be_a(Event) - expect(UserAlerts::AlertUserAboutNewDevice).to have_received(:call). - with(user, user.events.first.device, instance_of(String)) + expect(UserAlerts::AlertUserAboutNewDevice).to have_received(:call).with( + event: user.events.first, + device: user.events.first.device, + disavowal_token: instance_of(String), + ) end end end