fix(auth): let the app delete an account that has a second factor - #888
Merged
Conversation
Account deletion and the record wipe both demand a fresh second-factor proof once a factor is enrolled, and that proof lived on the session row. A native client has no session row, so an account with TOTP or a security key could not delete itself from the app on any request it could build: requireFreshMfaIfEnrolled fell through to requireFreshMfa, which resolves via getSession and answers 401 to a perfectly valid token. App Store guideline 5.1.1(v) asks for exactly that deletion. Add requireFreshMfaOrElevationIfEnrolled beside the cookie-only sibling rather than softening it. Its cookie arm is the sibling's, unchanged; its Bearer arm takes the single-use elevation the second-factor management routes already accept, with the fresh-factor rule pinned on, so a password-proved elevation cannot erase an account any more than a password login satisfies the gate on the web. No factor rides the request body: one mint surface, one redemption path. Scope stays fail-closed, so a narrow token is refused before the header is read. The elevation is spent immediately before the erasure, after the typed confirmation and the last-admin check, so a rejected request does not burn a proof against a 5-per-15-minute mint ceiling. The Bearer branch of requireMfaManagementAuth moves into a shared helper so both gates read and claim in the same place. The elevation guard grows a section naming the two routes, because the set of surfaces an elevation can unlock only stays small if adding to it is a visible edit.
The published contract still told a client that a Bearer token can never clear the step-up on account deletion or the record wipe, which is what an app generates its client from. Describe the elevation arm on both: which factors qualify, that a password proof does not, that the proof rides the X-Step-Up header, and that it is spent only when the erasure runs. The 403 a narrow-scope token gets was never published either; it is named now, ahead of the second-factor question it precedes. The mint endpoint lists the surfaces an elevation unlocks, so both routes join that list. Also note on the record wipe that API tokens are on its own delete list: a client that wipes over Bearer signs itself out, which is worth knowing before the retry rather than after it.
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
1 participant
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.
Apple requires that an account can be deleted from inside the app (guideline 5.1.1(v)). For an account with a second factor, ours could not be.
Both erasure routes gate on
requireFreshMfaIfEnrolled, which resolves torequireFreshMfa, which is cookie-only by construction. A Bearer token can never satisfy it, so a user with an authenticator app or a security key had no way to delete their account or wipe their data from the phone. Without a second factor it already worked.The fix uses the step-up ceremony this repo already has rather than accepting a code in the request body.
POST /api/auth/step-upmints a single-use, token-bound elevation valid for five minutes against a freshly proved factor, and a structural test freezes "one mint surface, one redemption path". The two erasure routes join the set that accepts an elevation, through a newrequireFreshMfaOrElevationIfEnrolledbeside the old gate.requireFreshMfaitself is untouched and stays cookie-only. The native app already speaks this ceremony for managing two-factor, so nothing new has to be learned on that side.The wire contract, for the client: mint at
/api/auth/step-up, then sendX-Step-Upalongside the Bearer token on the delete. A missing, expired, spent, foreign or too-weak elevation all answer 401 with the samemeta.errorCode, deliberately indistinguishable. A narrow-scope token is still refused before any factor check.Recovery codes are not accepted on this path: the mint refuses them by design, because spending a break-glass credential to authorise another one is the wrong trade, and widening it would also open disabling two-factor. Someone who has lost their authenticator erases from the web.
Tests, each written red first: the new gate (13 cases), the structural guard extended to name both routes, and an integration test on real Postgres that enrols a factor, mints an elevation over Bearer and deletes end to end (9 cases). Gate: typecheck, lint, prettier, full unit suite, four integration files, openapi in sync.