-
Notifications
You must be signed in to change notification settings - Fork 167
LG-12294: Send single aggregated email notification for new device sign-in #10370
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
5ba842a
b64a278
ad643c2
a5074da
f5be57a
233a2ec
940c15e
ef95a14
0cdd5a0
97a688f
cdaf48d
43fa4cf
e82c599
4f1589d
7b565d0
a6912c7
ac33b34
f21288d
4940491
7545984
9c07f9d
9c9c912
a679446
b262f74
faae091
b9ff0a8
27e8f72
6259d16
0aa2d49
c3b5bde
8191b61
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 | ||
|
Comment on lines
+7
to
+8
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is the workflow different here than in
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, the other code path accounts for a user signing in using their PIV ("Sign in with your government employee ID" on the Sign In page). In that flow, they never submit their email and password, so we don't get that |
||
| 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 | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see this method returns true and satisfies the emails_sent increment 👍