diff --git a/backend/src/BigRegister.Api/Program.cs b/backend/src/BigRegister.Api/Program.cs index fbadce2..614162e 100644 --- a/backend/src/BigRegister.Api/Program.cs +++ b/backend/src/BigRegister.Api/Program.cs @@ -506,6 +506,10 @@ api.MapPost("/beoordeling/{id}/besluit", (string id, RecordBesluitRequest req, H statusCode: StatusCodes.Status409Conflict); app.Logger.LogInformation("aanvraag besluit id={Id} besluit={Besluit}", a.Id, besluit); + // RB-07/BIO-007: the gate above records that a behandelaar was allowed to act; this + // records what they decided. Without it /beheer/audit cannot answer "who rejected this + // aanvraag", which is the question the trail exists for. + AuditAuthz(ctx, "aanvraag:besluit", $"aanvraag/{a.Id}/{besluit}", true, Authz.ResolvePrincipal(ctx)); // WP-60: the local decision above already committed — a ZGW failure here is caught and // flagged rather than allowed to diverge silently, same handling as submit's create-zaak @@ -598,8 +602,9 @@ api.MapGet("/flags", () => Results.Ok(FeatureFlagStore.All().Select(f => new FeatureFlagDto(f.Key, f.Description, f.Enabled)).ToList())) .Produces>(); -api.MapPut("/admin/flags/{key}", (string key, SetFeatureFlagRequest req, HttpContext ctx) => FlagsAdmin(ctx, () => - FeatureFlagStore.Set(key, req.Enabled) ? Results.NoContent() : Results.NotFound())) +api.MapPut("/admin/flags/{key}", (string key, SetFeatureFlagRequest req, HttpContext ctx) => + FlagsAdmin(ctx, $"feature-flags/{key}={req.Enabled}", () => + FeatureFlagStore.Set(key, req.Enabled) ? Results.NoContent() : Results.NotFound())) .Produces(StatusCodes.Status204NoContent) .Produces(StatusCodes.Status404NotFound) .ProducesProblem(StatusCodes.Status403Forbidden); @@ -629,7 +634,7 @@ api.MapPost("/brief/submit", (HttpContext ctx) => { var isDrafter = Authz.ResolvePrincipal(ctx).Role == PrincipalRole.Drafter; var r = BriefStore.Submit(ctx.Zorgverlener().Bsn, isDrafter, Now()); - LogBrief("submit", r); + LogBrief(ctx, "submit", r); return BriefResult(ctx, r, "Alleen de opsteller mag indienen."); }) .WithName("briefSubmit") // distinct name so the generated client method isn't `submit2` @@ -640,7 +645,7 @@ api.MapPost("/brief/submit", (HttpContext ctx) => api.MapPost("/brief/approve", (HttpContext ctx) => { var r = BriefStore.Approve(ctx.Zorgverlener().Bsn, Authz.ResolvePrincipal(ctx), Now()); - LogBrief("approve", r); + LogBrief(ctx, "approve", r); return BriefResult(ctx, r, "De beoordelaar mag niet de opsteller zijn."); }) .Produces() @@ -650,7 +655,7 @@ api.MapPost("/brief/approve", (HttpContext ctx) => api.MapPost("/brief/reject", (RejectBriefRequest req, HttpContext ctx) => { var r = BriefStore.Reject(ctx.Zorgverlener().Bsn, Authz.ResolvePrincipal(ctx), req.Comments, Now()); - LogBrief("reject", r); + LogBrief(ctx, "reject", r); return BriefResult(ctx, r, "De beoordelaar mag niet de opsteller zijn."); }) .Produces() @@ -663,7 +668,7 @@ api.MapPost("/brief/send", (HttpContext ctx) => // port); the backend only guards the approved→sent transition (not role-gated // today — see Authz.CanActOn(Send, …), a mechanical dispatch step). var r = BriefStore.Send(ctx.Zorgverlener().Bsn, Now()); - LogBrief("send", r); + LogBrief(ctx, "send", r); return BriefResult(ctx, r, "Versturen kan niet in deze status."); }) .Produces() @@ -785,14 +790,19 @@ 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). A denial -// is audited (PRD-0002 §8); the allow path is left un-logged (the endpoints log their -// own effect, e.g. publish). +// `orgtemplate:edit` capability RoleCapabilities emits (single Authz source). +// +// RB-07/BIO-007: every gate below audits the real decision, allow *and* deny. Auditing +// only denials left /beheer/audit able to answer "who was turned away" but not "who +// changed this", which for a register whose integrity is the product is the wrong half +// (PRD-0002 §8 lists approvals alongside denials). The allow row is written by the gate, +// not by the endpoint, so a new admin endpoint cannot be added that forgets it. IResult OrgAdmin(HttpContext ctx, Func action) { var principal = Authz.ResolvePrincipal(ctx); - if (Authz.CanManageOrgTemplates(principal)) return action(); - AuditAuthz(ctx, "orgtemplate:edit", "org-templates", false, principal); + var ok = Authz.CanManageOrgTemplates(principal); + AuditAuthz(ctx, "orgtemplate:edit", "org-templates", ok, principal); + if (ok) return action(); return Results.Problem(detail: "Alleen een beheerder mag organisatiesjablonen beheren.", statusCode: StatusCodes.Status403Forbidden); } @@ -802,8 +812,9 @@ IResult OrgAdmin(HttpContext ctx, Func action) IResult StamdataAdmin(HttpContext ctx, Func action) { var principal = Authz.ResolvePrincipal(ctx); - if (Authz.CanEditStamdata(principal)) return action(); - AuditAuthz(ctx, "stamdata:edit", "stamdata", false, principal); + var ok = Authz.CanEditStamdata(principal); + AuditAuthz(ctx, "stamdata:edit", "stamdata", ok, principal); + if (ok) return action(); return Results.Problem(detail: "Alleen een beheerder mag stamdata onderhouden.", statusCode: StatusCodes.Status403Forbidden); } @@ -813,8 +824,9 @@ IResult StamdataAdmin(HttpContext ctx, Func action) IResult CasesAdmin(HttpContext ctx, Func action) { var principal = Authz.ResolvePrincipal(ctx); - if (Authz.CanManageCases(principal)) return action(); - AuditAuthz(ctx, "cases:manage", "cases", false, principal); + var ok = Authz.CanManageCases(principal); + AuditAuthz(ctx, "cases:manage", "cases", ok, principal); + if (ok) return action(); return Results.Problem(detail: "Alleen een beheerder mag aanvragen beheren.", statusCode: StatusCodes.Status403Forbidden); } @@ -825,18 +837,23 @@ IResult CasesAdmin(HttpContext ctx, Func action) // zorgverlener with X-Role=admin still gets denied. `resource` feeds the denial's audit row. IResult Beoordelen(HttpContext ctx, string resource, Func action) { - if (Authz.CanBeoordelen(ctx.Caller())) return action(); - AuditAuthz(ctx, "aanvraag:beoordelen", resource, false, Authz.ResolvePrincipal(ctx)); + var ok = Authz.CanBeoordelen(ctx.Caller()); + AuditAuthz(ctx, "aanvraag:beoordelen", resource, ok, Authz.ResolvePrincipal(ctx)); + if (ok) return action(); return Results.Problem(detail: "Alleen een behandelaar mag aanvragen beoordelen.", statusCode: StatusCodes.Status403Forbidden); } -// One gate for the feature-flag toggle — the enforce twin of `flags:manage` (WP-47). -IResult FlagsAdmin(HttpContext ctx, Func action) +// One gate for the feature-flag toggle — the enforce twin of `flags:manage` (WP-47). Takes a +// per-call `resource` like Beoordelen does, because the toggle endpoint writes no log line of +// its own (BIO-007): a bare "feature-flags" row would say a flag changed without saying which, +// and this is the surface CQ-004/ADR-C-009 hinge on. +IResult FlagsAdmin(HttpContext ctx, string resource, Func action) { var principal = Authz.ResolvePrincipal(ctx); - if (Authz.CanManageFeatureFlags(principal)) return action(); - AuditAuthz(ctx, "flags:manage", "feature-flags", false, principal); + var ok = Authz.CanManageFeatureFlags(principal); + AuditAuthz(ctx, "flags:manage", resource, ok, principal); + if (ok) return action(); return Results.Problem(detail: "Alleen een beheerder mag functievlaggen beheren.", statusCode: StatusCodes.Status403Forbidden); } @@ -892,9 +909,16 @@ IResult BriefResult(HttpContext ctx, (BriefStore.Outcome outcome, BriefEntity? e _ => Results.Problem(detail: "Ongeldige overgang voor de huidige status van de brief.", statusCode: StatusCodes.Status409Conflict), }; -void LogBrief(string action, (BriefStore.Outcome outcome, BriefEntity? entity) r) => - app.Logger.LogInformation("brief {Action} outcome={Outcome} status={Status}", - action, r.outcome, r.entity?.Status.Tag ?? "-"); +// RB-07/BIO-007: every brief transition already funnelled through here for its log line, +// so the audit row goes here too — a fifth transition cannot be added that logs but leaves +// no trail. Resource is the bare "brief" (RB-02: never the owner's BSN); the decision is +// the transition's own outcome, so a 403 or a 409 is as visible as a success. +void LogBrief(HttpContext ctx, string action, (BriefStore.Outcome outcome, BriefEntity? entity) r) +{ + app.Logger.LogInformation("brief {Action} outcome={Outcome} status={Status}", + action, r.outcome, r.entity?.Status.Tag ?? "-"); + AuditAuthz(ctx, "brief:" + action, "brief", r.outcome == BriefStore.Outcome.Ok, Authz.ResolvePrincipal(ctx)); +} // Audit + outcome for a submit, with NO personal data: only kind, outcome, // generated reference and the caller's correlation id (the observability seam — a diff --git a/backend/tests/BigRegister.Tests/AuthzAuditTests.cs b/backend/tests/BigRegister.Tests/AuthzAuditTests.cs index 6a4848f..094f456 100644 --- a/backend/tests/BigRegister.Tests/AuthzAuditTests.cs +++ b/backend/tests/BigRegister.Tests/AuthzAuditTests.cs @@ -3,6 +3,7 @@ using System.Net.Http.Json; using System.Text.RegularExpressions; using BigRegister.Api.Contracts; using BigRegister.Api.Data; +using BigRegister.Domain.Features; using Microsoft.AspNetCore.Mvc.Testing; namespace BigRegister.Tests; @@ -43,6 +44,43 @@ public class AuthzAuditTests(TestWebApplicationFactory factory) : IClassFixture< Assert.Contains(await AuditLog(), e => e.Action == "brief:reveal-bignummer"); } + /// RB-07/BIO-007: the trail used to record only denials, so `/beheer/audit` could answer + /// "who was turned away" but not "who changed this" — for a register whose integrity is the + /// product, the wrong half. Every gate now audits the real decision. + [Fact] + public async Task An_allowed_admin_action_is_recorded() + { + (await _client.SendAsync(Admin(HttpMethod.Get, "/api/v1/admin/cases"))).EnsureSuccessStatusCode(); + Assert.Contains(await AuditLog(), e => e.Action == "cases:manage" && e.Decision == "allow" && e.Role == "Admin"); + } + + /// 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] + public async Task A_feature_flag_toggle_records_which_flag_changed() + { + var toggle = Admin(HttpMethod.Put, $"/api/v1/admin/flags/{FeatureFlags.InschrijvingOpen}"); + toggle.Content = JsonContent.Create(new { enabled = false }); + (await _client.SendAsync(toggle)).EnsureSuccessStatusCode(); + + Assert.Contains(await AuditLog(), e => + e.Action == "flags:manage" && e.Decision == "allow" && + e.Resource == $"feature-flags/{FeatureFlags.InschrijvingOpen}=False"); + } + + /// Every brief transition funnels through LogBrief, so all four are covered by the audit + /// call living there. The allow side is asserted in + /// BriefEndpointTests.Submit_succeeds_when_required_sections_filled, which already has + /// the fill-the-sections scaffolding; this is the refused side — a rejected transition must + /// leave a row rather than being dropped. + [Fact] + public async Task A_refused_brief_transition_is_recorded() + { + // No brief exists for this subject and nothing is filled in → illegal transition. + Assert.Equal(HttpStatusCode.Conflict, (await _client.PostAsync("/api/v1/brief/submit", null)).StatusCode); + Assert.Contains(await AuditLog(), e => e.Action == "brief:submit" && e.Decision == "deny"); + } + /// RB-02/BIO-008: the schema test below asserts on **column names**, so a BSN inside a /// column called `Resource` was invisible to it — and one was there, concatenated as /// `"brief/" + Bsn`. This asserts on the stored **values** instead. Four documents diff --git a/backend/tests/BigRegister.Tests/BeoordelingTests.cs b/backend/tests/BigRegister.Tests/BeoordelingTests.cs index 19c0291..f79ffbb 100644 --- a/backend/tests/BigRegister.Tests/BeoordelingTests.cs +++ b/backend/tests/BigRegister.Tests/BeoordelingTests.cs @@ -2,6 +2,7 @@ using System.Net; using System.Net.Http.Headers; using System.Net.Http.Json; using BigRegister.Api.Contracts; +using BigRegister.Api.Data; using Microsoft.AspNetCore.Mvc.Testing; namespace BigRegister.Tests; @@ -150,6 +151,12 @@ public class BeoordelingTests(TestWebApplicationFactory factory) : IClassFixture var view = (await detail.Content.ReadFromJsonAsync())!; Assert.Equal("Goedgekeurd", view.Aanvraag.Status.Tag); Assert.False(view.Decisions.CanBesluiten); // terminal — no further decision allowed + + // RB-07/BIO-007: the gate records that a behandelaar was allowed to act; this records + // what they decided, which is the question /beheer/audit exists to answer. + Assert.Contains(AuthzAuditStore.List(), e => + e.Action == "aanvraag:besluit" && e.Decision == "allow" && + e.Resource == $"aanvraag/{a.Id}/Goedkeuren"); } finally { diff --git a/backend/tests/BigRegister.Tests/BriefEndpointTests.cs b/backend/tests/BigRegister.Tests/BriefEndpointTests.cs index d38973c..22ec6ef 100644 --- a/backend/tests/BigRegister.Tests/BriefEndpointTests.cs +++ b/backend/tests/BigRegister.Tests/BriefEndpointTests.cs @@ -160,6 +160,11 @@ public class BriefEndpointTests(TestWebApplicationFactory factory) : IClassFixtu var submitted = await res.Content.ReadFromJsonAsync(); Assert.NotNull(submitted); Assert.Equal("submitted", submitted.Brief.Status.Tag); + + // RB-07/BIO-007: the allow side of the transition leaves a row, not just a log line. + // Resource is the bare "brief" — never the owner's BSN (RB-02). + Assert.Contains(AuthzAuditStore.List(), + e => e.Action == "brief:submit" && e.Decision == "allow" && e.Resource == "brief"); } [Fact] diff --git a/docs/project/refactor-backlog-setup/refactor-backlog/implementation/rb-07.md b/docs/project/refactor-backlog-setup/refactor-backlog/implementation/rb-07.md new file mode 100644 index 0000000..50e9e34 --- /dev/null +++ b/docs/project/refactor-backlog-setup/refactor-backlog/implementation/rb-07.md @@ -0,0 +1,63 @@ +# RB-07 — audit the allow path, not just the denial + +Status: **implemented** · 2026-08-27 · Source findings: `07-bio2-compliance.md` BIO-007 (+ the outstanding half of CQ-004) · `99-backlog.md` RB-07 + +## What was wrong + +All five authorization gates called `AuditAuthz(..., allowed: false, ...)` only on the deny +branch; the allow branch called `action()` and returned. So `/beheer/audit` — the queryable +trail the product ships as its audit surface — could answer "who was turned away" but never +"who changed this". + +Nothing recorded: `PUT /admin/flags/{key}`, `PUT /admin/org-template/{subOrgId}`, +`POST /admin/org-template/{subOrgId}/rollback/{version}`, `DELETE /admin/cases/{id}`, +`DELETE /admin/uploads/{documentId}`, `POST /brief/approve|reject|send`, and +`POST /beoordeling/{id}/besluit`. The comment above `OrgAdmin` claimed the endpoints logged +their own effect instead; publish and admin case delete do, the other six did not log at all. + +## What changed + +| File | Change | +| ------------------------- | ----------------------------------------------------------------------------- | +| `Program.cs` × 5 gates | `var ok = Authz.CanX(p); AuditAuthz(ctx, …, ok, p); if (ok) return action();` | +| `Program.cs` `FlagsAdmin` | takes a per-call `resource` (see below) | +| `Program.cs` `LogBrief` | takes `HttpContext`, writes the audit row alongside the log line | +| `Program.cs` besluit | one `aanvraag:besluit` row recording **what** was decided | +| `AuthzAuditTests.cs` | allow-path row; the flag key + value; a refused brief transition | +| `BriefEndpointTests.cs` | the allow side of `brief:submit` | +| `BeoordelingTests.cs` | the `aanvraag:besluit` row | + +**The row is written by the gate, not the endpoint.** That is the point: a new admin +endpoint cannot be added that forgets to audit itself. Same reasoning for the brief — every +transition already funnelled through `LogBrief` for its log line, so the audit call went +there too, which covers `submit`/`approve`/`reject`/`send` in one place and any fifth +transition automatically. The decision recorded is the transition's own outcome, so a 403 or +a 409 is as visible as a success. + +**`FlagsAdmin` gained a `resource` parameter** — the one deviation from BIO-007's minimal +remediation, and the reason is in the finding itself: the toggle endpoint writes no log line +of its own, so a constant `"feature-flags"` row would record that a flag changed without +recording _which_. It now writes `feature-flags/=`. One call site. +`OrgAdmin`/`CasesAdmin` keep their coarse refs because those endpoints do log the specific +object; **that asymmetry is deliberate, not an oversight.** + +**The besluit gets a second row.** The `Beoordelen` gate records that a behandelaar was +_allowed to act_; `aanvraag:besluit` records _what they decided_ +(`aanvraag//Goedkeuren`). Only the first would leave "who rejected this aanvraag" +unanswerable, which is the question the trail exists for. + +## Consequences worth knowing + +- **Row volume goes up.** `StamdataAdmin` gates read endpoints, so every admin page load now + writes rows. That is what "audit the allow path" means and BIO-007 asks for it explicitly; + if `AuthzAuditStore` ever needs retention or sampling, this is the change that made it + necessary. +- **This unblocks ADR-C-009.** Clause (4) of agent 06's four-part test is "writes are + admin-capability-gated **and** audited". Both surfaces now are, so the amendment can be + signed without ratifying a control the code does not implement. +- **CQ-004's outstanding half is closed.** `PUT /admin/flags/{key}` writes an audit row. + +## Verification + +`dotnet test`: **252 passed, 1 failed** — the pre-existing +`OpenZaakIntegrationTests.Admin_cases_…`, which needs a live container.