Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Adds authentication-method metadata to account.login security events and records previously missing passwordless login events.
Changes:
- Adds password, OTP, third-party, and passkey method tags.
- Records login events for OTP and third-party authentication.
- Expands unit coverage across affected flows.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
lib/account-events.ts |
Defines login method metadata. |
lib/routes/utils/security-event.ts |
Passes method metadata to event storage. |
lib/routes/utils/security-event.spec.ts |
Tests method forwarding and omission. |
lib/routes/utils/signin.js |
Tags password authentication. |
lib/routes/utils/signin.spec.ts |
Verifies password tagging. |
lib/routes/passwordless.ts |
Records OTP login events. |
lib/routes/passwordless.spec.ts |
Tests OTP login events. |
lib/routes/passkeys.ts |
Tags passkey authentication. |
lib/routes/passkeys.spec.ts |
Verifies passkey tagging. |
lib/routes/linked-accounts.ts |
Records third-party login events. |
lib/routes/linked-accounts.spec.ts |
Tests third-party login events. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+349
to
+354
| await recordSecurityEvent('account.login', { | ||
| db: this.db, | ||
| request, | ||
| account: { uid: account.uid }, | ||
| method: 'passwordless.otp', | ||
| }); |
Comment on lines
+634
to
+639
| await recordSecurityEvent('account.login', { | ||
| db: this.db, | ||
| request, | ||
| account: { uid: accountRecord.uid }, | ||
| method: 'passwordless.thirdParty', | ||
| }); |
| location: opts?.request.app.geo.location, | ||
| ...(clientId && { client_id: clientId }), | ||
| ...(service && { service }), | ||
| ...(opts?.method && { method: opts.method }), |
## Because - `account.login` is a catch-all. A password sign-in and a passkey sign-in write the same row, so security history cannot tell them apart. This slowed the recent attack investigation. - Passwordless OTP writes only `account.passwordless_login_otp_verified`, and third party auth writes no security event at all. Those two sign-ins are invisible in security history today. ## This pull request - Adds `method` to `SecurityEventAdditionalInfo` in `account-events.ts`, as the union `password | passwordless.otp | passwordless.thirdParty | passkey`. - Types the options of the shared `recordSecurityEvent` helper in `routes/utils/security-event.ts`, so a misspelled `method` fails the build instead of reaching the database. - Sets the method on the password flow (`routes/utils/signin.js`, which builds its own `additionalInfo`) and on the passkey flow (`routes/passkeys.ts`). - Writes an `account.login` event for passwordless OTP (`routes/passwordless.ts`) and for third party auth (`routes/linked-accounts.ts`). Neither wrote one before. - Lets a caller pass `tokenId` to the helper, and passes the new session token id from the two flows that create a session while unauthenticated. - Adds a co-located spec per flow, and two for the helper. `additionalInfo` is already a JSON column, so this needs no new event name, no `securityEventNames` row, and no migration. The passwordless OTP `account.login` is in addition to `account.passwordless_login_otp_verified`, not a replacement. Nothing mutates or deletes an existing row. ## Issue that this pull request solves Closes: FXA-14325
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Because
account.loginis a catch-all. A password sign-in and a passkey sign-in write the same row, so security history cannot tell them apart.account.passwordless_login_otp_verified, and third party auth writes no security event at all. Those two sign-ins are invisible in security history today.This pull request
methodtoSecurityEventAdditionalInfoinaccount-events.ts, as the unionpassword | passwordless.otp | passwordless.thirdParty | passkey.methodthrough the sharedrecordSecurityEventhelper inroutes/utils/security-event.ts.routes/utils/signin.js, which builds its ownadditionalInfo) and on the passkey flow (routes/passkeys.ts).account.loginevent for passwordless OTP (routes/passwordless.ts) and for third party auth (routes/linked-accounts.ts). Neither wrote one before.additionalInfois already a JSON column, so this needs no new event name, nosecurityEventNamesrow, and no migration. The passwordless OTPaccount.loginis in addition toaccount.passwordless_login_otp_verified, not a replacement. Nothing mutates or deletes an existing row.Issue that this pull request solves
Closes: FXA-14325
Checklist
Put an
xin the boxes that applyHow to review (Optional)
routes/utils/security-event.tsis the shared helper, used by passkeys and passwordless.routes/utils/signin.jscallsaccountEventsManager.recordSecurityEventdirectly with its ownadditionalInfoliteral, so the helper change does nothing for it.account-events.ts, thenroutes/utils/security-event.ts, then the four call sites.sendSigninNotificationsis shared by/account/loginand/session/reauth. Both are password authentication, sopasswordis correct for both.method. A consumer must read a missingmethodas unknown, never aspassword. There is deliberately no default in the type and none at read time.Screenshots (Optional)
No user interface change.
Other information (Optional)
This is item 2 from the ticket only. Item 1, keeping the unverified to verified transition instead of overwriting the row, needs its own ticket. It is riskier: it touches the logic that skips login confirmation emails.
Out of scope on purpose: the Amplitude
emitMetricsEvent('account.login')calls are a different pipeline and are unchanged. Renderingmethodin the admin panel or in Settings is not part of this change.Verified locally: the
fxa-auth-serverJest unit suite andnx lint fxa-auth-server. Functional tests were not run.