test(backend): assert every route is authz-gated (RB-12)
BL-006: the backend has zero automated architecture enforcement.
BIO-016 names the concrete consequence for authorization — nothing
asserted the *set* of gated endpoints, so BIO-003's X-Admin gate
(outside Authz) and BIO-004's two ungated endpoints were caught only
by a human reading Program.cs, not by CI.
Adds RouteInventoryTests: walks the real app's EndpointDataSource and
asserts every mapped route either carries a .Gate("XAdmin") metadata
marker (added at the 16 call sites that already call one of the five
admin wrappers — OrgAdmin/StamdataAdmin/CasesAdmin/Beoordelen/
FlagsAdmin) or appears in a written-down, reasoned allow-list. Proved
it's hard to fool by adding a throwaway unguarded route, watching the
test go red, and reverting.
The allow-list is not "public routes" as the ticket's shorthand put
it — 19 of its 31 entries are ownership-scoped inline (ctx.Zorgverlener()/
ctx.Caller()) endpoints, not public ones, and labelling them public
would misrepresent the exact property BIO-004 was about. Each entry
instead carries its own reason. Implementation note has the full
route-by-route breakdown and judgement calls.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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).
|
||||
Reference in New Issue
Block a user