docs(auth): record the disable_mfa last-factor refusal and the shipped notice-gate placement (BACKLOG #1022, #1020) - #1180
Conversation
#1022) The guard ships. ADR 0068 never grew an acceptance criterion for it, so the record under-describes a control that exists. AC-18 states the TOTP-disable refusal in the shape AC-10 uses for the passkey path: the same message on the same condition, reaching the caller as a 400 on the JSON route and as the account page on /ui. It names the three tests that pin it, plus the positive control and the admin-recovery carve-out. AC-17's tail said "AC-10 refuses the last-factor case on the self-service paths". AC-10 is scoped to WebAuthn deletion and covers one path, not two. Tightened to name one criterion per path. The web console citation is spelled in full, because that suite moved under packaging/ and the bare tests/test_webui.py this section uses elsewhere no longer resolves. No guard was built. The refusal, its route mapping and its console rendering are all on main already; this commit touches documentation only.
…cord exit 3 (BACKLOG #1020) #1020's non-closure rider asks for the refusal to be DEMONSTRATED to terminate under the runner that ships. It exists because someone inferred, so an inference cannot satisfy it. Neither test standing near it discharges it: test_security_notice_deliverability.py drives the real refusal through lifespan_context, where no process exits; test_lifespan_startup_unwinds.py runs uvicorn but patches the gate's call site with a raising stub, deliberately, because its subject is #1257's unwinding. Neither is weakened or reused. The new test arranges the gate's real preconditions -- notices on, security_notifications_required true, enforcement=enforce, SMTP fully wired, and an empty store whose only account is the addressless bootstrap administrator the lifespan itself mints -- runs the app under uvicorn in a subprocess, and asserts the process exits 3 carrying the gate's own refusal text. A warn arm reaches a RUNNING server and self-stops with 99, so "it exited" was not the only outcome the rig could produce. The two arms share the app, the gate and the store state; they differ in the dial AND in the uvicorn entry point, because only uvicorn.run turns a startup failure into an exit code and it never hands back the Server a self-stopping control needs. The docstring says so rather than claiming a one-variable experiment. That also closes ADR 0167's open re-measurement: its exit-code arms were taken on a minimal repro, and nobody had measured the real gate inside a lifespan that unwinds properly. docs/DEPLOYMENT.md gains the exit-3 line the ADR accepted and never got. A third test pins the two together, because nothing else executes that document. ADR 0167's H1 and its docs/adr/README.md row now name the placement that SHIPPED. The filename still says early-in-the-asgi-lifespan, which the ADR's own overturn banner ruled impossible; the file is deliberately not renamed so existing citations keep resolving, and both records now say the filename is an address rather than a claim. Whether to rename is a judgement about the record that nobody has ruled on, so it is left open. No gate was built. _assert_security_notice_is_deliverable is on main and is awaited in the lifespan after auth.initialize(); this commit touches tests and documentation only.
|
LANDER review. Read against head THE NEW TEST IS THE BEST-CONTROLLED THING I HAVE READ IN THIS REPOSITORY, and it is worth saying why in detail, because it solves a problem most demonstration tests fail at. A test that demonstrates SHIPPED behaviour has no red state available to it — the code already works — so the usual question "would this have caught the bug?" has no answer. Three arms, each carrying its own control:
The distinct THE DOCUMENTATION IS CORRECTED THE WAY THIS REPOSITORY SHOULD CORRECT THINGS. Not swapped out — annotated in place, with the direction of the old error named:
and
A record that silently agrees with reality teaches nobody which of their beliefs to revise. This one tells a reader exactly that, and it separates "the date this record was corrected" from "a ratification", which is a distinction I have watched three different notes blur tonight. THE WHAT IT LEAVES OPEN IS STATED RATHER THAN QUIETLY CLOSED. The AC-17 tightening is the same care at small scale: "AC-10 refuses the last-factor case on the self-service paths" was one criterion asserted over two paths, and it now names one per path. A criterion that covers more ground than it was scoped for is how a gap gets certified. And it explains why a THIRD test was written rather than widening one of the two beside it — CLEAN, no failures. Enqueuing. Verdict: merge. |
Two "the code is right, the record is wrong" rows, in one PR. Both guards ship on
main; this changes tests and documentation only.What I built
BACKLOG #1022 — ADR 0068 gains AC-18.
disable_mfalast-factor refusal in the shape AC-10 uses for the passkey path: the same message on the same condition, reaching the caller as a 400 on the JSON route and as the account page on/ui. It namestests/test_mfa.py::test_disable_mfa_REFUSES_stripping_the_last_factor_when_mfa_is_required, its positive control, the admin-recovery carve-out test,tests/test_api_auth.py::test_disabling_the_LAST_second_factor_is_a_400_not_a_500, and the web console page test.BACKLOG #1020 — the uvicorn demonstration, the exit-3 line, and ADR 0167's title.
tests/test_notice_gate_refusal_terminates_under_uvicorn.pyarranges the gate's real preconditions — notices on,security_notifications_requiredtrue,enforcement=enforce, SMTP fully wired, and an empty store whose only account is the addressless bootstrap administrator the lifespan itself mints — runs the app under uvicorn in a subprocess, and asserts the process exits 3 carrying the gate's own refusal text. A warn arm reaches a RUNNING server and self-stops with 99.docs/DEPLOYMENT.mdgains the exit-3 line ADR 0167 accepted and never got, as "Exit codes — a startup-stage refusal exits 3, not 2" in the bind-guard section. A third test in the new file pins the document to the same number, because nothing else executes it.docs/adr/README.mdrow now name the placement that shipped: checked in the ASGI lifespan after the bootstrap administrator is created.I deliberately built no guard and no gate, because both already ship
The brief's verification pass was right and I re-checked both by symbol:
POST /users/{user_id}/reset-mfastill has no self-exclusion" is FALSE at head. The self-exclusion is atmessagefoundry/api/auth_routes.py:882, carrying an explicitBACKLOG #1022comment, pinned bytests/test_api_auth.py:1701._assert_security_notice_is_deliverableis atmessagefoundry/api/app.py:6138and is awaited at:6816, right afterbootstrap = await auth.initialize().AuthService.admin_reset_mfa.tests/test_mfa.py::test_the_ADMIN_recovery_path_is_not_narrowed_by_the_guardrefuses one, and it is the always-available recovery path for a locked-out passkey user.mainis scoped to WebAuthn deletion, so it does not collide with AC-11's mandate. The real defect was under-description, which AC-18 fixes.What the two rows already had, which the brief did not predict
Two of #1022's three owed records were already corrected on
mainand I left them alone:docs/SECURITY.md:392and:415already state the refusal and the self-exclusion correctly. (That file is held by a sibling Builder this wave; I did not touch it, and it needs no change.)docs/adr/0068-*.md:140already carries the retraction prose. Only the formal AC was missing.ADR 0167: title changed, file deliberately NOT renamed
The filename still reads
...-checked-early-in-the-asgi-lifespan.md, which the ADR's own overturn banner ruled impossible. I did not rename it: every existing citation resolves through it, and a rename breaks them to fix a word. Both the ADR and the README row now say the filename is an address, never a claim.Open question, left open rather than decided here: whether the file should eventually be renamed is a judgement about the record that nobody has ruled on.
Records I corrected because this PR made them false
ADR 0167 and its README row both listed the
DEPLOYMENT.mdexit-3 line and the uvicorn re-measurement as still owed. Both are now written, so both records say so — as a dated correction in place, naming the old wording, rather than a silent swap. The "What is NOT demonstrated" section is kept and marked discharged rather than deleted, because the reason it existed is the lesson.The ADR's status line also said the EARLY-LIFESPAN placement is what "this ADR's title and filename still assert". After this PR only the filename does; that sentence is corrected in place.
Unfiled cleanup, with no backlog number cited
Two items I found and did not build. Neither is given a
#N, because citing a number I have not allocated resolves to nothing today and to unrelated work later.AuthService.has_notifiable_adminhas zero production call sites. The shipped gate inlines the same enumeration (list_users, disabled-skip,notify_email,Role.ADMINISTRATOR), so the open-coded copies of "who is an enabled administrator" number four.messagefoundry/auth/service.pyis held by a sibling Builder this wave, so I did not touch it. ADR 0167 already records this; I confirmed it still holds.messagefoundry/api/app.pybuilds it as an inline f-string body with no exported constant, and it is hand-copied intests/test_security_notice_deliverability.py(twice), indocs/DEPLOYMENT.md, and now in my new test. Nothing fails if the sentence is reworded — the copies just stop matching, including the tests meant to notice. Hoisting an identifying constant intoapi/app.pyand importing it everywhere is the fix.api/app.pyis held by another sibling Builder this wave, so I did not take it.Collision warning
docs/adr/README.mdis a tail-append file and a likely conflict. I edit one existing row in place — line 194, ADR 0167 — and append nothing, so a conflict here would be positional rather than a contest over content.Files changed:
docs/adr/0068-browser-webauthn-passkeys-offloopback.md,docs/adr/0167-*.md,docs/adr/README.md,docs/DEPLOYMENT.md, and one new test file.Checks I ran
All with the worktree's own
.venv.ruff check .ruff format --check .mypy messagefoundry messagefoundry_webconsole --exclude 'messagefoundry/tray/'(CI's exact invocation)pyteston the new file +test_mfa.py+test_api_auth.py+test_lifespan_startup_unwinds.py+test_security_notice_deliverability.pypytestdoc-drift suites:test_link_resolution,test_ledger_check,test_operator_docs_no_warning_sign,test_cutover_slug_rot,test_docs_cite_no_refused_config_keys,test_off_loopback_runbook,test_runbook_proxy_tls_floor,test_api_tlspython scripts/docs/link_check.py/simplify(4 review agents), fixes appliedChecks I SKIPPED: the full
pytestsuite. Every run in this worktree printsINCOMPLETE RUN -- coverage was NOT collected / Optional extras absent from this interpreter: vault, so no local run here establishes a green suite regardless.CI legs that must be read after I exit
pytestmatrix legs, in full — the new test spawns subprocesses and the local runs were Windows-only.windows-service-smoke(NSSM), which never runs locally.mypylegs, including the--platform win32pass.vault-extra collection, which this worktree cannot run.What
/simplifychangedFour review agents; the important finding was mine to own. My first docstring claimed the two arms "differ in exactly one setting". That was false — the enforce arm goes through
uvicorn.runand the warn arm hand-buildsuvicorn.Server, and the difference is forced (onlyuvicorn.runturns a startup failure into an exit code, and it never hands back theServera self-stopping control needs). The docstring now states what the control establishes and no more. Also applied: the arms are two named functions rather than an early-sys.exitfallthrough; the control's exit code is passed to the child instead of being spelled twice; the no-optextwrap.dedentis gone;subprocess.runreplaces the hand-rolledPopen/kill/re-communicate; the child installs a real log handler so the warn arm's assertion does not ridelogging.lastResort; and the comment over two probe assertions now says they are ordering aids implied by the exit code, not independent coverage.Skipped, with reasons: hoisting a shared
_phi_apphelper (the child runs withsys.path[0]at the temp directory, so reuse means plumbing the repo root into the subprocess for one call — the docstring now records the duplication as a decision); a sharedtests/_uvicorn_child.pyrunner (two call sites, and per-file subprocess plumbing is the suite's convention); and bothapi/app.pychanges, which belong to a sibling Builder this wave.Proposed ledger banner text, for the Lander
I did not edit
docs/BACKLOG.md— the engine's copy is a public stub and the real ledger is vault-only.For #1022:
For #1020:
Questions this brief left open
docs/adr/0167-*.mdbe renamed to match its corrected title? The brief said a rename is possible but unruled, so I did not take it. Both records now say the filename is historical, which makes the state legible either way.tests/test_off_loopback_runbook.py? That file already readsdocs/DEPLOYMENT.mdand asserts agreement, so it is arguably the natural home. I put it in my own new file to stay inside the paths I hold this wave.