From fbd27ed641e3ecda38869e78aa09cfa84e6a3d5c Mon Sep 17 00:00:00 2001 From: Edwin van den Houdt Date: Thu, 27 Aug 2026 10:54:07 +0200 Subject: [PATCH] fix(privacy): mask the BSN recorded as the document audit Actor (RB-04) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit DocumentStore wrote one audit row per upload and per user delete carrying the acting citizen's raw BSN as AuditEntry.Actor, persisted to SQLite — on a store whose own doc comment says it holds metadata only, never file content "or other PII". Same shape as RB-02, in a second store. Masked at the two citizen call sites rather than inside Audit, because the third actor is the literal "admin" and MaskTail("admin", 3) is "**min"; masking centrally would mean guessing which actors are BSNs and which are role names. Audit's doc comment now states that actors arrive redacted. StoredDocument.Owner is untouched: it is the authorization key that DeleteOwned, ForeignIds and RB-01's content check all compare against, so the BSN stays where it is load-bearing and leaves the trail where it was only decoration. No endpoint exposes AuditLog, so no response shape changes. Co-Authored-By: Claude Opus 5 --- .../src/BigRegister.Api/Data/DocumentStore.cs | 12 +++++- .../BigRegister.Tests/UploadAccessTests.cs | 22 +++++++++-- .../refactor-backlog/implementation/rb-04.md | 37 +++++++++++++++++++ 3 files changed, 66 insertions(+), 5 deletions(-) create mode 100644 docs/project/refactor-backlog-setup/refactor-backlog/implementation/rb-04.md diff --git a/backend/src/BigRegister.Api/Data/DocumentStore.cs b/backend/src/BigRegister.Api/Data/DocumentStore.cs index 4b60054..82054f1 100644 --- a/backend/src/BigRegister.Api/Data/DocumentStore.cs +++ b/backend/src/BigRegister.Api/Data/DocumentStore.cs @@ -1,3 +1,5 @@ +using BigRegister.Domain.People; + namespace BigRegister.Api.Data; /// @@ -58,7 +60,7 @@ public static class DocumentStore db.Documents.Add(doc); db.SaveChanges(); } - Audit("upload", doc.DocumentId, categoryId, owner); + Audit("upload", doc.DocumentId, categoryId, Pii.MaskTail(owner, 3)); return doc; } @@ -156,7 +158,7 @@ public static class DocumentStore db.Documents.Remove(d); db.SaveChanges(); } - Audit("delete-user", documentId, categoryId, owner); + Audit("delete-user", documentId, categoryId, Pii.MaskTail(owner, 3)); return DeleteResult.Ok; } @@ -178,6 +180,12 @@ public static class DocumentStore return true; } + /// Append one metadata-only audit row. must arrive + /// **already redacted** (RB-04/BIO-005) — the two citizen call sites pass + /// of the owner BSN, `delete-admin` passes the literal + /// `"admin"`. The unmasked BSN lives only in , which is + /// the authorization key and stays untouched. Masking here instead would have to guess + /// which actors are BSNs and which are role names. public static void Audit(string action, string documentId, string categoryId, string actor) { lock (_gate) diff --git a/backend/tests/BigRegister.Tests/UploadAccessTests.cs b/backend/tests/BigRegister.Tests/UploadAccessTests.cs index 54e094f..6537b01 100644 --- a/backend/tests/BigRegister.Tests/UploadAccessTests.cs +++ b/backend/tests/BigRegister.Tests/UploadAccessTests.cs @@ -2,13 +2,16 @@ 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; -/// RB-01/BIO-004: GET /uploads/{id}/content and /uploads/status used to take no -/// HttpContext at all — a diploma or identity scan was protected by GUID -/// unguessability alone, while DELETE on the same resource was owner-scoped. +/// Who may see what about an upload. RB-01/BIO-004: GET /uploads/{id}/content and +/// /uploads/status used to take no HttpContext at all — a diploma or identity scan was +/// protected by GUID unguessability alone, while DELETE on the same resource was +/// owner-scoped. RB-04/BIO-005: the document audit trail recorded the raw owner BSN as +/// its Actor, on a store whose own doc comment says it holds no PII. public class UploadAccessTests(TestWebApplicationFactory factory) : IClassFixture { private readonly HttpClient _client = factory.CreateClient(); @@ -69,6 +72,19 @@ public class UploadAccessTests(TestWebApplicationFactory factory) : IClassFixtur ("X-Medewerker", "medewerker-1"), ("X-Rollen", "geen"))).StatusCode); } + [Fact] + public async Task The_document_audit_trail_records_a_masked_actor() + { + var id = await UploadAsOwner(); + (await _client.DeleteAsync($"/api/v1/uploads/{id}")).EnsureSuccessStatusCode(); + + var rows = DocumentStore.AuditLog.Where(e => e.DocumentId == id).ToList(); + Assert.Equal(new[] { "upload", "delete-user" }, rows.Select(e => e.Action)); + Assert.All(rows, e => Assert.Equal("******782", e.Actor)); + // The unmasked BSN stays where it is load-bearing — the ownership key, not the trail. + Assert.All(rows, e => Assert.DoesNotContain(DocumentStore.DemoOwner, e.Actor)); + } + [Fact] public async Task Status_reports_another_citizens_localId_as_unknown() { diff --git a/docs/project/refactor-backlog-setup/refactor-backlog/implementation/rb-04.md b/docs/project/refactor-backlog-setup/refactor-backlog/implementation/rb-04.md new file mode 100644 index 0000000..817072e --- /dev/null +++ b/docs/project/refactor-backlog-setup/refactor-backlog/implementation/rb-04.md @@ -0,0 +1,37 @@ +# RB-04 — mask the BSN recorded as `AuditEntry.Actor` + +Status: **implemented** · 2026-08-27 · Source findings: `07-bio2-compliance.md` BIO-005 · `99-backlog.md` RB-04 + +## What was wrong + +`DocumentStore` writes one audit row per upload and per user delete, with the acting +citizen's raw BSN as `AuditEntry.Actor`, persisted to SQLite. The class's own doc comment +says "The audit log holds metadata only (never file content **or other PII**)" — a BSN in +every row is precisely other PII. Same failure shape as RB-02, in a second store. + +## What changed + +| File | Change | +| ------------------------------- | ----------------------------------------------------------------- | +| `Data/DocumentStore.cs` `Add` | `Audit("upload", …, Pii.MaskTail(owner, 3))` | +| `Data/DocumentStore.cs` `DeleteOwned` | `Audit("delete-user", …, Pii.MaskTail(owner, 3))` | +| `Data/DocumentStore.cs` `Audit` | doc comment: actors arrive **already redacted** | +| `UploadAccessTests.cs` | **new** `The_document_audit_trail_records_a_masked_actor` | + +**Masked at the two call sites, not inside `Audit`** — unlike RB-03, where masking in the +mapper was the point. `Audit`'s third actor is the literal `"admin"` (from `AdminDelete`), +and `MaskTail("admin", 3)` is `"**min"`: masking centrally would mean guessing which +actors are BSNs and which are role names. The contract is stated on `Audit` instead. + +**`StoredDocument.Owner` is untouched**, per the ticket. It is the authorization key — +`DeleteOwned`, `ForeignIds` and now the RB-01 content check all compare against it — so it +has to stay whole. The BSN remains where it is load-bearing and leaves the trail where it +was only decoration. + +Nothing reads `DocumentStore.AuditLog` today (no endpoint exposes it), so this is a +data-at-rest fix with no response-shape change. + +## Verification + +`dotnet format --verify-no-changes` clean. `dotnet test`: **252 passed, 1 failed** — the +pre-existing `OpenZaakIntegrationTests.Admin_cases_…`, which needs a live container.