From 9665f0763ade85bb28b7527bc3f1b83c1935cac1 Mon Sep 17 00:00:00 2001 From: MaximumTrainer Date: Sun, 13 Sep 2026 18:38:14 +0100 Subject: [PATCH] fix(api): scope document upload and download to the session Closes #104 These were the only session endpoints carrying ApplicantOrOperator without a resource-based ownership check. The policy asserts the caller holds some applicant or operator credential; it says nothing about which session that credential names, which is the whole point of the per-session token. Upload trusted the route's sessionId, so any applicant token could write documents into any session - landing a Submission row that reads in the operator console as the real applicant's upload. Download was worse than an absent check. sessionId and nodeId were in the route but never reached the service, which resolved the file id straight from blob storage: any authenticated caller could fetch any stored document given its id, with the path segments that appeared to scope the request doing nothing. These are identity and address documents, and the ids are not secrets - they are returned to the browser, stored in Submission.DataJson and shown in the operator console. Both now run the ownership check the other endpoints use: Write for upload, so the terminal-status rule applies too, and Read for download. Ownership alone would not close the read, since the owner of one session could still request another's file through their own route. The lookup is now scoped: GetDocumentAsync takes the session and node, and resolves a file only if an upload recorded against them carries the id. The port signature changed rather than a check being added beside it, so the unscoped path cannot be called at all. A file belonging elsewhere returns 404, not 403, since a refusal would confirm the id is real. Co-Authored-By: Claude Opus 5 (1M context) --- docs/runbook.md | 8 +- .../Controllers/WorkflowController.cs | 17 +- .../DocumentOwnershipTests.cs | 371 ++++++++++++++++++ .../TestWebAppFactory.cs | 9 +- .../Interfaces/IWorkflowService.cs | 14 +- .../Queries/GetDocumentQuery.cs | 7 +- .../QueryHandlers/GetDocumentQueryHandler.cs | 71 +++- .../Services/WorkflowService.cs | 4 +- 8 files changed, 491 insertions(+), 10 deletions(-) create mode 100644 src/backend/OpenOnboarding.Application.Tests/DocumentOwnershipTests.cs diff --git a/docs/runbook.md b/docs/runbook.md index 9a231a7..35a08f1 100644 --- a/docs/runbook.md +++ b/docs/runbook.md @@ -143,8 +143,14 @@ response carries `applicantToken`, an HMAC-signed JWT the browser sends as extends the credential, so a journey being worked on never outlives its own token, and there is no separate refresh endpoint to protect. Operators get no token back: their own credential already covers the session. +- **Documents.** Upload and download are scoped to the session in the route, like every other + session endpoint. Upload follows the terminal-status rule below. Download also scopes the + *lookup*: a file id is resolved only if an upload recorded against that session and node carries + it, so holding an id for another applicant's document does not make it readable through your own + session. A file that exists but belongs elsewhere returns `404`, never `403` — a refusal would + confirm the id is real. - **Terminal sessions.** Once a session is `Completed` or `Abandoned` its token is refused for - writes — step submission and `POST /api/analytics/events` both return `403`. Reads still work, so + writes — step submission, document upload and `POST /api/analytics/events` all return `403`. Reads still work, so the completion screen renders. This bounds how long a credential left behind on a shared machine stays useful: the visit, not the full lifetime. Operators are unaffected; the rule is about a stale applicant credential, not about who may act on a finished session. Abandoning an already diff --git a/src/backend/OpenOnboarding.Api/Controllers/WorkflowController.cs b/src/backend/OpenOnboarding.Api/Controllers/WorkflowController.cs index 58b5c98..bb50f1b 100644 --- a/src/backend/OpenOnboarding.Api/Controllers/WorkflowController.cs +++ b/src/backend/OpenOnboarding.Api/Controllers/WorkflowController.cs @@ -227,6 +227,14 @@ public async Task UploadDocuments( IList files, CancellationToken cancellationToken) { + // Same ownership rule as every other session endpoint. The route's sessionId was previously + // trusted, so any applicant token could write documents into any session - and the resulting + // Submission row reads in the operator console as the real applicant's upload. Checked + // before the files are read, so a refused caller never reaches storage or the virus scanner. + var session = await sessionAnalyticsService.GetSessionAsync(sessionId, cancellationToken); + var authResult = await authorizationService.AuthorizeAsync(User, session, SessionOwnershipRequirement.Write); + if (!authResult.Succeeded) return Forbid(); + if (files == null || files.Count == 0) return BadRequest(new ProblemDetails { Title = "No files provided.", Status = 400 }); @@ -292,7 +300,14 @@ public async Task GetDocument( [FromRoute] string fileId, CancellationToken cancellationToken) { - var (stream, info) = await workflowService.GetDocumentAsync(fileId, cancellationToken); + var session = await sessionAnalyticsService.GetSessionAsync(sessionId, cancellationToken); + var authResult = await authorizationService.AuthorizeAsync(User, session, SessionOwnershipRequirement.Read); + if (!authResult.Succeeded) return Forbid(); + + // The session and node are passed through rather than left in the route as decoration. + // Ownership alone is not enough: it would still let the owner of one session ask for + // another's document through their own route. Anything not recorded here is a 404. + var (stream, info) = await workflowService.GetDocumentAsync(sessionId, nodeId, fileId, cancellationToken); return File(stream, info.ContentType, info.FileName); } diff --git a/src/backend/OpenOnboarding.Application.Tests/DocumentOwnershipTests.cs b/src/backend/OpenOnboarding.Application.Tests/DocumentOwnershipTests.cs new file mode 100644 index 0000000..1e3e3de --- /dev/null +++ b/src/backend/OpenOnboarding.Application.Tests/DocumentOwnershipTests.cs @@ -0,0 +1,371 @@ +using System.Net; +using System.Net.Http.Headers; +using System.Net.Http.Json; +using Microsoft.AspNetCore.Mvc.Testing; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.DependencyInjection.Extensions; +using OpenOnboarding.Application.Interfaces; +using OpenOnboarding.Application.Tests.TestHelpers; + +namespace OpenOnboarding.Application.Tests; + +/// +/// Document upload and download were the only session endpoints carrying +/// [Authorize(Policy = "ApplicantOrOperator")] without a resource-based ownership check. The +/// policy asserts the caller holds some applicant or operator credential; it says nothing about +/// which session that credential names, which is the whole point of the per-session token. +/// +/// Download was worse than an absent check: sessionId and nodeId were in the route but +/// never reached the service, which resolved the file id straight from storage. The segments that +/// looked like they scoped the request did nothing at all. +/// +/// +public sealed class DocumentOwnershipTests +{ + private const string OperatorApiKey = "test-api-key"; + + private sealed record StartResponse(Guid SessionId, bool IsCompleted, NodeStub? CurrentNode, string? ApplicantToken); + private sealed record NodeStub(Guid Id, string Key, string Type); + private sealed record FlowStub(Guid Id); + private sealed record StoredFile(string FileId, string FileName); + + private static WebApplicationFactory CreateFactory() + => TestWebAppFactory.Create(configureServices: services => + { + // Otherwise uploads hit the real filesystem provider. The double keeps the test about + // authorization rather than about storage. + services.RemoveAll(); + services.AddSingleton(); + }); + + private static HttpClient CreateOperatorClient(WebApplicationFactory factory) + { + var client = factory.CreateClient(); + client.DefaultRequestHeaders.Add("X-Api-Key", OperatorApiKey); + return client; + } + + /// A flow whose start node accepts documents, so a session is ready to receive one. + private static async Task CreateDocumentFlowAsync(HttpClient operatorClient) + { + var response = await operatorClient.PostAsJsonAsync("/api/flows", new + { + name = $"Document flow {Guid.NewGuid()}", + nodes = new[] + { + new + { + key = "upload", + type = "DocumentUpload", + title = "Upload", + isStartNode = true, + jsonContent = "{\"acceptedFileTypes\":[\"text/plain\"],\"maxFiles\":5}" + } + }, + connections = Array.Empty() + }); + + response.EnsureSuccessStatusCode(); + return (await response.Content.ReadFromJsonAsync())!.Id; + } + + private static async Task StartSessionAsync(HttpClient browser, Guid flowId) + { + var response = await browser.PostAsJsonAsync("/api/workflow/sessions/start", new { flowId }); + response.EnsureSuccessStatusCode(); + return (await response.Content.ReadFromJsonAsync())!; + } + + private static HttpRequestMessage UploadRequest(Guid sessionId, Guid nodeId, string? token, string? apiKey = null) + { + var content = new MultipartFormDataContent(); + var file = new ByteArrayContent("hello"u8.ToArray()); + file.Headers.ContentType = new MediaTypeHeaderValue("text/plain"); + content.Add(file, "files", "evidence.txt"); + + var request = new HttpRequestMessage( + HttpMethod.Post, $"/api/workflow/sessions/{sessionId}/steps/{nodeId}/documents") + { + Content = content + }; + + if (token is not null) + request.Headers.Authorization = new AuthenticationHeaderValue("Bearer", token); + if (apiKey is not null) + request.Headers.Add("X-Api-Key", apiKey); + + return request; + } + + private static HttpRequestMessage DownloadRequest(Guid sessionId, Guid nodeId, string fileId, string? token, string? apiKey = null) + { + var request = new HttpRequestMessage( + HttpMethod.Get, $"/api/workflow/sessions/{sessionId}/steps/{nodeId}/documents/{fileId}"); + + if (token is not null) + request.Headers.Authorization = new AuthenticationHeaderValue("Bearer", token); + if (apiKey is not null) + request.Headers.Add("X-Api-Key", apiKey); + + return request; + } + + private static async Task UploadAndGetFileIdAsync(HttpClient client, StartResponse session) + { + var response = await client.SendAsync( + UploadRequest(session.SessionId, session.CurrentNode!.Id, session.ApplicantToken)); + response.EnsureSuccessStatusCode(); + + var stored = await response.Content.ReadFromJsonAsync>(); + return stored!.Single().FileId; + } + + // ── Upload ───────────────────────────────────────────────────────────────── + + [Fact] + public async Task Upload_WithATokenForAnotherSession_IsRefused() + { + using var factory = CreateFactory(); + using var operatorClient = CreateOperatorClient(factory); + var flowId = await CreateDocumentFlowAsync(operatorClient); + + using var browser = factory.CreateClient(); + var mine = await StartSessionAsync(browser, flowId); + var theirs = await StartSessionAsync(browser, flowId); + + // My credential, their session id in the route. + var response = await browser.SendAsync( + UploadRequest(theirs.SessionId, theirs.CurrentNode!.Id, mine.ApplicantToken)); + + Assert.Equal(HttpStatusCode.Forbidden, response.StatusCode); + } + + [Fact] + public async Task Upload_WithATokenForAnotherSession_WritesNothingIntoThatSession() + { + using var factory = CreateFactory(); + using var operatorClient = CreateOperatorClient(factory); + var flowId = await CreateDocumentFlowAsync(operatorClient); + + using var browser = factory.CreateClient(); + var mine = await StartSessionAsync(browser, flowId); + var theirs = await StartSessionAsync(browser, flowId); + + await browser.SendAsync(UploadRequest(theirs.SessionId, theirs.CurrentNode!.Id, mine.ApplicantToken)); + + // The refusal has to happen before the write. A rejected upload that still lands a + // Submission row shows up in the operator console as the real applicant's document. + var submissions = await operatorClient.GetFromJsonAsync>( + $"/api/workflow/sessions/{theirs.SessionId}/submissions"); + + Assert.Empty(submissions!); + } + + [Fact] + public async Task Upload_ToMyOwnSession_Succeeds() + { + using var factory = CreateFactory(); + using var operatorClient = CreateOperatorClient(factory); + var flowId = await CreateDocumentFlowAsync(operatorClient); + + using var browser = factory.CreateClient(); + var mine = await StartSessionAsync(browser, flowId); + + var response = await browser.SendAsync( + UploadRequest(mine.SessionId, mine.CurrentNode!.Id, mine.ApplicantToken)); + + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + } + + [Fact] + public async Task Upload_ToAnAbandonedSession_IsRefused() + { + using var factory = CreateFactory(); + using var operatorClient = CreateOperatorClient(factory); + var flowId = await CreateDocumentFlowAsync(operatorClient); + + using var browser = factory.CreateClient(); + var mine = await StartSessionAsync(browser, flowId); + + var abandon = new HttpRequestMessage(HttpMethod.Delete, $"/api/workflow/sessions/{mine.SessionId}"); + abandon.Headers.Authorization = new AuthenticationHeaderValue("Bearer", mine.ApplicantToken); + (await browser.SendAsync(abandon)).EnsureSuccessStatusCode(); + + // Consistent with the terminal-status rule: a finished journey accepts no more writes. + var response = await browser.SendAsync( + UploadRequest(mine.SessionId, mine.CurrentNode!.Id, mine.ApplicantToken)); + + Assert.Equal(HttpStatusCode.Forbidden, response.StatusCode); + } + + [Fact] + public async Task Upload_ByAnOperator_ReachesAnySession() + { + using var factory = CreateFactory(); + using var operatorClient = CreateOperatorClient(factory); + var flowId = await CreateDocumentFlowAsync(operatorClient); + + using var browser = factory.CreateClient(); + var applicantSession = await StartSessionAsync(browser, flowId); + + var response = await operatorClient.SendAsync(UploadRequest( + applicantSession.SessionId, applicantSession.CurrentNode!.Id, token: null, apiKey: OperatorApiKey)); + + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + } + + // ── Download ─────────────────────────────────────────────────────────────── + + [Fact] + public async Task Download_OfMyOwnDocument_Succeeds() + { + using var factory = CreateFactory(); + using var operatorClient = CreateOperatorClient(factory); + var flowId = await CreateDocumentFlowAsync(operatorClient); + + using var browser = factory.CreateClient(); + var mine = await StartSessionAsync(browser, flowId); + var fileId = await UploadAndGetFileIdAsync(browser, mine); + + var response = await browser.SendAsync( + DownloadRequest(mine.SessionId, mine.CurrentNode!.Id, fileId, mine.ApplicantToken)); + + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + } + + [Fact] + public async Task Download_WithATokenForAnotherSession_IsRefused() + { + using var factory = CreateFactory(); + using var operatorClient = CreateOperatorClient(factory); + var flowId = await CreateDocumentFlowAsync(operatorClient); + + using var browser = factory.CreateClient(); + var mine = await StartSessionAsync(browser, flowId); + var theirs = await StartSessionAsync(browser, flowId); + var theirFileId = await UploadAndGetFileIdAsync(browser, theirs); + + // Their session id in the route, my credential. + var response = await browser.SendAsync( + DownloadRequest(theirs.SessionId, theirs.CurrentNode!.Id, theirFileId, mine.ApplicantToken)); + + Assert.Equal(HttpStatusCode.Forbidden, response.StatusCode); + } + + [Fact] + public async Task Download_OfAnotherSessionsFile_ThroughMyOwnSessionRoute_IsRefused() + { + using var factory = CreateFactory(); + using var operatorClient = CreateOperatorClient(factory); + var flowId = await CreateDocumentFlowAsync(operatorClient); + + using var browser = factory.CreateClient(); + var mine = await StartSessionAsync(browser, flowId); + var theirs = await StartSessionAsync(browser, flowId); + var theirFileId = await UploadAndGetFileIdAsync(browser, theirs); + + // The ownership check passes - this is my own session - so only scoping the lookup to the + // route can stop it. This is the case an authorization check alone would miss. + var response = await browser.SendAsync( + DownloadRequest(mine.SessionId, mine.CurrentNode!.Id, theirFileId, mine.ApplicantToken)); + + Assert.Equal(HttpStatusCode.NotFound, response.StatusCode); + } + + [Fact] + public async Task Download_OfAFileIdThatWasNeverUploaded_Returns404() + { + using var factory = CreateFactory(); + using var operatorClient = CreateOperatorClient(factory); + var flowId = await CreateDocumentFlowAsync(operatorClient); + + using var browser = factory.CreateClient(); + var mine = await StartSessionAsync(browser, flowId); + + var response = await browser.SendAsync(DownloadRequest( + mine.SessionId, mine.CurrentNode!.Id, Guid.NewGuid().ToString(), mine.ApplicantToken)); + + // 404 rather than 403: a 403 would confirm the id names a real document. + Assert.Equal(HttpStatusCode.NotFound, response.StatusCode); + } + + [Fact] + public async Task Download_NeverReachesStorage_WhenTheFileIsNotRecordedAgainstTheSession() + { + using var factory = CreateFactory(); + var storage = (RecordingDocumentStorageService)factory.Services.GetRequiredService(); + + using var operatorClient = CreateOperatorClient(factory); + var flowId = await CreateDocumentFlowAsync(operatorClient); + + using var browser = factory.CreateClient(); + var mine = await StartSessionAsync(browser, flowId); + var theirs = await StartSessionAsync(browser, flowId); + var theirFileId = await UploadAndGetFileIdAsync(browser, theirs); + + storage.Reads.Clear(); + + await browser.SendAsync( + DownloadRequest(mine.SessionId, mine.CurrentNode!.Id, theirFileId, mine.ApplicantToken)); + + // The bytes must never be fetched, not merely withheld after the fact. + Assert.Empty(storage.Reads); + } + + [Fact] + public async Task Download_ByAnOperator_ReachesAnySessionsDocument() + { + using var factory = CreateFactory(); + using var operatorClient = CreateOperatorClient(factory); + var flowId = await CreateDocumentFlowAsync(operatorClient); + + using var browser = factory.CreateClient(); + var applicantSession = await StartSessionAsync(browser, flowId); + var fileId = await UploadAndGetFileIdAsync(browser, applicantSession); + + var response = await operatorClient.SendAsync(DownloadRequest( + applicantSession.SessionId, applicantSession.CurrentNode!.Id, fileId, token: null, apiKey: OperatorApiKey)); + + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + } + + /// + /// Stores in memory and records every read, so a test can assert the bytes were never fetched + /// rather than only that the response withheld them. + /// + private sealed class RecordingDocumentStorageService : IDocumentStorageService + { + private readonly Dictionary _files = []; + + public List Reads { get; } = []; + + public Task StoreAsync(Stream stream, string fileName, string contentType, CancellationToken cancellationToken = default) + { + var info = new StoredFileInfo(Guid.NewGuid().ToString(), fileName, contentType, stream.Length, DateTimeOffset.UtcNow); + _files[info.FileId] = info; + return Task.FromResult(info); + } + + public Task<(Stream Stream, StoredFileInfo Info)> GetStreamAsync(string fileId, CancellationToken cancellationToken = default) + { + Reads.Add(fileId); + + if (!_files.TryGetValue(fileId, out var info)) + throw new FileNotFoundException($"No stored file '{fileId}'."); + + return Task.FromResult<(Stream, StoredFileInfo)>((new MemoryStream("hello"u8.ToArray()), info)); + } + + public Task ScanAsync(string fileId, CancellationToken cancellationToken = default) + => Task.FromResult(new ScanResult(true, null)); + + public Task DeleteAsync(string fileId, CancellationToken cancellationToken = default) + { + _files.Remove(fileId); + return Task.CompletedTask; + } + + public Task> ListOlderThanAsync(DateTimeOffset threshold, CancellationToken cancellationToken = default) + => Task.FromResult>([]); + } +} diff --git a/src/backend/OpenOnboarding.Application.Tests/TestWebAppFactory.cs b/src/backend/OpenOnboarding.Application.Tests/TestWebAppFactory.cs index 7a11130..dae868c 100644 --- a/src/backend/OpenOnboarding.Application.Tests/TestWebAppFactory.cs +++ b/src/backend/OpenOnboarding.Application.Tests/TestWebAppFactory.cs @@ -20,10 +20,15 @@ internal static class TestWebAppFactory /// Defaults to Testing, which disables rate limiting so unrelated tests are not throttled. /// Pass Development to exercise the real limiters. /// + /// + /// Extra test doubles to register, applied after the DbContext swap so it can replace anything + /// the application registered - document storage, for instance, which otherwise writes to disk. + /// public static WebApplicationFactory Create( string? dbName = null, IReadOnlyDictionary? configurationOverrides = null, - string environment = "Testing") + string environment = "Testing", + Action? configureServices = null) { // A shared InMemoryDatabaseRoot ensures all DbContext instances across DI scopes // (startup seed scope, test seed scope, request scope) read from the same store. @@ -79,6 +84,8 @@ public static WebApplicationFactory Create( services.AddDbContext(options => options.UseInMemoryDatabase(resolvedDbName, dbRoot)); + + configureServices?.Invoke(services); }); }); } diff --git a/src/backend/OpenOnboarding.Application/Interfaces/IWorkflowService.cs b/src/backend/OpenOnboarding.Application/Interfaces/IWorkflowService.cs index a28b421..a837ee1 100644 --- a/src/backend/OpenOnboarding.Application/Interfaces/IWorkflowService.cs +++ b/src/backend/OpenOnboarding.Application/Interfaces/IWorkflowService.cs @@ -9,7 +9,19 @@ public interface IWorkflowService Task GetNextStepAsync(Guid sessionId, CancellationToken cancellationToken = default); Task AbandonSessionAsync(Guid sessionId, CancellationToken cancellationToken = default); Task> UploadDocumentsAsync(Guid sessionId, Guid nodeId, IReadOnlyList files, long maxFileSizeBytes, CancellationToken cancellationToken = default); - Task<(Stream Stream, StoredFileInfo Info)> GetDocumentAsync(string fileId, CancellationToken cancellationToken = default); + /// + /// Reads a document stored against a specific session and node. + /// + /// The session and node are part of the lookup, not decoration: a file id alone used to resolve + /// straight from storage, so any caller holding any id could read any applicant's document. The + /// signature takes them so that path cannot be called at all. + /// + /// + /// + /// The file is not recorded against this session and node - including when it exists and belongs + /// to another session. Not found rather than forbidden: a refusal would confirm the id is real. + /// + Task<(Stream Stream, StoredFileInfo Info)> GetDocumentAsync(Guid sessionId, Guid nodeId, string fileId, CancellationToken cancellationToken = default); } public record DocumentUploadItem(Stream Stream, string FileName, string ContentType, long Length); diff --git a/src/backend/OpenOnboarding.Application/Queries/GetDocumentQuery.cs b/src/backend/OpenOnboarding.Application/Queries/GetDocumentQuery.cs index 3e36157..60f0876 100644 --- a/src/backend/OpenOnboarding.Application/Queries/GetDocumentQuery.cs +++ b/src/backend/OpenOnboarding.Application/Queries/GetDocumentQuery.cs @@ -1,4 +1,9 @@ using MediatR; using OpenOnboarding.Application.Interfaces; namespace OpenOnboarding.Application.Queries; -public record GetDocumentQuery(string FileId) : IRequest<(Stream Stream, StoredFileInfo Info)>; + +/// +/// Reads one document. The session and node scope the lookup - a file id on its own is not enough +/// to identify a document a caller is entitled to. +/// +public record GetDocumentQuery(Guid SessionId, Guid NodeId, string FileId) : IRequest<(Stream Stream, StoredFileInfo Info)>; diff --git a/src/backend/OpenOnboarding.Infrastructure/QueryHandlers/GetDocumentQueryHandler.cs b/src/backend/OpenOnboarding.Infrastructure/QueryHandlers/GetDocumentQueryHandler.cs index bf33ad6..471ef3f 100644 --- a/src/backend/OpenOnboarding.Infrastructure/QueryHandlers/GetDocumentQueryHandler.cs +++ b/src/backend/OpenOnboarding.Infrastructure/QueryHandlers/GetDocumentQueryHandler.cs @@ -1,11 +1,76 @@ +using System.Text.Json; using MediatR; +using Microsoft.EntityFrameworkCore; +using OpenOnboarding.Application.Exceptions; using OpenOnboarding.Application.Interfaces; using OpenOnboarding.Application.Queries; +using OpenOnboarding.Infrastructure.Persistence; namespace OpenOnboarding.Infrastructure.QueryHandlers; -internal sealed class GetDocumentQueryHandler(IDocumentStorageService documentStorageService) : IRequestHandler +/// +/// Resolves a document only when it is recorded against the session and node named in the query. +/// +/// This previously passed the file id straight to storage, so the session and node the caller had +/// been authorised for played no part in the lookup. Any caller holding any id could read any +/// applicant's document. An ownership check on the endpoint alone does not close that: the owner of +/// one session could still name their own session in the route and ask for someone else's file. +/// +/// +internal sealed class GetDocumentQueryHandler( + OnboardingDbContext dbContext, + IDocumentStorageService documentStorageService) + : IRequestHandler { - public Task<(Stream Stream, StoredFileInfo Info)> Handle(GetDocumentQuery query, CancellationToken cancellationToken) - => documentStorageService.GetStreamAsync(query.FileId, cancellationToken); + private static readonly JsonSerializerOptions JsonOptions = new(JsonSerializerDefaults.Web); + + public async Task<(Stream Stream, StoredFileInfo Info)> Handle( + GetDocumentQuery query, + CancellationToken cancellationToken) + { + if (!await IsRecordedAgainstAsync(query, cancellationToken)) + { + // Not found rather than forbidden: distinguishing "not yours" from "does not exist" + // confirms the id names a real document. + throw new NotFoundException( + $"Document '{query.FileId}' was not found for session '{query.SessionId}'."); + } + + return await documentStorageService.GetStreamAsync(query.FileId, cancellationToken); + } + + /// + /// Whether an upload recorded for this session and node carries the file id. + /// + /// Document uploads are written as a Submission whose DataJson holds the stored + /// file descriptors, so the session-to-file link already exists; it was simply never consulted. + /// + /// + private async Task IsRecordedAgainstAsync(GetDocumentQuery query, CancellationToken cancellationToken) + { + var submissions = await dbContext.Submissions + .AsNoTracking() + .Where(submission => submission.SessionId == query.SessionId && submission.NodeId == query.NodeId) + .Select(submission => submission.DataJson) + .ToListAsync(cancellationToken); + + return submissions.Any(dataJson => ContainsFileId(dataJson, query.FileId)); + } + + private static bool ContainsFileId(string dataJson, string fileId) + { + try + { + // A form submission stores an object rather than an array of files; those simply do not + // match, which is correct - no document was uploaded there. + var stored = JsonSerializer.Deserialize>(dataJson, JsonOptions); + + return stored is not null + && stored.Any(file => string.Equals(file.FileId, fileId, StringComparison.Ordinal)); + } + catch (JsonException) + { + return false; + } + } } diff --git a/src/backend/OpenOnboarding.Infrastructure/Services/WorkflowService.cs b/src/backend/OpenOnboarding.Infrastructure/Services/WorkflowService.cs index eec4cd8..3c3288c 100644 --- a/src/backend/OpenOnboarding.Infrastructure/Services/WorkflowService.cs +++ b/src/backend/OpenOnboarding.Infrastructure/Services/WorkflowService.cs @@ -27,6 +27,6 @@ public Task AbandonSessionAsync(Guid sessionId, Cancellatio public Task> UploadDocumentsAsync(Guid sessionId, Guid nodeId, IReadOnlyList files, long maxFileSizeBytes, CancellationToken cancellationToken = default) => mediator.Send(new UploadDocumentsCommand(sessionId, nodeId, files, maxFileSizeBytes), cancellationToken); - public Task<(Stream Stream, StoredFileInfo Info)> GetDocumentAsync(string fileId, CancellationToken cancellationToken = default) - => mediator.Send(new GetDocumentQuery(fileId), cancellationToken); + public Task<(Stream Stream, StoredFileInfo Info)> GetDocumentAsync(Guid sessionId, Guid nodeId, string fileId, CancellationToken cancellationToken = default) + => mediator.Send(new GetDocumentQuery(sessionId, nodeId, fileId), cancellationToken); }