azaion/admin
1
0
Fork 0
mirror of https://github.com/azaion/admin.git synced 2026-06-21 13:01:08 +00:00
Code Issues Packages Projects Releases Wiki Activity
Files
8e7c602f51169db02609ec83c0fb407ceb6478df
admin/_docs/03_implementation/reviews/batch_01_cycle2_review.md
T
Oleksandr Bezdieniezhnykh 491993f9c1
ci/woodpecker/push/01-test Pipeline failed
Details
ci/woodpecker/push/02-build-push unknown status
Details
[AZ-536] [AZ-537] [AZ-538] Argon2id, login rate limit + lockout, CORS https-only 
AZ-536 — replace unsalted SHA-384 password hashing with Argon2id (RFC 9106).
Stored as PHC string with 64 MiB / 3 iter / 1 lane defaults; legacy SHA-384
hashes detected by prefix and lazily re-hashed on next successful login.
Verify uses CryptographicOperations.FixedTimeEquals on both formats.

AZ-537 — add per-IP sliding window rate limit on /login (ASP.NET Core
RateLimiter, 10/60s default — production-tight) plus DB-backed per-account
limit (5/300s) and consecutive-failure lockout (10 / 15 min) on the users
row. Adds a generic audit_events table with INSERT/SELECT-only grants for
the app role so the per-account count is queryable and admins cannot erase
their own forensic trail. BusinessExceptionHandler maps AccountLocked to
423 and LoginRateLimited to 429, both with Retry-After.

AZ-538 — drop the http://admin.azaion.com origin from CORS, gate
UseHsts() + UseHttpsRedirection() to non-Development envs (1y / preload).

Test infra: Npgsql in the e2e project + a DbHelper for direct DB
inspection used by the AZ-536/537 ACs. appsettings.Development.json
raises PerIpPermitLimit to 1000 so the suite (~270 logins from one
container IP) doesn't false-trip the limiter.

Tests: 53 pass + 3 documented skips (per-IP rate limit needs distinct
client IPs; HSTS/HTTPS redirect need ASPNETCORE_ENVIRONMENT=Production).

Code review: PASS_WITH_WARNINGS — 0 Critical, 0 High, 1 Medium, 3 Low.
See _docs/03_implementation/reviews/batch_01_cycle2_review.md.

Closes AZ-530 epic batch 1 of 4.

Co-authored-by: Cursor <cursoragent@cursor.com>
2026-05-14 04:52:31 +03:00

7.6 KiB
Raw Blame History

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.

Reference in New Issue View Git Blame Copy Permalink