Repository navigation
_refresh_cached_username re-reads the holder of the new name before refusing (BACKLOG #3070) - #2428
Conversation
set_totp_secret now stages only while TOTP is off, has no clear, and returns the stored cell (or None when it wrote nothing). enable_totp takes a required expected_secret_cell and writes only while the row still holds that cell and TOTP is off. Both are the shared totp_stage / totp_enable UserChange, so SQLite, Postgres and SQL Server get the same compare-and-set. The stage also forgets last_totp_step, as a disable does. confirm_mfa_enrollment enables against the cell it verified; a begin from another session inside the window now fails the confirm closed (no factor, no codes, rotated session ended), audited reason secret_replaced (write_lost when the cell is unchanged). begin_mfa_enrollment refuses when the stage writes nothing. provision_first_administrator enables with the cell it staged. Tests: a hooked interleaving and a live asyncio race (both red without the fix), an unrelated-write control, store contract on all backends, two provision refusals. Proposed PR title: TOTP enable binds the verified secret: compare-and-set on the staged cell (BACKLOG #3025) Proposed banner: BUILT, not merged. Engine branch b259-3025: the enable is a compare-and-set on the staged totp_secret cell on all three stores, the stage is conditional on TOTP off, a re-stage inside the confirm window fails closed with reason secret_replaced. Closing steps 1-3 built; step 4 answered by reading (passkey challenge keyed per session, persisted credential is the verified one); step 5 (ADR 0207) and the SECURITY.md sentence are not this branch's.
…de stays spent (BACKLOG #3025) Round 1 made totp_stage clear last_totp_step. Round 2 found that this undoes provision_first_administrator's consume-before-stage: the code typed at the terminal would pass the first sign-in again. The clear is reverted, and a test pins the provisioning step as spent (red with the clear). The replace-race retry test goes back to a later step. The confirm docstring and comment now list refusal causes as examples. Proposed PR title: TOTP enable binds the verified secret: compare-and-set on the staged cell (BACKLOG #3025) Proposed banner: BUILT, not merged. Engine branch b259-3025: the enable is a compare-and-set on the staged totp_secret cell on all three stores, the stage is conditional on TOTP off, and a re-stage inside the confirm window fails closed with reason secret_replaced. Closing steps 1-3 are built. Step 4 is answered by reading: the passkey challenge is keyed per session, and the persisted credential is the verified one. Step 5 (ADR 0207) and the SECURITY.md sentence are not this branch's.
…ename (BACKLOG #3069) _refresh_cached_username now skips the write when the row holds neither the caller's old name nor its new one (Manager ruling, batch 259): INFO log citing #3069, no audit row, no conflict, no notice. The check runs after the already-applied no-op and before the held pre-check. Tests: the pinning test is inverted into an end-to-end pass test (plan, sign-in rename, apply) plus a direct test that the skip precedes the held pre-check. The #2291 parametrized test now has the row hold the plan's old name, since the interim-name shape is skipped upstream.
SECURITY.md: the enrolment confirm text no longer says a re-stage inside the confirm window is not caught; it names the compare-and-set on the staged totp_secret cell and all five refusal reasons, adding secret_replaced and write_lost. ADR 0171 Amendment B gets a dated correction note on set_totp_secret / enable_totp. ADR 0207 (Proposed) gets only a dated note on Context item 6. Batch 259, stacked with BACKLOG #3069. Proposed PR title: A stale reconciler rename plan no longer overwrites a newer sign-in rename (BACKLOG #3069), and docs for the #3025 TOTP enable Proposed banner: #3069 BUILT -- _refresh_cached_username skips a rename whose row holds neither the plan's old nor new name (INFO, no audit, no notice), before the held pre-check; pinning test inverted, #2291 shapes rebuilt. Branch b259-3069, stacked on b259-3025.
…G #3069) code-review at xhigh found the skip also stopped a sign-in, whose name comes from the bind it just made, so a pass that wrote an older name first would win. _refresh_cached_username takes from_plan (True only from the reconciler); the login still writes. New test pins the login path; the #2291 test keeps its interim-name premise, driven as the login caller. INFO line now names the discarded name; comments no longer claim the skip orders writers or is a version check. _rename_audits filters by action with no 50-row cap. SECURITY.md names a store-key rotation as a secret_replaced cause (BACKLOG #3025 doc part). Proposed PR title: A stale reconciler rename plan no longer overwrites a newer sign-in rename (BACKLOG #3069), and docs for the #3025 TOTP enable Proposed banner: #3069 BUILT -- a reconciler rename whose row holds neither the plan's old nor new name is skipped (INFO, no audit, no notice) before the held pre-check; sign-ins still write. Pinning test inverted, login path pinned. Branch b259-3069, stacked on b259-3025.
…#3069) code-review round 2 at xhigh. from_plan has no default, so each caller states its policy; the login passes False. The skip comment is the one statement of the rule and names its residuals (a skipped plan can hold the newer name; away-and-back; the login can still write an older name). The INFO line no longer claims the plan was older. Tests now pass from_plan as the caller they describe; the race test runs one of each; the #2291 pre_check case runs as the reconciler, the only caller that reaches it. SECURITY.md (BACKLOG #3025 doc part): fixed the pronoun and wrote the reason list as an inclusion, not a closed set. Proposed PR title: A stale reconciler rename plan no longer overwrites a newer sign-in rename (BACKLOG #3069), and docs for the #3025 TOTP enable Proposed banner: #3069 BUILT -- a reconciler rename whose row holds neither the plan's old nor new name is skipped (INFO, no audit, no notice) before the held pre-check; sign-ins still write; from_plan is a required keyword. Residuals named in code (name heuristic, not an ordering). Branch b259-3069, stacked on b259-3025.
…070) _refresh_cached_username trusted the holder row its caller read. The reconciler reads held before the method's own pre-read of the row, so a holder that let the name go in between filed a pre_check conflict audit and a lockout warning nobody faced. A held naming another row is now re-read after that pre-read; only a row holding the name now is refused, and the audit names it. Otherwise the rename proceeds. The pre_check discriminator, the #3069 stale-plan skip and the compare-and-set are unchanged. Docstring no longer says held is passed rather than re-read. Tests: a stale held whose holder released the name writes, audits and notifies once with no warning; the control, where another row holds it now, refuses and audits that row. Both red without the fix. Proposed PR title: Auth: a stale held read no longer files a false username-refresh conflict (BACKLOG #3070) Proposed banner: SHIPPED -- _refresh_cached_username re-reads the holder before refusing, so a stale held files no conflict or lockout warning (BACKLOG #3070).
…OG #3070) The re-read used _load_user_by_username, which returns None for a row failing its seal, so such a holder read as "nobody holds the name" and a true lockout became a "retry next pass" warning. It now uses _lookup_by_username and refuses on found.refused, naming the holder the caller verified. The #3070 rule is stated once, at the re-read; the docstring points there and says the holder of new_username is re-read, not held's row. The cost note is corrected. Tests added: the real reconciler pass with a holder that lets the name go between the pass's held read and the refresh (red on the base); this row taking the name between the pre-read and the re-read (no self-conflict; red on the base); a seal-failing holder still refused.
…ACKLOG #3025) The refusal revokes the session and the code's TOTP step is already consumed, so a plain retry gets a 401. The text said "a retry confirms". It now says: sign in again and confirm with a code from a later step. The reason sentence gets an antecedent, and the two long sentences near it are split.
…nguishes names (BACKLOG #3069) - _refuse: restore why each refusal passes the name the row held, not old_username. On the login path the two can differ, and passing old_username would bring back the BACKLOG #2291 defect. - The pre-read block no longer restates the #3069 skip rule; it points at the one statement at the skip. - The skip comment and its INFO line no longer claim the next pass re-plans: a pass probes least-recently-probed accounts first, capped at ad_session_recheck_max_users, so the label can lag several passes. - confirm_mfa write_lost comment (BACKLOG #3025): a plain retry gets a 401, because the session is revoked and the step consumed. - test_a_refused_rename_is_audited_under_the_name_the_row_holds: the pre_check case now runs with the row on jdoe-interim, so its actor check distinguishes the names. Mutating the pre_check refusal to pass old_username turns that case red. The docstring says the case pins the method's contract, since no production caller reaches it today. Proposed PR title: A stale reconciler rename plan no longer overwrites a newer sign-in rename (BACKLOG #3069), with the TOTP confirm text for BACKLOG #3025 Proposed banner: SHIPPED -- the reconciler skips a rename plan whose row a sign-in renamed since (from_plan required on _refresh_cached_username); residuals named once at the skip (BACKLOG #3069)
Round 1's seal special-case rested on a false premise and could name a holder that had let the name go; _UserLookup.refused also fires when the holder is deleted during its seal check. A seal-failed holder reads as None at all four holder reads in this path, so that is one gap, named at the re-read as NOT CLOSED rather than half-closed on one read. The re-read is _load_user_by_username again. The docstring now points at the pre-check rather than restating it. Tests: the seal stub test is dropped; the self-rename hook fires once; the pass test counts its by-name reads so it pins held read, then re-read. BACKLOG #3070 Proposed PR title: Auth: a stale held read no longer files a false username-refresh conflict (BACKLOG #3070) Proposed banner: SHIPPED -- _refresh_cached_username re-reads the holder of the new name before a pre_check refusal, so a stale held files no conflict or lockout warning, and the audit names the current holder (BACKLOG #3070).
…ep-up (BACKLOG #3025) Review round on 1c90908. The paragraph already says a refused confirm ends the session with a 401, so the added sentence restating it (and crediting the spent step with the 401, which it does not cause) is dropped. The recovery after write_lost or a store-key rotation now names the step-up any confirm passes, and why a later step's code is needed.
…ce (BACKLOG #3069) - _refuse rationale: the one exception is the row_gone refusal at the pre-read; the post-write row_gone passes the name the row held. - The write_lost comment no longer credits the consumed step with the 401 and points at the ROTATE BEFORE ENABLING comment for recovery rather than restating it (BACKLOG #3025). - The no-holder WARNING and the first-writer-wins comment no longer promise the next pass: a later sign-in, or a pass that probes the account, copies the name down. - The pre_check test docstring no longer says the reconciler never reaches the pre-check; it says which shape is unreachable. - New test_a_lost_enable_write_ends_the_session_and_confirms_after_a_ fresh_sign_in pins the write_lost reason, the ended session, the spent step and the later-step recovery (BACKLOG #3025). Proposed PR title: A stale reconciler rename plan no longer overwrites a newer sign-in rename (BACKLOG #3069), with the TOTP confirm text for BACKLOG #3025 Proposed banner: SHIPPED -- the reconciler skips a rename plan whose row a sign-in renamed since (from_plan required on _refresh_cached_username); residuals named once at the skip (BACKLOG #3069)
…efusing (BACKLOG #3070) Merge origin/main (9f279f1) into b260-3070. The branch was stacked on b259-3069, which landed on main squashed inside engine PR 2358. Every conflicted and auto-merged file was taken from main, and the index was confirmed equal to origin/main before #3070's net change (b259-3069 tip 3aba14d to b260-3070 tip f88fae5) was re-applied. The diff against main is now only messagefoundry/auth/service.py and tests/test_ad_session_reconcile.py. The change: when the caller's `held` names another row, the pre-check re-reads the holder with _load_user_by_username(new_username), and only a row that holds the name now is refused; the refusal and the audit name that fresh holder. Four new tests, red without the fix.
…se matches the outcome (BACKLOG #3070) - The #3070 test helper passes audit_soft=True, as the reconciler has since #3018 merged in from main; it ran the hard audit path before. - The self-rename test counts every holder read; the pass test asserts no WARNING at all, not only lockout text. - The NOT CLOSED note says what a seal-failed holder actually produces (a false "no row holds the name, retries" refusal), that the login's caller read keeps the seal state, and "at least" for the reads it lists. - The held docstring says held=None skips the pre-check instead of calling held a hint. - changelog.d/3070.fixed.md added.
…efusing (BACKLOG #3070) Review round 2 on the main merge: the held docstring states the login caller's None once, and the changelog fragment says the rename (not the holder) is refused and names the seal-failed holder it does not catch. This is the branch's final commit; its subject is the proposed PR title. The branch adds over main only messagefoundry/auth/service.py, tests/test_ad_session_reconcile.py and changelog.d/3070.fixed.md.
|
QA for head e73eb51 (the Builder's line, posted by the Manager): QA -- korus roles/BUILDER.md step 11 / Level: xhigh, from the brief. Tag: none returned. / Rounds: 2. Findings: round 1 found a real merge miss (the test helper omitted |
|
Manager, on the red Check-run 117802090813 reports 15 anchors broken, every one in |
|
Lander, 2026-10-18: SENT BACK at e73eb51. Do not enqueue this head. Two of the open findings make this path worse than main, so they are fixed here rather than filed:
Findings 1, 2, 5, 6, 7, 8, 10 and 11, and finding 3's pre-existing sites, go to the ledger as rows in the Lander's next pass. None of them changes behaviour or weakens a control against main. Post the new head with a QA line naming it. This is a Lander decision under the owner's Driver delegation. |
…OG #3070) Lander send-back on PR 2428, findings 4 and 3 (the new site only). Finding 4: the held re-read in _refresh_cached_username now uses _lookup_by_username. A holder that fails its seal at the re-read is refused as pre_check with the lockout warning, naming the verified held id, instead of falling through to the write_noop "retries" line. Finding 3: a store error on that one re-read is caught (_session_write_errors), logged with the account id and the error class, and the rename is skipped for this pass, so the pass applies the rest. Tests: a keyed-store seal case and a two-rename pass with a faulting re-read; the two existing hooks now patch _lookup_by_username.
…lone (BACKLOG #3070) The holder re-read's store-fault skip now applies only on the reconciler's path (audit_soft); any other caller re-raises, pinned by a control test. The error tuple is built at construction beside the audit one, so the comment's premise holds. The comment names the refused residual (a holder deleted mid-read), the changelog no longer claims more than the code does, and the keyed test setup closes its store on a setup failure.
…tches (BACKLOG #3070) The store-fault skip line now names the holder the first read saw, so a skipped rename still points an operator at the row. The audit_soft docstring says it also gates that skip, and the changelog no longer says a holder seal-failed at the first read files no conflict: it files a write_noop conflict naming no holder.
|
Manager: the send-back is repaired. The new head is 60339cf, three fast-forward commits from e73eb51: 585852d (the two fixes), 0beae23 (review round 1), 60339cf (review round 2). The Builder's Finding 4. The Finding 3, the new site only. That one re-read catches the store's error tuple ( Mutation: removing the Checks: ruff clean; mypy linux 361, win32 342, tests 1383, all clean; pytest test_ad_.py, test_auth_service.py, test_user_seal.py: 730 passed. The full suite was not run. QA -- korus roles/BUILDER.md step 11 / Level: xhigh, from the brief. Tag: none returned. / Rounds: 2. Findings: round 1 fixed four (the fault skip gated to the reconciler's path; the error tuple warmed at construction; changelog overclaims; keyed-store cleanup on setup failure); round 2 fixed three (the skip line names the first read's holder; the audit_soft docstring; a false changelog claim); 9 open, listed below. Open findings, for rows. None of them is worse than main, by the Builder's reading, which the Manager checked:
|
|
Lander: enqueued pinned at 60339cf. All required contexts completed successfully on this head. Review proof: the QA comment naming this head (issuecomment-6256082647, code-review at xhigh, 2 rounds). The send-back's findings 3 (new site) and 4 are fixed here with tests. The other open findings are filed as vault #3862, in open vault PR 2530. |
github-actions
Bot
added
the
ci-red
|
CI failed in Its own head can still be green: the queue revalidates the merge, and the path gates that skip on a pull request run there. Read the run before retrying. https://github.com/MEFORORG/MessageFoundry/actions/runs/39177122501 |
|
Lander: the queue ejected this PR on run 39177122501, test (windows-2025): 1 failed of 42503, tests/test_upload_api.py::test_a_deadline_during_the_create_row_still_writes_it (list_files empty after _settle). This PR's diff (auth/service.py _refresh_cached_username, tests/test_ad_session_reconcile.py, a changelog fragment) cannot reach the upload deadline path. Engine 2375's ubuntu web console leg showed the same shape earlier tonight in test_ui_dr_outlive.py, which then passed on re-run: a deadline-outlive test asserting before the outliving work lands, on a loaded runner. Re-enqueued pinned at 60339cf as the one rerun. A second red on this leg is a finding. The flaky test family goes to the ledger as a row. |
1 participant
Batch 260 (auth), the last open row: BACKLOG #3070 (vault ledger). Opened by the Manager adopting the batch 260 handoff (Manager cca95a, stopped on a usage limit).
What changes
_refresh_cached_usernameinmessagefoundry/auth/service.pynow re-reads the holder of the new username with_load_user_by_username(new_username)when the caller'sheldnames another row, before it refuses aspre_check. So a staleheldfiles no conflict and no lockout warning, and the refusal and its audit row name the current holder.messagefoundry/auth/service.py: the re-read (+38/-10).tests/test_ad_session_reconcile.py: 4 new tests. All 4 fail with main'sservice.py; this was checked twice, after the merge and after review round 1.changelog.d/3070.fixed.md: added in review round 1, because CONTRIBUTING step 4 asks for a fragment when an audit row operators see changes.The branch was stacked on batch 259's
b259-3069, which reached main squashed in engine PR 2358. The Builder merged main (53a6241) and took main's version of every file. It confirmedgit diff --cached --stat origin/mainwas empty, which proves all of b259's content is on main, then re-applied #3070's net diff (3aba14d to f88fae5) cleanly. The net diff against main is the three files above.Merge danger
Reversible: yes. No schema or migration change. Blast radius: the directory reconciler's rename path only.
Checks (Builder, on the final tree)
service.pyand the changelog.test_ad_session_reconcile.pyandtest_ad_directory_identity.py: 301 passed. Other AD and auth suites: 443 passed.git merge-tree --write-tree origin/main e73eb5115exited 0 at main 9f279f1.Open findings, for the Lander to file as rows (code review at xhigh, 2 rounds)
_lookup_by_username, which keeps.refused, is the existing helper for this.held, which closes staleness in both directions.heldnames another row.pre_checklabel now also covers a holder that changed between reads, which points to concurrent writers.held.idis not recorded.held: UserRecord | Noneis now used only for its id.Proposed banner
SHIPPED -- _refresh_cached_username re-reads the holder of the new name before a pre_check refusal, so a stale held files no conflict or lockout warning, and the audit names the current holder (BACKLOG #3070).