fix(auth): a check-then-act race let two concurrent first logins both bind one federated subject (BACKLOG #1256) - #640
Open
wshallwshall wants to merge 1 commit into
Open
Conversation
…ins could both bind (BACKLOG #1256) auth/service.py reads the current holder and writes the binding in two separate awaits, so two concurrent FIRST logins for one (issuer, subject) can both observe "no holder" and both bind. The guard is correct and covers every non-concurrent case; what it cannot do is make its own read-then-write atomic. A partial/filtered unique index ux_users_federated_subject now closes it on all three backends, and the race loser is rendered as the SAME federated_subject_already_bound outcome the sequential path returns rather than a 500 for a condition the gate handles cleanly a microsecond earlier. THE ACCEPTANCE DEMONSTRATES THE RACE, WHICH IS THIS ITEM'S WHOLE POINT. Its correction block says a test asserting no UNIQUE constraint exists PASSES ON THE DEFECT -- it re-derives the guard's own in-code comment, which already records the 0-of-13/8/10 measurement. So the test runs two concurrent logins with the interleave FORCED by an asyncio.Barrier: both reads complete before either write. Removing the index reds it -- both bind. A SECOND TEST ASSERTS BOTH LOGINS OBSERVED NO HOLDER. Without it, a refactor that quietly serialised them would leave the first test green while the index was never consulted -- this row's own failure mode, one level up. A race test that merely starts two coroutines and hopes is a coin flip that passes on the defect whenever the scheduler serialises, and more often on fast hardware. THREE CORRECTIONS MADE WHILE BUILDING, each from asking what the codebase does: The SQLite index cannot live in _SCHEMA. _SCHEMA runs at store.py:2087 and _migrate at :2088, so on a users table predating the federated columns it references a column that does not exist yet and the store fails to open. It sits in _migrate beside ix_queue_body_ref, whose comment states that reasoning. SQL Server needed a RE-TYPE migration, not just a declaration change. The ALTER TABLE ADD guards are COL_LENGTH(...) IS NULL and fire only when a column is ABSENT, so an existing table keeps NVARCHAR(MAX) -- which cannot be an index key. COL_LENGTH returns -1 for MAX, which is the discriminator, and the re-type must precede the index because _SCHEMA applies in order. NVARCHAR(256) not 450: 2 x 256 x 2 = 1024 bytes, inside the 1700-byte nonclustered key limit. I ADDED A CONTRACT EXCEPTION AND REVERTED IT. FederatedSubjectConflict existed so auth/ would not import three drivers -- but auth/service.py:2558 already solves that without one, joining the class MRO names and testing for Integrity / UniqueViolation. That is ADR 0068 4's duplicate-label race: the same check-then-act shape, already solved. Sound reasoning, wrong mechanism, because the constraint I designed around had already been dissolved. The WHERE ... IS NOT NULL filter is stylistic on SQLite and Postgres and REQUIRED on SQL Server, where NULLs compare EQUAL in a unique index -- unfiltered it would admit exactly ONE unfederated user in the table. Full suite: 13854 passed, 1 failed. The failure is test_selfheal_installed_parity, which this change does not touch: my checkout's worktree-selfheal.ps1 matches origin/main exactly, so the INSTALLED payload on this machine is stale, not the repository. Same test failed the same way on #1367 earlier tonight. Do not fix it by reinstalling. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Built by builder-1. NOT ARMED -- it touches
auth/service.pyand three store backends, which is squarely the standing auth hold.The defect
The federated binding guard is check-then-act: it reads the current holder and writes the binding in two separate awaits. So two concurrent FIRST logins for one (issuer, subject) can both see "no holder" and both bind. A partial/filtered unique index on all three backends closes it, and the loser now gets the same
federated_subject_already_boundrefusal the sequential path returns instead of a 500.Read the test first, because this item's lesson is that the obvious test is worthless
A test asserting no UNIQUE constraint exists PASSES ON THE DEFECT -- it merely re-derives the guard's own in-code comment. So the acceptance runs two concurrent logins with the interleave FORCED by an
asyncio.Barrier, and it is mutation-tested: remove the index and it reds, both bind.A second test asserts both logins observed no holder. Without it, a refactor that quietly serialised them would leave the first test green while the index was never consulted -- which is this row's own failure mode one level up.
Three things the author got wrong and fixed, each by asking what the codebase already does
The SQLite index cannot live in
_SCHEMA. It runs atstore.py:2087and_migrateat:2088, so on a users table predating the federated columns it references a column that does not exist yet and the store fails to OPEN. It now sits in_migratebesideix_queue_body_ref, which records that same reason.SQL Server needed a RE-TYPE migration, not a declaration change. The
ALTER TABLE ADDguards fire only when a column is ABSENT, so an existing table keepsNVARCHAR(MAX), which cannot be an index key.COL_LENGTHreturns -1 for MAX, and that is the discriminator. This would have passed every test on a fresh database and failed on every real one.A contract exception was added and then reverted.
auth/service.py:2558already solves the same problem without one -- ADR 0068 4's duplicate-label race, the identical check-then-act shape. Sound reasoning, wrong mechanism.Verification
ruff check and format clean over 979 files, mypy strict clean over 267, both OIDC suites 29 passed, full suite 13854 passed.
One local failure,
test_selfheal_installed_parity, is not from this branch and is fixed separately: it compares a user-scope installed file against the checkout and skips on CI.Co-Authored-By: Claude Opus 5 noreply@anthropic.com