mirror of
https://github.com/azaion/satellite-provider.git
synced 2026-06-22 16:51:14 +00:00
[AZ-487] JWT validation baseline (HS256, all endpoints)
Adds Microsoft.AspNetCore.Authentication.JwtBearer 8.0.21 and the SatelliteProvider.Api.Authentication.AddSatelliteJwt extension that validates HS256 tokens against a shared JWT_SECRET (>=32 bytes, fail fast at startup). Every minimal-API endpoint now carries .RequireAuthorization(); the middleware chain is UseExceptionHandler -> UseHttpsRedirection -> UseCors -> UseAuthentication -> UseAuthorization -> endpoints. Swagger UI gets a Bearer security definition so the Authorize button works. Test infrastructure: JwtTokenFactory (unit) and JwtTestHelpers (integration) mint deterministic tokens against the same secret; the integration test runner attaches a default Bearer token to its shared HttpClient so existing tests continue to exercise protected endpoints. JwtIntegrationTests adds AC-1..AC-4 and AC-7 (Swagger advertises Bearer) end-to-end; AuthenticationServiceCollectionExtensionsTests covers AC-5 (missing/empty/short secret fail-fast) plus env-var precedence; JwtTokenFactoryTests covers AC-6 (claims pass through the JwtSecurityTokenHandler.ValidateToken path JwtBearer uses). docker-compose and scripts/run-tests.sh now propagate JWT_SECRET to the api and integration-tests containers, with a >=32-byte guard. .env.example documents the required keys; .env stays gitignored. Code review verdict: PASS_WITH_WARNINGS (2 Low findings surfaced in _docs/03_implementation/reviews/batch_01_cycle2_review.md). Cross-component coordination: gps-denied-onboard and the mission planner UI must attach Bearer tokens before this lands in dev. Co-authored-by: Cursor <cursoragent@cursor.com>
This commit is contained in:
@@ -0,0 +1,81 @@
|
||||
# Code Review Report — Batch 01 cycle 2
|
||||
|
||||
**Batch**: AZ-487 (JWT validation baseline)
|
||||
**Date**: 2026-05-11
|
||||
**Verdict**: PASS_WITH_WARNINGS
|
||||
|
||||
## Findings
|
||||
|
||||
| # | Severity | Category | File:Line | Title |
|
||||
|---|----------|----------|-----------|-------|
|
||||
| 1 | Low | Style | _docs/02_document/components/01_web_api/description.md | Task spec referenced a doc path that does not exist in the codebase |
|
||||
| 2 | Low | Security | SatelliteProvider.Api/appsettings.Development.json | Dev-only JWT secret placeholder is committed (intentional per spec) |
|
||||
|
||||
### Finding Details
|
||||
|
||||
**F1: Task spec referenced a doc path that does not exist in the codebase** (Low / Style)
|
||||
- Location: `_docs/02_document/components/01_web_api/description.md` (referenced; does not exist)
|
||||
- Description: The AZ-487 task spec lists `_docs/02_document/components/01_web_api/description.md` as a doc to update. The codebase's component-doc folders are `01_common`, `02_data_access`, `03_tile_downloader`, `04_region_processing`, `05_route_management` — there is no `01_web_api` folder. The WebApi component's documentation lives in `_docs/02_document/modules/api_program.md`.
|
||||
- Suggestion: Either (a) create the missing folder with a brief stub that defers to `api_program.md`, or (b) update the task spec for AZ-488 to point at `modules/api_program.md` and acknowledge that WebApi has no `components/*` folder. This batch chose (b) — updated `architecture.md` § Architecture Vision + § Security Architecture and `modules/api_program.md`. Surface to user for a doc-organization decision.
|
||||
- Task: AZ-487
|
||||
|
||||
**F2: Dev-only JWT secret placeholder is committed** (Low / Security)
|
||||
- Location: `SatelliteProvider.Api/appsettings.Development.json`
|
||||
- Description: The dev placeholder `DEV-ONLY-DO-NOT-USE-IN-PROD-replace-with-real-secret-via-JWT_SECRET-env-var` (78 bytes) is committed to the repo. Anyone with read access could mint a token signed with that secret. The mitigations (per task spec) are: (i) only loaded when `ASPNETCORE_ENVIRONMENT=Development`, (ii) the `JWT_SECRET` env var overrides it whenever set, (iii) the placeholder text itself signals it must be replaced.
|
||||
- Suggestion: Accept as-is for dev ergonomics; production environments must set `JWT_SECRET` (validated in `scripts/run-tests.sh` and at startup). Document on the README that this dev placeholder is intentional.
|
||||
- Task: AZ-487
|
||||
|
||||
## Phase Notes
|
||||
|
||||
### Phase 1 — Context Loading
|
||||
- Task spec read: `_docs/02_tasks/todo/AZ-487_jwt_validation_baseline.md`
|
||||
- Suite contract referenced: `suite/_docs/10_auth.md` (consumed via implementation, not modified)
|
||||
- Module layout, architecture doc, existing test patterns reviewed.
|
||||
|
||||
### Phase 2 — Spec Compliance
|
||||
All 8 acceptance criteria have at least one automated test:
|
||||
|
||||
| AC | Description | Test |
|
||||
|----|-------------|------|
|
||||
| AC-1 | Anonymous → 401 | `JwtIntegrationTests.AnonymousRequest_To_AnyEndpoint_Returns401` |
|
||||
| AC-2 | Expired → 401 | `JwtIntegrationTests.ExpiredToken_Returns401` |
|
||||
| AC-3 | Invalid signature → 401 | `JwtIntegrationTests.InvalidSignature_Returns401` |
|
||||
| AC-4 | Valid token reaches handler | `JwtIntegrationTests.ValidToken_Returns200_OnHealthyEndpoint` |
|
||||
| AC-5 | Missing/short secret → fail fast | `AuthenticationServiceCollectionExtensionsTests.AddSatelliteJwt_ThrowsOnMissingSecret/Empty/Short` |
|
||||
| AC-6 | `HttpContext.User` exposes claims | `JwtTokenFactoryTests.Create_WithExtraClaims_PropagatesClaimsThroughValidation` (exercises same `JwtSecurityTokenHandler.ValidateToken` path JwtBearer uses to populate `HttpContext.User`) |
|
||||
| AC-7 | Swagger Authorize button | `JwtIntegrationTests.SwaggerDocument_AdvertisesBearerSecurityScheme` (OpenAPI document declares `Bearer` security scheme) |
|
||||
| AC-8 | All existing tests pass | Verified at Step 11 (test-run gate); shared `HttpClient` attaches default Bearer token in `IntegrationTests/Program.cs` |
|
||||
|
||||
Contract verification: AZ-487 is a consumer of `suite/_docs/10_auth.md`. The implementation matches the consumed contract (HS256, ≥32-byte HMAC key, no issuer/audience validation, expiration required).
|
||||
|
||||
### Phase 3 — Code Quality
|
||||
- New code follows SRP (`AuthenticationServiceCollectionExtensions` does one thing).
|
||||
- Tests follow AAA pattern with explicit comments (matches project style).
|
||||
- No bare catches introduced. The pre-existing empty catch in `IntegrationTests/Program.cs::WaitForApiReady` is out of scope (pre-existing, not modified).
|
||||
- No comments narrating obvious code.
|
||||
|
||||
### Phase 4 — Security Quick-Scan
|
||||
- No SQL/command-injection vectors introduced.
|
||||
- No sensitive data in logs (error messages mention secret length, not value).
|
||||
- Dev placeholder secret: see F2.
|
||||
|
||||
### Phase 5 — Performance Scan
|
||||
- JWT validation per request is microsecond-scale HMAC + claims parsing; no I/O, no caching needed (per NFR).
|
||||
- No N+1, no unbounded fetches, no new blocking calls.
|
||||
|
||||
### Phase 6 — Cross-Task Consistency
|
||||
Single-task batch; not applicable.
|
||||
|
||||
### Phase 7 — Architecture Compliance
|
||||
- All new code in `SatelliteProvider.Api/Authentication/` — owned by the WebApi component per `module-layout.md`.
|
||||
- Imports stay within the allowed dependency table (`Microsoft.AspNetCore.Authentication.JwtBearer`, `Microsoft.IdentityModel.Tokens` are external NuGet packages, not other components).
|
||||
- Test code lives under `SatelliteProvider.Tests/` and `SatelliteProvider.IntegrationTests/` (separate test projects per layout rule 5).
|
||||
- No new cross-sibling ProjectReferences. No new cyclic dependencies. No duplicate cross-component symbols.
|
||||
|
||||
## Baseline Delta
|
||||
|
||||
`_docs/02_document/architecture_compliance_baseline.md` is not present in the repo; baseline delta section omitted.
|
||||
|
||||
## Verdict Rationale
|
||||
|
||||
Two Low findings, both already accepted in the task spec or surfacable as doc-organization decisions. No Critical, High, or Medium findings. Verdict: **PASS_WITH_WARNINGS** — proceed to commit per implement skill Step 10.
|
||||
Reference in New Issue
Block a user