diff --git a/docs/BACKLOG.md b/docs/BACKLOG.md index 960be8883..e81ed7407 100644 --- a/docs/BACKLOG.md +++ b/docs/BACKLOG.md @@ -12398,6 +12398,35 @@ BUILDS it.* **Provenance.** Diagnosed by the lane whose own commit tripped it: seven tests in one file failed in a full run and **passed in isolation, twice**. It reported the negative control alongside the fix -- restoring the bare import reproduced exactly those seven failures -- which is what makes the green meaningful. Recorded here rather than left in session mail because the collision outlives the commit that revealed it. It is the same shape as the rest of this cluster: **the name resolved to the neighbouring module, and nothing said so.** ## 1256. the federated binding guards account-continuity but never subject-exclusivity, so two accounts can bind one identity +> **THE ATOMICITY HALF IS BUILT 2026-08-27; banner left open for the archive pass.** A partial/filtered +> unique index `ux_users_federated_subject` on `(oidc_issuer, oidc_subject)` now exists 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. +> +> **THE ACCEPTANCE DEMONSTRATES THE RACE, as this row's correction demands.** Two concurrent first +> logins for one subject, interleave FORCED by an `asyncio.Barrier` so both reads complete before +> either write. Removing the index reds it: both bind. **A second test asserts both logins observed +> NO holder** -- without that, a future refactor that quietly serialises 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 CORRECTIONS MADE WHILE BUILDING, each found by asking what the codebase already does:** +> 1. The SQLite index CANNOT live in `_SCHEMA`: it runs at `store.py:2087`, `_migrate` at `:2088`, so +> on a users table predating the federated columns it references a column that does not exist yet. +> It sits in `_migrate` beside `ix_queue_body_ref`, which records that same reasoning. +> 2. 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. NVARCHAR(256), not 450: 2x256x2 = 1024 bytes, inside the 1700-byte limit. +> 3. **A contract exception was added and REVERTED.** `FederatedSubjectConflict` in `store/base.py` +> 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` +> (ADR 0068 4's duplicate-label race, the same check-then-act shape). Sound reasoning, wrong +> mechanism, because the constraint it designed around had already been dissolved. Do not re-add. +> +> **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. A reader who learned the rule from the SQLite file would delete it as redundant. +> > 🔢 **Re-scored 2026-08-20 -> P2.** Value **5/10** · Difficulty **4/10** · _fill-in_. The behavioural half is closed in shipped code -- auth/service.py:1219-1232 refuses a second account binding one (issuer, subject), with the lookup on all three backends and a sequential test at tests/test_auth_oidc_service.py:456 -- so the remainder is only the database constraint that makes the read-then-write at :1219/:1232 atomic, which is worth a 5 as auth hardening with the app guard covering every non-concurrent case. Difficulty drops to 4 because the amendment moved the design half (refuse rather than re-point) into shipped code and comment, leaving a well-precedented seam: all three backends already declare unique indexes (ux_webauthn_label, ux_search_presets_owner_name), so the novel cost is one first-of-kind SQL Server ALTER COLUMN off NVARCHAR(MAX) (store/sqlserver.py:1358; the file contains zero ALTER COLUMN today), a filtered index for SQL Server NULL semantics, and a CI-only concurrent-bind test on the two server backends. _(was 7/10 · 5/10.)_ > diff --git a/messagefoundry/auth/service.py b/messagefoundry/auth/service.py index 874c194c7..1fdafbedb 100644 --- a/messagefoundry/auth/service.py +++ b/messagefoundry/auth/service.py @@ -1269,9 +1269,34 @@ async def _complete_ad_login( # First federated login for this account (or an unbound AD account's first): record the # (issuer, sub) binding so a later reassigned-username login carrying a different subject is # refused by the guard above. A matching binding is left untouched (no updated_at churn). - await self._store.set_user_federated_subject( - user.id, federated_subject[0], federated_subject[1] - ) + try: + await self._store.set_user_federated_subject( + user.id, federated_subject[0], federated_subject[1] + ) + except Exception as exc: + # BACKLOG #1256. THE GUARD ABOVE IS CHECK-THEN-ACT: its read and this write are + # separate awaits, so two concurrent FIRST logins for one subject can both see + # `holder is None` and both reach here. `ux_users_federated_subject` refuses the + # loser on all three backends, and this renders that refusal as the SAME outcome the + # sequential path returns -- otherwise the race loser gets a 500 for a condition the + # gate handles cleanly one microsecond earlier. + # + # MRO BY NAME, matching the duplicate-label race at `_enroll_webauthn` (ADR 0068 4): + # each backend raises its own integrity class -- sqlite3.IntegrityError, asyncpg's + # UniqueViolationError, pyodbc's IntegrityError -- and naming them here would make + # this module import-aware of every driver and silently stop covering a backend added + # later. Anything that is NOT an integrity violation re-raises untouched. + mro = "".join(t.__name__ for t in type(exc).__mro__) + if "Integrity" not in mro and "UniqueViolation" not in mro: + raise + await self._directory_reject_audit( + principal.username, "oidc", "federated_subject_already_bound" + ) + return LoginOutcome( + ok=False, + error="federated sign-in failed", + reason="federated_subject_already_bound", + ) # BACKLOG #1248. Binding an external identity decides WHO MAY SIGN IN as this account # from now on, so it is a privilege change and gets the same two records the role # resync below emits: an audit row, and an out-of-band notice to the account holder diff --git a/messagefoundry/store/postgres.py b/messagefoundry/store/postgres.py index afd5d4513..ddc291da9 100644 --- a/messagefoundry/store/postgres.py +++ b/messagefoundry/store/postgres.py @@ -545,6 +545,14 @@ def __init__(self, method: str, outbox_ids: tuple[str, ...]) -> None: oidc_subject TEXT, password_claimed_at DOUBLE PRECISION )""", + # BACKLOG #1256: the atomicity the CHECK-THEN-ACT guard in auth/service.py cannot give itself -- + # its read and its write are separate awaits, so two concurrent FIRST logins for one subject can + # both observe "no holder" and both bind. Same shape as ux_webauthn_label (ADR 0068 4). + # PARTIAL so unfederated rows coexist; Postgres treats NULLs as distinct, so the WHERE states + # intent here and is REQUIRED on SQL Server, where NULLs compare equal. + """CREATE UNIQUE INDEX IF NOT EXISTS ux_users_federated_subject + ON users(oidc_issuer, oidc_subject) + WHERE oidc_issuer IS NOT NULL AND oidc_subject IS NOT NULL""", """CREATE TABLE IF NOT EXISTS roles ( id TEXT PRIMARY KEY, display_name TEXT NOT NULL, diff --git a/messagefoundry/store/sqlserver.py b/messagefoundry/store/sqlserver.py index c9671846a..727fc5eda 100644 --- a/messagefoundry/store/sqlserver.py +++ b/messagefoundry/store/sqlserver.py @@ -1376,7 +1376,12 @@ def __init__(self, conn: Any, cur: Any) -> None: channel_scope NVARCHAR(MAX) NULL, totp_secret NVARCHAR(MAX) NULL, totp_enabled BIT NOT NULL DEFAULT 0, totp_enrolled_at FLOAT NULL, totp_recovery_codes NVARCHAR(MAX) NULL, last_totp_step INT NULL, - oidc_issuer NVARCHAR(MAX) NULL, oidc_subject NVARCHAR(MAX) NULL, + -- BACKLOG #1256: SIZED, NOT MAX, BECAUSE A MAX COLUMN CANNOT BE AN INDEX KEY on SQL + -- Server, and ux_users_federated_subject below is what makes the application guard's + -- read-then-write atomic. 256 is this file's dominant width (55 uses); the composite + -- key is then 2 x 256 x 2 bytes = 1024, inside the 1700-byte nonclustered limit. + -- NVARCHAR(450), the other precedent here, would be 1800 across two columns and fail. + oidc_issuer NVARCHAR(256) NULL, oidc_subject NVARCHAR(256) NULL, password_claimed_at FLOAT NULL)""", """IF COL_LENGTH('users','channel_scope') IS NULL ALTER TABLE users ADD channel_scope NVARCHAR(MAX) NULL""", @@ -1395,9 +1400,28 @@ def __init__(self, conn: Any, cur: Any) -> None: # Federated (issuer, sub) identity keying (BACKLOG #1015): COL_LENGTH-gated ADD on a pre-existing # users table. NULL on existing rows = "not yet federated" (username stays the sole key). Idempotent. """IF COL_LENGTH('users','oidc_issuer') IS NULL - ALTER TABLE users ADD oidc_issuer NVARCHAR(MAX) NULL""", + ALTER TABLE users ADD oidc_issuer NVARCHAR(256) NULL""", """IF COL_LENGTH('users','oidc_subject') IS NULL - ALTER TABLE users ADD oidc_subject NVARCHAR(MAX) NULL""", + ALTER TABLE users ADD oidc_subject NVARCHAR(256) NULL""", + # BACKLOG #1256: RE-TYPE A PRE-EXISTING MAX COLUMN, WHICH THE COL_LENGTH-GATED ADDs ABOVE CANNOT + # REACH. They fire only when the column is ABSENT, so a users table created before this change + # keeps NVARCHAR(MAX) -- and a MAX column CANNOT BE AN INDEX KEY, so the index below would fail + # against exactly the databases that already exist. COL_LENGTH returns -1 for a MAX column, which + # is how this tells "already sized" from "needs re-typing" without reading catalogue views. + # + # MUST RUN BEFORE the index. _SCHEMA is applied in order, so position here is load-bearing. + """IF COL_LENGTH('users','oidc_issuer') = -1 + ALTER TABLE users ALTER COLUMN oidc_issuer NVARCHAR(256) NULL""", + """IF COL_LENGTH('users','oidc_subject') = -1 + ALTER TABLE users ALTER COLUMN oidc_subject NVARCHAR(256) NULL""", + # The atomicity the application guard cannot provide: auth/service.py reads the current holder and + # writes the binding in two separate awaits, so two concurrent FIRST logins for one subject can + # both observe "no holder" and both bind. FILTERED so unfederated rows coexist -- and on SQL Server + # the filter is REQUIRED, not stylistic: unlike SQLite and Postgres it treats NULLs as EQUAL in a + # unique index, so an unfiltered index would permit exactly ONE unfederated user in the table. + """IF INDEXPROPERTY(OBJECT_ID('users'),'ux_users_federated_subject','IndexID') IS NULL + CREATE UNIQUE INDEX ux_users_federated_subject ON users(oidc_issuer, oidc_subject) + WHERE oidc_issuer IS NOT NULL AND oidc_subject IS NOT NULL""", # Claimed-ness of the bootstrap admin (BACKLOG #1245): NULL on an existing row would read as # "never claimed", which is what would retire an account whose holder claimed it long ago — this # defect, re-introduced by its own fix. So the ADD is paired with a one-time backfill: a local diff --git a/messagefoundry/store/store.py b/messagefoundry/store/store.py index f96581a94..1448cd241 100644 --- a/messagefoundry/store/store.py +++ b/messagefoundry/store/store.py @@ -3169,6 +3169,25 @@ async def _migrate(db: aiosqlite.Connection) -> None: # The body_ref deref index lives here (not _SCHEMA) so it is created only AFTER the column is # guaranteed present — on a Step-A queue it'd otherwise reference a not-yet-added column. await db.execute("CREATE INDEX IF NOT EXISTS ix_queue_body_ref ON queue(body_ref)") + # BACKLOG #1256: the federated guard in auth/service.py is CHECK-THEN-ACT -- it reads the + # current holder and writes the binding in two separate awaits, so two concurrent FIRST + # logins for one subject can both observe "no holder" and both bind. This index is the + # atomicity that guard cannot give itself. Same shape as ux_webauthn_label and ADR 0068 4's + # concurrent-enroll race: the loser's driver error is rendered by the caller as the same + # refusal its pre-check returns. + # + # HERE RATHER THAN _SCHEMA, for the reason ix_queue_body_ref states one line up: _SCHEMA runs + # BEFORE this migration, and on a users table predating the federated columns the index would + # reference a column that does not exist yet. + # + # PARTIAL so unfederated rows coexist. SQLite treats NULLs as distinct here so the WHERE is + # not strictly required -- it is written anyway because on SQL Server the same filter IS + # required (NULLs compare EQUAL there, so an unfiltered index admits ONE unfederated user). + await db.execute( + "CREATE UNIQUE INDEX IF NOT EXISTS ux_users_federated_subject" + " ON users(oidc_issuer, oidc_subject)" + " WHERE oidc_issuer IS NOT NULL AND oidc_subject IS NOT NULL" + ) # BACKLOG #154: a DB whose `response` table predates resp_headers gains it here (NULL on existing # rows = "no captured headers", byte-identical). The table itself is created by _SCHEMA. cur = await db.execute("PRAGMA table_info(response)") diff --git a/tests/test_auth_oidc_concurrent_bind.py b/tests/test_auth_oidc_concurrent_bind.py new file mode 100644 index 000000000..8ae948660 --- /dev/null +++ b/tests/test_auth_oidc_concurrent_bind.py @@ -0,0 +1,215 @@ +# SPDX-License-Identifier: AGPL-3.0-or-later +# Copyright (C) 2026 MessageFoundry Organization and contributors +"""BACKLOG #1256: two concurrent FIRST logins for one federated subject must not both bind. + +**THE APPLICATION GUARD IS CORRECT AND IS NOT WHAT THIS TESTS.** ``auth/service.py`` resolves the +presenting subject, refuses with ``federated_subject_already_bound`` when a different account holds it, +and only then records the binding. Every non-concurrent case is covered, and +``test_one_subject_cannot_bind_two_accounts`` in the sibling module pins exactly that. + +What the guard cannot do is make its own read-then-write atomic. The read and the write are separate +awaits, so two logins interleaving between them can both observe "no holder" and both bind. +``ux_users_federated_subject`` closes that, and the caller renders the loser's integrity error as the +same refusal the sequential path returns. + +***WHY NOT SIMPLY ASSERT THE INDEX EXISTS.*** Because that test PASSES ON THE DEFECT. The guard's own +in-code comment already records "no UNIQUE constraint names these columns on any backend" -- so a test +that measures constraints re-derives a comment and says nothing about the race. #1256's correction block +says this in as many words. The acceptance had to demonstrate the race itself. + +***THE INTERLEAVING IS FORCED, NOT HOPED FOR.*** A test that merely starts two coroutines and hopes they +interleave is a coin flip that passes on the defect whenever the scheduler happens to serialise them -- +silently, and more often on a fast machine. The barrier below makes both reads complete before either +write can proceed, so the race is deterministic. ``test_the_race_was_actually_exercised`` asserts the +barrier did its job; without it a green run here would not distinguish "the index held" from "the two +logins never actually raced". +""" + +from __future__ import annotations + +import asyncio +from typing import Any + +import pytest +from cryptography.hazmat.primitives.asymmetric import rsa + +from messagefoundry.auth import oidc +from messagefoundry.auth.ldap import AdPrincipal +from messagefoundry.store.store import MessageStore +from tests.test_auth_oidc_service import ( + PRINCIPAL, + _claims, + _FakeLdap, + _flow, + _mint, + _service, +) + +#: One verified identity, presenting twice at the same instant. +SUBJECT = "S-1-concurrent" + + +@pytest.fixture(scope="module") +def rsa_key() -> rsa.RSAPrivateKey: + """Local rather than imported: ``rsa_key`` is a module-scoped FIXTURE in the sibling suite, and a + fixture is resolved by name in the module that requests it -- importing the function object does + not register it here. Same key size, same scope, so the cost is identical.""" + return rsa.generate_private_key(public_exponent=65537, key_size=2048) + + +async def _run_race( + store: MessageStore, + rsa_key: rsa.RSAPrivateKey, + monkeypatch: pytest.MonkeyPatch, +) -> tuple[list[Any], list[bool]]: + """Two federated logins for ONE subject, held open until both have read the current holder. + + Returns the two outcomes and what each login OBSERVED at the read -- the second is what proves the + race happened rather than the two calls quietly serialising. + """ + other = AdPrincipal( + username="bsmith", # a genuinely different on-prem object, not a rename + display_name="B Smith", + email="bsmith@corp.example", + dn="CN=bsmith,DC=corp,DC=example", + groups=PRINCIPAL.groups, + ) + ldap = _FakeLdap(by_username={"jdoe": PRINCIPAL, "bsmith": other}) + service = await _service(store, rsa_key, ldap=ldap) + + # The token endpoint is a module global, so a per-call stub would have the two logins overwrite + # each other's token and collapse onto one account. Hand each caller its own token instead, keyed + # by the code it presents. + tokens = { + "code-jdoe": _mint(rsa_key, _claims(sub=SUBJECT, preferred_username="jdoe@corp.example")), + "code-bsmith": _mint( + rsa_key, _claims(sub=SUBJECT, preferred_username="bsmith@corp.example") + ), + } + + def fake_exchange(**kwargs: Any) -> dict[str, object]: + return {"id_token": tokens[kwargs["code"]], "access_token": "at-never-stored"} + + monkeypatch.setattr(oidc, "exchange_code", fake_exchange) + + # FORCE THE INTERLEAVE. Both logins must finish reading the holder before either writes. + barrier = asyncio.Barrier(2) + observed: list[bool] = [] + real_read = store.get_user_by_federated_subject + + async def read_then_wait(*args: Any, **kwargs: Any) -> Any: + holder = await real_read(*args, **kwargs) + observed.append(holder is None) + await barrier.wait() # neither proceeds to the write until both have read + return holder + + monkeypatch.setattr(store, "get_user_by_federated_subject", read_then_wait) + + async def login(code: str) -> Any: + return await service.authenticate_oidc( + code, _flow(), redirect_uri="https://ops.example/ui/oidc/callback" + ) + + outcomes = await asyncio.gather( + login("code-jdoe"), login("code-bsmith"), return_exceptions=True + ) + return list(outcomes), observed + + +async def test_only_one_of_two_concurrent_binds_succeeds( + rsa_key: rsa.RSAPrivateKey, monkeypatch: pytest.MonkeyPatch +) -> None: + """THE DEFECT ITSELF: before ux_users_federated_subject, BOTH of these bound.""" + store = await MessageStore.open(":memory:") + try: + outcomes, _ = await _run_race(store, rsa_key, monkeypatch) + for out in outcomes: + assert not isinstance(out, BaseException), ( + f"a login raised rather than returning: {out!r}" + ) + ok = [o for o in outcomes if o.ok] + refused = [o for o in outcomes if not o.ok] + assert len(ok) == 1, f"expected exactly one bind to win, got {len(ok)}: {outcomes!r}" + assert len(refused) == 1 + assert refused[0].reason == "federated_subject_already_bound", ( + "the race loser must get the SAME refusal the sequential path returns, not a 500 or a " + f"different reason: {refused[0]!r}" + ) + finally: + await store.close() + + +async def test_the_race_was_actually_exercised( + rsa_key: rsa.RSAPrivateKey, monkeypatch: pytest.MonkeyPatch +) -> None: + """THE CONTROL ON THE TEST ABOVE, and it is the row that makes its green mean something. + + If the two logins serialised -- scheduler, a lock, a future refactor -- the second would read a + holder that already exists, the ordinary guard would refuse it, and the test above would pass + WITHOUT the index ever being consulted. It would then keep passing after a revert. + + Both reads observing "no holder" is what says the interleave really happened. + """ + store = await MessageStore.open(":memory:") + try: + _, observed = await _run_race(store, rsa_key, monkeypatch) + assert observed == [True, True], ( + "both logins had to observe NO holder for this to be the concurrent case; " + f"observed {observed!r} -- the calls serialised and the index was never exercised" + ) + finally: + await store.close() + + +async def test_a_second_bind_for_a_DIFFERENT_subject_is_untouched( + rsa_key: rsa.RSAPrivateKey, monkeypatch: pytest.MonkeyPatch +) -> None: + """NEGATIVE CONTROL. The index is filtered and two-column, so two accounts binding two DIFFERENT + subjects must both succeed -- otherwise this change would refuse ordinary federation.""" + store = await MessageStore.open(":memory:") + try: + other = AdPrincipal( + username="bsmith", + display_name="B Smith", + email="bsmith@corp.example", + dn="CN=bsmith,DC=corp,DC=example", + groups=PRINCIPAL.groups, + ) + ldap = _FakeLdap(by_username={"jdoe": PRINCIPAL, "bsmith": other}) + service = await _service(store, rsa_key, ldap=ldap) + + tokens = { + "c1": _mint(rsa_key, _claims(sub="S-1-alice", preferred_username="jdoe@corp.example")), + "c2": _mint(rsa_key, _claims(sub="S-2-bob", preferred_username="bsmith@corp.example")), + } + monkeypatch.setattr( + oidc, + "exchange_code", + lambda **kw: {"id_token": tokens[kw["code"]], "access_token": "at"}, + ) + for code in ("c1", "c2"): + out = await service.authenticate_oidc( + code, _flow(), redirect_uri="https://ops.example/ui/oidc/callback" + ) + assert out.ok, f"a distinct subject was refused: {out!r}" + finally: + await store.close() + + +def test_the_index_is_declared_on_every_backend() -> None: + """Deliberately LAST and deliberately NOT the acceptance test -- see this module's docstring. + + Asserting the index exists cannot see the race and would pass on the defect if the columns were + unconstrained. It earns its place only as a parity check that no backend was missed, which the + race test above cannot give: it runs on SQLite alone, because the Postgres and SQL Server suites + need live servers. + """ + import pathlib + + root = pathlib.Path(__file__).resolve().parents[1] / "messagefoundry" / "store" + for backend in ("store.py", "postgres.py", "sqlserver.py"): + src = (root / backend).read_text(encoding="utf-8") + assert "ux_users_federated_subject" in src, ( + f"{backend} declares no federated-subject unique index, so the race it closes on the " + "other backends is still open there" + )