[cristim]

Part 2 of #442 (part 1, TOTP replay, merged as #596). Wrong codes on the MFA lifecycle methods now feed the same durable lockout as Login.

Changes

• MFAEnable, MFADisable (TOTP and recovery code) and MFARegenerateRecoveryCodes call recordFailedLogin on a wrong code, so they share Login's counter, MaxFailedLoginAttempts and AccountLockoutDuration. The count is the existing atomic SQL update.
• A replayed code (part 1's claim returns false) counts as a failure, as it does at Login.
• A locked account is refused before any code or password is checked.

Decisions for review

• Locked-account error: ErrMFAAuthFailed (opaque 401), the existing sentinel. Login's equivalent is also an opaque 401, so no new API contract.
• Counter reset: a correct code on these endpoints does not reset the counter. Login resets only on a completed login (RecordSuccessfulLogin, which also bumps last_login_at), so the same holds here.
• Empty code is not counted (Login returns mfa_required without counting).
• Wrong password on MFADisable is not counted (the issue is about codes).
• Sessions of a locked account are not revoked: ValidateSession does not look at the lock today, and revoking is a separate behavior change.
• MFASetup is unchanged (password only, no code).
• DB or claim errors are not counted, as in Login.
• Decision (a): a stolen-session holder can lock the victim's login for 15 minutes. The counter is shared with Login and only a completed login resets it, so one wrong code after expiry re-locks, and sessions are not revoked on lock. Accepted: not a new capability class (unauthenticated wrong-password Login already locks the same counter), gated by requireMFASession's per-IP and per-user 10/15min limits. Owner options: revoke other sessions when the lock trips, or stop counting Regenerate/Enable failures for old sessions.
• Decision (b): a locked account answers 401 "authentication failed" while a wrong code answers 400 "invalid MFA code", so the status flip tells a session holder the lock tripped. A correct code while locked is refused too, so it is not a guessing oracle. The frontend shows it as an inline message (no forced logout). Owner decision whether to map locked to 429 or a clearer message.

Not in this PR

• Per-user key on login's TOTP/recovery leg (rate_limiter.go, handler_auth.go): Login already counts MFA failures toward the durable lockout, so rotating IPs is bounded; a per-user rate-limit key is a separate change.
• Check the session before the per-IP mfa bucket in requireMFASession (middleware.go ~578).

Tests

• service_mfa_lockout_db_test.go (integration, real Postgres): Login baseline locks at N; Disable, Regenerate and Enable each lock at N wrong codes and then refuse a correct code with no state change; correct code below the threshold succeeds and keeps the count; replay counts; empty code does not; N concurrent failures count exactly N.
• server/mfa_lockout_dispatch_test.go: through HandleRequest on /api/auth/mfa/disable, N wrong codes lock, the next request is refused with 401 and touches neither TOTP claim nor disable.
• Unit tests for locked refusal on all three methods; existing wrong-code tests now expect RecordFailedLogin.
• Before the fix, the integration lifecycle subtests and the HTTP test fail (account never locks) while the Login baseline passes; after, all pass.

Refs #442

Summary by CodeRabbit

• Bug Fixes
  • Failed or replayed MFA codes for enrollment, disabling MFA, and regenerating recovery codes now count toward the account’s failed-login lockout threshold. Empty codes do not count.
  • MFA changes and recovery-code regeneration are refused while an account is locked, even when the submitted code is valid.

(c) The per-user key on Login's TOTP leg and the session-before-IP-bucket ordering stay open on #442.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: Repository: LeanerCloud/cloud-commitments-platform/.coderabbit.yaml
• Review profile: CHILL
• Plan: Essentials
• Run ID: b8a3c3b3-b6c2-43fc-a989-f05f4551f567

📥 Commits

Reviewing files that changed from the base of the PR and between eec6185 and 3dbce55.

📒 Files selected for processing (4)

• internal/auth/service_mfa.go
• internal/auth/service_mfa_lockout_db_test.go
• internal/auth/service_mfa_test.go
• internal/server/mfa_lockout_dispatch_test.go

Included review availability: This review used your included allowance. 0 included reviews remain after this review. Your included PR review attempts over the past 7 days set your current allowance at 1 review per hour.

---

📝 Walkthrough

Walkthrough

MFA enable, disable, and recovery-code regeneration now reject locked accounts. Non-empty invalid or replayed codes record failed logins; empty codes do not. Tests cover shared lockout thresholds, state preservation while locked, failure-count behavior, concurrent failures, and HTTP responses.

Changes

MFA lifecycle lockout

Layer / File(s)	Summary
MFA lifecycle enforcement internal/auth/service_mfa.go, internal/auth/service_mfa_test.go	The lifecycle operations reject locked accounts. Non-empty invalid and replayed proofs record failed logins. Tests cover lock rejection and ensure empty codes do not count as failures.
Lockout thresholds and handler responses internal/auth/service_mfa_lockout_db_test.go, internal/server/mfa_lockout_dispatch_test.go	Integration tests check the shared failure threshold, failure counts for valid, invalid, replayed, empty, and concurrent codes, and HTTP responses after MFA-disable lockout.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to 3dbce

No actionable merge-blocking issue was established; the change is ready for normal merge checks.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 35.71% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 14 functions across 4 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main change: wrong MFA codes now lock accounts during enable, disable, and recovery-code regeneration.

• Fix all pre-merge checks with AI

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

---

_{Comment @coderabbitai help to get the list of available commands.}