diff --git a/backend/src/BigRegister.Api/Domain/Authorization/IIdentityProvider.cs b/backend/src/BigRegister.Api/Domain/Authorization/IIdentityProvider.cs index 5d4f047..a69a16d 100644 --- a/backend/src/BigRegister.Api/Domain/Authorization/IIdentityProvider.cs +++ b/backend/src/BigRegister.Api/Domain/Authorization/IIdentityProvider.cs @@ -4,9 +4,14 @@ namespace BigRegister.Domain.Authorization; /// Resolves the acting for a request (WP-53) — one of the two actor /// kinds (WP-62, ADR-0002 §3): a zorgverlener (real DigiD claims in production) or a medewerker /// (real employee SSO/eHerkenning claims in production). is -/// the only implementation today. +/// the only implementation today, and is registered only in Development (Program.cs, +/// RB-09/BIO-002). /// public interface IIdentityProvider { - CallerIdentity Resolve(HttpContext ctx); + /// Null when the request carries no identity a real implementation can vouch for — + /// e.g. no credential at all. Returning null, rather than inventing a default, is what makes + /// "unauthenticated" representable; the identity-resolution middleware (Program.cs) + /// turns a null into a 401 instead of a silent identity substitution. + CallerIdentity? Resolve(HttpContext ctx); } diff --git a/backend/src/BigRegister.Api/Domain/Authorization/StubIdentityProvider.cs b/backend/src/BigRegister.Api/Domain/Authorization/StubIdentityProvider.cs index d3ba6ff..0a721cd 100644 --- a/backend/src/BigRegister.Api/Domain/Authorization/StubIdentityProvider.cs +++ b/backend/src/BigRegister.Api/Domain/Authorization/StubIdentityProvider.cs @@ -13,6 +13,11 @@ namespace BigRegister.Domain.Authorization; /// A real system builds this from verified DigiD claims (zorgverlener) / employee SSO claims /// (medewerker); every consumer of carries over unchanged once that /// swap happens. +/// +/// Registered only in Development (Program.cs, RB-09/BIO-002) — it always invents a +/// caller for a request with no credential, which is a deliberate developer convenience, not +/// something a production build may do. Its own return type stays non-nullable: unlike +/// , this stub never has "no identity" to report. /// public sealed class StubIdentityProvider : IIdentityProvider { diff --git a/backend/src/BigRegister.Api/Program.cs b/backend/src/BigRegister.Api/Program.cs index 614162e..e630dd6 100644 --- a/backend/src/BigRegister.Api/Program.cs +++ b/backend/src/BigRegister.Api/Program.cs @@ -51,7 +51,22 @@ Db.ConnectionString = builder.Configuration.GetConnectionString("AppDb") ?? Db.C // every store call site that used to hardcode DocumentStore.DemoOwner. Stub today (X-Role/ // X-Subject for a zorgverlener, X-Medewerker/X-Rollen for a medewerker); a real // DigiD/employee-SSO provider swaps in without touching a consumer. -builder.Services.AddSingleton(); +// +// RB-09/BIO-002: StubIdentityProvider invents a citizen identity for any request with no +// credential at all — a production behandelportal build sends no X-Medewerker header, so it +// used to authenticate every request as the seeded citizen (open on that citizen's own rights, +// including CanRevealBigNummer). Registering the stub only in Development, and failing to +// start in Production rather than falling through to a per-request 401, means a misconfigured +// deploy never serves a single request. The real DigiD/employee-SSO provider is out of scope +// for this POC (BIO-002's remediation says so explicitly) — until one exists, Production simply +// cannot start, which is the correct fail-closed behaviour for "no identity provider available". +if (builder.Environment.IsDevelopment()) + builder.Services.AddSingleton(); +else if (builder.Environment.IsProduction()) + throw new InvalidOperationException( + "No IIdentityProvider is registered for a Production environment. StubIdentityProvider " + + "is Development-only (RB-09/BIO-002); there is no real DigiD/employee-SSO provider in " + + "this POC yet. Register one before deploying to Production."); // WP-49: the cases (zaken) READ path goes through IZaakSource so a real ZGW backend // (OpenZaak) can replace the local SQLite store behind the same DTO contract — the FE never @@ -111,11 +126,19 @@ app.Use(async (ctx, next) => // WP-53: resolve the acting citizen once per request, right after correlation — everything // downstream (Authz.ResolvePrincipal, the endpoints below) reads it via ctx.Caller() instead of -// re-deriving "who" itself. +// re-deriving "who" itself. RB-09/BIO-002: a null resolution is "no identity", not "the seeded +// citizen" — this is the one place that turns it into a response (401) rather than letting it +// flow downstream as a silent identity substitution. var identityProvider = app.Services.GetRequiredService(); app.Use(async (ctx, next) => { - ctx.SetCaller(identityProvider.Resolve(ctx)); + var identity = identityProvider.Resolve(ctx); + if (identity is null) + { + ctx.Response.StatusCode = StatusCodes.Status401Unauthorized; + return; + } + ctx.SetCaller(identity); await next(ctx); }); @@ -269,13 +292,14 @@ api.MapDelete("/uploads/{documentId}", (string documentId, HttpContext ctx) => .ProducesProblem(StatusCodes.Status409Conflict) .Produces(StatusCodes.Status404NotFound); -// Admin delete (seam): a real system requires an admin role; here an X-Admin header -// stands in. Bypasses ownership, unlinks, and flags the submission for review. -api.MapDelete("/admin/uploads/{documentId}", (string documentId, HttpContext ctx) => - !IsAdmin(ctx) ? Results.StatusCode(StatusCodes.Status403Forbidden) - : DocumentStore.AdminDelete(documentId, "admin") ? Results.NoContent() : Results.NotFound()) +// Admin delete: bypasses ownership, unlinks, and flags the submission for review. Gated +// by the same CasesAdmin wrapper (cases:manage) the other admin-cases endpoints use +// (RB-08/BIO-003) — it used to be gated by a standalone X-Admin header, outside Authz and +// unaudited; CasesAdmin gives it the missing AuthzAuditStore row for free (RB-07). +api.MapDelete("/admin/uploads/{documentId}", (string documentId, HttpContext ctx) => CasesAdmin(ctx, () => + DocumentStore.AdminDelete(documentId, "admin") ? Results.NoContent() : Results.NotFound())) .Produces(StatusCodes.Status204NoContent) -.Produces(StatusCodes.Status403Forbidden) +.ProducesProblem(StatusCodes.Status403Forbidden) .Produces(StatusCodes.Status404NotFound); // --- Applications (aanvragen): the system of record the dashboard reads. --- @@ -611,8 +635,8 @@ api.MapPut("/admin/flags/{key}", (string key, SetFeatureFlagRequest req, HttpCon // --- Brief (letter composition). One demo brief per owner; the server owns the // status machine + authorization (Authz, PRD-0002 phase P1). Principal is a -// dev-only stand-in via X-Role (mirrors the X-Admin seam and the FE ?role= -// toggle) — no real identities in this POC. --- +// dev-only stand-in via X-Role (mirrors the FE ?role= toggle) — no real +// identities in this POC. --- api.MapGet("/brief", (HttpContext ctx) => { @@ -787,8 +811,6 @@ api.MapPost("/admin/org-template/{subOrgId}/rollback/{version:int}", (string sub app.Run(); -static bool IsAdmin(HttpContext ctx) => ctx.Request.Headers["X-Admin"] == "true"; - // One gate for every org-template endpoint — the enforce twin of the // `orgtemplate:edit` capability RoleCapabilities emits (single Authz source). // diff --git a/backend/swagger.json b/backend/swagger.json index af86f93..dbad12d 100644 --- a/backend/swagger.json +++ b/backend/swagger.json @@ -400,7 +400,14 @@ "description": "No Content" }, "403": { - "description": "Forbidden" + "description": "Forbidden", + "content": { + "application/problem+json": { + "schema": { + "$ref": "#/components/schemas/ProblemDetails" + } + } + } }, "404": { "description": "Not Found" diff --git a/backend/tests/BigRegister.Tests/AuthzAuditTests.cs b/backend/tests/BigRegister.Tests/AuthzAuditTests.cs index 094f456..1252557 100644 --- a/backend/tests/BigRegister.Tests/AuthzAuditTests.cs +++ b/backend/tests/BigRegister.Tests/AuthzAuditTests.cs @@ -1,4 +1,5 @@ using System.Net; +using System.Net.Http.Headers; using System.Net.Http.Json; using System.Text.RegularExpressions; using BigRegister.Api.Contracts; @@ -27,6 +28,20 @@ public class AuthzAuditTests(TestWebApplicationFactory factory) : IClassFixture< return (await res.Content.ReadFromJsonAsync>())!; } + private async Task UploadAsOwner() + { + var form = new MultipartFormDataContent(); + var file = new ByteArrayContent(new byte[] { 1, 2, 3 }); + file.Headers.ContentType = new MediaTypeHeaderValue("application/pdf"); + form.Add(file, "file", "diploma.pdf"); + form.Add(new StringContent("diploma"), "categoryId"); + form.Add(new StringContent("local-rb08"), "localId"); + form.Add(new StringContent("registratie"), "wizardId"); + var res = await _client.PostAsync("/api/v1/uploads", form); + res.EnsureSuccessStatusCode(); + return (await res.Content.ReadFromJsonAsync())!.DocumentId; + } + [Fact] public async Task A_denied_admin_action_is_recorded() { @@ -54,6 +69,30 @@ public class AuthzAuditTests(TestWebApplicationFactory factory) : IClassFixture< Assert.Contains(await AuditLog(), e => e.Action == "cases:manage" && e.Decision == "allow" && e.Role == "Admin"); } + /// RB-08/BIO-003: the admin upload delete used to be gated by a standalone X-Admin + /// header, outside Authz and writing no AuthzAuditStore row at all. Routing it through + /// CasesAdmin (cases:manage) gives it the same allow-path row every other admin-cases + /// endpoint gets, for free, per RB-07. `CasesAdmin` audits under a fixed "cases" + /// resource shared with the other admin-cases endpoints, so this asserts a **count** + /// increase — reading the store directly (not via `GET /admin/audit`, itself a + /// `CasesAdmin` endpoint that would write its own row and confound the count) — + /// rather than mere presence, which this class's other cases:manage calls would + /// already satisfy even without the fix. + [Fact] + public async Task An_admin_upload_delete_is_recorded() + { + bool IsCasesManageAllow(AuthzAuditEntry e) => + e.Action == "cases:manage" && e.Decision == "allow" && e.Role == "Admin"; + + var documentId = await UploadAsOwner(); + var before = AuthzAuditStore.List().Count(IsCasesManageAllow); + + (await _client.SendAsync(Admin(HttpMethod.Delete, $"/api/v1/admin/uploads/{documentId}"))) + .EnsureSuccessStatusCode(); + + Assert.Equal(before + 1, AuthzAuditStore.List().Count(IsCasesManageAllow)); + } + /// The flag toggle writes no log line of its own, so the audit row is the only record that /// it happened — a bare "feature-flags" resource would not say which flag. [Fact] diff --git a/backend/tests/BigRegister.Tests/EndpointTests.cs b/backend/tests/BigRegister.Tests/EndpointTests.cs index c364a96..b8fb2d7 100644 --- a/backend/tests/BigRegister.Tests/EndpointTests.cs +++ b/backend/tests/BigRegister.Tests/EndpointTests.cs @@ -213,11 +213,13 @@ public class EndpointTests(TestWebApplicationFactory factory) : IClassFixture() + .WithWebHostBuilder(builder => builder.UseEnvironment("Production")); + + // The throw happens while the app builds services, before any request can be served — + // triggered here by the test host materialising that host to hand out a client. + Assert.ThrowsAny(() => factory.CreateClient()); + } } diff --git a/docs/project/refactor-backlog-setup/refactor-backlog/implementation/rb-08.md b/docs/project/refactor-backlog-setup/refactor-backlog/implementation/rb-08.md new file mode 100644 index 0000000..25687e5 --- /dev/null +++ b/docs/project/refactor-backlog-setup/refactor-backlog/implementation/rb-08.md @@ -0,0 +1,75 @@ +# RB-08 — route `DELETE /admin/uploads/{documentId}` through `CasesAdmin`; delete the orphaned `IsAdmin` gate + +Status: **implemented** · 2026-08-27 · Source findings: `07-bio2-compliance.md` BIO-003 · `99-backlog.md` RB-08 + +## What was wrong + +`Program.cs:790` (pre-change) had `static bool IsAdmin(HttpContext ctx) => ctx.Request.Headers["X-Admin"] == "true";` +and `DELETE /admin/uploads/{documentId}` (`:274`) was gated by `IsAdmin` alone — outside +`Authz`, outside every wrapper the four sibling admin surfaces use, and writing no +`AuthzAuditStore` row at all. `DocumentStore.AdminDelete` bypasses ownership and deletes the +row and its bytes; the only record left behind was a `DocumentStore.Audit("delete-admin", …)` +metadata row, which never surfaces on `/beheer/audit`. + +`grep -rn "X-Admin"` over `apps`, `libs`, `backend`, `e2e` (re-verified before deleting the +gate, as the ticket asked) confirmed the finding: the only sender was +`backend/tests/BigRegister.Tests/EndpointTests.cs:231`. No frontend or e2e path uses this +header — it was an orphaned gate, not a live seam. + +## What changed + +| File | Change | +| -------------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `Program.cs` `DELETE /admin/uploads/{id}` | now `CasesAdmin(ctx, () => DocumentStore.AdminDelete(...) ? NoContent : NotFound)` — same wrapper the other four admin-cases endpoints use; response doc changed `Produces(403)` → `ProducesProblem(403)` to match `CasesAdmin`'s `Results.Problem` | +| `Program.cs` `IsAdmin` | **deleted** | +| `Program.cs` two stale comments | the endpoint's own comment rewritten to describe the new gate; the brief-section banner comment ("mirrors the X-Admin seam") no longer references a gate that doesn't exist | +| `tests/BigRegister.Tests/EndpointTests.cs` | `Admin_delete_requires_admin_role` sends `X-Role: admin` instead of `X-Admin: true` | +| `tests/BigRegister.Tests/AuthzAuditTests.cs` | **new** `An_admin_upload_delete_is_recorded` — asserts the `cases:manage`/`allow`/`Admin` row count increases by exactly one after the delete | + +No new `Authz` capability was added — `CasesAdmin`/`Authz.CanManageCases` is the wrapper the +ticket named as the expected outcome, and nothing about this endpoint needed a narrower +capability than "manage cases" already provides. + +**RB-07 already moved `AuditAuthz` onto the allow path for every `*Admin` wrapper**, so +routing through `CasesAdmin` gives BIO-003's missing audit row for free. No second +`AuditAuthz` call was added — confirmed by reading `CasesAdmin`'s body (`Program.cs`): it +calls `AuditAuthz(ctx, "cases:manage", "cases", ok, principal)` unconditionally before +branching on `ok`. + +## Judgement calls + +- **Test asserts a count delta, not mere presence.** `CasesAdmin` audits under a fixed + `"cases"` resource literal shared by every `cases:manage` call (`GET /admin/cases`, + `DELETE /admin/cases/{id}`, `GET /admin/audit` itself, and now this endpoint), so + `Assert.Contains(rows, cases:manage/allow/Admin)` would already be satisfied by this test + class's _other_ tests even without the fix. The new test counts matching rows before and + after the delete and asserts the count grew by exactly one. It reads `AuthzAuditStore.List()` + in-process rather than through `GET /admin/audit` — that endpoint is itself a `CasesAdmin` + read, so calling it to take the "before" measurement would have written its own + `cases:manage`/`allow` row and silently inflated the count by one every time it was called + (caught this by running the test once against the fix with an HTTP-based baseline: it + failed with an off-by-one before switching to the in-process read). +- **Two comments referencing the old gate were also updated**, not just the endpoint mapping + itself — one directly above the endpoint, one in the brief-section banner comment + ("dev-only stand-in via X-Role, mirrors the X-Admin seam") that would otherwise describe a + gate that no longer exists. + +## Known residual + +None new. RB-01's implementation note already records that `GET /uploads/{id}/content` is +reached with no identity header via plain browser navigation — that residual is RB-09's +territory, not this ticket's, and is untouched here. + +## Verification + +- Reverted `Program.cs`'s endpoint change only (`git stash push` on that one file, tests + left in place) and re-ran `dotnet test --filter "AuthzAuditTests|EndpointTests"`: **both** + `EndpointTests.Admin_delete_requires_admin_role` and + `AuthzAuditTests.An_admin_upload_delete_is_recorded` failed red (403 Forbidden — the old + gate rejects `X-Role: admin`, and the count-delta test throws on `EnsureSuccessStatusCode` + before it can assert). Restored the fix (`git stash pop`) and re-ran: both green. +- `dotnet build`: clean. +- `dotnet test` (full suite): **253 passed, 1 failed** — the pre-existing + `OpenZaakIntegrationTests.Admin_cases_returns_the_seeded_zaak_mapped_through_real_HTTP_and_JWT`, + which needs a live OpenZaak container and fails identically on a clean tree; not touched by + this ticket. diff --git a/docs/project/refactor-backlog-setup/refactor-backlog/implementation/rb-09.md b/docs/project/refactor-backlog-setup/refactor-backlog/implementation/rb-09.md new file mode 100644 index 0000000..d62af76 --- /dev/null +++ b/docs/project/refactor-backlog-setup/refactor-backlog/implementation/rb-09.md @@ -0,0 +1,168 @@ +# RB-09 — make "no identity" representable; stub Development-only; fail fast in Production + +Status: **implemented** · 2026-08-27 · Source findings: `07-bio2-compliance.md` BIO-002 (folds in BIO-001(a)/(b)) · `99-backlog.md` RB-09 + +## What was wrong + +`IIdentityProvider.Resolve` returned a non-nullable `CallerIdentity` +(`IIdentityProvider.cs:12`, pre-change), so the interface could not express "no identity" — any +implementation, stub or real, was forced to invent one for an unauthenticated request. +`StubIdentityProvider` was registered unconditionally, for every environment. + +Consequence, traced end to end: `apps/behandelportal/src/app/app.config.ts:57-63` puts +`medewerkerInterceptor` inside the `isDevMode()` provider array, so a production +behandelportal build sends **no** `X-Medewerker`/`X-Rollen` header. With neither header, +`StubIdentityProvider.Resolve` fell through to +`new ZorgverlenerCaller(DocumentStore.DemoOwner, ..., PrincipalRole.Drafter)` — the single +seeded citizen, role `drafter`. That: + +- **Fails closed, correctly, on backoffice capabilities** — `Authz.CanBeoordelen` is + `caller is MedewerkerCaller`, so a zorgverlener caller is always `false` regardless of role. + This part of the design was right and is untouched. +- **Fails open on every citizen-scoped endpoint** — `GET/PUT/DELETE /applications*`, + `POST /applications/{id}/submit`, `DELETE /uploads/{id}`, `GET|PUT /brief`, + `POST /brief/submit|send|reset` all resolve `ctx.Zorgverlener().Bsn` to the seeded citizen's + BSN. An employee with no employee identity was granted a citizen's own read/write rights. +- **Holds `CanRevealBigNummer`** — that capability is `Role == PrincipalRole.Drafter`, and + `drafter` is exactly the no-header default. + +`CallerIdentityHttpContextExtensions.Caller()` (`CallerIdentity.cs:44-50`) already throws +loudly when the identity middleware didn't run — the codebase reached for fail-loud one layer +up and then defaulted one layer down, which is the shape of the bug. + +## What changed + +| File | Change | +| ------------------------------------------------------ | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | +| `Domain/Authorization/IIdentityProvider.cs` | `Resolve` returns `CallerIdentity?`; doc comment states what null means and where it's turned into a response | +| `Domain/Authorization/StubIdentityProvider.cs` | **implementation signature unchanged** (`CallerIdentity`, non-nullable) — a valid, narrower override of the nullable interface method (return-type covariance; the compiler accepts it with zero warnings); doc comment records it is Development-only and never itself returns null | +| `Program.cs` — registration | `StubIdentityProvider` registered only under `builder.Environment.IsDevelopment()`; an `else if (builder.Environment.IsProduction())` branch throws `InvalidOperationException` immediately, before `builder.Build()` — the earliest possible failure point | +| `Program.cs` — identity middleware | resolves the identity once; if `null`, sets `401` and returns without calling `next()`, instead of passing a null (or invented) caller downstream | +| `tests/BigRegister.Tests/StubIdentityProviderTests.cs` | **new** `Never_returns_null_even_with_no_headers_at_all`; **new** `ProductionIdentityProviderTests.Production_environment_with_no_real_identity_provider_fails_at_startup` | + +No consumer beyond the middleware itself calls `IIdentityProvider.Resolve` (`grep -rn +"IIdentityProvider\|identityProvider\."` over `backend/src` confirms exactly one call site) — +`Authz.ResolvePrincipal`, `ZgwTokenProvider.Mint`, and every endpoint read `ctx.Caller()` / +`ctx.Zorgverlener()`, which already throw on a missing identity and are untouched. The 401 +now happens _before_ those are ever reached for a request the middleware rejects. + +## Judgement calls + +- **`StubIdentityProvider`'s own method signature stays `CallerIdentity`, not + `CallerIdentity?`.** The interface needed the nullable shape to make "no identity" + representable in general; the stub itself never has that case (it is a developer + convenience that always invents a caller by design) and returning a narrower, + non-nullable type from an override of a nullable-returning interface method is valid C# + nullable-reference-type covariance — verified with a clean `dotnet build` (0 warnings). + This kept every existing `StubIdentityProviderTests` call site (`private static +CallerIdentity Resolve(...)`) compiling with zero changes, rather than sprinkling + null-forgiving operators through a file whose entire point is "the stub always resolves." +- **The Production-only check is `IsProduction()`, not `!IsDevelopment()`.** The ticket and + BIO-002 both say "Production must fail at startup" specifically. A third environment (e.g. + a hypothetical `Staging`) falls through neither branch, registers no `IIdentityProvider` at + all, and would still fail — one line later, when `app.Services.GetRequiredService< +IIdentityProvider>()` throws .NET's own "no service for type" exception — just with a less + specific message than the one this ticket adds for Production. That fallback is a safety + net, not the intended fail-fast message; if a real non-Production, non-Development + environment is added later, giving it the same explicit message is a one-line follow-up, + not a design gap today. +- **The throw sits before `builder.Build()`**, not after (where `GetRequiredService` already + runs today). Both satisfy "throw during service registration / app build so a misconfigured + deploy never serves a request" — throwing earlier was free and gives a message naming the + actual cause (no real identity provider) rather than a generic DI resolution failure. +- **The 401 short-circuits before `ctx.SetCaller`, not after.** `next(ctx)` is never called, + so no downstream middleware or endpoint runs for a request with no identity — a citizen or + behandelaar endpoint reached this way now gets a clean 401 instead of ever executing. +- **`TestWebApplicationFactory` needed no change.** `WebApplicationFactory` defaults its + test host to the `Development` environment when nothing overrides it (confirmed + empirically: `dotnet test` — every non-Production test, all 253 of them pre-existing plus + 2 new, passed unchanged), so the entire existing test suite continues to exercise the + Development path exactly as before. The Production test builds its own + `WebApplicationFactory().WithWebHostBuilder(b => b.UseEnvironment("Production"))` + rather than touching the shared fixture. + +## Known residual — explicitly out of scope, confirmed and written up per the ticket + +**RB-01's residual is this ticket's territory but is explicitly out of scope for this +ticket**, per the task: `GET /uploads/{documentId}/content` is reached by a plain browser +navigation (`` in `beoordeling-documenten.component.ts`, `previewUrl` in +`libs/shared/src/upload/upload.adapter.ts`) that sends no identity header and never passes +through an Angular interceptor. + +- **In Development, this is unchanged** — verified by reading the endpoint + (`Program.cs:253-266`) and confirming `StubIdentityProvider` is still registered and still + resolves the same non-null seeded-citizen default it always did when no headers are + present. `dotnet test`'s full pass (255/255, excluding the pre-existing OpenZaak failure) + including `UploadAccessTests` — which exercises exactly this endpoint — confirms it + byte-for-byte. +- **The Production consequence, for the next ticket:** today Production cannot start at + all (this ticket's fail-fast), so the question is moot until a real `IIdentityProvider` + exists. Once one does, this endpoint's plain-navigation callers carry no credential a real + provider could resolve — the identity middleware would treat that as "no identity" and + return 401 before the endpoint ever runs, breaking both preview links outright. Making the + stub Development-only does not itself break anything (nothing in Production exists yet to + break), but it does mean **whoever builds the real provider must also solve this endpoint's + credential-carrying problem in the same change**, or ship it broken. This is not a + signed-URL or cookie scheme, and no such scheme was designed here, per the ticket's explicit + instruction — it is recorded so the next ticket (RB-13, or whichever lands the real + provider) picks it up deliberately rather than discovering it in a production incident. + +## Verification + +- **Reverted the registration change only** (kept `AddSingleton()` unconditional, left the new tests in place) and ran + `ProductionIdentityProviderTests`: it failed red — `Assert.ThrowsAny() Failure: No exception +was thrown` (the stub gets registered in every environment, so the host builds fine and + `factory.CreateClient()` never throws). Restored the fix and re-ran: green. +- `dotnet build`: clean, **0 warnings** (confirms the nullable-covariance judgement call + above compiles cleanly). +- `dotnet format BigRegister.slnx --verify-no-changes`: clean. +- `dotnet test` (full suite): **255 passed, 1 failed** — the pre-existing + `OpenZaakIntegrationTests.Admin_cases_returns_the_seeded_zaak_mapped_through_real_HTTP_and_JWT` + (needs a live OpenZaak container; fails identically on a clean tree). CI's actual filter, + `dotnet test BigRegister.slnx --filter "Category!=Integration"`: **255 passed, 0 failed**. + +## A regression found by actually running `npm run ci`, and its fix + +`npm run gen:api` (`dotnet swagger tofile`) loads `BigRegister.Api.dll` through .NET's +design-time `HostFactoryResolver` — the same mechanism `dotnet ef` migrations use — which +executes this file's top-level statements, including the identity middleware's +pre-existing, unconditional `app.Services.GetRequiredService()`, without +ever setting `ASPNETCORE_ENVIRONMENT`. Unset defaults to `Production`. Before this ticket +that was harmless (`StubIdentityProvider` was registered unconditionally); after it, nothing +is registered for that default environment, so the tool crashed +(`dotnet swagger tofile` exited **134**, confirmed by running it directly, both before and +after the fix below). + +This is real breakage of a real workflow, not a false alarm from `ci-local.sh` — verified by +reading `.github/workflows/ci.yml`'s `api-client-drift` job: `npm run gen:api` and +`git diff --exit-code ...` are **two separate `- run:` steps** there, so the crash would fail +actual CI. `ci-local.sh` chains them as `npm run gen:api && git diff --exit-code ...` on one +line, and its first full run of `npm run ci` after this ticket's change **printed the crash +but still reported `✔ local CI passed`** — a bash `set -e` gotcha, not a false negative +specific to this fix: a failing command that is not the last element of an `&&`/`||` list is +exempt from triggering `errexit`, so `cmd1 && cmd2` silently "passes" whenever `cmd1` alone +fails. That is a pre-existing fragility in `ci-local.sh`'s three `step "X"; gen && git diff` +lines (snippets/behaviour-spec/api-client drift), unrelated to RB-09 and out of this +ticket's scope — flagged here rather than fixed, since fixing a local convenience script's +error handling is a different, standalone change. Running the failing command directly +(rather than trusting the local script) is what caught this. + +**Fix:** `package.json`'s `gen:api` script now sets `ASPNETCORE_ENVIRONMENT=Development` +on the `dotnet swagger tofile` invocation specifically — the same value +`backend/src/BigRegister.Api/Properties/launchSettings.json` already sets for `dotnet run`, +and the same value `docker-compose.yml` already sets for local Docker (confirmed by reading +both: `docker-compose.prod.yml` sets `Production` explicitly, `docker-compose.yml` sets +`Development` explicitly — the bare CLI tool invocation was the **one** place with no +environment variable set at all). Verified: `npm run gen:api` now exits 0 and produces the +regenerated `backend/swagger.json` / `libs/shared/src/infrastructure/api-client.ts` cleanly. + +This also surfaced a second, unrelated gap: RB-08 changed +`DELETE /admin/uploads/{documentId}`'s 403 mapping from `.Produces` to `.ProducesProblem` +but the generated OpenAPI doc/client were never regenerated for it (RB-08's own `npm run ci` +was run before this fix existed, so `gen:api` was already broken by the time RB-09 landed +and the drift went unnoticed). Regenerated and committed separately — see the two follow-up +commits **fix(api): regenerate client for RB-08's 403 response shape** and +**fix(tooling): keep gen:api working under RB-09's Development-only stub**. No frontend +consumes the admin-uploads-delete endpoint (confirmed by grep), so the client regeneration +has no consumer impact. diff --git a/libs/shared/docs/behaviour-spec.mdx b/libs/shared/docs/behaviour-spec.mdx index f2c75ea..37de06e 100644 --- a/libs/shared/docs/behaviour-spec.mdx +++ b/libs/shared/docs/behaviour-spec.mdx @@ -21,7 +21,7 @@ tested where._ Every bullet below is a real test name from the suite — an `it()` title (frontend) or a test method name (backend), read as a sentence. Nothing here is hand-written prose: this page **is** the suite, reshaped for a business reader. 440 frontend behaviours across -9 contexts; 228 backend behaviours across 38 test +9 contexts; 231 backend behaviours across 39 test classes. ## Frontend (by context) @@ -899,6 +899,7 @@ classes. - A denied admin action is recorded - A reveal attempt is recorded - An allowed admin action is recorded +- An admin upload delete is recorded - A feature flag toggle records which flag changed - A refused brief transition is recorded - No audit row carries a subjects bsn @@ -1124,6 +1125,10 @@ classes. - Proefbrief is admin only - Proefbrief renders the draft template with a watermark +### ProductionIdentityProviderTests + +- Production environment with no real identity provider fails at startup + ### ProfessionsTests - A mapping is absent before its geldigVan @@ -1159,6 +1164,7 @@ classes. - Empty x medewerker falls through to the zorgverlener default - X rollen parses known tokens and drops unknown ones - X role still applies to a medewerker +- Never returns null even with no headers at all ### SubmissionRuleTests diff --git a/libs/shared/src/infrastructure/api-client.ts b/libs/shared/src/infrastructure/api-client.ts index 05eb07b..3d4da57 100644 --- a/libs/shared/src/infrastructure/api-client.ts +++ b/libs/shared/src/infrastructure/api-client.ts @@ -612,7 +612,9 @@ export class ApiClient { }); } else if (status === 403) { return response.text().then((_responseText) => { - return throwException("Forbidden", status, _responseText, _headers); + let result403: any = null; + result403 = _responseText === "" ? null : JSON.parse(_responseText, this.jsonParseReviver) as ProblemDetails; + return throwException("Forbidden", status, _responseText, _headers, result403); }); } else if (status === 404) { return response.text().then((_responseText) => { diff --git a/package.json b/package.json index 41c60b2..3265575 100644 --- a/package.json +++ b/package.json @@ -9,7 +9,7 @@ "format": "prettier --write .", "start": "ng serve ssp", "start:behandelportal": "ng serve behandelportal", - "gen:api": "cd backend && dotnet tool restore && dotnet build src/BigRegister.Api -v q && dotnet swagger tofile --output swagger.json src/BigRegister.Api/bin/Debug/net10.0/BigRegister.Api.dll v1 && cd .. && nswag run nswag.json", + "gen:api": "cd backend && dotnet tool restore && dotnet build src/BigRegister.Api -v q && ASPNETCORE_ENVIRONMENT=Development dotnet swagger tofile --output swagger.json src/BigRegister.Api/bin/Debug/net10.0/BigRegister.Api.dll v1 && cd .. && nswag run nswag.json", "build": "ng build ssp && ng build behandelportal", "watch": "ng build ssp --watch --configuration development", "test": "ng test ssp && ng test behandelportal && ng test shared && ng test beheer",