Repository navigation
spec: design user disable/enable and email change (#356, #357) - #358
Conversation
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.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d8ea51ac74
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| **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. |
There was a problem hiding this comment.
When two active Owners disable each other concurrently, both transactions can observe two active Owners and then update different AspNetUsers rows. ConcurrencyStamp only detects competing updates to the same user row, so neither update conflicts and both can commit, leaving zero active Owners. The parallel test proposed here will expose the race, but the design also needs account-scoped serialization—such as an advisory lock or another database-enforced invariant—around the count and disable operation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c25fcf2 (before this round landed). The spec now takes the account row FOR UPDATE via the existing AccountRepository.GetCurrentLockedAsync, and re-reads the target, its Owner membership, and the active-Owner count inside the lock before writing. Recorded as a standing constraint on every future Owner-removing path, including the #355 role-demotion slice.
| 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. |
There was a problem hiding this comment.
In the current JwtTokenService and authentication pipeline, access tokens neither contain SecurityStamp nor trigger a stamp lookup, while the proposed middleware checks only DisabledAt. Consequently, an email change leaves every existing bearer token usable until expiry, and a token issued before a disable becomes usable again if the account is re-enabled within that window. Rotating the stamp and revoking refresh tokens therefore does not provide the claimed session re-establishment; the design needs a per-request stamp/generation comparison or must explicitly weaken and test that guarantee.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c25fcf2 (before this round landed), and this finding is what drove the rewrite. Confirmed against source: the token carries sub/email/account_id/role/must_change_password only, and validation checks signature, issuer, audience, lifetime — no stamp anywhere on the request path. Enforcement is now a monotonic CredentialEpoch claim compared per request; enable deliberately does not restore the old value. Round 2 then found the same fix incomplete on the refresh side — see the IssuedEpoch thread.
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.
|
@codex please review the revised spec (commit The first draft was reviewed adversarially and reworked. The central change: enforcement moved from a Also corrected: the last-Owner guard now takes the account row Worth attacking specifically:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c25fcf21ab
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| One migration: three columns, `CredentialEpoch` non-nullable with default `0`, | ||
| no data statements. `MigrationSecurityReviewTests` stays green. |
There was a problem hiding this comment.
In the barrier-controlled refresh-vs-disable/email race, the storage design adds CredentialEpoch only to ApplicationUser, so the durable refresh-token record has no issuance epoch to compare with the user's current value. If a refresh loads the old user state, the epoch bump and bulk revocation commit, and then that refresh inserts its child, the child's access token is rejected, but presenting the child again lets RefreshAsync read the new user epoch and mint a valid current-epoch pair—immediately after an email change or after re-enabling a disabled user. Persist or cryptographically bind the issuance epoch to every refresh token and reject a mismatch before rotation; otherwise the claimed race closure and session termination do not hold.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 87f922d.
Verified the mechanism before changing anything: RefreshToken has no epoch field, and RevokeAllActiveForUserAsync is a bulk ExecuteUpdate over RevokedAt == null — it cannot see a row that does not exist yet, so a child inserted after it commits stays Active. Presenting that child makes RefreshAsync read the user's current CredentialEpoch and mint a valid current-epoch pair: immediately after an email change, and after re-enable for a disable.
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 the guarantee no longer rests on it. Default 0 backfills correctly in both directions, so the deploy evicts no live session.
The race tests were also restated: assert by presenting the child token, and re-present it after enable. The weaker assertion — that the child's access token is rejected — passes with the defect fully present, which is how the previous draft talked itself into being finished.
Recorded in the spec as a general rule: a revocation epoch has to be bound to every credential the system will later accept, not just the one whose weakness prompted it. The previous rewrite was specifically about the access token having no binding, and left the credential whose entire job is to outlive it with none.
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.
|
@codex all three findings are addressed in The round-2 finding was the real one: binding Please re-review
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 87f922dfad
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| 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 |
There was a problem hiding this comment.
If any password reset raced RefreshAsync before this migration, the refresh can already have inserted an attacker-held child after the reset's bulk revocation. Backfilling both that token and its user to epoch 0 certifies the stale child as current, so it remains usable after deployment until another epoch bump. Because the migration cannot infer which legacy tokens predate the user's latest credential change, the cutover must establish a clean boundary by revoking existing refresh tokens or assigning them an epoch that differs from the users' initial epoch.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in bd864c7.
You are right that the framing was backwards: the previous draft called "no live session is evicted" a virtue, and it is the defect. The pre-epoch code already carries the revoke-vs-refresh race, so a legacy row sitting RevokedAt IS NULL may already be an attacker's orphan from an earlier password reset — and backfilling every user and token to 0 certifies exactly that row as current, where it stays usable until some unrelated future bump.
Agreed too that the migration cannot be selective: it has no way to tell which legacy tokens predate their user's last credential change. So the cutover now revokes every existing refresh token (UPDATE refresh_tokens SET "RevokedAt" = now() WHERE "RevokedAt" IS NULL) and accepts one forced re-login at deploy. For a farm's worth of users that costs nothing, and it is the only moment the mechanism can be handed a trustworthy starting state.
Added a cutover test asserted against a database seeded pre-migration, since the guarantee is a boundary drawn across data the migration did not create. Also noted that this is the migration's only data statement — no INSERTs, so MigrationSecurityReviewTests' shape assertions are unaffected, but its count pins need updating with it.
| `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 |
There was a problem hiding this comment.
The check must occur before revoked-token grace/replay handling, not merely before rotation. In the current IdentityProvider.RefreshAsync flow (src/Cluckwork.Infrastructure/Identity/IdentityProvider.cs:102-120), a revoked pre-bump token whose replacement is no longer active reaches RevokeAllActiveForUserAsync before the user is loaded; after the legitimate user logs in again, an attacker can therefore present that superseded token and revoke the new current-epoch refresh token. Reject an epoch mismatch before either grace advancement or replay-triggered family revocation, while preserving family revocation for genuine same-epoch reuse.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed against source and fixed in bd864c7. This one was precise and I had the placement wrong.
Verified the ordering in RefreshAsync: the token is resolved, and if RevokedAt is not null the #176 grace/replay branch runs — TryGraceReplacementAsync, and on failure RevokeAllActiveForUserAsync then return — before FindByIdAsync ever loads the user. So an epoch comparison placed anywhere near the rotation simply never executes on that path.
And the consequence is the part I would have missed: it is not a bypass, it is 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. No "can the attacker get in?" test would ever have caught it.
The comparison now sits ahead of the grace branch: resolve the token, load the user, compare epochs, and only then enter grace/replay. A mismatch fails closed and inert — generic Identity.InvalidRefreshToken, 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 genuine same-epoch reuse, which is the theft signal #176 exists to catch, and there is now a test pinning both sides so the fix cannot be quietly implemented by weakening that detection.
Recorded the shared shape, since this is its third appearance in the document: 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.
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.
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.
|
@codex both round-3 findings are addressed in
Please re-review
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b198dfb14d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| 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 |
There was a problem hiding this comment.
Fresh evidence in the revised cutover is that revocation changes only RevokedAt while legacy tokens and users both remain at epoch 0. After the user logs in again, their new token is also epoch 0; presenting any stolen legacy token therefore passes the early epoch comparison and, because the migration revoked its replacement or left none, enters the replay branch and revokes the user's new family repeatedly. Moreover, Production runs the migration before the new serving process, so an old binary still serving during a rolling cutover can insert another token whose new column receives the database default 0, and the new binary will accept it. Initialize users/new credentials to an epoch distinct from all legacy/defaulted rows, or establish another boundary after old writers have stopped.
AGENTS.md reference: AGENTS.md:L50-L50
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 21df253. Both halves were right, and the second half is one I would not have found.
The differentiation. Revoking legacy tokens is not the same as retiring them. With users and legacy tokens both at 0, the victim's new token after re-login is also 0 — so the stolen legacy token passes the comparison, falls into the revoked-token replay branch, and burns the fresh family. The denial of service from the previous thread, reintroduced by the fix for the one before it.
Users now start at 1 (column default and C# initializer), legacy rows stay at 0, and since epochs only increment, 0 is permanently retired — no user will ever carry it.
The rolling deploy. This is the part I had not considered, and your AGENTS.md:50 pointer is the right one: migrate runs as a separate job before the new serving process (#263), so an old binary can still be inserting refresh tokens while the columns already exist. Its INSERTs take the database default — which is now exactly the retired sentinel. Those rows are inert by construction rather than by timing, so there is no window in which an old writer mints something the new binary honours. Their holders get logged out; nobody gets a credential the mechanism cannot see.
On keeping the revocation anyway. The epoch separation already makes legacy tokens inert, so the UPDATE is now defence-in-depth — but the direction matters and is worth stating: if the comparison ever regresses, revoked-and-retired tokens degrade to a denial of service, whereas active-and-retired ones would degrade to access. Given the choice of failure modes, take the one that locks people out.
Tests now assert the second half explicitly — after the user logs in again, the stolen legacy token still cannot burn their 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.
Recorded as its own rule, since it is a different mistake from the previous three: 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.
… event 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.
|
@codex round-4 finding addressed in Both halves were right. Users now start at Kept the legacy revocation as defence-in-depth, deliberately in that direction — a regressed comparison then degrades to lockout rather than to access. Please re-review
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 21df253b48
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| - A refresh token active before the migration is rejected after it, and the user | ||
| is forced to log in again. |
There was a problem hiding this comment.
The cutover coverage exercises only the legacy refresh token, but every access token issued by the current JwtTokenService also predates the epoch claim and remains cryptographically valid for up to 15 minutes. If CredentialEpochMiddleware treats an absent or unparsable claim as “not applicable” rather than a mismatch, those bearer tokens continue authorizing requests after deployment, contradicting the forced-re-login boundary. Add a pre-migration access-token case that makes an authenticated request after cutover and requires 401, explicitly pinning missing/malformed epoch claims to fail closed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 87dc267. You are right, and the omission had a cause worth naming.
The cutover covered the credential I had just been thinking about (refresh tokens) and forgot the other one already in flight. Every access token minted before the deploy carries no epoch claim and stays cryptographically valid for its remaining ~15 minutes, so a middleware reading absence 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 in the previous round — so it fails through the ordinary comparison with no separate branch to forget. That keeps it uniform rather than a special case, which matters because a special case is exactly what gets dropped in a refactor.
On why this was easy to miss: the pull comes from the codebase's own convention. 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. Stated in the spec as a rule: 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 post-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 recorded the pattern across rounds, since this is the third instance: 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.
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.
|
@codex round-5 finding addressed in Absent or unparsable epoch claims now parse to That is three migration-boundary defects in a row (refresh-token epoch equality, the rolling-deploy default, now pre-cutover access tokens), so I have gone looking for the fourth rather than waiting to be told. Please check specifically:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 87dc267834
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| **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 |
There was a problem hiding this comment.
In the rolling sequence described here, epoch 0 protects only requests that reach the new binary. A replica still running the old authentication pipeline validates bearer signature/lifetime without CredentialEpochMiddleware, so after a new replica disables or changes credentials for a user, a load balancer can still route that user's pre-deploy access token—and any associated step-up operation—to the old replica, which continues authorizing it until drained. This violates the immediate suspension and forced-relogin boundary; require old serving processes to be drained before exposing the new mutations, or introduce a compatibility rollout phase rather than handling only rows written by old processes.
AGENTS.md reference: AGENTS.md:L50-L50
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 35c701a. This is the inverse of the round-4 finding and I had only solved one half of it: that one was about rows written by an old binary, this is about 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 CredentialEpochMiddleware to consult — so a disable on a new replica leaves the load balancer free to route that user's pre-deploy access token to an old one that authorizes it. Immediate on part of the fleet, which is worse than a documented delay because nothing surfaces it.
Agreed that nothing the mutation does can close this; the gap belongs to the replica that never learned to enforce. The feature now ships in two deploys with the ordering as a hard constraint:
- A — the mechanism, inert (Credential epoch: per-request revocation check, deployed inert ahead of the user-admin mutations #364): epoch columns, migration and cutover, the claim, the middleware,
RefreshAsync's comparison, the SPA 401 plumbing. It 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. It can roll at any pace. - B — the mutations (Users: disable / re-enable a user #356, then Users: change a user's email address #357): not exposed until every pre-A process is drained.
The drain itself belongs to the deployment/ops repo per this repo's host-agnostic boundary; what belongs here is the requirement, and it is now stated as one — an orchestrator that reports "rollout complete" because the new replicas are healthy, without confirming the old ones are gone, does not satisfy it.
Delivery is therefore three PRs. The split is not bookkeeping: (1) is where every migration-boundary correction from rounds 3–5 lives, and it is the part that has to be correct on a mixed fleet.
Recorded 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, which is why the sentinel was not enough here: a sentinel defends against bad data; only ordering defends against absent enforcement.
…change
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.
|
@codex round-6 finding addressed in You were right that the sentinel only covers old writers, not old readers. The mechanism now ships as its own inert deploy (#364) — epoch, migration/cutover, claim, middleware, Six rounds, six real defects, and the last four were all variants of "the old world is not gone yet" in different credential types and now in different processes. So please attack that seam once more, from angles I have not already asked about:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1a937075ad
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| **The migration still revokes every existing refresh token** (`UPDATE | ||
| refresh_tokens SET "RevokedAt" = now() WHERE "RevokedAt" IS NULL`), even though |
There was a problem hiding this comment.
During Deploy A's mixed-fleet window, this update marks every legacy token revoked while the current pre-A IdentityProvider.RefreshAsync can still serve requests. That implementation treats a revoked token without a live replacement as replay and calls RevokeAllActiveForUserAsync, so an old tab refreshing after the user has re-logged in on a new replica revokes the freshly issued epoch-1 family; the old SPA then redirects cleanly, but only after invalidating the user's new session. Postpone the revocation/enforcement cutover until old refresh readers are drained, or introduce a compatibility phase that prevents the old replay branch from acting on cutover-revoked rows.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 5a04260. You caught a straight contradiction: deploy A cannot both revoke every refresh token and claim to be inert.
Traced it through the pre-epoch RefreshAsync and it is exactly as you describe — a revoked token with no live replacement is read as a replay, answered with RevokeAllActiveForUserAsync. 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 and gets a clean epoch-1 family; a forgotten tab 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.
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 and behaves exactly as it does today; a new replica enforces fully. No state in between.
Nothing is lost by dropping it, which is the part I had wrong earlier: the revocation was never what drew the boundary. The epoch separation is — legacy tokens carry IssuedEpoch = 0, users carry 1, and the mismatch is rejected inert. Revoking them was belt-and-braces on top of a mechanism that already held. That belt-and-braces now lives in a revoke-legacy-tokens verb run after the drain, when no reader can misinterpret it, and the direction of the defence is preserved: a regressed comparison degrades to lockout rather than to access.
Added a test that asserts the migration mutates no rows — specifically so that anyone "helpfully" reintroducing the cutover revocation into it fails CI rather than discovering this on a mixed fleet.
The rule I took from this, recorded in the spec: 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 the change looks in the new code.
| Since epochs only ever increment from `1`, a token carrying `0` can never match | ||
| any user and is rejected inert, forever. |
There was a problem hiding this comment.
The claim that epoch-0 tokens are inert forever stops holding if Deploy A is rolled back to the pre-A binary, because that binary ignores IssuedEpoch. In particular, an old replica can insert an active default-0 child after a new replica's password-reset revocation—the exact refresh race described earlier—and the new binary rejects it only while A remains deployed; a rollback makes that child usable again despite the password reset. Specify a rollback cutover that drains A processes and revokes all active refresh tokens before restarting the pre-A fleet, or otherwise make rollback fail closed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 5a04260. "Epoch 0 is inert forever" was doing unexamined work — it quietly assumed a binary that reads IssuedEpoch is still running.
The concrete chain is the one you name: during the mixed-fleet window an old replica inserts an active default-0 child right after a new replica's password reset revoked the family. The new binary rejects that child on sight, so it looks handled. A rollback drops the check entirely and resurrects it — and the password reset that was supposed to kill it silently did not.
Rollback therefore 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. One forced re-login, and it makes the rollback fail closed. The procedure belongs to the deployment/ops repo per this repo's host-agnostic boundary; the requirement is now stated here, and there is a test that mints a default-0 child and asserts it is unusable under a simulated pre-A binary — i.e. only if the rollback revocation happened.
The general rule, recorded: 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 — rollback is the one deploy nobody rehearses and everybody eventually performs.
Worth noting these two findings are the same mistake pointing in opposite directions along the deploy timeline: the other thread was the old binary still running forward of the migration, this is the old binary running after it. Both were invisible as long as I reasoned about the new code in isolation.
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.
|
@codex both round-7 findings addressed in
Seven rounds, nine real defects, and the last five have all been variants of "the old world is still here." I would rather find the remaining ones than ship them, so please keep going — and specifically:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5a04260573
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| 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 |
There was a problem hiding this comment.
During a gradual Deploy B rollout, a disabled user's login or refresh can still land on an A-only replica. Deploy A's explicit contents include only epoch stamping/comparison, while the current IdentityProvider.LoginAsync and RefreshAsync have no disabled-state checks; if those checks remain with #356, the A replica can mint a current-epoch token that every B replica accepts, bypassing the suspension. Include all disabled-state readers—login, refresh, epoch middleware, and step-up issuance—in A, leaving only the mutations and UI for B.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in ea9820e — this answers the question I had asked, and the answer is no, B is not strictly additive over A. The disabled-state checks in LoginAsync, RefreshAsync and StepUpGrantService.IssueAsync were scheduled with the mutations, so during B's own gradual rollout an A-only replica would mint a current-epoch token for a disabled user that every B replica then honours. The split is now every reader in A, only mutations and UI in B, with DisabledAt shipping unused in A. Stated as the general rule: readers ship before writers, always — a reader deployed early is inert, a writer deployed early is a hole.
| 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. |
There was a problem hiding this comment.
When Deploy B itself rolls gradually, a browser can load the new SPA from a B replica and then have /disable, /enable, or /email routed to an A replica where those routes do not exist, producing intermittent 404s for the new administrative actions. The stated drain only excludes pre-A processes, not the A replicas being replaced by B, so stage the B API across the fleet before exposing the SPA or require an A-to-B drain before clients receive the new UI.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ea9820e. Same class as the A→B boundary, one deploy later, and the stated drain did not cover it. B's API is now staged fleet-wide before the SPA is exposed (or A→B drained before clients receive the new UI), so a browser served the new bundle by a B replica cannot get a 404 from an A replica without the route.
| 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. |
There was a problem hiding this comment.
On the repository's PostgreSQL 18 target, constant defaults avoid a heap rewrite, but each ALTER TABLE ... ADD COLUMN still needs an ACCESS EXCLUSIVE lock. If an old serving process has a long transaction touching refresh_tokens, the migrate job can wait indefinitely and its queued exclusive request can in turn block later refresh/login traffic; DatabaseReadyHealthCheck only gates the replacement process and does not protect the still-serving old fleet. Specify a short migration lock_timeout with a fail-and-retry procedure so this metadata-only migration cannot become an unbounded authentication outage.
AGENTS.md reference: AGENTS.md:L50-L50
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ea9820e. "Additive" was doing more work than it earned — constant defaults skip the heap rewrite, but each ADD COLUMN still takes ACCESS EXCLUSIVE, and a queued exclusive request blocks login and refresh behind it. You are right that DatabaseReadyHealthCheck is no help: it gates the replacement process, not the old fleet still taking traffic. The migration now sets a short lock_timeout and fails fast. 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 also the table that grows — #259 and #270 exist because of it — so it is the one most likely to be busy.
| 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, |
There was a problem hiding this comment.
The new verb is described only as performing an UPDATE, without pinning its predicate or repeat behavior. An implementation that updates every epoch-0 row on each invocation will overwrite historical RevokedAt values and generate table-wide WAL again whenever an operator retries the command. Define it as updating only IssuedEpoch == 0 && RevokedAt IS NULL, preserve existing timestamps, and test that a second run changes zero rows.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ea9820e. Predicate pinned to IssuedEpoch = 0 AND RevokedAt IS NULL, existing timestamps preserved, and a test asserting a second run changes zero rows. Operators retry one-off verbs — seed and bootstrap-admin are idempotent for exactly this reason — so a verb that rewrote RevokedAt history and regenerated table-wide WAL on every retry would have been found the hard way.
…fety
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.
|
@codex all four round-8 findings addressed in Calling the design frozen here, and saying why rather than just going quiet. Rounds 1–4 found defects in the mechanism itself — it did not do what it claimed. Rounds 5–8 found defects at the deploy boundary — the mechanism was right, the old world had not gone away. Round 8 is the first where three of four findings are ordinary operational hygiene (a lock timeout, a verb 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. Nine drafts is where a design document stops earning its keep. What genuinely remains 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, and I would rather move it than keep growing this into a runbook. So: one final pass, please, restricted to anything that would make the shipped feature incorrect — a wrong guarantee, a missing guard, a race, a test that would pass with the defect present. If the only remaining findings are deployment procedure, say so and I will treat that as converged and take them to the ops repo. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ea9820ee23
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| 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 |
There was a problem hiding this comment.
Fresh evidence beyond the earlier placement fix is that this comparison is only a point-in-time read: a revoked epoch-E token can pass it, then a password/email/disable transaction can commit epoch E+1 and a fresh login can mint an E+1 token before the old request reaches the currently user-wide RevokeAllActiveForUserAsync. That stale request then revokes the new family despite having passed the check against the old epoch; the proposed test on lines 644–649 is sequential and would pass with this race present. Scope replay-triggered revocation to IssuedEpoch == stored.IssuedEpoch (or serialize it with epoch bumps), and add a barrier-controlled test that pauses after the comparison, bumps the epoch and logs in, then resumes the replay.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in d88b151. This is a genuine race and it is distinct from the placement fix, so the freeze I called a comment ago is retracted rather than quietly amended.
Moving the comparison ahead of the grace branch fixed the case where the check never ran. It does nothing for 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 disable/email/password change commits E+1 and a fresh login mints an E+1 family — which the stale request then burns. Same denial of service, entered by passing the check rather than skipping it.
Replay revocation is now predicated on IssuedEpoch = stored.IssuedEpoch rather than UserId alone. That is also 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.
You are right that the test I had written would pass with this present — it is sequential. Replaced with a barrier-controlled one: present the revoked epoch-E token, pause after the comparison succeeds, bump and log in fresh at E+1, then release; the E+1 family must survive.
The rule I took from it, since it 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.
| 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. |
There was a problem hiding this comment.
Fresh evidence after the round-8 fix is that the canonical Delivery breakdown still assigns the first PR only the epoch fields, middleware, and refresh comparison, omitting DisabledAt and the disabled-state checks in LoginAsync, RefreshAsync, and StepUpGrantService.IssueAsync. If implementation scope is taken from this issue breakdown, Deploy A will lack those readers and the gradual Deploy B rollout can again mint a current-epoch credential for a disabled user on an A replica. Move the column and all three readers into item 1/#364 so this checklist agrees with the hard rollout requirement above.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d88b151. You are right and this is the more embarrassing of the two: the previous round moved the readers into deploy A in the Rolling deploys section and left the canonical Delivery checklist listing only the epoch fields, middleware and refresh comparison — so anyone scoping implementation from the checklist, which is the section people actually work from, would have rebuilt the exact hole.
Item 1 now reads CredentialEpoch, IssuedEpoch, DisabledAt/DisabledBy, the additive migration, the claim, the middleware, the epoch comparison and epoch-scoped replay revocation, the SPA 401 plumbing, and all three disabled-state readers (LoginAsync, RefreshAsync, StepUpGrantService.IssueAsync), with a note that this is not optional detail and why. #364's issue body carries the same correction.
Recorded as its own item: a document that states a constraint in one section and contradicts it in the section people work from has not fixed anything.
…checklist 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.
|
@codex both round-9 findings addressed in Retracting the freeze I called last comment. I asked for a pass restricted to anything that would make the shipped feature wrong, and you returned a genuine race — so the answer to "is this converged?" was no, and the filter did its job. Recorded in the spec rather than quietly amended.
Same request as before, and it is a real question rather than a formality: another pass restricted to anything that would make the shipped feature incorrect — a wrong guarantee, a missing guard, a race, or a test that would pass with the defect present. Two specific seams, since those are where the last five rounds have lived:
If the remaining findings are deployment procedure rather than correctness, say so and I will take them to the ops repo and stop iterating here. |
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Design doc only — no code. Implementation lands as #356 (disable, first) then #357 (email, reuses its machinery).
Second commit reworks the design after an adversarial review found the enforcement mechanism does not do what the first draft claimed. The spec ends with a What the first draft got wrong section recording all nine corrections, so the same reasoning is not re-derived later.
What it covers
Two gaps on the Users page: no way to take someone's access away, and no way to correct the address they log in with. Today a departing worker can only have their password reset — the account stays live — and a typo'd email means abandoning the account for a second one, leaving a phantom user on the history the first one recorded.
The load-bearing correction
The first draft rotated
SecurityStampand revoked refresh tokens, then called the session terminated. It is not. The access token carriessub,email,account_id,role,must_change_passwordand nothing else; validation checks signature, issuer, audience, lifetime. Nothing on the request path reads a stamp. So:DisabledAtmeant re-enabling resurrected every unexpired pre-disable token — a suspension lifted after five minutes handing 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. It also closes a race bulk revocation cannot:RefreshAsyncloads token+user then revokes and inserts a child, whileRevokeAllActiveForUserAsyncis a separate bulk update — so a refresh in flight can insert a live child after the revoke commits. Minted under the old epoch, that child is useless.Other corrections worth reading
ConcurrencyStamp. Two Owners disabling each other touch different rows and share no concurrency token — both guards pass, both commit, zero Owners remain. It needs the account row lockedFOR UPDATEwith the count re-read inside the lock (the patternAccountRepositoryalready has), and that becomes a standing constraint on every future Owner-removing path including Users: change a user's role after creation (promote/demote) #355.NormalizedUserName— the column carrying the unique index.NormalizedEmail's index is not unique. Omitting it reserves the old address forever and bypasses conflict detection on the new one.CreateUserHandlergates only Owner creation, so a stolen Owner token already mints a Manager, ungated. Gating these operations is still right, for honest reasons; the durable-persistence gap belongs to Require recent step-up authentication for privileged user administration #308 and is filed as Step-up (#308) gates only Owner creation — a stolen Owner token still buys durable access #360.IdempotencyMiddlewarealready runs the pipeline underSingleAttemptExecution, so the inner Identity save is never independently replayed — and the probe it prescribed keyed on "the email is already ours," which is not attempt-unique evidence the way a freshly minted 256-bit token hash is.recover-admingains an id-based lookup, since locating by email is circular after an email-typo lockout.AccountIdin every lookup —ApplicationUserhas no tenant query filter.Out of scope
Hard delete (#272). Role editing (#355). Step-up coverage (#360). Cross-replica step-up replay (#338) is documented as a residual, not fixed here.