# Code Review Report

**Batch**: 4 (cycle 2) — AZ-534 (totp_2fa_login)
**Date**: 2026-05-14
**Verdict**: PASS_WITH_WARNINGS

## Phases Covered
- Phase 1: Context loading (read AZ-534 spec + Program.cs / MfaService / AuthService / RefreshTokenService deltas)
- Phase 2: Spec compliance (6/6 ACs covered, see below)
- Phase 3: Code quality (SOLID, naming, error handling, complexity)
- Phase 4: Security quick-scan (TOTP replay, recovery codes, encryption-at-rest, step-1 token audience pinning, AMR propagation)
- Phase 5: Performance scan (per-login DB writes, recovery-code verification cost)
- Phase 6: Cross-task consistency (sessions schema reused; AMR claim feeds future AZ-533 mission gate)
- Phase 7: Architecture compliance (DI registration follows existing pattern; no new cross-component imports; ProjectReference layering respected)

## AC Coverage

| AC  | Test                                                                                          | Status   |
|-----|-----------------------------------------------------------------------------------------------|----------|
| 1   | `MfaLoginTests.AC1_Enroll_returns_secret_otpauth_qr_and_recovery_codes`                       | Covered  |
| 2   | `MfaLoginTests.AC2_Confirm_enables_MFA`                                                       | Covered  |
| 3   | `MfaLoginTests.AC3_Login_returns_mfa_required_then_step2_returns_tokens_with_amr_pwd_mfa`     | Covered  |
| 4   | `MfaLoginTests.AC4_Recovery_code_works_once_then_fails`                                       | Covered  |
| 5   | `MfaLoginTests.AC5_Disable_requires_password_and_code_then_login_returns_tokens_directly`     | Covered  |
| 6   | `MfaLoginTests.AC6_Mfa_secret_is_encrypted_at_rest`                                           | Covered  |

6 of 6 acceptance criteria covered by running tests.

## Findings

| # | Severity | Category        | File                                                                | Title                                                                  |
|---|----------|-----------------|---------------------------------------------------------------------|------------------------------------------------------------------------|
| 1 | Medium   | Spec-Gap        | `Azaion.AdminApi/Program.cs` (`/sessions/mission` endpoint)         | Cross-batch: `amr=mfa` gate on mission-issuance still a TODO          |
| 2 | Low      | Security        | `Azaion.Services/MfaService.TryConsumeRecoveryCode`                 | Conditional update returns `true` even on 0-row write                  |
| 3 | Low      | Operational     | `Azaion.AdminApi/Program.cs` (DataProtection block)                 | Default key-store is ephemeral inside containers                       |
| 4 | Low      | Performance     | `Azaion.Services/MfaService.VerifyForLogin`                         | Successful login costs an extra `UPDATE users` for `mfa_last_used_window` |

### Finding Details

**F1: Mission-issuance MFA gate still a TODO** (Medium / Spec-Gap)
- Location: `Azaion.AdminApi/Program.cs` — `/sessions/mission` endpoint, line ~489
- Description: Batch 3 deferred the `RequireClaim("amr","mfa")` gate on `/sessions/mission` with the comment *"until MFA ships this is a code comment per the AZ-533 spec, not an enforced gate."* MFA has now shipped. The endpoint is still gated only on `RequireAuthorization` — any password-only access token can issue a mission token.
- Suggestion: small follow-up ticket (or amendment to AZ-533) — add `.RequireAuthorization(p => p.RequireClaim("amr", "mfa"))` (or a named policy) and remove the TODO. Intentionally not done in this batch (scope discipline: AZ-534 spec does not list it as an AC).
- Task: AZ-533 / AZ-534 follow-up

**F2: Recovery-code conditional update bypass** (Low / Security)
- Location: `Azaion.Services/MfaService.TryConsumeRecoveryCode`, line ~316–322
- Description: The conditional `WHERE id = @userId AND mfa_recovery_codes = @priorJson` defends against the read-modify-write race between two concurrent `/login/mfa` calls submitting the same recovery code. But we don't check the affected row count — both flows hit `auditLog.RecordMfaRecoveryUsed` and return tokens. Only the *write* of the consumed-code state is single-shot; the *outcome* (token issuance) double-spends. Practical risk is low (recovery codes are 80-bit secrets, not user-known; concurrent same-code attacks require an attacker who already has the code), but it's a real correctness gap.
- Suggestion: capture the LinqToDB `UpdateAsync` return value and treat 0 as "lost the race; reject this attempt". Adds one branch.
- Task: AZ-534 follow-up

**F3: DataProtection key-store ephemeral by default** (Low / Operational)
- Location: `Azaion.AdminApi/Program.cs` — DataProtection configuration block, line ~151
- Description: When `DataProtection:KeysFolder` is not set, ASP.NET Core defaults to `~/.aspnet/DataProtection-Keys` inside the container. On container restart that path is lost → every previously-encrypted `mfa_secret` becomes unrecoverable, locking out every enrolled user. The Program.cs comment is explicit about it ("Production deployments MUST set..."), and the SUT log even prints the framework's own warning. Ops needs the runbook entry, not just a code comment.
- Suggestion: (a) document `DataProtection:KeysFolder` in `_docs/04_deploy/deployment_procedures.md` next to the JWKS key-rotation section; (b) add a startup warning when running in Production *and* the folder is unset.
- Task: AZ-534 follow-up (operational)

**F4: Successful login costs an extra UPDATE** (Low / Performance)
- Location: `Azaion.Services/MfaService.VerifyForLogin`, line ~260–264
- Description: Every TOTP success persists `mfa_last_used_window` (RFC 6238 replay defence). One `UPDATE users` per `/login/mfa` for MFA-enabled users. At admin-only MFA scope (handful of accounts) this is a non-issue. If MFA is later mandated for `Role IN (Admin, ApiAdmin, ResourceUploader)` and the fleet grows, watch the `users` row write rate.
- Suggestion: monitor only — no change today.
- Task: AZ-534 (informational)

## Notes (non-blocking)

- The AC-5 test deliberately uses a **recovery code** for the mid-test login so the TOTP window stays unused for the subsequent `/disable` call. Without that, the same code presented twice within 30 s would be rejected by the (correct) replay-window check, producing a flaky 31-second `Task.Delay`. Worth highlighting in case anyone refactors that test later.
- `User.MfaRecoveryCodes` mapped in `AzaionDbSchemaHolder` with `DataType.BinaryJson` so inserts work; the disable path uses raw SQL because LinqToDB doesn't carry the type annotation through to literal `null` values in update-set expressions. Captured in the batch report (Decision #6).
- `RoleEnum.Service = 60` from batch 3 is unaffected by this change. No new role added.

## Verdict Rationale

PASS_WITH_WARNINGS — 6/6 ACs pass; full E2E suite green (82/82 enabled tests). Architecture is consistent with the existing `Auth*Service`/`SessionService` separation, DI registration follows the existing pattern, and the `amr` claim now feeds correctly through `/login` → session → `/token/refresh`. The findings are deferred-improvement items, not blocking defects.