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

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.