diff --git a/backend/src/BigRegister.Api/Program.cs b/backend/src/BigRegister.Api/Program.cs index 614162e..270eacd 100644 --- a/backend/src/BigRegister.Api/Program.cs +++ b/backend/src/BigRegister.Api/Program.cs @@ -269,13 +269,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 +612,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 +788,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/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 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.