admin/_docs/03_implementation/reviews/batch_06_cycle2_review.md

Code Review Report

Batch: 6 (cycle 2, batch 6 of 6) Tasks: AZ-556 (unify_login_error_codes), AZ-557 (mfa_brute_force_lockout) Date: 2026-05-14 Verdict: PASS_WITH_WARNINGS

Findings

#	Severity	Category	File:Line	Title
1	Medium	Spec-Gap	e2e/Azaion.E2E/Tests/PasswordHashingTests.cs	AZ-556 AC-5 — no dedicated paired-latency timing test
2	Medium	Spec-Gap	e2e/Azaion.E2E/Tests/MfaLoginTests.cs	AZ-557 AC-3 — CountRecentFailedLogins 2+3 mix covered only behaviourally
3	Low	Spec-Gap	e2e/Azaion.E2E/Tests/MfaLoginTests.cs	AZ-557 AC-4 — /login/mfa per-IP burst test deliberately omitted (matches AZ-537 stub)
4	Low	Maintainability	Azaion.Common/BusinessException.cs	Five deprecated ExceptionEnum members + two BusinessExceptionHandler mappings are dead in the login path

Finding Details

F1: AZ-556 AC-5 — no dedicated paired-latency timing test (Medium / Spec-Gap)

• Location: e2e/Azaion.E2E/Tests/PasswordHashingTests.cs (test suite scope)
• Description: AC-5 calls for 1000 paired "unknown email" vs "known + wrong password" requests with p50/p95 within 5%. We have Login_timing_is_independent_of_password_length_ac5 (per-length timing), but not the unknown-vs-wrong paired comparison.
• Suggestion: Structural mitigation already in place — Security.VerifyDummy is constructed from HashPassword(...) so it uses the same Argon2id parameters as the real verify. Adding 1000 paired E2E samples would add ~3 minutes to every CI run and Argon2id work-factor noise dominates the 5% ceiling anyway. Recommendation: accept structural argument; tracker follow-up if the deploy gate insists on the live measurement.
• Task: AZ-556

F2: AZ-557 AC-3 — CountRecentFailedLogins 2+3 mix covered only behaviourally (Medium / Spec-Gap)

• Location: e2e/Azaion.E2E/Tests/MfaLoginTests.cs
• Description: AC-3 expects a direct assertion that CountRecentFailedLogins returns 5 given 2 login_failed + 3 mfa_login_failed rows. We test the contract end-to-end (AZ557_AC1, AZ557_AC2) — a wrong MFA crosses the threshold seeded by a FailedLoginCount = 9 row, which only works if the counter aggregates both event types — but we do not exercise AuditLog.CountRecentFailedLogins directly with the exact 2+3 mix.
• Suggestion: Acceptable today (behavioral coverage proves the contract). A direct unit test would require introducing a unit-test project for Azaion.Services. Recommendation: defer to the test-decompose pass.
• Task: AZ-557

F3: AZ-557 AC-4 — /login/mfa per-IP burst test deliberately omitted (Low / Spec-Gap)

• Location: e2e/Azaion.E2E/Tests/MfaLoginTests.cs
• Description: AC-4 expects HTTP 429 on a single-IP burst at /login/mfa. The endpoint correctly carries .RequireRateLimiting(LoginPerIpPolicy) (Azaion.AdminApi/Program.cs:374). The behavioral test is intentionally not added — the same policy is exercised at /login and the corresponding LoginRateLimitTests.AC1_Per_ip_rate_limit_returns_429 is stubbed (Task.CompletedTask) because tripping the per-IP limiter from inside the suite destabilises every subsequent test that runs from the same client.
• Suggestion: Accept the stub pattern from AZ-537 — code-level evidence (single policy object, single attachment line) covers the AC.
• Task: AZ-557

F4: Deprecated ExceptionEnum members + handler mappings are dead in the login path (Low / Maintainability)

• Location: Azaion.Common/BusinessException.cs, Azaion.AdminApi/BusinessExceptionHandler.cs:55-56
• Description: NoEmailFound, WrongPassword, UserDisabled, AccountLocked, LoginRateLimited are no longer thrown by UserService.ValidateUser / MfaService.VerifyForLogin. NoEmailFound + WrongPassword are still thrown by admin-side MFA Enroll/Confirm/Disable (lines 75, 81, 138, 166, 173 of MfaService.cs), so they remain live — but UserDisabled, AccountLocked, LoginRateLimited have no remaining production throws.
• Suggestion: Intentional. The AZ-556 task spec calls for a deprecation window so cross-workspace verifiers (gps-denied, satellite-provider, ui) that pattern-match on the old codes don't break. The deprecation notes in BusinessException.cs already point to a future removal ticket.
• Task: AZ-556

Phase Summary

Phase	Result
1 — Context loading	Task specs + dependencies table read
2 — Spec compliance	AZ-556 ACs 1/2/3/6/7 covered; AC-4 covered structurally via Security.VerifyDummy + audit-row test; AC-5 documented gap (F1). AZ-557 ACs 1/2/5/6/7 covered; AC-3 covered behaviourally (F2); AC-4 by code-attachment + stub-parity (F3).
3 — Code quality	SRP: RegisterFailedLoginCore + FailureKind enum keep both factors on one accounting path. DRY: shared lockout logic deduplicated. No swallowed errors.
4 — Security quick-scan	Net security improvement (closes F-AUTH-1, F-AUTH-3, F-AUTH-MFA). No new injection surfaces. DummyHashForTiming plaintext is a labelled side-channel artefact, not a credential.
5 — Performance scan	Security.VerifyDummy adds an Argon2id call to the unknown-email + disabled paths (required by threat model, bounded by per-IP limiter). CountRecentFailedLogins gained a second predicate on the existing composite index — no plan change.
6 — Cross-task consistency	AZ-557 cleanly consumes AZ-556 primitives (InvalidCredentials, audit recorders, shared accounting). No conflicting patterns.
7 — Architecture compliance	Azaion.Services → Azaion.Common (for AuthConfig) is the established layer direction. No new cross-component internal imports. No new cyclic deps.

Verdict Logic

No Critical or High findings. Two Medium and two Low → PASS_WITH_WARNINGS.

Auto-Fix Gate Disposition

#	Severity	Category	Eligible?	Disposition
1	Medium	Spec-Gap	Escalate	Documented structural mitigation; tracker follow-up if needed
2	Medium	Spec-Gap	Escalate	Behavioral coverage accepted; defer unit-test scaffolding
3	Low	Spec-Gap	Auto-fix-eligible by severity, but accepted as parity with AZ-537 stub	No change
4	Low	Maintainability	Auto-fix-eligible by severity, but intentional (deprecation window)	No change

No findings require code changes in this batch. Verdict stays PASS_WITH_WARNINGS — the implement skill auto-fix gate proceeds.