# Code Review Report

**Batch**: 3 (cycle 2) — AZ-535 (logout_revocation), AZ-533 (mission_token_uav)
**Date**: 2026-05-14
**Verdict**: PASS_WITH_WARNINGS

## Phases Covered
- Phase 1: Context loading (read AZ-535 + AZ-533 specs)
- Phase 2: Spec compliance (8/8 ACs covered, see below)
- Phase 3: Code quality (SOLID, naming, error handling, complexity)
- Phase 4: Security quick-scan (revocation surface, mission audience pinning, role separation)
- Phase 5: Performance scan (snapshot endpoint bound, partial indexes)
- Phase 6: Cross-task consistency (sessions table reused; revocation reasons pluralized cleanly)
- Phase 7: Architecture compliance (no new cross-component imports; ProjectReference layering respected)

## AC Coverage

| Task   | AC  | Test                                                                                          | Status   |
|--------|-----|-----------------------------------------------------------------------------------------------|----------|
| AZ-535 | 1   | `LogoutRevocationTests.AC1_Logout_revokes_caller_session_and_blocks_refresh`                  | Covered  |
| AZ-535 | 1   | `LogoutRevocationTests.AC1_Logout_is_idempotent`                                              | Covered  |
| AZ-535 | 2   | `LogoutRevocationTests.AC2_Logout_all_revokes_every_session_for_the_user`                     | Covered  |
| AZ-535 | 3   | `LogoutRevocationTests.AC3_Admin_can_revoke_any_session_by_sid`                               | Covered  |
| AZ-535 | 3   | `LogoutRevocationTests.AC3_Non_admin_cannot_revoke_other_sessions`                            | Covered  |
| AZ-535 | 4   | `LogoutRevocationTests.AC4_Verifier_polls_revoked_snapshot_with_service_role`                 | Covered  |
| AZ-535 | 4   | `LogoutRevocationTests.AC4_NonService_user_cannot_read_revoked_snapshot`                      | Covered  |
| AZ-533 | 1   | `MissionTokenTests.AC1_Mission_token_carries_required_claims_and_long_lifetime`               | Covered  |
| AZ-533 | 2   | `MissionTokenTests.AC2_Mission_id_must_match_pattern`                                         | Covered  |
| AZ-533 | 2   | `MissionTokenTests.AC2_Planned_duration_must_be_within_bounds(0.05)` + `(13)`                 | Covered  |
| AZ-533 | 3   | `MissionTokenTests.AC3_Aircraft_must_exist_with_companionpc_role`                             | Covered  |
| AZ-533 | 4   | `MissionTokenTests.AC4_Aircraft_login_auto_revokes_open_mission_sessions`                     | Covered  |

8 of 8 acceptance criteria covered by running tests.

## Findings

| # | Severity | Category        | File                                                                | Title                                                                  |
|---|----------|-----------------|---------------------------------------------------------------------|------------------------------------------------------------------------|
| 1 | Medium   | Spec-Gap        | `Azaion.AdminApi/Program.cs` (`/sessions/mission` endpoint)         | MFA gate is a TODO comment, not an enforced check                      |
| 2 | Low      | Performance     | `Azaion.Services/SessionService.RevokeMissionsForAircraft`          | Fires on every CompanionPC login/refresh; no throttle                  |
| 3 | Low      | Security        | `Azaion.AdminApi/Program.cs` (`/sessions/revoked`)                  | `since` floor (12 h) is silent — clients can't tell they were clamped  |
| 4 | Low      | Maintainability | `e2e/Azaion.E2E/Tests/MissionTokenTests.GetUserId`                  | Re-logs in to read `nameid` — adds 250 ms per use                      |

### Finding Details

**F1: MFA gate is a TODO** (Medium / Spec-Gap)
- Location: `Azaion.AdminApi/Program.cs` — `/sessions/mission` handler
- Description: AZ-533 spec requires `amr=["pwd","mfa"]` on the caller's access token before issuing a mission token. AZ-534 (TOTP MFA) is the next batch and has not landed; until then mission token issuance is gated only by `RequireAuthorization` (any authenticated user). The TODO comment in `Program.cs` makes the dependency explicit so AZ-534's PR will surface this site.
- Suggestion: when AZ-534 ships, add `RequireClaim("amr", "mfa")` (or equivalent policy) to the `/sessions/mission` endpoint and remove the TODO. Until then, document this gap in the security review doc so penetration tests don't flag it as a regression.
- Task: AZ-533

**F2: Auto-revoke fires on every CompanionPC auth call** (Low / Performance)
- Location: `Azaion.AdminApi/Program.cs` — `/login` and `/token/refresh` handlers; `Azaion.Services/SessionService.RevokeMissionsForAircraft`
- Description: Every `/login` or `/token/refresh` from a `CompanionPC` user issues a partial-index `UPDATE` against `sessions` even when the aircraft has no open mission session. The partial index `sessions_aircraft_active_idx (aircraft_id, class) WHERE revoked_at IS NULL AND aircraft_id IS NOT NULL` makes the no-op case O(0 rows touched) — but it's still a round-trip. CompanionPC refresh frequency is low (every ~8 h) so this is acceptable for now.
- Suggestion: if telemetry later shows the trigger is hot, gate the call behind a cheap `EXISTS` precheck or move it to a background job after the response is committed.
- Task: AZ-533

**F3: `since` floor is silent** (Low / Security/Observability)
- Location: `Azaion.AdminApi/Program.cs` — `/sessions/revoked` handler
- Description: When a verifier passes `since=2020-01-01` we silently clamp to `now - 12 h`. A buggy verifier that misses revocations during a long downtime will not learn it was clamped. Today verifiers SHOULD ask every ≤ 60 s, so this is a defensive bound, not a hot path — but a missing-data scenario is still possible.
- Suggestion: emit a `Warning` log when clamp triggers (`since < floor`). Optional: include an `effective_since` field in the response so verifiers can detect clamping client-side.
- Task: AZ-535

**F4: `GetUserId` test helper does an extra login** (Low / Maintainability)
- Location: `e2e/Azaion.E2E/Tests/MissionTokenTests.GetUserId`
- Description: The helper logs in twice — once for setup, then again to read the user's `nameid` from the JWT. Each login costs ~500 ms (Argon2). Across the 6 mission tests this is ~6 extra logins × ~500 ms.
- Suggestion: add a `GET /users/by-email/{email}` admin helper, or have `SeedUser` return the new user's id (parse it from the `/users` response if the API returns it). Defer until the test suite is the bottleneck.
- Task: AZ-533

## Notes (non-blocking)

- The pre-existing flake in `PasswordHashingTests.AC5_Verify_uses_constant_time_comparator_no_obvious_timing_leak` is NOT a batch-3 regression. Verified: the test passes when run in isolation. Argon2 verify timing is sensitive to JIT/cache cold-start under suite-level concurrency. If it keeps flaking, the right fix is to relax the `0.5 × overall mean` bound or warm-up Argon2 with a non-test login first.
- `RoleEnum.Service = 60` is added between `ResourceUploader = 50` and `ApiAdmin = 1000`. Existing role-string parsers (the LinqToDB converter on `User.Role`) work because the column type is `text` and the converter is `Enum.Parse(typeof(RoleEnum), v)`.

## Verdict Rationale

PASS_WITH_WARNINGS — 8/8 ACs pass, code follows the existing patterns (one service per concern, business exceptions for 4xx), no security regressions. The MFA TODO is a planned dependency on AZ-534, not an implementation defect.