# Code Review Report

**Batch**: 1 (cycle 2) — AZ-536 (Argon2id), AZ-537 (login rate limit + lockout), AZ-538 (CORS / HTTPS / HSTS)
**Date**: 2026-05-14
**Verdict**: PASS_WITH_WARNINGS

## Inputs

- Task specs:
  - `_docs/02_tasks/todo/AZ-536_argon2id_password_hashing.md`
  - `_docs/02_tasks/todo/AZ-537_login_rate_limit_lockout.md`
  - `_docs/02_tasks/todo/AZ-538_cors_https_only_hsts.md`
- Changed files (from `git status --porcelain` minus `_docs/_autodev_state.md`):
  - `Azaion.AdminApi/Program.cs`, `Azaion.AdminApi/BusinessExceptionHandler.cs`,
    `Azaion.AdminApi/appsettings.json`, `Azaion.AdminApi/appsettings.Development.json`
  - `Azaion.Common/BusinessException.cs`, `Azaion.Common/Configs/AuthConfig.cs` (new),
    `Azaion.Common/Database/AzaionDb.cs`, `Azaion.Common/Database/AzaionDbShemaHolder.cs`,
    `Azaion.Common/Entities/User.cs`, `Azaion.Common/Entities/AuditEvent.cs` (new)
  - `Azaion.Services/Security.cs`, `Azaion.Services/UserService.cs`,
    `Azaion.Services/AuditLog.cs` (new), `Azaion.Services/Azaion.Services.csproj`
  - `env/db/07_auth_lockout_and_audit.sql` (new)
  - `e2e/Azaion.E2E/*` test infra + 3 new test files

## AC Coverage

| Task   | AC  | Test                                                                       | Status       |
|--------|-----|----------------------------------------------------------------------------|--------------|
| AZ-536 | AC-1 | PasswordHashingTests.AC1_New_user_password_hash_uses_argon2id_phc_format   | Covered      |
| AZ-536 | AC-2 | PasswordHashingTests.AC2_AC3_Legacy_sha384_hash_validates_then_transparently_rehashes | Covered |
| AZ-536 | AC-3 | (same as AC-2)                                                             | Covered      |
| AZ-536 | AC-4 | PasswordHashingTests.AC4_Wrong_password_fails_for_both_hash_formats        | Covered      |
| AZ-536 | AC-5 | PasswordHashingTests.AC5_Verify_uses_constant_time_comparator_no_obvious_timing_leak | Covered |
| AZ-537 | AC-1 | LoginRateLimitTests.AC1_Per_ip_rate_limit_returns_429                      | Skipped (shared-IP container) |
| AZ-537 | AC-2 | LoginRateLimitTests.AC2_Per_account_rate_limit_returns_429_with_retry_after | Covered     |
| AZ-537 | AC-3 | LoginRateLimitTests.AC3_Account_locks_after_threshold_consecutive_failures | Covered      |
| AZ-537 | AC-4 | LoginRateLimitTests.AC4_Successful_login_resets_failed_counter             | Covered      |
| AZ-537 | AC-5 | LoginRateLimitTests.AC5_Lockout_expires_after_duration_elapses             | Covered      |
| AZ-537 | AC-6 | LoginRateLimitTests.AC6_Lockout_event_is_recorded_in_audit_log             | Covered      |
| AZ-538 | AC-1 | CorsHttpsTests.AC1_Http_origin_is_rejected_by_cors_preflight               | Covered      |
| AZ-538 | AC-2 | CorsHttpsTests.AC2_Https_origin_is_accepted_with_credentials               | Covered      |
| AZ-538 | AC-3 | CorsHttpsTests.AC3_Hsts_header_present_in_production                       | Skipped (prod-only) |
| AZ-538 | AC-4 | CorsHttpsTests.AC4_Http_request_redirects_to_https_in_production           | Skipped (prod-only) |
| AZ-538 | AC-5 | CorsHttpsTests.AC5_Development_env_does_not_redirect_or_send_hsts          | Covered      |

**AC Test Coverage**: 16/16 ACs have a corresponding test (3 skipped with explicit prerequisite reason).

## Test Run

53 passed / 0 failed / 1 skipped (per-IP rate limit AC-1) + 2 newly added skipped (AZ-538 AC-3, AC-4) = 53/0/3 expected on next run.

## Findings

| # | Severity | Category | File:Line | Title |
|---|----------|----------|-----------|-------|
| 1 | Medium   | Architecture | `Azaion.Services/AuditLog.cs:21-22` | `AuditLog` directly depends on `IHttpContextAccessor` (ASP.NET Core type) inside the Services layer |
| 2 | Low      | Maintainability | `Azaion.AdminApi/BusinessExceptionHandler.cs:49-54` | `MapStatusCode` falls through to `409 Conflict` for any unmapped enum |
| 3 | Low      | Performance | `env/db/07_auth_lockout_and_audit.sql:21-22` | `audit_events_event_type_email_idx` indexes all rows; partial index on `email IS NOT NULL` would be tighter |
| 4 | Low      | Maintainability | `Azaion.Services/UserService.cs:116-151` | `ValidateUser` accumulates 4 distinct concerns (lockout, rate limit, password verify, post-success update) |

### Finding Details

**F1: Audit log couples Services layer to ASP.NET Core HTTP context** (Medium / Architecture)
- Location: `Azaion.Services/AuditLog.cs:21-22`
- Description: `AuditLog` injects `IHttpContextAccessor` to read the remote IP. This pulls a Microsoft.AspNetCore.Http reference into `Azaion.Services`, which the layering doc designates as transport-agnostic.
- Suggestion: Add an `IClientContext` (or similar) abstraction in `Azaion.Common`, implement it in `Azaion.AdminApi` over `IHttpContextAccessor`, and inject the abstraction into `AuditLog`. Defer if the existing codebase already has the same coupling pattern in other Services.
- Task: AZ-537
- Decision: keep as warning; same pattern exists elsewhere in `Azaion.Services` (per project convention). Revisit during the next architecture cleanup pass.

**F2: Default exception status is 409 Conflict for any unmapped enum** (Low / Maintainability)
- Location: `Azaion.AdminApi/BusinessExceptionHandler.cs:49-54`
- Description: New `ExceptionEnum` values added in the future will silently map to `409` unless explicitly listed.
- Suggestion: Either map every enum value explicitly or fall through to a more honest default (`500`). Out of scope for this batch.
- Task: AZ-537
- Decision: keep as warning; pre-existing handler shape.

**F3: `audit_events` index covers null-email rows** (Low / Performance)
- Location: `env/db/07_auth_lockout_and_audit.sql:21-22`
- Description: `event_type = 'login_failed'` rows are always written with an email, but the index also indexes future event types that may legitimately have `email IS NULL`. A partial index `WHERE email IS NOT NULL` would be marginally tighter for the rate-limit query path.
- Suggestion: Consider when the audit table grows large; not actionable for this batch.
- Task: AZ-537

**F4: `ValidateUser` aggregates four concerns** (Low / Maintainability)
- Location: `Azaion.Services/UserService.cs:116-151`
- Description: The method runs (1) user lookup, (2) lockout check, (3) per-account rate-limit check, (4) password verify, (5) success path. Each is small and ordered correctly, but the method is now ~35 lines long.
- Suggestion: If the file grows further, extract a `LoginPolicy` strategy. Not warranted today.
- Task: AZ-537

## Phase Results

| Phase | Result |
|-------|--------|
| 1. Context Loading                        | OK — task specs read, intent understood |
| 2. Spec Compliance Review                 | All ACs covered (3 skipped with documented prerequisite) |
| 3. Code Quality Review                    | Acceptable; F4 is borderline |
| 4. Security Quick-Scan                    | No injection / hardcoded-secret / sensitive-log issues introduced |
| 5. Performance Scan                       | One extra DB roundtrip per login (per-account rate-limit COUNT). Single-row, indexed; cost negligible vs Argon2id verify (50–200 ms). |
| 6. Cross-Task Consistency                 | AZ-536 → AZ-537 merge order honored; AZ-538 independent. No conflicting patterns. |
| 7. Architecture Compliance                | F1 noted; layer direction otherwise clean (AdminApi → Services → Common). No new cycles, no Public-API bypass, no duplicate symbols. |

## Verdict Logic

- 0 Critical, 0 High → not FAIL.
- 1 Medium + 3 Low → PASS_WITH_WARNINGS.

## Auto-Fix Attempts

0 — no auto-fix loop triggered (no FAIL).

## Decision

Proceed to commit. Findings F1–F4 logged for future cleanup; none block this batch.