From d8ea51ac74738d1cec29c5a258a5ed78043c88a0 Mon Sep 17 00:00:00 2001 From: mforce <> Date: Sat, 1 Aug 2026 18:33:15 -0700 Subject: [PATCH 01/13] spec: design user disable/enable and email change (#356, #357) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two gaps on the Users page: there is no way to take someone's access away, and no way to correct the address they log in with. A departing worker can only have their password reset, which leaves a live account behind; a typo'd email is unfixable without abandoning the account. Records the design for both, and the reasoning behind the choices that are not obvious: - Disable is soft, never a hard delete — users are referenced by audit rows and created-by trails. Erasure stays with #272. - One flag covers offboarding and suspension. Suspension is what sets the enforcement bar: access tokens live ~15 min with no server-side denylist, so a per-request check is the only option under which the flag means what the UI claims. A short-TTL cache would reintroduce the multi-replica problem #338 already tracks. - Self-disable and last-active-Owner are both refused. bootstrap-admin creates exactly one Owner, and recover-admin resets a password without re-enabling anything, so without both guards one click can leave a farm with no UI path back in. recover-admin now clears DisabledAt for the same reason. - Both operations require a step-up grant (#308). A stolen Owner token could otherwise repoint a worker's email and then reset that worker's password — which is deliberately ungated for worker targets — turning a 15-minute window into durable access. - The ConcurrencyStamp retry hazard is the #269 probe shape, not SingleAttemptExecution: neither write is a counter, so a replay is unobservable and the cure is asking the database whether our own write is durable. Role editing (#355) is filed separately — same page and machinery, but out of scope here. --- ...026-08-01-user-disable-and-email-design.md | 269 ++++++++++++++++++ 1 file changed, 269 insertions(+) create mode 100644 docs/superpowers/specs/2026-08-01-user-disable-and-email-design.md diff --git a/docs/superpowers/specs/2026-08-01-user-disable-and-email-design.md b/docs/superpowers/specs/2026-08-01-user-disable-and-email-design.md new file mode 100644 index 000000000..c5b9bfa7c --- /dev/null +++ b/docs/superpowers/specs/2026-08-01-user-disable-and-email-design.md @@ -0,0 +1,269 @@ +# Disable a user, and change a user's email address + +**Date:** 2026-08-01 +**Phase:** 1.1 (epic #14) +**Status:** design, awaiting implementation plan + +## Problem + +The Users page ships create / list / rename / set-password / flock-assign +(`UserEndpoints`). There is no way to take someone's access away, and no way to +correct the address they log in with. + +A worker leaves the farm and the only lever an Owner has is resetting their +password — which leaves a live account behind, reachable by anyone who learns +the new password. A typo'd email at creation time is unfixable: the account must +be abandoned and a second one created, leaving a phantom user attached to +whatever history the first one recorded. + +Both gaps live on the same screen and share the same machinery, so they are +designed together and shipped as two PRs. + +## Scope + +**In:** an Owner can disable and re-enable a user; an Owner can change a user's +email address. + +**Out:** hard delete. Users are referenced by audit rows, created-by trails on +daily entries and sales orders, and refresh-token history — a delete either +cascades and shreds the audit log or nulls out and leaves history recorded by +"(nobody)". Personal-data erasure is tracked separately as **#272** (GDPR + PH +DPA), which is where a real delete belongs, behind a retention policy. + +**Out:** role editing. `CreateUser` fixes a role for the life of the account, so +promoting a worker to Manager today means creating a second account. Same page, +same step-up machinery, genuinely a bigger gap than either feature here — filed +as its own slice rather than folded in. + +## Decisions + +### Disable is one flag serving both offboarding and suspension + +A disabled account covers "this person left" and "this person is suspended +pending an investigation" with a single flag. That second case is what sets the +enforcement bar: a suspension that takes up to 15 minutes to bite is not a +suspension. + +### Enforcement is a per-request check, not a token-expiry wait + +Access tokens live ~15 minutes and there is no server-side denylist +(`IIdentityProvider` documents this on `SetUserPasswordAsync`). So a disable +reaches an already-issued token one of three ways: wait for it to expire, check +the database on every authenticated request, or cache that check. + +We check per request. The traffic is a farm's worth of users, not a public API — +one indexed PK lookup is noise next to what a daily-entry POST already does, and +it is the only option under which the flag means what the UI claims it means. A +short-TTL cache would be per-process in-memory, reintroducing the multi-replica +problem **#338** already tracks for step-up grants, and quietly downgrading +"immediate" to "within 30s, per replica." + +### Two guards, or the farm can lock itself out + +Users administration is `OwnerOnly`, so an Owner can disable another Owner, and +`bootstrap-admin` creates exactly one Owner by default. Disabling yourself, or +disabling the last active Owner, would leave a farm with no way back in through +the UI — and `recover-admin` resets a password without re-enabling anything. + +Both are refused. This is `bootstrap-admin`'s exactly-one-first-run-Owner +invariant approached from the other end. + +Owners remain disable-able in general: "one of our two Owners left" is a real +case, and routing it through a CLI would be over-restriction. + +### `recover-admin` re-enables its target + +`BreakGlassResetAsync` clears `DisabledAt`. Without this the break-glass path +cannot recover from a disable — the exact scenario where it is most needed. + +### Changing an email revokes the target's sessions + +The email is the login identifier. Changing it rotates the security stamp, +revokes every refresh token, and invalidates outstanding step-up grants, the +same posture `SetUserPasswordAsync` already takes. The collateral is one +re-login; the principle is that a changed identifier re-establishes the session. + +### Both operations require a step-up grant (#308) + +Consider a stolen Owner access token with ~15 minutes of validity. Change a +worker's email to an attacker-controlled address, then reset that worker's +password — which is *not* step-up-gated for worker targets +(`ResetWorkersPassword_NeedsNoStepUp_OrdinaryAdministrationStaysUngated`). That +converts a short window into durable access under an account nobody has reason +to audit. Gating the identifier change closes the half of that chain the +attacker cannot otherwise obtain. + +Disable is gated on the same grounds: it is the lever that removes access. + +### No email verification exists, and the UI says so + +There is no SMTP path in this application. The Owner's word is the verification. +The Help page states this so nobody waits for a confirmation mail that will never +arrive. + +## Design + +### 1. Storage + +`ApplicationUser` gains: + +```csharp +public DateTimeOffset? DisabledAt { get; set; } // null = active +public Guid? DisabledBy { get; set; } // the Owner who did it +``` + +A nullable timestamp, not an `IsActive` bool. The domain aggregates (`Product`, +`EggGrade`, `InventoryItem`, `ExpenseCategory`) use a bool because they carry no +audit trail of their own; a user account is a security object where "when, and by +whom" is the first question asked. `DisabledBy` is a plain `Guid?` with no FK — +the referenced Owner may themselves be disabled later, and an FK buys nothing. + +One new migration: two nullable columns, no backfill, no data statements. +Nothing resembling the hand-carried `InitialCreate` content, so a plain +`dotnet ef migrations add` is correct and `MigrationSecurityReviewTests` stays +green (no INSERTs, no credential-shaped rows). + +### 2. Application layer + +Two new features under `Features/Users/`, handler-per-feature: + +**`SetUserActive/`** — `SetUserActiveCommand(Guid UserId, bool Active, string? StepUpToken)`. +One handler backs both directions, mirroring `SetProductActiveHandler`. Guards, +in order: + +| Condition | Result | +|---|---| +| Target is the caller | 422 `Users.CannotDisableSelf` | +| Target is the last active Owner (disable only) | 422 `Users.LastOwner` | +| Target not in this account | 404 | + +On disable: stamp `DisabledAt` / `DisabledBy`, rotate `SecurityStamp`, revoke +every refresh token, invalidate outstanding step-up grants. On enable: clear +`DisabledAt` / `DisabledBy`. Audit `User.Disable` / `User.Enable`. + +**`ChangeUserEmail/`** — `ChangeUserEmailCommand(Guid UserId, string Email, string? StepUpToken)`. +Writes `Email`, `UserName`, and `NormalizedEmail` together — never one alone. +409 on the unique index. Then the same revoke-everything treatment as a disable. +Audit `User.EmailChanged`, recording old → new. + +Both call `IStepUpGrantService.ValidateAsync` first, in the shape +`CreateUserHandler` already uses. + +`IIdentityProvider` gains `SetUserActiveAsync` and `ChangeUserEmailAsync`, both +account-scoped so a foreign id resolves to NotFound rather than a cross-tenant +write. `UserSummary` and `UserProfile` gain `DisabledAt`. + +#### Retry hazard (#269 family) + +Both mutate an Identity user through `UserManager`, which is a `ConcurrencyStamp` +CAS. A replayed `SaveChanges` after an ambiguous commit re-issues the `UPDATE` +under a stale stamp, matches zero rows, and reports failure for work that landed. + +Unlike `AccessFailedAsync`, neither is a counter — a replay writes nothing new +and is harmless in itself. So the cure is the `RefreshAsync` pattern, **not** +`SingleAttemptExecution`: on failure, ask the database whether this attempt's own +write is durable (is `DisabledAt` already our value? is the email already ours?), +and let the original error stand otherwise. Fails closed. Both are also +idempotency-key-wrapped writes, so the client-retry path covers most of it +already. + +### 3. API surface + +``` +POST /api/v1/users/{id}/disable -> 204 (step-up) +POST /api/v1/users/{id}/enable -> 204 (step-up) +PUT /api/v1/users/{id}/email -> 204 (step-up; 409 on conflict) +``` + +Disable/enable are separate verbs rather than a field on `PUT /users/{id}`, +following `POST /products/{id}/deactivate` and `/egg-grades/{id}/deactivate`. + +Email is separate from `UpdateUser` because it carries different auth weight — +`UpdateUser` (display name) is ungated and stays that way. + +All three inherit the group's `OwnerOnly` policy and the `Idempotency-Key` +requirement. `WithMaxRequestBodyBytes(2048)` on the email PUT, matching the +password endpoint's reasoning. + +`ListUsers` and `GetUser` responses grow `disabledAt`. + +### 4. Enforcement + +New `DisabledUserMiddleware`, placed beside `MustChangePasswordMiddleware`: + +- **before `UseAuthorization`** — so it applies uniformly regardless of an + endpoint's `AuthPolicies` tier; +- **before the idempotency middleware** — so a blocked write burns no key. + +One indexed PK lookup per authenticated request. Refuses with 401 and a distinct +`Auth.AccountDisabled` title, so the SPA can show "Your account has been +disabled" rather than a generic session-expired bounce. + +`LoginAsync` refuses a disabled user with the *same* generic +`Identity.InvalidCredentials` as a wrong password, still paying the PBKDF2 cost — +the reasoning the lockout branch already documents, so the reply never reveals +account state. `RefreshAsync` refuses too. + +### 5. SPA + +Users page lists disabled users inline, always visible: a "Disabled" badge and a +de-emphasized row, with the action toggling to "Enable". Hiding people who still +own history invites "where did Maria go?" and a duplicate account. A +`Show disabled` filter is the right answer once a farm has years of turnover — +`ListUsers` returns a flat unpaged array today, so that is a larger change than +this feature warrants. + +Disable goes through a confirm dialog (existing `useConfirm()` hook): names the +user, warns that they are signed out immediately and lose access, Cancel / +Disable. Enable is a plain one-click — it is not destructive. + +Email editing is a form on the user row, surfacing the 409 as a field error. + +### 6. Testing + +**Domain / Application** — guard rules: self-disable, last-active-Owner, both +directions of the toggle, email normalization, email conflict. + +**Integration** + +- A disabled user cannot log in, and the refusal is indistinguishable from a + wrong password. +- A disabled user cannot refresh. +- A **live access token** stops working on the next request. This is the point of + the middleware and the test that fails if it is ever dropped. +- Step-up grants die with the disable. +- An email change signs the target out; the new address logs in; the old does not. +- 409 on an address already taken. +- `recover-admin` re-enables a disabled target. + +**Race** — two Owners disabling each other concurrently must not land at zero +active Owners. This is the `Version`-bump family of bug the repo has shipped +three times, wearing Identity's `ConcurrencyStamp` clothing. It gets a +parallel-race integration test like every other mutation. + +**Retry** — a `RetryBoundaryTests` case per the hazard in §2, using +`TransientCommitFaultInterceptor`. Not `TransientCommandFaultInterceptor`: these +are multi-statement saves, where that interceptor is a fail-*before* and yields a +green test with the defect fully present. + +**SPA (Vitest)** — the confirm dialog, disabled-row rendering, the enable path, +the email-edit form including the 409. + +### 7. Docs + +- `specs/product/GLOSSARY.md` gains **disabled user**. +- Help page gains a Users section: what disabling does, that it is reversible, + that history is retained, and that an email change sends no verification mail. +- Strings ship with es/tl inline per the translate-now policy, added to + `TRANSLATED_NAMESPACES`. + +## Delivery + +Two issues against epic #14, both added to its checklist: + +1. **Disable / enable a user** — storage, guards, middleware, `recover-admin` + re-enable, SPA toggle + confirm. +2. **Change a user's email** — reuses the revoke-sessions machinery from (1), + so it ships second. + +Role editing is filed separately. From c25fcf21abf51a18bd41d60361f246dc6e8008aa Mon Sep 17 00:00:00 2001 From: mforce <> Date: Sat, 1 Aug 2026 18:46:11 -0700 Subject: [PATCH 02/13] spec: rework around a credential epoch after review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An adversarial review of the first draft found the enforcement mechanism does not do what the draft claimed, and the claim was load-bearing for both features. The draft rotated SecurityStamp and revoked refresh tokens, then called the session terminated. It is not: the access token carries sub, email, account_id, role, and must_change_password, and validation checks only signature, issuer, audience, and lifetime. Nothing on the request path reads a stamp. So an email change left the target's live tab working, and a per-request read of DisabledAt meant re-enabling resurrected every unexpired pre-disable token — a suspension lifted after five minutes handed back the session it was meant to end. The user now carries a monotonic CredentialEpoch, stamped into every access token and compared per request. Disable, email change, and every password reset bump it; enable deliberately does not. That asymmetry is the point, and it also closes a race the draft missed: a refresh already in flight can insert a live child token after a bulk revoke commits, and the child is useless if it was minted under a superseded epoch. Also corrected: - The last-Owner guard was called race-safe on ConcurrencyStamp. Two Owners disabling each other touch different rows and share no concurrency token. It needs the account row locked FOR UPDATE, with the count re-read inside the lock — the pattern AccountRepository already has, and a constraint on every future Owner-removing path. - The email write omitted NormalizedUserName, which is the column carrying the unique index; NormalizedEmail's is not unique. Omitting it would reserve the old address forever and bypass conflict detection on the new one. - The step-up justification described a takeover chain that does not need an email change at all — CreateUserHandler gates only Owner creation, so a stolen Owner token already mints a Manager. Gating these operations is still right, for honest reasons; the persistence gap belongs to #308 and is filed separately. - The retry section is deleted. IdempotencyMiddleware already runs the pipeline under SingleAttemptExecution, so the inner Identity save is never independently replayed, and the probe it prescribed keyed on evidence that is not attempt-unique. - A sole Owner may not change their own email; recover-admin gains a lookup that does not require knowing the typo. - Middleware placement is stated as an order, not a vibe, with AccountId in every lookup — ApplicationUser has no tenant query filter. - The SPA must carry the 401's error title through auth teardown, or the promised "your account has been disabled" can never render. Every correction is recorded at the end of the spec so the same reasoning is not re-derived later. --- ...026-08-01-user-disable-and-email-design.md | 466 ++++++++++++------ 1 file changed, 319 insertions(+), 147 deletions(-) diff --git a/docs/superpowers/specs/2026-08-01-user-disable-and-email-design.md b/docs/superpowers/specs/2026-08-01-user-disable-and-email-design.md index c5b9bfa7c..cb4b7dcb0 100644 --- a/docs/superpowers/specs/2026-08-01-user-disable-and-email-design.md +++ b/docs/superpowers/specs/2026-08-01-user-disable-and-email-design.md @@ -3,6 +3,9 @@ **Date:** 2026-08-01 **Phase:** 1.1 (epic #14) **Status:** design, awaiting implementation plan +**Revision:** second draft, after an adversarial review of the first. The +changes are substantive and are recorded in *What the first draft got wrong* at +the end — read that before treating any of this as a small delta. ## Problem @@ -30,10 +33,16 @@ cascades and shreds the audit log or nulls out and leaves history recorded by "(nobody)". Personal-data erasure is tracked separately as **#272** (GDPR + PH DPA), which is where a real delete belongs, behind a retention policy. -**Out:** role editing. `CreateUser` fixes a role for the life of the account, so -promoting a worker to Manager today means creating a second account. Same page, -same step-up machinery, genuinely a bigger gap than either feature here — filed -as its own slice rather than folded in. +**Out:** role editing (**#355**). `CreateUser` fixes a role for the life of the +account, so promoting a worker to Manager today means creating a second account. +Same page, same machinery, genuinely a bigger gap than either feature here — +filed as its own slice. + +**Out, but adjacent and newly filed:** step-up (#308) currently gates only +*Owner* creation, so a stolen Owner access token can already mint a Manager with +an attacker-controlled password and keep access long past the token's own +lifetime. That is a gap in #308's coverage, not something this feature +introduces or can close — see *Threat model* below. ## Decisions @@ -44,62 +53,164 @@ pending an investigation" with a single flag. That second case is what sets the enforcement bar: a suspension that takes up to 15 minutes to bite is not a suspension. -### Enforcement is a per-request check, not a token-expiry wait - -Access tokens live ~15 minutes and there is no server-side denylist -(`IIdentityProvider` documents this on `SetUserPasswordAsync`). So a disable -reaches an already-issued token one of three ways: wait for it to expire, check -the database on every authenticated request, or cache that check. - -We check per request. The traffic is a farm's worth of users, not a public API — -one indexed PK lookup is noise next to what a daily-entry POST already does, and -it is the only option under which the flag means what the UI claims it means. A -short-TTL cache would be per-process in-memory, reintroducing the multi-replica -problem **#338** already tracks for step-up grants, and quietly downgrading -"immediate" to "within 30s, per replica." +### The per-request check is a credential epoch, not the disabled flag -### Two guards, or the farm can lock itself out +This is the central mechanism, and the first draft got it wrong by reaching for +the flag directly. -Users administration is `OwnerOnly`, so an Owner can disable another Owner, and -`bootstrap-admin` creates exactly one Owner by default. Disabling yourself, or -disabling the last active Owner, would leave a farm with no way back in through -the UI — and `recover-admin` resets a password without re-enabling anything. +An access token today carries `sub`, `email`, `account_id`, `role`, and +`must_change_password` — and nothing else (`JwtTokenService`). Validation checks +signature, issuer, audience, and lifetime (`CluckworkIdentityServiceCollectionExtensions`). +There is no `SecurityStamp` in the token and nothing on the request path reads +one. Three consequences follow, and the flag cannot address any of them: -Both are refused. This is `bootstrap-admin`'s exactly-one-first-run-Owner -invariant approached from the other end. +1. Rotating `SecurityStamp` on a change **does not invalidate a live access + token**. It invalidates step-up grants, which bind to the stamp, and nothing + else. +2. If the per-request check reads `DisabledAt`, then **re-enabling resurrects + every still-unexpired pre-disable access token**. A suspension lifted after + five minutes hands back the exact session it was meant to end. +3. An email change would revoke refresh tokens but leave the target's current + tab working — the spec's own promise of "signs the target out" would be false. -Owners remain disable-able in general: "one of our two Owners left" is a real -case, and routing it through a CLI would be over-restriction. +So the user carries a **credential epoch**: a monotonically increasing `int`, +stamped into every access token as a claim, compared against the database on +every authenticated request. A mismatch is a 401. -### `recover-admin` re-enables its target +```csharp +public int CredentialEpoch { get; set; } // starts at 0, only ever increments +``` -`BreakGlassResetAsync` clears `DisabledAt`. Without this the break-glass path -cannot recover from a disable — the exact scenario where it is most needed. +Bumping it is the single act that means "every credential minted before now is +dead." Disable bumps it. Email change bumps it. Password reset bumps it (all +three paths — self-service, an Owner's `SetUserPasswordAsync`, break-glass). +**Enable does not bump it and does not restore the old value** — that asymmetry +is the whole point of a monotonic counter, and it is what stops finding (2) +above. + +This subsumes the disabled check rather than sitting beside it: the middleware +compares one claim to one column, and `DisabledAt` goes back to being what it +should be — the record of a state, not the enforcement mechanism. + +`RefreshAsync` re-checks both the disabled state and the epoch. That closes the +issuance race below. + +### Why not a cache + +A short-TTL cache of the check would be per-process in-memory, reintroducing the +multi-replica problem **#338** already tracks for step-up grants and quietly +downgrading "immediate" to "within 30s, per replica." The traffic here is a +farm's worth of users; one indexed PK lookup per authenticated request is noise +next to what a daily-entry POST already does. + +### Revoking refresh tokens is not enough on its own + +`RefreshAsync` loads the token and user, then revokes the presented token and +inserts a child. `RevokeAllActiveForUserAsync` is a separate bulk update. A +refresh already in flight can therefore load *before* a disable, let the bulk +revoke commit, and insert its new active child *after* — a live refresh token +the disable never saw. For an email change that child works immediately; for a +disable it lies dormant and works the moment someone re-enables. + +The epoch closes this: the child was minted under the old epoch, so `RefreshAsync` +refuses it, and any access token it could have produced fails the per-request +check. Bulk revocation stays — it is still the right thing to do — but it is no +longer what the guarantee rests on. + +### The last-active-Owner guard needs an account-wide lock + +Two guards are required or the farm can lock itself out: a user may not disable +themselves, and the last active Owner may not be disabled. `bootstrap-admin` +creates exactly one Owner by default, and `recover-admin` resets a password +without re-enabling anything. + +A naive check is **not race-safe**, and Identity's `ConcurrencyStamp` does not +save it. Owners A and B disable each other concurrently: each reads two active +Owners, each targets a *different* user row, both guards pass, both commit, zero +active Owners remain. There is no shared concurrency token between two different +rows. + +The operation needs an account-wide serialization point, taken inside the same +transaction, using the pattern already in the repo +(`AccountRepository.GetCurrentLockedAsync`, `SELECT … FOR UPDATE`): + +1. Lock the tenant's `Accounts` row. +2. Re-read, account-scoped: the target user, its Owner membership, and the + active-Owner count. +3. Apply the guards. +4. Write, and commit. + +Every future path that can remove an Owner — role demotion (#355), any eventual +delete — must take the same lock. That is a standing constraint, not a detail of +this slice. + +### `recover-admin` re-enables, and gains a lookup that does not need the email + +`BreakGlassResetAsync` clears `DisabledAt` and bumps the epoch. Without the +first, the break-glass path cannot recover from a disable — the exact scenario +where it is most needed. + +`AdminRecoveryService` currently locates its target *by email*, which is +circular if the lockout was caused by an email typo. It gains an +`--account`-plus-`--user-id` lookup as an alternative to `--email`. + +### Changing an email + +The email is the login identifier. The change bumps the epoch, revokes every +refresh token, and invalidates step-up grants; the target logs in again at the +new address. The collateral is one re-login. + +**All four columns are written together**, through Identity's configured +normalizer: `Email`, `NormalizedEmail`, `UserName`, **and `NormalizedUserName`**. +The unique index is `UserNameIndex` on `NormalizedUserName`; `EmailIndex` on +`NormalizedEmail` is **not** unique. Writing only the first three would leave the +old address permanently reserved, bypass conflict detection on the new one, and +allow duplicate `NormalizedEmail` rows that make `FindByEmailAsync` ambiguous. + +A **sole Owner may not change their own email.** Step-up proves the old +password; it does not prove the new address was typed correctly. A typo would +change the only login identifier and end the session in the same request. Same +guard family as self-disable. + +The conflict response mirrors what `CreateUser` already does, which deliberately +does not reveal whether a colliding address belongs to another tenant — email +uniqueness is global because login has no account discriminator, and a bare +"address already taken" would be a cross-tenant oracle the existing code is +careful to avoid. + +### There is no email verification, and the UI says so -### Changing an email revokes the target's sessions +There is no SMTP path in this application. The Owner's word is the verification. +The Help page states this so nobody waits for a confirmation mail that will +never arrive. -The email is the login identifier. Changing it rotates the security stamp, -revokes every refresh token, and invalidates outstanding step-up grants, the -same posture `SetUserPasswordAsync` already takes. The collateral is one -re-login; the principle is that a changed identifier re-establishes the session. +## Threat model, stated accurately -### Both operations require a step-up grant (#308) +Both operations require a step-up grant (#308), on the grounds that they are the +levers that remove or repoint access. -Consider a stolen Owner access token with ~15 minutes of validity. Change a -worker's email to an attacker-controlled address, then reset that worker's -password — which is *not* step-up-gated for worker targets -(`ResetWorkersPassword_NeedsNoStepUp_OrdinaryAdministrationStaysUngated`). That -converts a short window into durable access under an account nobody has reason -to audit. Gating the identifier change closes the half of that chain the -attacker cannot otherwise obtain. +The first draft justified this with a stolen-Owner-token chain and **that +justification was wrong**. `CreateUserHandler` requires step-up only when the new +user's role is Owner; creating a Manager, Sales user, or Worker is ungated, as is +resetting any non-Owner's password. A stolen Owner bearer token already converts +into durable Manager access without touching anybody's email. Gating email change +does not close that chain, because the chain does not need it. -Disable is gated on the same grounds: it is the lever that removes access. +Two honest statements replace it: -### No email verification exists, and the UI says so +- Gating these operations is still right: they are destructive and + identity-altering, and #308's purpose is to require a recent proof of the + human before such an act. +- The durable-persistence gap is real and belongs to #308, not here. Closing it + means gating *all* administrative user creation and *every* administrative + password reset, which is a change to #308's stated design ("avoid a blanket + prompt") and needs its own decision. **Filed separately.** -There is no SMTP path in this application. The Owner's word is the verification. -The Help page states this so nobody waits for a confirmation mail that will never -arrive. +One residual limit, documented rather than fixed: step-up grants live in an +in-process registry, so "single-use" holds per replica. A captured access token +plus grant can be spent once against each. That is **#338**, pre-existing, and it +becomes more load-bearing once step-up is what stands between a stolen token and +disabling the farm's users. ## Design @@ -108,28 +219,33 @@ arrive. `ApplicationUser` gains: ```csharp -public DateTimeOffset? DisabledAt { get; set; } // null = active -public Guid? DisabledBy { get; set; } // the Owner who did it +public DateTimeOffset? DisabledAt { get; set; } // null = active; a record, not the gate +public Guid? DisabledBy { get; set; } // denormalized audit metadata +public int CredentialEpoch { get; set; } // monotonic; the gate ``` -A nullable timestamp, not an `IsActive` bool. The domain aggregates (`Product`, -`EggGrade`, `InventoryItem`, `ExpenseCategory`) use a bool because they carry no -audit trail of their own; a user account is a security object where "when, and by -whom" is the first question asked. `DisabledBy` is a plain `Guid?` with no FK — -the referenced Owner may themselves be disabled later, and an FK buys nothing. +`DisabledAt` is a nullable timestamp rather than the `IsActive` bool the domain +aggregates use: a user account is a security object where "when, and by whom" is +the first question asked. + +`DisabledBy` carries **no FK**, and the honest reason is that it is denormalized +audit metadata, not a modelled relationship. (The first draft claimed an FK would +be invalidated by later disabling the referenced Owner. That is false — disabling +a row does not break a foreign key.) A plain `Guid?` does permit a cross-account +value; the writer is always the acting Owner in the same account, and the column +is never read for authorization. -One new migration: two nullable columns, no backfill, no data statements. -Nothing resembling the hand-carried `InitialCreate` content, so a plain -`dotnet ef migrations add` is correct and `MigrationSecurityReviewTests` stays -green (no INSERTs, no credential-shaped rows). +One migration: three columns, `CredentialEpoch` non-nullable with default `0`, +no data statements. `MigrationSecurityReviewTests` stays green. ### 2. Application layer -Two new features under `Features/Users/`, handler-per-feature: +Two features under `Features/Users/`, handler-per-feature. **`SetUserActive/`** — `SetUserActiveCommand(Guid UserId, bool Active, string? StepUpToken)`. -One handler backs both directions, mirroring `SetProductActiveHandler`. Guards, -in order: +One handler backs both directions, mirroring `SetProductActiveHandler`. + +Inside one transaction, after taking the account lock: | Condition | Result | |---|---| @@ -137,35 +253,35 @@ in order: | Target is the last active Owner (disable only) | 422 `Users.LastOwner` | | Target not in this account | 404 | -On disable: stamp `DisabledAt` / `DisabledBy`, rotate `SecurityStamp`, revoke -every refresh token, invalidate outstanding step-up grants. On enable: clear -`DisabledAt` / `DisabledBy`. Audit `User.Disable` / `User.Enable`. +On disable: stamp `DisabledAt` / `DisabledBy`, **bump `CredentialEpoch`**, rotate +`SecurityStamp`, revoke every refresh token, invalidate step-up grants. +On enable: clear `DisabledAt` / `DisabledBy`, and **leave the epoch alone**. +Audit `User.Disable` / `User.Enable`. **`ChangeUserEmail/`** — `ChangeUserEmailCommand(Guid UserId, string Email, string? StepUpToken)`. -Writes `Email`, `UserName`, and `NormalizedEmail` together — never one alone. -409 on the unique index. Then the same revoke-everything treatment as a disable. -Audit `User.EmailChanged`, recording old → new. +Writes the four columns atomically through the normalizer, bumps the epoch, and +takes the same revoke-everything path. Refuses a sole Owner changing their own +address. Audit `User.EmailChanged`, old → new. Both call `IStepUpGrantService.ValidateAsync` first, in the shape -`CreateUserHandler` already uses. - -`IIdentityProvider` gains `SetUserActiveAsync` and `ChangeUserEmailAsync`, both -account-scoped so a foreign id resolves to NotFound rather than a cross-tenant -write. `UserSummary` and `UserProfile` gain `DisabledAt`. - -#### Retry hazard (#269 family) - -Both mutate an Identity user through `UserManager`, which is a `ConcurrencyStamp` -CAS. A replayed `SaveChanges` after an ambiguous commit re-issues the `UPDATE` -under a stale stamp, matches zero rows, and reports failure for work that landed. - -Unlike `AccessFailedAsync`, neither is a counter — a replay writes nothing new -and is harmless in itself. So the cure is the `RefreshAsync` pattern, **not** -`SingleAttemptExecution`: on failure, ask the database whether this attempt's own -write is durable (is `DisabledAt` already our value? is the email already ours?), -and let the original error stand otherwise. Fails closed. Both are also -idempotency-key-wrapped writes, so the client-retry path covers most of it -already. +`CreateUserHandler` uses. `IIdentityProvider` gains `SetUserActiveAsync` and +`ChangeUserEmailAsync`, both account-scoped so a foreign user id resolves to +NotFound, never a cross-tenant write. `UserSummary` / `UserProfile` gain +`DisabledAt`. + +`StepUpGrantService.IssueAsync` additionally refuses a disabled user, so a grant +cannot be minted in the window after a concurrent disable. + +**On retries:** nothing special is needed. These are HTTP writes, so +`IdempotencyMiddleware` already runs the whole downstream pipeline inside its +request-wide transaction under `SingleAttemptExecution` — the inner Identity save +is never independently replayed, and an ambiguous outer commit is resolved by the +middleware replaying its own published response. A non-HTTP caller (a CLI verb) +wraps the user update, token revocation, and audit write in `AmbientTransaction`, +whose owned path is already single-attempt. (The first draft prescribed a +`RefreshAsync`-style durability probe here. That was wrong twice over: the save is +not independently replayed, and "the email is already ours" is not +attempt-unique evidence the way a freshly minted 256-bit token hash is.) ### 3. API surface @@ -176,94 +292,150 @@ PUT /api/v1/users/{id}/email -> 204 (step-up; 409 on conflict) ``` Disable/enable are separate verbs rather than a field on `PUT /users/{id}`, -following `POST /products/{id}/deactivate` and `/egg-grades/{id}/deactivate`. +following `POST /products/{id}/deactivate`. Email is separate from `UpdateUser` +because it carries different auth weight — `UpdateUser` (display name) is +ungated and stays that way. All three inherit the group's `OwnerOnly` policy and +the `Idempotency-Key` requirement. `WithMaxRequestBodyBytes(2048)` on the email +PUT, matching the password endpoint. -Email is separate from `UpdateUser` because it carries different auth weight — -`UpdateUser` (display name) is ungated and stays that way. +`ListUsers` and `GetUser` grow `disabledAt`. -All three inherit the group's `OwnerOnly` policy and the `Idempotency-Key` -requirement. `WithMaxRequestBodyBytes(2048)` on the email PUT, matching the -password endpoint's reasoning. +### 4. Enforcement -`ListUsers` and `GetUser` responses grow `disabledAt`. +`JwtTokenService` stamps the epoch into every access token. -### 4. Enforcement +New `CredentialEpochMiddleware`, registered at an explicit position in +`Program.cs`: + +``` +UseAuthentication → TenantResolutionMiddleware → CredentialEpoch + → MustChangePassword → UseAuthorization → Idempotency +``` -New `DisabledUserMiddleware`, placed beside `MustChangePasswordMiddleware`: +**After authentication** — before it, every caller looks anonymous and the +middleware is inert. **Before `UseAuthorization`** — so it applies uniformly +regardless of an endpoint's `AuthPolicies` tier. **Before idempotency** — so a +blocked write burns no key. -- **before `UseAuthorization`** — so it applies uniformly regardless of an - endpoint's `AuthPolicies` tier; -- **before the idempotency middleware** — so a blocked write burns no key. +It must: -One indexed PK lookup per authenticated request. Refuses with 401 and a distinct -`Auth.AccountDisabled` title, so the SPA can show "Your account has been -disabled" rather than a generic session-expired bounce. +- look the user up by **both `UserId` and `AccountId`** — `ApplicationUser` has + no global tenant query filter, so the filter that protects every other entity + is absent here; +- use an untracked projection, so it does not poison the request's Identity + change tracker; +- skip re-execution under `IExceptionHandlerFeature`, as + `MustChangePasswordMiddleware` does; +- leave `auth/logout` reachable, so a dead session can clear its own cookie. + +It answers 401 with a distinct `Auth.CredentialsSuperseded` title, and disable +additionally surfaces `Auth.AccountDisabled`, so the SPA can tell "you were +disabled" from "your credentials were rotated." `LoginAsync` refuses a disabled user with the *same* generic `Identity.InvalidCredentials` as a wrong password, still paying the PBKDF2 cost — -the reasoning the lockout branch already documents, so the reply never reveals -account state. `RefreshAsync` refuses too. +the reply must never reveal account state, the reasoning the lockout branch +already documents. `RefreshAsync` refuses a disabled user **and** any token +minted under a superseded epoch. ### 5. SPA -Users page lists disabled users inline, always visible: a "Disabled" badge and a -de-emphasized row, with the action toggling to "Enable". Hiding people who still -own history invites "where did Maria go?" and a duplicate account. A -`Show disabled` filter is the right answer once a farm has years of turnover — -`ListUsers` returns a flat unpaged array today, so that is a larger change than -this feature warrants. - -Disable goes through a confirm dialog (existing `useConfirm()` hook): names the -user, warns that they are signed out immediately and lose access, Cancel / -Disable. Enable is a plain one-click — it is not destructive. - -Email editing is a form on the user row, surfacing the 409 as a field error. +Disabled users stay listed inline: a "Disabled" badge, a de-emphasized row, and +the action toggling to "Enable". Hiding people who still own history invites +"where did Maria go?" and a duplicate account. A `Show disabled` filter is the +right answer once a farm has years of turnover — `ListUsers` returns a flat +unpaged array today, so that is a larger change than this warrants. + +Disable goes through a confirm dialog (existing `useConfirm()`): names the user, +warns they are signed out immediately and lose access, Cancel / Disable. Enable +is one click — not destructive. Email editing is a form on the user row, +surfacing the 409 as a field error. + +**The 401 reason has to survive the auth teardown.** Today `client.ts` turns any +authenticated 401 into refresh-and-retry then calls a parameterless +`onUnauthenticated`, and `Login.tsx` maps every 401 to "invalid credentials". So +returning a distinct title is necessary but not sufficient: the teardown path +must carry the original error title through to the login screen, or the promised +"Your account has been disabled" can never render. That plumbing is part of this +slice, not an afterthought. ### 6. Testing -**Domain / Application** — guard rules: self-disable, last-active-Owner, both -directions of the toggle, email normalization, email conflict. +**Application** — the guards: self-disable, last-active-Owner, sole-Owner +self-email-change, both toggle directions, email normalization, conflict. **Integration** - A disabled user cannot log in, and the refusal is indistinguishable from a wrong password. - A disabled user cannot refresh. -- A **live access token** stops working on the next request. This is the point of - the middleware and the test that fails if it is ever dropped. -- Step-up grants die with the disable. -- An email change signs the target out; the new address logs in; the old does not. -- 409 on an address already taken. -- `recover-admin` re-enables a disabled target. - -**Race** — two Owners disabling each other concurrently must not land at zero -active Owners. This is the `Version`-bump family of bug the repo has shipped -three times, wearing Identity's `ConcurrencyStamp` clothing. It gets a -parallel-race integration test like every other mutation. - -**Retry** — a `RetryBoundaryTests` case per the hazard in §2, using -`TransientCommitFaultInterceptor`. Not `TransientCommandFaultInterceptor`: these -are multi-statement saves, where that interceptor is a fail-*before* and yields a -green test with the defect fully present. - -**SPA (Vitest)** — the confirm dialog, disabled-row rendering, the enable path, -the email-edit form including the 409. +- A **live access token stops working on the next request**. The test that fails + if the middleware is ever dropped. +- **Re-enabling does not resurrect a pre-disable access token.** The test that + fails if the epoch degrades back into a boolean check. +- An email change kills the target's live access token, not just their refresh + tokens. +- Step-up grants die with the disable, and none can be issued to a disabled user. +- 409 on an address already taken, including one held by **another account**, + without revealing which. +- A foreign-account user id → 404 on every new endpoint. +- `recover-admin` re-enables, and can locate its target without the email. + +**Race** — barrier-controlled, because sequential tests pass with every one of +these bugs present: + +- Two Owners disabling each other concurrently must not reach zero active Owners. +- A refresh in flight across a disable must not leave a usable child token. +- A refresh in flight across an email change must not leave a usable child token. +- A step-up issuance racing a disable must not produce a usable grant. + +**Middleware ordering** — asserted explicitly, since the guarantee is positional +and a silent reorder fails open. + +**SPA (Vitest)** — confirm dialog, disabled-row rendering, enable path, the +email-edit form including the 409, and the disabled-reason surviving all the way +to the login screen. ### 7. Docs -- `specs/product/GLOSSARY.md` gains **disabled user**. -- Help page gains a Users section: what disabling does, that it is reversible, - that history is retained, and that an email change sends no verification mail. +- `specs/product/GLOSSARY.md` gains **disabled user** and **credential epoch**. +- Help page: what disabling does, that it is reversible with history retained, + and that an email change signs the user out and sends no verification mail. - Strings ship with es/tl inline per the translate-now policy, added to `TRANSLATED_NAMESPACES`. ## Delivery -Two issues against epic #14, both added to its checklist: - -1. **Disable / enable a user** — storage, guards, middleware, `recover-admin` - re-enable, SPA toggle + confirm. -2. **Change a user's email** — reuses the revoke-sessions machinery from (1), - so it ships second. - -Role editing is filed separately. +1. **#356 — disable / enable a user.** Carries the credential epoch, the account + lock, the middleware, the `recover-admin` changes, and the SPA plumbing for + the 401 reason. Larger than the first draft implied. +2. **#357 — change a user's email.** Reuses all of it; adds the four-column + atomic write and the sole-Owner guard. Smaller than the first draft implied — + the retry work it described does not exist. + +Role editing is **#355**. The #308 step-up-coverage gap is filed separately. + +## What the first draft got wrong + +Recorded so the same reasoning is not re-derived later. + +1. **`SecurityStamp` rotation was treated as session termination.** It is not — + the token carries no stamp and nothing on the request path reads one. This + invalidated the draft's central promise for both features. +2. **The per-request check read `DisabledAt`.** Re-enabling would then resurrect + every unexpired pre-disable access token. +3. **The last-Owner guard was called race-safe on the strength of + `ConcurrencyStamp`.** Two Owners disabling each other touch different rows and + share no concurrency token. It needs an account-wide lock. +4. **The email write omitted `NormalizedUserName`** — the column that actually + carries the unique index. `NormalizedEmail`'s index is not unique. +5. **The step-up justification described a takeover chain that does not require + an email change.** A stolen Owner token already mints a Manager, ungated. +6. **A `RefreshAsync`-style durability probe was prescribed for a save that is + never independently replayed**, on evidence that is not attempt-unique. +7. **Bulk refresh-token revocation was assumed sufficient.** A concurrent refresh + can insert a live child after the revoke commits. +8. **Self-email-change was not guarded**, so a sole Owner could lock themselves + out with a typo, against a `recover-admin` that looks its target up by email. +9. **The `DisabledBy` FK omission was justified with a false claim** about + foreign keys being invalidated by a disabled referent. From 87f922dfad372f7142cb145217b14431f9cb5f8f Mon Sep 17 00:00:00 2001 From: mforce <> Date: Sat, 1 Aug 2026 19:04:37 -0700 Subject: [PATCH 03/13] spec: bind the epoch to refresh tokens, not just access tokens MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex round 2, against the fix from round 1 — and it is right. The second draft put CredentialEpoch on ApplicationUser alone. That rejects the access token a racing refresh mints, and stops there. The child REFRESH token is a durable row with RevokedAt == null, which RevokeAllActiveForUserAsync never saw (it is a bulk ExecuteUpdate over rows unrevoked at that instant; it cannot see a row that does not exist yet). Present that child and RefreshAsync reads the user's CURRENT epoch and mints a perfectly valid current-epoch pair — immediately after an email change, and after re-enabling for a disable. The resurrection the epoch exists to prevent, re-entered through the refresh door. RefreshToken now carries IssuedEpoch, stamped at every mint site (LoginAsync, the rotation, ChangeOwnPasswordAsync's re-issue), and RefreshAsync refuses a mismatch BEFORE rotating, so a superseded token is never consumed and never mints a child. A bump kills the family by construction rather than by winning a race. Bulk revocation stays for promptness but is no longer what the guarantee rests on. Default 0 backfills correctly in both directions: existing users start at epoch 0 and every existing token was minted under it, so the deploy evicts no live session. The race tests are restated to assert by PRESENTING the child token, and to re-present it after enable. Asserting only that the child's access token is rejected passes with the defect fully present — which is exactly how the second draft talked itself into being finished. The general lesson is recorded in the spec: a revocation epoch must be bound to every credential the system will later accept, not just the one whose weakness prompted it. The rewrite was specifically about the access token having no binding, and left the credential whose entire job is to outlive it with none. --- ...026-08-01-user-disable-and-email-design.md | 90 +++++++++++++++---- 1 file changed, 75 insertions(+), 15 deletions(-) diff --git a/docs/superpowers/specs/2026-08-01-user-disable-and-email-design.md b/docs/superpowers/specs/2026-08-01-user-disable-and-email-design.md index cb4b7dcb0..355329e27 100644 --- a/docs/superpowers/specs/2026-08-01-user-disable-and-email-design.md +++ b/docs/superpowers/specs/2026-08-01-user-disable-and-email-design.md @@ -3,9 +3,10 @@ **Date:** 2026-08-01 **Phase:** 1.1 (epic #14) **Status:** design, awaiting implementation plan -**Revision:** second draft, after an adversarial review of the first. The -changes are substantive and are recorded in *What the first draft got wrong* at -the end — read that before treating any of this as a small delta. +**Revision:** third draft. Each round of review found the previous round's +enforcement mechanism incomplete; the corrections are recorded at the end, in +*What the first draft got wrong* and *What the second draft got wrong*. Read +both before treating any of this as a small delta. ## Problem @@ -92,8 +93,10 @@ This subsumes the disabled check rather than sitting beside it: the middleware compares one claim to one column, and `DisabledAt` goes back to being what it should be — the record of a state, not the enforcement mechanism. -`RefreshAsync` re-checks both the disabled state and the epoch. That closes the -issuance race below. +The epoch has to be bound to **every** credential the system will later accept, +not only the access token. The refresh token gets its own stamped copy, checked +before rotation — see the next section for why leaving it off defeats the whole +mechanism. ### Why not a cache @@ -112,10 +115,25 @@ revoke commit, and insert its new active child *after* — a live refresh token the disable never saw. For an email change that child works immediately; for a disable it lies dormant and works the moment someone re-enables. -The epoch closes this: the child was minted under the old epoch, so `RefreshAsync` -refuses it, and any access token it could have produced fails the per-request -check. Bulk revocation stays — it is still the right thing to do — but it is no -longer what the guarantee rests on. +**The epoch closes this only if the refresh token carries its own issuance +epoch.** Putting `CredentialEpoch` on the user alone is not enough, and the +second draft got this wrong: the child's *access* token would indeed be rejected +(minted under the old epoch), but the child *refresh* token is a durable row with +`RevokedAt == null`, and presenting it makes `RefreshAsync` read the user's +**current** epoch and mint a perfectly valid current-epoch pair. For an email +change that works immediately; for a disable the child lies dormant and works the +moment someone re-enables — the exact resurrection the epoch exists to prevent, +re-entered through the refresh door. + +So `RefreshToken` carries `IssuedEpoch`, stamped at mint time, and `RefreshAsync` +refuses any token whose `IssuedEpoch` differs from the user's current +`CredentialEpoch` **before** rotating. A bump therefore kills the whole family by +construction, whether or not the bulk revoke happened to win the race. + +Bulk revocation stays — it is still the right thing to do, and it closes the +window promptly — but it is no longer what the guarantee rests on. That matters +because `RevokeAllActiveForUserAsync` is a bulk `ExecuteUpdate` over rows that +are unrevoked *at that instant*; it cannot see a row that does not exist yet. ### The last-active-Owner guard needs an account-wide lock @@ -235,8 +253,22 @@ a row does not break a foreign key.) A plain `Guid?` does permit a cross-account value; the writer is always the acting Owner in the same account, and the column is never read for authorization. -One migration: three columns, `CredentialEpoch` non-nullable with default `0`, -no data statements. `MigrationSecurityReviewTests` stays green. +`RefreshToken` gains the matching half: + +```csharp +public int IssuedEpoch { get; set; } // the user's CredentialEpoch when this token was minted +``` + +Without it the epoch is defeated through the refresh door — see *Revoking +refresh tokens is not enough on its own* above. Every mint site stamps it: +`LoginAsync`, `RefreshAsync`'s rotation, and `ChangeOwnPasswordAsync`'s +re-issue. + +One migration: three columns on `AspNetUsers` and one on `refresh_tokens`, both +epochs non-nullable with default `0`, no data statements. Default `0` is correct +for the backfill in both directions: every existing user starts at epoch `0`, and +every existing refresh token was minted under it, so no live session is evicted +by the deploy. `MigrationSecurityReviewTests` stays green. ### 2. Application layer @@ -335,8 +367,13 @@ disabled" from "your credentials were rotated." `LoginAsync` refuses a disabled user with the *same* generic `Identity.InvalidCredentials` as a wrong password, still paying the PBKDF2 cost — the reply must never reveal account state, the reasoning the lockout branch -already documents. `RefreshAsync` refuses a disabled user **and** any token -minted under a superseded epoch. +already documents. + +`RefreshAsync` refuses a disabled user, **and refuses any token whose +`IssuedEpoch` differs from the user's current `CredentialEpoch`** — checked +before the rotation, so a superseded token is never consumed and never mints a +child. That check, not the bulk revocation, is what makes a bump terminate the +whole token family. ### 5. SPA @@ -385,8 +422,13 @@ self-email-change, both toggle directions, email normalization, conflict. these bugs present: - Two Owners disabling each other concurrently must not reach zero active Owners. -- A refresh in flight across a disable must not leave a usable child token. -- A refresh in flight across an email change must not leave a usable child token. +- A refresh in flight across a disable must not leave a usable child token — + asserted **by presenting the child**, not by checking that the access token it + minted is rejected. The weaker assertion passes with the defect present. +- The same child must still be unusable **after the user is re-enabled**. This is + the one that fails if `IssuedEpoch` is left off the refresh token. +- A refresh in flight across an email change must not leave a usable child token, + asserted the same way. - A step-up issuance racing a disable must not produce a usable grant. **Middleware ordering** — asserted explicitly, since the guarantee is positional @@ -439,3 +481,21 @@ Recorded so the same reasoning is not re-derived later. out with a typo, against a `recover-admin` that looks its target up by email. 9. **The `DisabledBy` FK omission was justified with a false claim** about foreign keys being invalidated by a disabled referent. + +## What the second draft got wrong + +10. **The epoch was put on the user only.** That rejects the *access* token a + racing refresh mints, and stops there — the *child refresh token* is a + durable row the bulk revoke never saw, and presenting it makes + `RefreshAsync` read the user's current epoch and mint a valid current-epoch + pair. Immediately after an email change; after re-enabling, for a disable. + The fix is `RefreshToken.IssuedEpoch`, compared before rotation. + + The general lesson, and the reason this survived a rewrite that was + *specifically about* this failure mode: a revocation epoch has to be bound to + **every credential the system will later accept**, not just the one whose + weakness prompted it. The second draft found the access token had no binding + and fixed exactly that, while the refresh token — the credential whose entire + job is to outlive the access token — kept none. Any future credential type + (a step-up grant made durable per #338, an API key, a device token) must be + stamped at mint and checked at use, or it reopens the same door. From bd864c7382620067214c2c51c85c32d0a7d01382 Mon Sep 17 00:00:00 2001 From: mforce <> Date: Sat, 1 Aug 2026 19:20:14 -0700 Subject: [PATCH 04/13] spec: fix the epoch cutover and the check's position in RefreshAsync MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex round 3, against round 2's fix. Both findings are real, and both are the same mistake in different clothes. The cutover was designed to preserve live sessions, and the previous commit message called that a virtue. It is the defect. The pre-epoch code already has the revoke-vs-refresh race, so a legacy token sitting RevokedAt == null may already be an attacker's orphan from some earlier password reset. Backfilling every user and token to 0 certifies that row as current-epoch, and it stays usable until an unrelated future bump. The migration cannot tell which legacy tokens predate their user's last credential change, so it cannot be selective: the cutover now revokes every existing refresh token and accepts one forced re-login. That is the only moment the mechanism can be handed a trustworthy starting state, and for a farm's worth of users it costs nothing. The epoch comparison was placed "before the rotation," which is too late. RefreshAsync resolves the token, and if it is already revoked runs #176's grace/replay branch — calling RevokeAllActiveForUserAsync and returning — BEFORE FindByIdAsync ever loads the user. So the check never runs on that path. Not a bypass: a denial of service. An attacker holding a superseded pre-bump token waits for the legitimate user to log in again, presents the dead token, and burns down their brand-new current-epoch family, repeatably. The comparison moves ahead of the grace branch, and an epoch mismatch now fails inert — generic error, no family revocation — because a superseded token is evidence about a session that is already dead. Same-epoch reuse still burns the family, which is #176's actual purpose, and there is now a test pinning both sides so the fix cannot be implemented by weakening theft detection. The shared shape is named in the spec, since this is its third appearance: a check is only as good as the earliest path that can reach what it protects. Draft two bound the epoch to the access token and missed the refresh token. Draft three bound it to the refresh token and missed the branch that acts on one without loading the user. Enumerate every entry point to the guarded state, then place the guard above all of them — not next to the code that prompted it. The new test is the one that would not have been written otherwise: it fails as a denial of service, so no "can the attacker get in?" test would have caught it. --- ...026-08-01-user-disable-and-email-design.md | 94 ++++++++++++++++--- 1 file changed, 83 insertions(+), 11 deletions(-) diff --git a/docs/superpowers/specs/2026-08-01-user-disable-and-email-design.md b/docs/superpowers/specs/2026-08-01-user-disable-and-email-design.md index 355329e27..fff6c5daf 100644 --- a/docs/superpowers/specs/2026-08-01-user-disable-and-email-design.md +++ b/docs/superpowers/specs/2026-08-01-user-disable-and-email-design.md @@ -3,10 +3,10 @@ **Date:** 2026-08-01 **Phase:** 1.1 (epic #14) **Status:** design, awaiting implementation plan -**Revision:** third draft. Each round of review found the previous round's +**Revision:** fourth draft. Each round of review has found the previous round's enforcement mechanism incomplete; the corrections are recorded at the end, in -*What the first draft got wrong* and *What the second draft got wrong*. Read -both before treating any of this as a small delta. +*What the first/second/third draft got wrong*. Read all three before treating +any of this as a small delta — the corrections are the substance. ## Problem @@ -265,10 +265,26 @@ refresh tokens is not enough on its own* above. Every mint site stamps it: re-issue. One migration: three columns on `AspNetUsers` and one on `refresh_tokens`, both -epochs non-nullable with default `0`, no data statements. Default `0` is correct -for the backfill in both directions: every existing user starts at epoch `0`, and -every existing refresh token was minted under it, so no live session is evicted -by the deploy. `MigrationSecurityReviewTests` stays green. +epochs non-nullable with default `0`. + +**The cutover revokes every existing refresh token.** Backfilling users and +tokens both to `0` would preserve live sessions — and that is the defect, not a +feature. The pre-epoch codebase already has the revoke-vs-refresh race described +above, so at migration time an attacker-held child token may already be sitting +`RevokedAt == null` after some earlier password reset. Certifying every legacy +token as current-epoch would bless exactly that row, and it would stay usable +until some unrelated future bump. The migration cannot tell which legacy tokens +predate their user's last credential change, so it cannot be selective — the only +clean boundary is to invalidate all of them. + +The cost is one forced re-login for everyone at deploy. For a farm's worth of +users that is trivially cheap, and it is the single moment where the new +mechanism can be given a trustworthy starting state. + +This is the one data statement in the migration (`UPDATE refresh_tokens SET +"RevokedAt" = now() WHERE "RevokedAt" IS NULL`) — no INSERTs, so +`MigrationSecurityReviewTests`' shape assertions are unaffected, but its count +pins need updating alongside. ### 2. Application layer @@ -370,10 +386,26 @@ the reply must never reveal account state, the reasoning the lockout branch already documents. `RefreshAsync` refuses a disabled user, **and refuses any token whose -`IssuedEpoch` differs from the user's current `CredentialEpoch`** — checked -before the rotation, so a superseded token is never consumed and never mints a -child. That check, not the bulk revocation, is what makes a bump terminate the -whole token family. +`IssuedEpoch` differs from the user's current `CredentialEpoch`**. That check, +not the bulk revocation, is what makes a bump terminate the whole token family. + +**Where the check goes is load-bearing, and "before the rotation" is too late.** +The current flow looks up the token, and *if it is already revoked* runs the #176 +grace/replay branch — which, when grace does not apply, calls +`RevokeAllActiveForUserAsync` and returns. That happens **before** +`FindByIdAsync` loads the user, so an epoch check placed anywhere near the +rotation never runs on this path. The consequence is not a bypass but a +denial of service: an attacker holding a superseded pre-bump token waits for the +legitimate user to log in again, presents the dead token, and burns down the +victim's brand-new current-epoch family. Repeatably. + +So the order is: resolve the token, **load the user and compare epochs**, and +only then enter the grace/replay branch. An epoch mismatch fails closed *and +inert* — the generic `Identity.InvalidRefreshToken`, with **no** family +revocation, because a superseded token is evidence about a session that is +already dead and says nothing about the live one. Family revocation is preserved +for its real purpose: genuine same-epoch reuse, which is still the theft signal +#176 exists to catch. ### 5. SPA @@ -430,6 +462,19 @@ these bugs present: - A refresh in flight across an email change must not leave a usable child token, asserted the same way. - A step-up issuance racing a disable must not produce a usable grant. +- **A superseded token must not burn down the current family.** Bump the epoch, + let the user log in again, then present the old pre-bump token: the new session + keeps working. This is the test that fails if the epoch comparison sits + anywhere after the grace/replay branch — and it fails as a denial of service, + not as an access grant, so it will not show up in any "can the attacker get + in?" test. +- **Genuine same-epoch reuse still burns the family.** The counterpart, so the + fix above cannot be implemented by simply weakening #176's theft detection. + +**Cutover** — a refresh token that was active before the migration is rejected +after it, and the user is forced to log in again. Asserted against a database +seeded pre-migration, since the whole point is a boundary the migration draws +across data it did not create. **Middleware ordering** — asserted explicitly, since the guarantee is positional and a silent reorder fails open. @@ -499,3 +544,30 @@ Recorded so the same reasoning is not re-derived later. job is to outlive the access token — kept none. Any future credential type (a step-up grant made durable per #338, an API key, a device token) must be stamped at mint and checked at use, or it reopens the same door. + +## What the third draft got wrong + +11. **The cutover was designed to preserve live sessions**, and that was stated + as a virtue ("evicts no live session"). It is the defect. The pre-epoch code + already has the revoke-vs-refresh race, so a legacy token sitting + `RevokedAt == null` may already be an attacker's orphan; backfilling every + token and user to `0` certifies it as current. The migration cannot tell + which legacy tokens predate their user's last credential change, so the only + clean boundary is to revoke all of them and accept one forced re-login. + +12. **The epoch comparison was placed "before the rotation," which is too late.** + A revoked token reaches #176's grace/replay branch — and + `RevokeAllActiveForUserAsync` — before `FindByIdAsync` loads the user, so the + check never runs on that path. An attacker holding a superseded token can + therefore burn down the victim's new current-epoch family after they log back + in. The comparison moves ahead of the grace branch, and a mismatch fails + inert rather than revoking anything. + + Both of these share a shape worth naming, because it is the third time it has + appeared in this document: **a check is only as good as the earliest path + that can reach the thing it protects.** Draft two bound the epoch to the + access token and missed the refresh token; draft three bound it to the + refresh token and missed the branch that acts on a refresh token *without + ever loading the user*. When adding a guard, enumerate every entry point to + the guarded state first, then place the guard above all of them — do not + place it next to the code that prompted the guard. From c4c143ffa8ee88ef99a32759a83a89761356bfbb Mon Sep 17 00:00:00 2001 From: mforce <> Date: Sat, 1 Aug 2026 19:20:33 -0700 Subject: [PATCH 05/13] spec: remove the stale 'before rotating' phrasing Two summary lines still said the epoch check sits before the rotation, which is precisely the placement the previous commit found insufficient. Both now point at the grace/replay branch. --- .../specs/2026-08-01-user-disable-and-email-design.md | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/docs/superpowers/specs/2026-08-01-user-disable-and-email-design.md b/docs/superpowers/specs/2026-08-01-user-disable-and-email-design.md index fff6c5daf..56443222c 100644 --- a/docs/superpowers/specs/2026-08-01-user-disable-and-email-design.md +++ b/docs/superpowers/specs/2026-08-01-user-disable-and-email-design.md @@ -95,8 +95,8 @@ should be — the record of a state, not the enforcement mechanism. The epoch has to be bound to **every** credential the system will later accept, not only the access token. The refresh token gets its own stamped copy, checked -before rotation — see the next section for why leaving it off defeats the whole -mechanism. +ahead of the grace/replay branch — see the next section for why leaving it off +defeats the whole mechanism. ### Why not a cache @@ -127,8 +127,11 @@ re-entered through the refresh door. So `RefreshToken` carries `IssuedEpoch`, stamped at mint time, and `RefreshAsync` refuses any token whose `IssuedEpoch` differs from the user's current -`CredentialEpoch` **before** rotating. A bump therefore kills the whole family by -construction, whether or not the bulk revoke happened to win the race. +`CredentialEpoch`. A bump therefore kills the whole family by construction, +whether or not the bulk revoke happened to win the race. + +That comparison sits **ahead of the #176 grace/replay branch**, not merely ahead +of the rotation — see *Enforcement* for why the difference is the whole finding. Bulk revocation stays — it is still the right thing to do, and it closes the window promptly — but it is no longer what the guarantee rests on. That matters From b198dfb14dd2597ce6d50bdb520f396df42342a2 Mon Sep 17 00:00:00 2001 From: mforce <> Date: Sat, 1 Aug 2026 19:20:50 -0700 Subject: [PATCH 06/13] spec: stop the draft-two correction contradicting draft three --- .../specs/2026-08-01-user-disable-and-email-design.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/docs/superpowers/specs/2026-08-01-user-disable-and-email-design.md b/docs/superpowers/specs/2026-08-01-user-disable-and-email-design.md index 56443222c..3eb233d0b 100644 --- a/docs/superpowers/specs/2026-08-01-user-disable-and-email-design.md +++ b/docs/superpowers/specs/2026-08-01-user-disable-and-email-design.md @@ -537,7 +537,8 @@ Recorded so the same reasoning is not re-derived later. durable row the bulk revoke never saw, and presenting it makes `RefreshAsync` read the user's current epoch and mint a valid current-epoch pair. Immediately after an email change; after re-enabling, for a disable. - The fix is `RefreshToken.IssuedEpoch`, compared before rotation. + The fix is `RefreshToken.IssuedEpoch` — though the third draft then placed + that comparison too late in turn; see item 12. The general lesson, and the reason this survived a rewrite that was *specifically about* this failure mode: a revocation epoch has to be bound to From 21df253b48f7e9ff2dd525615dd364e1813823e4 Mon Sep 17 00:00:00 2001 From: mforce <> Date: Sat, 1 Aug 2026 19:29:30 -0700 Subject: [PATCH 07/13] spec: retire epoch 0 so the cutover boundary has a value, not just an event MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex round 4, against round 3's cutover fix. Right again. Revoking every legacy refresh token is not differentiation. Legacy tokens and users both sat at epoch 0, so once the victim logs in again their NEW token is also 0 — a stolen legacy token passes the epoch comparison, enters the revoked-token replay branch, and burns down the fresh family. The exact denial of service the check-placement fix existed to prevent, reintroduced by the fix for the cutover. Users now start at 1 and epoch 0 is permanently retired: it is the value a row gets when written by something that does not know the column exists, and it can never equal a live user's epoch. That also closes a rolling-deploy window the previous draft never considered. Production runs migrate as a separate job BEFORE the new serving process (#263), so an old binary can still be inserting refresh tokens while the columns exist. Its INSERTs take the database default, and 0 is precisely the value no user will ever carry — those rows are inert by construction rather than by timing. Nobody gets a credential the new binary honours; their holders just get logged out. The migration still revokes legacy tokens even though the epoch separation already makes them inert, and the direction of that defence-in-depth matters: if the comparison ever regresses, revoked-and-retired tokens degrade to a denial of service, while active-and-retired ones would degrade to access. Given the choice, take the failure mode that locks people out. Named the rule, because it is a different mistake from items 10-12: a boundary needs a VALUE, not just an EVENT. 'Revoke everything at the cutover' is an event, and an event does not survive a writer still running or a row written afterwards. A reserved sentinel the new world can never produce holds without depending on when anything happened. Tests: the cutover case now asserts the second half — that after the user logs in again, the stolen legacy token still cannot burn the new family. Revocation alone passes the first assertion and fails that one. Plus a default-0 row standing in for the old binary mid-rollout. --- ...026-08-01-user-disable-and-email-design.md | 117 +++++++++++++----- 1 file changed, 89 insertions(+), 28 deletions(-) diff --git a/docs/superpowers/specs/2026-08-01-user-disable-and-email-design.md b/docs/superpowers/specs/2026-08-01-user-disable-and-email-design.md index 3eb233d0b..d6b132e2f 100644 --- a/docs/superpowers/specs/2026-08-01-user-disable-and-email-design.md +++ b/docs/superpowers/specs/2026-08-01-user-disable-and-email-design.md @@ -3,10 +3,10 @@ **Date:** 2026-08-01 **Phase:** 1.1 (epic #14) **Status:** design, awaiting implementation plan -**Revision:** fourth draft. Each round of review has found the previous round's -enforcement mechanism incomplete; the corrections are recorded at the end, in -*What the first/second/third draft got wrong*. Read all three before treating -any of this as a small delta — the corrections are the substance. +**Revision:** fifth draft. Every round of review so far has found a real defect +in the previous round's fix. The corrections are recorded at the end, in +*What the first/second/third/fourth draft got wrong* — read them before treating +any of this as a small delta. The corrections are the substance. ## Problem @@ -79,9 +79,12 @@ stamped into every access token as a claim, compared against the database on every authenticated request. A mismatch is a 401. ```csharp -public int CredentialEpoch { get; set; } // starts at 0, only ever increments +public int CredentialEpoch { get; set; } = 1; // starts at 1, only ever increments ``` +It starts at `1`, not `0`, and `0` is permanently retired — see *Storage* for why +that one-off matters more than it looks. + Bumping it is the single act that means "every credential minted before now is dead." Disable bumps it. Email change bumps it. Password reset bumps it (all three paths — self-service, an Owner's `SetUserPasswordAsync`, break-glass). @@ -267,25 +270,54 @@ refresh tokens is not enough on its own* above. Every mint site stamps it: `LoginAsync`, `RefreshAsync`'s rotation, and `ChangeOwnPasswordAsync`'s re-issue. -One migration: three columns on `AspNetUsers` and one on `refresh_tokens`, both -epochs non-nullable with default `0`. - -**The cutover revokes every existing refresh token.** Backfilling users and -tokens both to `0` would preserve live sessions — and that is the defect, not a -feature. The pre-epoch codebase already has the revoke-vs-refresh race described -above, so at migration time an attacker-held child token may already be sitting -`RevokedAt == null` after some earlier password reset. Certifying every legacy -token as current-epoch would bless exactly that row, and it would stay usable -until some unrelated future bump. The migration cannot tell which legacy tokens -predate their user's last credential change, so it cannot be selective — the only -clean boundary is to invalidate all of them. - -The cost is one forced re-login for everyone at deploy. For a farm's worth of -users that is trivially cheap, and it is the single moment where the new -mechanism can be given a trustworthy starting state. - -This is the one data statement in the migration (`UPDATE refresh_tokens SET -"RevokedAt" = now() WHERE "RevokedAt" IS NULL`) — no INSERTs, so +One migration: three columns on `AspNetUsers` and one on `refresh_tokens`. + +**Epoch `0` is permanently retired.** It is the value a row receives when it is +written by something that does not know the column exists, and it must never +equal a live user's epoch. So: + +- `AspNetUsers.CredentialEpoch` — non-nullable, **default `1`**, and the C# + property initializes to `1`. Existing users are backfilled to `1`. +- `refresh_tokens.IssuedEpoch` — non-nullable, **default `0`**, with no C# + initializer worth trusting: every mint site stamps it explicitly from the + user's current epoch. The default exists only to catch writers that don't. + +Since epochs only ever increment from `1`, a token carrying `0` can never match +any user and is rejected inert, forever. + +That asymmetry is doing two jobs. + +**One: it makes the cutover actually bite.** Revoking legacy tokens is not +sufficient on its own. If users and legacy tokens both sat at `0`, then after the +victim logs in again their *new* token is also `0` — so a stolen legacy token +**passes** the epoch comparison, falls into the revoked-token replay branch, and +burns down the fresh family. The denial of service the check placement above +exists to prevent, reintroduced by the cutover itself. Users at `1` and legacy +tokens at `0` is what separates them. + +**Two: it survives a rolling deploy.** Production runs `migrate` as a separate +job *before* the new serving process (#263), so an old binary can still be +serving while the columns exist. It knows nothing about `IssuedEpoch`, so its +`INSERT`s take the database default `0` — which is exactly the value no user will +ever carry. Tokens minted in that window are inert the moment the new binary +takes over. Their holders get logged out; nobody gets a credential the new +mechanism cannot see. There is no window in which an old writer can mint +something the new binary honours. + +**The migration still revokes every existing refresh token** (`UPDATE +refresh_tokens SET "RevokedAt" = now() WHERE "RevokedAt" IS NULL`), even though +the epoch separation already makes them inert. This is deliberate +defence-in-depth, and the direction matters: if the epoch comparison ever +regresses, revoked-and-retired legacy tokens degrade to a denial of service, +whereas active-and-retired ones would degrade to *access*. Given a choice of +failure modes, take the one that locks people out rather than the one that lets +them in. + +The cost of the whole cutover is one forced re-login for everyone at deploy. For +a farm's worth of users that is trivially cheap, and it is the single moment +where the mechanism can be given a trustworthy starting state. + +That `UPDATE` is the migration's only data statement — no INSERTs, so `MigrationSecurityReviewTests`' shape assertions are unaffected, but its count pins need updating alongside. @@ -474,10 +506,17 @@ these bugs present: - **Genuine same-epoch reuse still burns the family.** The counterpart, so the fix above cannot be implemented by simply weakening #176's theft detection. -**Cutover** — a refresh token that was active before the migration is rejected -after it, and the user is forced to log in again. Asserted against a database -seeded pre-migration, since the whole point is a boundary the migration draws -across data it did not create. +**Cutover** — asserted against a database seeded pre-migration, since the whole +point is a boundary the migration draws across data it did not create: + +- A refresh token active before the migration is rejected after it, and the user + is forced to log in again. +- **After that user logs in again, the stolen legacy token still cannot burn + their new family.** The test that fails if legacy tokens and users share an + epoch — revocation alone passes the first assertion and fails this one. +- A row inserted with the column default `0` — standing in for an old binary + still serving during a rolling deploy — is rejected by the new binary, and + rejected *inert*. **Middleware ordering** — asserted explicitly, since the guarantee is positional and a silent reorder fails open. @@ -575,3 +614,25 @@ Recorded so the same reasoning is not re-derived later. ever loading the user*. When adding a guard, enumerate every entry point to the guarded state first, then place the guard above all of them — do not place it next to the code that prompted the guard. + +## What the fourth draft got wrong + +13. **The cutover revoked legacy tokens but left them at the same epoch as their + users.** Revocation is not differentiation. After the victim logs in again + their new token is also epoch `0`, so a stolen legacy token *passes* the + comparison, enters the replay branch, and burns the fresh family — the same + denial of service, reintroduced by the fix for the previous one. + + Users now start at `1` and `0` is permanently retired. That also closes a + rolling-deploy window the draft never considered: `migrate` runs before the + new serving process (#263), so an old binary can still be inserting refresh + tokens whose unknown column takes the database default — and `0` is precisely + the value no user will ever carry, so those rows are inert by construction + rather than by timing. + + Worth stating as its own rule, because it is a different mistake from items + 10–12: **a boundary needs a value, not just an event.** "Revoke everything at + the cutover" is an event, and an event does not survive a writer that is + still running or a row written after it. Reserving a sentinel the new world + can never produce is what makes the boundary hold without depending on when + anything happened. From 87dc267834310d61d3b0999db3b37f09b0b0724e Mon Sep 17 00:00:00 2001 From: mforce <> Date: Sat, 1 Aug 2026 19:35:28 -0700 Subject: [PATCH 08/13] spec: an absent epoch claim is a mismatch, not an exemption MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex round 5. The cutover covered refresh tokens and forgot the other credential already in flight: every access token minted before the deploy carries no epoch claim and stays cryptographically valid for its remaining ~15 minutes. A middleware that reads an absent claim as 'not applicable' lets the entire pre-cutover fleet authorize straight through the boundary the migration just drew. Absent or unparsable now parses to 0 — the sentinel retired last round — so it fails through the ordinary comparison with no special branch to forget. Recorded why this was easy to miss, because the pull is from the codebase's own conventions: must_change_password is OMITTED from the token when false, so 'claim absent => does not apply' is an idiom already in the reader's hand, and it is correct there. A convention that is right for a feature flag is a vulnerability for a revocation check. An omitted optional flag means no; an omitted mandatory one has to mean refuse. Nothing in the shape of the code distinguishes them. Tests: a PRE-migration access token making an ordinary authenticated request after cutover must 401, and a present-but-malformed claim is pinned separately — absent and unparsable are two code paths and only one of them is obvious. Also named the pattern across rounds: items 11, 13 and 14 are all migration-boundary failures in three different credential types. A design that introduces a per-request check has to enumerate every credential already in flight on the day it ships, not just the ones it mints afterwards. --- ...026-08-01-user-disable-and-email-design.md | 56 ++++++++++++++++++- 1 file changed, 53 insertions(+), 3 deletions(-) diff --git a/docs/superpowers/specs/2026-08-01-user-disable-and-email-design.md b/docs/superpowers/specs/2026-08-01-user-disable-and-email-design.md index d6b132e2f..e19331698 100644 --- a/docs/superpowers/specs/2026-08-01-user-disable-and-email-design.md +++ b/docs/superpowers/specs/2026-08-01-user-disable-and-email-design.md @@ -3,10 +3,10 @@ **Date:** 2026-08-01 **Phase:** 1.1 (epic #14) **Status:** design, awaiting implementation plan -**Revision:** fifth draft. Every round of review so far has found a real defect +**Revision:** sixth draft. Every round of review so far has found a real defect in the previous round's fix. The corrections are recorded at the end, in -*What the first/second/third/fourth draft got wrong* — read them before treating -any of this as a small delta. The corrections are the substance. +*What the first/second/third/fourth/fifth draft got wrong* — read them before +treating any of this as a small delta. The corrections are the substance. ## Problem @@ -411,6 +411,22 @@ It must: `MustChangePasswordMiddleware` does; - leave `auth/logout` reachable, so a dead session can clear its own cookie. +**An absent or unparsable epoch claim is a mismatch, not an exemption.** This is +the one place the middleware can fail open, and the codebase's own conventions +lead straight into it: `must_change_password` is *omitted* from the token when +false, so "this claim isn't here, therefore it doesn't apply" is the idiom a +reader already has in hand. Applied to the epoch it is a hole — every access +token minted before the deploy carries no epoch claim and stays cryptographically +valid for up to ~15 minutes, so treating absence as "not applicable" would let +the entire pre-cutover fleet keep authorizing requests straight through the +boundary the migration just drew. + +The rule that makes this uniform rather than a special case: **a missing or +malformed claim parses to `0`**, and `0` is the retired sentinel no user can ever +carry (see *Storage*). So it flows through the ordinary comparison and 401s, with +no separate branch to forget. Same treatment for a claim that is present but not +an integer. + It answers 401 with a distinct `Auth.CredentialsSuperseded` title, and disable additionally surfaces `Auth.AccountDisabled`, so the SPA can tell "you were disabled" from "your credentials were rotated." @@ -517,6 +533,14 @@ point is a boundary the migration draws across data it did not create: - A row inserted with the column default `0` — standing in for an old binary still serving during a rolling deploy — is rejected by the new binary, and rejected *inert*. +- **An access token minted before the cutover is refused after it.** Every + pre-deploy access token carries no epoch claim and stays cryptographically + valid for its remaining lifetime, so this is the case that catches a middleware + which treats an absent claim as "not applicable." Assert a *pre-migration* + token making an ordinary authenticated request post-cutover and getting 401. +- A token whose epoch claim is present but malformed (non-integer, empty) is + refused the same way. Pinned separately, because "absent" and "unparsable" + are two different code paths and only one of them is obvious. **Middleware ordering** — asserted explicitly, since the guarantee is positional and a silent reorder fails open. @@ -636,3 +660,29 @@ Recorded so the same reasoning is not re-derived later. still running or a row written after it. Reserving a sentinel the new world can never produce is what makes the boundary hold without depending on when anything happened. + +## What the fifth draft got wrong + +14. **The cutover covered refresh tokens and forgot access tokens.** Every access + token minted before the deploy carries no epoch claim and stays + cryptographically valid for its remaining ~15 minutes. If the middleware + reads an absent claim as "not applicable," the whole pre-cutover fleet + authorizes straight through the boundary the migration just drew. + + Absent or unparsable now parses to `0` — the retired sentinel — so it fails + through the ordinary comparison with no special branch. + + The reason this was easy to miss is worth recording: the codebase's own + convention pulls the other way. `must_change_password` is omitted from the + token when false, so "claim absent ⇒ doesn't apply" is an idiom already in + the reader's hand, and it is correct *there*. **A convention that is right + for a feature flag is a vulnerability for a revocation check.** An omitted + optional flag means "no", but an omitted mandatory one has to mean "refuse" — + and nothing in the shape of the code distinguishes them. + + This is also the fourth consecutive round where the defect lived in the gap + between "the new mechanism works" and "the old world is actually gone." + Items 11, 13, and 14 are all migration-boundary failures, in three different + credential types. A design that introduces a per-request check has to + enumerate **every credential already in flight** on the day it ships, not + just the ones it mints afterwards. From 35c701a14eeb9bab0af7e9aca87aac5190e88f8f Mon Sep 17 00:00:00 2001 From: mforce <> Date: Sat, 1 Aug 2026 19:43:59 -0700 Subject: [PATCH 09/13] =?UTF-8?q?spec:=20split=20the=20mechanism=20from=20?= =?UTF-8?q?the=20mutations=20=E2=80=94=20a=20new=20gate=20is=20a=20fleet?= =?UTF-8?q?=20change?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex round 6, and it is the inverse of round 4's finding: that one was about rows WRITTEN by an old binary, this is about requests SERVED by one. The retired epoch 0 makes an old writer's tokens inert. It does nothing about an old replica, which runs the pre-epoch pipeline and has no CredentialEpochMiddleware to consult. So during a rolling deploy an Owner disables a user on a new replica while the load balancer keeps routing that user's pre-deploy access token to an old one, which authorizes it. The suspension is immediate on part of the fleet — worse than a documented delay, because nothing surfaces it. Nothing the mutation does can fix that; the gap belongs to the replica that never learned to enforce. So the feature now ships in two deploys with the ordering as a hard constraint: A. the mechanism, inert — epoch columns, migration and cutover, the claim, the middleware, RefreshAsync's comparison, and the SPA 401 plumbing. Makes no new promise: the only epoch bumps are the password-reset paths that already revoke refresh tokens today, so an old replica behaves exactly as it does now and there is no window of false assurance. B. the mutations — disable, enable, email change, the SPA. Not exposed until every pre-A process is drained. The drain belongs to the deployment repo; what belongs here is the requirement, stated as one. An orchestrator that calls a rollout complete because the new replicas are healthy, without confirming the old ones are gone, does not satisfy it. Delivery is therefore three PRs, not two, and the split is not bookkeeping: (1) is where every migration-boundary correction lives and is the part that must be correct on a mixed fleet; #356 and #357 become ordinary feature work on a mechanism already everywhere. Named the general form: adding a per-request check is a fleet-wide change, not a code change. Until the last old process is gone the guarantee is whatever the weakest replica enforces, so the gate and the promise cannot ship together. And noted the asymmetry with the round-4 fix — a sentinel defends against bad data; only ordering defends against absent enforcement. --- ...026-08-01-user-disable-and-email-design.md | 93 +++++++++++++++++-- 1 file changed, 85 insertions(+), 8 deletions(-) diff --git a/docs/superpowers/specs/2026-08-01-user-disable-and-email-design.md b/docs/superpowers/specs/2026-08-01-user-disable-and-email-design.md index e19331698..628628b01 100644 --- a/docs/superpowers/specs/2026-08-01-user-disable-and-email-design.md +++ b/docs/superpowers/specs/2026-08-01-user-disable-and-email-design.md @@ -3,10 +3,10 @@ **Date:** 2026-08-01 **Phase:** 1.1 (epic #14) **Status:** design, awaiting implementation plan -**Revision:** sixth draft. Every round of review so far has found a real defect +**Revision:** seventh draft. Every round of review so far has found a real defect in the previous round's fix. The corrections are recorded at the end, in -*What the first/second/third/fourth/fifth draft got wrong* — read them before -treating any of this as a small delta. The corrections are the substance. +the *What the Nth draft got wrong* sections — read them before treating any of +this as a small delta. The corrections are the substance. ## Problem @@ -458,7 +458,45 @@ already dead and says nothing about the live one. Family revocation is preserved for its real purpose: genuine same-epoch reuse, which is still the theft signal #176 exists to catch. -### 5. SPA +### 4a. Rolling deploys: the check has to exist everywhere before the mutations do + +The retired-`0` sentinel makes rows *written* by an old binary inert. It does +nothing about requests *served* by one. + +An old replica runs the pre-epoch pipeline: it validates the bearer's signature, +issuer, audience, and lifetime, and has no `CredentialEpochMiddleware` to +consult. So during a rolling deploy — new replicas up, old ones still serving — +an Owner can disable a user on a new replica while the load balancer keeps +routing that user's pre-deploy access token to an old one, which authorizes it +happily. The suspension is not immediate; it is immediate *on some fraction of +the fleet*, which is worse than a documented delay because nothing surfaces it. + +This is not fixable by anything the mutation does. The enforcement gap belongs to +the replica that never learned to enforce. + +**So the feature ships in two deploys, and the ordering is a hard constraint:** + +1. **Deploy A — the mechanism, inert.** `CredentialEpoch`, `IssuedEpoch`, the + migration and cutover, `JwtTokenService` stamping the claim, the middleware, + and `RefreshAsync`'s comparison. No disable, no enable, no email change. + Nothing here makes a promise the mixed fleet cannot keep: the only epoch bumps + are the existing password-reset paths, which already revoke refresh tokens + today, so on an old replica the behaviour is exactly what it is now. Deploy A + can therefore roll at any pace without a window of false assurance. +2. **Deploy B — the mutations.** `POST /disable`, `POST /enable`, + `PUT /email`, and the SPA. By the time an epoch bump can be triggered by a + *new* operation, every serving replica enforces the check. + +Between them the old fleet must be fully drained. That drain is the deployment +repo's job, not this one's — what belongs here is the **requirement**: deploy B +must not be exposed until no process from before deploy A is still serving. An +orchestrator that reports "rollout complete" on the basis of new replicas being +healthy, without confirming old ones are gone, does not satisfy it. + +A single-replica install has none of this, and the reference compose stack is +single-replica. The design cannot lean on that — #338 already contemplates more +than one — and a constraint that only holds at one replica is the kind that gets +discovered during the first scale-up. Disabled users stay listed inline: a "Disabled" badge, a de-emphasized row, and the action toggling to "Enable". Hiding people who still own history invites @@ -559,13 +597,27 @@ to the login screen. ## Delivery -1. **#356 — disable / enable a user.** Carries the credential epoch, the account - lock, the middleware, the `recover-admin` changes, and the SPA plumbing for - the 401 reason. Larger than the first draft implied. -2. **#357 — change a user's email.** Reuses all of it; adds the four-column +Three PRs, not two, and the first two are **separate deploys** — see *Rolling +deploys* above for why the ordering is a correctness constraint rather than a +preference. + +1. **The credential epoch, inert** (new issue). `CredentialEpoch`, + `IssuedEpoch`, the migration and cutover, the claim, the middleware, + `RefreshAsync`'s comparison, and the SPA plumbing that carries a 401's reason + through auth teardown. Makes no new promise: the only epoch bumps are the + password-reset paths that already revoke refresh tokens today. +2. **#356 — disable / enable a user.** The mutations, the account lock, the + guards, the `recover-admin` changes, the SPA toggle and confirm. Must not be + exposed until every pre-(1) process is drained. +3. **#357 — change a user's email.** Reuses all of it; adds the four-column atomic write and the sole-Owner guard. Smaller than the first draft implied — the retry work it described does not exist. +The split is not bookkeeping. (1) is where every one of the migration-boundary +corrections lives, and it is the part that has to be correct on a mixed fleet; +(2) and (3) are ordinary feature work on top of a mechanism that is already +everywhere. + Role editing is **#355**. The #308 step-up-coverage gap is filed separately. ## What the first draft got wrong @@ -686,3 +738,28 @@ Recorded so the same reasoning is not re-derived later. credential types. A design that introduces a per-request check has to enumerate **every credential already in flight** on the day it ships, not just the ones it mints afterwards. + +## What the sixth draft got wrong + +15. **The rolling-deploy analysis covered rows written by an old binary and not + requests served by one.** The retired `0` makes an old writer's tokens inert; + it does nothing about an old *replica*, which runs the pre-epoch pipeline and + has no middleware to consult. Disable a user on a new replica, and the load + balancer can still route their pre-deploy access token to an old one that + authorizes it. The suspension is immediate on part of the fleet — worse than + a documented delay, because nothing surfaces it. + + The mechanism now ships as its own inert deploy first, and the mutations only + after the old fleet is drained. + + The general form, and the one to carry into any future work of this kind: + **adding a per-request check is a fleet-wide change, not a code change.** + Until the last old process is gone, the guarantee is whatever the *weakest* + replica enforces. Any feature whose promise depends on a new gate has to ship + behind that gate being universal — which means the gate and the promise + cannot be in the same deploy. + + Note the asymmetry with item 13. That one was solved by reserving a value the + old world could not produce. This one cannot be, because the old world is not + producing anything — it is *failing to check*. A sentinel defends against bad + data; only ordering defends against absent enforcement. From 1a937075ad80067010e5a74415e7864db595bcf9 Mon Sep 17 00:00:00 2001 From: mforce <> Date: Sat, 1 Aug 2026 19:44:14 -0700 Subject: [PATCH 10/13] spec: the intro still said two PRs --- .../specs/2026-08-01-user-disable-and-email-design.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/docs/superpowers/specs/2026-08-01-user-disable-and-email-design.md b/docs/superpowers/specs/2026-08-01-user-disable-and-email-design.md index 628628b01..c152d986b 100644 --- a/docs/superpowers/specs/2026-08-01-user-disable-and-email-design.md +++ b/docs/superpowers/specs/2026-08-01-user-disable-and-email-design.md @@ -21,7 +21,9 @@ be abandoned and a second one created, leaving a phantom user attached to whatever history the first one recorded. Both gaps live on the same screen and share the same machinery, so they are -designed together and shipped as two PRs. +designed together — and shipped as three PRs, because the machinery has to be +deployed and drained before either feature can honestly promise what it promises. +See *Delivery*. ## Scope From 5a042605734378d20647d0bfafac95e9ddafeeaf Mon Sep 17 00:00:00 2001 From: mforce <> Date: Sat, 1 Aug 2026 19:55:28 -0700 Subject: [PATCH 11/13] spec: make deploy A actually additive, and give rollback an order MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex round 7, two P1s, both answering questions I had asked it to check and both fatal to claims the previous draft made. Deploy A was called inert while its migration revoked every refresh token. Those are contradictory. The pre-epoch RefreshAsync reads a revoked token with no live replacement as a REPLAY and answers it by revoking the whole family — so a legacy row the migration marked revoked is, to an old replica, indistinguishable from a stolen one. The user re-logs in on a new replica, a forgotten tab refreshes against an old one, and the old replay branch burns down the epoch-1 family that was just issued. The deploy that promised to break nothing breaks the session of anyone who left a tab open. The migration is now additive only: four columns with defaults, no UPDATE, no INSERT. An old replica sees a database byte-identical in every column it reads. Nothing is lost, because the revocation was never what drew the boundary — the epoch separation is, and legacy tokens are already rejected inert on the mismatch. The defence-in-depth moves to a revoke-legacy-tokens verb run after the drain, when no reader can misinterpret it. Second: 'epoch 0 is inert forever' quietly assumed the new binary is still running. A rollback to the pre-A image drops the IssuedEpoch check entirely, resurrecting any default-0 token — including a child an old replica inserted right after a new replica's password reset revoked the family. The reset that was supposed to kill it silently did not. Rollback now has its own ordered procedure: drain deploy-A processes, revoke all active refresh tokens, then start the pre-A fleet. Two rules recorded. A compatibility window is a constraint on WRITES, not only on reads: it is not enough that the old binary tolerate the new schema, it must tolerate every value the new deploy puts into it. Adding a column is safe; changing a row an old code path already has opinions about is not, however harmless it looks in the new code. And: a security property that holds only while the new code is deployed is not a property, it is a configuration — every 'X can never happen' needs the rollback path checked before it is written down, because rollback is the one deploy nobody rehearses and everybody eventually performs. Tests: assert the migration mutates no rows (fails if anyone reintroduces the cutover revocation into it), and assert rollback fails closed. Marked item 11's remedy as superseded — its diagnosis was right, its fix was not. --- ...026-08-01-user-disable-and-email-design.md | 165 ++++++++++++++---- 1 file changed, 129 insertions(+), 36 deletions(-) diff --git a/docs/superpowers/specs/2026-08-01-user-disable-and-email-design.md b/docs/superpowers/specs/2026-08-01-user-disable-and-email-design.md index c152d986b..ade31f24d 100644 --- a/docs/superpowers/specs/2026-08-01-user-disable-and-email-design.md +++ b/docs/superpowers/specs/2026-08-01-user-disable-and-email-design.md @@ -3,7 +3,7 @@ **Date:** 2026-08-01 **Phase:** 1.1 (epic #14) **Status:** design, awaiting implementation plan -**Revision:** seventh draft. Every round of review so far has found a real defect +**Revision:** eighth draft. Every round of review so far has found a real defect in the previous round's fix. The corrections are recorded at the end, in the *What the Nth draft got wrong* sections — read them before treating any of this as a small delta. The corrections are the substance. @@ -306,22 +306,58 @@ takes over. Their holders get logged out; nobody gets a credential the new mechanism cannot see. There is no window in which an old writer can mint something the new binary honours. -**The migration still revokes every existing refresh token** (`UPDATE -refresh_tokens SET "RevokedAt" = now() WHERE "RevokedAt" IS NULL`), even though -the epoch separation already makes them inert. This is deliberate -defence-in-depth, and the direction matters: if the epoch comparison ever -regresses, revoked-and-retired legacy tokens degrade to a denial of service, -whereas active-and-retired ones would degrade to *access*. Given a choice of -failure modes, take the one that locks people out rather than the one that lets -them in. - -The cost of the whole cutover is one forced re-login for everyone at deploy. For -a farm's worth of users that is trivially cheap, and it is the single moment -where the mechanism can be given a trustworthy starting state. - -That `UPDATE` is the migration's only data statement — no INSERTs, so -`MigrationSecurityReviewTests`' shape assertions are unaffected, but its count -pins need updating alongside. +**The migration mutates no rows at all.** An earlier draft had it revoke every +existing refresh token as defence-in-depth. That is exactly the statement that +must not run while a pre-epoch replica is still serving, and the reason is worth +following carefully because it is what makes deploy A genuinely inert: + +The pre-epoch `RefreshAsync` treats a revoked token with no live replacement as a +**replay**, and answers it by calling `RevokeAllActiveForUserAsync`. So a legacy +token that the migration marked revoked is, to an old replica, indistinguishable +from a stolen one. The user re-logs in on a new replica and gets a clean epoch-1 +family; a forgotten tab then refreshes against an *old* replica with its +cutover-revoked token; the old replay branch fires and burns down the family that +was just issued. The old tab redirects to login tidily, having destroyed the +user's live session on the way out. + +That is a user-visible break caused by the deploy that claimed to cause none. + +So the migration is **additive only**: four columns with defaults, no `UPDATE`, +no `INSERT`. An old replica sees a database byte-identical in every column it +reads, and behaves exactly as it does today. A new replica gets full enforcement +immediately. There is no state in between. + +Nothing is lost, because **the revocation was never what drew the boundary** — +the epoch separation is. Legacy tokens carry `IssuedEpoch = 0`, users carry `1`, +and the new binary rejects the mismatch inert. Revoking them was belt-and-braces +on top of a mechanism that already held. + +The belt-and-braces still has a home, just a later one. Once the pre-A fleet is +drained, a one-off `revoke-legacy-tokens` CLI verb (same run-then-exit shape as +`migrate`) performs the `UPDATE`. At that point no reader can misinterpret it, +and the direction of the defence is the one worth having: if the epoch comparison +ever regresses, revoked-and-retired tokens degrade to a denial of service, +whereas active-and-retired ones would degrade to *access*. + +`MigrationSecurityReviewTests` is unaffected either way — no INSERTs, and now no +data statements at all. + +#### Rollback fails open unless it is done deliberately + +"Epoch `0` is inert forever" holds only while a binary that reads `IssuedEpoch` +is running. Roll deploy A back and the pre-A binary ignores the column entirely. + +The concrete hazard: during the mixed-fleet window an old replica can insert an +active default-`0` child immediately after a new replica's password reset revoked +the family — the same refresh race described above. The new binary rejects that +child on sight. **A rollback resurrects it**, and the password reset that was +supposed to kill it silently did not. + +So rollback is not "redeploy the previous image." The order is: drain every +deploy-A process, **revoke all active refresh tokens**, then start the pre-A +fleet. That is one forced re-login, and it is what makes the rollback fail +closed. It belongs in the runbook, not in an operator's memory — the deployment +repo owns the procedure, this repo owns the requirement. ### 2. Application layer @@ -478,22 +514,29 @@ the replica that never learned to enforce. **So the feature ships in two deploys, and the ordering is a hard constraint:** -1. **Deploy A — the mechanism, inert.** `CredentialEpoch`, `IssuedEpoch`, the - migration and cutover, `JwtTokenService` stamping the claim, the middleware, - and `RefreshAsync`'s comparison. No disable, no enable, no email change. - Nothing here makes a promise the mixed fleet cannot keep: the only epoch bumps - are the existing password-reset paths, which already revoke refresh tokens - today, so on an old replica the behaviour is exactly what it is now. Deploy A - can therefore roll at any pace without a window of false assurance. -2. **Deploy B — the mutations.** `POST /disable`, `POST /enable`, - `PUT /email`, and the SPA. By the time an epoch bump can be triggered by a - *new* operation, every serving replica enforces the check. - -Between them the old fleet must be fully drained. That drain is the deployment -repo's job, not this one's — what belongs here is the **requirement**: deploy B -must not be exposed until no process from before deploy A is still serving. An -orchestrator that reports "rollout complete" on the basis of new replicas being -healthy, without confirming old ones are gone, does not satisfy it. +1. **Deploy A — the mechanism, additive.** `CredentialEpoch`, `IssuedEpoch`, an + **additive-only** migration, `JwtTokenService` stamping the claim, the + middleware, and `RefreshAsync`'s comparison. No disable, no enable, no email + change, and — critically — **no row mutations**: an old replica sees a + database byte-identical in every column it reads and behaves exactly as it + does today, while a new replica enforces fully. See *Storage* for why the + cutover revocation cannot live here. +2. **Drain.** Every pre-A process gone. +3. **Deploy B — the mutations.** `POST /disable`, `POST /enable`, `PUT /email`, + and the SPA. By the time an epoch bump can be triggered by a *new* operation, + every serving replica enforces the check. The optional + `revoke-legacy-tokens` verb can also run from here on. + +The drain is the deployment repo's job, not this one's — what belongs here is the +**requirement**: deploy B must not be exposed until no process from before deploy +A is still serving. An orchestrator that reports "rollout complete" on the basis +of new replicas being healthy, without confirming old ones are gone, does not +satisfy it. + +**Rollback has its own order** — drain deploy-A processes, revoke all active +refresh tokens, *then* start the pre-A fleet — because a pre-A binary ignores +`IssuedEpoch` and would resurrect any default-`0` token the epoch check had been +suppressing. Also in *Storage*. A single-replica install has none of this, and the reference compose stack is single-replica. The design cannot lean on that — #338 already contemplates more @@ -565,11 +608,18 @@ these bugs present: **Cutover** — asserted against a database seeded pre-migration, since the whole point is a boundary the migration draws across data it did not create: -- A refresh token active before the migration is rejected after it, and the user - is forced to log in again. +- A refresh token active before the migration is rejected by the new binary + after it — on the epoch mismatch alone, with the row still `RevokedAt IS NULL`. - **After that user logs in again, the stolen legacy token still cannot burn their new family.** The test that fails if legacy tokens and users share an - epoch — revocation alone passes the first assertion and fails this one. + epoch. +- **The migration mutates no rows.** Asserted directly: every + `refresh_tokens.RevokedAt` is unchanged across the migration. This is the test + that fails if someone "helpfully" reintroduces the cutover revocation into the + migration, which would break old replicas mid-rollout. +- **Rollback fails closed.** Run the migration, mint a default-`0` child, then + simulate the pre-A binary by ignoring `IssuedEpoch` — the child must be + unusable, which is only true if the rollback procedure revoked it first. - A row inserted with the column default `0` — standing in for an old binary still serving during a rolling deploy — is rejected by the new binary, and rejected *inert*. @@ -676,6 +726,10 @@ Recorded so the same reasoning is not re-derived later. which legacy tokens predate their user's last credential change, so the only clean boundary is to revoke all of them and accept one forced re-login. + *Superseded in part by items 13 and 16: the boundary is drawn by the epoch + separation, not by the revocation, and the revocation cannot run in the + migration at all. The diagnosis here was right; the remedy was not.* + 12. **The epoch comparison was placed "before the rotation," which is too late.** A revoked token reaches #176's grace/replay branch — and `RevokeAllActiveForUserAsync` — before `FindByIdAsync` loads the user, so the @@ -765,3 +819,42 @@ Recorded so the same reasoning is not re-derived later. old world could not produce. This one cannot be, because the old world is not producing anything — it is *failing to check*. A sentinel defends against bad data; only ordering defends against absent enforcement. + +## What the seventh draft got wrong + +16. **Deploy A was called inert while its migration revoked every refresh + token.** Those are contradictory. The pre-epoch `RefreshAsync` reads a + revoked token with no live replacement as a *replay* and answers it by + revoking the whole family — so a legacy row the migration marked revoked is, + to an old replica, indistinguishable from a stolen one. User re-logs in on a + new replica, a forgotten tab refreshes against an old one, and the old replay + branch burns down the epoch-1 family that was just issued. The deploy that + promised to break nothing breaks the session of anyone who left a tab open. + + The migration is now **additive only**. Nothing is lost, because the + revocation was never what drew the boundary — the epoch separation is, and + legacy tokens are already rejected inert on the mismatch. The + defence-in-depth moves to a `revoke-legacy-tokens` verb run after the drain, + when no reader can misinterpret it. + + The lesson is narrower than "drain first" and more useful: **a compatibility + window is a constraint on writes, not only on reads.** It is not enough that + the old binary tolerate the new *schema*; it must also tolerate every *value* + the new deploy puts into it. Adding a column is safe. Changing a row an old + code path already has opinions about is not, however harmless the change + looks in the new code. + +17. **"Epoch `0` is inert forever" quietly assumed the new binary is still + running.** A rollback to the pre-A image drops the `IssuedEpoch` check + entirely, resurrecting any default-`0` token — including a child an old + replica inserted right after a new replica's password reset revoked the + family. The reset that was supposed to kill it silently did not. + + Rollback therefore has its own ordered procedure: drain deploy-A processes, + revoke all active refresh tokens, then start the pre-A fleet. + + Generally: **a security property that holds only while the new code is + deployed is not a property, it is a configuration.** Every claim of the form + "X can never happen" needs the rollback path checked before it is written + down, because rollback is the one deploy nobody rehearses and everybody + eventually performs. From ea9820ee2356ccd8b9f5eb81fe6586d306053e48 Mon Sep 17 00:00:00 2001 From: mforce <> Date: Sat, 1 Aug 2026 20:05:15 -0700 Subject: [PATCH 12/13] =?UTF-8?q?spec:=20readers=20ship=20in=20A,=20not=20?= =?UTF-8?q?B=20=E2=80=94=20plus=20lock=20timeout=20and=20verb=20repeat-saf?= =?UTF-8?q?ety?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex round 8. One design finding, three operational. The design one answers a question I had asked it: is deploy B strictly additive over A? No. The disabled-state checks in LoginAsync, RefreshAsync and StepUpGrantService.IssueAsync were scheduled with the mutations in B, so during B's own gradual rollout a disabled user's login or refresh landing on an A-only replica mints a CURRENT-epoch token that every B replica then honours — the suspension bypassed by a credential issued after it. All disabled-state readers move to A, with DisabledAt shipping unused. That is the general rule the last several rounds kept circling from different directions, and it is worth stating once: readers ship before writers, always. A reader deployed early is inert; a writer deployed early is a hole. Applied one deploy later, the same logic means B's API must be staged fleet-wide before the SPA is exposed, or a browser served the new UI by a B replica gets 404s from an A replica without the route. Operational, now specified rather than waved at: - The additive migration still takes ACCESS EXCLUSIVE per ADD COLUMN. A long transaction on refresh_tokens from a still-serving old process makes the migrate job wait, and its queued exclusive request blocks login and refresh behind it. DatabaseReadyHealthCheck gates the replacement process, not the old fleet taking traffic. Short lock_timeout, fail and retry: a failed migrate job is a deploy that did not start; an unbounded lock wait is a deploy that took auth down. - revoke-legacy-tokens gets a pinned predicate and repeat-safety, so a retry does not rewrite RevokedAt history or regenerate table-wide WAL. Operators retry one-off verbs — seed and bootstrap-admin are idempotent for the same reason. Also added a closing section on where this stops. Rounds 1-4 found mechanism defects, 5-8 found deploy-boundary defects, and round 8 is the first where most findings are hygiene rather than anything that would make the shipped feature wrong. The core design has not been challenged since round 3. What remains open is deployment procedure, which belongs in the deployment/ops repo — this spec states requirements and should not grow into a runbook. --- ...026-08-01-user-disable-and-email-design.md | 117 +++++++++++++++--- 1 file changed, 102 insertions(+), 15 deletions(-) diff --git a/docs/superpowers/specs/2026-08-01-user-disable-and-email-design.md b/docs/superpowers/specs/2026-08-01-user-disable-and-email-design.md index ade31f24d..a3b5086ab 100644 --- a/docs/superpowers/specs/2026-08-01-user-disable-and-email-design.md +++ b/docs/superpowers/specs/2026-08-01-user-disable-and-email-design.md @@ -3,10 +3,10 @@ **Date:** 2026-08-01 **Phase:** 1.1 (epic #14) **Status:** design, awaiting implementation plan -**Revision:** eighth draft. Every round of review so far has found a real defect -in the previous round's fix. The corrections are recorded at the end, in -the *What the Nth draft got wrong* sections — read them before treating any of -this as a small delta. The corrections are the substance. +**Revision:** ninth draft, and the last unless something material appears. Every +round has found a real defect in the previous one's fix; the corrections are +recorded at the end in the *What the Nth draft got wrong* sections, and are the +substance. See *Where this stops being about the design* for why it ends here. ## Problem @@ -327,6 +327,20 @@ no `INSERT`. An old replica sees a database byte-identical in every column it reads, and behaves exactly as it does today. A new replica gets full enforcement immediately. There is no state in between. +**Additive is not the same as free.** Constant defaults avoid a heap rewrite on +the Postgres version this targets, but each `ALTER TABLE … ADD COLUMN` still +takes an `ACCESS EXCLUSIVE` lock. If a still-serving old process holds a long +transaction touching `refresh_tokens`, the migrate job waits — and its *queued* +exclusive request blocks every login and refresh behind it. `DatabaseReadyHealthCheck` +does not help: it gates the replacement process, not the old fleet still taking +traffic. A metadata-only migration would become an authentication outage. + +So the migration sets a short **`lock_timeout`** and fails fast rather than +queueing, leaving the operator to retry. A failed migrate job is a deploy that +did not start; an unbounded lock wait is a deploy that took auth down. `refresh_tokens` +is the table that grows (#259, #270 exist because of it), so this is the one +most likely to be busy. + Nothing is lost, because **the revocation was never what drew the boundary** — the epoch separation is. Legacy tokens carry `IssuedEpoch = 0`, users carry `1`, and the new binary rejects the mismatch inert. Revoking them was belt-and-braces @@ -339,6 +353,14 @@ and the direction of the defence is the one worth having: if the epoch compariso ever regresses, revoked-and-retired tokens degrade to a denial of service, whereas active-and-retired ones would degrade to *access*. +Its predicate is exactly `IssuedEpoch = 0 AND RevokedAt IS NULL`, and it is +**repeat-safe**: a second run updates zero rows, preserving the timestamps the +first one wrote. An implementation that stamps every epoch-`0` row on each +invocation would rewrite history and regenerate table-wide WAL every time an +operator retried — and operators retry one-off verbs, which is why `seed` and +`bootstrap-admin` are both idempotent. Pinned by a test asserting the second run +changes nothing. + `MigrationSecurityReviewTests` is unaffected either way — no INSERTs, and now no data statements at all. @@ -514,18 +536,41 @@ the replica that never learned to enforce. **So the feature ships in two deploys, and the ordering is a hard constraint:** -1. **Deploy A — the mechanism, additive.** `CredentialEpoch`, `IssuedEpoch`, an - **additive-only** migration, `JwtTokenService` stamping the claim, the - middleware, and `RefreshAsync`'s comparison. No disable, no enable, no email - change, and — critically — **no row mutations**: an old replica sees a - database byte-identical in every column it reads and behaves exactly as it - does today, while a new replica enforces fully. See *Storage* for why the - cutover revocation cannot live here. +The split is **every reader in A, only mutations and UI in B**. That is a +stronger rule than "the mechanism in A", and the difference is load-bearing: + +1. **Deploy A — the mechanism and every reader, additive.** `CredentialEpoch`, + `IssuedEpoch`, an **additive-only** migration, `JwtTokenService` stamping the + claim, `CredentialEpochMiddleware`, `RefreshAsync`'s epoch comparison — **and + every disabled-state check**: `LoginAsync`, `RefreshAsync`, and + `StepUpGrantService.IssueAsync` all refuse a disabled user as of A. The + `DisabledAt` column ships here too, unused by any mutation. + + Putting the disabled *readers* in B would leave a hole during B's own gradual + rollout: a disabled user's login or refresh landing on an A-only replica would + mint a **current-epoch** token, which every B replica then honours — the + suspension bypassed by a credential minted after it. Readers must precede the + writers that give them something to read. + + **No row mutations** — an old replica sees a database byte-identical in every + column it reads and behaves exactly as it does today, while a new replica + enforces fully. See *Storage* for why the cutover revocation cannot live here. + 2. **Drain.** Every pre-A process gone. -3. **Deploy B — the mutations.** `POST /disable`, `POST /enable`, `PUT /email`, - and the SPA. By the time an epoch bump can be triggered by a *new* operation, - every serving replica enforces the check. The optional - `revoke-legacy-tokens` verb can also run from here on. + +3. **Deploy B — the mutations and the UI.** `POST /disable`, `POST /enable`, + `PUT /email`, and the SPA. By the time an epoch bump can be triggered by a + *new* operation, or a user can be disabled at all, every serving replica both + enforces the check and reads the flag. + + **The API must be staged across the whole fleet before the SPA is exposed.** + B rolls gradually too, so a browser that loads the new SPA from a B replica + can have `/disable` routed to an A replica that has no such route — + intermittent 404s on the new administrative actions. Either stage the API + first and release the SPA last, or drain A→B before clients receive the new + UI. Same class of problem as the A→B boundary, one deploy later. + + The optional `revoke-legacy-tokens` verb can run from here on. The drain is the deployment repo's job, not this one's — what belongs here is the **requirement**: deploy B must not be exposed until no process from before deploy @@ -858,3 +903,45 @@ Recorded so the same reasoning is not re-derived later. "X can never happen" needs the rollback path checked before it is written down, because rollback is the one deploy nobody rehearses and everybody eventually performs. + +## What the eighth draft got wrong + +18. **The disabled-state readers were left in deploy B.** During B's own gradual + rollout a disabled user's login or refresh can land on an A-only replica, + which mints a **current-epoch** token that every B replica then honours — the + suspension bypassed by a credential issued after it. The split is therefore + *every reader in A, only mutations and UI in B*: `LoginAsync`, + `RefreshAsync`, and `StepUpGrantService.IssueAsync` all refuse a disabled + user as of A, with `DisabledAt` shipping unused. + + Stated generally, and this is the last variant of the rule the previous + rounds kept circling: **readers ship before writers, always.** A reader + deployed early is inert; a writer deployed early is a hole. The same logic + applies one deploy later to B itself — the API must be staged fleet-wide + before the SPA is exposed, or a browser served the new UI by a B replica gets + 404s from an A replica that has no such route. + +19. **Operational hygiene the design waved at rather than specified:** the + additive migration still takes an `ACCESS EXCLUSIVE` lock and needs a + `lock_timeout`, so a busy `refresh_tokens` cannot turn a metadata-only change + into an authentication outage; and `revoke-legacy-tokens` needs a pinned + predicate (`IssuedEpoch = 0 AND RevokedAt IS NULL`) plus repeat-safety, + because operators retry one-off verbs. + +## Where this stops being about the design + +Rounds 1–4 found defects in the mechanism: it did not do what it claimed. +Rounds 5–8 found defects in the **deploy boundary**: the mechanism was right, but +the old world had not gone away. Round 8 is the first where most findings are +ordinary operational hygiene — a lock timeout, a verb's predicate, a rollout +order — rather than anything that would make the shipped feature wrong. + +The core design has not been challenged since round 3. The epoch, its binding to +both credential types, the check's position ahead of the grace branch, the +account-row lock, the four-column email write, and the guards have all survived +untouched. That is the signal to stop iterating this document. + +What remains genuinely open is **deployment procedure** — drains, staging order, +lock timeouts, rollback steps. Per this repo's host-agnostic boundary that +belongs in the deployment/ops repo as procedure, not here as prose. This spec +states the requirements; it should not grow into a runbook. From d88b151e5ba3d6840fc6f88b08b43111b7550ad5 Mon Sep 17 00:00:00 2001 From: mforce <> Date: Sat, 1 Aug 2026 20:12:32 -0700 Subject: [PATCH 13/13] spec: scope replay revocation to the checked epoch; fix the delivery checklist MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex round 9, after I had called the design frozen. That call was premature and one of these is a genuine race, so the freeze is retracted rather than quietly amended. Moving the epoch comparison ahead of the grace branch fixed the case where the check never ran. It did not fix the case where the check runs and PASSES and the world moves underneath it: a request reads IssuedEpoch = E against CredentialEpoch = E, passes, and before it reaches RevokeAllActiveForUserAsync a disable/email/password change commits E+1 and a fresh login mints an E+1 family — which the stale request then burns. The same denial of service as before, entered by passing the check rather than by skipping it. Replay revocation is now predicated on IssuedEpoch = stored.IssuedEpoch rather than on UserId alone. That is the semantically correct answer independent of the race: theft detection should burn the compromised family, not one issued after the compromise was already answered by a rotation. The test has to be barrier-controlled — pause after the comparison succeeds, bump and re-login, then release — because the sequential version passes with the defect fully present. The rule, since this is a different mistake from the placement one: a check and the action it authorizes must be scoped to the same thing. Moving a guard earlier makes it run; it does not make what follows obey it. If the guard reads one row and the action writes a set, the set has to be narrowed to what the guard actually vouched for. Second finding: the Delivery checklist still listed deploy A as only the epoch fields, middleware and refresh comparison, contradicting the rolling-deploy section that had just moved DisabledAt and all three disabled-state readers into it. An implementer scoping from the checklist would have rebuilt the exact hole. A document that states a constraint in one section and contradicts it in the section people work from has not fixed anything. --- ...026-08-01-user-disable-and-email-design.md | 101 ++++++++++++++---- 1 file changed, 81 insertions(+), 20 deletions(-) diff --git a/docs/superpowers/specs/2026-08-01-user-disable-and-email-design.md b/docs/superpowers/specs/2026-08-01-user-disable-and-email-design.md index a3b5086ab..a6f75c39a 100644 --- a/docs/superpowers/specs/2026-08-01-user-disable-and-email-design.md +++ b/docs/superpowers/specs/2026-08-01-user-disable-and-email-design.md @@ -3,10 +3,10 @@ **Date:** 2026-08-01 **Phase:** 1.1 (epic #14) **Status:** design, awaiting implementation plan -**Revision:** ninth draft, and the last unless something material appears. Every -round has found a real defect in the previous one's fix; the corrections are -recorded at the end in the *What the Nth draft got wrong* sections, and are the -substance. See *Where this stops being about the design* for why it ends here. +**Revision:** tenth draft. Every round of review has found a real defect in the +previous one's fix — including after this document was declared frozen once. +The corrections are recorded at the end in the *What the Nth draft got wrong* +sections, and are the substance. ## Problem @@ -518,6 +518,20 @@ already dead and says nothing about the live one. Family revocation is preserved for its real purpose: genuine same-epoch reuse, which is still the theft signal #176 exists to catch. +**And the revocation itself must be scoped to the epoch that was checked.** +Passing the comparison is not enough, because the comparison is a point-in-time +read and `RevokeAllActiveForUserAsync` is user-wide. A request can read +`IssuedEpoch = E` against `CredentialEpoch = E`, pass — and then, before it +reaches the revocation, a disable/email/password change commits `E+1` and the +user logs in fresh at `E+1`. The stale request then burns a family minted after +the credentials it was checked against were rotated. Same denial of service as +before, now entered by *passing* the check rather than skipping it. + +The replay revocation is therefore predicated on `IssuedEpoch = stored.IssuedEpoch`, +not on `UserId` alone. That is also the semantically correct thing independent of +the race: theft detection should burn the compromised family, not one issued +after the compromise was already answered by a rotation. + ### 4a. Rolling deploys: the check has to exist everywhere before the mutations do The retired-`0` sentinel makes rows *written* by an old binary inert. It does @@ -647,6 +661,12 @@ these bugs present: anywhere after the grace/replay branch — and it fails as a denial of service, not as an access grant, so it will not show up in any "can the attacker get in?" test. +- **The same, but racing** — and this one has to be barrier-controlled, because + the sequential version above passes with the defect present. Present a revoked + epoch-`E` token, pause the request *after* the epoch comparison succeeds, bump + the epoch and log the user in fresh at `E+1`, then release. The `E+1` family + must survive. This is what fails if the replay revocation is scoped to + `UserId` rather than to the epoch that was actually checked. - **Genuine same-epoch reuse still burns the family.** The counterpart, so the fix above cannot be implemented by simply weakening #176's theft detection. @@ -698,11 +718,21 @@ Three PRs, not two, and the first two are **separate deploys** — see *Rolling deploys* above for why the ordering is a correctness constraint rather than a preference. -1. **The credential epoch, inert** (new issue). `CredentialEpoch`, - `IssuedEpoch`, the migration and cutover, the claim, the middleware, - `RefreshAsync`'s comparison, and the SPA plumbing that carries a 401's reason - through auth teardown. Makes no new promise: the only epoch bumps are the - password-reset paths that already revoke refresh tokens today. +1. **The credential epoch and every reader** (#364). `CredentialEpoch`, + `IssuedEpoch`, **`DisabledAt` / `DisabledBy`**, the additive migration, the + claim, the middleware, `RefreshAsync`'s epoch comparison and epoch-scoped + replay revocation, and the SPA plumbing that carries a 401's reason through + auth teardown. + + **Including all three disabled-state readers** — `LoginAsync`, `RefreshAsync`, + and `StepUpGrantService.IssueAsync` refuse a disabled user as of this deploy, + with the column written by nothing yet. This is not optional detail: if the + readers slip to (2), a replica part-way through (2)'s rollout mints a + current-epoch credential for a user another replica just disabled, and every + updated replica honours it. See *Rolling deploys*. + + Makes no new promise: the only epoch bumps are the password-reset paths that + already revoke refresh tokens today, and nothing can set `DisabledAt` yet. 2. **#356 — disable / enable a user.** The mutations, the account lock, the guards, the `recover-admin` changes, the SPA toggle and confirm. Must not be exposed until every pre-(1) process is drained. @@ -928,20 +958,51 @@ Recorded so the same reasoning is not re-derived later. predicate (`IssuedEpoch = 0 AND RevokedAt IS NULL`) plus repeat-safety, because operators retry one-off verbs. +## What the ninth draft got wrong + +20. **The replay revocation was scoped to the user, not to the epoch that was + checked.** Placing the epoch comparison ahead of the grace branch (item 12) + fixed the case where the check never ran. It did not fix the case where the + check *runs and passes*, and the world moves underneath it: read + `IssuedEpoch = E` against `CredentialEpoch = E`, pass, and before the request + reaches `RevokeAllActiveForUserAsync` a bump commits `E+1` and a fresh login + mints an `E+1` family — which the stale request then burns. The same denial + of service, entered by passing the check rather than skipping it. + + Revocation is now predicated on `IssuedEpoch = stored.IssuedEpoch`, which is + also the semantically right answer regardless of the race: theft detection + should burn the compromised family, not one issued after the compromise was + already answered. + + The rule: **a check and the action it authorizes must be scoped to the same + thing.** Moving a guard earlier makes it *run*; it does not make what follows + obey it. If the guard reads one row and the action writes a set, the set has + to be narrowed to what the guard actually vouched for. + +21. **The Delivery checklist contradicted the rollout requirement.** Item 18 + moved the disabled-state readers into deploy A in the *Rolling deploys* + section, but the canonical delivery breakdown still listed only the epoch + fields, middleware, and refresh comparison — so an implementer scoping work + from the checklist would rebuild exactly the hole item 18 closed. A document + that states a constraint in one section and contradicts it in the section + people actually work from has not fixed anything. + ## Where this stops being about the design Rounds 1–4 found defects in the mechanism: it did not do what it claimed. Rounds 5–8 found defects in the **deploy boundary**: the mechanism was right, but -the old world had not gone away. Round 8 is the first where most findings are -ordinary operational hygiene — a lock timeout, a verb's predicate, a rollout -order — rather than anything that would make the shipped feature wrong. - -The core design has not been challenged since round 3. The epoch, its binding to -both credential types, the check's position ahead of the grace branch, the -account-row lock, the four-column email write, and the guards have all survived -untouched. That is the signal to stop iterating this document. - -What remains genuinely open is **deployment procedure** — drains, staging order, -lock timeouts, rollback steps. Per this repo's host-agnostic boundary that +the old world had not gone away. Round 9 found one more of each — a real race in +the mechanism (item 20) and an internal contradiction (item 21) — after this +document had already been declared frozen once. That call was premature, and the +correction is recorded rather than quietly amended. + +What has genuinely stabilized is the *shape*: the epoch, its binding to both +credential types, the account-row lock, the four-column email write, and the +guards have survived untouched since round 3. What keeps producing findings is +the seam between the mechanism and everything around it — old replicas, later +requests, other sections of this document. + +What remains open beyond that is **deployment procedure** — drains, staging +order, lock timeouts, rollback steps. Per this repo's host-agnostic boundary that belongs in the deployment/ops repo as procedure, not here as prose. This spec states the requirements; it should not grow into a runbook.