Merge RB-12 + RB-15 + RB-16 — route-table authz gate, Swagger dev-only, peildatum 400
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> # Conflicts: # libs/shared/docs/behaviour-spec.mdx
This commit is contained in:
@@ -0,0 +1,115 @@
|
||||
# RB-12 — a route-table test: every route hits an authz wrapper or an explicit allow-list
|
||||
|
||||
Status: **implemented** · 2026-08-27 · Source findings: `07-bio2-compliance.md` BIO-016, `00-baseline.md` BL-006 · `99-backlog.md` RB-12
|
||||
|
||||
## What was wrong
|
||||
|
||||
BL-006, verbatim: "the backend has zero automated architecture enforcement … `Domain/`
|
||||
purity currently holds by convention." BIO-016 names the specific consequence for
|
||||
authorization: nothing asserted the **set** of gated endpoints, so an endpoint added
|
||||
without a gate (BIO-003's `X-Admin` gate outside `Authz`, BIO-004's two endpoints with
|
||||
no gate at all) failed no test. Both were caught by a human reading `Program.cs`, not by
|
||||
CI.
|
||||
|
||||
## What changed
|
||||
|
||||
| File | Change |
|
||||
| ------------------------------------------------ | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
|
||||
| `Program.cs` — 16 endpoint mappings | each chains a new `.Gate("XAdmin")` call, naming the admin wrapper (`OrgAdmin`, `StamdataAdmin`, `CasesAdmin`, `Beoordelen`, `FlagsAdmin`) already used inside its handler |
|
||||
| `Program.cs` — new types, end of file | `public sealed record AuthzGateMetadata(string Wrapper)` + a `Gate(...)` extension method on `IEndpointConventionBuilder` that attaches it via `.WithMetadata(...)` |
|
||||
| `tests/BigRegister.Tests/RouteInventoryTests.cs` | **new** — walks the real app's `EndpointDataSource`, asserts every route carries either an `AuthzGateMetadata` naming a known wrapper, or an entry in a written-down allow-list; a second test asserts every `.Gate(...)` name is one of the five known wrappers |
|
||||
|
||||
## Design: metadata at mapping time, not reflection over the compiled lambda
|
||||
|
||||
The ticket left the detection mechanism open, noting the wrappers are local functions
|
||||
in `Program.cs`. Reflecting over a compiled minimal-API lambda to determine which local
|
||||
function its closure calls is fragile-to-impossible (the call is inside IL a test would
|
||||
have to disassemble, and a local function's identity isn't easily recoverable from the
|
||||
delegate's `MethodInfo`). Endpoint **metadata**, attached at the same call site where the
|
||||
route is mapped, is exactly what `EndpointDataSource` hands back to a test host and
|
||||
doesn't depend on inspecting compiled code at all — so a `.Gate("XAdmin")` extension
|
||||
method was added and chained onto each of the 16 mappings that call one of the five
|
||||
wrappers.
|
||||
|
||||
This is a **declaration**, not a **derivation**: the test does not verify that
|
||||
`.Gate("CasesAdmin")` and an actual `CasesAdmin(ctx, …)` call inside the handler agree —
|
||||
it only verifies that a marker is present. A handler that swapped its `CasesAdmin(ctx,
|
||||
…)` call for a no-op without updating `.Gate(...)` would go undetected here. What _is_
|
||||
caught, reliably, is the actual BIO-003/BIO-004 failure mode: a new endpoint mapped with
|
||||
**no** marker and **no** allow-list entry — verified below by adding one and watching the
|
||||
test go red.
|
||||
|
||||
## Judgement call: the allow-list is not "public routes"
|
||||
|
||||
The ticket's literal framing — every route "goes through one of the authz wrappers …
|
||||
or appears in an explicit, named allow-list of deliberately-public routes" — doesn't fit
|
||||
this codebase as read. Only 16 of the app's 47 routes go through one of the five admin
|
||||
wrappers. The other 31 are not uniformly public:
|
||||
|
||||
- **10 are genuinely public** — orchestrator health probes and static/reference demo
|
||||
data (`SeedData`, the DUO/BRP fixtures, the scholing-threshold config value, the
|
||||
feature-flag catalog, `/me`'s reflection of the caller's own capabilities) that reads
|
||||
the same for every caller in this one-seeded-citizen POC.
|
||||
- **19 are ownership-scoped inline**, not public and not wrapper-gated: `GET
|
||||
/applications/{id}`, the upload endpoints, every brief transition, etc. all key off
|
||||
`ctx.Zorgverlener().Bsn` / `ctx.Caller()` — an authenticated citizen (or, for the
|
||||
uploads-content endpoint, a behandelaar) reading or writing only their own resource.
|
||||
Calling these "public" in an allow-list would misrepresent exactly the property
|
||||
BIO-004 was about — object-level authorization existing at all.
|
||||
- **1 (`POST /zgw/notificaties`) uses a different mechanism entirely** — a fixed-time
|
||||
shared-secret comparison for a non-Principal external caller (OpenZaak's
|
||||
notifications), audited the same way but never going through `Authz`.
|
||||
- **1 (`POST /brief/reset`) is deliberately, literally unguarded** — the endpoint's own
|
||||
pre-existing comment says so ("No guards — showcase affordance only").
|
||||
|
||||
The allow-list (`RouteInventoryTests.AllowList`) keeps all 31 as one array for the test's
|
||||
sake, but every entry carries its own reason string rather than a blanket "public" label —
|
||||
preserving BIO-016's actual intent ("makes 'this endpoint is public' a decision someone
|
||||
wrote down rather than an omission") generalised to "this endpoint's access boundary is
|
||||
_X_, deliberately," which is true of all 31 and false of "public" for 20 of them. This is
|
||||
recorded here rather than silently reinterpreted, per this task's brief: implementing the
|
||||
literal "public" framing would have been actively misleading about which endpoints have no
|
||||
access control at all.
|
||||
|
||||
## Other judgement calls
|
||||
|
||||
- **`AuthzGateMetadata` and its extension method are `public`, not `internal`.** The test
|
||||
project has no `InternalsVisibleTo` wired up for `BigRegister.Api` (checked — none
|
||||
exists anywhere in `backend/`), and adding one for a single marker type was more
|
||||
machinery than the alternative. Both types carry a comment stating why.
|
||||
- **A second test (`Every_gate_marker_names_a_known_admin_wrapper`) guards against a typo
|
||||
in a `.Gate(...)` call.** Without it, a call like `.Gate("CasesAdmn")` would just fall
|
||||
through to "unaccounted for" in the main test with a less specific failure message —
|
||||
fine, but a dedicated assertion names the actual mistake.
|
||||
- **The main test also asserts the reverse direction: no stale allow-list entries.** An
|
||||
allow-list entry for a route that was renamed or removed is exactly the kind of drift
|
||||
a "decision someone wrote down" ledger needs to catch, not just silently keep. Verified
|
||||
this fires: temporarily added one extra `AllowList` entry for a route that doesn't
|
||||
exist (via Edit, not committed) — every real route was still covered, so only the
|
||||
stale-entry assertion tripped, naming exactly that bogus entry. Reverted the same way.
|
||||
- **`RouteInventoryTests` uses the house `TestWebApplicationFactory` + `IClassFixture`
|
||||
idiom**, not a bare `new WebApplicationFactory<Program>()` per test. The first draft did
|
||||
the latter and immediately hit `SQLite Error 1: 'table "Applications" already exists'`
|
||||
— `Db.ConnectionString` (`Data/Db.cs`) is a shared mutable **static** field, and a bare
|
||||
factory doesn't override `ConnectionStrings:AppDb`, so two such factories in the same
|
||||
class end up pointed at the same file, and the second one's `Migrate()` collides with
|
||||
the first's already-created tables (the first factory's default `Dispose()` doesn't
|
||||
delete that file — only `TestWebApplicationFactory`'s override does, to its own
|
||||
per-instance temp path). This is exactly the hazard `TestWebApplicationFactory`'s own
|
||||
doc comment describes; switching to it (as every other endpoint-test class in this
|
||||
suite already does) fixed it outright — no product code involved, purely a test-fixture
|
||||
choice.
|
||||
|
||||
## Verification
|
||||
|
||||
- **Proved the test is hard to fool**, per the ticket's explicit ask: added a throwaway
|
||||
`api.MapDelete("/rb12-throwaway-unguarded/{id}", …)` with no `.Gate(...)` and no
|
||||
allow-list entry (via Edit, not `git checkout`) — `Every_mapped_route_is_authz_gated_or_
|
||||
on_the_named_allow_list` failed red, naming exactly that route. Reverted the same way;
|
||||
re-ran green. Repeated once more after switching to `TestWebApplicationFactory` to
|
||||
confirm the fixture change didn't weaken the check — same red, same green.
|
||||
- `dotnet build` (both `src/BigRegister.Api` and `tests/BigRegister.Tests`): clean, 0
|
||||
warnings.
|
||||
- `dotnet format BigRegister.slnx --verify-no-changes`: clean.
|
||||
- `dotnet test --filter "Category!=Integration"`: **257 passed, 0 failed** (255
|
||||
pre-existing + 2 new).
|
||||
@@ -0,0 +1,93 @@
|
||||
# RB-15 — Swagger and the OpenAPI document behind `IsDevelopment()`
|
||||
|
||||
Status: **implemented** · 2026-08-27 · Source findings: `07-bio2-compliance.md` BIO-015 · `99-backlog.md` RB-15
|
||||
|
||||
## What was wrong
|
||||
|
||||
`Program.cs:145-146` (pre-change) ran `app.UseSwagger(); app.UseSwaggerUI();`
|
||||
unconditionally — the full OpenAPI document (every route, every request/response shape)
|
||||
and SwaggerUI's interactive "Try it out" were reachable in every environment, including a
|
||||
real deployment, with no `app.Environment.IsDevelopment()` guard. BIO-015's own evidence
|
||||
notes this is one line of genuine attack-surface reduction with no POC cost.
|
||||
|
||||
## What changed
|
||||
|
||||
| File | Change |
|
||||
| --------------------------------------------- | -------------------------------------------------------------------------------------------------------- |
|
||||
| `Program.cs` | `app.UseSwagger(); app.UseSwaggerUI();` now run only inside `if (app.Environment.IsDevelopment()) { … }` |
|
||||
| `tests/BigRegister.Tests/SwaggerGateTests.cs` | **new** — asserts `/swagger/v1/swagger.json` is served in Development and 404s outside it |
|
||||
|
||||
`builder.Services.AddSwaggerGen(...)` and `AddEndpointsApiExplorer()` were left
|
||||
unconditional — they only register DI services (the swagger-generation machinery),
|
||||
expose nothing over HTTP by themselves, and (see below) are exactly what `npm run
|
||||
gen:api` depends on staying registered in every environment it might run against.
|
||||
|
||||
## The hazard, checked rather than assumed
|
||||
|
||||
RB-09 made a non-Development environment throw during `builder.Build()` (no
|
||||
`IIdentityProvider` registered for a bare/unset environment, which defaults to
|
||||
Production), which crashed `dotnet swagger tofile` until `package.json`'s `gen:api`
|
||||
script was pinned to `ASPNETCORE_ENVIRONMENT=Development` for that one invocation
|
||||
(`docs/.../implementation/rb-09.md`). This ticket's change sits in exactly the same
|
||||
pipeline, so it needed the same empirical check, not an assumption.
|
||||
|
||||
**Mechanism, confirmed by reading Swashbuckle's CLI behaviour and then proving it:**
|
||||
`dotnet swagger tofile` (`Swashbuckle.AspNetCore.Cli`) loads the built DLL through
|
||||
.NET's design-time `HostFactoryResolver`, builds the host, and then resolves
|
||||
`ISwaggerProvider` **directly out of the DI container** to produce `swagger.json` — it
|
||||
never issues an HTTP request through the ASP.NET Core middleware pipeline this ticket's
|
||||
`if (app.Environment.IsDevelopment())` guard lives in. Gating `UseSwagger()`/
|
||||
`UseSwaggerUI()` therefore cannot affect it, in any environment, by construction — those
|
||||
are pipeline middleware; the CLI tool bypasses the pipeline entirely.
|
||||
|
||||
**Verified, not assumed:** ran `npm run gen:api` for real. It exited 0, printed "Swagger
|
||||
JSON/YAML successfully written to …/backend/swagger.json", and regenerated the NSwag
|
||||
client. `git status`/`git diff` on both `backend/swagger.json` and
|
||||
`libs/shared/src/infrastructure/api-client.ts` showed **zero changes** — the regenerated
|
||||
files are byte-identical to what's already committed, confirming the gate has no effect
|
||||
on the generated contract at all.
|
||||
|
||||
## Judgement calls
|
||||
|
||||
- **The guard wraps both `UseSwagger()` and `UseSwaggerUI()` together**, not just one —
|
||||
the ticket's own wording lists both, and gating only the document while leaving the UI
|
||||
reachable (or vice versa) would be a strange half-measure: SwaggerUI without the
|
||||
document 404s on load anyway, and the document without the UI still leaks the same
|
||||
route/shape enumeration BIO-015 is about.
|
||||
- **`AddSwaggerGen`/`AddEndpointsApiExplorer` were left unconditional.** They're
|
||||
DI-registration-time calls with no HTTP surface, and — now confirmed rather than
|
||||
assumed — `dotnet swagger tofile` needs `ISwaggerProvider` registered in whatever
|
||||
environment it runs the host under (pinned to Development by `gen:api`'s own script,
|
||||
but nothing stops a future non-Development invocation), so conditioning those
|
||||
registrations on `IsDevelopment()` would risk breaking the CLI tool for no
|
||||
attack-surface benefit — nobody can reach a DI-registered-but-never-routed service
|
||||
over HTTP.
|
||||
- **New tests build the "non-Development" case on a third environment name
|
||||
("Staging"), not `"Production"`.** RB-09 already made Production fail at startup
|
||||
entirely (no real `IIdentityProvider` exists yet) — a stronger guarantee than "no
|
||||
Swagger in Production," but one that means a `UseEnvironment("Production")` host
|
||||
never reaches this middleware to prove the gate itself works; it only proves RB-09's
|
||||
unrelated startup throw, which already has its own test. A `"Staging"` environment
|
||||
satisfies neither `IsDevelopment()` nor `IsProduction()`, so `Program.cs` registers no
|
||||
`IIdentityProvider` for it — the test supplies one via `ConfigureTestServices`
|
||||
(`StubIdentityProvider`, the same one Development uses) so the host actually boots,
|
||||
and the test exercises this ticket's real gate rather than a different ticket's.
|
||||
- **The Staging host is built via `factory.WithWebHostBuilder(...)`** (layering on the
|
||||
shared `TestWebApplicationFactory` fixture), not a bare `new
|
||||
WebApplicationFactory<Program>()` — RB-12's implementation note already records the
|
||||
"table already exists" collision a bare factory hits by sharing the mutable static
|
||||
`Db.ConnectionString` instead of the fixture's own per-class isolated temp path;
|
||||
layering avoids repeating that mistake here.
|
||||
|
||||
## Verification
|
||||
|
||||
- **Reverted the guard only** (unwrapped `UseSwagger()`/`UseSwaggerUI()` back to
|
||||
unconditional, via Edit, tests left in place) and ran `SwaggerGateTests`:
|
||||
`Swagger_document_is_not_served_outside_development` failed red (`Expected: NotFound,
|
||||
Actual: OK`). Restored the fix (via Edit) and re-ran: both green.
|
||||
- **`npm run gen:api`, run for real**: exit 0; `backend/swagger.json` and
|
||||
`libs/shared/src/infrastructure/api-client.ts` both unchanged (`git status` clean on
|
||||
both) — see "The hazard" above.
|
||||
- `dotnet build` (both projects): clean, 0 warnings.
|
||||
- `dotnet format BigRegister.slnx --verify-no-changes`: clean.
|
||||
- `dotnet test --filter "Category!=Integration"`: **259 passed, 0 failed** (257 + 2 new).
|
||||
@@ -0,0 +1,82 @@
|
||||
# RB-16 — `DateOnly.TryParse` on `?peildatum=`
|
||||
|
||||
Status: **implemented** · 2026-08-27 · Source findings: `07-bio2-compliance.md` BIO-019 · `99-backlog.md` RB-16
|
||||
|
||||
## What was wrong
|
||||
|
||||
`Program.cs:215` (pre-change) —
|
||||
`var rows = peildatum is { Length: > 0 } p ? t.RowsOn(DateOnly.Parse(p)) : t.Rows();`.
|
||||
`DateOnly.Parse` throws `FormatException` on anything unparseable; there was no
|
||||
`TryParse`, no 400 path, and `.Produces` on the endpoint declared only 200/403/404 — so
|
||||
an unparseable `?peildatum=` value 500'd, and in Development the exception detail was
|
||||
returned to the caller. §3c's baseline named `backend/Stamdata` 96.8% line but **71.7%
|
||||
branch** (BL-005) — this was one of the unentered branches.
|
||||
|
||||
## What changed
|
||||
|
||||
| File | Change |
|
||||
| ---------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
|
||||
| `Program.cs` — `GET /stamdata/{table}` | `DateOnly.Parse` replaced with `DateOnly.TryParse`; an unparseable value now returns `Results.Problem(detail: …, statusCode: 400)` instead of throwing; endpoint doc gained `.ProducesProblem(StatusCodes.Status400BadRequest)` |
|
||||
| `tests/BigRegister.Tests/StamdataEndpointTests.cs` | **new** `Unparseable_peildatum_is_400_not_500` |
|
||||
| `backend/swagger.json`, `libs/shared/src/infrastructure/api-client.ts` | regenerated (`npm run gen:api`) — the new 400 response is now part of the documented contract |
|
||||
|
||||
## What the fix looks like
|
||||
|
||||
```csharp
|
||||
DateOnly? peildatumWaarde = null;
|
||||
if (peildatum is { Length: > 0 } p)
|
||||
{
|
||||
if (!DateOnly.TryParse(p, out var parsed))
|
||||
return Results.Problem(detail: $"Ongeldige peildatum '{p}'.", statusCode: StatusCodes.Status400BadRequest);
|
||||
peildatumWaarde = parsed;
|
||||
}
|
||||
var rows = peildatumWaarde is { } d ? t.RowsOn(d) : t.Rows();
|
||||
```
|
||||
|
||||
Matches the shape every other bad-input check in this file already uses (e.g. the
|
||||
upload endpoint's `Results.Problem(detail: …, statusCode: 400)` for a malformed
|
||||
multipart request) — a `Results.Problem` with a Dutch detail message, not a bespoke
|
||||
response shape.
|
||||
|
||||
## Judgement calls
|
||||
|
||||
- **No FE change needed, and none made.** `libs/beheer/src/infrastructure/
|
||||
stamdata.adapter.ts`'s `load()` already routes every call through `runSubmit`
|
||||
(`libs/shared/src/application/submit.ts`), which try/catches any thrown
|
||||
`ApiException` — including the client's new 400 branch — into a generic `Result`
|
||||
error string via `problemDetail`. There is no status-code-specific branching to
|
||||
extend; ADR-0001's "the FE renders the decision, it does not recompute the rule"
|
||||
already covers "the server rejected this input" as a case the generic error path
|
||||
handles, same as the existing 403.
|
||||
- **Regenerated the API client and committed it in this ticket's diff**, rather than
|
||||
leaving it to drift. RB-09's implementation note records a real prior incident where
|
||||
a response-shape change (RB-08's 403 → `ProducesProblem`) landed without a
|
||||
regeneration and the drift went unnoticed until the next ticket's `gen:api` run. This
|
||||
ticket's `.ProducesProblem(400)` is exactly that same category of change, so
|
||||
`npm run gen:api` was run immediately as part of implementing it, not deferred.
|
||||
- **The Dutch detail message follows the file's own convention** (`$"Ongeldige
|
||||
peildatum '{p}'."`) rather than English — every other `Results.Problem(detail: …)`
|
||||
call in `Program.cs` (change-request rejection, upload validation, submit rejection)
|
||||
is Dutch; this is server-internal wire text, not `$localize`-wrapped UI copy (the FE
|
||||
never renders it verbatim — CLAUDE.md's `$localize` rule is about user-facing copy
|
||||
the FE owns, not backend `ProblemDetails.detail` strings), so no locale entry was
|
||||
needed.
|
||||
|
||||
## Verification
|
||||
|
||||
- **Reverted the fix only** (`DateOnly.Parse` restored, ternary un-nested, via Edit —
|
||||
test left in place) and ran `StamdataEndpointTests`:
|
||||
`Unparseable_peildatum_is_400_not_500` failed red (`Expected: BadRequest, Actual:
|
||||
InternalServerError` — confirming the endpoint really did 500, not some other status).
|
||||
Restored the fix (via Edit) and re-ran: all 6 tests in the class green.
|
||||
- `dotnet build`: clean, 0 warnings.
|
||||
- `dotnet format BigRegister.slnx --verify-no-changes`: clean.
|
||||
- `dotnet test --filter "Category!=Integration"`: **260 passed, 0 failed** (259 + 1
|
||||
new).
|
||||
- `npm run gen:api`: exit 0; `backend/swagger.json` gained the 400 response shape for
|
||||
this one endpoint; `libs/shared/src/infrastructure/api-client.ts` gained the
|
||||
matching `status === 400` branch. Both regenerated files committed alongside the
|
||||
code change.
|
||||
- `npm test` (all four Vitest projects — ssp/behandelportal/shared/beheer): **445
|
||||
passed, 0 failed**, confirming the regenerated client doesn't break any existing FE
|
||||
consumer of `stamdataTable(...)`.
|
||||
Reference in New Issue
Block a user