From 7f96d22da139e8dde7426888eeef0053eb38daba Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Thu, 13 Aug 2026 08:53:55 -0500 Subject: [PATCH] backlog: amend #1245 -- close two of its open connections and fix the confirming test's shape Peer review before push. Three changes, none of which move the verdict. States the mechanism more precisely: the docstring equates unclaimed-ness with must_change_password, so the flag is a PROXY for "never claimed" and the equivalence holds only while it has one writer. admin_reset_password is a second writer. The durable fix is to stop inferring claimed-ness from a mutable flag any path may raise, not to special-case the reset. Closes two connections the original bound left open, both of which hold: set_password issues an UPDATE that persists the flag, and _other_enabled_admin_exists returns true for any enabled ADMINISTRATOR other than the excluded id with no expiry wait. Individually-verified links are not a verified chain; the gaps between them are invisible exactly when every link holds. What remains unread is now named rather than implied. Records that the confirming test must assert the CORRECT behaviour under a strict xfail, not reproduce the defect. A test asserting today's behaviour turns the bug into expected behaviour, so the fix reads as a regression and the green defends the defect. strict is load-bearing: it reds when the test starts passing, so the fix cannot land silently. The reason string must name the mechanism, because the line numbers will drift. --- docs/BACKLOG.md | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/docs/BACKLOG.md b/docs/BACKLOG.md index 9f6a79f6..63f3faa5 100644 --- a/docs/BACKLOG.md +++ b/docs/BACKLOG.md @@ -8478,4 +8478,17 @@ _FHIR_ID_RE.fullmatch("abc\n") -> False the fix > **BOUND ON WHAT WAS VERIFIED.** The control flow above was read statically at `origin/main` and each cited line confirmed individually; it has **not** been reproduced by executing a login sequence. A test that resets the password of a claimed `admin` account on a two-administrator system and then asserts the temporary credential works is the confirming experiment, and it does not exist today. +> ⚠️ **Amendment, same day, strengthening the item on a peer review before it was pushed. Two of the original bound's open connections are now CLOSED, and the mechanism is stated more precisely.** +> +> **THE DEFECT IS A PROXY, and naming it that way makes the fix obvious.** The docstring at `auth/service.py:579` says the routine *"Only ever touches an unclaimed bootstrap (`must_change_password` still set)"* -- it **equates** unclaimed-ness with that flag. `must_change_password` is being used as a **proxy for "this account has never been claimed"**, and the equivalence holds only while the flag has exactly one writer. `admin_reset_password:2733` is a second writer, so it sets the proxy on an account that *was* claimed. The gate is not wrong about its own test; it is wrong that the test means what it is taken to mean. **The durable fix is to stop inferring claimed-ness from a mutable flag any path may raise** -- record it, or test something a reset cannot forge -- rather than to special-case the reset. +> +> **TWO UNOPENED CONNECTIONS FROM THE ORIGINAL BOUND ARE NOW READ, and both hold.** The original filing verified each link individually and said so; a peer correctly pointed out that individually-verified links are not a verified chain, because the gaps *between* them are invisible precisely when every link checked holds. +> - `store/store.py:7714-7729` -- `set_password` issues `UPDATE users SET ... must_change_password=?`, so the flag **is** persisted, not merely passed. +> - `auth/service.py:567-574` -- `_other_enabled_admin_exists` iterates users, skips disabled and the excluded id, and returns true for any holder of the ADMINISTRATOR role. A second enabled administrator **does** make `superseded` true, with no expiry wait. +> +> **WHAT REMAINS UNVERIFIED IS NOW SMALLER AND NAMED:** the chain has still not been executed end to end. What is unread is `get_user_by_username` returning the persisted flag on the read side. That is near-certain and it is still not the same as having run it. +> +> **THE CONFIRMING TEST MUST ASSERT THE CORRECT BEHAVIOUR, NOT REPRODUCE THE DEFECT.** A test written to assert what the code does today **turns the bug into expected behaviour**: whoever fixes this then sees a red test and concludes they broke something, so the green actively defends the defect. Write it as *reset a claimed `admin` on a two-administrator system, then **assert the temporary credential works***, and land it with **`@pytest.mark.xfail(strict=True)`**. `strict` is the load-bearing half -- it reds when the test starts **passing**, so the fix cannot land silently and the marker cannot rot into a permanently ignored line. A non-strict xfail is the same defect one level up. +> The `reason=` string is the **artifact**, and it must name the **mechanism, not the symptom**: the gate at `:584` tests `must_change_password`, `admin_reset_password:2733` re-raises it, and no route clears it. That sentence is what makes the test re-derivable once these line numbers drift, which they will. + **Cluster:** Authentication / account lifecycle. **Priority:** P2. **Verdict:** build. **Severity:** no deployment axis -- zero instances; on first deployment an administrator following the documented reset **would** render the `admin` account permanently unusable without either party being told.