Merge RB-08 + RB-09 — CasesAdmin on the admin upload delete; no-identity representable
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> # Conflicts: # libs/shared/docs/behaviour-spec.mdx
This commit is contained in:
@@ -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.
|
||||
@@ -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<T>` 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<Program>().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 (`<a href>` 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<IIdentityProvider,
|
||||
StubIdentityProvider>()` 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<IIdentityProvider>()`, 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.
|
||||
Reference in New Issue
Block a user