fix(auth): MFA enrollment consume-step, with the MFA test suite restructured (BACKLOG #1021)

docs/BACKLOG.md

@@ -4733,7 +4733,7 @@ Retiring the tree costs the engine nothing operationally: **`tests/test_ech_egre
 
 ## 1021. The MFA enrollment confirm verifies the activating TOTP through a bool wrapper that discards the step, so it is never consumed (ASVS 6.5.1)
 
-> 🔢 **Filed 2026-08-04 — not started.** Value **6/10** · Difficulty **4/10** · _quick win_. `confirm_mfa_enrollment` verifies the enrolling code with `totp.verify_totp`, a documented thin bool wrapper that computes the matched time-step and then collapses it to a bool, so the step cannot be recorded. `enable_totp` leaves `last_totp_step` NULL, and `consume_totp_step` rejects only when `last is not None and last >= step` — so on first deployment the activating code would remain usable on the login path for the remainder of its own step.
+> ✅ **Fixed 2026-08-06 — enrollment now consumes the activating TOTP step (`verify_totp_step` + `consume_totp_step`), mirroring the login path.** Value **6/10** · Difficulty **4/10** · _quick win_. `confirm_mfa_enrollment` proved the enrolling code through the `totp.verify_totp` bool wrapper, which computed the matched time-step then collapsed it to a bool, so the step was never recorded; with `last_totp_step` left NULL by `enable_totp`, the activating code would have remained usable on the login path for the remainder of its own step on first deployment. The confirm site now takes the matched step from `verify_totp_step` and requires `consume_totp_step` before minting recovery codes / `enable_totp`, so the step is single-use (ASVS 6.5.1) and enable stays atomic.
 
 **Cluster:** Security / authentication. **Priority:** P2. **Verdict:** build (small). **Severity:** would leave a narrow second-factor replay window at enrollment on first deployment — bounded, not a bypass.
 

messagefoundry/auth/service.py

@@ -2004,7 +2004,8 @@ async def confirm_mfa_enrollment(
         """Confirm a staged enrollment by proving a live TOTP code. On success: activate MFA, mint the
         single-use recovery codes (returned **once**, plaintext, for the user to save), mark the
         current session MFA-verified, audit + notify. Returns the recovery codes, or ``None`` when the
-        code was wrong. Raises :class:`ValueError` if no enrollment is staged / the user isn't local."""
+        code was wrong or its time-step was already consumed (single-use, BACKLOG #1021). Raises
+        :class:`ValueError` if no enrollment is staged / the user isn't local."""
         user = await self._store.get_user(identity.user_id)
         if user is None or user.auth_provider != AuthProvider.LOCAL.value:
             raise ValueError("only local users can enroll a TOTP authenticator")
@@ -2014,8 +2015,19 @@ async def confirm_mfa_enrollment(
         # Verify the enrollment proof under the SAME configured clock-skew window as a login (BACKLOG
         # #187): default 0 = strict current-step only. Enrolling under the same window a login uses
         # avoids the trap of a skewed-clock authenticator that confirms enrollment yet then fails every
-        # login (the mismatch surfaces at enroll time instead).
-        if not totp.verify_totp(secret, code.strip(), window=self._settings.totp_skew_steps):
+        # login (the mismatch surfaces at enroll time instead). The matched step is then CONSUMED
+        # (BACKLOG #1021), single-use per ASVS 6.5.1, mirroring _verify_second_factor's verify-then-
+        # consume so the activating code can't be replayed on POST /auth/mfa-verify inside its step
+        # window (verify_totp alone discarded the step, which would leave a second-factor replay window
+        # at enrollment on first deployment). Consume BEFORE minting recovery codes / enable_totp keeps
+        # enable atomic: a step that no longer advances the high-water mark fails on the same
+        # phase=enroll branch and MFA is not enabled.
+        matched_step = totp.verify_totp_step(
+            secret, code.strip(), window=self._settings.totp_skew_steps
+        )
+        if matched_step is None or not await self._store.consume_totp_step(
+            identity.user_id, matched_step
+        ):
             await self._audit(
                 "auth.mfa_failed",
                 actor=identity.username,

packaging/messagefoundry-webconsole/tests/test_ui_mfa_gate.py

@@ -13,6 +13,7 @@
 from __future__ import annotations
 
 import httpx
+import pytest
 
 from messagefoundry.api import create_app
 from messagefoundry.auth import Role, totp
@@ -23,6 +24,29 @@
 PW = "a-strong-test-passphrase"  # >=15, no app/vendor terms — satisfies the ASVS policy (WP-3)
 
 
+class _PinnedClock:
+    """Minimal ``time`` stand-in exposing only ``time()`` at a fixed instant.
+
+    A local twin of ``tests/_totp_clock._PinnedClock`` — this package has its own test root and does
+    not import the engine suite's helpers. ``totp`` reads the wall clock solely as ``time.time()``, so
+    swapping the module reference pins the TOTP step deterministically while leaving every other clock
+    real.
+    """
+
+    def __init__(self, instant: float) -> None:
+        self._instant = instant
+
+    def time(self) -> float:
+        return self._instant
+
+
+def _pin_totp_clock(monkeypatch: pytest.MonkeyPatch, instant: float) -> None:
+    """Pin the ``totp`` module clock so an enroll ceremony and a later /ui/mfa gate verify land in
+    distinct, provably-adjacent steps — needed now that enrollment consumes the activating step
+    (BACKLOG #1021), so a gate code from the SAME step would be refused as a replay."""
+    monkeypatch.setattr(totp, "time", _PinnedClock(instant))
+
+
 async def _service(engine: Engine, **kw: object) -> AuthService:
     service = AuthService(engine.store, AuthSettings(login_rate_limit_enabled=False, **kw))  # type: ignore[arg-type]
     await service.initialize()
@@ -185,30 +209,46 @@ async def test_the_page_asks_for_a_code_once_a_factor_exists(engine: Engine) ->
         assert 'name="password"' not in r.text
 
 
-async def test_a_satisfied_session_is_bounced_off_the_page(engine: Engine) -> None:
+async def test_a_satisfied_session_is_bounced_off_the_page(
+    engine: Engine, monkeypatch: pytest.MonkeyPatch
+) -> None:
     """RED when: the mfa_satisfied early-return is removed from GET /ui/mfa.
 
     Without it a verified operator who navigates back to /ui/mfa is asked to re-verify forever.
     """
     service = await _service(engine)
     await _add(service, "op", Role.OPERATOR)
     async with _client(engine, service) as c:
+        # Enroll consumes the activating TOTP step (BACKLOG #1021), so the gate code must sit in a
+        # strictly later step: pin the enroll ceremony to t0 and the gate verify to t0+period.
+        t0 = 1_000_000.0
+        _pin_totp_clock(monkeypatch, t0)
         secret = await _enroll_totp(service)
         await _login(c)
-        assert (await c.post("/ui/mfa", data={"code": totp.totp(secret)})).status_code == 303
+        t1 = t0 + totp.DEFAULT_PERIOD
+        _pin_totp_clock(monkeypatch, t1)
+        gate = await c.post("/ui/mfa", data={"code": totp.totp(secret, now=t1)})
+        assert gate.status_code == 303
         r = await c.get("/ui/mfa")
         assert r.status_code == 303 and r.headers["location"] == "/ui"
 
 
-async def test_a_valid_code_clears_the_gate(engine: Engine) -> None:
+async def test_a_valid_code_clears_the_gate(
+    engine: Engine, monkeypatch: pytest.MonkeyPatch
+) -> None:
     """RED when: POST /ui/mfa stops calling verify_mfa (or stops redirecting on success)."""
     service = await _service(engine)
     await _add(service, "op", Role.OPERATOR)
     async with _client(engine, service) as c:
+        # Enroll consumes the activating step (BACKLOG #1021); the gate code lives in a later step.
+        t0 = 1_000_000.0
+        _pin_totp_clock(monkeypatch, t0)
         secret = await _enroll_totp(service)
         await _login(c)
         assert (await c.get("/ui/messages")).status_code == 303  # confined
-        r = await c.post("/ui/mfa", data={"code": totp.totp(secret)})
+        t1 = t0 + totp.DEFAULT_PERIOD
+        _pin_totp_clock(monkeypatch, t1)
+        r = await c.post("/ui/mfa", data={"code": totp.totp(secret, now=t1)})
         assert r.status_code == 303 and r.headers["location"] == "/ui"
         assert (await c.get("/ui/messages")).status_code == 200  # released
 

tests/_totp_clock.py

@@ -31,9 +31,11 @@
 
 import time
 
+import pytest
+
 from messagefoundry.auth import totp
 
-__all__ = ["fresh_totp", "step_remaining"]
+__all__ = ["fresh_totp", "pin_totp_clock", "step_remaining"]
 
 #: Seconds that must remain in the current step before a code is generated. Comfortably larger than
 #: any plausible generate → verify gap (a store read plus an HMAC; the argon2id recovery-code path
@@ -65,3 +67,33 @@ def fresh_totp(
         # keeps a coarse clock from returning the step we just left.
         time.sleep(remaining + 0.05)
     return totp.totp(secret, period=period)
+
+
+class _PinnedClock:
+    """A minimal stand-in for the ``time`` module exposing only ``time()`` at a fixed instant.
+
+    ``totp`` reads the wall clock solely as ``time.time()`` (two call sites: :func:`totp.totp` and
+    :func:`totp.verify_totp_step`), so swapping the module reference for this pins the TOTP step
+    deterministically while leaving every OTHER clock — the service's session-expiry, rate-limiting
+    and audit timestamps, all on their own ``time`` imports — real. That surgical scope is what lets
+    a test place enrollment and a later verify in DISTINCT, provably-adjacent steps without the 30 s
+    boundary flake :func:`fresh_totp` can only narrow.
+    """
+
+    def __init__(self, instant: float) -> None:
+        self._instant = instant
+
+    def time(self) -> float:
+        return self._instant
+
+
+def pin_totp_clock(monkeypatch: pytest.MonkeyPatch, instant: float) -> None:
+    """Pin the ``totp`` module's clock to ``instant`` for the rest of the test (monkeypatch restores
+    the real clock at teardown).
+
+    Generate codes with ``totp.totp(secret, now=instant)`` so they land in the same step the pinned
+    service-side verify computes. Unlike :func:`fresh_totp` — which guarantees headroom WITHIN a step
+    but cannot advance one — this can place two verifies in strictly different steps, which is needed
+    now that enrollment consumes the activating step (BACKLOG #1021): a later login code must live in
+    a higher step than the consumed enrollment step to be accepted."""
+    monkeypatch.setattr(totp, "time", _PinnedClock(instant))

tests/test_admin_new_ip.py

@@ -17,10 +17,10 @@
 
 import httpx
 import pytest
-from _totp_clock import fresh_totp
+from _totp_clock import pin_totp_clock
 
 from messagefoundry.api import create_app
-from messagefoundry.auth import Role
+from messagefoundry.auth import Role, totp
 from messagefoundry.auth.identity import Identity
 from messagefoundry.auth.notifications import ADMIN_NEW_IP, SecurityEvent
 from messagefoundry.auth.service import AuthService
@@ -196,7 +196,9 @@ async def test_loopback_addresses_treated_as_same_host() -> None:
         await store.close()
 
 
-async def test_verify_mfa_reanchors_session_to_the_new_ip() -> None:
+async def test_verify_mfa_reanchors_session_to_the_new_ip(
+    monkeypatch: pytest.MonkeyPatch,
+) -> None:
     """Completing the second factor (TOTP) from a new address re-anchors the session, like reauth — so
     an MFA-required admin who roamed clears the new-IP signal with one credential proof, not two."""
     store = await MessageStore.open(":memory:")
@@ -208,13 +210,21 @@ async def test_verify_mfa_reanchors_session_to_the_new_ip() -> None:
         assert out.ok and out.identity is not None and out.token is not None
         identity, token = out.identity, out.token
         enroll = await service.begin_mfa_enrollment(identity)
+        # Pin the TOTP clock so the enrollment confirm and the later verify_mfa sit in distinct steps
+        # (enrollment now consumes the activating step, BACKLOG #1021).
+        t0 = 1_000_000.0
+        pin_totp_clock(monkeypatch, t0)
         await service.confirm_mfa_enrollment(
-            identity, fresh_totp(enroll.secret), token=token, client="10.1.1.1"
+            identity, totp.totp(enroll.secret, now=t0), token=token, client="10.1.1.1"
         )
         # Roam to a new address → flagged.
         assert await service.flag_new_client_ip(token, "10.2.2.2", path="/users") is True
-        # Completing MFA from the new address re-anchors the session (parity with reauth).
-        assert await service.verify_mfa(token, fresh_totp(enroll.secret), client="10.2.2.2") is True
+        # Completing MFA from the new address re-anchors the session (parity with reauth), using a code
+        # in a strictly later step than enrollment consumed.
+        t1 = t0 + totp.DEFAULT_PERIOD
+        pin_totp_clock(monkeypatch, t1)
+        code = totp.totp(enroll.secret, now=t1)
+        assert await service.verify_mfa(token, code, client="10.2.2.2") is True
         assert await service.flag_new_client_ip(token, "10.2.2.2", path="/users") is False
     finally:
         await store.close()

tests/test_api_auth.py

@@ -9,10 +9,10 @@
 
 import httpx
 import pytest
-from _totp_clock import fresh_totp
+from _totp_clock import fresh_totp, pin_totp_clock
 
 from messagefoundry.api import create_app
-from messagefoundry.auth import Role
+from messagefoundry.auth import Role, totp
 from messagefoundry.auth.ldap import AdPrincipal
 from messagefoundry.auth.service import AuthService
 from messagefoundry.auth.tokens import hash_token
@@ -145,7 +145,9 @@ async def test_security_events_feed_is_scoped_to_caller(engine: Engine) -> None:
     assert "auth.login_failed" in bob_actions  # bob sees his own failure
 
 
-async def test_mfa_enroll_confirm_and_step_up_gate(engine: Engine) -> None:
+async def test_mfa_enroll_confirm_and_step_up_gate(
+    engine: Engine, monkeypatch: pytest.MonkeyPatch
+) -> None:
     # WP-14 / ASVS 6.3.3: the full TOTP lifecycle over the API + the step-up MFA gate. An MFA-required
     # session 403s on a require_step_up route with X-MFA-Required until POST /auth/mfa-verify.
     service = await _service(engine, AuthSettings(login_rate_limit_enabled=False))
@@ -160,9 +162,16 @@ async def test_mfa_enroll_confirm_and_step_up_gate(engine: Engine) -> None:
         assert r.status_code == 200
         secret = r.json()["secret"]
 
-        # Confirm with a live code → activates MFA + returns the one-time recovery codes.
+        # Confirm with a live code → activates MFA + returns the one-time recovery codes. Pin the TOTP
+        # clock so the activating confirm code and the later /auth/mfa-verify code sit in distinct steps
+        # (enrollment now consumes the activating step, BACKLOG #1021). The in-process ASGI server
+        # shares this totp module, so the pin covers its server-side verify too.
         assert (await _reauth(c, tok, purpose="mfa_confirm")).status_code == 200
-        r = await c.post("/me/mfa/confirm", json={"code": fresh_totp(secret)}, headers=_auth(tok))
+        t0 = 1_000_000.0
+        pin_totp_clock(monkeypatch, t0)
+        r = await c.post(
+            "/me/mfa/confirm", json={"code": totp.totp(secret, now=t0)}, headers=_auth(tok)
+        )
         assert r.status_code == 200 and len(r.json()["recovery_codes"]) == 10
         st = (await c.get("/me/mfa", headers=_auth(tok))).json()
         assert st["enabled"] is True and st["required"] is True
@@ -175,7 +184,12 @@ async def test_mfa_enroll_confirm_and_step_up_gate(engine: Engine) -> None:
         # A require_step_up route is blocked with X-MFA-Required until the 2nd factor is verified.
         r = await c.put("/ad-group-map", json={"entries": []}, headers=_auth(tok2))
         assert r.status_code == 403 and r.headers.get("X-MFA-Required") == "1"
-        r = await c.post("/auth/mfa-verify", json={"code": fresh_totp(secret)}, headers=_auth(tok2))
+        # Verify from a strictly later step than enrollment consumed.
+        t1 = t0 + totp.DEFAULT_PERIOD
+        pin_totp_clock(monkeypatch, t1)
+        r = await c.post(
+            "/auth/mfa-verify", json={"code": totp.totp(secret, now=t1)}, headers=_auth(tok2)
+        )
         assert r.status_code == 200
         # Now it passes (password step-up satisfied at login; MFA now satisfied).
         r = await c.put("/ad-group-map", json={"entries": []}, headers=_auth(tok2))