fix(auth): refuse directory login on a disabled mirror row, and reset the failure counter only at full authentication (BACKLOG #1637, #1638)

[wshallwshall]

What this builds

Two rows as one pull request, because both add a refusal branch to the entry region of
_complete_ad_login and as separate PRs they would conflict in one large file.

BACKLOG #1637 -- an engine-disabled mirror row completed a directory login

_complete_ad_login checked provider and directory id and never disabled. An engine-disabled AD
mirror row therefore completed a Kerberos or OIDC login with an auth.login_success audit row and a
live session. Only identity_for_token refused it later.

Two guards, and the second is the one that closes it. A check on existing alone does not:
on a directory-side rename the caller's name-keyed read misses, existing is None, and the row the
login actually lands on comes from get_user_by_directory_object_id inside _upsert_ad_user.

• The guard that matters sits immediately after that id-keyed lookup, before any write. The
  resolver signals with a private _DirectoryLoginRefused that its one caller catches and audits.
  A check on the returned UserRecord would be too late: the rename refresh, the profile write and
  the role resync would already have run.
• The caller-side arm is kept, and its comment says plainly that it is for the reader, not for the
  control. The resolver's gate runs on by_name too, so deleting those four lines changes no
  outcome. Documenting it as load-bearing would have been the false-premise shape SDS-3.7 names.

Ordered after the two existing conflict branches: a recycled sAMAccountName meeting a stale
disabled row is a directory_identity_conflict, not a disabled login. The new holder's account is
neither disabled nor locked, and saying otherwise would misdirect the operator reading the audit row.

BACKLOG #1638 -- the failure counter cleared at the password step

record_login_success zeroes failed_attempts and NULLs locked_until in one UPDATE, and both
_login_local and _complete_ad_login called it before the second factor was proven. So a
holder of the first factor could shed a run of wrong codes by logging in again, and a directory
re-login cleared a second-factor lock outright.

The call now runs only when the session owes no further factor. verify_mfa already cleared on its
own success; finish_webauthn_assertion now does too. _complete_ad_login also refuses a locked
mirror row, through the same gate as the disabled check.

No fourth store method. record_login_success writes last_login_at in the same UPDATE, so
deferring the call moves that column's timing. Nothing reads it, ADR 0164 exists to reject deriving
anything from it, and a new store method would drag in four backends plus test_store_backend.py
for no gain. prior_failures is left where it is -- two tests pin the suspicious-login notice.

Decisions this brief asked me to record

The passkey clear, and why it is not an ADR 0068 reversal

On a successful finish_webauthn_assertion the failure counter is now cleared.

ADR 0068's recorded divergence is about not FEEDING _register_failure on assertion FAILURE, and
it stands unchanged (signatures are not guessable secrets; a flaky authenticator must not lock an
account). It says nothing about success. Since #1638 the password step no longer clears the counter,
so whichever leg completes the authentication has to -- and for a passkey-only account this leg is
the only one there is. Without it, that account's password failures would shed only by waiting out
the lockout window. Read this as closing a gap #1638 would otherwise open, not as a posture change.

_REASON_TO_CODE: disabled and locked are deliberately unmapped

Both collapse to the generic oidc_failed on the login page. A distinct code would confirm to an
unauthenticated caller that the named account exists and say which of the two states it is in.
The operator loses nothing: the precise reason is on the auth.login_failed audit row.

The omission is explained at the map and pinned by a test
(test_the_browser_layer_gives_a_refused_account_no_distinguishing_code), because to the next reader
of that map an absent entry looks exactly like an oversight.

Checked while there: neither messagefoundry_webconsole/routes/sso.py nor
messagefoundry/api/auth_routes.py reads outcome.reason at all, so the OIDC callback is the only
consumer and there is no second leak path.

Audit shape

Both refusals use the inline shape the existing local_account_conflict refusal uses, not
_directory_reject_audit. That helper requires a mechanism slug and the Kerberos leg reaches this
decision point without one (mech is optional and defaults to None). The reason is a closed-set
literal either way, so no directory- or IdP-supplied text is stored.

docs/SECURITY.md -- one sentence, per the Manager ruling

"The engine's per-account lockout protects Local accounts only" was already false before this
change (verify_mfa and finish_webauthn_assertion never filtered on auth_provider), and the
#1637/#1638 guards make it false in a new way. Shipping a control whose own security document says it
does not apply there is the compensating-control-on-a-false-premise defect SDS-3.7 forbids.

Scope: that one sentence. The wider packet-18 rewrite is untouched. Two follow-on notes for
whoever owns packet 18, neither fixed here:

1. The 6.1.1 pathway table cells for AD and OIDC say "no engine per-account lockout" / "no
   engine credential to lock out". Both describe feeding, and both remain accurate as written --
   but a reader may now want an enforcement note beside them.
2. The corrected sentence's own tail ("the lock is enforced across every factor leg even though only
   two legs feed it") now sits just after a broader claim and reads as a weaker restatement. Left
   alone to hold the one-sentence scope.

BACKLOG #1569 is subsumed by #1638

Same counter, same call, same line. Not built separately and not claimed. The Lander can close
#1569 citing #1638.

Proposed ledger banner text

docs/BACKLOG.md is a 23-line public stub in this repository (commit 11a3934c1); the real ledger
is vault-only, so no ledger file is touched by this PR. Text for the Lander:

#1637 -- Shipped <date> by PR <N>. _complete_ad_login now refuses an engine-disabled mirror
row before any write, on both the name-keyed and the id-keyed (rename) path, audited login_failed
with reason disabled. Kerberos and OIDC legs each covered by a test.

#1638 -- Shipped <date> by PR <N>. record_login_success runs only once the session owes no
further factor, on the local and the directory path; finish_webauthn_assertion clears on success;
_complete_ad_login refuses a locked mirror row. Subsumes #1569, which may be closed citing this
row.

Checks

Run in this worktree's own .venv, all green:

• ruff format (reformatted 2 files, now clean) and ruff check . -- All checks passed
• mypy messagefoundry -- strict, no issues in 274 source files
• pytest: test_asvs_login_deadline.py, test_auth_oidc_service.py,
  test_ad_directory_identity.py, test_auth_service.py, test_docs_security_pathways.py,
  test_auth_session_lifecycle.py, test_mfa_access_gate.py, test_webauthn.py -- 228 passed
• pytest tests/test_mfa.py plus packaging/messagefoundry-webconsole/tests -- run separately
  (the MFA suite is argon2-bound and takes about 3 minutes); result recorded in a PR comment if it
  landed after this body was written
• /simplify pass: it caught one real thing. My first draft's comment claimed the caller-side guard
  was load-bearing. It is not, and the comment now says so.
• Glyph scan over all 671 added lines: zero, with a positive control (180 hits in
  docs/FEATURE-MAP.md) proving the instrument works. The first attempt used grep -P and died on
  character value in \x{} too large -- an instrument failure, not a clean result (SDS-3.8).

Not run: the full suite. The local interpreter lacks the vault extra, so pytest prints
INCOMPLETE RUN -- coverage was NOT collected and every vault-gated module removed itself at
collection time. Nothing here touches vault code, but that is a reason, not a measurement.

Legs that must be read after this process exits (a Builder never sees them): the hosted-runner
legs, including windows-service-smoke (NSSM), the full-matrix pytest leg that installs the vault
extra, and the coverage gate.

What contradicts the brief

Three things, all minor, all in my favour to report rather than quietly absorb:

1. The brief warned that rows 1637 and 1638 "retract themselves further down" and that a reader who
   stops at the banner gets it backwards. They do not. Both bodies are short, and each agrees with
   its own banner and heading throughout. I also checked for the 2026-09-15 body-slide: no slide at
   1637 or 1638 -- each body matches its heading.
2. tests/test_mfa.py needed a test the brief did not anticipate. My first draft of the
   must-not-fire arm used a live TOTP for the passing factor and flaked: confirm_mfa_enrollment
   spends the current TOTP step (single-use, ASVS 6.5.1), and the wrong codes in between fall through
   to the argon2id recovery path and cost real seconds, so waiting for a later step is a race. It now
   uses a recovery code, which has no step to consume. The docstring records why.
3. Refusing a disabled row at login does not revoke that row's existing sessions, and I did not
   add that. It is out of scope for both rows' "What closing looks like", and identity_for_token
   plus the ADR 0079 reconciler already cover it. My tests assert the refusal mints no new
   session rather than that the row has none -- the earlier, successful login's session is still
   there by design.

Question this brief left open

None that blocked the build. One worth a second opinion: I placed the eligibility gate after the
directory_identity_conflict branch so a recycled name meeting a stale disabled row keeps the
conflict reason. The opposite order is defensible if you would rather a disabled row be refused as
early as possible regardless of who is presenting. I chose audit accuracy; the comment at the site
states the choice so it can be reversed knowingly.

[wshallwshall]

The deferred check leg finished green after the body was written, as that section said it would.

pytest tests/test_mfa.py packaging/messagefoundry-webconsole/tests -- 521 passed, 3 skipped in 3m57s, exit 0.

That closes every local check named in the body. The hosted-runner legs (windows-service-smoke, the full-matrix pytest leg that installs the vault extra, and the coverage gate) still have to be read by whoever picks this up -- a Builder never sees them.

[wshallwshall]

Lander review

Read at head 87ed174cc: 28 success, 14 skipped, zero failures, zero pending, all on the current head.

The one thing that could have made this worse than the defect

This PR stops clearing failed_attempts at the password step. If no completion path cleared it afterwards, it would trade a lockout bypass for a permanent lockout — strictly worse, and invisible to a test suite that only exercises the bypass.

It does not. I enumerated every record_login_success call site on the branch and resolved each to its enclosing method:

line 1298  ->  _login_local                 (the not-mfa_required branch)
line 1944  ->  _complete_ad_login
line 3423  ->  verify_mfa
line 3908  ->  finish_webauthn_assertion

Both MFA-completion legs clear it, unconditionally on their success paths. So every account that can fully authenticate has a path that clears the counter, and the accounts that do not are exactly the ones that have not completed authentication. The comment at 1298 is accurate as written.

The finish_webauthn_assertion comment is the one I would keep if anything were cut: it reasons explicitly about why clearing on success is not a reversal of the ADR 0068 divergence, which governs not feeding _register_failure on failure. Those are easy to conflate and the code does not.

The SECURITY.md enforcement claim, checked rather than read

The doc now says the lock is enforced wherever a pathway reaches an engine account row, directory accounts included, because verify_mfa and finish_webauthn_assertion never filtered on auth_provider. Measured on the branch:

verify_mfa                  auth_provider mentions = 0   locked_until mentions = 1
finish_webauthn_assertion   auth_provider mentions = 0   locked_until mentions = 1

Positive control in the same pass: _complete_ad_login shows 2 auth_provider mentions, so the instrument can find the token and the zeros are real absences rather than a broken search.

The no-enumeration property, which is the part most likely to be "fixed" later

_refuse_directory_row returns the generic invalid credentials regardless of which refusal fired, and _REASON_TO_CODE in messagefoundry_webconsole/routes/oidc.py deliberately omits disabled and locked so both collapse to oidc_failed. The comment says "do not complete this map" and gives the reason. That is the right shape: a distinct code would confirm to an unauthenticated caller that the account exists and which state it is in, while the operator still gets the precise slug on the auth.login_failed row.

The refusal slugs are closed-set literals, never directory- or IdP-supplied text, which keeps attacker-influenced strings off the audit row.

The honest guard, and why it earns its place

The existing is not None branch in _complete_ad_login carries a comment saying it is for the reader, not for the control — delete it and no outcome changes, because the gate inside _upsert_ad_user already covers every row it covers. Naming that rather than letting it read as load-bearing is exactly SDS-3.7, and the comment cites it. The arm that actually closes #1637 is the id-keyed one in the resolver, because the name-keyed read misses on a directory-side rename.

The ordering after the two conflict branches is also reasoned: a recycled sAMAccountName meeting a stale disabled row is a directory_identity_conflict, not a disabled login, and reporting otherwise would misdirect the operator reading the audit.

Stated no larger than it is

I verified the control flow and the two documentary claims above. I did not re-run the suite locally and I did not independently re-derive the ADR 0068 divergence this builds on; I checked that this change does not contradict it, not that it is itself correct.

Verdict: merge.