diff --git a/.cursor/skills/code-review/SKILL.md b/.cursor/skills/code-review/SKILL.md index 5dc1005..e46ca65 100644 --- a/.cursor/skills/code-review/SKILL.md +++ b/.cursor/skills/code-review/SKILL.md @@ -103,6 +103,7 @@ When multiple tasks were implemented in the same batch: - No conflicting patterns (e.g., one task uses repository pattern, another does raw SQL) - Shared code is not duplicated across task implementations - Dependencies declared in task specs are properly wired +- **Duplicate test helpers across test projects** (AZ-491): if two or more test projects (`Tests`, `IntegrationTests`, perf/load harnesses) contain near-identical helper logic — same method name plus structurally similar body, e.g. `MintValidToken` / `MintExpiredToken` / `TamperSignature` / image-fixture factories — flag as a **Medium / Maintainability** finding and recommend consolidation into the shared `SatelliteProvider.TestSupport` library. The cycle-2 retrospective documented why: the same bug existed in two copies (`SatelliteProvider.Tests/TestUtilities/JwtTokenFactory.cs` and `SatelliteProvider.IntegrationTests/JwtTestHelpers.cs`) and needed two separate fixes (commits `f64d0d7` + `11b7074`). Two near-identical implementations of credential-minting / fixture-generation logic are a security-relevance maintenance risk, not just a style issue. ## Phase 7: Architecture Compliance diff --git a/SatelliteProvider.IntegrationTests/Dockerfile b/SatelliteProvider.IntegrationTests/Dockerfile index b63cd08..22c8291 100644 --- a/SatelliteProvider.IntegrationTests/Dockerfile +++ b/SatelliteProvider.IntegrationTests/Dockerfile @@ -1,6 +1,7 @@ FROM mcr.microsoft.com/dotnet/sdk:8.0 AS build WORKDIR /src COPY ["SatelliteProvider.IntegrationTests/SatelliteProvider.IntegrationTests.csproj", "SatelliteProvider.IntegrationTests/"] +COPY ["SatelliteProvider.TestSupport/SatelliteProvider.TestSupport.csproj", "SatelliteProvider.TestSupport/"] RUN dotnet restore "SatelliteProvider.IntegrationTests/SatelliteProvider.IntegrationTests.csproj" COPY . . WORKDIR "/src/SatelliteProvider.IntegrationTests" diff --git a/SatelliteProvider.IntegrationTests/JwtIntegrationTests.cs b/SatelliteProvider.IntegrationTests/JwtIntegrationTests.cs index 217ef94..1bfe780 100644 --- a/SatelliteProvider.IntegrationTests/JwtIntegrationTests.cs +++ b/SatelliteProvider.IntegrationTests/JwtIntegrationTests.cs @@ -1,5 +1,6 @@ using System.Net; using System.Net.Http.Headers; +using SatelliteProvider.TestSupport; namespace SatelliteProvider.IntegrationTests; @@ -44,7 +45,7 @@ public static class JwtIntegrationTests Console.WriteLine("AZ-487 AC-2: Expired token returns 401"); using var client = new HttpClient { BaseAddress = new Uri(apiUrl), Timeout = TimeSpan.FromMinutes(1) }; - var expired = JwtTestHelpers.MintExpiredToken(secret); + var expired = JwtTokenFactory.CreateExpired(secret, JwtTestHelpers.DefaultSubject); client.DefaultRequestHeaders.Authorization = new AuthenticationHeaderValue("Bearer", expired); var response = await client.GetAsync(ProtectedTilesPath); @@ -64,8 +65,8 @@ public static class JwtIntegrationTests Console.WriteLine("AZ-487 AC-3: Tampered signature returns 401"); using var client = new HttpClient { BaseAddress = new Uri(apiUrl), Timeout = TimeSpan.FromMinutes(1) }; - var valid = JwtTestHelpers.MintValidToken(secret); - var tampered = JwtTestHelpers.TamperSignature(valid); + var valid = JwtTokenFactory.Create(secret, JwtTestHelpers.DefaultSubject); + var tampered = JwtTokenFactory.TamperSignature(valid); client.DefaultRequestHeaders.Authorization = new AuthenticationHeaderValue("Bearer", tampered); var response = await client.GetAsync(ProtectedRegionPath); @@ -85,7 +86,7 @@ public static class JwtIntegrationTests Console.WriteLine("AZ-487 AC-4: Valid token reaches handler unchanged"); using var client = new HttpClient { BaseAddress = new Uri(apiUrl), Timeout = TimeSpan.FromMinutes(2) }; - var valid = JwtTestHelpers.MintValidToken(secret); + var valid = JwtTokenFactory.Create(secret, JwtTestHelpers.DefaultSubject); client.DefaultRequestHeaders.Authorization = new AuthenticationHeaderValue("Bearer", valid); var response = await client.GetAsync(ProtectedTilesPath); diff --git a/SatelliteProvider.IntegrationTests/JwtTestHelpers.cs b/SatelliteProvider.IntegrationTests/JwtTestHelpers.cs index 5cf0f6d..7a870a4 100644 --- a/SatelliteProvider.IntegrationTests/JwtTestHelpers.cs +++ b/SatelliteProvider.IntegrationTests/JwtTestHelpers.cs @@ -1,8 +1,5 @@ -using System.IdentityModel.Tokens.Jwt; using System.Net.Http.Headers; -using System.Security.Claims; using System.Text; -using Microsoft.IdentityModel.Tokens; namespace SatelliteProvider.IntegrationTests; @@ -31,58 +28,6 @@ public static class JwtTestHelpers return secret; } - public static string MintValidToken(string secret, string subject = DefaultSubject, TimeSpan? lifetime = null, IEnumerable? extraClaims = null) - { - ArgumentNullException.ThrowIfNull(secret); - - var signingKey = new SymmetricSecurityKey(Encoding.UTF8.GetBytes(secret)); - var credentials = new SigningCredentials(signingKey, SecurityAlgorithms.HmacSha256); - - var now = DateTime.UtcNow; - var expires = now.Add(lifetime ?? TimeSpan.FromHours(1)); - // JwtSecurityToken rejects Expires <= NotBefore. Shift NotBefore - // behind Expires for the expired-token test fixture. - var notBefore = expires <= now ? expires.AddMinutes(-5) : now; - var claims = new List - { - new(JwtRegisteredClaimNames.Sub, subject), - new(JwtRegisteredClaimNames.Jti, Guid.NewGuid().ToString("N")) - }; - if (extraClaims is not null) - { - claims.AddRange(extraClaims); - } - - var token = new JwtSecurityToken( - issuer: null, - audience: null, - claims: claims, - notBefore: notBefore, - expires: expires, - signingCredentials: credentials); - - return new JwtSecurityTokenHandler().WriteToken(token); - } - - public static string MintExpiredToken(string secret, string subject = DefaultSubject) - { - return MintValidToken(secret, subject, lifetime: TimeSpan.FromMinutes(-10)); - } - - public static string TamperSignature(string token) - { - var parts = token.Split('.'); - if (parts.Length != 3) - { - throw new ArgumentException("JWT must have three dot-separated segments.", nameof(token)); - } - - var signature = parts[2]; - var firstChar = signature[0]; - parts[2] = (firstChar == 'A' ? 'B' : 'A') + signature[1..]; - return string.Join('.', parts); - } - public static void AttachDefaultAuthorization(HttpClient httpClient, string token) { httpClient.DefaultRequestHeaders.Authorization = new AuthenticationHeaderValue("Bearer", token); diff --git a/SatelliteProvider.IntegrationTests/Program.cs b/SatelliteProvider.IntegrationTests/Program.cs index 5e308d3..e366d03 100644 --- a/SatelliteProvider.IntegrationTests/Program.cs +++ b/SatelliteProvider.IntegrationTests/Program.cs @@ -1,3 +1,5 @@ +using SatelliteProvider.TestSupport; + namespace SatelliteProvider.IntegrationTests; class Program @@ -42,7 +44,7 @@ class Program Timeout = TimeSpan.FromMinutes(15) }; - var defaultToken = JwtTestHelpers.MintValidToken(jwtSecret); + var defaultToken = JwtTokenFactory.Create(jwtSecret, JwtTestHelpers.DefaultSubject); JwtTestHelpers.AttachDefaultAuthorization(httpClient, defaultToken); try diff --git a/SatelliteProvider.IntegrationTests/SatelliteProvider.IntegrationTests.csproj b/SatelliteProvider.IntegrationTests/SatelliteProvider.IntegrationTests.csproj index 41ac51a..e642543 100644 --- a/SatelliteProvider.IntegrationTests/SatelliteProvider.IntegrationTests.csproj +++ b/SatelliteProvider.IntegrationTests/SatelliteProvider.IntegrationTests.csproj @@ -10,7 +10,10 @@ - + + + + diff --git a/SatelliteProvider.IntegrationTests/UavUploadTests.cs b/SatelliteProvider.IntegrationTests/UavUploadTests.cs index 0d2a8ae..bcc98c1 100644 --- a/SatelliteProvider.IntegrationTests/UavUploadTests.cs +++ b/SatelliteProvider.IntegrationTests/UavUploadTests.cs @@ -4,6 +4,7 @@ using System.Net.Http.Json; using System.Security.Claims; using System.Text.Json; using Npgsql; +using SatelliteProvider.TestSupport; using SixLabors.ImageSharp; using SixLabors.ImageSharp.Formats.Jpeg; using SixLabors.ImageSharp.PixelFormats; @@ -49,7 +50,7 @@ public static class UavUploadTests } }; using var client = CreateClient(apiUrl); - AttachToken(client, JwtTestHelpers.MintValidToken(secret, extraClaims: GpsClaim())); + AttachToken(client, JwtTokenFactory.Create(secret, JwtTestHelpers.DefaultSubject, extraClaims: GpsClaim())); // Act var response = await PostBatch(client, metadata, new[] { CreateValidJpeg() }); @@ -83,7 +84,7 @@ public static class UavUploadTests items = coords.Select(c => new { latitude = c.Latitude, longitude = c.Longitude, tileZoom = 18, tileSizeMeters = 200.0, capturedAt = DateTime.UtcNow.ToString("o") }).ToArray() }; using var client = CreateClient(apiUrl); - AttachToken(client, JwtTestHelpers.MintValidToken(secret, extraClaims: GpsClaim())); + AttachToken(client, JwtTokenFactory.Create(secret, JwtTestHelpers.DefaultSubject, extraClaims: GpsClaim())); var good = CreateValidJpeg(); var wrongDimensions = CreateValidJpeg(width: 512, height: 512); @@ -149,7 +150,7 @@ public static class UavUploadTests } }; using var client = CreateClient(apiUrl); - AttachToken(client, JwtTestHelpers.MintValidToken(secret, extraClaims: GpsClaim())); + AttachToken(client, JwtTokenFactory.Create(secret, JwtTestHelpers.DefaultSubject, extraClaims: GpsClaim())); // Act var response = await PostBatch(client, metadata, new[] { CreateValidJpeg() }); @@ -173,7 +174,7 @@ public static class UavUploadTests // Arrange var coord = NextTestCoordinate(); using var client = CreateClient(apiUrl); - AttachToken(client, JwtTestHelpers.MintValidToken(secret, extraClaims: GpsClaim())); + AttachToken(client, JwtTokenFactory.Create(secret, JwtTestHelpers.DefaultSubject, extraClaims: GpsClaim())); var firstMetadata = new { @@ -241,7 +242,7 @@ public static class UavUploadTests // Arrange using var client = CreateClient(apiUrl); - AttachToken(client, JwtTestHelpers.MintValidToken(secret, extraClaims: new[] { new Claim(PermissionsClaimType, "FL") })); + AttachToken(client, JwtTokenFactory.Create(secret, JwtTestHelpers.DefaultSubject, extraClaims: new[] { new Claim(PermissionsClaimType, "FL") })); var coord = NextTestCoordinate(); var metadata = new { @@ -283,7 +284,7 @@ public static class UavUploadTests var placeholder = new byte[] { 0xFF, 0xD8, 0xFF, 0xD9 }; var files = Enumerable.Range(0, oversize).Select(_ => placeholder).ToArray(); using var client = CreateClient(apiUrl); - AttachToken(client, JwtTestHelpers.MintValidToken(secret, extraClaims: GpsClaim())); + AttachToken(client, JwtTokenFactory.Create(secret, JwtTestHelpers.DefaultSubject, extraClaims: GpsClaim())); // Act var response = await PostBatch(client, metadata, files); diff --git a/SatelliteProvider.Tests/TestUtilities/JwtTokenFactory.cs b/SatelliteProvider.TestSupport/JwtTokenFactory.cs similarity index 93% rename from SatelliteProvider.Tests/TestUtilities/JwtTokenFactory.cs rename to SatelliteProvider.TestSupport/JwtTokenFactory.cs index f1802c5..81c4fc9 100644 --- a/SatelliteProvider.Tests/TestUtilities/JwtTokenFactory.cs +++ b/SatelliteProvider.TestSupport/JwtTokenFactory.cs @@ -3,7 +3,7 @@ using System.Security.Claims; using System.Text; using Microsoft.IdentityModel.Tokens; -namespace SatelliteProvider.Tests.TestUtilities; +namespace SatelliteProvider.TestSupport; public static class JwtTokenFactory { @@ -27,7 +27,8 @@ public static class JwtTokenFactory // JwtSecurityToken rejects Expires <= NotBefore. For negative // lifetimes (expired-token test fixture) shift NotBefore behind // Expires so the constructor accepts the token and lifetime - // validation can fire downstream. + // validation can fire downstream. DO NOT remove this branch — + // see cycle-2 commits f64d0d7 + 11b7074 (LESSONS.md L-testing). var notBefore = expires <= now ? expires.AddMinutes(-5) : now; var claims = new List diff --git a/SatelliteProvider.TestSupport/SatelliteProvider.TestSupport.csproj b/SatelliteProvider.TestSupport/SatelliteProvider.TestSupport.csproj new file mode 100644 index 0000000..14a97eb --- /dev/null +++ b/SatelliteProvider.TestSupport/SatelliteProvider.TestSupport.csproj @@ -0,0 +1,15 @@ + + + + net8.0 + enable + enable + false + + + + + + + + diff --git a/SatelliteProvider.Tests/Authentication/AuthenticationServiceCollectionExtensionsTests.cs b/SatelliteProvider.Tests/Authentication/AuthenticationServiceCollectionExtensionsTests.cs index 5d2dcf2..d7820bb 100644 --- a/SatelliteProvider.Tests/Authentication/AuthenticationServiceCollectionExtensionsTests.cs +++ b/SatelliteProvider.Tests/Authentication/AuthenticationServiceCollectionExtensionsTests.cs @@ -7,7 +7,7 @@ using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Options; using Microsoft.IdentityModel.Tokens; using SatelliteProvider.Api.Authentication; -using SatelliteProvider.Tests.TestUtilities; +using SatelliteProvider.TestSupport; using AuthExtensions = SatelliteProvider.Api.Authentication.AuthenticationServiceCollectionExtensions; namespace SatelliteProvider.Tests.Authentication; diff --git a/SatelliteProvider.Tests/Authentication/JwtTokenFactoryTests.cs b/SatelliteProvider.Tests/Authentication/JwtTokenFactoryTests.cs index 1acb43c..e34a908 100644 --- a/SatelliteProvider.Tests/Authentication/JwtTokenFactoryTests.cs +++ b/SatelliteProvider.Tests/Authentication/JwtTokenFactoryTests.cs @@ -3,7 +3,7 @@ using System.Security.Claims; using System.Text; using FluentAssertions; using Microsoft.IdentityModel.Tokens; -using SatelliteProvider.Tests.TestUtilities; +using SatelliteProvider.TestSupport; namespace SatelliteProvider.Tests.Authentication; diff --git a/SatelliteProvider.Tests/SatelliteProvider.Tests.csproj b/SatelliteProvider.Tests/SatelliteProvider.Tests.csproj index 49367ab..ca6d690 100644 --- a/SatelliteProvider.Tests/SatelliteProvider.Tests.csproj +++ b/SatelliteProvider.Tests/SatelliteProvider.Tests.csproj @@ -43,6 +43,7 @@ + diff --git a/SatelliteProvider.sln b/SatelliteProvider.sln index 0230248..3f9c983 100644 --- a/SatelliteProvider.sln +++ b/SatelliteProvider.sln @@ -17,6 +17,8 @@ Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "SatelliteProvider.DataAcces EndProject Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "SatelliteProvider.IntegrationTests", "SatelliteProvider.IntegrationTests\SatelliteProvider.IntegrationTests.csproj", "{938FC7B2-759F-4B8F-90A3-21F8AD15BB1F}" EndProject +Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "SatelliteProvider.TestSupport", "SatelliteProvider.TestSupport\SatelliteProvider.TestSupport.csproj", "{C7E1A491-4914-4914-9914-491491491491}" +EndProject Global GlobalSection(SolutionConfigurationPlatforms) = preSolution Debug|Any CPU = Debug|Any CPU @@ -55,5 +57,9 @@ Global {938FC7B2-759F-4B8F-90A3-21F8AD15BB1F}.Debug|Any CPU.Build.0 = Debug|Any CPU {938FC7B2-759F-4B8F-90A3-21F8AD15BB1F}.Release|Any CPU.ActiveCfg = Release|Any CPU {938FC7B2-759F-4B8F-90A3-21F8AD15BB1F}.Release|Any CPU.Build.0 = Release|Any CPU + {C7E1A491-4914-4914-9914-491491491491}.Debug|Any CPU.ActiveCfg = Debug|Any CPU + {C7E1A491-4914-4914-9914-491491491491}.Debug|Any CPU.Build.0 = Debug|Any CPU + {C7E1A491-4914-4914-9914-491491491491}.Release|Any CPU.ActiveCfg = Release|Any CPU + {C7E1A491-4914-4914-9914-491491491491}.Release|Any CPU.Build.0 = Release|Any CPU EndGlobalSection EndGlobal diff --git a/_docs/02_document/module-layout.md b/_docs/02_document/module-layout.md index ee7d870..8719145 100644 --- a/_docs/02_document/module-layout.md +++ b/_docs/02_document/module-layout.md @@ -164,6 +164,17 @@ The cycle-1 (AZ-487) and cycle-2 (AZ-488) code reviews each surfaced an F1 (Low - **Consumed by**: DataAccess (entity defaults, type handler registration), TileDownloader (sets `TileEntity.Source` via `TileSourceConverter.ToWireValue`), Tests - **Important constraint**: Dapper's `SqlMapper.TypeHandler` is bypassed for enum reads (Dapper issue #259 — see `_docs/LESSONS.md` L-001). For any new enum that must round-trip through a database column, prefer the `string`-on-entity + `Enum`-at-API-boundary pattern with a converter class in this folder. Do NOT register a `TypeHandler` and assume it will be honored on reads. +### TestSupport (added by AZ-491) + +- **Directory**: `SatelliteProvider.TestSupport/` +- **csproj**: `SatelliteProvider.TestSupport/SatelliteProvider.TestSupport.csproj` (class library, no test framework) +- **Purpose**: Canonical home for cross-project test utilities. Currently holds `JwtTokenFactory` (HS256 token minting + signature tampering). Replaces the cycle-2 duplicate that lived in both `SatelliteProvider.Tests/TestUtilities/JwtTokenFactory.cs` and `SatelliteProvider.IntegrationTests/JwtTestHelpers.cs` and required parallel fixes. Future additions: shared image-fixture factories, shared deterministic clocks / test-data builders that need to be visible to both unit and integration projects. +- **Public API**: `SatelliteProvider.TestSupport/JwtTokenFactory.cs` (`Create`, `CreateExpired`, `TamperSignature`) +- **PackageReferences**: `Microsoft.IdentityModel.Tokens` 7.0.3, `System.IdentityModel.Tokens.Jwt` 7.0.3 (matches the integration tests' pre-AZ-491 explicit reference). +- **Consumed by**: `SatelliteProvider.Tests`, `SatelliteProvider.IntegrationTests` (both via `ProjectReference`). +- **Not consumed by**: production projects (`Api`, `Common`, `DataAccess`, `Services.*`). The TestSupport library is test-only by design; production code must NOT depend on it. +- **Runner-side concerns NOT in TestSupport**: `SatelliteProvider.IntegrationTests/JwtTestHelpers.cs` retains `ResolveSecretOrThrow`, `AttachDefaultAuthorization`, and the `DefaultSubject = "integration-tests"` constant — these are runner-specific (env-var reads, `HttpClient` mutation, runner-identity subject) and intentionally not consolidated. + ## Allowed Dependencies (layering) | Layer | Components | May import from (compile-time ProjectReferences) | diff --git a/_docs/02_document/modules/tests_integration.md b/_docs/02_document/modules/tests_integration.md index ba7daba..f37e28e 100644 --- a/_docs/02_document/modules/tests_integration.md +++ b/_docs/02_document/modules/tests_integration.md @@ -12,7 +12,7 @@ Console application that runs end-to-end integration tests against a live API in - `ComplexRouteTests` — routes with geofencing - `ExtendedRouteTests` — routes with `requestMaps: true` and tile ZIP creation - `MigrationTests` — direct PostgreSQL schema/index validation (no HTTP). AZ-484 cycle added: `NewUniqueConstraintIncludesSourceColumn_AZ484_AC1`, `BackfillUpdateAssignsGoogleMapsAndCapturedAt_AZ484_AC4`, `MultiSourceInsertCoexistsUnderNewIndex_AZ484_AC1`, `MostRecentAcrossSourcesSelection_AZ484_AC2`, `SameSourceUpsertReplacesPreviousRow_AZ484_AC3` (latter four use temp tables to keep production data untouched). -- `JwtIntegrationTests` (added by AZ-487, cycle 2) — `AnonymousRequest_To_AnyEndpoint_Returns401`, `ExpiredToken_Returns401`, `InvalidSignature_Returns401`, `ValidToken_Returns200_OnHealthyEndpoint`, `SwaggerDocument_AdvertisesBearerSecurityScheme`. Helpers in `JwtTestHelpers` mint HS256 tokens; the test runner sets `JWT_SECRET` on the API container and attaches a Bearer token to every existing test's HTTP requests so the pre-cycle-2 suite continues to pass. +- `JwtIntegrationTests` (added by AZ-487, cycle 2; helpers consolidated by AZ-491 cycle 3) — `AnonymousRequest_To_AnyEndpoint_Returns401`, `ExpiredToken_Returns401`, `InvalidSignature_Returns401`, `ValidToken_Returns200_OnHealthyEndpoint`, `SwaggerDocument_AdvertisesBearerSecurityScheme`. HS256 token minting lives in the shared `SatelliteProvider.TestSupport.JwtTokenFactory` (consumed via `ProjectReference`); runner-specific concerns (`JwtTestHelpers.ResolveSecretOrThrow`, `AttachDefaultAuthorization`, `DefaultSubject = "integration-tests"`) remain in this project. The test runner sets `JWT_SECRET` on the API container and attaches a Bearer token to every existing test's HTTP requests so the pre-cycle-2 suite continues to pass. - `UavUploadTests` (added by AZ-488, cycle 2) — `HappyPathSingleItem_PersistsRow`, `MixedBatch_ReturnsPerItemResults`, `MultiSourceCoexistence_AZ484_Cycle2`, `SameSourceUpsert_AZ484_Cycle2`, `NoToken_Returns401`, `ValidTokenWithoutGpsPermission_Returns403`, `OversizedBatch_Returns400`. Uses a wall-clock-seeded coordinate counter (`_coordinateCounter` initialized from `DateTime.UtcNow`) so each docker-compose run picks a fresh coordinate band — the postgres named volume persists across runs and a naïve `int = 0` counter collided with prior runs on the per-source unique index (fixed mid-Step-11). - `StubAndErrorContractTests` (existing) — updated in cycle 2 to drop the legacy `StubUpload_Returns501` expectation since AZ-488 implemented the endpoint. diff --git a/_docs/02_document/modules/tests_unit.md b/_docs/02_document/modules/tests_unit.md index 47abe8f..bdea00a 100644 --- a/_docs/02_document/modules/tests_unit.md +++ b/_docs/02_document/modules/tests_unit.md @@ -24,7 +24,7 @@ Existing baseline (pre-cycle-2) test classes cover `TileService`, `RegionService - `JwtSecurityTokenHandler.MapInboundClaims = false` is set explicitly in JWT tests so claims read by their original names (`sub`, `permissions`, …) rather than the framework-remapped names. ## Dependencies -- Project references: `SatelliteProvider.Services.TileDownloader`, `SatelliteProvider.Services.RegionProcessing`, `SatelliteProvider.Services.RouteManagement`, `SatelliteProvider.Common`, `SatelliteProvider.DataAccess`, `SatelliteProvider.Api` (for the Authentication tests — added in AZ-487). +- Project references: `SatelliteProvider.Services.TileDownloader`, `SatelliteProvider.Services.RegionProcessing`, `SatelliteProvider.Services.RouteManagement`, `SatelliteProvider.Common`, `SatelliteProvider.DataAccess`, `SatelliteProvider.Api` (for the Authentication tests — added in AZ-487), `SatelliteProvider.TestSupport` (added by AZ-491; provides the canonical `JwtTokenFactory` consumed by both this project and `SatelliteProvider.IntegrationTests`). - NuGet: xUnit (2.5.3), Moq (4.20.72), FluentAssertions (8.8.0), coverlet.collector (6.0.0), Microsoft.NET.Test.Sdk (17.8.0), Microsoft.Extensions.* (Caching.Memory, Configuration, DI, Logging, Options, Http), `Microsoft.AspNetCore.Authentication.JwtBearer` 8.0.25 (consumed transitively via the `ProjectReference` to `SatelliteProvider.Api`; AZ-487 added the dependency at 8.0.21, AZ-496 bumped it to 8.0.25), `SixLabors.ImageSharp` 3.1.11 (added by AZ-488 for the gate tests). - `appsettings.json` copied to output (used by Authentication tests for the `Jwt` section binding scenario). diff --git a/_docs/02_tasks/todo/AZ-491_consolidate_jwt_test_helpers.md b/_docs/02_tasks/done/AZ-491_consolidate_jwt_test_helpers.md similarity index 100% rename from _docs/02_tasks/todo/AZ-491_consolidate_jwt_test_helpers.md rename to _docs/02_tasks/done/AZ-491_consolidate_jwt_test_helpers.md diff --git a/_docs/03_implementation/batch_02_cycle3_report.md b/_docs/03_implementation/batch_02_cycle3_report.md new file mode 100644 index 0000000..589de03 --- /dev/null +++ b/_docs/03_implementation/batch_02_cycle3_report.md @@ -0,0 +1,66 @@ +# Batch Report — Batch 02 cycle 3 + +**Batch**: 02 (cycle 3) +**Tasks**: AZ-491 (consolidate JWT test-mint helpers) +**Date**: 2026-05-12 + +## Task Results + +| Task | Status | Files Modified | Tests | AC Coverage | Issues | +|------|--------|---------------|-------|-------------|--------| +| AZ-491_consolidate_jwt_test_helpers | Done | 2 added + 12 modified + 1 deleted (see below) | Existing 253 unit tests + integration JWT/UAV tests now consume the consolidated factory (Step 16 final gate) | 6/6 ACs covered | 0 blockers | + +## AC Test Coverage: All covered (6 of 6) +## Code Review Verdict: pending (this batch report precedes per-batch review) +## Auto-Fix Attempts: 0 +## Stuck Agents: None + +## What was implemented + +Chose **Option A** — new `SatelliteProvider.TestSupport` class library. Rationale: cleaner long-term layering (production code may NOT depend on test-support; integration tests do NOT acquire a dependency on unit tests); pre-stages AZ-492 which needs the same canonical mint surface for the perf harness. + +### Added + +- `SatelliteProvider.TestSupport/SatelliteProvider.TestSupport.csproj` (class library, .NET 8.0, no test framework). PackageReferences: `Microsoft.IdentityModel.Tokens` 7.0.3, `System.IdentityModel.Tokens.Jwt` 7.0.3. +- `SatelliteProvider.TestSupport/JwtTokenFactory.cs` — canonical implementation. Exposes `Create(secret, subject=DefaultSubject, lifetime, extraClaims, algorithm)`, `CreateExpired(secret, subject)`, `TamperSignature(token)`. The `Expires <= NotBefore` workaround (shift `notBefore` behind `expires` for non-positive lifetimes) is preserved with an inline comment back-referencing the cycle-2 commits `f64d0d7` + `11b7074` to prevent regression. + +### Modified + +- `SatelliteProvider.Tests/SatelliteProvider.Tests.csproj` — added `ProjectReference` to TestSupport. +- `SatelliteProvider.Tests/Authentication/JwtTokenFactoryTests.cs` — `using SatelliteProvider.Tests.TestUtilities;` → `using SatelliteProvider.TestSupport;`. The 4 existing test methods (`Create_ProducesTokenValidatedByMatchingParameters`, `Create_WithExtraClaims_PropagatesClaimsThroughValidation`, `CreateExpired_TokenFailsValidationWithLifetimeException`, `TamperSignature_TokenFailsValidationWithSignatureException`) now exercise the consolidated factory. +- `SatelliteProvider.Tests/Authentication/AuthenticationServiceCollectionExtensionsTests.cs` — same namespace switch for the `JwtTokenFactory.Create(envSecret)` call site. +- `SatelliteProvider.IntegrationTests/SatelliteProvider.IntegrationTests.csproj` — added `ProjectReference` to TestSupport; removed the explicit `System.IdentityModel.Tokens.Jwt` PackageReference (now flowing transitively through TestSupport). +- `SatelliteProvider.IntegrationTests/JwtTestHelpers.cs` — stripped to runner-only concerns: `JwtSecretEnvVar`, `DefaultSubject = "integration-tests"`, `ResolveSecretOrThrow`, `AttachDefaultAuthorization`. Removed `MintValidToken`, `MintExpiredToken`, `TamperSignature`. The `using System.IdentityModel.Tokens.Jwt;` / `using Microsoft.IdentityModel.Tokens;` imports were removed since they are no longer needed at this site. +- `SatelliteProvider.IntegrationTests/Program.cs` — `JwtTestHelpers.MintValidToken(jwtSecret)` → `JwtTokenFactory.Create(jwtSecret, JwtTestHelpers.DefaultSubject)`. `using SatelliteProvider.TestSupport;` added. +- `SatelliteProvider.IntegrationTests/JwtIntegrationTests.cs` — 3 call sites switched: `MintExpiredToken(secret)` → `JwtTokenFactory.CreateExpired(secret, JwtTestHelpers.DefaultSubject)`; `MintValidToken(secret)` → `JwtTokenFactory.Create(secret, JwtTestHelpers.DefaultSubject)`; `TamperSignature` → `JwtTokenFactory.TamperSignature`. `using SatelliteProvider.TestSupport;` added. +- `SatelliteProvider.IntegrationTests/UavUploadTests.cs` — 6 call sites of `JwtTestHelpers.MintValidToken(secret, extraClaims: ...)` switched to `JwtTokenFactory.Create(secret, JwtTestHelpers.DefaultSubject, extraClaims: ...)`. `using SatelliteProvider.TestSupport;` added. +- `SatelliteProvider.IntegrationTests/Dockerfile` — added `COPY ["SatelliteProvider.TestSupport/SatelliteProvider.TestSupport.csproj", "SatelliteProvider.TestSupport/"]` before the `dotnet restore` step so the build context restores the new ProjectReference correctly. The API Dockerfile is unchanged (TestSupport is NOT a production dependency). +- `SatelliteProvider.sln` — new project entry for TestSupport + ProjectConfigurationPlatforms rows. +- `.cursor/skills/code-review/SKILL.md` Phase 6 — added a bolded checklist row: "Duplicate test helpers across test projects (AZ-491)" — flags near-identical credential-minting / fixture-generation logic across `Tests` + `IntegrationTests` + future perf harnesses as a **Medium / Maintainability** finding with recommended consolidation into `SatelliteProvider.TestSupport`. The cycle-2 retro `f64d0d7 + 11b7074` precedent is cited inline so future agents understand this is a security-relevance maintenance risk, not just a style nit. +- `_docs/02_document/module-layout.md` — added a new "TestSupport (added by AZ-491)" Shared / Cross-Cutting entry with Public API, PackageReferences, Consumed-by, and an explicit "NOT consumed by production projects" invariant. Documents what stays in `JwtTestHelpers` (runner-side concerns) vs. what moved. +- `_docs/02_document/modules/tests_unit.md` and `_docs/02_document/modules/tests_integration.md` — updated to reflect the new dependency on `SatelliteProvider.TestSupport` and the split of mint logic (TestSupport) vs. runner concerns (`JwtTestHelpers`). + +### Deleted + +- `SatelliteProvider.Tests/TestUtilities/JwtTokenFactory.cs` (82 lines; content migrated verbatim to TestSupport with the `notBefore` regression-prevention comment expanded). The `TestUtilities/` directory itself is retained because it still owns `UavTileImageFactory.cs` (out of scope per the AZ-491 task spec § Excluded). + +## AC Verification + +| AC | Status | Evidence | +|----|--------|----------| +| AC-1: Single source of truth — only one `JwtSecurityToken(` constructor call in test code | ✓ | `rg "new JwtSecurityToken"` returns only `SatelliteProvider.TestSupport/JwtTokenFactory.cs:46` | +| AC-2: Integration tests pass unchanged | Deferred to Step 16 | Call-site signatures preserved (subject explicitly passed via `JwtTestHelpers.DefaultSubject`); semantics identical | +| AC-3: Unit tests pass unchanged | Deferred to Step 16 | Identical method shapes; only the import statement changed | +| AC-4: Runner-side concerns preserved | ✓ | `ResolveSecretOrThrow` + `AttachDefaultAuthorization` + `DefaultSubject` unchanged in `JwtTestHelpers.cs` | +| AC-5: Cycle-2 IDX12401 fix preserved | ✓ | The `notBefore` shift branch is verbatim from the cycle-2 fixes, with an additional regression-prevention comment citing commits `f64d0d7` + `11b7074` | +| AC-6: Code-review rule lands | ✓ | `.cursor/skills/code-review/SKILL.md` Phase 6 now carries a duplicate-helper detection rule with severity, finding-category, and project-name guidance | + +## Open follow-ups (non-blocking) + +- **AZ-492 dependency**: the perf harness PBI can now consume `SatelliteProvider.TestSupport.JwtTokenFactory` directly (or via a shell-based JWT minter that mirrors the same parameters). The `_docs/_process_leftovers/2026-05-11_perf-pt07-harness.md` leftover entry can be closed by AZ-492's implementation. +- **AZ-494 dependency**: the planned `iss` / `aud` issuer/audience parameters on `JwtTokenFactory.Create` will be added by AZ-494 (still gated on external admin team input for the actual values). The current signature deliberately keeps `issuer: null, audience: null` so all current tests continue to pass against the existing `ValidateIssuer = false / ValidateAudience = false` API configuration. +- **Test-suite gate**: AZ-491 AC-2 + AC-3 require `./scripts/run-tests.sh --full` to be green. Deferred to Step 16 (Final Test Run). Risk for AZ-491 is moderate — the consolidation touches every JWT test call site — but every change is a syntactic rewrite, not a semantic one. + +## Next Batch: AZ-493 (Integration test DB-reset hook) + +AZ-493 is 2 SP. It introduces a startup-side database-reset hook in `SatelliteProvider.IntegrationTests/Program.cs` to truncate FK-safe tables (`tiles`, `regions`, `routes`, ...) before the test suite runs, eliminating the wallclock-seeded `_coordinateCounter` workaround that UavUploadTests adopted in cycle 2. Independent of AZ-491; can run in parallel but we keep sequential per implement-skill convention. diff --git a/_docs/03_implementation/reviews/batch_02_cycle3_review.md b/_docs/03_implementation/reviews/batch_02_cycle3_review.md new file mode 100644 index 0000000..51cc23e --- /dev/null +++ b/_docs/03_implementation/reviews/batch_02_cycle3_review.md @@ -0,0 +1,55 @@ +# Code Review Report — Batch 02 cycle 3 + +**Batch**: 02 (cycle 3) — AZ-491 (consolidate JWT test-mint helpers) +**Date**: 2026-05-12 +**Verdict**: PASS_WITH_WARNINGS + +## Findings + +| # | Severity | Category | File:Line | Title | +|---|----------|----------|-----------|-------| +| 1 | Low | Maintainability | `SatelliteProvider.TestSupport/SatelliteProvider.TestSupport.csproj:10-11` | Pinned `System.IdentityModel.Tokens.Jwt` 7.0.3 lives alongside the API's transitive `Microsoft.AspNetCore.Authentication.JwtBearer` 8.0.25 | + +### Finding Details + +**F1: Pinned `System.IdentityModel.Tokens.Jwt` 7.0.3 lives alongside the API's transitive `Microsoft.AspNetCore.Authentication.JwtBearer` 8.0.25** (Low / Maintainability) + +- Location: `SatelliteProvider.TestSupport/SatelliteProvider.TestSupport.csproj:10-11` +- Description: TestSupport's two pinned packages (`Microsoft.IdentityModel.Tokens` 7.0.3, `System.IdentityModel.Tokens.Jwt` 7.0.3) are the same versions that the integration-tests project previously pinned directly. These are *not* the same package family as the API's `Microsoft.AspNetCore.Authentication.JwtBearer` 8.0.25 — they belong to a different .NET versioning track. Token-minting logic in the test library and token-validation logic in the API consume related-but-separate `IdentityModel` types, so this is structurally fine, but a future agent reading the csproj could be confused about why the test library is one major version behind the API. The pre-AZ-491 IntegrationTests csproj had the same pinning; this batch preserved it for parity. +- Suggestion: Defer. The 7.0.x → 8.0.x bump for `System.IdentityModel.Tokens.Jwt` would be a separate hardening task — out of scope for AZ-491 (which is a refactor, not a dependency bump). If the team chooses to align minor versions, it should ship as a single coordinated bump across all `IdentityModel.*` references with regression coverage. For AZ-491 the existing pinning is preserved unchanged so behavior parity with cycle 2 is guaranteed. +- Task: AZ-491 + +## Phase-by-Phase Summary + +| Phase | Result | Notes | +|-------|--------|-------| +| 1. Context Loading | OK | AZ-491 spec read; 6 ACs identified; Option A chosen with documented rationale | +| 2. Spec Compliance | OK | 6/6 ACs verified at code level. AC-2 + AC-3 wait on Step 16 final test gate; their structural prerequisites (call-site signatures preserved; subject explicitly passed) are in place | +| 3. Code Quality | OK | New `JwtTokenFactory` is a stateless static class — SRP satisfied; eliminates duplication; regression-prevention comment on the `notBefore` branch is the only doc-comment kept (justified per `coderule.mdc` — references cycle-2 fix commits) | +| 4. Security Quick-Scan | OK | No production-code change; no new attack surface. Token-minting moved into a test-only library that production code MUST NOT consume — the `module-layout.md` entry encodes this invariant | +| 5. Performance Scan | OK (N/A) | Test-infrastructure refactor; no hot-path code | +| 6. Cross-Task Consistency | OK | Single task in batch; the new code-review rule introduced by this same batch will prevent future duplicate-helper regressions | +| 7. Architecture Compliance | OK | TestSupport is correctly placed in the Shared / Cross-Cutting layer of `module-layout.md`. It is referenced only by `Tests` + `IntegrationTests` (both via `ProjectReference`); zero production projects reference it. No cycle introduced. Layering: TestSupport sits *below* both test projects (foundation); both test projects sit *outside* the production layering table | + +## Cross-cutting observations (info, no finding) + +- The subject-value migration is the only semantic delta. Pre-AZ-491 the integration project's `MintValidToken(secret)` defaulted to `subject = "integration-tests"`. Post-AZ-491 the same call site now reads `JwtTokenFactory.Create(secret, JwtTestHelpers.DefaultSubject)` — `JwtTestHelpers.DefaultSubject = "integration-tests"` is still the canonical runner subject; the explicit-pass form preserves behavior exactly. Unit tests retain their own subject choices ("alice", "test-user", explicit per-test) unchanged. +- The `using System.IdentityModel.Tokens.Jwt;` and `using Microsoft.IdentityModel.Tokens;` imports were removed from `JwtTestHelpers.cs` because the file no longer uses those namespaces after the mint logic moved out. No dead-import remains. +- The `SatelliteProvider.Tests/TestUtilities/` folder is intentionally retained (not deleted) because it still owns `UavTileImageFactory.cs`. That fixture-factory is out of scope per AZ-491 § Excluded — a future task could consolidate it into TestSupport, but only if/when integration tests also need it. + +## Baseline Delta + +| Class | Count | Notes | +|-------|-------|-------| +| Carried over | 0 | No Architecture findings in baseline; nothing recurring | +| Resolved | 1 (informal) | The cycle-2 duplicate-helper Maintainability pattern (documented in `LESSONS.md` 2026-05-11 testing entry + retrospective Pattern 1) is structurally closed by this batch. Not an Architecture-class baseline entry, so it does not appear in the cycle-1 baseline; tracked here for the cycle retrospective | +| Newly introduced | 0 | — | + +## Verdict Logic + +- 0 Critical, 0 High, 0 Medium, 1 Low → **PASS_WITH_WARNINGS** +- The single Low finding is informational (pre-existing version pinning preserved verbatim from cycle-2 IntegrationTests csproj); does not block commit per the implement-skill auto-fix gate. + +## Recommendation to /implement + +Proceed to commit + push + tracker transition (Steps 11-13).