You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Because:
- Disabling an account only blocked the email and password login form.
Passkey, third-party and passwordless sign-in, password reset, and
existing sessions and OAuth tokens all kept working.
- The disabled state is the containment action for fraud and abuse work,
so it has to hold on every path.
This commit:
- Adds a shared assertAccountEnabled check to every route that signs in,
resets, or mints a credential for an account.
- Joins accounts.disabledAt into the session-token lookup so every token
strategy rejects a disabled account's sessions.
- Checks the account on every OAuth token grant.
- Makes admin disable revoke sessions, tokens and OAuth grants, and record
the security event server-side.
- Adds errno 240 ACCOUNT_DISABLED with the fxa-settings message.
Closes FXA-14470
Closes FXA-14462
Because:
- The Build job type-checks what the unit suites transpile without
checking, and two of the new call sites did not compile.
This commit:
- Adds disabledAt to the passkey route's account type so the shared
check accepts it.
- Widens the DangerZone test props so disabledAt can hold a timestamp.
Because:
- The remote /token spec builds its own auth-db stub, which lacked the
method the grant handler now calls.
This commit:
- Adds an accountDisabledAt stub resolving to null.
Because:
- Refusing to re-disable an already-disabled account meant a failed
revocation could never be retried without enabling the account first.
This commit:
- Drops the disabledAt IS NULL condition from the disable update.
This matrix covers only part of the new procedure. There are no fixtures or assertions for accountResetTokens, passwordChangeTokens, passwordForgotTokens, or the joined deviceCommands delete, so regressions can leave high-value credentials active while this suite passes. Add populated fixtures for each deletion target and verify target rows are removed without affecting another UID.
The controller tests mock this method, while the existing DatabaseService integration suite does not exercise it, so the new direct OAuth deletes are unverified. Add an integration test that inserts codes and refresh tokens for both the target and another UID, invokes this method, and verifies only the target rows are removed.
Because:
- Flagging first meant a failed revocation left the account disabled
with live credentials, which a later enable would reactivate.
This commit:
- Runs revokeAccountTokens before the disabledAt update so a failure
leaves the account enabled and the action retryable.
Errno 240 is now returned from legacy password login/reset paths, but fxa-content-server/app/scripts/lib/auth-errors.js has no matching entry. That client therefore displays the raw English server message instead of a localized account-disabled error. Add the corresponding content-server error mapping and localization before replacing errno 149 on those paths.
Integration fixture omits four token/device tables
The integration fixture exercises only five of the procedure's eight token/device tables. No row is inserted for accountResetTokens, passwordChangeTokens, passwordForgotTokens, or deviceCommands, so regressions in those new DELETE statements would still leave this suite green. Seed and assert each deleted table, including preservation of another account's rows.
Because:
- A failed security-event write returned an error after the account
state had already changed, and skipped the profile notification.
This commit:
- Records the security event, clears the profile cache and notifies
attached services independently, reporting any failure to Sentry.
This integration test does not exercise all rows the new procedure promises to revoke. It never seeds or asserts accountResetTokens, passwordChangeTokens, passwordForgotTokens, or the joined deviceCommands, so regressions in four deletion clauses would still pass. Seed those tables for the target and another UID, then verify target removal and preservation of unrelated rows.
…e place
Because:
- Checking each sign-in route separately touched nine files and left
future routes to remember the rule themselves.
This commit:
- Refuses to create any session, token or unblock code for a disabled
account inside the db layer, where every sign-in and reset path ends.
- Removes the per-route checks and their route-level tests.
- Keeps the early password-login check and the OAuth grant check.
The integration coverage omits accountResetTokens, passwordChangeTokens, passwordForgotTokens, and deviceCommands, even though the new procedure is responsible for deleting all four. A missing or broken DELETE for any of these security-sensitive credentials would still pass this suite; seed each table and include it in the post-revocation assertions.
I was expecting to see this happen at the authorization strategy layer. The approach you have here seems hard to maintain and touches a lot of files!
Good idea @dschom - sign-in check now moved to the db functions that create sessions and tokens, so every path hits it and the per-route checks are gone.
Note, a disabled user now sees the error after entering their OTP code rather than before. Should be fine for a disabled account.
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
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.
dschom
left a comment
Because
This pull request
lib/db.ts, where every sign-in and reset path ends.sessionWithDevice_20so session-token lookups carrydisabledAt, and rejects those tokens in every session strategy./oauth/tokengrant type.revokeAccountTokens_1; admin disable now revokes sessions, devices, auth tokens, OAuth refresh tokens and codes.account.disableandaccount.enablesecurity events server-side.ACCOUNT_DISABLED, replacing errno 149 on the password login paths.Issue that this pull request solves
Closes: FXA-14470
Closes: FXA-14462
Checklist
Put an
xin the boxes that applyHow to review (Optional)
Start with the token-creating functions in
lib/db.ts, thenmakeCredentialFninlib/server.js, the migration, then the admin-server controller.Manual test on a local stack (
yarn start):account.disablein security events; click Enable.account.enableis recorded.Other information (Optional)
revokeAccountTokens_1.sessionWithDevice_19in a later patch.