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 23bd3996464..30037eebc39 100644 --- a/app/services/user_alerts/alert_user_about_new_device.rb +++ b/app/services/user_alerts/alert_user_about_new_device.rb @@ -27,7 +27,7 @@ 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.., + created_at: sign_in_events_start_time(user:).., event_type: [ 'sign_in_before_2fa', 'sign_in_unsuccessful_2fa', @@ -49,5 +49,20 @@ def self.send_alert(user:, disavowal_event:, disavowal_token:) user.update(sign_in_new_device_at: nil) true end + + def self.sign_in_events_start_time(user:) + # Avoid scenarios where stale events may be reflected in the time since sign in: + # + # 1. The feature is enabled for a short time in a deployed environment before being disabled + # 2. In local development, the server is not always active and the job may not run until later + # + # Typically, it's guaranteed that even in the worst-case of a sign-in occurring immediately + # after a scheduled job run, it should take no longer than twice the scheduled delay. A small + # buffer is added to account for delays of the job run or within the job itself. + [ + user.sign_in_new_device_at, + (IdentityConfig.store.new_device_alert_delay_in_minutes * 3).minutes.ago, + ].max + end end end 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 f8d72f3587f..321619d2e9a 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 @@ -52,7 +52,7 @@ end describe '.send_alert' do - let(:sign_in_new_device_at) { 10.minutes.ago } + let(:sign_in_new_device_at) { 3.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) @@ -62,6 +62,10 @@ UserAlerts::AlertUserAboutNewDevice.send_alert(user:, disavowal_event:, disavowal_token:) end + before do + allow(IdentityConfig.store).to receive(:new_device_alert_delay_in_minutes).and_return(5) + end + it 'returns true' do expect(result).to eq(true) end @@ -78,17 +82,20 @@ :event, user:, event_type: :sign_in_notification_timeframe_expired, - created_at: 5.minutes.ago, + created_at: Time.zone.now, ) 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) + # 1. Exclude events outside possible window of time of recurring job run + create(:event, user:, event_type: :sign_in_after_2fa, created_at: 16.minutes.ago) - # 2. Include authentication events inside the timeframe, inclusive + # 2. Exclude events outside the timeframe, e.g. previous sign-in + create(:event, user:, event_type: :sign_in_after_2fa, created_at: 4.minutes.ago) - # 2.1 Include sign-in before 2FA + # 3. Include authentication events inside the timeframe, inclusive + + # 3.1 Include sign-in before 2FA sign_in_before_2fa_event = create( :event, user:, @@ -96,15 +103,15 @@ created_at: sign_in_new_device_at, ) - # 2.2 Include sign-in unsuccessful 2FA + # 3.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, + created_at: 2.minutes.ago, ) - # 3. Exclude sign in notification timeframe expired event + # 4. Exclude sign in notification timeframe expired event disavowal_event delivery = instance_double(ActionMailer::MessageDelivery) @@ -125,16 +132,19 @@ 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) + create(:event, user:, event_type: :sign_in_after_2fa, created_at: 1.minute.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) + # 1. Exclude events outside possible window of time of recurring job run + create(:event, user:, event_type: :sign_in_after_2fa, created_at: 16.minutes.ago) + + # 2. Exclude events outside the timeframe, e.g. previous sign-in + create(:event, user:, event_type: :sign_in_after_2fa, created_at: 4.minutes.ago) - # 2. Include authentication events inside the timeframe, inclusive + # 3. Include authentication events inside the timeframe, inclusive - # 2.1 Include sign-in before 2FA + # 3.1 Include sign-in before 2FA sign_in_before_2fa_event = create( :event, user:, @@ -142,19 +152,19 @@ created_at: sign_in_new_device_at, ) - # 2.2 Include sign-in unsuccessful 2FA + # 3.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, + created_at: 2.minutes.ago, ) - # 2.3 Include sign-in after 2FA + # 3.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) + # 4. Exclude events not related to authentication, e.g. actions after sign-in + create(:event, user:, event_type: :password_changed, created_at: 30.seconds.ago) delivery = instance_double(ActionMailer::MessageDelivery) expect(delivery).to receive(:deliver_now_or_later)