fix(uploads): delete the dead POST /registrations (RB-06)
POST /registrations passed its Documents list straight to Submit, which calls
DocumentStore.Link on every digital documentId in it — and linking a document
blocks its owner from ever deleting it (DeleteOwned returns 409 Linked). That
path had no ForeignIds ownership check, so any authenticated citizen could
post another citizen's document id and permanently block them from deleting
their own diploma scan. POST /applications/{id}/submit, the endpoint actually
in use, has had that guard since it was written.
Deleted rather than guarded: the endpoint is dead. No frontend caller, and
the whole registratie flow goes through /applications/{id}/submit.
RegistratieRequest went with it, and so did SubmissionRules.RejectRegistratie
— reachable only from here, and contradicted by the live path, which treats a
handmatig diploma as "does not auto-approve" rather than a 422 rejection. Its
own message said as much while being returned as a rejection. That last part
is a judgement call beyond the ticket's wording; reverting the two
SubmissionRules hunks restores it in isolation.
Coverage moved rather than vanished: the problem+json shape assertion is now
on /change-requests (the other endpoint on the same Submit helper), and the
linked-delete 409 test goes through the real submit path.
swagger.json, the generated client and the behaviour spec regenerated.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
Claude Opus 5
@@ -14,12 +14,12 @@ whether a given client-chosen `localId` exists anywhere in the store, plus its d
|
||||
|
||||
## What changed
|
||||
|
||||
| File | Change |
|
||||
| ------------------------------------------------ | --------------------------------------------------------------------------------------------------------------------- |
|
||||
| `Program.cs` `/uploads/{documentId}/content` | takes `HttpContext`; allowed for the owning `ZorgverlenerCaller` or a caller passing `Authz.CanBeoordelen`; else `404` |
|
||||
| `Program.cs` `/uploads/status` | takes `HttpContext`; scoped to `ctx.Zorgverlener().Bsn` |
|
||||
| `Data/DocumentStore.cs` `ByLocalIds` | second parameter `owner`; filters on it (the only call site is the endpoint above) |
|
||||
| `tests/BigRegister.Tests/UploadAccessTests.cs` | **new** — 5 cases |
|
||||
| File | Change |
|
||||
| ---------------------------------------------- | ---------------------------------------------------------------------------------------------------------------------- |
|
||||
| `Program.cs` `/uploads/{documentId}/content` | takes `HttpContext`; allowed for the owning `ZorgverlenerCaller` or a caller passing `Authz.CanBeoordelen`; else `404` |
|
||||
| `Program.cs` `/uploads/status` | takes `HttpContext`; scoped to `ctx.Zorgverlener().Bsn` |
|
||||
| `Data/DocumentStore.cs` `ByLocalIds` | second parameter `owner`; filters on it (the only call site is the endpoint above) |
|
||||
| `tests/BigRegister.Tests/UploadAccessTests.cs` | **new** — 5 cases |
|
||||
|
||||
The two actor kinds are matched, not branched on a boolean, because `ctx.Zorgverlener()`
|
||||
**throws** for a `MedewerkerCaller` — a behandelaar reading an aanvraag's linked documents
|
||||
|
||||
@@ -10,7 +10,7 @@ Status: **implemented** · 2026-08-27 · Source findings: `07-bio2-compliance.md
|
||||
audit page — so a BSN was written to durable storage and shown in a UI, on the one trail
|
||||
four documents describe as data-minimised and PII-free.
|
||||
|
||||
The endpoint is the *BIG-nummer reveal*, whose own comment says the audit carries
|
||||
The endpoint is the _BIG-nummer reveal_, whose own comment says the audit carries
|
||||
"NO PII. Never the value that was (or wasn't) revealed" — and it did not carry the
|
||||
BIG-nummer. It carried the BSN instead, in the adjacent argument.
|
||||
|
||||
@@ -28,10 +28,10 @@ value-asserting test is part of this ticket's definition of done rather than a f
|
||||
|
||||
## What changed
|
||||
|
||||
| File | Change |
|
||||
| ----------------------- | ---------------------------------------------------------------------------------------------------- |
|
||||
| `Program.cs` | resource ref is `"brief"`; a comment records why the id added nothing |
|
||||
| `AuthzAuditTests.cs` | **new** `No_audit_row_carries_a_subjects_bsn` — asserts on stored **values**, every string field |
|
||||
| File | Change |
|
||||
| -------------------- | ------------------------------------------------------------------------------------------------ |
|
||||
| `Program.cs` | resource ref is `"brief"`; a comment records why the id added nothing |
|
||||
| `AuthzAuditTests.cs` | **new** `No_audit_row_carries_a_subjects_bsn` — asserts on stored **values**, every string field |
|
||||
|
||||
No identifier was lost. `BriefStore` keys one brief per owner, so `brief/<bsn>` named the
|
||||
same thing the row's acting principal already implies; there is no second brief the ref
|
||||
|
||||
@@ -10,21 +10,21 @@ both cross-owner lists read by someone who is **not** the subject:
|
||||
- `GET /admin/cases` (`cases:manage`)
|
||||
- `GET /werkvoorraad` (`aanvraag:beoordelen`)
|
||||
|
||||
`GET /beoordeling/{id}` — the *detail* view of the same data — already masked. So the
|
||||
`GET /beoordeling/{id}` — the _detail_ view of the same data — already masked. So the
|
||||
detail screen showed `******782` while the list one click earlier showed the whole BSN.
|
||||
|
||||
## What changed
|
||||
|
||||
| File | Change |
|
||||
| --------------------------- | ----------------------------------------------------------------------------------- |
|
||||
| `Domain/People/Pii.cs` | **new** — `Pii.MaskTail`, moved out of `Program.cs` |
|
||||
| `Contracts/Mappers.cs` | `Owner = Pii.MaskTail(a.Owner, 3)` |
|
||||
| `Program.cs` | local `MaskTail` deleted; two call sites point at `Pii.MaskTail` |
|
||||
| `AdminCasesTests.cs` | asserts the masked value and that `DemoOwner` does not appear |
|
||||
| `WerkvoorraadTests.cs` | same assertion, replacing the `IsNullOrEmpty` one |
|
||||
| File | Change |
|
||||
| ---------------------- | ---------------------------------------------------------------- |
|
||||
| `Domain/People/Pii.cs` | **new** — `Pii.MaskTail`, moved out of `Program.cs` |
|
||||
| `Contracts/Mappers.cs` | `Owner = Pii.MaskTail(a.Owner, 3)` |
|
||||
| `Program.cs` | local `MaskTail` deleted; two call sites point at `Pii.MaskTail` |
|
||||
| `AdminCasesTests.cs` | asserts the masked value and that `DemoOwner` does not appear |
|
||||
| `WerkvoorraadTests.cs` | same assertion, replacing the `IsNullOrEmpty` one |
|
||||
|
||||
**Masked in the mapper, not at the endpoints.** The point of the ticket is that both
|
||||
lists *inherit* it, so a third cross-owner list cannot be added that forgets to mask.
|
||||
lists _inherit_ it, so a third cross-owner list cannot be added that forgets to mask.
|
||||
|
||||
**`MaskTail` moved to `Domain/People/Pii.cs`** because it now has three callers across
|
||||
three folders (`Contracts`, `Program.cs`, and `Data` once **RB-04** lands), and a second
|
||||
|
||||
@@ -11,12 +11,12 @@ 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))` |
|
||||
| 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` |
|
||||
| `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`),
|
||||
|
||||
@@ -26,10 +26,10 @@ The two `"returned null body"` throws in `GetAsync`/`PostAsync` interpolated the
|
||||
|
||||
## What changed
|
||||
|
||||
| File | Change |
|
||||
| ------------------------ | --------------------------------------------------------------------------------- |
|
||||
| `Zgw/ZgwHttpClient.cs` | `Redact(url)` (path only) at all three sites; snippet → `res.ReasonPhrase` |
|
||||
| `ZgwDivergenceTests.cs` | **new** `A_recorded_divergence_carries_no_response_body_and_no_query_string` |
|
||||
| File | Change |
|
||||
| ----------------------- | ---------------------------------------------------------------------------- |
|
||||
| `Zgw/ZgwHttpClient.cs` | `Redact(url)` (path only) at all three sites; snippet → `res.ReasonPhrase` |
|
||||
| `ZgwDivergenceTests.cs` | **new** `A_recorded_divergence_carries_no_response_body_and_no_query_string` |
|
||||
|
||||
Status + path is enough to route a failure to the right endpoint. The diagnostic detail
|
||||
that was lost already has a deliberate home: `ZGW_DEBUG_HTTP=1` wires
|
||||
|
||||
@@ -0,0 +1,69 @@
|
||||
# RB-06 — delete the dead `POST /registrations`
|
||||
|
||||
Status: **implemented** · 2026-08-27 · Source findings: `07-bio2-compliance.md` BIO-010 · `99-backlog.md` RB-06
|
||||
|
||||
## What was wrong
|
||||
|
||||
`POST /registrations` took a `Documents` list and passed it straight to `Submit`, which
|
||||
calls `DocumentStore.Link(...)` on every digital `documentId` in it. Linking a document
|
||||
**blocks its owner from deleting it** (`DeleteOwned` → 409 `Linked`).
|
||||
|
||||
There was no `ForeignIds` ownership check on that path. The real submit endpoint,
|
||||
`POST /applications/{id}/submit`, has had one since it was written:
|
||||
|
||||
```csharp
|
||||
if (documentIds is { Count: > 0 } && DocumentStore.ForeignIds(documentIds, ctx.Zorgverlener().Bsn) is { Count: > 0 } foreignIds)
|
||||
return Results.Problem(detail: $"Onbekend of niet-eigen document(en): …", statusCode: 400);
|
||||
```
|
||||
|
||||
So any authenticated citizen could post another citizen's document id and permanently
|
||||
block them from deleting their own diploma scan.
|
||||
|
||||
## Deleted rather than guarded
|
||||
|
||||
The ticket allowed either. Deleted, because the endpoint is dead: no frontend caller (the
|
||||
generated client's `registrations` method was unreferenced), and the whole registratie flow
|
||||
goes through `POST /applications/{id}/submit`.
|
||||
|
||||
| File | Change |
|
||||
| -------------------------------------------- | ------------------------------------------------- |
|
||||
| `Program.cs` | endpoint deleted |
|
||||
| `Contracts/Dtos.cs` | `RegistratieRequest` deleted (no other reference) |
|
||||
| `Domain/Submissions/SubmissionRules.cs` | `RejectRegistratie` deleted — see below |
|
||||
| `backend/swagger.json`, `api-client.ts` | regenerated (`npm run gen:api`) |
|
||||
| `EndpointTests.cs`, `SubmissionRuleTests.cs` | retargeted, see below |
|
||||
|
||||
### Why `RejectRegistratie` went with it
|
||||
|
||||
It was reachable only from this endpoint, and the live path deliberately **contradicts**
|
||||
it. `RejectRegistratie("handmatig")` returned a 422 rejection; the modern submit does
|
||||
|
||||
```csharp
|
||||
"registratie" => (null, req.DiplomaHerkomst == "duo"),
|
||||
```
|
||||
|
||||
— a manual diploma is not rejected, it simply does not auto-approve and goes to a
|
||||
behandelaar. Its own message even said so ("doorgestuurd voor handmatige beoordeling")
|
||||
while being returned as a rejection. Leaving it behind would have left an obsolete rule
|
||||
with a passing spec, which is exactly how it gets reintroduced.
|
||||
|
||||
**This is the one judgement call in this ticket** — the backlog row says "delete the dead
|
||||
endpoint", not "delete the rule". Reverting just the `SubmissionRules`/`SubmissionRuleTests`
|
||||
hunks restores it without touching anything else.
|
||||
|
||||
### Test coverage that moved rather than vanished
|
||||
|
||||
- `Registration_with_manual_diploma_is_rejected_with_problem_details` was the only test
|
||||
asserting the `Submit` helper's `application/problem+json` rejection shape. That assertion
|
||||
moved into `Change_request_with_bad_phone_is_rejected_with_problem_details` —
|
||||
`/change-requests` is the other endpoint on the same helper.
|
||||
- `User_delete_blocked_with_409_once_linked_to_submission` covered `DocumentStore.Link`
|
||||
blocking a delete. Retargeted to `POST /applications/{id}/submit`, i.e. the path that is
|
||||
actually in use. `POST /registrations` was the only other caller of `Link`.
|
||||
- `Registration_with_duo_diploma_succeeds` was deleted outright — `Change_request_with_valid_phone_succeeds`
|
||||
is the same assertion on the same helper.
|
||||
|
||||
## Verification
|
||||
|
||||
`npm run ci`. `dotnet test`: **249 passed, 1 failed** — the pre-existing
|
||||
`OpenZaakIntegrationTests.Admin_cases_…`, which needs a live container.
|
||||
Reference in New Issue
Block a user