fix(auth): emit notifyAttachedServices for passwordless and linked-accounts#20358
fix(auth): emit notifyAttachedServices for passwordless and linked-accounts#20358
Conversation
There was a problem hiding this comment.
Pull request overview
This PR restores downstream SNS notifications (e.g., Basket/Braze) for account creation/login flows that bypass AccountHandler.createAccount, specifically passwordless OTP and Google/Apple linked-account sign-ins, so new signups again produce the expected verified/login/profileDataChange events.
Changes:
- Added
notifyAttachedServicesForAccountSessionhelper to centralize emittingverified,login, andprofileDataChangeSNS events. - Updated passwordless OTP confirm flow to emit attached-services notifications and added assertions in Jest coverage.
- Updated linked-account login/create flow to emit attached-services notifications based on which of the three linked-account paths occurred, with added Jest assertions.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/fxa-auth-server/lib/routes/utils/account.ts | Adds a helper to emit attached-services SNS notifications for create/login/profile-change scenarios. |
| packages/fxa-auth-server/lib/routes/passwordless.ts | Calls the new helper during passwordless OTP confirm to emit downstream events. |
| packages/fxa-auth-server/lib/routes/passwordless.spec.ts | Updates module mocking and adds assertions for SNS event emission in passwordless confirm tests. |
| packages/fxa-auth-server/lib/routes/linked-accounts.ts | Tracks linked-account flow path and calls the new helper to emit SNS notifications. |
| packages/fxa-auth-server/lib/routes/linked-accounts.spec.ts | Adds assertions validating SNS event emission for linked-account create/link/re-login cases. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (isNewAccount) { | ||
| notifications.push( | ||
| log.notifyAttachedServices('verified', request, { | ||
| country, | ||
| countryCode, | ||
| email: account.email, | ||
| locale: account.locale, | ||
| service, | ||
| uid: account.uid, | ||
| userAgent, | ||
| }) | ||
| ); | ||
| } |
There was a problem hiding this comment.
notifyAttachedServicesForAccountSession claims to mirror AccountHandler.createAccount, but it emits the verified notification based solely on isNewAccount. In AccountHandler.createAccount, the verified event is gated by account.emailVerified (so new-but-unverified accounts do not emit verified). Consider adding an explicit emailVerified/shouldEmitVerified option (or passing it via account) and using that to decide whether to emit verified, to keep behavior aligned and prevent future misuse of this helper in unverified-signup flows.
| const isNewAccount = linkedAccountFlow === 'new-account'; | ||
| const deviceCount = isNewAccount | ||
| ? 1 | ||
| : (await this.db.sessions(accountRecord.uid)).length; |
There was a problem hiding this comment.
For existing-account third-party logins, deviceCount is computed via await this.db.sessions(accountRecord.uid) before the new session token is created later in this handler. This will typically undercount by 1 compared to other login flows (e.g. sendSigninNotifications in routes/utils/signin.js, which counts sessions after creating the session). Consider moving the notifyAttachedServicesForAccountSession call to after createSessionToken, or adjusting deviceCount to include the imminent session so downstream device/session tracking isn't skewed.
| : (await this.db.sessions(accountRecord.uid)).length; | |
| : (await this.db.sessions(accountRecord.uid)).length + 1; |
d94ec18 to
9e6e01b
Compare
| // accounts for the session token created below, matching the post- | ||
| // createSessionToken count in signin.js sendSigninNotifications. | ||
| const isNewAccount = linkedAccountFlow === 'new-account'; | ||
| const deviceCount = isNewAccount |
There was a problem hiding this comment.
Would moving the notification after the session token creation avoid the need for this calculation?
|
|
||
| // Should emit SNS verified + login + profileDataChange events so | ||
| // Basket/Braze learn about the new passwordless account. Missing | ||
| // these calls was the root cause of the basket regression. |
There was a problem hiding this comment.
Link ticket number about regression?
| }); | ||
|
|
||
| // Mirror the SNS notifications that AccountHandler.createAccount | ||
| // fires, since passwordless bypasses that path. The +1 on existing |
There was a problem hiding this comment.
Wouldn't the session count be accurate already (+1 unnecessary), since the token was created above?
…count flows Because: - Basket/Braze stopped receiving user records for accounts.firefox.com signups once passwordless OTP was enabled in production - Passwordless and Google/Apple third-party auth both created new FxA accounts via db.createAccount without calling log.notifyAttachedServices, so the SNS topic that Basket subscribes to never saw a verified event This commit: - Adds notifyAttachedServicesForAccountSession helper to routes/utils/account.ts - Calls the helper from passwordless confirmCode and linked-accounts loginOrCreateAccount so downstream subscribers receive verified, login, and profileDataChange events - Places notification after createSessionToken so db.sessions reflects the actual session count without needing a +1 adjustment - Adds CORS credentials support for passwordless endpoints - Adds isResend/isNewAccount tags to passwordless statsd metrics Fixes FXA-13416
9e6e01b to
8571684
Compare
Because
accounts.firefox.comsignups once passwordless OTP was enabled in production for the Settings client/account/passwordless/confirm_code) and Google/Apple third-party auth (/linked_account/login) both created new FxA accounts viadb.createAccountwithout callinglog.notifyAttachedServices, so the SNS topic that Basket subscribes to never saw averifiedevent for these signupsThis pull request
notifyAttachedServicesForAccountSessionhelper toroutes/utils/account.tspasswordless.tsconfirmCodeto call the helper, passingdeviceCount: 1for new signups anddb.sessions(uid).lengthfor existing-account loginslinked-accounts.tsloginOrCreateAccountto track which of its three paths was taken via alinkedAccountFlowflag, then calls the helper with the matchingisNewAccount+profileChangedflags (new-account, new-link-on-existing-account, or existing-linked-account re-login)Issue that this pull request solves
Closes: https://mozilla-hub.atlassian.net/browse/FXA-13416
Checklist
Other information
This will be a bit hard to verify until it gets to stage/production