fix(uploads): authorize the document-content and status endpoints (RB-01)
GET /uploads/{documentId}/content took only (string documentId) — no
HttpContext, so no authorization was possible. It streams diploma and
identity scans, protected by GUID unguessability alone, while DELETE on the
same resource has always been owner-scoped. GET /uploads/status had the same
shape and confirmed whether any client-chosen localId exists, plus its
documentId.
Both now take HttpContext. Content is readable by the owning
ZorgverlenerCaller or a caller passing Authz.CanBeoordelen — matched on the
caller kind rather than branched on a boolean, because ctx.Zorgverlener()
throws for a MedewerkerCaller and the behandelportal's beoordeling screen is
a legitimate reader. Status is scoped to ctx.Zorgverlener().Bsn via a new
owner parameter on DocumentStore.ByLocalIds (one call site).
404, not 403, on both: a foreign document id must not be distinguishable
from one that never existed, and a foreign localId reads back as "unknown".
Residual, recorded in the implementation note: both callers reach the URL as
a plain browser navigation (<a href> / previewUrl), which carries no identity
header and no interceptor, so StubIdentityProvider resolves it to the seeded
citizen. That is BIO-002 and belongs to RB-09; the links keep working today
only because one citizen owns every document in the POC.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
@@ -73,13 +73,13 @@ public static class DocumentStore
|
||||
|
||||
/// Status for the poll-on-return pattern: a known localId is "complete" (it
|
||||
/// arrived), an unknown one is still in flight / never started.
|
||||
public static IReadOnlyList<StoredDocument> ByLocalIds(IEnumerable<string> localIds)
|
||||
public static IReadOnlyList<StoredDocument> ByLocalIds(IEnumerable<string> localIds, string owner)
|
||||
{
|
||||
var set = localIds.ToHashSet();
|
||||
lock (_gate)
|
||||
{
|
||||
using var db = Db.Create();
|
||||
return db.Documents.Where(d => set.Contains(d.LocalId)).ToList();
|
||||
return db.Documents.Where(d => set.Contains(d.LocalId) && d.Owner == owner).ToList();
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -228,10 +228,18 @@ api.MapPost("/uploads", async (HttpRequest request, HttpContext ctx, IDocumentSo
|
||||
|
||||
// Serve stored bytes so a re-opened wizard can preview/download an upload. Inline
|
||||
// for pdf/image (browser renders it), attachment otherwise (download).
|
||||
api.MapGet("/uploads/{documentId}/content", (string documentId) =>
|
||||
// Scoped like DELETE on the same resource (RB-01/BIO-004): the owning citizen, or a
|
||||
// behandelaar reading an aanvraag's linked documents. A foreign id 404s rather than
|
||||
// 403s, so the endpoint never confirms that a document id exists.
|
||||
api.MapGet("/uploads/{documentId}/content", (string documentId, HttpContext ctx) =>
|
||||
{
|
||||
var doc = DocumentStore.Get(documentId);
|
||||
if (doc is null) return Results.NotFound();
|
||||
var allowed = ctx.Caller() switch
|
||||
{
|
||||
ZorgverlenerCaller z => doc?.Owner == z.Bsn,
|
||||
var caller => Authz.CanBeoordelen(caller),
|
||||
};
|
||||
if (doc is null || !allowed) return Results.NotFound();
|
||||
var inline = doc.ContentType == "application/pdf" || doc.ContentType.StartsWith("image/");
|
||||
return Results.File(doc.Content, doc.ContentType, fileDownloadName: inline ? null : doc.FileName);
|
||||
})
|
||||
@@ -239,10 +247,12 @@ api.MapGet("/uploads/{documentId}/content", (string documentId) =>
|
||||
.Produces(StatusCodes.Status404NotFound);
|
||||
|
||||
// Poll-on-return: which of these client localIds have arrived at the BFF.
|
||||
api.MapGet("/uploads/status", (string? localIds) =>
|
||||
api.MapGet("/uploads/status", (string? localIds, HttpContext ctx) =>
|
||||
{
|
||||
var ids = (localIds ?? "").Split(',', StringSplitOptions.RemoveEmptyEntries | StringSplitOptions.TrimEntries);
|
||||
var found = DocumentStore.ByLocalIds(ids).ToDictionary(d => d.LocalId);
|
||||
// Owner-scoped (RB-01/BIO-004): someone else's localId reads back as "unknown", the
|
||||
// same answer an id that never existed gets.
|
||||
var found = DocumentStore.ByLocalIds(ids, ctx.Zorgverlener().Bsn).ToDictionary(d => d.LocalId);
|
||||
var results = ids.Select(id => found.TryGetValue(id, out var d)
|
||||
? new UploadStatusItemDto(id, "complete", d.DocumentId)
|
||||
: new UploadStatusItemDto(id, "unknown", null)).ToList();
|
||||
|
||||
@@ -0,0 +1,83 @@
|
||||
using System.Net;
|
||||
using System.Net.Http.Headers;
|
||||
using System.Net.Http.Json;
|
||||
using BigRegister.Api.Contracts;
|
||||
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.
|
||||
public class UploadAccessTests(TestWebApplicationFactory factory) : IClassFixture<TestWebApplicationFactory>
|
||||
{
|
||||
private readonly HttpClient _client = factory.CreateClient();
|
||||
|
||||
private const string OtherCitizen = "999999990";
|
||||
|
||||
private async Task<string> 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-rb01"), "localId");
|
||||
form.Add(new StringContent("registratie"), "wizardId");
|
||||
var res = await _client.PostAsync("/api/v1/uploads", form);
|
||||
Assert.Equal(HttpStatusCode.Created, res.StatusCode);
|
||||
return (await res.Content.ReadFromJsonAsync<UploadResponse>())!.DocumentId;
|
||||
}
|
||||
|
||||
private Task<HttpResponseMessage> Get(string path, params (string Name, string Value)[] headers)
|
||||
{
|
||||
var req = new HttpRequestMessage(HttpMethod.Get, path);
|
||||
foreach (var (name, value) in headers) req.Headers.Add(name, value);
|
||||
return _client.SendAsync(req);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task The_owner_can_read_the_bytes()
|
||||
{
|
||||
var id = await UploadAsOwner();
|
||||
Assert.Equal(HttpStatusCode.OK, (await Get($"/api/v1/uploads/{id}/content")).StatusCode);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task Another_citizen_gets_404_not_403()
|
||||
{
|
||||
var id = await UploadAsOwner();
|
||||
// 404, not 403: a foreign id must not be distinguishable from one that never existed.
|
||||
Assert.Equal(HttpStatusCode.NotFound,
|
||||
(await Get($"/api/v1/uploads/{id}/content", ("X-Subject", OtherCitizen))).StatusCode);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task A_behandelaar_can_read_a_linked_document()
|
||||
{
|
||||
var id = await UploadAsOwner();
|
||||
Assert.Equal(HttpStatusCode.OK,
|
||||
(await Get($"/api/v1/uploads/{id}/content", ("X-Medewerker", "medewerker-1"))).StatusCode);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task A_medewerker_without_the_behandelaar_rol_does_not()
|
||||
{
|
||||
var id = await UploadAsOwner();
|
||||
Assert.Equal(HttpStatusCode.NotFound,
|
||||
(await Get($"/api/v1/uploads/{id}/content",
|
||||
("X-Medewerker", "medewerker-1"), ("X-Rollen", "geen"))).StatusCode);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task Status_reports_another_citizens_localId_as_unknown()
|
||||
{
|
||||
await UploadAsOwner();
|
||||
var res = await Get("/api/v1/uploads/status?localIds=local-rb01", ("X-Subject", OtherCitizen));
|
||||
res.EnsureSuccessStatusCode();
|
||||
var status = (await res.Content.ReadFromJsonAsync<UploadStatusDto>())!;
|
||||
var item = Assert.Single(status.Results);
|
||||
Assert.Equal("unknown", item.Status);
|
||||
Assert.Null(item.DocumentId);
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,57 @@
|
||||
# RB-01 — authorize `GET /uploads/{id}/content` and `/uploads/status`
|
||||
|
||||
Status: **implemented** · 2026-08-27 · Source findings: `07-bio2-compliance.md` BIO-004 · `99-backlog.md` RB-01
|
||||
|
||||
## What was wrong
|
||||
|
||||
`GET /uploads/{documentId}/content` took `(string documentId)` — no `HttpContext`, so no
|
||||
authorization was possible at all. It streams diploma and identity scans; the only
|
||||
protection was the unguessability of the document GUID. `DELETE` on the same resource has
|
||||
been owner-scoped (`DocumentStore.DeleteOwned`) since it was written.
|
||||
|
||||
`GET /uploads/status?localIds=` had the same shape, and leaks less but still confirms
|
||||
whether a given client-chosen `localId` exists anywhere in the store, plus its documentId.
|
||||
|
||||
## 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 |
|
||||
|
||||
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
|
||||
(`beoordeling-documenten.component.ts`) is a legitimate caller here:
|
||||
|
||||
```csharp
|
||||
var allowed = ctx.Caller() switch
|
||||
{
|
||||
ZorgverlenerCaller z => doc?.Owner == z.Bsn,
|
||||
var caller => Authz.CanBeoordelen(caller),
|
||||
};
|
||||
```
|
||||
|
||||
**404, not 403**, per the ticket: a foreign document id must not be distinguishable from
|
||||
one that never existed. `doc is null || !allowed` collapses both to the same answer, and
|
||||
`/uploads/status` reports a foreign `localId` as `"unknown"` — the same word an id that
|
||||
never existed gets.
|
||||
|
||||
## Known residual — this endpoint is reached without identity headers
|
||||
|
||||
Both callers link to the URL directly (`<a href>` in `beoordeling-documenten.component.ts`,
|
||||
`previewUrl` in `libs/shared/src/upload/upload.adapter.ts`), so the request is a plain
|
||||
browser navigation that carries **no** `X-Medewerker` / `X-Subject` header and never passes
|
||||
through an Angular interceptor. `StubIdentityProvider` therefore resolves it to the seeded
|
||||
citizen, which owns every document in the POC, so both links keep working — by coincidence,
|
||||
not by authorization. That coincidence **is** BIO-002, and it is fixed by **RB-09** (making
|
||||
`IIdentityProvider` able to express "no identity"), not here. RB-09 will need this endpoint
|
||||
to receive a real credential — a signed URL or a cookie — rather than the ambient default.
|
||||
|
||||
## Verification
|
||||
|
||||
`dotnet build` clean. `dotnet test`: **250 passed, 1 failed** — the failure is
|
||||
`OpenZaakIntegrationTests.Admin_cases_returns_the_seeded_zaak_mapped_through_real_HTTP_and_JWT`,
|
||||
which needs a live OpenZaak container and **fails identically on a stashed tree**, i.e. it
|
||||
pre-dates this change.
|
||||
Reference in New Issue
Block a user