Skip to content

LG-12294: Send single aggregated email notification for new device sign-in#10370

Merged
aduth merged 31 commits intomainfrom
aduth-lg-12294-aggregated-sign-in-email
Apr 12, 2024
Merged

LG-12294: Send single aggregated email notification for new device sign-in#10370
aduth merged 31 commits intomainfrom
aduth-lg-12294-aggregated-sign-in-email

Conversation

@aduth
Copy link
Contributor

@aduth aduth commented Apr 5, 2024

🎫 Ticket

LG-12294

🛠 Summary of changes

Implements the dispatching of a new aggregated email notification for sign-in, which happens 5 minutes after successful email and password submission, or immediately upon successful MFA, whichever occurs first.

This builds upon ("wires up") previous work in #10314 (email template) and #10317 (worker for delayed notification).

📜 Testing Plan

Verify that you are sent an email immediately upon full authentication:

  1. In a private browsing session ("Incognito"), visit http://localhost:3000
  2. Sign in with email and password
  3. Complete MFA step
  4. Observe you see an email notification immediately with listed events
    • Note that it's likely to show "unknown location" since events save '::1' as the IP address in local development

Verify that you are sent an email after delay upon partial authentication:

  1. Optional: Change configured delay and/or worker frequency to avoid having to wait a full 5 minutes:
    • Set new_device_alert_delay_in_minutes: 1 in config/application.yml (changes notification to send after 1 minute instead of after 5 minutes)
    • Set cron_5m in job_configurations.rb to '0/1 * * * *' (changes jobs to run every 1 minute instead of every 5 minutes)
  2. In a private browsing session ("Incognito"), visit http://localhost:3000
  3. Sign in with email and password
  4. Wait until the next worker run after the delay specified in configuration
  5. Observe you see an email notification with listed events
    • Note that it's likely to show "unknown location" since events save '::1' as the IP address in local development

👀 Screenshots

Partial Authentication Full Authentication
image image

@aduth aduth marked this pull request as ready for review April 9, 2024 21:04
@aduth aduth requested a review from a team April 9, 2024 21:04
aduth added 22 commits April 10, 2024 09:59
…gn-in

changelog: Upcoming Features, Sign In, Send single aggregated email notification for new device sign-in
Avoid race conditions between Time.zone.now occurring after the creation of the event and therefore later querying would not include the event
Avoid unoptimized query error, since email template events table groups by device location
Causes lots of unrelated spec failures
@aduth aduth force-pushed the aduth-lg-12294-aggregated-sign-in-email branch from 54b62f4 to b9ff0a8 Compare April 10, 2024 15:17
create_out_of_band_user_event_with_disavowal(:sign_in_notification_timeframe_expired)

true
UserAlerts::AlertUserAboutNewDevice.send_alert(user:, disavowal_event:, disavowal_token:)
Copy link
Contributor

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 👍

Comment on lines +7 to +8
event.user.sign_in_new_device_at ||= event.created_at
event.user.save
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 sign_in_new_device_at assigned.

Copy link
Contributor

@kevinsmaster5 kevinsmaster5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@aduth
Copy link
Contributor Author

aduth commented Apr 11, 2024

I did a final round of testing here before merge, and pushed a minor content change and additional test case. Specifically, I tested an edge case where someone submits their username and password, is delayed in finishing the MFA step long enough to trigger the email notification, but eventually finishes MFA.

This works as expected insofar as it sends two emails, one for the initial delay, and another for the final authentication. This benefits from the same alternative codepath discussed in #10370 (comment) .

0aa2d49 adds feature test coverage for this scenario.

6259d16 improves content to avoid a weird "failed to authenticate 0 times" if the user never MFAs. I also noticed a period was missing, fixed in 27e8f72.

Before After
Screenshot 2024-04-11 at 9 49 42 AM Screenshot 2024-04-11 at 9 49 49 AM

@aduth aduth merged commit f354510 into main Apr 12, 2024
@aduth aduth deleted the aduth-lg-12294-aggregated-sign-in-email branch April 12, 2024 12:37
@mitchellhenke mitchellhenke mentioned this pull request Apr 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants