From c19b90fbb138be7bb1adb6fc6daa094375766ce0 Mon Sep 17 00:00:00 2001 From: Scott Hall Date: Fri, 16 Oct 2026 17:21:57 -0500 Subject: [PATCH 01/18] TOTP enable is bound to the secret the confirm verified (BACKLOG #3025) 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. --- messagefoundry/auth/service.py | 61 ++++- messagefoundry/store/base.py | 29 +- messagefoundry/store/postgres.py | 26 +- messagefoundry/store/sqlserver.py | 32 +-- messagefoundry/store/store.py | 75 ++++-- .../tests/test_ui_oidc_step_up.py | 8 +- .../tests/test_webui.py | 6 +- tests/_lockout_store_contract.py | 42 +-- tests/_webauthn_store_contract.py | 108 +++++++- tests/test_admin_reset_totp.py | 18 +- tests/test_approver_provenance.py | 6 +- tests/test_auth_hardening.py | 18 +- tests/test_docs_security_pathways.py | 6 +- tests/test_generated_credential_failure.py | 6 +- tests/test_mfa.py | 252 +++++++++++++++++- tests/test_oidc_json_step_up_hint.py | 6 +- tests/test_provision_first_administrator.py | 86 +++++- 17 files changed, 646 insertions(+), 139 deletions(-) diff --git a/messagefoundry/auth/service.py b/messagefoundry/auth/service.py index da69512cfb..2599c116e7 100644 --- a/messagefoundry/auth/service.py +++ b/messagefoundry/auth/service.py @@ -3112,15 +3112,27 @@ async def provision_first_administrator( # Enrolled BEFORE the role, which stays last, so every interruption still leaves a # roleless row a re-run completes -- and the repair branch above clears a half-enrolled # one before this runs again. - await self._store.set_totp_secret(user_id, secret=totp_secret) + # Conditional (BACKLOG #3025): another run that enabled TOTP on this row first makes + # the stage write nothing. The enable below is bound to the cell this run staged. + staged_cell = await self._store.set_totp_secret(user_id, secret=totp_secret) + if staged_cell is None: + raise FirstAdministratorRefused( + "the authenticator enrolment was changed while this command ran, so this run " + "did not stage its authenticator and granted no role; remove the authenticator " + "entry it showed. If another provision-admin run is in progress, let it " + "finish. Then run the command again" + ) plain_codes = tuple( totp.generate_recovery_codes(self._settings.mfa_recovery_code_count) ) hashes = [await self._argon2(hash_password, c) for c in plain_codes] - # Conditional (BACKLOG #2224): a factor clear by another run's repair branch, or another - # run that enabled TOTP first, since the secret was staged above makes it match no row. - # Refused before the role, so the row stays roleless and a re-run completes it. - if not await self._store.enable_totp(user_id, recovery_code_hashes=hashes): + # Conditional (BACKLOG #2224, #3025): a factor clear by another run's repair branch, + # another run that enabled TOTP first, or another run that staged its own secret, since + # the secret was staged above makes it match no row. Refused before the role, so the + # row stays roleless and a re-run completes it. + if not await self._store.enable_totp( + user_id, recovery_code_hashes=hashes, expected_secret_cell=staged_cell + ): raise FirstAdministratorRefused( "the authenticator enrolment was changed while this command ran, so this run " "did not turn TOTP on and granted no role; remove the authenticator entry it " @@ -10038,10 +10050,17 @@ async def begin_mfa_enrollment(self, identity: Identity) -> MfaEnrollment: user = await self._load_user(identity.user_id) if user is None: raise ValueError("no such user") + already_enabled = "MFA is already enabled; disable it before re-enrolling" if user.totp_enabled: - raise ValueError("MFA is already enabled; disable it before re-enrolling") + raise ValueError(already_enabled) secret = totp.generate_secret() - await self._store.set_totp_secret(identity.user_id, secret=secret) + # Conditional (BACKLOG #3025): a confirm that enabled TOTP after the check above makes the + # stage write nothing, so a begin never replaces the secret of an enabled factor. Nothing + # written can also mean the row is gone, which is told apart by reading it again. + if await self._store.set_totp_secret(identity.user_id, secret=secret) is None: + if await self._load_user(identity.user_id) is None: + raise ValueError("no such user") + raise ValueError(already_enabled) await self._audit("auth.mfa_enroll_started", actor=identity.username) return MfaEnrollment(secret=secret, otpauth_uri=totp.otpauth_uri(secret, identity.username)) @@ -10058,8 +10077,9 @@ async def confirm_mfa_enrollment( (or a time-step already consumed -- single-use, BACKLOG #1021) elevates nothing and carries none. A good code on a session revoked before the rotation returns ``session_lost`` with MFA still OFF (BACKLOG #1902), so a lost session never leaves MFA on with codes nobody saw. So - does a good code whose activation matches no row, because a reset cleared the staged secret - or a second confirm enabled first (BACKLOG #2224). That confirm ends the session it rotated, + does a good code whose activation matches no row, because a reset cleared the staged secret, + a second confirm enabled first (BACKLOG #2224), or a second begin staged a secret this + confirm never verified (BACKLOG #3025). That confirm ends the session it rotated, hands back no codes, and audits ``auth.mfa_enroll_refused``. This is one of the two legs that turn an MFA-pending session into an MFA-satisfied one for a @@ -10076,6 +10096,10 @@ async def confirm_mfa_enrollment( secret = self._totp_secret(user) if not secret: raise ValueError("no enrollment in progress") + # The stored cell this code is checked against. The enable below turns TOTP on only while + # the row still holds this cell (BACKLOG #3025), so a begin from another session that + # stages a different secret inside the ceremony cannot decide which factor is enabled. + verified_cell = user.totp_secret_cell if await self._enrolment_too_early( token, user, arrived_at, event="auth.mfa_failed", client=client ): @@ -10124,10 +10148,13 @@ async def confirm_mfa_enrollment( ) if elevation.token is None: # not ``ok``, spelled so mypy narrows the token below return elevation - if not await self._store.enable_totp(identity.user_id, recovery_code_hashes=hashes): - # BACKLOG #2224: the conditional enable matched no row, because an administrator's - # reset cleared the staged secret inside the rotation window, or a second confirm - # enabled TOTP first. This confirm turns nothing on and its codes are dropped. The + if not await self._store.enable_totp( + identity.user_id, recovery_code_hashes=hashes, expected_secret_cell=verified_cell + ): + # BACKLOG #2224, #3025: the conditional enable matched no row, because an + # administrator's reset cleared the staged secret inside the rotation window, a second + # confirm enabled TOTP first, or a second begin staged a secret this confirm never + # verified. This confirm turns nothing on and its codes are dropped. The # rotated session was stamped MFA-verified against a factor that is gone or not this # ceremony's, so it is ended rather than handed back: ``session_lost``, the same # fail-closed answer as a session revoked before the rotation. @@ -10143,8 +10170,14 @@ async def confirm_mfa_enrollment( reason = "account_gone" elif after.totp_enabled: reason = "already_enabled" - else: + elif after.totp_secret_cell is None: reason = "secret_cleared" + elif after.totp_secret_cell != verified_cell: + reason = "secret_replaced" + else: + # The row still holds the verified cell with TOTP off, so the write lost to some + # other concurrent write to the row; a retry confirms. + reason = "write_lost" await self._audit( "auth.mfa_enroll_refused", actor=identity.username, diff --git a/messagefoundry/store/base.py b/messagefoundry/store/base.py index a089041088..5452d24187 100644 --- a/messagefoundry/store/base.py +++ b/messagefoundry/store/base.py @@ -2263,9 +2263,13 @@ async def remember_login_address(self, user_id: str, address: str, *, now: float async def forget_login_addresses(self, user_id: str, *, before: float) -> None: ... # --- MFA: native TOTP second factor (local accounts, WP-14) -------------- + # CONDITIONAL (BACKLOG #3025): stages ``secret``, sealed, only where TOTP is still off, and + # returns the stored cell, or None when nothing was written. A caller must refuse on None, and + # passes the returned cell to ``enable_totp`` as ``expected_secret_cell``. There is no clear: a + # factor, staged or enabled, is removed with ``disable_totp``. async def set_totp_secret( - self, user_id: str, *, secret: str | None, now: float | None = None - ) -> None: ... + self, user_id: str, *, secret: str, now: float | None = None + ) -> str | None: ... async def get_totp_secret(self, user_id: str) -> str | None: ... @@ -2274,15 +2278,20 @@ async def get_totp_secret(self, user_id: str) -> str | None: ... # second, unchecked read. ``None`` for no cell. No I/O. def open_totp_secret(self, user_id: str, cell: str | None) -> str | None: ... - # CONDITIONAL (BACKLOG #2224): writes only where a secret is staged (``totp_secret IS NOT NULL``) - # and TOTP is still off, in the same UPDATE, and returns whether a row was written. An - # ``admin_reset_mfa`` that clears the secret, or a second confirm that enables first, between - # the caller's checks and this write makes it match no row. A caller must refuse on False and - # never treat MFA as on, the way ``set_password(require_totp=...)`` callers do. It does not - # check that the staged secret is the one the caller verified; a re-stage in the window is - # not caught. + # CONDITIONAL (BACKLOG #2224, #3025): writes only where the staged ``totp_secret`` cell is still + # ``expected_secret_cell`` (the cell the caller verified a code against) and TOTP is still off, + # in the same write, and returns whether a row was written. An ``admin_reset_mfa`` that clears + # the secret, a second confirm that enables first, or a second begin that stages another + # secret, between the caller's read and this write, makes it match no row. ``None`` matches + # nothing. A caller must refuse on False and never treat MFA as on, the way + # ``set_password(require_totp=...)`` callers do. async def enable_totp( - self, user_id: str, *, recovery_code_hashes: list[str], now: float | None = None + self, + user_id: str, + *, + recovery_code_hashes: list[str], + expected_secret_cell: str | None, + now: float | None = None, ) -> bool: ... # ADR 0171 Amendment B (BACKLOG #2226), for `admin-reset-totp`: swaps an ENABLED enrolment's diff --git a/messagefoundry/store/postgres.py b/messagefoundry/store/postgres.py index 3cc1d25830..fc203bdcfa 100644 --- a/messagefoundry/store/postgres.py +++ b/messagefoundry/store/postgres.py @@ -312,6 +312,7 @@ totp_disable, totp_enable, totp_replace, + totp_stage, totp_step_spend, transit_attested_audit, transit_withdrawn_audit, @@ -8887,14 +8888,13 @@ async def set_password_hash( # --- MFA: native TOTP second factor (local accounts, WP-14) -------------- async def set_totp_secret( - self, user_id: str, *, secret: str | None, now: float | None = None - ) -> None: - """Stage (or clear) a user's base32 TOTP secret, store-cipher encrypted. Does not enable MFA.""" + self, user_id: str, *, secret: str, now: float | None = None + ) -> str | None: + """See :meth:`MessageStore.set_totp_secret`: conditional on TOTP being off (#3025).""" now = time.time() if now is None else now sealed = self._enc(secret, aad=cell_aad("users", "totp_secret", user_id)) - await self._write_user_txn( - user_id, lambda r: r.changed(totp_secret=sealed, updated_at=float(now)) - ) + written = await self._write_user_txn(user_id, totp_stage(sealed, now)) + return None if written is None else sealed async def get_totp_secret(self, user_id: str) -> str | None: d = await self._fetchone("SELECT totp_secret FROM users WHERE id=$1", user_id) @@ -8905,13 +8905,17 @@ def open_totp_secret(self, user_id: str, cell: str | None) -> str | None: return self._dec(cell, aad=cell_aad("users", "totp_secret", user_id)) async def enable_totp( - self, user_id: str, *, recovery_code_hashes: list[str], now: float | None = None + self, + user_id: str, + *, + recovery_code_hashes: list[str], + expected_secret_cell: str | None, + now: float | None = None, ) -> bool: now = time.time() if now is None else now - # Conditional, and the result says whether it wrote: see ``totp_enable`` (#2224). - return ( - await self._write_user_txn(user_id, totp_enable(recovery_code_hashes, now)) is not None - ) + # Conditional, and the result says whether it wrote: see ``totp_enable`` (#2224, #3025). + change = totp_enable(recovery_code_hashes, now, expected_secret_cell=expected_secret_cell) + return await self._write_user_txn(user_id, change) is not None async def replace_totp_enrolment( self, diff --git a/messagefoundry/store/sqlserver.py b/messagefoundry/store/sqlserver.py index de45f24c5b..a3632b06dd 100644 --- a/messagefoundry/store/sqlserver.py +++ b/messagefoundry/store/sqlserver.py @@ -305,6 +305,7 @@ totp_disable, totp_enable, totp_replace, + totp_stage, totp_step_spend, transit_attested_audit, transit_withdrawn_audit, @@ -13031,18 +13032,13 @@ async def set_password_hash( # --- MFA: native TOTP second factor (local accounts, WP-14) -------------- async def set_totp_secret( - self, user_id: str, *, secret: str | None, now: float | None = None - ) -> None: - """Stage (or clear) a user's base32 TOTP secret, store-cipher encrypted. Does not enable MFA.""" + self, user_id: str, *, secret: str, now: float | None = None + ) -> str | None: + """See :meth:`MessageStore.set_totp_secret`: conditional on TOTP being off (#3025).""" now = time.time() if now is None else now - enc = ( - self._cipher.encrypt(secret, aad=cell_aad("users", "totp_secret", user_id)) - if secret - else None - ) - await self._write_user_txn( - user_id, lambda r: r.changed(totp_secret=enc, updated_at=float(now)) - ) + sealed = self._enc(secret, aad=cell_aad("users", "totp_secret", user_id)) + written = await self._write_user_txn(user_id, totp_stage(sealed, now)) + return None if written is None else sealed async def get_totp_secret(self, user_id: str) -> str | None: d = await self._fetchone("SELECT totp_secret FROM users WHERE id=?", (user_id,)) @@ -13053,13 +13049,17 @@ def open_totp_secret(self, user_id: str, cell: str | None) -> str | None: return self._dec(cell, aad=cell_aad("users", "totp_secret", user_id)) async def enable_totp( - self, user_id: str, *, recovery_code_hashes: list[str], now: float | None = None + self, + user_id: str, + *, + recovery_code_hashes: list[str], + expected_secret_cell: str | None, + now: float | None = None, ) -> bool: now = time.time() if now is None else now - # Conditional (see ``totp_enable``, #2224); the write helper's OUTPUT says whether it wrote. - return ( - await self._write_user_txn(user_id, totp_enable(recovery_code_hashes, now)) is not None - ) + # Conditional, and the result says whether it wrote: see ``totp_enable`` (#2224, #3025). + change = totp_enable(recovery_code_hashes, now, expected_secret_cell=expected_secret_cell) + return await self._write_user_txn(user_id, change) is not None async def replace_totp_enrolment( self, diff --git a/messagefoundry/store/store.py b/messagefoundry/store/store.py index 9f35ada26e..29a3d570dc 100644 --- a/messagefoundry/store/store.py +++ b/messagefoundry/store/store.py @@ -2635,14 +2635,47 @@ def apply(r: UserAuthzRecord) -> UserAuthzRecord: return apply -def totp_enable(recovery_code_hashes: Sequence[str], now: float) -> UserChange: - """What ``enable_totp`` writes, only where a secret is staged and TOTP is still off (BACKLOG - #2224). The enrolment confirm rotates the session BEFORE it enables TOTP (BACKLOG #1902), so an - ``admin_reset_mfa`` can clear the staged secret inside that window, and a second confirm can - enable first; either way this writes nothing and the caller refuses.""" +def totp_stage(sealed_secret: str | None, now: float) -> UserChange: + """What ``set_totp_secret`` writes: the sealed secret, only where TOTP is still off (BACKLOG + #3025). A begin that lands once TOTP is on writes nothing, so it can never replace the secret of + an enabled factor. There is no clear here: an empty secret raises, and ``disable_totp`` clears. + + ``last_totp_step`` goes too, as in :func:`totp_disable`: the high-water mark belonged to the + secret being replaced, and a new secret has no history. Without that, a confirm refused because a + second begin replaced its secret leaves its spent step behind, and the replacing secret's own + code from the same 30 seconds is refused as a replay.""" + if not sealed_secret: + raise ValueError("a TOTP secret is required") def apply(r: UserAuthzRecord) -> UserAuthzRecord | None: - if r.row.totp_secret is None or r.row.totp_enabled: + if r.row.totp_enabled: + return None + return r.changed(totp_secret=sealed_secret, last_totp_step=None, updated_at=float(now)) + + return apply + + +def totp_enable( + recovery_code_hashes: Sequence[str], now: float, *, expected_secret_cell: str | None +) -> UserChange: + """What ``enable_totp`` writes, only where TOTP is still off and the staged secret is still the + cell the caller verified (BACKLOG #2224, #3025). The enrolment confirm rotates the session BEFORE + it enables TOTP (BACKLOG #1902), so inside that window an ``admin_reset_mfa`` can clear the + staged secret, a second confirm can enable first, or a second begin can stage another secret; + in each case this writes nothing and the caller refuses. + + The compare is on the stored cell, byte for byte. A write to another column of the row carries + the cell over unchanged, so it does not make the compare refuse. At least a stage, a clear, + ``totp_replace`` and a store-key rotation that re-encrypts the column do change it; a rotation + landing inside a confirm makes that confirm refuse, which fails closed and a retry succeeds. + ``None`` matches nothing.""" + + def apply(r: UserAuthzRecord) -> UserAuthzRecord | None: + if ( + expected_secret_cell is None + or r.row.totp_secret != expected_secret_cell + or r.row.totp_enabled + ): return None return r.changed( totp_enabled=True, @@ -14831,18 +14864,18 @@ async def set_user_notify_email( # --- MFA: native TOTP second factor (local accounts, WP-14) -------------- async def set_totp_secret( - self, user_id: str, *, secret: str | None, now: float | None = None - ) -> None: - """Stage (or clear) a user's base32 TOTP secret, store-cipher encrypted at rest. Does **not** - enable MFA — enrollment is confirmed by :meth:`enable_totp` after the user proves a live code. - ``secret=None`` clears the staged secret.""" + self, user_id: str, *, secret: str, now: float | None = None + ) -> str | None: + """Stage a user's base32 TOTP secret, store-cipher encrypted at rest, only where TOTP is still + off (BACKLOG #3025, see :func:`totp_stage`). Does **not** enable MFA — enrollment is + confirmed by :meth:`enable_totp` after the user proves a live code. Returns the stored cell, + which :meth:`enable_totp` compares against, or ``None`` when nothing was written.""" now = time.time() if now is None else now sealed = self._enc(secret, aad=cell_aad("users", "totp_secret", user_id)) async with _writer_guard(self._db, self._lock): - await self._write_user( - user_id, lambda r: r.changed(totp_secret=sealed, updated_at=float(now)) - ) + written = await self._write_user(user_id, totp_stage(sealed, now)) await self._commit() + return None if written is None else sealed async def get_totp_secret(self, user_id: str) -> str | None: """The user's decrypted base32 TOTP secret, or ``None`` when not enrolled/staged.""" @@ -14857,14 +14890,20 @@ def open_totp_secret(self, user_id: str, cell: str | None) -> str | None: return self._dec(cell, aad=cell_aad("users", "totp_secret", user_id)) async def enable_totp( - self, user_id: str, *, recovery_code_hashes: list[str], now: float | None = None + self, + user_id: str, + *, + recovery_code_hashes: list[str], + expected_secret_cell: str | None, + now: float | None = None, ) -> bool: """Activate TOTP for a user (post-confirm), storing the argon2id hashes of their one-time - recovery codes. Writes only where a secret is staged and TOTP is still off, and returns - whether it wrote (BACKLOG #2224, see :func:`totp_enable`).""" + recovery codes. Writes only where the staged secret is still ``expected_secret_cell`` and TOTP + is still off, and returns whether it wrote (BACKLOG #2224, #3025, see :func:`totp_enable`).""" now = time.time() if now is None else now + change = totp_enable(recovery_code_hashes, now, expected_secret_cell=expected_secret_cell) async with _writer_guard(self._db, self._lock): - written = await self._write_user(user_id, totp_enable(recovery_code_hashes, now)) + written = await self._write_user(user_id, change) await self._commit() return written is not None diff --git a/packaging/messagefoundry-webconsole/tests/test_ui_oidc_step_up.py b/packaging/messagefoundry-webconsole/tests/test_ui_oidc_step_up.py index 81454da45f..afa995d2e3 100644 --- a/packaging/messagefoundry-webconsole/tests/test_ui_oidc_step_up.py +++ b/packaging/messagefoundry-webconsole/tests/test_ui_oidc_step_up.py @@ -234,8 +234,12 @@ async def test_a_code_at_the_mfa_gate_for_a_satisfied_oidc_session_opens_nothing token = await _federated_sign_in(service, c, monkeypatch) identity = await service.identity_for_token(token, activity=False) assert identity is not None and await service.mfa_satisfied(token) - await service.store.set_totp_secret(identity.user_id, secret="JBSWY3DPEHPK3PXP") - assert await service.store.enable_totp(identity.user_id, recovery_code_hashes=[]) + staged_cell = await service.store.set_totp_secret( + identity.user_id, secret="JBSWY3DPEHPK3PXP" + ) + assert await service.store.enable_totp( + identity.user_id, expected_secret_cell=staged_cell, recovery_code_hashes=[] + ) r = await c.post("/ui/mfa", data={"code": "000000"}, headers=_SAME, follow_redirects=False) assert r.status_code == 303 and r.headers["location"] == "/ui", r.headers assert c.cookies.get("mf_session") == token, "nothing rotated, so the cookie must not move" diff --git a/packaging/messagefoundry-webconsole/tests/test_webui.py b/packaging/messagefoundry-webconsole/tests/test_webui.py index eebfef122e..c40f15e331 100644 --- a/packaging/messagefoundry-webconsole/tests/test_webui.py +++ b/packaging/messagefoundry-webconsole/tests/test_webui.py @@ -6253,8 +6253,10 @@ async def test_reauth_both_factors_enrolled_renders_code_and_passkey(engine: Eng from messagefoundry.auth import totp as totp_mod secret = totp_mod.generate_secret() - await service.store.set_totp_secret(boss_id, secret=secret) - await service.store.enable_totp(boss_id, recovery_code_hashes=[]) + staged_cell = await service.store.set_totp_secret(boss_id, secret=secret) + await service.store.enable_totp( + boss_id, expected_secret_cell=staged_cell, recovery_code_hashes=[] + ) await _cookie_login(c, "boss") # fresh MFA-pending session boss_creds = await service.store.list_webauthn_credentials(boss_id) diff --git a/tests/_lockout_store_contract.py b/tests/_lockout_store_contract.py index a5a33f218b..6c07b2edd1 100644 --- a/tests/_lockout_store_contract.py +++ b/tests/_lockout_store_contract.py @@ -191,14 +191,18 @@ async def _assert_escalation(store: Any) -> None: now=1.0, password_generated=False, ) - await store.set_totp_secret("lock-totp", secret="JBSWY3DPEHPK3PXP", now=1.0) - assert await store.enable_totp("lock-totp", recovery_code_hashes=[], now=1.0) + staged_cell = await store.set_totp_secret("lock-totp", secret="JBSWY3DPEHPK3PXP", now=1.0) + assert await store.enable_totp( + "lock-totp", expected_secret_cell=staged_cell, recovery_code_hashes=[], now=1.0 + ) # A directory account, with TOTP enrolled too: NEITHER lock escalates (ADR 0197 Decision 5). await store.create_user( user_id="lock-ad", username="lock-ad", auth_provider="ad", now=1.0, password_generated=False ) - await store.set_totp_secret("lock-ad", secret="JBSWY3DPEHPK3PXP", now=1.0) - assert await store.enable_totp("lock-ad", recovery_code_hashes=[], now=1.0) + staged_cell = await store.set_totp_secret("lock-ad", secret="JBSWY3DPEHPK3PXP", now=1.0) + assert await store.enable_totp( + "lock-ad", expected_secret_cell=staged_cell, recovery_code_hashes=[], now=1.0 + ) # A local account with no TOTP: only the second-step lock escalates. await store.create_user( user_id="lock-plain", @@ -422,8 +426,10 @@ async def _assert_conditional_rotation(store: Any) -> None: True, ) # TOTP on: the caller "checks" here and sees it... - await store.set_totp_secret("rot-u1", secret="JBSWY3DPEHPK3PXP", now=2.0) - assert await store.enable_totp("rot-u1", recovery_code_hashes=[], now=2.0) + staged_cell = await store.set_totp_secret("rot-u1", secret="JBSWY3DPEHPK3PXP", now=2.0) + assert await store.enable_totp( + "rot-u1", expected_secret_cell=staged_cell, recovery_code_hashes=[], now=2.0 + ) user = await store.get_user("rot-u1") assert user is not None and user.totp_enabled # ... then an administrator's factor reset lands between the check and the write ... @@ -439,8 +445,10 @@ async def _assert_conditional_rotation(store: Any) -> None: user = await store.get_user("rot-u1") assert user is not None and user.password_hash == "h-issued" and user.password_generated # With TOTP standing, the same write lands. - await store.set_totp_secret("rot-u1", secret="JBSWY3DPEHPK3PXP", now=4.0) - assert await store.enable_totp("rot-u1", recovery_code_hashes=[], now=4.0) + staged_cell = await store.set_totp_secret("rot-u1", secret="JBSWY3DPEHPK3PXP", now=4.0) + assert await store.enable_totp( + "rot-u1", expected_secret_cell=staged_cell, recovery_code_hashes=[], now=4.0 + ) assert await store.set_password( "rot-u1", password_hash="h-chosen", @@ -455,19 +463,15 @@ async def _assert_conditional_rotation(store: Any) -> None: False, False, ) - # TOTP flagged on over a NULL secret is no way past: the conditional write refuses it too - # (review round 2). Since BACKLOG #2224 a confirm racing a factor reset cannot produce this - # state, because ``enable_totp`` is conditional; ``set_totp_secret(secret=None)`` still can. - await store.set_totp_secret("rot-u1", secret=None, now=5.0) + # TOTP flagged on over a NULL secret is no way past (review round 2). Since BACKLOG #2224 a + # confirm racing a factor reset cannot produce that state, because ``enable_totp`` is + # conditional, and since BACKLOG #3025 neither can a stage: ``set_totp_secret`` has no clear and + # writes nothing once TOTP is on, so the enabled secret is left as it was. + before = await store.get_totp_secret("rot-u1") + assert await store.set_totp_secret("rot-u1", secret="JBSWY3DPEHPK3PXP"[::-1], now=5.0) is None user = await store.get_user("rot-u1") assert user is not None and user.totp_enabled - assert not await store.set_password( - "rot-u1", - password_hash="h-second", - password_generated=False, - must_change_password=False, - require_totp=True, - ) + assert await store.get_totp_secret("rot-u1") == before # disable_totp forgets the removed secret's step high-water mark (review round 2): a new # secret's first code in the same 30 seconds is not refused as a replay. assert await store.consume_totp_step("rot-u1", 1_000) diff --git a/tests/_webauthn_store_contract.py b/tests/_webauthn_store_contract.py index f44ee33e6e..86ce9e7f14 100644 --- a/tests/_webauthn_store_contract.py +++ b/tests/_webauthn_store_contract.py @@ -166,20 +166,37 @@ async def _assert_totp_contract(store: Any) -> None: # Secret staging round-trip (cipher-covered column) + clear. assert await store.get_totp_secret("totp-u1") is None - await store.set_totp_secret("totp-u1", secret="JBSWY3DPEHPK3PXP", now=2.0) + first_cell = await store.set_totp_secret("totp-u1", secret="JBSWY3DPEHPK3PXP", now=2.0) + assert first_cell is not None assert await store.get_totp_secret("totp-u1") == "JBSWY3DPEHPK3PXP" - await store.set_totp_secret("totp-u1", secret=None, now=3.0) + await store.disable_totp("totp-u1", now=3.0) assert await store.get_totp_secret("totp-u1") is None # Enable + recovery codes: single-use consumption is atomic (the double-spend guard). - await store.set_totp_secret("totp-u1", secret="JBSWY3DPEHPK3PXP", now=4.0) - assert await store.enable_totp("totp-u1", recovery_code_hashes=["rc1", "rc2"], now=5.0) is True + staged_cell = await store.set_totp_secret("totp-u1", secret="JBSWY3DPEHPK3PXP", now=4.0) + assert ( + await store.enable_totp( + "totp-u1", + expected_secret_cell=staged_cell, + recovery_code_hashes=["rc1", "rc2"], + now=5.0, + ) + is True + ) user = await store.get_user("totp-u1") assert user is not None and user.totp_enabled is True assert set(await store.get_recovery_code_hashes("totp-u1")) == {"rc1", "rc2"} # The enable is conditional (BACKLOG #2224): a second confirm that lands once TOTP is on matches # no row, so it cannot replace the recovery codes the first one handed out. - assert await store.enable_totp("totp-u1", recovery_code_hashes=["loser"], now=5.5) is False + assert ( + await store.enable_totp( + "totp-u1", + recovery_code_hashes=["loser"], + expected_secret_cell=staged_cell, + now=5.5, + ) + is False + ) assert set(await store.get_recovery_code_hashes("totp-u1")) == {"rc1", "rc2"} assert await store.consume_recovery_code_hash("totp-u1", "rc1", now=6.0) is True assert await store.consume_recovery_code_hash("totp-u1", "rc1", now=7.0) is False @@ -201,12 +218,22 @@ async def _assert_totp_contract(store: Any) -> None: # The reset race (BACKLOG #2224): an enable that lands after a factor reset cleared the staged # secret matches no row, so TOTP is never flagged on over a NULL secret. - assert await store.enable_totp("totp-u1", recovery_code_hashes=["late"], now=9.0) is False + assert ( + await store.enable_totp( + "totp-u1", recovery_code_hashes=["late"], expected_secret_cell=staged_cell, now=9.0 + ) + is False + ) user = await store.get_user("totp-u1") assert user is not None and user.totp_enabled is False assert await store.get_totp_secret("totp-u1") is None assert await store.get_recovery_code_hashes("totp-u1") == [] - assert await store.enable_totp("missing-user", recovery_code_hashes=[], now=9.0) is False + assert ( + await store.enable_totp( + "missing-user", recovery_code_hashes=[], expected_secret_cell=staged_cell, now=9.0 + ) + is False + ) # replace_totp_enrolment (ADR 0171 Amendment B, BACKLOG #2226): conditional on TOTP being ON # and on the enrolment instant the caller read, so it can never turn TOTP on for an account @@ -234,8 +261,13 @@ async def audit_rows() -> int: assert await store.get_totp_secret("totp-u1") is None assert await audit_rows() == 0 # On an enabled row it swaps the seed, the codes and the step in one write, with TOTP still on. - await store.set_totp_secret("totp-u1", secret="JBSWY3DPEHPK3PXP", now=11.0) - assert await store.enable_totp("totp-u1", recovery_code_hashes=["o1", "o2"], now=12.0) is True + staged_cell = await store.set_totp_secret("totp-u1", secret="JBSWY3DPEHPK3PXP", now=11.0) + assert ( + await store.enable_totp( + "totp-u1", expected_secret_cell=staged_cell, recovery_code_hashes=["o1", "o2"], now=12.0 + ) + is True + ) assert await store.consume_totp_step("totp-u1", 300) is True # the old seed's history await store.create_session( token_hash="totp-u1-session", user_id="totp-u1", expires_at=1e12, auth_mechanism="password" @@ -268,3 +300,61 @@ async def audit_rows() -> int: assert await audit_rows() == 1 await store.delete_user("totp-u1") + await _assert_totp_binding_contract(store) + + +async def _assert_totp_binding_contract(store: Any) -> None: + """BACKLOG #3025: the enable turns on only the secret the caller verified, on every backend. + + ``set_totp_secret`` returns the stored cell; ``enable_totp`` writes only while the row still + holds that cell. A second stage in between makes it refuse, and a write to any other column of + the row in between does not.""" + await store.create_user( + user_id="totp-u2", + username="totp-bob", + auth_provider="local", + password_hash="h", + now=1.0, + password_generated=False, + ) + verified = await store.set_totp_secret("totp-u2", secret="JBSWY3DPEHPK3PXP", now=2.0) + assert verified is not None + # A second begin stages another secret after the first one was verified. + other_seed = "JBSWY3DPEHPK3PXP"[::-1] + replacing = await store.set_totp_secret("totp-u2", secret=other_seed, now=3.0) + assert replacing is not None and replacing != verified + assert ( + await store.enable_totp( + "totp-u2", recovery_code_hashes=["x"], expected_secret_cell=verified, now=4.0 + ) + is False + ) + user = await store.get_user("totp-u2") + assert user is not None and user.totp_enabled is False + assert await store.get_totp_secret("totp-u2") == other_seed + assert await store.get_recovery_code_hashes("totp-u2") == [] + # None matches nothing, even with a secret staged. + assert ( + await store.enable_totp("totp-u2", recovery_code_hashes=[], expected_secret_cell=None) + is False + ) + + # The false-refusal trap: writes to OTHER columns between the stage and the enable carry the + # stored cell over byte for byte, so the compare still matches. + await store.set_user_notify_email("totp-u2", email="bob@example.test", now=5.0) + await store.record_login_success("totp-u2", now=6.0) + assert ( + await store.enable_totp( + "totp-u2", recovery_code_hashes=["y"], expected_secret_cell=replacing, now=7.0 + ) + is True + ) + assert await store.get_totp_secret("totp-u2") == other_seed + assert await store.get_recovery_code_hashes("totp-u2") == ["y"] + + # Once TOTP is on, a stage writes nothing and the enabled secret stands. + assert await store.set_totp_secret("totp-u2", secret="JBSWY3DPEHPK3PXP", now=8.0) is None + assert await store.get_totp_secret("totp-u2") == other_seed + assert await store.set_totp_secret("missing-user", secret="JBSWY3DPEHPK3PXP") is None + + await store.delete_user("totp-u2") diff --git a/tests/test_admin_reset_totp.py b/tests/test_admin_reset_totp.py index dea6ba602b..3ece2bc352 100644 --- a/tests/test_admin_reset_totp.py +++ b/tests/test_admin_reset_totp.py @@ -296,8 +296,13 @@ async def add_directory_admin() -> None: user_id="u-ad", username="ad-admin", auth_provider="ad", password_generated=False ) await store.set_user_roles("u-ad", [Role.ADMINISTRATOR.value], assigned_by="test") - await store.set_totp_secret("u-ad", secret=PROVISION_TOTP_SECRET) - assert await store.enable_totp("u-ad", recovery_code_hashes=[]) is True + staged_cell = await store.set_totp_secret("u-ad", secret=PROVISION_TOTP_SECRET) + assert ( + await store.enable_totp( + "u-ad", expected_secret_cell=staged_cell, recovery_code_hashes=[] + ) + is True + ) # The local one disabled, so the directory Administrator is the sole enabled one. local = await store.get_user_by_username(_ADMIN) assert local is not None @@ -625,8 +630,13 @@ async def re_enrol(store: MessageStore) -> None: user = await store.get_user_by_username(_ADMIN) assert user is not None await store.disable_totp(user.id) - await store.set_totp_secret(user.id, secret=other_seed) - assert await store.enable_totp(user.id, recovery_code_hashes=[], now=1.0) is True + staged_cell = await store.set_totp_secret(user.id, secret=other_seed) + assert ( + await store.enable_totp( + user.id, expected_secret_cell=staged_cell, recovery_code_hashes=[], now=1.0 + ) + is True + ) def enrol_while_re_enrolled(**_k: object) -> tuple[str, str, float]: _with_store(db, re_enrol) diff --git a/tests/test_approver_provenance.py b/tests/test_approver_provenance.py index f715fd20bc..2eac26d7fe 100644 --- a/tests/test_approver_provenance.py +++ b/tests/test_approver_provenance.py @@ -183,8 +183,10 @@ async def test_totp_enrolled_after_the_request_is_flagged(engine: Engine) -> Non sink = _Sink() gate = _gate(engine, service, sink) approval_id = await _request(gate, maker) - await engine.store.set_totp_secret(checker, secret="JBSWY3DPEHPK3PXP") - assert await engine.store.enable_totp(checker, recovery_code_hashes=[]) + staged_cell = await engine.store.set_totp_secret(checker, secret="JBSWY3DPEHPK3PXP") + assert await engine.store.enable_totp( + checker, expected_secret_cell=staged_cell, recovery_code_hashes=[] + ) await _release(gate, approval_id, checker) diff --git a/tests/test_auth_hardening.py b/tests/test_auth_hardening.py index 2a4a894cca..435bbd18e1 100644 --- a/tests/test_auth_hardening.py +++ b/tests/test_auth_hardening.py @@ -444,8 +444,10 @@ async def test_every_refused_combined_sign_in_answers_alike_on_the_json_and_cons user = await engine.store.get_user_by_username("carol") assert user is not None secret = totp.generate_secret() - await engine.store.set_totp_secret(user.id, secret=secret) - assert await engine.store.enable_totp(user.id, recovery_code_hashes=[]) + staged_cell = await engine.store.set_totp_secret(user.id, secret=secret) + assert await engine.store.enable_totp( + user.id, expected_secret_cell=staged_cell, recovery_code_hashes=[] + ) now = [3_000_000.0] def code(valid: bool) -> str: @@ -656,9 +658,11 @@ async def test_recovery_code_verify_cost_does_not_vary_with_the_code( # leaking how many remain, which is the half an attacker with only the password can measure. codes = ["AAAA-1111", "BBBB-2222", "CCCC-3333"] # enable_totp needs a staged secret (BACKLOG #2224); this test never verifies a TOTP code. - await engine.store.set_totp_secret(user.id, secret="JBSWY3DPEHPK3PXP") + staged_cell = await engine.store.set_totp_secret(user.id, secret="JBSWY3DPEHPK3PXP") assert await engine.store.enable_totp( - user.id, recovery_code_hashes=[hash_password(c) for c in codes] + user.id, + expected_secret_cell=staged_cell, + recovery_code_hashes=[hash_password(c) for c in codes], ) presented = {"first": codes[0], "last": codes[-1], "wrong": "ZZZZ-9999"}[present] @@ -711,10 +715,12 @@ def counting(stored_hash: str, password: str) -> bool: user = await engine.store.get_user_by_username("totpeq") assert user is not None secret = totp.generate_secret() - await engine.store.set_totp_secret(user.id, secret=secret) + staged_cell = await engine.store.set_totp_secret(user.id, secret=secret) # Fewer live codes than slots, so a walk that forgot the padding would show as 2, not 10. live = [hash_password(c) for c in ("AAAA-1111", "BBBB-2222")] - assert await engine.store.enable_totp(user.id, recovery_code_hashes=live) + assert await engine.store.enable_totp( + user.id, expected_secret_cell=staged_cell, recovery_code_hashes=live + ) walk = live + [svc._DUMMY_PASSWORD_HASH] * (slots - len(live)) # A fixed arrival moment, so the step cannot roll over between minting and verifying the code. arrived = 1_900_000_000.0 diff --git a/tests/test_docs_security_pathways.py b/tests/test_docs_security_pathways.py index 780f9fa057..70822cf886 100644 --- a/tests/test_docs_security_pathways.py +++ b/tests/test_docs_security_pathways.py @@ -2278,8 +2278,10 @@ async def probe() -> None: ) # Item 6: once TOTP is on, a covered account cannot remove it, passkey or not ... # (enable_totp needs a staged secret, BACKLOG #2224.) - await store.set_totp_secret(identity.user_id, secret="JBSWY3DPEHPK3PXP") - assert await store.enable_totp(identity.user_id, recovery_code_hashes=[]) + staged_cell = await store.set_totp_secret(identity.user_id, secret="JBSWY3DPEHPK3PXP") + assert await store.enable_totp( + identity.user_id, expected_secret_cell=staged_cell, recovery_code_hashes=[] + ) await store.add_webauthn_credential( WebAuthnCredential( credential_id_hash="ninth-sweep-hash", diff --git a/tests/test_generated_credential_failure.py b/tests/test_generated_credential_failure.py index e6ad7c5248..c7f24d4012 100644 --- a/tests/test_generated_credential_failure.py +++ b/tests/test_generated_credential_failure.py @@ -113,8 +113,10 @@ async def test_create_reset_and_factor_reset_write_nothing_when_no_credential_ca roles=["viewer"], actor="test", ) - await store.set_totp_secret(target, secret="JBSWY3DPEHPK3PXP") - assert await store.enable_totp(target, recovery_code_hashes=["h"]) + staged_cell = await store.set_totp_secret(target, secret="JBSWY3DPEHPK3PXP") + assert await store.enable_totp( + target, expected_secret_cell=staged_cell, recovery_code_hashes=["h"] + ) await store.add_webauthn_credential( WebAuthnCredential( credential_id_hash="kept-hash", diff --git a/tests/test_mfa.py b/tests/test_mfa.py index 8b2eb0c907..288b576063 100644 --- a/tests/test_mfa.py +++ b/tests/test_mfa.py @@ -1317,7 +1317,7 @@ async def test_AC2a_a_code_does_not_pass_the_lock_on_an_account_without_active_t assert after is not None and _columns(after) == _columns(before) # --- passkey-only: a factor is enrolled, but it is not TOTP ------------------------------- - await store.set_totp_secret(identity.user_id, secret=None) + await store.disable_totp(identity.user_id) await store.add_webauthn_credential( WebAuthnCredential( credential_id_hash="ac2a-passkey-hash", @@ -2419,10 +2419,19 @@ async def test_a_session_revoked_after_rotation_still_delivers_the_codes( real_enable = store.enable_totp async def revoke_then_enable( - user_id: str, *, recovery_code_hashes: list[str], now: float | None = None + user_id: str, + *, + recovery_code_hashes: list[str], + expected_secret_cell: str | None, + now: float | None = None, ) -> bool: await store.revoke_user_sessions(user_id) - return await real_enable(user_id, recovery_code_hashes=recovery_code_hashes, now=now) + return await real_enable( + user_id, + recovery_code_hashes=recovery_code_hashes, + expected_secret_cell=expected_secret_cell, + now=now, + ) monkeypatch.setattr(store, "enable_totp", revoke_then_enable) enrolled = await service.confirm_mfa_enrollment( @@ -2459,12 +2468,21 @@ async def test_a_reset_between_rotation_and_enable_leaves_mfa_off_with_no_null_s real_enable = store.enable_totp async def reset_then_enable( - user_id: str, *, recovery_code_hashes: list[str], now: float | None = None + user_id: str, + *, + recovery_code_hashes: list[str], + expected_secret_cell: str | None, + now: float | None = None, ) -> bool: # The factor clear of ``admin_reset_mfa``, and none of its session revocation, so the # rotated session is still live when the enable runs and only this fix can end it. await store.disable_totp(user_id) - return await real_enable(user_id, recovery_code_hashes=recovery_code_hashes, now=now) + return await real_enable( + user_id, + recovery_code_hashes=recovery_code_hashes, + expected_secret_cell=expected_secret_cell, + now=now, + ) monkeypatch.setattr(store, "enable_totp", reset_then_enable) refused = await service.confirm_mfa_enrollment( @@ -2506,10 +2524,19 @@ async def test_the_real_admin_reset_inside_the_window_leaves_mfa_off( real_enable = store.enable_totp async def admin_reset_then_enable( - user_id: str, *, recovery_code_hashes: list[str], now: float | None = None + user_id: str, + *, + recovery_code_hashes: list[str], + expected_secret_cell: str | None, + now: float | None = None, ) -> bool: await service.admin_reset_mfa(user_id, actor="other-admin") - return await real_enable(user_id, recovery_code_hashes=recovery_code_hashes, now=now) + return await real_enable( + user_id, + recovery_code_hashes=recovery_code_hashes, + expected_secret_cell=expected_secret_cell, + now=now, + ) monkeypatch.setattr(store, "enable_totp", admin_reset_then_enable) refused = await service.confirm_mfa_enrollment( @@ -2543,10 +2570,24 @@ async def test_a_second_confirm_that_enabled_first_keeps_the_winners_codes( real_enable = store.enable_totp async def other_confirm_first( - user_id: str, *, recovery_code_hashes: list[str], now: float | None = None + user_id: str, + *, + recovery_code_hashes: list[str], + expected_secret_cell: str | None, + now: float | None = None, ) -> bool: - assert await real_enable(user_id, recovery_code_hashes=["winner-hash"], now=now) - return await real_enable(user_id, recovery_code_hashes=recovery_code_hashes, now=now) + assert await real_enable( + user_id, + recovery_code_hashes=["winner-hash"], + expected_secret_cell=expected_secret_cell, + now=now, + ) + return await real_enable( + user_id, + recovery_code_hashes=recovery_code_hashes, + expected_secret_cell=expected_secret_cell, + now=now, + ) monkeypatch.setattr(store, "enable_totp", other_confirm_first) refused = await service.confirm_mfa_enrollment( @@ -2564,6 +2605,185 @@ async def other_confirm_first( await store.close() +# --- BACKLOG #3025: the enable turns on only the secret the confirm verified ----------------------- + + +async def test_a_begin_inside_the_confirm_window_cannot_choose_the_enabled_secret( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """RED when: a begin landing between the confirm's code check and its enable decides which + secret is enabled (BACKLOG #3025, ADR 0207 AC-13). + + The confirm checks the code against the staged secret, stamps and rotates the session, then + enables. A begin from another session on the same account stages a fresh secret in that window. + Before the fix the enable turned on whatever was staged, so the account ended up holding the + second begin's factor while the first session was handed recovery codes and a satisfied session. + The confirm must now fail closed: TOTP off, no codes, the rotated session ended, and the audit + names the reason. The replacing secret is still only staged, and confirms on its own. + """ + store = await _store() + try: + notifier = _FakeNotifier() + service = AuthService( + store, AuthSettings(mfa_verify_min_elapsed_seconds=0), security_notifier=notifier + ) + identity, token, password = await login_admin(service) + first = await service.begin_mfa_enrollment(identity) + t0 = 1_000_000.0 + pin_totp_clock(monkeypatch, t0) + + real_elevated = service._elevated + second: list[Any] = [] + + async def elevated_then_begin(*args: Any, **kwargs: Any) -> Any: + out = await real_elevated(*args, **kwargs) + # The other session's begin: the service method is keyed on the account, which is all + # the two sessions share. + second.append(await service.begin_mfa_enrollment(identity)) + return out + + monkeypatch.setattr(service, "_elevated", elevated_then_begin) + refused = await service.confirm_mfa_enrollment( + identity, totp.totp(first.secret, now=t0), token=token + ) + assert len(second) == 1 and second[0].secret != first.secret + assert refused.ok is False and refused.session_lost is True + assert refused.recovery_codes == () + user = await store.get_user(identity.user_id) + assert user is not None and user.totp_enabled is False, ( + "the enable turned on a secret the confirm never verified" + ) + assert await store.get_totp_secret(identity.user_id) == second[0].secret + assert await store.get_recovery_code_hashes(identity.user_id) == [] + assert await store.list_sessions(identity.user_id) == [] + assert not any(e.event_type == MFA_ENABLED for e in notifier.events) + rows = await store.list_audit( + actor=ADMIN_USERNAME, action="auth.mfa_enroll_refused", limit=10 + ) + assert len(rows) == 1 and '"reason": "secret_replaced"' in str(rows[0]["detail"]) + + # The replacing secret confirms through its own ceremony, and that is what is enabled. Its + # code comes from the SAME 30-second step the refused confirm spent: the stage forgot that + # step's high-water mark, which belonged to the replaced secret. + monkeypatch.setattr(service, "_elevated", real_elevated) + again = await service.login(ADMIN_USERNAME, password) + assert again.token is not None + confirmed = await service.confirm_mfa_enrollment( + identity, totp.totp(second[0].secret, now=t0), token=again.token + ) + assert confirmed.ok + assert (await service.mfa_status(identity)).enabled is True + assert await store.get_totp_secret(identity.user_id) == second[0].secret + finally: + await store.close() + + +async def test_an_unrelated_write_inside_the_confirm_window_still_confirms( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """RED when: the #3025 compare-and-set refuses a confirm that nothing raced. + + Writes to other columns of the user row re-seal the row but carry the stored secret cell over + unchanged, so a notify-address change and a sign-in stamp between the begin and the confirm, and + between the code check and the enable, must not make the enable refuse.""" + store = await _store() + try: + service = AuthService(store, AuthSettings(mfa_verify_min_elapsed_seconds=0)) + identity, token, _ = await login_admin(service) + enroll = await service.begin_mfa_enrollment(identity) + await store.set_user_notify_email(identity.user_id, email="admin@example.test") + await store.record_login_success(identity.user_id) + + real_elevated = service._elevated + + async def elevated_then_write(*args: Any, **kwargs: Any) -> Any: + out = await real_elevated(*args, **kwargs) + await store.record_login_success(identity.user_id) + return out + + monkeypatch.setattr(service, "_elevated", elevated_then_write) + confirmed = await service.confirm_mfa_enrollment( + identity, fresh_totp(enroll.secret), token=token + ) + assert confirmed.ok and len(confirmed.recovery_codes) == 10 + assert (await service.mfa_status(identity)).enabled is True + assert await store.get_totp_secret(identity.user_id) == enroll.secret + finally: + await store.close() + + +async def test_a_begin_whose_stage_lands_after_an_enable_is_refused( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """RED when: a begin that read TOTP off, then lost to a confirm that enabled it, overwrites the + enabled secret (BACKLOG #3025). The stage is conditional on TOTP being off, so the begin + refuses and the enabled secret stands.""" + store = await _store() + try: + service = AuthService(store, AuthSettings(mfa_verify_min_elapsed_seconds=0)) + identity, token, _ = await login_admin(service) + before_enable = await service._load_user(identity.user_id) + assert before_enable is not None and not before_enable.totp_enabled + enroll = await service.begin_mfa_enrollment(identity) + assert ( + await service.confirm_mfa_enrollment(identity, fresh_totp(enroll.secret), token=token) + ).ok + + async def stale_read(_user_id: str) -> Any: + return before_enable # the begin's read, taken before the enable landed + + monkeypatch.setattr(service, "_load_user", stale_read) + with pytest.raises(ValueError, match="already enabled"): + await service.begin_mfa_enrollment(identity) + assert await store.get_totp_secret(identity.user_id) == enroll.secret + finally: + await store.close() + + +@pytest.mark.parametrize("delay", [0.0, 0.01, 0.05, 0.2]) +async def test_a_live_begin_racing_a_confirm_never_enables_an_unverified_secret( + monkeypatch: pytest.MonkeyPatch, delay: float +) -> None: + """BACKLOG #3025, closing step 3: two live requests race, with no hook between them. + + The confirm and a second begin run concurrently; the begin starts after ``delay``. Where the + begin lands is up to the scheduler, so each run checks the invariant rather than one outcome: + whatever happened, an enabled factor is the secret the confirm verified, and a confirm that + handed back codes left that factor enabled.""" + store = await _store() + try: + service = AuthService(store, AuthSettings(mfa_verify_min_elapsed_seconds=0)) + identity, token, _ = await login_admin(service) + first = await service.begin_mfa_enrollment(identity) + t0 = 1_000_000.0 + pin_totp_clock(monkeypatch, t0) + + async def late_begin() -> Any: + await asyncio.sleep(delay) + try: + return await service.begin_mfa_enrollment(identity) + except ValueError: + return None # the confirm enabled first; the begin refused + + outcome, _second = await asyncio.gather( + service.confirm_mfa_enrollment(identity, totp.totp(first.secret, now=t0), token=token), + late_begin(), + ) + user = await store.get_user(identity.user_id) + assert user is not None + if user.totp_enabled: + assert await store.get_totp_secret(identity.user_id) == first.secret, ( + "an enabled factor is not the secret the confirm verified" + ) + if outcome.ok: + assert user.totp_enabled and outcome.recovery_codes + else: + assert outcome.recovery_codes == () + assert not user.totp_enabled + finally: + await store.close() + + # --- ADR 0197 Amendment A, wave 1 (BACKLOG #1131): every account is born with a way past ---------- # # A lock exists to bound guesses. On an engine-generated 192-bit credential it bounds nothing, so @@ -2856,8 +3076,10 @@ async def test_the_census_names_a_covered_chosen_password_with_no_totp_and_only_ await create_admin(service) await _created_holder(service, username="generated") enrolled_id, _ = await _created_holder(service, username="enrolled") - await store.set_totp_secret(enrolled_id, secret="JBSWY3DPEHPK3PXP") - assert await store.enable_totp(enrolled_id, recovery_code_hashes=[]) + staged_cell = await store.set_totp_secret(enrolled_id, secret="JBSWY3DPEHPK3PXP") + assert await store.enable_totp( + enrolled_id, expected_secret_cell=staged_cell, recovery_code_hashes=[] + ) disabled_id, _ = await _created_holder(service, username="disabled") await store.set_password( disabled_id, password_hash="h", password_generated=False, must_change_password=False @@ -3038,8 +3260,10 @@ async def test_the_census_reads_every_account_it_should_and_nothing_it_should_no await store.create_user( user_id="dir-totp", username="directory", auth_provider="ad", password_generated=False ) - await store.set_totp_secret("dir-totp", secret="JBSWY3DPEHPK3PXP") - assert await store.enable_totp("dir-totp", recovery_code_hashes=[]) + staged_cell = await store.set_totp_secret("dir-totp", secret="JBSWY3DPEHPK3PXP") + assert await store.enable_totp( + "dir-totp", expected_secret_cell=staged_cell, recovery_code_hashes=[] + ) await store.create_user( user_id="hashless", username="half-built", diff --git a/tests/test_oidc_json_step_up_hint.py b/tests/test_oidc_json_step_up_hint.py index 5c65fbd963..8d22372c39 100644 --- a/tests/test_oidc_json_step_up_hint.py +++ b/tests/test_oidc_json_step_up_hint.py @@ -250,9 +250,11 @@ async def test_mfa_verify_refuses_a_satisfied_oidc_session_with_the_idp_header( ordinary 401, so the refusal keys on the session's mechanism.""" uid = await _add_admin(service) secret = "JBSWY3DPEHPK3PXP" - await service.store.set_totp_secret(uid, secret=secret) + staged_cell = await service.store.set_totp_secret(uid, secret=secret) wrong = "000000" if totp.totp(secret) != "000000" else "111111" - assert await service.store.enable_totp(uid, recovery_code_hashes=[]) + assert await service.store.enable_totp( + uid, expected_secret_cell=staged_cell, recovery_code_hashes=[] + ) oidc_token = await _session(service, uid, "tok-mfa-oidc", mechanism="oidc") password_token = await _session(service, uid, "tok-mfa-pw", mechanism="password") for token in (oidc_token, password_token): diff --git a/tests/test_provision_first_administrator.py b/tests/test_provision_first_administrator.py index 5e0b88e4a7..cf0f1bef39 100644 --- a/tests/test_provision_first_administrator.py +++ b/tests/test_provision_first_administrator.py @@ -1528,8 +1528,10 @@ async def test_a_repair_of_a_row_that_holds_an_address_is_refused_without_an_ema must_change_password=False, password_generated=False, ) - await store.set_totp_secret("roleless", secret="JBSWY3DPEHPK3PXP") - assert await store.enable_totp("roleless", recovery_code_hashes=["h1", "h2"]) + staged_cell = await store.set_totp_secret("roleless", secret="JBSWY3DPEHPK3PXP") + assert await store.enable_totp( + "roleless", expected_secret_cell=staged_cell, recovery_code_hashes=["h1", "h2"] + ) await store.add_webauthn_credential( WebAuthnCredential( credential_id_hash="holder-passkey-hash", @@ -2369,8 +2371,10 @@ async def test_the_repair_branch_clears_the_earlier_holders_factors_and_sessions must_change_password=False, password_generated=False, ) - await store.set_totp_secret("earlier", secret="JBSWY3DPEHPK3PXP") - assert await store.enable_totp("earlier", recovery_code_hashes=["h1", "h2"]) + staged_cell = await store.set_totp_secret("earlier", secret="JBSWY3DPEHPK3PXP") + assert await store.enable_totp( + "earlier", expected_secret_cell=staged_cell, recovery_code_hashes=["h1", "h2"] + ) await store.add_webauthn_credential( WebAuthnCredential( credential_id_hash="earlier-passkey-hash", @@ -2455,10 +2459,19 @@ async def test_an_enable_that_matches_no_row_refuses_before_the_role( real_enable = store.enable_totp async def cleared_then_enable( - user_id: str, *, recovery_code_hashes: list[str], now: float | None = None + user_id: str, + *, + recovery_code_hashes: list[str], + expected_secret_cell: str | None, + now: float | None = None, ) -> bool: await store.disable_totp(user_id) - return await real_enable(user_id, recovery_code_hashes=recovery_code_hashes, now=now) + return await real_enable( + user_id, + recovery_code_hashes=recovery_code_hashes, + expected_secret_cell=expected_secret_cell, + now=now, + ) monkeypatch.setattr(store, "enable_totp", cleared_then_enable) kw = provision_totp() @@ -2482,6 +2495,67 @@ async def cleared_then_enable( await store.close() +async def test_a_re_stage_between_the_stage_and_the_enable_refuses_before_the_role( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """BACKLOG #3025: the enable is bound to the cell THIS run staged. Another run that stages its + own secret in between makes the enable match no row, so the run refuses before the role and + never turns on a secret it did not prove.""" + store = await MessageStore.open(":memory:") + try: + service = AuthService(store, AuthSettings()) + real_enable = store.enable_totp + other_seed = PROVISION_TOTP_SECRET[::-1] + + async def restaged_then_enable(user_id: str, **kw: object) -> bool: + assert await store.set_totp_secret(user_id, secret=other_seed) is not None + return await real_enable(user_id, **kw) # type: ignore[arg-type] + + monkeypatch.setattr(store, "enable_totp", restaged_then_enable) + with pytest.raises(FirstAdministratorRefused, match="changed while this command ran"): + await service.provision_first_administrator( + username="site-admin", password=_PASSWORD, actor="test", **provision_totp() + ) + row = await store.get_user_by_username("site-admin") + assert row is not None and not row.totp_enabled + assert await store.get_totp_secret(row.id) == other_seed + assert not await store.get_user_role_ids(row.id) + finally: + await store.close() + + +async def test_a_stage_that_lands_after_another_runs_enable_refuses_before_the_role( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """BACKLOG #3025: the stage is conditional on TOTP being off. Another run that enabled TOTP on + the row first makes this run's stage write nothing, so it refuses before hashing any codes or + granting the role, and the other run's enabled secret stands.""" + store = await MessageStore.open(":memory:") + try: + service = AuthService(store, AuthSettings()) + real_stage = store.set_totp_secret + other_seed = PROVISION_TOTP_SECRET[::-1] + + async def other_run_enabled_first(user_id: str, **kw: object) -> str | None: + cell = await real_stage(user_id, secret=other_seed) + assert await store.enable_totp( + user_id, recovery_code_hashes=[], expected_secret_cell=cell + ) + return await real_stage(user_id, **kw) # type: ignore[arg-type] + + monkeypatch.setattr(store, "set_totp_secret", other_run_enabled_first) + with pytest.raises(FirstAdministratorRefused, match="did not stage its authenticator"): + await service.provision_first_administrator( + username="site-admin", password=_PASSWORD, actor="test", **provision_totp() + ) + row = await store.get_user_by_username("site-admin") + assert row is not None and row.totp_enabled + assert await store.get_totp_secret(row.id) == other_seed + assert not await store.get_user_role_ids(row.id) + finally: + await store.close() + + async def test_a_sign_in_during_the_repair_does_not_survive_into_the_administrator_role() -> None: """Review round 2: the earlier holder's password works until the repair writes the new one. A session minted in that window must not outlive the repair, so the sessions are revoked again From 44c2dcee2ef474aae1bcb658937de822ce280565 Mon Sep 17 00:00:00 2001 From: Scott Hall Date: Fri, 16 Oct 2026 17:54:05 -0500 Subject: [PATCH 02/18] Review round 2: the TOTP stage keeps the step mark, so provision's code 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. --- messagefoundry/auth/service.py | 8 ++++---- messagefoundry/store/store.py | 10 +++++----- tests/test_mfa.py | 8 +++++--- tests/test_provision_first_administrator.py | 21 +++++++++++++++++++++ 4 files changed, 35 insertions(+), 12 deletions(-) diff --git a/messagefoundry/auth/service.py b/messagefoundry/auth/service.py index 2599c116e7..9d60a23b0a 100644 --- a/messagefoundry/auth/service.py +++ b/messagefoundry/auth/service.py @@ -10077,9 +10077,9 @@ async def confirm_mfa_enrollment( (or a time-step already consumed -- single-use, BACKLOG #1021) elevates nothing and carries none. A good code on a session revoked before the rotation returns ``session_lost`` with MFA still OFF (BACKLOG #1902), so a lost session never leaves MFA on with codes nobody saw. So - does a good code whose activation matches no row, because a reset cleared the staged secret, - a second confirm enabled first (BACKLOG #2224), or a second begin staged a secret this - confirm never verified (BACKLOG #3025). That confirm ends the session it rotated, + does a good code whose activation matches no row, for example because a reset cleared the + staged secret, a second confirm enabled first (BACKLOG #2224), or a second begin staged a + secret this confirm never verified (BACKLOG #3025). That confirm ends the session it rotated, hands back no codes, and audits ``auth.mfa_enroll_refused``. This is one of the two legs that turn an MFA-pending session into an MFA-satisfied one for a @@ -10151,7 +10151,7 @@ async def confirm_mfa_enrollment( if not await self._store.enable_totp( identity.user_id, recovery_code_hashes=hashes, expected_secret_cell=verified_cell ): - # BACKLOG #2224, #3025: the conditional enable matched no row, because an + # BACKLOG #2224, #3025: the conditional enable matched no row, for example because an # administrator's reset cleared the staged secret inside the rotation window, a second # confirm enabled TOTP first, or a second begin staged a secret this confirm never # verified. This confirm turns nothing on and its codes are dropped. The diff --git a/messagefoundry/store/store.py b/messagefoundry/store/store.py index 29a3d570dc..125acd5e9e 100644 --- a/messagefoundry/store/store.py +++ b/messagefoundry/store/store.py @@ -2640,17 +2640,17 @@ def totp_stage(sealed_secret: str | None, now: float) -> UserChange: #3025). A begin that lands once TOTP is on writes nothing, so it can never replace the secret of an enabled factor. There is no clear here: an empty secret raises, and ``disable_totp`` clears. - ``last_totp_step`` goes too, as in :func:`totp_disable`: the high-water mark belonged to the - secret being replaced, and a new secret has no history. Without that, a confirm refused because a - second begin replaced its secret leaves its spent step behind, and the replacing secret's own - code from the same 30 seconds is refused as a replay.""" + ``last_totp_step`` is kept on purpose. ``provision_first_administrator`` consumes its code's + step BEFORE it stages, and that step must stay spent. The cost: after a confirm refused because a + second begin replaced its secret, the replacing secret's code from the same 30 seconds can be + refused as a replay, and the holder uses the next code.""" if not sealed_secret: raise ValueError("a TOTP secret is required") def apply(r: UserAuthzRecord) -> UserAuthzRecord | None: if r.row.totp_enabled: return None - return r.changed(totp_secret=sealed_secret, last_totp_step=None, updated_at=float(now)) + return r.changed(totp_secret=sealed_secret, updated_at=float(now)) return apply diff --git a/tests/test_mfa.py b/tests/test_mfa.py index 288b576063..c8c3b71d81 100644 --- a/tests/test_mfa.py +++ b/tests/test_mfa.py @@ -2663,13 +2663,15 @@ async def elevated_then_begin(*args: Any, **kwargs: Any) -> Any: assert len(rows) == 1 and '"reason": "secret_replaced"' in str(rows[0]["detail"]) # The replacing secret confirms through its own ceremony, and that is what is enabled. Its - # code comes from the SAME 30-second step the refused confirm spent: the stage forgot that - # step's high-water mark, which belonged to the replaced secret. + # code comes from a later step: the stage keeps the high-water mark, so the step the refused + # confirm spent stays spent. monkeypatch.setattr(service, "_elevated", real_elevated) again = await service.login(ADMIN_USERNAME, password) assert again.token is not None + t1 = t0 + totp.DEFAULT_PERIOD + pin_totp_clock(monkeypatch, t1) confirmed = await service.confirm_mfa_enrollment( - identity, totp.totp(second[0].secret, now=t0), token=again.token + identity, totp.totp(second[0].secret, now=t1), token=again.token ) assert confirmed.ok assert (await service.mfa_status(identity)).enabled is True diff --git a/tests/test_provision_first_administrator.py b/tests/test_provision_first_administrator.py index cf0f1bef39..9a1de19930 100644 --- a/tests/test_provision_first_administrator.py +++ b/tests/test_provision_first_administrator.py @@ -2556,6 +2556,27 @@ async def other_run_enabled_first(user_id: str, **kw: object) -> str | None: await store.close() +async def test_the_provisioning_codes_step_stays_spent_after_the_stage() -> None: + """BACKLOG #3025 review: provision consumes its code's step BEFORE it stages the secret, so the + stage must not reset the step high-water mark, or the code typed at the terminal would pass the + first sign-in's second factor again (ASVS 6.5.1, BACKLOG #1021).""" + from messagefoundry.auth import totp + + store = await MessageStore.open(":memory:") + try: + service = AuthService(store, AuthSettings()) + kw = provision_totp() + await service.provision_first_administrator( + username="site-admin", password=_PASSWORD, actor="test", **kw + ) + row = await store.get_user_by_username("site-admin") + assert row is not None and row.totp_enabled + step = int(kw["totp_code_read_at"] // totp.DEFAULT_PERIOD) + assert not await store.consume_totp_step(row.id, step) + finally: + await store.close() + + async def test_a_sign_in_during_the_repair_does_not_survive_into_the_administrator_role() -> None: """Review round 2: the earlier holder's password works until the repair writes the new one. A session minted in that window must not outlive the repair, so the sessions are revoked again From 957a7462ac80ec35b029ba02be3d617fcb5cd7c9 Mon Sep 17 00:00:00 2001 From: Scott Hall Date: Fri, 16 Oct 2026 18:32:41 -0500 Subject: [PATCH 03/18] A stale reconciler rename plan no longer overwrites a newer sign-in rename (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. --- messagefoundry/auth/service.py | 35 ++++++++- tests/test_ad_session_reconcile.py | 115 ++++++++++++++++++++++++----- 2 files changed, 129 insertions(+), 21 deletions(-) diff --git a/messagefoundry/auth/service.py b/messagefoundry/auth/service.py index 9d60a23b0a..f991cccdf8 100644 --- a/messagefoundry/auth/service.py +++ b/messagefoundry/auth/service.py @@ -6860,6 +6860,9 @@ async def _refuse(held_by: str | None, detected: str, holds: str) -> None: # ``old_username`` (BACKLOG #2291). The reconciler captured that at plan time, and a # sign-in may have renamed the row since, so it can name an account that no longer # exists. The audit actor and the warning both use the name an operator would find. + # Since BACKLOG #3069 skips a plan whose old name the row no longer holds, every caller + # past the pre-read passes a name equal to ``old_username``; the parameter is kept so + # the audit's source stays the row as read, not the plan. await self._audit( "auth.ad_username_refresh_conflict", actor=holds, @@ -6909,7 +6912,9 @@ async def _refuse(held_by: str | None, detected: str, holds: str) -> None: # read. # # A row that already carries the new name has nothing to change, so it gets no second audit - # row and no second notice. The notice names the name the row actually had, not the plan's. + # row and no second notice. A row that carries neither name was renamed by someone else + # since the caller read it, so the plan is stale and is skipped (BACKLOG #3069); past that + # check the row holds the caller's old name, so the notice and every refusal name it. # The write below compares on this name (BACKLOG #2290), so a refresh that read it just # before another refresh of this row wrote cannot write as well. And each refusal after # this read audits under this name (BACKLOG #2291), so the read comes BEFORE the ``held`` @@ -6940,6 +6945,26 @@ async def _refuse(held_by: str | None, detected: str, holds: str) -> None: user_id, ) return + if before.username != old_username: + # BACKLOG #3069. The row holds neither the caller's old name nor its new one, so another + # writer renamed it after the caller read it: a sign-in after the reconciler planned, in + # the case that motivated this. The compare-and-set below compares on the name just + # read, so it would match and write this caller's older name over the newer one. Skip + # instead, and let the next pass or sign-in plan from a fresh read. Nothing was + # attempted, so no audit row, no conflict and no notice; INFO says why. + # + # AFTER the already-applied check, because a row that holds the new name is the no-op + # above, not a skip. BEFORE the ``held`` pre-check, because a stale plan is not a + # collision: filing one would send an operator looking for a holder this caller has no + # current claim against. + _log.info( + "AD account %s was renamed by another writer after this rename refresh was " + "planned (now %s), so the stale plan wrote nothing and sends no audit row or " + "notice (BACKLOG #3069)", + user_id, + scrub_log_argument(before.username), + ) + return if held is not None and held.id != user_id: await _refuse(held.id, "pre_check", before.username) return @@ -7012,9 +7037,11 @@ async def _refuse(held_by: str | None, detected: str, holds: str) -> None: # - The row's name moved after the pre-read, so the compare failed: another refresh of # this same row wrote first (BACKLOG #2290). That caller audits and notifies, so this one # does neither. It is not a conflict either, because no other row holds anything. The - # first writer wins, so when two refreshes carry DIFFERENT new names the stored one can - # be the older of the two until the next sign-in or pass copies the directory's name - # down again. Logged at INFO so the skipped write is visible without paging anyone. + # first writer wins, so when two refreshes carry DIFFERENT new names and both read the + # row before either wrote, the stored one can be the older of the two until the next + # sign-in or pass copies the directory's name down again. In sequence the BACKLOG #3069 + # skip above stops the older one before it writes. Logged at INFO so the skipped write + # is visible without paging anyone. if not written: after = await self._load_user(user_id) if after is None: diff --git a/tests/test_ad_session_reconcile.py b/tests/test_ad_session_reconcile.py index b8d7952136..13eed41615 100644 --- a/tests/test_ad_session_reconcile.py +++ b/tests/test_ad_session_reconcile.py @@ -26,6 +26,7 @@ from dataclasses import replace from typing import Any +import aiosqlite import pytest from pydantic import ValidationError @@ -54,6 +55,7 @@ SCOPE_SOURCE_MANUAL, MessageStore, UserRecord, + VerifiedUser, ) from tests._admin_account import create_local_user_chosen @@ -2628,20 +2630,100 @@ async def test_a_rename_already_applied_by_another_caller_is_not_told_twice( await store.close() -async def test_the_notice_names_the_name_the_row_had_not_the_plans() -> None: - """The reconciler's ``old_username`` is the name at PLAN time. A sign-in can rename the row - before the plan applies, and the notice must then name what the row was actually called.""" +async def _rename_audits(store: MessageStore) -> list[aiosqlite.Row]: + """Every audit row a rename refresh writes, success or refusal.""" + return [ + a + for a in await store.list_audit() + if a["action"] in ("auth.ad_username_refreshed", "auth.ad_username_refresh_conflict") + ] + + +async def test_a_stale_plan_does_not_overwrite_a_newer_sign_in_rename( + caplog: pytest.LogCaptureFixture, +) -> None: + """BACKLOG #3069. The reconciler plans ``jdoe`` -> ``jdoe-married``; before the plan applies, a + directory sign-in renames the row to the newer ``jdoe-remarried``; then the plan applies. + + The newer name stays. The stale plan writes nothing, audits nothing and tells nobody, and says + why at INFO. Driven through the real pass: the hook is the ``held`` read the pass makes just + before it applies each rename, which is the last point between its plan and its apply. Before + #3069 the compare-and-set compared on the name just read, so it matched ``jdoe-remarried`` and + wrote the plan's older name over it. + """ + store, _ldap, service, notifier, user_id = await _renamed_service() + try: + load_by_name = service._load_user_by_username + signed_in: list[str] = [] + + async def _sign_in_renames_first(username: str) -> VerifiedUser | None: + if username == "jdoe-married" and not signed_in: + signed_in.append(username) + assert await store.set_user_username( + user_id, "jdoe-remarried", expected_username="jdoe" + ) + return await load_by_name(username) + + service._load_user_by_username = _sign_in_renames_first # type: ignore[method-assign] + caplog.set_level(logging.INFO, logger="messagefoundry.auth.service") + caplog.clear() # only the pass's own records, not the sign-in setup's + + plan = await service.reconcile_directory_sessions() + + assert [(r.old_username, r.new_username) for r in plan.renames] == [ + ("jdoe", "jdoe-married") + ] + assert signed_in, "the sign-in rename never ran between the plan and the apply" + after = await store.get_user(user_id) + assert after is not None and after.username == "jdoe-remarried", ( + "a stale reconciler plan overwrote a newer sign-in rename" + ) + assert not await _rename_audits(store), "a stale plan that wrote nothing still audited" + assert not [e for e in notifier.sent if e.event_type == USERNAME_CHANGED] + skipped = [ + r + for r in caplog.records + if r.name == "messagefoundry.auth.service" and "BACKLOG #3069" in r.getMessage() + ] + assert [r.levelno for r in skipped] == [logging.INFO], "the skipped stale plan was silent" + finally: + await store.close() + + +async def test_a_stale_plan_is_skipped_not_filed_as_a_conflict_when_another_row_holds_the_name( + caplog: pytest.LogCaptureFixture, +) -> None: + """BACKLOG #3069, the ordering half: the stale-plan skip runs BEFORE the ``held`` pre-check. + + The row was renamed to ``jdoe-remarried`` after the plan, and another row now holds the plan's + ``jdoe-married``. The plan has no current claim on the row, so a conflict audit and a lockout + warning would both be false. Driven on the method directly, so ``held`` can name the other row. + """ store, _ldap, service, notifier, user_id = await _renamed_service() try: - assert await store.set_user_username(user_id, "jdoe-interim", expected_username="jdoe") + assert await store.set_user_username(user_id, "jdoe-remarried", expected_username="jdoe") + await store.create_user( + user_id="holder-row", + username="jdoe-married", + auth_provider="ad", + password_generated=False, + ) + held = await store.get_user_by_username("jdoe-married") + assert held is not None and held.id == "holder-row" + caplog.set_level(logging.INFO, logger="messagefoundry.auth.service") + caplog.clear() await service._refresh_cached_username( - user_id=user_id, old_username="jdoe", new_username="jdoe-married", held=None + user_id=user_id, old_username="jdoe", new_username="jdoe-married", held=held ) - [notice] = [e for e in notifier.sent if e.event_type == USERNAME_CHANGED] - assert notice.detail["old_username"] == "jdoe-interim" - assert notice.detail["new_username"] == "jdoe-married" + after = await store.get_user(user_id) + assert after is not None and after.username == "jdoe-remarried" + assert not await _rename_audits(store), "a stale plan was audited" + assert not [e for e in notifier.sent if e.event_type == USERNAME_CHANGED] + assert not [r for r in caplog.records if r.levelno >= logging.WARNING], ( + "a stale plan warned of a lockout" + ) finally: await store.close() @@ -2681,16 +2763,15 @@ async def _record_write(*a: object, **kw: object) -> bool: @pytest.mark.parametrize("shape", ["pre_check", "write_race", "write_noop", "row_gone"]) async def test_a_refused_rename_is_audited_under_the_name_the_row_holds(shape: str) -> None: - """BACKLOG #2291. The caller's ``old_username`` is the reconciler's plan-time name, and a - sign-in can rename the row before the plan applies. A refusal audited under the plan's name - would name an account that no longer exists, so every refusal names what the row holds. + """BACKLOG #2291. Every refusal is audited under the name the row holds, as the refresh read it, + so the audit names an account an operator can find. - The row is renamed to ``jdoe-interim`` first and the refresh is passed the stale ``jdoe``. - One case per way the refresh refuses after it has read the row; each is asserted to have - refused, so none passes by never reaching the branch.""" + Since BACKLOG #3069 a refusal runs only when the row holds the caller's old name (a row + renamed since the plan is skipped first), so the row here holds the plan's ``jdoe``. What this + pins: each refusal shape reaches its branch, files one conflict under that name, and sends no + notice.""" store, _ldap, service, notifier, user_id = await _renamed_service() try: - assert await store.set_user_username(user_id, "jdoe-interim", expected_username="jdoe") held: UserRecord | None = None if shape == "pre_check": await store.create_user( @@ -2743,8 +2824,8 @@ async def _row_deleted(*a: object, **kw: object) -> bool: if a["action"] == "auth.ad_username_refresh_conflict" ] assert json.loads(conflict["detail"])["detected"] == shape - assert conflict["actor"] == "jdoe-interim", ( - f"a {shape} refusal audited the caller's stale name, not the one the row held" + assert conflict["actor"] == "jdoe", ( + f"a {shape} refusal was not audited under the name the row held" ) assert not [e for e in notifier.sent if e.event_type == USERNAME_CHANGED] finally: From 546435302253673df184d396d6c8db734665a788 Mon Sep 17 00:00:00 2001 From: Scott Hall Date: Fri, 16 Oct 2026 18:34:19 -0500 Subject: [PATCH 04/18] Docs state the TOTP enable compare-and-set built under BACKLOG #3025 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. --- docs/SECURITY.md | 19 +++++++++++-------- ...y-path-for-a-sole-administrator-lockout.md | 7 +++++++ ...directory-account-s-first-engine-factor.md | 5 +++++ 3 files changed, 23 insertions(+), 8 deletions(-) diff --git a/docs/SECURITY.md b/docs/SECURITY.md index d0a9314cc6..83d4249ed5 100644 --- a/docs/SECURITY.md +++ b/docs/SECURITY.md @@ -2767,14 +2767,17 @@ account `[security].require_mfa` covers, which keeps TOTP because in this releas way past the sign-in lock (ADR 0197 Amendment A). Such an account's first factor must be TOTP, too. An administrator clears a lost authenticator via `POST /users/{id}/reset-mfa` (which also revokes the user's sessions). -A confirm turns TOTP on only while a secret is still staged and TOTP is still off (BACKLOG -#2224). If a reset clears the secret during the confirm, or another confirm turns TOTP on first, -the confirm turns nothing on and returns no recovery codes. It ends the session it just rotated and -answers as a lost session: `401` on `POST /me/mfa/confirm`, and the sign-in page in the web -console. It is audited `auth.mfa_enroll_refused`, whose `reason` is `secret_cleared`, -`already_enabled` or `account_gone`, read after the refusal. The check is that some secret is -staged, not that it is the one the confirm verified: a second enrolment start that re-stages the -secret inside the same window is not caught by it. With +A confirm turns TOTP on only while the row still holds the staged secret the confirm verified, +and TOTP is still off (BACKLOG #2224, #3025): the enable is a compare-and-set on the stored +`totp_secret` cell the code was checked against. If a reset clears the secret during the confirm, +another confirm turns TOTP on first, or a second enrolment start re-stages a different secret +inside the same window, the confirm turns nothing on and returns no recovery codes. It ends the +session it just rotated and answers as a lost session: `401` on `POST /me/mfa/confirm`, and the +sign-in page in the web console. It is audited `auth.mfa_enroll_refused`. Its `reason` is +`account_gone`, `already_enabled`, `secret_cleared`, `secret_replaced` or `write_lost`, read from +the row after the refusal, so it points an operator at the likely cause rather than proving it. +`write_lost` means the row still held the verified secret with TOTP off, so the enable lost to +some other concurrent write, and a retry confirms. With `[security].require_mfa` on — **the default since BACKLOG #187 (secure-by-default, including the loopback bind)** — **every account** must satisfy MFA under the default scope, `every_local_account` (a value now wider than its name, BACKLOG #1144). Setting the scope to diff --git a/docs/adr/0171-offline-administrator-unlock-a-host-gated-cli-recovery-path-for-a-sole-administrator-lockout.md b/docs/adr/0171-offline-administrator-unlock-a-host-gated-cli-recovery-path-for-a-sole-administrator-lockout.md index cac8fa2445..713c0256f8 100644 --- a/docs/adr/0171-offline-administrator-unlock-a-host-gated-cli-recovery-path-for-a-sole-administrator-lockout.md +++ b/docs/adr/0171-offline-administrator-unlock-a-host-gated-cli-recovery-path-for-a-sole-administrator-lockout.md @@ -312,6 +312,13 @@ writes only where TOTP is off, and passes through a state the property above forbids. The shared TOTP store contract, `tests/_webauthn_store_contract.py`, covers the method on every backend. +> **Correction (2026-10-16, BACKLOG #3025).** The paragraph above describes the store methods as +> they stood when this amendment was written. Since #3025, `set_totp_secret` also writes only where +> TOTP is off, and returns the stored cell or `None` when it wrote nothing; it still leaves the +> recovery codes and the step. `enable_totp` now also requires the staged cell to be the one the +> caller verified (`expected_secret_cell`). Neither existing method can now write an enabled +> factor's seed at all, so the conclusion stands: only `replace_totp_enrolment` swaps it. + ### Not addressed - The command trusts the host gate, as `admin-unlock` does. Anyone who can run it already holds the diff --git a/docs/adr/0207-a-single-use-enrolment-code-for-a-directory-account-s-first-engine-factor.md b/docs/adr/0207-a-single-use-enrolment-code-for-a-directory-account-s-first-engine-factor.md index 08a24de563..b5e1eea2b0 100644 --- a/docs/adr/0207-a-single-use-enrolment-code-for-a-directory-account-s-first-engine-factor.md +++ b/docs/adr/0207-a-single-use-enrolment-code-for-a-directory-account-s-first-engine-factor.md @@ -84,6 +84,11 @@ federated sign-in on a site that turned the claim gate off. the enable, decides which authenticator the account ends up holding. The session that proved the first code is stamped all the same. This gap is in the shared step, so it reaches every account type. The Lander's review of this record says it goes to the maintainer ledger as its own row. + + *Note (2026-10-16).* The shared-step fix was built under BACKLOG #3025, on branch `b259-3025`: + `enable_totp` turns TOTP on only while the row still holds the secret cell the confirm + verified, and `set_totp_secret` writes only where TOTP is off. This item describes the code as + it stood when this record was drafted. 7. **The passkey finish route draws the console write floor, but not the ceremony budget.** The engine API's `POST /me/mfa/confirm` and the web console's TOTP confirm route (`ui_mfa_verify`) both draw the per-actor ceremony budget (`allow_reauth_attempt`) before the service runs. The From 9036293fd24dec902da49171cb9b36485dd5492d Mon Sep 17 00:00:00 2001 From: Scott Hall Date: Fri, 16 Oct 2026 19:02:18 -0500 Subject: [PATCH 05/18] Review round 1: the stale-plan skip is the reconciler's alone (BACKLOG #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. --- docs/SECURITY.md | 5 +- ...directory-account-s-first-engine-factor.md | 2 +- messagefoundry/auth/service.py | 54 +++++++++++------- tests/test_ad_session_reconcile.py | 56 +++++++++++++++---- 4 files changed, 85 insertions(+), 32 deletions(-) diff --git a/docs/SECURITY.md b/docs/SECURITY.md index 83d4249ed5..4cea493189 100644 --- a/docs/SECURITY.md +++ b/docs/SECURITY.md @@ -2771,7 +2771,10 @@ A confirm turns TOTP on only while the row still holds the staged secret the con and TOTP is still off (BACKLOG #2224, #3025): the enable is a compare-and-set on the stored `totp_secret` cell the code was checked against. If a reset clears the secret during the confirm, another confirm turns TOTP on first, or a second enrolment start re-stages a different secret -inside the same window, the confirm turns nothing on and returns no recovery codes. It ends the +inside the same window, the confirm turns nothing on and returns no recovery codes. The compare is +on the stored ciphertext, so at least a store-key rotation that re-encrypts the column inside the +window refuses too, and reads as `secret_replaced` below although no second enrolment started; a +retry confirms. It ends the session it just rotated and answers as a lost session: `401` on `POST /me/mfa/confirm`, and the sign-in page in the web console. It is audited `auth.mfa_enroll_refused`. Its `reason` is `account_gone`, `already_enabled`, `secret_cleared`, `secret_replaced` or `write_lost`, read from diff --git a/docs/adr/0207-a-single-use-enrolment-code-for-a-directory-account-s-first-engine-factor.md b/docs/adr/0207-a-single-use-enrolment-code-for-a-directory-account-s-first-engine-factor.md index b5e1eea2b0..ba83be3f5e 100644 --- a/docs/adr/0207-a-single-use-enrolment-code-for-a-directory-account-s-first-engine-factor.md +++ b/docs/adr/0207-a-single-use-enrolment-code-for-a-directory-account-s-first-engine-factor.md @@ -85,7 +85,7 @@ federated sign-in on a site that turned the claim gate off. first code is stamped all the same. This gap is in the shared step, so it reaches every account type. The Lander's review of this record says it goes to the maintainer ledger as its own row. - *Note (2026-10-16).* The shared-step fix was built under BACKLOG #3025, on branch `b259-3025`: + *Note (2026-10-16).* The shared-step fix was built under BACKLOG #3025 (first pushed as branch `b259-3025`): `enable_totp` turns TOTP on only while the row still holds the secret cell the confirm verified, and `set_totp_secret` writes only where TOTP is off. This item describes the code as it stood when this record was drafted. diff --git a/messagefoundry/auth/service.py b/messagefoundry/auth/service.py index f991cccdf8..6fd2389220 100644 --- a/messagefoundry/auth/service.py +++ b/messagefoundry/auth/service.py @@ -6512,6 +6512,7 @@ async def reconcile_directory_sessions(self) -> reconcile.ReconcilePlan: old_username=refresh.old_username, new_username=refresh.new_username, held=await self._load_user_by_username(refresh.new_username), + from_plan=True, ) # BACKLOG #2538. After the revocations, like the hold below: the referred accounts were left # unjudged, and every other account's revocation above has already been applied. @@ -6778,9 +6779,16 @@ async def _refresh_cached_username( new_username: str, held: UserRecord | None, client: str | None = None, + from_plan: bool = False, ) -> None: """Copy a directory-reported rename down onto a row's cached ``username`` (BACKLOG #1532). + ``from_plan`` is True for the reconciler, whose ``new_username`` comes from a probe made + earlier in the pass and can be older than a rename a sign-in wrote since. Only then does a + row that holds neither name mean the request is stale (BACKLOG #3069). The login caller's + name comes from the bind it just made, so it is the newest the engine has, and it still + writes over whatever the row holds. + **ONE implementation for two callers, deliberately.** The login path (:meth:`_upsert_ad_user`) and the ADR 0079 reconciler pass both reach a renamed account and both must resolve the collision case the same way. Two copies of this decision would be two @@ -6860,9 +6868,9 @@ async def _refuse(held_by: str | None, detected: str, holds: str) -> None: # ``old_username`` (BACKLOG #2291). The reconciler captured that at plan time, and a # sign-in may have renamed the row since, so it can name an account that no longer # exists. The audit actor and the warning both use the name an operator would find. - # Since BACKLOG #3069 skips a plan whose old name the row no longer holds, every caller - # past the pre-read passes a name equal to ``old_username``; the parameter is kept so - # the audit's source stays the row as read, not the plan. + # Since BACKLOG #3069 the reconciler's stale plan is skipped before any refusal, so on + # that path the two names agree. On the login path they can still differ, when a pass + # wrote between the sign-in's read and this one. await self._audit( "auth.ad_username_refresh_conflict", actor=holds, @@ -6912,9 +6920,9 @@ async def _refuse(held_by: str | None, detected: str, holds: str) -> None: # read. # # A row that already carries the new name has nothing to change, so it gets no second audit - # row and no second notice. A row that carries neither name was renamed by someone else - # since the caller read it, so the plan is stale and is skipped (BACKLOG #3069); past that - # check the row holds the caller's old name, so the notice and every refusal name it. + # row and no second notice. On the reconciler's path, a row that carries neither name was + # renamed by someone else since the plan, so the plan is stale and is skipped (BACKLOG + # #3069). The notice and every refusal name what the row held. # The write below compares on this name (BACKLOG #2290), so a refresh that read it just # before another refresh of this row wrote cannot write as well. And each refusal after # this read audits under this name (BACKLOG #2291), so the read comes BEFORE the ``held`` @@ -6945,23 +6953,31 @@ async def _refuse(held_by: str | None, detected: str, holds: str) -> None: user_id, ) return - if before.username != old_username: - # BACKLOG #3069. The row holds neither the caller's old name nor its new one, so another - # writer renamed it after the caller read it: a sign-in after the reconciler planned, in - # the case that motivated this. The compare-and-set below compares on the name just - # read, so it would match and write this caller's older name over the newer one. Skip - # instead, and let the next pass or sign-in plan from a fresh read. Nothing was - # attempted, so no audit row, no conflict and no notice; INFO says why. + if from_plan and before.username != old_username: + # BACKLOG #3069. The row holds neither the plan's old name nor its new one, so another + # writer renamed it after the reconciler planned, most likely a sign-in carrying a + # newer directory name. The compare-and-set below compares on the name just read, so it + # would match and write the plan's older name over the newer one. Skip instead, and let + # the next pass plan from a fresh read. Nothing was attempted, so no audit row, no + # conflict and no notice; INFO says why. + # + # RECONCILER ONLY. A sign-in's name comes from the bind it just made. Skipping it here + # would keep a name the reconciler wrote from an older probe, the inverse of the point. + # + # NOT A VERSION CHECK. Name equality cannot see a directory that renamed the account + # away and back (jdoe -> jdoe-married -> jdoe): the row still holds the plan's old name, + # so the plan writes. The next pass's probe copies the directory's name down again. # # AFTER the already-applied check, because a row that holds the new name is the no-op # above, not a skip. BEFORE the ``held`` pre-check, because a stale plan is not a # collision: filing one would send an operator looking for a holder this caller has no # current claim against. _log.info( - "AD account %s was renamed by another writer after this rename refresh was " - "planned (now %s), so the stale plan wrote nothing and sends no audit row or " - "notice (BACKLOG #3069)", + "AD account %s was renamed by another writer after this reconciler pass planned " + "its rename to %s (now %s), so the stale plan wrote nothing and sends no audit " + "row or notice (BACKLOG #3069)", user_id, + scrub_log_argument(new_username), scrub_log_argument(before.username), ) return @@ -7039,9 +7055,9 @@ async def _refuse(held_by: str | None, detected: str, holds: str) -> None: # does neither. It is not a conflict either, because no other row holds anything. The # first writer wins, so when two refreshes carry DIFFERENT new names and both read the # row before either wrote, the stored one can be the older of the two until the next - # sign-in or pass copies the directory's name down again. In sequence the BACKLOG #3069 - # skip above stops the older one before it writes. Logged at INFO so the skipped write - # is visible without paging anyone. + # sign-in or pass copies the directory's name down again. The BACKLOG #3069 skip above + # does not order these: it stops only a reconciler plan that a write overtook before + # this pre-read. Logged at INFO so the skipped write is visible without paging anyone. if not written: after = await self._load_user(user_id) if after is None: diff --git a/tests/test_ad_session_reconcile.py b/tests/test_ad_session_reconcile.py index 13eed41615..92838adaa9 100644 --- a/tests/test_ad_session_reconcile.py +++ b/tests/test_ad_session_reconcile.py @@ -2599,8 +2599,8 @@ async def test_a_rename_already_applied_by_another_caller_is_not_told_twice( case in sequence. When both refreshes read the old name at once, the store's compare-and-set stops the second one instead (BACKLOG #2290, ``test_two_refreshes_of_one_rename_at_once_write_audit_and_notify_once``). - Driven on the method directly, with ``held`` set as the reconciler reads it, because the pass - offers no hook between its plan and its apply. + Driven on the method directly, with ``held`` set as the reconciler reads it, so the sign-in + lands exactly between the plan and the apply. """ store, _ldap, service, notifier, user_id = await _renamed_service() try: @@ -2634,8 +2634,8 @@ async def _rename_audits(store: MessageStore) -> list[aiosqlite.Row]: """Every audit row a rename refresh writes, success or refusal.""" return [ a - for a in await store.list_audit() - if a["action"] in ("auth.ad_username_refreshed", "auth.ad_username_refresh_conflict") + for action in ("auth.ad_username_refreshed", "auth.ad_username_refresh_conflict") + for a in await store.list_audit(action=action, limit=10_000) ] @@ -2690,6 +2690,33 @@ async def _sign_in_renames_first(username: str) -> VerifiedUser | None: await store.close() +async def test_a_sign_in_rename_is_not_skipped_when_a_pass_wrote_an_older_name_first() -> None: + """BACKLOG #3069, the other caller. A pass applies its plan ``jdoe`` -> ``jdoe-married`` between + a sign-in's read of the row (still ``jdoe``) and its refresh, which carries the directory's + newer ``jdoe-remarried``. The sign-in's name comes from the bind it just made, so it wins: the + stale-plan skip is the reconciler's alone, and the login writes, audits and notifies once.""" + store, _ldap, service, notifier, user_id = await _renamed_service() + try: + assert await store.set_user_username(user_id, "jdoe-married", expected_username="jdoe") + + await service._refresh_cached_username( + user_id=user_id, old_username="jdoe", new_username="jdoe-remarried", held=None + ) + + after = await store.get_user(user_id) + assert after is not None and after.username == "jdoe-remarried", ( + "a sign-in's current directory name lost to an older name a pass wrote" + ) + [refreshed] = [ + a for a in await _rename_audits(store) if a["action"] == "auth.ad_username_refreshed" + ] + assert refreshed["actor"] == "jdoe-remarried" + [notice] = [e for e in notifier.sent if e.event_type == USERNAME_CHANGED] + assert notice.detail["old_username"] == "jdoe-married" + finally: + await store.close() + + async def test_a_stale_plan_is_skipped_not_filed_as_a_conflict_when_another_row_holds_the_name( caplog: pytest.LogCaptureFixture, ) -> None: @@ -2714,7 +2741,11 @@ async def test_a_stale_plan_is_skipped_not_filed_as_a_conflict_when_another_row_ caplog.clear() await service._refresh_cached_username( - user_id=user_id, old_username="jdoe", new_username="jdoe-married", held=held + user_id=user_id, + old_username="jdoe", + new_username="jdoe-married", + held=held, + from_plan=True, ) after = await store.get_user(user_id) @@ -2766,12 +2797,15 @@ async def test_a_refused_rename_is_audited_under_the_name_the_row_holds(shape: s """BACKLOG #2291. Every refusal is audited under the name the row holds, as the refresh read it, so the audit names an account an operator can find. - Since BACKLOG #3069 a refusal runs only when the row holds the caller's old name (a row - renamed since the plan is skipped first), so the row here holds the plan's ``jdoe``. What this - pins: each refusal shape reaches its branch, files one conflict under that name, and sends no - notice.""" + The row is renamed to ``jdoe-interim`` first and the refresh is passed the stale ``jdoe``. + Since BACKLOG #3069 that shape reaches a refusal only on the LOGIN caller's path (a pass that + wrote between the sign-in's read and the refresh's): the reconciler's stale plan is skipped + first. So this is driven as the login calls it, with ``from_plan`` left False. One case per way + the refresh refuses after it has read the row; each is asserted to have refused, so none passes + by never reaching the branch.""" store, _ldap, service, notifier, user_id = await _renamed_service() try: + assert await store.set_user_username(user_id, "jdoe-interim", expected_username="jdoe") held: UserRecord | None = None if shape == "pre_check": await store.create_user( @@ -2824,8 +2858,8 @@ async def _row_deleted(*a: object, **kw: object) -> bool: if a["action"] == "auth.ad_username_refresh_conflict" ] assert json.loads(conflict["detail"])["detected"] == shape - assert conflict["actor"] == "jdoe", ( - f"a {shape} refusal was not audited under the name the row held" + assert conflict["actor"] == "jdoe-interim", ( + f"a {shape} refusal audited the caller's stale name, not the one the row held" ) assert not [e for e in notifier.sent if e.event_type == USERNAME_CHANGED] finally: From 47c876c39e6758393ac74d190ee949f750b62896 Mon Sep 17 00:00:00 2001 From: Scott Hall Date: Fri, 16 Oct 2026 19:31:48 -0500 Subject: [PATCH 06/18] Review round 2: from_plan is required, residuals named once (BACKLOG #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. --- docs/SECURITY.md | 17 ++++---- messagefoundry/auth/service.py | 51 +++++++++++----------- tests/test_ad_session_reconcile.py | 69 ++++++++++++++++++++++-------- 3 files changed, 88 insertions(+), 49 deletions(-) diff --git a/docs/SECURITY.md b/docs/SECURITY.md index 4cea493189..f04ab91523 100644 --- a/docs/SECURITY.md +++ b/docs/SECURITY.md @@ -2771,14 +2771,15 @@ A confirm turns TOTP on only while the row still holds the staged secret the con and TOTP is still off (BACKLOG #2224, #3025): the enable is a compare-and-set on the stored `totp_secret` cell the code was checked against. If a reset clears the secret during the confirm, another confirm turns TOTP on first, or a second enrolment start re-stages a different secret -inside the same window, the confirm turns nothing on and returns no recovery codes. The compare is -on the stored ciphertext, so at least a store-key rotation that re-encrypts the column inside the -window refuses too, and reads as `secret_replaced` below although no second enrolment started; a -retry confirms. It ends the -session it just rotated and answers as a lost session: `401` on `POST /me/mfa/confirm`, and the -sign-in page in the web console. It is audited `auth.mfa_enroll_refused`. Its `reason` is -`account_gone`, `already_enabled`, `secret_cleared`, `secret_replaced` or `write_lost`, read from -the row after the refusal, so it points an operator at the likely cause rather than proving it. +inside the same window, the confirm turns nothing on and returns no recovery codes. The refused +confirm ends the session it just rotated and answers as a lost session: `401` on +`POST /me/mfa/confirm`, and the sign-in page in the web console. It is audited +`auth.mfa_enroll_refused`. The `reason` values it records include `account_gone`, `already_enabled`, +`secret_cleared`, `secret_replaced` and `write_lost`, each read from the row after the refusal, +so it points an operator at the likely cause rather than proving it. The compare is on the stored +ciphertext, so at least a store-key rotation that re-encrypts the column inside the window refuses +too, and reads as `secret_replaced` although no second enrolment started; signing in again and +confirming with a later code succeeds. `write_lost` means the row still held the verified secret with TOTP off, so the enable lost to some other concurrent write, and a retry confirms. With `[security].require_mfa` on — **the default since BACKLOG #187 (secure-by-default, including the diff --git a/messagefoundry/auth/service.py b/messagefoundry/auth/service.py index 6fd2389220..4a423d22ee 100644 --- a/messagefoundry/auth/service.py +++ b/messagefoundry/auth/service.py @@ -5815,6 +5815,7 @@ async def _upsert_ad_user( old_username=existing.username, new_username=principal.username, held=by_name, + from_plan=False, client=client, ) # BACKLOG #1139. AN ABSENT DIRECTORY ATTRIBUTE IS NOT AN INSTRUCTION TO ERASE. @@ -6778,16 +6779,14 @@ async def _refresh_cached_username( old_username: str, new_username: str, held: UserRecord | None, + from_plan: bool, client: str | None = None, - from_plan: bool = False, ) -> None: """Copy a directory-reported rename down onto a row's cached ``username`` (BACKLOG #1532). - ``from_plan`` is True for the reconciler, whose ``new_username`` comes from a probe made - earlier in the pass and can be older than a rename a sign-in wrote since. Only then does a - row that holds neither name mean the request is stale (BACKLOG #3069). The login caller's - name comes from the bind it just made, so it is the newest the engine has, and it still - writes over whatever the row holds. + ``from_plan`` is required, so each caller states its policy (BACKLOG #3069; the reasoning + is at the skip below, and only there). True for the reconciler: a row that holds neither + the plan's old name nor its new one is skipped. False for the login, which writes. **ONE implementation for two callers, deliberately.** The login path (:meth:`_upsert_ad_user`) and the ADR 0079 reconciler pass both reach a renamed account and @@ -6868,9 +6867,7 @@ async def _refuse(held_by: str | None, detected: str, holds: str) -> None: # ``old_username`` (BACKLOG #2291). The reconciler captured that at plan time, and a # sign-in may have renamed the row since, so it can name an account that no longer # exists. The audit actor and the warning both use the name an operator would find. - # Since BACKLOG #3069 the reconciler's stale plan is skipped before any refusal, so on - # that path the two names agree. On the login path they can still differ, when a pass - # wrote between the sign-in's read and this one. + # On the reconciler's path the two names now agree (BACKLOG #3069, the skip below). await self._audit( "auth.ad_username_refresh_conflict", actor=holds, @@ -6954,28 +6951,34 @@ async def _refuse(held_by: str | None, detected: str, holds: str) -> None: ) return if from_plan and before.username != old_username: - # BACKLOG #3069. The row holds neither the plan's old name nor its new one, so another - # writer renamed it after the reconciler planned, most likely a sign-in carrying a - # newer directory name. The compare-and-set below compares on the name just read, so it - # would match and write the plan's older name over the newer one. Skip instead, and let - # the next pass plan from a fresh read. Nothing was attempted, so no audit row, no - # conflict and no notice; INFO says why. + # BACKLOG #3069. THE ONE STATEMENT OF THIS RULE. The row holds neither the plan's old + # name (read when the pass listed its accounts) nor its new one, so another writer, + # most likely a sign-in, renamed it since. The compare-and-set below compares on the + # name just read, so it would match and could write an older name over a newer one. + # Skip, and let the next pass plan from a fresh read. Nothing was attempted, so no audit + # row, no conflict and no notice; INFO says why. # - # RECONCILER ONLY. A sign-in's name comes from the bind it just made. Skipping it here - # would keep a name the reconciler wrote from an older probe, the inverse of the point. - # - # NOT A VERSION CHECK. Name equality cannot see a directory that renamed the account - # away and back (jdoe -> jdoe-married -> jdoe): the row still holds the plan's old name, - # so the plan writes. The next pass's probe copies the directory's name down again. + # A HEURISTIC ON NAMES, NOT AN ORDERING OF DIRECTORY READS. Nothing here knows which + # directory read is newer, so at least these residuals stand: + # - the sign-in may have written before this pass's probe, so the skipped plan can + # hold the NEWER name; the label then lags until the next pass or sign-in; + # - a directory that renamed the account away and back (jdoe -> jdoe-married -> jdoe) + # leaves the row on the plan's old name, so the plan writes and notifies; + # - the login (``from_plan`` False) writes whatever it carries, so a sign-in whose + # bind ran before a pass that probed and wrote a newer name can still write the + # older one. It is exempt because its name came from a bind made moments before, + # and skipping it would as often keep an older name a pass wrote. + # Each converges on the next sign-in or pass, which copies the directory's name down. # # AFTER the already-applied check, because a row that holds the new name is the no-op # above, not a skip. BEFORE the ``held`` pre-check, because a stale plan is not a # collision: filing one would send an operator looking for a holder this caller has no # current claim against. _log.info( - "AD account %s was renamed by another writer after this reconciler pass planned " - "its rename to %s (now %s), so the stale plan wrote nothing and sends no audit " - "row or notice (BACKLOG #3069)", + "AD account %s was renamed by another writer after this reconciler pass read it, " + "so its planned rename to %s was skipped (the row now holds %s); nothing was " + "written, audited or sent, and the next pass plans from a fresh read " + "(BACKLOG #3069)", user_id, scrub_log_argument(new_username), scrub_log_argument(before.username), diff --git a/tests/test_ad_session_reconcile.py b/tests/test_ad_session_reconcile.py index 92838adaa9..b29757a0ee 100644 --- a/tests/test_ad_session_reconcile.py +++ b/tests/test_ad_session_reconcile.py @@ -2612,7 +2612,11 @@ async def test_a_rename_already_applied_by_another_caller_is_not_told_twice( caplog.set_level(logging.INFO, logger="messagefoundry.auth.service") await service._refresh_cached_username( - user_id=user_id, old_username="jdoe", new_username="jdoe-married", held=held + user_id=user_id, + old_username="jdoe", + new_username="jdoe-married", + held=held, + from_plan=True, ) assert not [e for e in notifier.sent if e.event_type == USERNAME_CHANGED] @@ -2700,7 +2704,11 @@ async def test_a_sign_in_rename_is_not_skipped_when_a_pass_wrote_an_older_name_f assert await store.set_user_username(user_id, "jdoe-married", expected_username="jdoe") await service._refresh_cached_username( - user_id=user_id, old_username="jdoe", new_username="jdoe-remarried", held=None + user_id=user_id, + old_username="jdoe", + new_username="jdoe-remarried", + held=None, + from_plan=False, ) after = await store.get_user(user_id) @@ -2775,7 +2783,11 @@ async def _record_write(*a: object, **kw: object) -> bool: store.set_user_username = _record_write # type: ignore[method-assign] await service._refresh_cached_username( - user_id=user_id, old_username="jdoe", new_username="jdoe-married", held=None + user_id=user_id, + old_username="jdoe", + new_username="jdoe-married", + held=None, + from_plan=True, ) assert writes == [], "the refresh wrote to a row it had just read as gone" @@ -2797,15 +2809,18 @@ async def test_a_refused_rename_is_audited_under_the_name_the_row_holds(shape: s """BACKLOG #2291. Every refusal is audited under the name the row holds, as the refresh read it, so the audit names an account an operator can find. - The row is renamed to ``jdoe-interim`` first and the refresh is passed the stale ``jdoe``. - Since BACKLOG #3069 that shape reaches a refusal only on the LOGIN caller's path (a pass that - wrote between the sign-in's read and the refresh's): the reconciler's stale plan is skipped - first. So this is driven as the login calls it, with ``from_plan`` left False. One case per way - the refresh refuses after it has read the row; each is asserted to have refused, so none passes - by never reaching the branch.""" + The row is renamed to ``jdoe-interim`` first and the refresh is passed the stale ``jdoe``, + driven as the login calls it (``from_plan=False``): since BACKLOG #3069 the reconciler skips + that shape before any refusal. ``pre_check`` is the exception. The login reaches this method + only with ``held=None``, so a pre-check refusal is the reconciler's alone, and there the row + still holds the plan's ``jdoe``. One case per way the refresh refuses after it has read the + row; each is asserted to have refused, so none passes by never reaching the branch.""" store, _ldap, service, notifier, user_id = await _renamed_service() try: - assert await store.set_user_username(user_id, "jdoe-interim", expected_username="jdoe") + from_plan = shape == "pre_check" + holds = "jdoe" if from_plan else "jdoe-interim" + if not from_plan: + assert await store.set_user_username(user_id, holds, expected_username="jdoe") held: UserRecord | None = None if shape == "pre_check": await store.create_user( @@ -2849,7 +2864,11 @@ async def _row_deleted(*a: object, **kw: object) -> bool: store.set_user_username = _row_deleted # type: ignore[method-assign] await service._refresh_cached_username( - user_id=user_id, old_username="jdoe", new_username="jdoe-married", held=held + user_id=user_id, + old_username="jdoe", + new_username="jdoe-married", + held=held, + from_plan=from_plan, ) [conflict] = [ @@ -2858,7 +2877,7 @@ async def _row_deleted(*a: object, **kw: object) -> bool: if a["action"] == "auth.ad_username_refresh_conflict" ] assert json.loads(conflict["detail"])["detected"] == shape - assert conflict["actor"] == "jdoe-interim", ( + assert conflict["actor"] == holds, ( f"a {shape} refusal audited the caller's stale name, not the one the row held" ) assert not [e for e in notifier.sent if e.event_type == USERNAME_CHANGED] @@ -2884,7 +2903,11 @@ async def test_a_stale_held_row_is_not_a_conflict_when_this_row_already_has_the_ caplog.set_level(logging.INFO, logger="messagefoundry.auth.service") caplog.clear() # only the refresh's own records, not the sign-in setup's await service._refresh_cached_username( - user_id=user_id, old_username="jdoe", new_username="jdoe-married", held=stale_held + user_id=user_id, + old_username="jdoe", + new_username="jdoe-married", + held=stale_held, + from_plan=True, ) assert not [ @@ -2910,7 +2933,11 @@ async def test_a_row_gone_refusal_does_not_warn_of_a_lockout( caplog.set_level(logging.WARNING, logger="messagefoundry.auth.service") caplog.clear() # only the refresh's own records, not the sign-in setup's await service._refresh_cached_username( - user_id=user_id, old_username="jdoe", new_username="jdoe-married", held=None + user_id=user_id, + old_username="jdoe", + new_username="jdoe-married", + held=None, + from_plan=False, ) [warning] = [r for r in caplog.records if r.name == "messagefoundry.auth.service"] assert "removed" in warning.getMessage() @@ -2945,7 +2972,11 @@ async def _lose_to_nobody(*a: object, **kw: object) -> bool: caplog.set_level(logging.WARNING, logger="messagefoundry.auth.service") caplog.clear() # only the refresh's own records, not the sign-in setup's await service._refresh_cached_username( - user_id=user_id, old_username="jdoe", new_username="jdoe-married", held=None + user_id=user_id, + old_username="jdoe", + new_username="jdoe-married", + held=None, + from_plan=False, ) [warning] = [r for r in caplog.records if r.name == "messagefoundry.auth.service"] assert "no row holds the new name" in warning.getMessage() @@ -3010,10 +3041,14 @@ async def _count_write( # one waiting on the barrier for ever. A task group, so if one refresh raises the other is # cancelled rather than left pending on the barrier. async with asyncio.timeout(10), asyncio.TaskGroup() as tg: - for _ in range(2): + for from_plan in (False, True): # a sign-in and a pass tg.create_task( service._refresh_cached_username( - user_id=user_id, old_username="jdoe", new_username="jdoe-married", held=None + user_id=user_id, + old_username="jdoe", + new_username="jdoe-married", + held=None, + from_plan=from_plan, ) ) From cc31972a151bf0344158e543b2561621eb274897 Mon Sep 17 00:00:00 2001 From: Scott Hall Date: Fri, 16 Oct 2026 20:05:56 -0500 Subject: [PATCH 07/18] A stale held read no longer files a false rename conflict (BACKLOG #3070) _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). --- messagefoundry/auth/service.py | 27 +++++++---- tests/test_ad_session_reconcile.py | 75 ++++++++++++++++++++++++++++++ 2 files changed, 92 insertions(+), 10 deletions(-) diff --git a/messagefoundry/auth/service.py b/messagefoundry/auth/service.py index 4a423d22ee..9a336da247 100644 --- a/messagefoundry/auth/service.py +++ b/messagefoundry/auth/service.py @@ -6793,14 +6793,15 @@ async def _refresh_cached_username( both must resolve the collision case the same way. Two copies of this decision would be two policies, and the one that ran less often would be the one nobody noticed drifting. - ``held`` is the row currently holding ``new_username``, read by the caller. Passed rather than - re-read for the reason :meth:`_upsert_ad_user` gives about ``by_name``: it puts the dependency - in the signature instead of only in prose, and here it also saves a query the login caller - **provably** already has the answer to. ``_complete_ad_login`` reads that exact row before it - calls down, and refuses outright when it exists with a different id -- so the login path - reaches this method only with ``held=None``, and the collision branch below is reachable from - the **reconciler alone**, which probes an account it has already identified and has no such - guard. + ``held`` is the row holding ``new_username`` when the caller read it. It is a hint, not the + verdict: a ``held`` naming another row is re-read after this method's own read of the row, + and only the fresh holder is refused and audited (BACKLOG #3070), because the caller's read + is older and the holder may have let the name go since. ``held=None`` costs no read, which + is what it saves the login caller, which **provably** already has the answer. + ``_complete_ad_login`` reads that exact row before it calls down, and refuses outright when + it exists with a different id -- so the login path reaches this method only with + ``held=None``, and the collision branch below is reachable from the **reconciler alone**, + which probes an account it has already identified and has no such guard. **THE CHECK-THEN-ACT RACE IS REAL AND IS ABSORBED BELOW, NOT PREVENTED BY THE STORE.** This docstring used to say the store's in-statement guard made "a row claiming the name between @@ -6985,8 +6986,14 @@ async def _refuse(held_by: str | None, detected: str, holds: str) -> None: ) return if held is not None and held.id != user_id: - await _refuse(held.id, "pre_check", before.username) - return + # BACKLOG #3070. The caller read ``held`` before ``before``, so the holder may have let + # the name go since. Filing on that read would audit a conflict and warn of a lockout + # nobody faces, so a conflict is re-read here, after ``before``, and the refusal names + # the row that holds the name now. One extra read, on the conflict path only. + current = await self._load_user_by_username(new_username) + if current is not None and current.id != user_id: + await _refuse(current.id, "pre_check", before.username) + return try: # BACKLOG #2290. A compare-and-set on the name just read, so of two refreshes of this # row running at once only one writes. diff --git a/tests/test_ad_session_reconcile.py b/tests/test_ad_session_reconcile.py index b29757a0ee..be5c6841d7 100644 --- a/tests/test_ad_session_reconcile.py +++ b/tests/test_ad_session_reconcile.py @@ -2921,6 +2921,81 @@ async def test_a_stale_held_row_is_not_a_conflict_when_this_row_already_has_the_ await store.close() +async def _refresh_with_stale_held(service: AuthService, store: MessageStore, user_id: str) -> None: + """Run the refresh as the reconciler does, with a ``held`` naming ``gone-holder``: a record of + another row that held the name when the caller read it, and holds it no more (BACKLOG #3070).""" + row = await store.get_user(user_id) + assert row is not None + await service._refresh_cached_username( + user_id=user_id, + old_username="jdoe", + new_username="jdoe-married", + held=replace(row, id="gone-holder", username="jdoe-married"), + from_plan=True, + ) + + +async def test_a_stale_held_row_that_let_the_name_go_files_no_conflict( + caplog: pytest.LogCaptureFixture, +) -> None: + """BACKLOG #3070. The reconciler's ``held`` named another row, and by the refresh no row holds + the name. The rename writes, audits and notifies once, and nothing warns of a lockout. Before + #3070 this filed a ``pre_check`` conflict naming ``gone-holder``.""" + store, _ldap, service, notifier, user_id = await _renamed_service() + try: + caplog.set_level(logging.INFO, logger="messagefoundry.auth.service") + caplog.clear() + + await _refresh_with_stale_held(service, store, user_id) + + after = await store.get_user(user_id) + assert after is not None and after.username == "jdoe-married" + assert [a["action"] for a in await _rename_audits(store)] == [ + "auth.ad_username_refreshed" + ], "a stale held row filed a conflict for a name nobody holds" + assert not [r for r in caplog.records if r.levelno >= logging.WARNING], ( + "a stale held row warned of a lockout nobody faces" + ) + assert len([e for e in notifier.sent if e.event_type == USERNAME_CHANGED]) == 1 + finally: + await store.close() + + +async def test_a_stale_held_row_refusal_names_the_row_that_holds_the_name_now( + caplog: pytest.LogCaptureFixture, +) -> None: + """BACKLOG #3070, the control. The reconciler's ``held`` named ``gone-holder``, and a different + row holds the name now. The refresh still refuses, and the audit and the warning name the + current holder, not the stale ``held``.""" + store, _ldap, service, notifier, user_id = await _renamed_service() + try: + await store.create_user( + user_id="holder-row", + username="jdoe-married", + auth_provider="ad", + password_generated=False, + ) + caplog.set_level(logging.INFO, logger="messagefoundry.auth.service") + caplog.clear() + + await _refresh_with_stale_held(service, store, user_id) + + after = await store.get_user(user_id) + assert after is not None and after.username == "jdoe" + [conflict] = await _rename_audits(store) + assert conflict["action"] == "auth.ad_username_refresh_conflict" + detail = json.loads(conflict["detail"]) + assert detail["detected"] == "pre_check" + assert detail["held_by_user_id"] == "holder-row", ( + "the conflict audit named the caller's stale holder, not the current one" + ) + warnings = [r for r in caplog.records if r.levelno >= logging.WARNING] + assert ["holder-row" in r.getMessage() for r in warnings] == [True] + assert not [e for e in notifier.sent if e.event_type == USERNAME_CHANGED] + finally: + await store.close() + + async def test_a_row_gone_refusal_does_not_warn_of_a_lockout( caplog: pytest.LogCaptureFixture, ) -> None: From 20d01a3156cd28527f6e84a2986d962b003af7df Mon Sep 17 00:00:00 2001 From: Scott Hall Date: Fri, 16 Oct 2026 20:28:43 -0500 Subject: [PATCH 08/18] Review round 1: a holder failing its seal still holds the name (BACKLOG #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. --- messagefoundry/auth/service.py | 33 ++++++--- tests/test_ad_session_reconcile.py | 111 ++++++++++++++++++++++++++++- 2 files changed, 132 insertions(+), 12 deletions(-) diff --git a/messagefoundry/auth/service.py b/messagefoundry/auth/service.py index 9a336da247..47254b9473 100644 --- a/messagefoundry/auth/service.py +++ b/messagefoundry/auth/service.py @@ -6794,10 +6794,9 @@ async def _refresh_cached_username( policies, and the one that ran less often would be the one nobody noticed drifting. ``held`` is the row holding ``new_username`` when the caller read it. It is a hint, not the - verdict: a ``held`` naming another row is re-read after this method's own read of the row, - and only the fresh holder is refused and audited (BACKLOG #3070), because the caller's read - is older and the holder may have let the name go since. ``held=None`` costs no read, which - is what it saves the login caller, which **provably** already has the answer. + verdict: when it names another row, the holder of ``new_username`` is read again before any + refusal (BACKLOG #3070; the reasoning is at that read, and only there). ``held=None`` costs + no read, which is what it saves the login caller, which **provably** already has the answer. ``_complete_ad_login`` reads that exact row before it calls down, and refuses outright when it exists with a different id -- so the login path reaches this method only with ``held=None``, and the collision branch below is reachable from the **reconciler alone**, @@ -6986,13 +6985,25 @@ async def _refuse(held_by: str | None, detected: str, holds: str) -> None: ) return if held is not None and held.id != user_id: - # BACKLOG #3070. The caller read ``held`` before ``before``, so the holder may have let - # the name go since. Filing on that read would audit a conflict and warn of a lockout - # nobody faces, so a conflict is re-read here, after ``before``, and the refusal names - # the row that holds the name now. One extra read, on the conflict path only. - current = await self._load_user_by_username(new_username) - if current is not None and current.id != user_id: - await _refuse(current.id, "pre_check", before.username) + # BACKLOG #3070. THE ONE STATEMENT OF THIS RULE. The caller read ``held`` before + # ``before``, so its holder may have let the name go since, and filing on that read + # would audit a conflict and warn of a lockout nobody faces. So the holder of + # ``new_username`` is read again here, after ``before``, and only a row that holds the + # name now is refused; the audit names that row. If nobody holds it, or this row does + # (a sign-in renamed it just now, and the compare-and-set below then writes nothing), + # it is no conflict. A verified holder load, not one statement, but only when the + # caller's read named another row. + # + # A row that fails its seal still holds its name (BACKLOG #2592) and still locks this + # account out at its next sign-in, so it is refused too. Its id is not exposed + # unverified, so the audit names the holder the caller verified, as before #3070. + found = await self._lookup_by_username(new_username) + if found.refused or (found.user is not None and found.user.id != user_id): + await _refuse( + found.user.id if found.user is not None else held.id, + "pre_check", + before.username, + ) return try: # BACKLOG #2290. A compare-and-set on the name just read, so of two refreshes of this diff --git a/tests/test_ad_session_reconcile.py b/tests/test_ad_session_reconcile.py index be5c6841d7..e78d9c28a1 100644 --- a/tests/test_ad_session_reconcile.py +++ b/tests/test_ad_session_reconcile.py @@ -47,7 +47,7 @@ USERNAME_CHANGED, ) from messagefoundry.auth.permissions import Role -from messagefoundry.auth.service import AuthService, DirectoryObjectIdMissing +from messagefoundry.auth.service import AuthService, DirectoryObjectIdMissing, _UserLookup from messagefoundry.config.settings import AuthSettings from messagefoundry.store.crypto import CipherError from messagefoundry.store.store import ( @@ -2996,6 +2996,115 @@ async def test_a_stale_held_row_refusal_names_the_row_that_holds_the_name_now( await store.close() +async def test_a_pass_whose_holder_read_went_stale_writes_the_rename( + caplog: pytest.LogCaptureFixture, +) -> None: + """BACKLOG #3070, through the real reconciler pass. The pass reads ``held`` and finds + ``gone-holder`` on ``jdoe-married``; before the refresh reads the row, that holder renames itself + away. The hook is the pass's own ``held`` read, so the refresh's re-read, which goes through + ``_lookup_by_username`` and not this hook, sees the store as it is. The rename lands once, with + no conflict audit and no lockout warning.""" + store, _ldap, service, notifier, user_id = await _renamed_service() + try: + load_by_name = service._load_user_by_username + hooked: list[str] = [] + + async def _holder_lets_go(username: str) -> VerifiedUser | None: + if username != "jdoe-married" or hooked: + return await load_by_name(username) + hooked.append(username) + await store.create_user( + user_id="gone-holder", + username="jdoe-married", + auth_provider="ad", + password_generated=False, + ) + seen = await load_by_name(username) + assert seen is not None and seen.id == "gone-holder" + assert await store.set_user_username( + "gone-holder", "jdoe-retired", expected_username="jdoe-married" + ) + return seen + + service._load_user_by_username = _holder_lets_go # type: ignore[method-assign] + caplog.set_level(logging.INFO, logger="messagefoundry.auth.service") + caplog.clear() + + plan = await service.reconcile_directory_sessions() + + assert plan.aborted is None + assert hooked, "the pass never read the holder of the new name" + after = await store.get_user(user_id) + assert after is not None and after.username == "jdoe-married" + assert [a["action"] for a in await _rename_audits(store)] == [ + "auth.ad_username_refreshed" + ], "a stale held read in the pass filed a conflict" + assert not [ + r + for r in caplog.records + if r.levelno >= logging.WARNING and "already held" in r.getMessage() + ], "a stale held read in the pass warned of a lockout" + assert len([e for e in notifier.sent if e.event_type == USERNAME_CHANGED]) == 1 + finally: + await store.close() + + +async def test_a_stale_held_row_is_no_conflict_when_this_row_took_the_name_since( + caplog: pytest.LogCaptureFixture, +) -> None: + """BACKLOG #3070. ``held`` is stale, and between the refresh's read of the row and its re-read + of the holder a sign-in gives the name to this very row. That is not a conflict against itself: + the compare-and-set on the name read first writes nothing, and the refresh says so at INFO with + no audit row, notice or warning (BACKLOG #2290).""" + store, _ldap, service, notifier, user_id = await _renamed_service() + try: + lookup = service._lookup_by_username + + async def _sign_in_renames_first(username: str) -> _UserLookup: + assert await store.set_user_username(user_id, username, expected_username="jdoe") + return await lookup(username) + + service._lookup_by_username = _sign_in_renames_first # type: ignore[method-assign] + caplog.set_level(logging.INFO, logger="messagefoundry.auth.service") + caplog.clear() + + await _refresh_with_stale_held(service, store, user_id) + + after = await store.get_user(user_id) + assert after is not None and after.username == "jdoe-married" + assert not await _rename_audits(store), "this row filed a conflict against itself" + assert not [e for e in notifier.sent if e.event_type == USERNAME_CHANGED] + assert not [r for r in caplog.records if r.levelno >= logging.WARNING] + assert any("BACKLOG #2290" in r.getMessage() for r in caplog.records) + finally: + await store.close() + + +async def test_a_holder_that_fails_its_seal_is_still_a_conflict() -> None: + """BACKLOG #3070 with #2592. A row that fails its seal still holds its name and still locks + this account out at its next sign-in, so the re-read refuses on it rather than reading it as + "nobody holds the name". Its id is not exposed unverified, so the audit names the holder the + caller verified.""" + store, _ldap, service, notifier, user_id = await _renamed_service() + try: + + async def _unsealed_holder(username: str) -> _UserLookup: + return _UserLookup(None, True) + + service._lookup_by_username = _unsealed_holder # type: ignore[method-assign] + + await _refresh_with_stale_held(service, store, user_id) + + after = await store.get_user(user_id) + assert after is not None and after.username == "jdoe" + [conflict] = await _rename_audits(store) + detail = json.loads(conflict["detail"]) + assert (detail["detected"], detail["held_by_user_id"]) == ("pre_check", "gone-holder") + assert not [e for e in notifier.sent if e.event_type == USERNAME_CHANGED] + finally: + await store.close() + + async def test_a_row_gone_refusal_does_not_warn_of_a_lockout( caplog: pytest.LogCaptureFixture, ) -> None: From 1c90908d493fe0fc8c77e171e1edc38d97f816a8 Mon Sep 17 00:00:00 2001 From: Scott Hall Date: Fri, 16 Oct 2026 20:32:26 -0500 Subject: [PATCH 09/18] SECURITY.md: a refused TOTP confirm is retried by signing in again (BACKLOG #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. --- docs/SECURITY.md | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/docs/SECURITY.md b/docs/SECURITY.md index f04ab91523..a3d32813d1 100644 --- a/docs/SECURITY.md +++ b/docs/SECURITY.md @@ -2775,13 +2775,14 @@ inside the same window, the confirm turns nothing on and returns no recovery cod confirm ends the session it just rotated and answers as a lost session: `401` on `POST /me/mfa/confirm`, and the sign-in page in the web console. It is audited `auth.mfa_enroll_refused`. The `reason` values it records include `account_gone`, `already_enabled`, -`secret_cleared`, `secret_replaced` and `write_lost`, each read from the row after the refusal, -so it points an operator at the likely cause rather than proving it. The compare is on the stored -ciphertext, so at least a store-key rotation that re-encrypts the column inside the window refuses -too, and reads as `secret_replaced` although no second enrolment started; signing in again and -confirming with a later code succeeds. +`secret_cleared`, `secret_replaced` and `write_lost`. Each is read from the row after the refusal, +so a `reason` points an operator at the likely cause rather than proving it. The compare is on the +stored ciphertext, so at least a store-key rotation that re-encrypts the column inside the window +refuses too. That refusal reads as `secret_replaced` although no second enrolment started. `write_lost` means the row still held the verified secret with TOTP off, so the enable lost to -some other concurrent write, and a retry confirms. With +some other concurrent write. A plain retry of a refused confirm gets a `401`, because the session +is ended and the code's TOTP step is spent. After a `write_lost` or a store-key rotation, the user +signs in again and confirms with a code from a later step. With `[security].require_mfa` on — **the default since BACKLOG #187 (secure-by-default, including the loopback bind)** — **every account** must satisfy MFA under the default scope, `every_local_account` (a value now wider than its name, BACKLOG #1144). Setting the scope to From 01f4a28b32e1c89a651b26ad9625eb048229e19b Mon Sep 17 00:00:00 2001 From: Scott Hall Date: Fri, 16 Oct 2026 20:34:59 -0500 Subject: [PATCH 10/18] Review repair: refusal actor rationale restored, pre_check case distinguishes 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) --- messagefoundry/auth/service.py | 33 ++++++++++++++++++++---------- tests/test_ad_session_reconcile.py | 23 ++++++++++++--------- 2 files changed, 35 insertions(+), 21 deletions(-) diff --git a/messagefoundry/auth/service.py b/messagefoundry/auth/service.py index 4a423d22ee..059ef3cf82 100644 --- a/messagefoundry/auth/service.py +++ b/messagefoundry/auth/service.py @@ -6867,7 +6867,12 @@ async def _refuse(held_by: str | None, detected: str, holds: str) -> None: # ``old_username`` (BACKLOG #2291). The reconciler captured that at plan time, and a # sign-in may have renamed the row since, so it can name an account that no longer # exists. The audit actor and the warning both use the name an operator would find. - # On the reconciler's path the two names now agree (BACKLOG #3069, the skip below). + # On the reconciler's path the two names now agree, because the BACKLOG #3069 skip below + # returns first. On the LOGIN path they can still differ, when a pass wrote between the + # sign-in's read and this one. So each ``_refuse`` call passes the name the row held, + # never ``old_username``: passing that would bring back the #2291 defect on the login + # path, an audit row naming an account that no longer exists. ``row_gone`` is the one + # exception, since no row is left to hold a name. await self._audit( "auth.ad_username_refresh_conflict", actor=holds, @@ -6917,9 +6922,9 @@ async def _refuse(held_by: str | None, detected: str, holds: str) -> None: # read. # # A row that already carries the new name has nothing to change, so it gets no second audit - # row and no second notice. On the reconciler's path, a row that carries neither name was - # renamed by someone else since the plan, so the plan is stale and is skipped (BACKLOG - # #3069). The notice and every refusal name what the row held. + # row and no second notice. The reconciler's rule for a row that carries neither name is + # stated once, at the BACKLOG #3069 skip below. The notice and every refusal name what the + # row held. # The write below compares on this name (BACKLOG #2290), so a refresh that read it just # before another refresh of this row wrote cannot write as well. And each refusal after # this read audits under this name (BACKLOG #2291), so the read comes BEFORE the ``held`` @@ -6955,20 +6960,24 @@ async def _refuse(held_by: str | None, detected: str, holds: str) -> None: # name (read when the pass listed its accounts) nor its new one, so another writer, # most likely a sign-in, renamed it since. The compare-and-set below compares on the # name just read, so it would match and could write an older name over a newer one. - # Skip, and let the next pass plan from a fresh read. Nothing was attempted, so no audit - # row, no conflict and no notice; INFO says why. + # Skip, and leave the name to a later read of the directory. Nothing was attempted, so + # no audit row, no conflict and no notice; INFO says why. # # A HEURISTIC ON NAMES, NOT AN ORDERING OF DIRECTORY READS. Nothing here knows which # directory read is newer, so at least these residuals stand: # - the sign-in may have written before this pass's probe, so the skipped plan can - # hold the NEWER name; the label then lags until the next pass or sign-in; + # hold the NEWER name; the label then lags until a later read, below; # - a directory that renamed the account away and back (jdoe -> jdoe-married -> jdoe) # leaves the row on the plan's old name, so the plan writes and notifies; # - the login (``from_plan`` False) writes whatever it carries, so a sign-in whose # bind ran before a pass that probed and wrote a newer name can still write the # older one. It is exempt because its name came from a bind made moments before, # and skipping it would as often keep an older name a pass wrote. - # Each converges on the next sign-in or pass, which copies the directory's name down. + # Each converges when a later read copies the directory's name down: the account's next + # sign-in, or a pass that probes it. That need not be the next pass, because a pass + # probes the least-recently-probed accounts first, at most + # ``ad_session_recheck_max_users`` of them (``reconcile.select_candidates``), so the + # label can lag several passes. # # AFTER the already-applied check, because a row that holds the new name is the no-op # above, not a skip. BEFORE the ``held`` pre-check, because a stale plan is not a @@ -6977,8 +6986,8 @@ async def _refuse(held_by: str | None, detected: str, holds: str) -> None: _log.info( "AD account %s was renamed by another writer after this reconciler pass read it, " "so its planned rename to %s was skipped (the row now holds %s); nothing was " - "written, audited or sent, and the next pass plans from a fresh read " - "(BACKLOG #3069)", + "written, audited or sent, and the name is copied down when a later pass probes " + "the account or it signs in again (BACKLOG #3069)", user_id, scrub_log_argument(new_username), scrub_log_argument(before.username), @@ -10222,7 +10231,9 @@ async def confirm_mfa_enrollment( reason = "secret_replaced" else: # The row still holds the verified cell with TOTP off, so the write lost to some - # other concurrent write to the row; a retry confirms. + # other concurrent write to the row. A plain retry gets a 401, because the session + # was revoked above and this code's step is consumed; the user signs in again and + # confirms with a code from a later step. reason = "write_lost" await self._audit( "auth.mfa_enroll_refused", diff --git a/tests/test_ad_session_reconcile.py b/tests/test_ad_session_reconcile.py index b29757a0ee..6b5c11b99e 100644 --- a/tests/test_ad_session_reconcile.py +++ b/tests/test_ad_session_reconcile.py @@ -2810,17 +2810,20 @@ async def test_a_refused_rename_is_audited_under_the_name_the_row_holds(shape: s so the audit names an account an operator can find. The row is renamed to ``jdoe-interim`` first and the refresh is passed the stale ``jdoe``, - driven as the login calls it (``from_plan=False``): since BACKLOG #3069 the reconciler skips - that shape before any refusal. ``pre_check`` is the exception. The login reaches this method - only with ``held=None``, so a pre-check refusal is the reconciler's alone, and there the row - still holds the plan's ``jdoe``. One case per way the refresh refuses after it has read the - row; each is asserted to have refused, so none passes by never reaching the branch.""" + driven with ``from_plan=False``: since BACKLOG #3069 the reconciler skips that shape before + any refusal, so on its path the two names agree and the actor check could not tell them apart. + One case per way the refresh refuses after it has read the row; each is asserted to have + refused, so none passes by never reaching the branch. + + ``pre_check`` is a direct call no production caller makes today. The login reaches this method + only with ``held=None`` (``_complete_ad_login`` refuses a held name first), and the reconciler, + the only caller passing ``held``, has the two names equal there. The case is kept because it + pins the method's own contract: a refusal names what the row held, whichever caller passes a + holder.""" store, _ldap, service, notifier, user_id = await _renamed_service() try: - from_plan = shape == "pre_check" - holds = "jdoe" if from_plan else "jdoe-interim" - if not from_plan: - assert await store.set_user_username(user_id, holds, expected_username="jdoe") + holds = "jdoe-interim" + assert await store.set_user_username(user_id, holds, expected_username="jdoe") held: UserRecord | None = None if shape == "pre_check": await store.create_user( @@ -2868,7 +2871,7 @@ async def _row_deleted(*a: object, **kw: object) -> bool: old_username="jdoe", new_username="jdoe-married", held=held, - from_plan=from_plan, + from_plan=False, ) [conflict] = [ From bb2609083100e4895f1783cb6986467487a59ea6 Mon Sep 17 00:00:00 2001 From: Scott Hall Date: Fri, 16 Oct 2026 20:53:53 -0500 Subject: [PATCH 11/18] Review round 2: the re-read keeps one holder rule (BACKLOG #3070) 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). --- messagefoundry/auth/service.py | 25 ++++++-------- tests/test_ad_session_reconcile.py | 55 +++++++++++------------------- 2 files changed, 30 insertions(+), 50 deletions(-) diff --git a/messagefoundry/auth/service.py b/messagefoundry/auth/service.py index 47254b9473..4d9193619b 100644 --- a/messagefoundry/auth/service.py +++ b/messagefoundry/auth/service.py @@ -6793,10 +6793,10 @@ async def _refresh_cached_username( both must resolve the collision case the same way. Two copies of this decision would be two policies, and the one that ran less often would be the one nobody noticed drifting. - ``held`` is the row holding ``new_username`` when the caller read it. It is a hint, not the - verdict: when it names another row, the holder of ``new_username`` is read again before any - refusal (BACKLOG #3070; the reasoning is at that read, and only there). ``held=None`` costs - no read, which is what it saves the login caller, which **provably** already has the answer. + ``held`` is the row holding ``new_username`` when the caller read it, and a hint rather than + the verdict: the rule for it is at the ``held`` pre-check below (BACKLOG #3070), and only + there. ``held=None`` costs no read, which saves the login caller one; it **provably** + already has the answer. ``_complete_ad_login`` reads that exact row before it calls down, and refuses outright when it exists with a different id -- so the login path reaches this method only with ``held=None``, and the collision branch below is reachable from the **reconciler alone**, @@ -6994,16 +6994,13 @@ async def _refuse(held_by: str | None, detected: str, holds: str) -> None: # it is no conflict. A verified holder load, not one statement, but only when the # caller's read named another row. # - # A row that fails its seal still holds its name (BACKLOG #2592) and still locks this - # account out at its next sign-in, so it is refused too. Its id is not exposed - # unverified, so the audit names the holder the caller verified, as before #3070. - found = await self._lookup_by_username(new_username) - if found.refused or (found.user is not None and found.user.id != user_id): - await _refuse( - found.user.id if found.user is not None else held.id, - "pre_check", - before.username, - ) + # NOT CLOSED HERE: a holder that fails its seal reads as None, here as at the caller's + # read and the two write-path holder reads below, so it is not reported as a lockout. + # That is one gap across all four reads and wants one holder lookup that keeps the seal + # state, not a special case on this read. + current = await self._load_user_by_username(new_username) + if current is not None and current.id != user_id: + await _refuse(current.id, "pre_check", before.username) return try: # BACKLOG #2290. A compare-and-set on the name just read, so of two refreshes of this diff --git a/tests/test_ad_session_reconcile.py b/tests/test_ad_session_reconcile.py index e78d9c28a1..fb8070b768 100644 --- a/tests/test_ad_session_reconcile.py +++ b/tests/test_ad_session_reconcile.py @@ -47,7 +47,7 @@ USERNAME_CHANGED, ) from messagefoundry.auth.permissions import Role -from messagefoundry.auth.service import AuthService, DirectoryObjectIdMissing, _UserLookup +from messagefoundry.auth.service import AuthService, DirectoryObjectIdMissing from messagefoundry.config.settings import AuthSettings from messagefoundry.store.crypto import CipherError from messagefoundry.store.store import ( @@ -3001,18 +3001,20 @@ async def test_a_pass_whose_holder_read_went_stale_writes_the_rename( ) -> None: """BACKLOG #3070, through the real reconciler pass. The pass reads ``held`` and finds ``gone-holder`` on ``jdoe-married``; before the refresh reads the row, that holder renames itself - away. The hook is the pass's own ``held`` read, so the refresh's re-read, which goes through - ``_lookup_by_username`` and not this hook, sees the store as it is. The rename lands once, with - no conflict audit and no lockout warning.""" + away. The hook acts on the first by-name read of ``jdoe-married``, which is the pass's ``held`` + read; the second is the refresh's re-read, which sees the store as it is. Counting both pins + that order. The rename lands once, with no conflict audit and no lockout warning.""" store, _ldap, service, notifier, user_id = await _renamed_service() try: load_by_name = service._load_user_by_username hooked: list[str] = [] async def _holder_lets_go(username: str) -> VerifiedUser | None: - if username != "jdoe-married" or hooked: + if username != "jdoe-married": return await load_by_name(username) hooked.append(username) + if len(hooked) > 1: + return await load_by_name(username) await store.create_user( user_id="gone-holder", username="jdoe-married", @@ -3033,7 +3035,9 @@ async def _holder_lets_go(username: str) -> VerifiedUser | None: plan = await service.reconcile_directory_sessions() assert plan.aborted is None - assert hooked, "the pass never read the holder of the new name" + assert hooked == ["jdoe-married", "jdoe-married"], ( + "expected the pass's held read and then the refresh's re-read, and nothing else" + ) after = await store.get_user(user_id) assert after is not None and after.username == "jdoe-married" assert [a["action"] for a in await _rename_audits(store)] == [ @@ -3058,18 +3062,22 @@ async def test_a_stale_held_row_is_no_conflict_when_this_row_took_the_name_since no audit row, notice or warning (BACKLOG #2290).""" store, _ldap, service, notifier, user_id = await _renamed_service() try: - lookup = service._lookup_by_username + load_by_name = service._load_user_by_username + hooked: list[str] = [] - async def _sign_in_renames_first(username: str) -> _UserLookup: - assert await store.set_user_username(user_id, username, expected_username="jdoe") - return await lookup(username) + async def _sign_in_renames_first(username: str) -> VerifiedUser | None: + if not hooked: + hooked.append(username) + assert await store.set_user_username(user_id, username, expected_username="jdoe") + return await load_by_name(username) - service._lookup_by_username = _sign_in_renames_first # type: ignore[method-assign] + service._load_user_by_username = _sign_in_renames_first # type: ignore[method-assign] caplog.set_level(logging.INFO, logger="messagefoundry.auth.service") caplog.clear() await _refresh_with_stale_held(service, store, user_id) + assert hooked == ["jdoe-married"], "the refresh never re-read the holder" after = await store.get_user(user_id) assert after is not None and after.username == "jdoe-married" assert not await _rename_audits(store), "this row filed a conflict against itself" @@ -3080,31 +3088,6 @@ async def _sign_in_renames_first(username: str) -> _UserLookup: await store.close() -async def test_a_holder_that_fails_its_seal_is_still_a_conflict() -> None: - """BACKLOG #3070 with #2592. A row that fails its seal still holds its name and still locks - this account out at its next sign-in, so the re-read refuses on it rather than reading it as - "nobody holds the name". Its id is not exposed unverified, so the audit names the holder the - caller verified.""" - store, _ldap, service, notifier, user_id = await _renamed_service() - try: - - async def _unsealed_holder(username: str) -> _UserLookup: - return _UserLookup(None, True) - - service._lookup_by_username = _unsealed_holder # type: ignore[method-assign] - - await _refresh_with_stale_held(service, store, user_id) - - after = await store.get_user(user_id) - assert after is not None and after.username == "jdoe" - [conflict] = await _rename_audits(store) - detail = json.loads(conflict["detail"]) - assert (detail["detected"], detail["held_by_user_id"]) == ("pre_check", "gone-holder") - assert not [e for e in notifier.sent if e.event_type == USERNAME_CHANGED] - finally: - await store.close() - - async def test_a_row_gone_refusal_does_not_warn_of_a_lockout( caplog: pytest.LogCaptureFixture, ) -> None: From 07c109aee22ae55c6e6897c9c0d3259b4bee42ba Mon Sep 17 00:00:00 2001 From: Scott Hall Date: Fri, 16 Oct 2026 21:08:24 -0500 Subject: [PATCH 12/18] SECURITY.md: the refused-confirm recovery is stated once, with its step-up (BACKLOG #3025) Review round on 1c90908d. 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. --- docs/SECURITY.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/SECURITY.md b/docs/SECURITY.md index a3d32813d1..bbc0cc82a5 100644 --- a/docs/SECURITY.md +++ b/docs/SECURITY.md @@ -2780,9 +2780,9 @@ so a `reason` points an operator at the likely cause rather than proving it. The stored ciphertext, so at least a store-key rotation that re-encrypts the column inside the window refuses too. That refusal reads as `secret_replaced` although no second enrolment started. `write_lost` means the row still held the verified secret with TOTP off, so the enable lost to -some other concurrent write. A plain retry of a refused confirm gets a `401`, because the session -is ended and the code's TOTP step is spent. After a `write_lost` or a store-key rotation, the user -signs in again and confirms with a code from a later step. With +some other concurrent write. After a `write_lost` or a store-key rotation the staged secret still +stands. The user signs in again and confirms with a code from a later step, passing the same +step-up as any confirm, because the refused confirm spent its code's step. With `[security].require_mfa` on — **the default since BACKLOG #187 (secure-by-default, including the loopback bind)** — **every account** must satisfy MFA under the default scope, `every_local_account` (a value now wider than its name, BACKLOG #1144). Setting the scope to From 3aba14d2a6fc64d5002684ca78cc0353c0592ca4 Mon Sep 17 00:00:00 2001 From: Scott Hall Date: Fri, 16 Oct 2026 21:10:22 -0500 Subject: [PATCH 13/18] Review round on 01f4a28b: exception named exactly, retry lag worded once (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) --- messagefoundry/auth/service.py | 26 +++++++------- tests/test_ad_session_reconcile.py | 11 +++--- tests/test_mfa.py | 55 ++++++++++++++++++++++++++++++ 3 files changed, 75 insertions(+), 17 deletions(-) diff --git a/messagefoundry/auth/service.py b/messagefoundry/auth/service.py index 059ef3cf82..8c06f30baf 100644 --- a/messagefoundry/auth/service.py +++ b/messagefoundry/auth/service.py @@ -6871,8 +6871,8 @@ async def _refuse(held_by: str | None, detected: str, holds: str) -> None: # returns first. On the LOGIN path they can still differ, when a pass wrote between the # sign-in's read and this one. So each ``_refuse`` call passes the name the row held, # never ``old_username``: passing that would bring back the #2291 defect on the login - # path, an audit row naming an account that no longer exists. ``row_gone`` is the one - # exception, since no row is left to hold a name. + # path, an audit row naming an account that no longer exists. The one exception is the + # ``row_gone`` refusal at the pre-read, where no row was read to hold a name. await self._audit( "auth.ad_username_refresh_conflict", actor=holds, @@ -6895,12 +6895,14 @@ async def _refuse(held_by: str | None, detected: str, holds: str) -> None: return if held_by is None: # The write lost to another writer, but the re-read found no row holding the name - # now. Nothing is locked out, and the next sign-in or pass copies the name down - # again, so the lockout warning below would be false (BACKLOG #2291). + # now. Nothing is locked out, and a later sign-in or a pass that probes the account + # copies the name down again, so the lockout warning below would be false (BACKLOG + # #2291). _log.warning( "AD account %s was renamed in the directory but the write lost to another " "writer (detected %s), and no row holds the new name now; nothing was written, " - "and the next sign-in or reconcile pass retries it (BACKLOG #2291)", + "and the account's next sign-in, or a later reconcile pass that probes it, " + "retries it (BACKLOG #2291)", scrub_log_argument(holds), detected, ) @@ -7066,10 +7068,10 @@ async def _refuse(held_by: str | None, detected: str, holds: str) -> None: # this same row wrote first (BACKLOG #2290). That caller audits and notifies, so this one # does neither. It is not a conflict either, because no other row holds anything. The # first writer wins, so when two refreshes carry DIFFERENT new names and both read the - # row before either wrote, the stored one can be the older of the two until the next - # sign-in or pass copies the directory's name down again. The BACKLOG #3069 skip above - # does not order these: it stops only a reconciler plan that a write overtook before - # this pre-read. Logged at INFO so the skipped write is visible without paging anyone. + # row before either wrote, the stored one can be the older of the two until a later + # read copies the directory's name down again. The BACKLOG #3069 skip above, which + # names when that read comes, does not order these: it stops only a reconciler plan + # that a write overtook before this pre-read. Logged at INFO so the skipped write is visible without paging anyone. if not written: after = await self._load_user(user_id) if after is None: @@ -10231,9 +10233,9 @@ async def confirm_mfa_enrollment( reason = "secret_replaced" else: # The row still holds the verified cell with TOTP off, so the write lost to some - # other concurrent write to the row. A plain retry gets a 401, because the session - # was revoked above and this code's step is consumed; the user signs in again and - # confirms with a code from a later step. + # other concurrent write to the row. The session is revoked above, so a plain + # retry is a lost session; recovery is as for a session lost before the rotation + # (the ROTATE BEFORE ENABLING comment above). reason = "write_lost" await self._audit( "auth.mfa_enroll_refused", diff --git a/tests/test_ad_session_reconcile.py b/tests/test_ad_session_reconcile.py index 6b5c11b99e..fb7d3a0353 100644 --- a/tests/test_ad_session_reconcile.py +++ b/tests/test_ad_session_reconcile.py @@ -2815,11 +2815,12 @@ async def test_a_refused_rename_is_audited_under_the_name_the_row_holds(shape: s One case per way the refresh refuses after it has read the row; each is asserted to have refused, so none passes by never reaching the branch. - ``pre_check`` is a direct call no production caller makes today. The login reaches this method - only with ``held=None`` (``_complete_ad_login`` refuses a held name first), and the reconciler, - the only caller passing ``held``, has the two names equal there. The case is kept because it - pins the method's own contract: a refusal names what the row held, whichever caller passes a - holder.""" + The ``pre_check`` case drives a shape no production caller produces today: a holder passed + while the row's name differs from ``old_username``. The reconciler reaches the pre-check in + production, but the skip has returned first unless the two names agree. The login passes + ``held`` too, but it is always None there (``_complete_ad_login`` refuses a held name first). + The case is kept because it pins the method's own contract: a refusal names what the row held, + whichever caller passes a holder.""" store, _ldap, service, notifier, user_id = await _renamed_service() try: holds = "jdoe-interim" diff --git a/tests/test_mfa.py b/tests/test_mfa.py index c8c3b71d81..fd4070ef14 100644 --- a/tests/test_mfa.py +++ b/tests/test_mfa.py @@ -2680,6 +2680,61 @@ async def elevated_then_begin(*args: Any, **kwargs: Any) -> Any: await store.close() +async def test_a_lost_enable_write_ends_the_session_and_confirms_after_a_fresh_sign_in( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """RED when: a ``write_lost`` refusal hands the session back, records another reason, or leaves + no way to confirm afterwards (BACKLOG #3025). + + The enable is made to match no row while the row still holds the verified secret with TOTP + off, which is the ``write_lost`` shape. The refused confirm must end the session. The same + step's code is spent, so after a fresh sign-in it is refused as a wrong code, and a code from + a later step confirms the same staged secret.""" + store = await _store() + try: + service = AuthService(store, AuthSettings(mfa_verify_min_elapsed_seconds=0)) + identity, token, password = await login_admin(service) + staged = await service.begin_mfa_enrollment(identity) + t0 = 1_000_000.0 + pin_totp_clock(monkeypatch, t0) + + real_enable = store.enable_totp + + async def enable_loses(*args: Any, **kwargs: Any) -> bool: + return False + + monkeypatch.setattr(store, "enable_totp", enable_loses) + refused = await service.confirm_mfa_enrollment( + identity, totp.totp(staged.secret, now=t0), token=token + ) + assert refused.ok is False and refused.session_lost is True + assert refused.recovery_codes == () + assert await store.list_sessions(identity.user_id) == [] + rows = await store.list_audit( + actor=ADMIN_USERNAME, action="auth.mfa_enroll_refused", limit=10 + ) + assert len(rows) == 1 and '"reason": "write_lost"' in str(rows[0]["detail"]) + + monkeypatch.setattr(store, "enable_totp", real_enable) + again = await service.login(ADMIN_USERNAME, password) + assert again.token is not None + replay = await service.confirm_mfa_enrollment( + identity, totp.totp(staged.secret, now=t0), token=again.token + ) + assert replay.ok is False and replay.session_lost is False, ( + "the spent step's code was accepted, or answered as a lost session" + ) + t1 = t0 + totp.DEFAULT_PERIOD + pin_totp_clock(monkeypatch, t1) + confirmed = await service.confirm_mfa_enrollment( + identity, totp.totp(staged.secret, now=t1), token=again.token + ) + assert confirmed.ok + assert (await service.mfa_status(identity)).enabled is True + finally: + await store.close() + + async def test_an_unrelated_write_inside_the_confirm_window_still_confirms( monkeypatch: pytest.MonkeyPatch, ) -> None: From b5e045519da680d979387eac5c6f1957363cd551 Mon Sep 17 00:00:00 2001 From: Scott Hall Date: Sun, 18 Oct 2026 16:29:58 -0500 Subject: [PATCH 14/18] Review round 1 on the main merge: tests take the soft-audit path, prose 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. --- changelog.d/3070.fixed.md | 7 +++++++ messagefoundry/auth/service.py | 18 +++++++++++------- tests/test_ad_session_reconcile.py | 18 +++++++++--------- 3 files changed, 27 insertions(+), 16 deletions(-) create mode 100644 changelog.d/3070.fixed.md diff --git a/changelog.d/3070.fixed.md b/changelog.d/3070.fixed.md new file mode 100644 index 0000000000..30d4004509 --- /dev/null +++ b/changelog.d/3070.fixed.md @@ -0,0 +1,7 @@ +- **A directory reconcile pass no longer files a false rename conflict from a stale holder read.** + In the shipped code the reconciler read which account held an AD account's new name, then read + the account itself, and refused the rename on the first read. A holder that let the name go in + between still produced an `auth.ad_username_refresh_conflict` audit row and a lockout warning + that nobody faced, and the rename waited for a later pass. Now the holder is read again after the + account, and only an account that holds the name then is refused. The conflict row's + `held_by_user_id` names that account, not the one the first read saw. Vault BACKLOG #3070. diff --git a/messagefoundry/auth/service.py b/messagefoundry/auth/service.py index fb6ffd1cf1..14a3c446d4 100644 --- a/messagefoundry/auth/service.py +++ b/messagefoundry/auth/service.py @@ -7677,9 +7677,10 @@ async def _refresh_cached_username( both must resolve the collision case the same way. Two copies of this decision would be two policies, and the one that ran less often would be the one nobody noticed drifting. - ``held`` is the row holding ``new_username`` when the caller read it, and a hint rather than - the verdict: the rule for it is at the ``held`` pre-check below (BACKLOG #3070), and only - there. ``held=None`` costs no read, which saves the login caller one; it **provably** + ``held`` is the row holding ``new_username`` when the caller read it. One naming another + row is re-checked, not trusted: the rule is at the ``held`` pre-check below (BACKLOG + #3070). ``held=None`` skips that pre-check, so a holder appearing after the caller's read is + caught only by the write's guard. The login caller passes None because it **provably** already has the answer. ``_complete_ad_login`` reads that exact row before it calls down, and refuses outright when it exists with a different id -- so the login path reaches this method only with @@ -7891,10 +7892,13 @@ async def _refuse(held_by: str | None, detected: str, holds: str) -> None: # it is no conflict. A verified holder load, not one statement, but only when the # caller's read named another row. # - # NOT CLOSED HERE: a holder that fails its seal reads as None, here as at the caller's - # read and the two write-path holder reads below, so it is not reported as a lockout. - # That is one gap across all four reads and wants one holder lookup that keeps the seal - # state, not a special case on this read. + # NOT CLOSED HERE: a holder that fails its seal reads as None at this re-read, at the + # reconciler's own ``held`` read, and at least at the two write-path holder reads + # below. So the write's guard stops the rename and the write-path refusal logs that no + # row holds the new name and a later pass retries it, which is false while that holder + # stays seal-failed. The login's caller read (``_lookup_by_username``) keeps the seal + # state and refuses. The gap wants one holder lookup that keeps the seal state for the + # reconciler's reads, not a special case on this one. current = await self._load_user_by_username(new_username) if current is not None and current.id != user_id: await _refuse(current.id, "pre_check", before.username) diff --git a/tests/test_ad_session_reconcile.py b/tests/test_ad_session_reconcile.py index 4f3b8cdeaa..266c0bd7e0 100644 --- a/tests/test_ad_session_reconcile.py +++ b/tests/test_ad_session_reconcile.py @@ -3265,7 +3265,8 @@ async def test_a_stale_held_row_is_not_a_conflict_when_this_row_already_has_the_ async def _refresh_with_stale_held(service: AuthService, store: MessageStore, user_id: str) -> None: - """Run the refresh as the reconciler does, with a ``held`` naming ``gone-holder``: a record of + """Run the refresh as the reconciler does (``audit_soft`` on, BACKLOG #3018), with a ``held`` + naming ``gone-holder``: a record of another row that held the name when the caller read it, and holds it no more (BACKLOG #3070).""" row = await store.get_user(user_id) assert row is not None @@ -3275,6 +3276,7 @@ async def _refresh_with_stale_held(service: AuthService, store: MessageStore, us new_username="jdoe-married", held=replace(row, id="gone-holder", username="jdoe-married"), from_plan=True, + audit_soft=True, ) @@ -3386,11 +3388,9 @@ async def _holder_lets_go(username: str) -> VerifiedUser | None: assert [a["action"] for a in await _rename_audits(store)] == [ "auth.ad_username_refreshed" ], "a stale held read in the pass filed a conflict" - assert not [ - r - for r in caplog.records - if r.levelno >= logging.WARNING and "already held" in r.getMessage() - ], "a stale held read in the pass warned of a lockout" + assert not [r for r in caplog.records if r.levelno >= logging.WARNING], ( + "a stale held read in the pass warned" + ) assert len([e for e in notifier.sent if e.event_type == USERNAME_CHANGED]) == 1 finally: await store.close() @@ -3409,8 +3409,8 @@ async def test_a_stale_held_row_is_no_conflict_when_this_row_took_the_name_since hooked: list[str] = [] async def _sign_in_renames_first(username: str) -> VerifiedUser | None: - if not hooked: - hooked.append(username) + hooked.append(username) + if len(hooked) == 1: assert await store.set_user_username(user_id, username, expected_username="jdoe") return await load_by_name(username) @@ -3420,7 +3420,7 @@ async def _sign_in_renames_first(username: str) -> VerifiedUser | None: await _refresh_with_stale_held(service, store, user_id) - assert hooked == ["jdoe-married"], "the refresh never re-read the holder" + assert hooked == ["jdoe-married"], "expected exactly one holder re-read, and nothing else" after = await store.get_user(user_id) assert after is not None and after.username == "jdoe-married" assert not await _rename_audits(store), "this row filed a conflict against itself" From e73eb5115ddaa5b19f206c4cca84e2b32bf3c7e1 Mon Sep 17 00:00:00 2001 From: Scott Hall Date: Sun, 18 Oct 2026 16:40:17 -0500 Subject: [PATCH 15/18] _refresh_cached_username re-reads the holder of the new name before refusing (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. --- changelog.d/3070.fixed.md | 6 ++++-- messagefoundry/auth/service.py | 3 +-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/changelog.d/3070.fixed.md b/changelog.d/3070.fixed.md index 30d4004509..480437d124 100644 --- a/changelog.d/3070.fixed.md +++ b/changelog.d/3070.fixed.md @@ -3,5 +3,7 @@ the account itself, and refused the rename on the first read. A holder that let the name go in between still produced an `auth.ad_username_refresh_conflict` audit row and a lockout warning that nobody faced, and the rename waited for a later pass. Now the holder is read again after the - account, and only an account that holds the name then is refused. The conflict row's - `held_by_user_id` names that account, not the one the first read saw. Vault BACKLOG #3070. + account, and the rename is refused only when another account holds the name at that second + read. The conflict row's `held_by_user_id` names that account, not the one the first read saw. + A holder whose record fails its integrity check still reads as no holder at either read, so + that case is not reported as a conflict. Vault BACKLOG #3070. diff --git a/messagefoundry/auth/service.py b/messagefoundry/auth/service.py index 14a3c446d4..1ece5e4242 100644 --- a/messagefoundry/auth/service.py +++ b/messagefoundry/auth/service.py @@ -7680,8 +7680,7 @@ async def _refresh_cached_username( ``held`` is the row holding ``new_username`` when the caller read it. One naming another row is re-checked, not trusted: the rule is at the ``held`` pre-check below (BACKLOG #3070). ``held=None`` skips that pre-check, so a holder appearing after the caller's read is - caught only by the write's guard. The login caller passes None because it **provably** - already has the answer. + caught only by the write's guard. The login caller **provably** already has the answer: ``_complete_ad_login`` reads that exact row before it calls down, and refuses outright when it exists with a different id -- so the login path reaches this method only with ``held=None``, and the collision branch below is reachable from the **reconciler alone**, From 585852db1ddc54f7413b3053bb97c296ca8c8f45 Mon Sep 17 00:00:00 2001 From: Scott Hall Date: Sun, 18 Oct 2026 17:20:34 -0500 Subject: [PATCH 16/18] Holder re-read keeps the seal state and survives a store fault (BACKLOG #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. --- changelog.d/3070.fixed.md | 7 +- messagefoundry/auth/service.py | 36 +++++-- tests/test_ad_session_reconcile.py | 157 ++++++++++++++++++++++++++--- 3 files changed, 172 insertions(+), 28 deletions(-) diff --git a/changelog.d/3070.fixed.md b/changelog.d/3070.fixed.md index 480437d124..0c8a86dc33 100644 --- a/changelog.d/3070.fixed.md +++ b/changelog.d/3070.fixed.md @@ -5,5 +5,8 @@ that nobody faced, and the rename waited for a later pass. Now the holder is read again after the account, and the rename is refused only when another account holds the name at that second read. The conflict row's `held_by_user_id` names that account, not the one the first read saw. - A holder whose record fails its integrity check still reads as no holder at either read, so - that case is not reported as a conflict. Vault BACKLOG #3070. + A holder whose record fails its integrity check at the second read is still refused with the + lockout warning, naming the holder the first read verified. A store error on the second read + skips that one rename for the pass instead of ending the pass. A holder that already fails its + integrity check at the first read still reads as no holder there, so that case is not reported + as a conflict. Vault BACKLOG #3070. diff --git a/messagefoundry/auth/service.py b/messagefoundry/auth/service.py index 1ece5e4242..7e762b0316 100644 --- a/messagefoundry/auth/service.py +++ b/messagefoundry/auth/service.py @@ -7891,16 +7891,32 @@ async def _refuse(held_by: str | None, detected: str, holds: str) -> None: # it is no conflict. A verified holder load, not one statement, but only when the # caller's read named another row. # - # NOT CLOSED HERE: a holder that fails its seal reads as None at this re-read, at the - # reconciler's own ``held`` read, and at least at the two write-path holder reads - # below. So the write's guard stops the rename and the write-path refusal logs that no - # row holds the new name and a later pass retries it, which is false while that holder - # stays seal-failed. The login's caller read (``_lookup_by_username``) keeps the seal - # state and refuses. The gap wants one holder lookup that keeps the seal state for the - # reconciler's reads, not a special case on this one. - current = await self._load_user_by_username(new_username) - if current is not None and current.id != user_id: - await _refuse(current.id, "pre_check", before.username) + # A holder that fails its seal at this re-read still holds the name, so it is refused + # as the caller's read would have been, with the lockout warning, not left to the + # write-path refusal's "a later pass retries it", which is false while that holder + # stays seal-failed. Its id is not verified, so the audit names ``held``, the last + # verified holder. NOT CLOSED HERE: the reconciler's own ``held`` read, and at least + # the two write-path holder reads below, still read a seal-failed holder as None. + # + # A store fault on this re-read skips this one rename for this pass rather than + # raising: the caller is the reconciler's rename loop, and a raise would end the pass + # before the renames after this one. Nothing was written; a later pass that probes + # the account retries it. The cached store-error tuple, so a fault never imports a + # driver on the event loop. + try: + current = await self._lookup_by_username(new_username) + except _session_write_errors() as exc: + _log.warning( + "AD account %s: reading the holder of its new directory name raised %s, so its " + "rename was not applied on this pass; a later pass that probes it retries it " + "(BACKLOG #3070)", + user_id, + type(exc).__name__, + ) + return + if current.refused or (current.user is not None and current.user.id != user_id): + holder_id = held.id if current.user is None else current.user.id + await _refuse(holder_id, "pre_check", before.username) return try: # BACKLOG #2290. A compare-and-set on the name just read, so of two refreshes of this diff --git a/tests/test_ad_session_reconcile.py b/tests/test_ad_session_reconcile.py index 266c0bd7e0..e1956c347b 100644 --- a/tests/test_ad_session_reconcile.py +++ b/tests/test_ad_session_reconcile.py @@ -26,6 +26,7 @@ import uuid from collections.abc import Callable from dataclasses import replace +from pathlib import Path from typing import Any, Literal, get_args import aiosqlite @@ -51,10 +52,12 @@ ) from messagefoundry.auth.permissions import Role from messagefoundry.auth.service import ( + AUTHORITY_SEAL_FAILED_ACTION, AuthService, DirectoryObjectIdMissing, _AuditWatermark, _RevocationApplied, + _UserLookup, ) from messagefoundry.config.settings import AuthSettings from messagefoundry.store.crypto import CipherError, generate_key, make_cipher @@ -68,6 +71,7 @@ ) from tests._admin_account import create_local_user_chosen from tests.test_approval_requester_recheck import _Sink +from tests.test_user_seal import _raw_for PW = "Sup3rSecret!!" @@ -2803,12 +2807,18 @@ async def test_an_unchanged_name_plans_no_rename() -> None: # --- BACKLOG #2017: the holder is told of a directory rename (ASVS 6.3.7) -------------------------- -async def _renamed_service() -> tuple[ - MessageStore, _FakeLdap, AuthService, _CapturingNotifier, str -]: +async def _renamed_service( + keyed_path: Path | None = None, +) -> tuple[MessageStore, _FakeLdap, AuthService, _CapturingNotifier, str]: """A signed-in ``jdoe`` with a known notification address and a capturing notifier, and the - directory already renaming it to ``jdoe-married`` for the next pass. Returns the row's id.""" - store = await MessageStore.open(":memory:") + directory already renaming it to ``jdoe-married`` for the next pass. Returns the row's id. + With ``keyed_path`` the store is a keyed file there, so every user read checks its seal.""" + if keyed_path is None: + store = await MessageStore.open(":memory:") + else: + store = await MessageStore.open( + keyed_path, cipher=make_cipher(generate_key(), write_v2=True) + ) ldap = _FakeLdap({"jdoe": _principal("jdoe")}) service = AuthService(store, _ad_settings(), ldap=ldap) # type: ignore[arg-type] notifier = _CapturingNotifier() @@ -3351,29 +3361,29 @@ async def test_a_pass_whose_holder_read_went_stale_writes_the_rename( that order. The rename lands once, with no conflict audit and no lockout warning.""" store, _ldap, service, notifier, user_id = await _renamed_service() try: - load_by_name = service._load_user_by_username + lookup_by_name = service._lookup_by_username hooked: list[str] = [] - async def _holder_lets_go(username: str) -> VerifiedUser | None: + async def _holder_lets_go(username: str) -> _UserLookup: if username != "jdoe-married": - return await load_by_name(username) + return await lookup_by_name(username) hooked.append(username) if len(hooked) > 1: - return await load_by_name(username) + return await lookup_by_name(username) await store.create_user( user_id="gone-holder", username="jdoe-married", auth_provider="ad", password_generated=False, ) - seen = await load_by_name(username) - assert seen is not None and seen.id == "gone-holder" + seen = await lookup_by_name(username) + assert seen.user is not None and seen.user.id == "gone-holder" assert await store.set_user_username( "gone-holder", "jdoe-retired", expected_username="jdoe-married" ) return seen - service._load_user_by_username = _holder_lets_go # type: ignore[method-assign] + service._lookup_by_username = _holder_lets_go # type: ignore[method-assign] caplog.set_level(logging.INFO, logger="messagefoundry.auth.service") caplog.clear() @@ -3405,16 +3415,16 @@ async def test_a_stale_held_row_is_no_conflict_when_this_row_took_the_name_since no audit row, notice or warning (BACKLOG #2290).""" store, _ldap, service, notifier, user_id = await _renamed_service() try: - load_by_name = service._load_user_by_username + lookup_by_name = service._lookup_by_username hooked: list[str] = [] - async def _sign_in_renames_first(username: str) -> VerifiedUser | None: + async def _sign_in_renames_first(username: str) -> _UserLookup: hooked.append(username) if len(hooked) == 1: assert await store.set_user_username(user_id, username, expected_username="jdoe") - return await load_by_name(username) + return await lookup_by_name(username) - service._load_user_by_username = _sign_in_renames_first # type: ignore[method-assign] + service._lookup_by_username = _sign_in_renames_first # type: ignore[method-assign] caplog.set_level(logging.INFO, logger="messagefoundry.auth.service") caplog.clear() @@ -3431,6 +3441,121 @@ async def _sign_in_renames_first(username: str) -> VerifiedUser | None: await store.close() +async def test_a_holder_that_fails_its_seal_by_the_re_read_is_refused_with_the_lockout_warning( + tmp_path: Path, caplog: pytest.LogCaptureFixture +) -> None: + """BACKLOG #3070, on a keyed store, so both reads check the holder's seal. The caller's read + verifies ``holder-row`` on ``jdoe-married``; its record is then changed outside the store, so + the refresh's re-read finds the name held by a row that fails its seal. That row still holds + the name, so the refusal is the ``pre_check`` lockout one naming the verified holder, as before + #3070, and not the write path's "no row holds the new name, a later pass retries it".""" + path = tmp_path / "sealed.db" + store, _ldap, service, notifier, user_id = await _renamed_service(path) + try: + await store.create_user( + user_id="holder-row", + username="jdoe-married", + auth_provider="ad", + password_generated=False, + ) + held = await service._load_user_by_username("jdoe-married") + assert held is not None and held.id == "holder-row", "the caller's read must verify" + await _raw_for(path)("UPDATE users SET display_name=? WHERE id=?", ("bent", "holder-row")) + caplog.set_level(logging.INFO, logger="messagefoundry.auth.service") + caplog.clear() + + await service._refresh_cached_username( + user_id=user_id, + old_username="jdoe", + new_username="jdoe-married", + held=held, + from_plan=True, + audit_soft=True, + ) + + after = await store.get_user(user_id) + assert after is not None and after.username == "jdoe" + assert len(await store.list_audit(action=AUTHORITY_SEAL_FAILED_ACTION)) == 1, ( + "the re-read did not check the holder's seal" + ) + [conflict] = await _rename_audits(store) + assert conflict["action"] == "auth.ad_username_refresh_conflict" + detail = json.loads(conflict["detail"]) + assert (detail["detected"], detail["held_by_user_id"]) == ("pre_check", "holder-row") + messages = [r.getMessage() for r in caplog.records if r.levelno >= logging.WARNING] + assert [m for m in messages if "refused at its next sign-in" in m and "holder-row" in m], ( + "a seal-failed holder at the re-read lost the lockout warning" + ) + assert not [m for m in messages if "retries it" in m] + assert not [e for e in notifier.sent if e.event_type == USERNAME_CHANGED] + finally: + await store.close() + + +async def test_a_store_fault_on_the_holder_re_read_skips_only_that_rename( + caplog: pytest.LogCaptureFixture, +) -> None: + """BACKLOG #3070. Two accounts are renamed in one pass. The first rename the pass applies is + handed a ``held`` naming another row, and the refresh's re-read of that holder raises a store + error. That rename is not applied and is logged; the pass goes on and applies the other one. + The first by-name read of either new name is the pass's ``held`` read for the first rename it + applies, so the faulting rename is always the earlier of the two.""" + store = await MessageStore.open(":memory:") + ldap = _FakeLdap({"jdoe": _principal("jdoe"), "asmith": _principal("asmith")}) + service = AuthService(store, _ad_settings(), ldap=ldap) # type: ignore[arg-type] + notifier = _CapturingNotifier() + service._security_notifier = notifier + try: + await service.initialize() + ids: dict[str, str] = {} + for name in ("jdoe", "asmith"): + await _signed_in_ad_user(service, store, name) + row = await store.get_user_by_username(name) + assert row is not None + ids[f"{name}-wed"] = row.id + ldap.present = { + f"{name}-wed": _principal(f"{name}-wed", object_id=_object_id_for(name)) + for name in ("jdoe", "asmith") + } + lookup_by_name = service._lookup_by_username + faulting: list[str] = [] + + async def _first_re_read_faults(username: str) -> _UserLookup: + if username not in ids: + return await lookup_by_name(username) + if not faulting: + faulting.append(username) + own = await service._load_user(ids[username]) + assert own is not None + return _UserLookup(replace(own, id="other-holder", username=username), True) + if username == faulting[0]: + raise sqlite3.OperationalError("database is locked") + return await lookup_by_name(username) + + service._lookup_by_username = _first_re_read_faults # type: ignore[method-assign] + caplog.set_level(logging.INFO, logger="messagefoundry.auth.service") + caplog.clear() + + plan = await service.reconcile_directory_sessions() + + assert plan.aborted is None + assert len(plan.renames) == 2 + [skipped] = faulting + [applied] = set(ids) - {skipped} + still = await store.get_user(ids[skipped]) + assert still is not None and still.username == skipped.removesuffix("-wed") + moved = await store.get_user(ids[applied]) + assert moved is not None and moved.username == applied, "the pass stopped at the fault" + [refreshed] = await _rename_audits(store) + assert refreshed["action"] == "auth.ad_username_refreshed" + assert json.loads(refreshed["detail"])["user_id"] == ids[applied] + [logged] = [r.getMessage() for r in caplog.records if "BACKLOG #3070" in r.getMessage()] + assert ids[skipped] in logged and "OperationalError" in logged + assert [e.username for e in notifier.sent if e.event_type == USERNAME_CHANGED] == [applied] + finally: + await store.close() + + async def test_a_row_gone_refusal_does_not_warn_of_a_lockout( caplog: pytest.LogCaptureFixture, ) -> None: From 0beae23bc1fe6f855dc91fae647e93cbefd458a0 Mon Sep 17 00:00:00 2001 From: Scott Hall Date: Sun, 18 Oct 2026 17:32:41 -0500 Subject: [PATCH 17/18] Review round 1 on the send-back: the fault skip is the reconciler's alone (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. --- changelog.d/3070.fixed.md | 15 ++++---- messagefoundry/auth/service.py | 21 ++++++---- tests/test_ad_session_reconcile.py | 62 ++++++++++++++++++++++-------- 3 files changed, 69 insertions(+), 29 deletions(-) diff --git a/changelog.d/3070.fixed.md b/changelog.d/3070.fixed.md index 0c8a86dc33..557b253e8a 100644 --- a/changelog.d/3070.fixed.md +++ b/changelog.d/3070.fixed.md @@ -3,10 +3,11 @@ the account itself, and refused the rename on the first read. A holder that let the name go in between still produced an `auth.ad_username_refresh_conflict` audit row and a lockout warning that nobody faced, and the rename waited for a later pass. Now the holder is read again after the - account, and the rename is refused only when another account holds the name at that second - read. The conflict row's `held_by_user_id` names that account, not the one the first read saw. - A holder whose record fails its integrity check at the second read is still refused with the - lockout warning, naming the holder the first read verified. A store error on the second read - skips that one rename for the pass instead of ending the pass. A holder that already fails its - integrity check at the first read still reads as no holder there, so that case is not reported - as a conflict. Vault BACKLOG #3070. + account, and that check refuses the rename only when another account holds the name at the + second read. The conflict row's `held_by_user_id` then names that account, not the one the first + read saw. A holder whose record fails its integrity check at the second read is still refused + with the lockout warning, naming the holder the first read verified. A store error on the second + read skips that one rename for the pass instead of ending the pass; a store error on the + pass's other rename reads still ends it. A holder that already fails its integrity check at the + first read still reads as no holder there, so that case is not reported as a conflict. Vault + BACKLOG #3070. diff --git a/messagefoundry/auth/service.py b/messagefoundry/auth/service.py index 7e762b0316..0ad4a9df67 100644 --- a/messagefoundry/auth/service.py +++ b/messagefoundry/auth/service.py @@ -2982,8 +2982,10 @@ def __init__( self._reconcile_unkeyed_refused: dict[str, None] = {} if self.directory_reconcile_enabled: # Built now, at startup, so a reconciler's first refused audit write does not import - # the server drivers on the event loop (see `_audit_write_errors`). + # the server drivers on the event loop (see `_audit_write_errors`). The same for the + # rename refresh's holder re-read, which catches `_session_write_errors` (#3070). _audit_write_errors() + _session_write_errors() # Advisory, NON-STICKY federated-IdP health (ADR 0142 AC-8) — see the oidc_available docstring. self._oidc_unavailable_reason: str | None = None self._oidc_client_auth: oidc.ClientAuthentication | None = None @@ -7896,16 +7898,21 @@ async def _refuse(held_by: str | None, detected: str, holds: str) -> None: # write-path refusal's "a later pass retries it", which is false while that holder # stays seal-failed. Its id is not verified, so the audit names ``held``, the last # verified holder. NOT CLOSED HERE: the reconciler's own ``held`` read, and at least - # the two write-path holder reads below, still read a seal-failed holder as None. + # the two write-path holder reads below, still read a seal-failed holder as None; and + # ``refused`` also reads true for a holder deleted or re-keyed between the re-read's + # own row and role reads, a narrow window that still files this refusal. # - # A store fault on this re-read skips this one rename for this pass rather than - # raising: the caller is the reconciler's rename loop, and a raise would end the pass - # before the renames after this one. Nothing was written; a later pass that probes - # the account retries it. The cached store-error tuple, so a fault never imports a - # driver on the event loop. + # On the reconciler's path (``audit_soft``) a store fault on this re-read skips this + # one rename for this pass rather than raising: a raise would end the pass before the + # renames after this one. Nothing was written; a later pass that probes the account + # retries it. The other store reads in the rename loop still raise. A sign-in re-raises, + # so it fails loud as it does elsewhere in this method. The error tuple is cached and + # built at construction when the reconciler runs, so no driver is imported here. try: current = await self._lookup_by_username(new_username) except _session_write_errors() as exc: + if not audit_soft: + raise _log.warning( "AD account %s: reading the holder of its new directory name raised %s, so its " "rename was not applied on this pass; a later pass that probes it retries it " diff --git a/tests/test_ad_session_reconcile.py b/tests/test_ad_session_reconcile.py index e1956c347b..6520bad78b 100644 --- a/tests/test_ad_session_reconcile.py +++ b/tests/test_ad_session_reconcile.py @@ -2819,17 +2819,23 @@ async def _renamed_service( store = await MessageStore.open( keyed_path, cipher=make_cipher(generate_key(), write_v2=True) ) - ldap = _FakeLdap({"jdoe": _principal("jdoe")}) - service = AuthService(store, _ad_settings(), ldap=ldap) # type: ignore[arg-type] - notifier = _CapturingNotifier() - service._security_notifier = notifier - await service.initialize() - await _signed_in_ad_user(service, store, "jdoe") - jdoe = await store.get_user_by_username("jdoe") - assert jdoe is not None - # Set explicitly, so the address assertion below cannot pass on whatever the birth chose. - await store.set_user_notify_email(jdoe.id, email="holder@example.org") - ldap.present = {"jdoe-married": _principal("jdoe-married", object_id=_object_id_for("jdoe"))} + try: + ldap = _FakeLdap({"jdoe": _principal("jdoe")}) + service = AuthService(store, _ad_settings(), ldap=ldap) # type: ignore[arg-type] + notifier = _CapturingNotifier() + service._security_notifier = notifier + await service.initialize() + await _signed_in_ad_user(service, store, "jdoe") + jdoe = await store.get_user_by_username("jdoe") + assert jdoe is not None + # Set explicitly, so the address assertion below cannot pass on whatever the birth chose. + await store.set_user_notify_email(jdoe.id, email="holder@example.org") + ldap.present = { + "jdoe-married": _principal("jdoe-married", object_id=_object_id_for("jdoe")) + } + except BaseException: + await store.close() + raise return store, ldap, service, notifier, jdoe.id @@ -3501,11 +3507,11 @@ async def test_a_store_fault_on_the_holder_re_read_skips_only_that_rename( The first by-name read of either new name is the pass's ``held`` read for the first rename it applies, so the faulting rename is always the earlier of the two.""" store = await MessageStore.open(":memory:") - ldap = _FakeLdap({"jdoe": _principal("jdoe"), "asmith": _principal("asmith")}) - service = AuthService(store, _ad_settings(), ldap=ldap) # type: ignore[arg-type] - notifier = _CapturingNotifier() - service._security_notifier = notifier try: + ldap = _FakeLdap({"jdoe": _principal("jdoe"), "asmith": _principal("asmith")}) + service = AuthService(store, _ad_settings(), ldap=ldap) # type: ignore[arg-type] + notifier = _CapturingNotifier() + service._security_notifier = notifier await service.initialize() ids: dict[str, str] = {} for name in ("jdoe", "asmith"): @@ -3556,6 +3562,32 @@ async def _first_re_read_faults(username: str) -> _UserLookup: await store.close() +async def test_a_store_fault_on_the_holder_re_read_still_raises_off_the_reconciler_path() -> None: + """BACKLOG #3070, the control for the skip above. Only the reconciler (``audit_soft``) skips a + rename on a faulting holder re-read; any other caller, such as a sign-in, gets the error.""" + store, _ldap, service, _notifier, user_id = await _renamed_service() + try: + + async def _faults(username: str) -> _UserLookup: + raise sqlite3.OperationalError("database is locked") + + row = await store.get_user(user_id) + assert row is not None + service._lookup_by_username = _faults # type: ignore[method-assign] + with pytest.raises(sqlite3.OperationalError): + await service._refresh_cached_username( + user_id=user_id, + old_username="jdoe", + new_username="jdoe-married", + held=replace(row, id="gone-holder", username="jdoe-married"), + from_plan=False, + ) + after = await store.get_user(user_id) + assert after is not None and after.username == "jdoe" + finally: + await store.close() + + async def test_a_row_gone_refusal_does_not_warn_of_a_lockout( caplog: pytest.LogCaptureFixture, ) -> None: From 60339cf1d57dc3eda983d90fcdb734f6a0db9cc9 Mon Sep 17 00:00:00 2001 From: Scott Hall Date: Sun, 18 Oct 2026 17:41:29 -0500 Subject: [PATCH 18/18] Review round 2 on the send-back: skip line names the holder, prose matches (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. --- changelog.d/3070.fixed.md | 4 ++-- messagefoundry/auth/service.py | 10 ++++++---- tests/test_ad_session_reconcile.py | 1 + 3 files changed, 9 insertions(+), 6 deletions(-) diff --git a/changelog.d/3070.fixed.md b/changelog.d/3070.fixed.md index 557b253e8a..4365cf5c4e 100644 --- a/changelog.d/3070.fixed.md +++ b/changelog.d/3070.fixed.md @@ -9,5 +9,5 @@ with the lockout warning, naming the holder the first read verified. A store error on the second read skips that one rename for the pass instead of ending the pass; a store error on the pass's other rename reads still ends it. A holder that already fails its integrity check at the - first read still reads as no holder there, so that case is not reported as a conflict. Vault - BACKLOG #3070. + first read still reads as no holder there, so the write's guard stops the rename and the + conflict row names no holder, with a warning that a later pass retries it. Vault BACKLOG #3070. diff --git a/messagefoundry/auth/service.py b/messagefoundry/auth/service.py index 0ad4a9df67..f672195c9b 100644 --- a/messagefoundry/auth/service.py +++ b/messagefoundry/auth/service.py @@ -7672,7 +7672,8 @@ async def _refresh_cached_username( ``audit_soft`` is passed by the reconciler alone (BACKLOG #3018), so its audit writes go through :meth:`_audit_reconciler_row`. The login path leaves it off, so there a refused - write still fails the sign-in, as it always has. + write still fails the sign-in, as it always has. It also lets a store fault on the ``held`` + re-read skip the rename rather than raise (BACKLOG #3070; the reason is at that read). **ONE implementation for two callers, deliberately.** The login path (:meth:`_upsert_ad_user`) and the ADR 0079 reconciler pass both reach a renamed account and @@ -7914,10 +7915,11 @@ async def _refuse(held_by: str | None, detected: str, holds: str) -> None: if not audit_soft: raise _log.warning( - "AD account %s: reading the holder of its new directory name raised %s, so its " - "rename was not applied on this pass; a later pass that probes it retries it " - "(BACKLOG #3070)", + "AD account %s: re-reading the holder of its new directory name (account %s " + "held it at the first read) raised %s, so its rename was not applied on this " + "pass; a later pass that probes it retries it (BACKLOG #3070)", user_id, + held.id, type(exc).__name__, ) return diff --git a/tests/test_ad_session_reconcile.py b/tests/test_ad_session_reconcile.py index 6520bad78b..cab883e29f 100644 --- a/tests/test_ad_session_reconcile.py +++ b/tests/test_ad_session_reconcile.py @@ -3557,6 +3557,7 @@ async def _first_re_read_faults(username: str) -> _UserLookup: assert json.loads(refreshed["detail"])["user_id"] == ids[applied] [logged] = [r.getMessage() for r in caplog.records if "BACKLOG #3070" in r.getMessage()] assert ids[skipped] in logged and "OperationalError" in logged + assert "other-holder" in logged, "the skip line dropped the holder the first read saw" assert [e.username for e in notifier.sent if e.event_type == USERNAME_CHANGED] == [applied] finally: await store.close()