fix(privacy): mask the BSN recorded as the document audit Actor (RB-04)

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 <noreply@anthropic.com>
This commit is contained in:
eho
2026-08-27 10:54:07 +02:00
co-authored by Claude Opus 5
parent 487818e67a
commit fbd27ed641
3 changed files with 66 additions and 5 deletions
@@ -1,3 +1,5 @@
using BigRegister.Domain.People;
namespace BigRegister.Api.Data;
/// <summary>
@@ -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;
}
/// <summary>Append one metadata-only audit row. <paramref name="actor"/> must arrive
/// **already redacted** (RB-04/BIO-005) — the two citizen call sites pass
/// <see cref="Pii.MaskTail"/> of the owner BSN, `delete-admin` passes the literal
/// `"admin"`. The unmasked BSN lives only in <see cref="StoredDocument.Owner"/>, which is
/// the authorization key and stays untouched. Masking here instead would have to guess
/// which actors are BSNs and which are role names.</summary>
public static void Audit(string action, string documentId, string categoryId, string actor)
{
lock (_gate)
@@ -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<TestWebApplicationFactory>
{
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()
{
@@ -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.