LG-9429: Avoid sending new device notification before fully registered

[aduth]

🎫 Ticket

LG-9429

🛠 Summary of changes

Updates device notification logic to avoid sending new device notifications prior to the user being fully registered, so that a user doesn't receive a "New Sign-In" notification if they complete account registration on a second device, which is common if they were to start the process on a computer and complete it on their phone.

DRAFT: Leaving this as draft for now because, while I think "fully registered" aligning to the RegistrationLog record makes a lot of sense, it's unclear if this is reliable enough to exist for users who have existed since prior to the introduction of that table in #3121. Instead, it may make more sense to have it reflect the presence of any MFAs, essentially aliasing fully_registered? to two_factor_enabled? or using the method directly (though I think "fully registered" adds some clarity of intent). Updated as of b7661d0.

📜 Testing Plan

1. Go to http://localhost:3000
2. Click "Create an account"
3. Enter an email
4. Confirm Rules of Use checkbox
5. Click "Submit"
6. Open the email you received
7. In a separate session from what you used for Steps 1-6 (e.g. on your mobile device, or in a different browser, or in an Incognito/private window), open the link to confirm your email address
8. Complete the password selection step
9. Select phone as MFA
10. Complete the phone MFA setup

Before: An email notification is sent about "New sign-in" to your account
After: No email is sent about "New sign-in" to your account when the user is in the middle of account creation

[zachmargolis]

what if we renamed this for clarity/consistency?

Suggested change

	trait :signed_up do
	trait :fully_registered do

[aduth]

what if we renamed this for clarity/consistency?

Yeah, that seems like a good idea, though I suspect it might touch a ton of files. I'd probably create a quick follow-on pull request for that.

[aduth]

Follow-up at #8302

[jc-gsa]

Sending a video of a possible issue over Slack.

[aduth]

Sending a video of a possible issue over Slack.

For posterity, the issue is that the notification is still being sent when using backup codes as the selected MFA method.

I'll take a look at this (and other MFA methods) to see what's going on.

[aduth]

The issue seems to stem from when the create_user_event is called relative to the second-factor being registered. For phones, the event is created before the phone is registered to the user, so the user is not yet considered "fully registered". For backup codes (and potentially others), the event is registered afterward, so the user is considered fully registered and the notification is sent.

I think we'll need to either...

• Change the ordering of how the events are created to be consistently before the second factor is registered to the account
• Or, revert back to the original approach for considering a fully registered user as one who has an associated RegistrationLog record

[aduth]

• Or, revert back to the original approach for considering a fully registered user as one who has an associated RegistrationLog record

Digging into this, @zachmargolis shared that the original concern that the data not exist for accounts created before the table was created is not actually an issue, since the data has been backfilled.

I reverted to this approach in c813c8b, which also addresses the bug where an email would still be sent when selecting certain MFAs.

I manually tested this across all MFA types, but am also open to some suggestions as far as test coverage. I'd like to have some regression coverage for this, though also there's not much precedent for this level of feature testing of all aspects of all features across all potential MFA methods. It would be nice if the behavior contained in confirm_email_in_a_different_browser could trust that common behaviors would remain consistent. It seems like this sort of common event tracking ought to live in a common controller / module that we'd unit test. Or maybe we do just have some shared feature spec examples for new device emails when confirming email in a second browser that's tested for every MFA method 🤷

[zachmargolis]

LGTM