From 92d1681dc6c27b896e61fa9e5c49fe6b7fcc3c38 Mon Sep 17 00:00:00 2001 From: MaximumTrainer Date: Sun, 13 Sep 2026 09:50:17 +0100 Subject: [PATCH 1/5] fix(api): partition every rate limit by caller Each policy was a single limiter shared by all callers, so one client exhausting a budget locked out every other. On session-start - anonymous by design and the entry point of every journey - 100 requests a minute denied service to all applicants. Each policy now partitions: session-start by client address, analytics-ingest by the applicant session in the token, the operator policies by principal. Keys are prefixed by kind so an address cannot collide with a session id. X-Forwarded-For is honoured only from a configured proxy, since the connection address behind a load balancer is the balancer's and would collapse every caller into one partition again. The ASP.NET loopback defaults are cleared: nothing is trusted until it is named. A global ceiling runs alongside the per-caller policies so a distributed flood still has a bound. The runbook claimed limits were already per IP. It now describes the real partitioning, the proxy configuration, and that counters are per-instance rather than cluster-wide. Co-Authored-By: Claude Opus 5 (1M context) --- docs/runbook.md | 47 ++- src/backend/OpenOnboarding.Api/Program.cs | 96 +++-- .../RateLimiting/RateLimitPartitionKeys.cs | 56 +++ .../OpenOnboarding.Api/appsettings.json | 8 +- .../RateLimitPartitioningTests.cs | 360 ++++++++++++++++++ .../TestHelpers/ClientAddressStartupFilter.cs | 38 ++ .../TestWebAppFactory.cs | 7 + 7 files changed, 574 insertions(+), 38 deletions(-) create mode 100644 src/backend/OpenOnboarding.Api/RateLimiting/RateLimitPartitionKeys.cs create mode 100644 src/backend/OpenOnboarding.Application.Tests/RateLimitPartitioningTests.cs create mode 100644 src/backend/OpenOnboarding.Application.Tests/TestHelpers/ClientAddressStartupFilter.cs diff --git a/docs/runbook.md b/docs/runbook.md index d6b9614..86ab114 100644 --- a/docs/runbook.md +++ b/docs/runbook.md @@ -429,4 +429,49 @@ All other state is in PostgreSQL — safe for horizontal scaling. ### Rate Limits -Default rate limit: **100 requests per minute per IP** (configurable). Adjust in `Program.cs` `AddRateLimiter()`. +Every policy is **partitioned per caller**, so one client exhausting its budget does not affect any +other. Configure the limits under `RateLimiting` in `appsettings.json` or as +`RateLimiting__` environment variables. + +| Policy | Endpoint | Partitioned by | Setting | Default (per minute) | +| --- | --- | --- | --- | --- | +| `session-start` | `POST /api/workflow/sessions/start` | Client IP — the endpoint is anonymous by design | `RateLimiting:SessionStartPerMinute` | 100 | +| `analytics-ingest` | `POST /api/analytics/events` | Applicant session id from the token; operators fall back to their principal | `RateLimiting:AnalyticsIngestPerMinute` | 120 | +| `webhook-registration` | `POST /api/flows/{flowId}/webhooks` | Authenticated principal, falling back to client IP | `RateLimiting:WebhookRegistrationPerMinute` | 20 | +| `general` | declared, not yet attached to an endpoint | Authenticated principal, falling back to client IP | `RateLimiting:GeneralPerMinute` | 300 | +| global ceiling | every request | nothing — one bucket for the whole instance | `RateLimiting:GlobalCeilingPerMinute` | 3000 | + +The global ceiling runs **in addition to** the endpoint policy, so a flood spread across thousands +of partitions still has a bound. A request rejected by either returns `429` with `Retry-After: 60`. + +#### Behind a proxy + +The partition key is the connection address, which behind a load balancer is the balancer's own +address — every caller would land in one partition again. `X-Forwarded-For` corrects this, but only +from a proxy this API trusts; a header from anywhere else is an anonymous caller choosing their own +partition key, and is ignored. + +**Nothing is trusted by default.** The ASP.NET defaults (loopback) are cleared at startup, so +`X-Forwarded-For` has no effect until you name your proxy: + +```jsonc +"ForwardedHeaders": { + "KnownProxies": ["10.1.2.3"], // individual proxy addresses + "KnownNetworks": ["10.0.0.0/8"], // or CIDR ranges + "ForwardLimit": 1 // hops to walk back; raise only if you have chained proxies +} +``` + +If you deploy behind an ingress and leave this unset, every request partitions by the ingress +address and the limits behave as global ones. Set it. + +#### Multi-replica behaviour + +**Limits are per-instance, not cluster-wide.** The limiters hold their counters in process memory, +so with *N* replicas a caller's effective budget is up to *N* × the configured value, depending on +which instance each request lands on. This is accepted deliberately, for the same reason as the SSE +fan-out design: the alternative is a shared counter store on the path of every request. + +Size the limits accordingly — divide the budget you actually want by your replica count — and treat +these as a coarse abuse bound, not a precise quota. A cluster-wide limit belongs at the ingress or +in a shared store; if you need one, that is where to put it. diff --git a/src/backend/OpenOnboarding.Api/Program.cs b/src/backend/OpenOnboarding.Api/Program.cs index 44603b5..b909d03 100644 --- a/src/backend/OpenOnboarding.Api/Program.cs +++ b/src/backend/OpenOnboarding.Api/Program.cs @@ -5,14 +5,18 @@ using Microsoft.AspNetCore.Authorization; using Microsoft.AspNetCore.Diagnostics; using Microsoft.AspNetCore.Mvc; +using Microsoft.AspNetCore.HttpOverrides; using Microsoft.AspNetCore.RateLimiting; using Microsoft.EntityFrameworkCore; using Microsoft.OpenApi.Models; +using System.Net; using System.Reflection; using System.Text.Json.Serialization; +using System.Threading.RateLimiting; using OpenOnboarding.Api.Authentication; using OpenOnboarding.Api.Authorization; using OpenOnboarding.Api.Configuration; +using OpenOnboarding.Api.RateLimiting; using OpenOnboarding.Application.Exceptions; using OpenOnboarding.Infrastructure.DependencyInjection; using OpenOnboarding.Infrastructure.Persistence; @@ -185,6 +189,30 @@ var environment = builder.Environment.EnvironmentName; var rateLimitingEnabled = !string.Equals(environment, "Testing", StringComparison.OrdinalIgnoreCase); +// Behind a proxy the connection address is the load balancer's, so every caller would land in one +// rate limit partition. X-Forwarded-For fixes that, but only from a proxy we trust - a header from +// anywhere else is an unauthenticated caller choosing their own partition key. The ASP.NET +// defaults (loopback) are cleared so nothing is trusted unless it is configured here. +builder.Services.Configure(options => +{ + options.ForwardedHeaders = ForwardedHeaders.XForwardedFor | ForwardedHeaders.XForwardedProto; + options.ForwardLimit = builder.Configuration.GetValue("ForwardedHeaders:ForwardLimit") ?? 1; + options.KnownProxies.Clear(); + options.KnownIPNetworks.Clear(); + + foreach (var proxy in builder.Configuration.GetSection("ForwardedHeaders:KnownProxies").Get() ?? []) + { + if (IPAddress.TryParse(proxy, out var address)) + options.KnownProxies.Add(address); + } + + foreach (var network in builder.Configuration.GetSection("ForwardedHeaders:KnownNetworks").Get() ?? []) + { + if (System.Net.IPNetwork.TryParse(network, out var parsed)) + options.KnownIPNetworks.Add(parsed); + } +}); + builder.Services.AddRateLimiter(options => { options.RejectionStatusCode = StatusCodes.Status429TooManyRequests; @@ -204,52 +232,45 @@ await context.HttpContext.Response.WriteAsJsonAsync(new ProblemDetails var sessionStartLimit = builder.Configuration.GetValue("RateLimiting:SessionStartPerMinute", 100); var webhookRegLimit = builder.Configuration.GetValue("RateLimiting:WebhookRegistrationPerMinute", 20); var generalLimit = builder.Configuration.GetValue("RateLimiting:GeneralPerMinute", 300); + var analyticsIngestLimit = builder.Configuration.GetValue("RateLimiting:AnalyticsIngestPerMinute", 120); + var globalCeiling = builder.Configuration.GetValue("RateLimiting:GlobalCeilingPerMinute", 3000); - options.AddSlidingWindowLimiter("session-start", opt => + static SlidingWindowRateLimiterOptions PerMinute(int permitLimit) => new() { - opt.Window = TimeSpan.FromMinutes(1); - opt.SegmentsPerWindow = 4; - opt.PermitLimit = sessionStartLimit; - opt.QueueProcessingOrder = System.Threading.RateLimiting.QueueProcessingOrder.OldestFirst; - opt.QueueLimit = 0; - }); + Window = TimeSpan.FromMinutes(1), + SegmentsPerWindow = 4, + PermitLimit = permitLimit, + QueueProcessingOrder = QueueProcessingOrder.OldestFirst, + QueueLimit = 0 + }; - options.AddSlidingWindowLimiter("webhook-registration", opt => - { - opt.Window = TimeSpan.FromMinutes(1); - opt.SegmentsPerWindow = 4; - opt.PermitLimit = webhookRegLimit; - opt.QueueProcessingOrder = System.Threading.RateLimiting.QueueProcessingOrder.OldestFirst; - opt.QueueLimit = 0; - }); + // Each policy is partitioned by whoever the request belongs to. AddSlidingWindowLimiter + // would build a single limiter shared by every caller, which is what made one client able + // to lock out the rest. + options.AddPolicy("session-start", context => RateLimitPartition.GetSlidingWindowLimiter( + RateLimitPartitionKeys.ClientAddress(context), _ => PerMinute(sessionStartLimit))); - var analyticsIngestLimit = builder.Configuration.GetValue("RateLimiting:AnalyticsIngestPerMinute", 120); + options.AddPolicy("analytics-ingest", context => RateLimitPartition.GetSlidingWindowLimiter( + RateLimitPartitionKeys.ApplicantSession(context), _ => PerMinute(analyticsIngestLimit))); - options.AddSlidingWindowLimiter("analytics-ingest", opt => - { - opt.Window = TimeSpan.FromMinutes(1); - opt.SegmentsPerWindow = 4; - opt.PermitLimit = analyticsIngestLimit; - opt.QueueProcessingOrder = System.Threading.RateLimiting.QueueProcessingOrder.OldestFirst; - opt.QueueLimit = 0; - }); + options.AddPolicy("webhook-registration", context => RateLimitPartition.GetSlidingWindowLimiter( + RateLimitPartitionKeys.Principal(context), _ => PerMinute(webhookRegLimit))); - options.AddSlidingWindowLimiter("general", opt => - { - opt.Window = TimeSpan.FromMinutes(1); - opt.SegmentsPerWindow = 4; - opt.PermitLimit = generalLimit; - opt.QueueProcessingOrder = System.Threading.RateLimiting.QueueProcessingOrder.OldestFirst; - opt.QueueLimit = 0; - }); + options.AddPolicy("general", context => RateLimitPartition.GetSlidingWindowLimiter( + RateLimitPartitionKeys.Principal(context), _ => PerMinute(generalLimit))); + + // A ceiling over every partition, so a flood spread across many callers still has a bound. + // It runs in addition to the endpoint policy, not instead of it. + options.GlobalLimiter = PartitionedRateLimiter.Create( + _ => RateLimitPartition.GetSlidingWindowLimiter("global", _ => PerMinute(globalCeiling))); } else { // Testing: add NoLimiter policies so attributes don't error - options.AddPolicy("session-start", _ => System.Threading.RateLimiting.RateLimitPartition.GetNoLimiter("testing")); - options.AddPolicy("webhook-registration", _ => System.Threading.RateLimiting.RateLimitPartition.GetNoLimiter("testing")); - options.AddPolicy("general", _ => System.Threading.RateLimiting.RateLimitPartition.GetNoLimiter("testing")); - options.AddPolicy("analytics-ingest", _ => System.Threading.RateLimiting.RateLimitPartition.GetNoLimiter("testing")); + options.AddPolicy("session-start", _ => RateLimitPartition.GetNoLimiter("testing")); + options.AddPolicy("webhook-registration", _ => RateLimitPartition.GetNoLimiter("testing")); + options.AddPolicy("general", _ => RateLimitPartition.GetNoLimiter("testing")); + options.AddPolicy("analytics-ingest", _ => RateLimitPartition.GetNoLimiter("testing")); } }); @@ -337,6 +358,9 @@ await context.Response.WriteAsJsonAsync(new ProblemDetails }); }); +// First in the pipeline: everything downstream that reads the client address - the rate limit +// partitioner above all - must see the real caller, not the proxy in front of it. +app.UseForwardedHeaders(); app.UseHttpsRedirection(); app.UseCors("FrontendDev"); app.UseMiddleware(); diff --git a/src/backend/OpenOnboarding.Api/RateLimiting/RateLimitPartitionKeys.cs b/src/backend/OpenOnboarding.Api/RateLimiting/RateLimitPartitionKeys.cs new file mode 100644 index 0000000..ca70791 --- /dev/null +++ b/src/backend/OpenOnboarding.Api/RateLimiting/RateLimitPartitionKeys.cs @@ -0,0 +1,56 @@ +using System.Security.Claims; +using OpenOnboarding.Api.Authentication; + +namespace OpenOnboarding.Api.RateLimiting; + +/// +/// Derives the key that decides whose budget a request spends. +/// +/// A rate limit without a partition key is a shared bucket: one caller exhausting it locks out +/// every other caller. That is a denial of service against session-start in particular, +/// which is anonymous by design and is where every journey begins. +/// +/// +/// Every key is prefixed by the kind of thing it names, so an address can never collide with a +/// session id or a principal id that happens to have the same text. +/// +/// +public static class RateLimitPartitionKeys +{ + /// Key used when a request has no address at all - a unix socket or an in-process test host. + public const string Unknown = "addr:unknown"; + + /// + /// The caller's network address, after UseForwardedHeaders has resolved + /// X-Forwarded-For from a trusted proxy. A header from anywhere else is ignored, so a + /// caller cannot rotate it to mint fresh budgets. + /// + public static string ClientAddress(HttpContext context) + { + var address = context.Connection.RemoteIpAddress; + return address is null ? Unknown : $"addr:{address}"; + } + + /// + /// The authenticated principal, falling back to the client address while a request is still + /// anonymous - the limiter runs before authorization, so unauthenticated calls reach it too. + /// + public static string Principal(HttpContext context) + { + var id = context.User.FindFirstValue(ClaimTypes.NameIdentifier) + ?? context.User.Identity?.Name; + + return string.IsNullOrWhiteSpace(id) ? ClientAddress(context) : $"user:{id}"; + } + + /// + /// The applicant session the caller's token names, so one applicant's browser cannot throttle + /// another's. Operators carry no session claim and fall back to their own principal. + /// + public static string ApplicantSession(HttpContext context) + { + var sessionId = context.User.FindFirstValue(ApplicantSessionAuthenticationDefaults.SessionIdClaim); + + return string.IsNullOrWhiteSpace(sessionId) ? Principal(context) : $"session:{sessionId}"; + } +} diff --git a/src/backend/OpenOnboarding.Api/appsettings.json b/src/backend/OpenOnboarding.Api/appsettings.json index ec525a5..bed6534 100644 --- a/src/backend/OpenOnboarding.Api/appsettings.json +++ b/src/backend/OpenOnboarding.Api/appsettings.json @@ -37,7 +37,13 @@ "SessionStartPerMinute": 100, "WebhookRegistrationPerMinute": 20, "GeneralPerMinute": 300, - "AnalyticsIngestPerMinute": 120 + "AnalyticsIngestPerMinute": 120, + "GlobalCeilingPerMinute": 3000 + }, + "ForwardedHeaders": { + "KnownProxies": [], + "KnownNetworks": [], + "ForwardLimit": 1 }, "VirusScan": { "Enabled": false, diff --git a/src/backend/OpenOnboarding.Application.Tests/RateLimitPartitioningTests.cs b/src/backend/OpenOnboarding.Application.Tests/RateLimitPartitioningTests.cs new file mode 100644 index 0000000..b117d37 --- /dev/null +++ b/src/backend/OpenOnboarding.Application.Tests/RateLimitPartitioningTests.cs @@ -0,0 +1,360 @@ +using System.Net; +using System.Net.Http.Headers; +using System.Net.Http.Json; +using System.Security.Claims; +using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Mvc.Testing; +using OpenOnboarding.Api.Authentication; +using OpenOnboarding.Api.RateLimiting; +using OpenOnboarding.Application.Tests.TestHelpers; + +namespace OpenOnboarding.Application.Tests; + +/// +/// A rate limit is only a defence if it bounds the caller who is misbehaving. When every caller +/// shares one bucket the limit becomes the attack: 100 requests a minute to session-start +/// stops every applicant from beginning a journey. +/// +/// These run in the Development environment because the Testing environment swaps every limiter for +/// a no-op. +/// +/// +public sealed class RateLimitPartitioningTests +{ + private const int Limit = 3; + + private const string ClientA = "198.51.100.10"; + private const string ClientB = "198.51.100.20"; + private const string TrustedProxy = "10.1.2.3"; + private const string UntrustedHop = "203.0.113.77"; + + private sealed record StartResponse(Guid SessionId, string? ApplicantToken); + private sealed record FlowStub(Guid Id); + + private static WebApplicationFactory CreateFactory( + IReadOnlyDictionary? extraSettings = null) + { + var settings = new Dictionary + { + ["RateLimiting:SessionStartPerMinute"] = Limit.ToString(), + ["RateLimiting:AnalyticsIngestPerMinute"] = Limit.ToString(), + ["RateLimiting:WebhookRegistrationPerMinute"] = Limit.ToString(), + ["RateLimiting:GeneralPerMinute"] = "1000", + // High enough to stay out of the way of the per-partition assertions; the ceiling gets + // its own test. + ["RateLimiting:GlobalCeilingPerMinute"] = "1000" + }; + + if (extraSettings is not null) + { + foreach (var setting in extraSettings) + settings[setting.Key] = setting.Value; + } + + return TestWebAppFactory.Create(configurationOverrides: settings, environment: "Development"); + } + + private static HttpRequestMessage StartRequest(Guid flowId, string clientAddress, string? forwardedFor = null) + { + var request = new HttpRequestMessage(HttpMethod.Post, "/api/workflow/sessions/start") + { + Content = JsonContent.Create(new { flowId }) + }; + request.Headers.Add(ClientAddressStartupFilter.HeaderName, clientAddress); + + if (forwardedFor is not null) + request.Headers.Add("X-Forwarded-For", forwardedFor); + + return request; + } + + private static async Task CreateFlowAsync(HttpClient operatorClient) + { + var payload = new + { + name = $"Partition flow {Guid.NewGuid()}", + nodes = new[] + { + new { key = "start", type = "Form", title = "Start", isStartNode = true, jsonContent = "{\"fields\":[]}" } + }, + connections = Array.Empty() + }; + + var response = await operatorClient.PostAsJsonAsync("/api/flows", payload); + response.EnsureSuccessStatusCode(); + return (await response.Content.ReadFromJsonAsync())!.Id; + } + + private static async Task ExhaustSessionStartAsync( + HttpClient client, Guid flowId, string clientAddress, string? forwardedFor = null) + { + for (var i = 0; i < Limit; i++) + { + var response = await client.SendAsync(StartRequest(flowId, clientAddress, forwardedFor)); + Assert.NotEqual(HttpStatusCode.TooManyRequests, response.StatusCode); + } + + var overTheLimit = await client.SendAsync(StartRequest(flowId, clientAddress, forwardedFor)); + Assert.Equal(HttpStatusCode.TooManyRequests, overTheLimit.StatusCode); + } + + [Fact] + public async Task SessionStart_OneClientExhaustingItsBudget_DoesNotLockOutAnother() + { + using var factory = CreateFactory(); + using var operatorClient = factory.CreateClient(); + operatorClient.DefaultRequestHeaders.Add("X-Api-Key", "test-api-key"); + var flowId = await CreateFlowAsync(operatorClient); + + using var client = factory.CreateClient(); + await ExhaustSessionStartAsync(client, flowId, ClientA); + + var otherClient = await client.SendAsync(StartRequest(flowId, ClientB)); + + Assert.NotEqual(HttpStatusCode.TooManyRequests, otherClient.StatusCode); + } + + [Fact] + public async Task SessionStart_EachClient_ReceivesItsFullPermitLimit() + { + using var factory = CreateFactory(); + using var operatorClient = factory.CreateClient(); + operatorClient.DefaultRequestHeaders.Add("X-Api-Key", "test-api-key"); + var flowId = await CreateFlowAsync(operatorClient); + + using var client = factory.CreateClient(); + await ExhaustSessionStartAsync(client, flowId, ClientA); + + // The second client starts from a full budget, not from whatever the first one left. + await ExhaustSessionStartAsync(client, flowId, ClientB); + } + + [Fact] + public async Task SessionStart_BehindATrustedProxy_PartitionsByTheForwardedClientAddress() + { + using var factory = CreateFactory(new Dictionary + { + ["ForwardedHeaders:KnownProxies:0"] = TrustedProxy + }); + + using var operatorClient = factory.CreateClient(); + operatorClient.DefaultRequestHeaders.Add("X-Api-Key", "test-api-key"); + var flowId = await CreateFlowAsync(operatorClient); + + using var client = factory.CreateClient(); + + // Every request arrives from the proxy; only X-Forwarded-For tells the two callers apart. + await ExhaustSessionStartAsync(client, flowId, TrustedProxy, forwardedFor: ClientA); + + var otherClient = await client.SendAsync(StartRequest(flowId, TrustedProxy, forwardedFor: ClientB)); + + Assert.NotEqual(HttpStatusCode.TooManyRequests, otherClient.StatusCode); + } + + [Fact] + public async Task SessionStart_ForgedForwardedFor_FromAnUntrustedSource_CannotEvadeTheLimit() + { + using var factory = CreateFactory(new Dictionary + { + ["ForwardedHeaders:KnownProxies:0"] = TrustedProxy + }); + + using var operatorClient = factory.CreateClient(); + operatorClient.DefaultRequestHeaders.Add("X-Api-Key", "test-api-key"); + var flowId = await CreateFlowAsync(operatorClient); + + using var client = factory.CreateClient(); + await ExhaustSessionStartAsync(client, flowId, UntrustedHop, forwardedFor: ClientA); + + // A fresh header value from the same untrusted connection must not mint a fresh budget. + var rotated = await client.SendAsync(StartRequest(flowId, UntrustedHop, forwardedFor: ClientB)); + + Assert.Equal(HttpStatusCode.TooManyRequests, rotated.StatusCode); + } + + [Fact] + public async Task AnalyticsIngest_IsPartitionedBySession_SoOneApplicantCannotThrottleAnother() + { + using var factory = CreateFactory(new Dictionary + { + ["RateLimiting:SessionStartPerMinute"] = "1000" + }); + + using var operatorClient = factory.CreateClient(); + operatorClient.DefaultRequestHeaders.Add("X-Api-Key", "test-api-key"); + var flowId = await CreateFlowAsync(operatorClient); + + using var browser = factory.CreateClient(); + var first = await StartSessionAsync(browser, flowId); + var second = await StartSessionAsync(browser, flowId); + + for (var i = 0; i < Limit; i++) + { + var response = await browser.SendAsync(IngestRequest(first, flowId)); + Assert.Equal(HttpStatusCode.Accepted, response.StatusCode); + } + + var exhausted = await browser.SendAsync(IngestRequest(first, flowId)); + Assert.Equal(HttpStatusCode.TooManyRequests, exhausted.StatusCode); + + // The second applicant shares the client address but not the session. + var otherApplicant = await browser.SendAsync(IngestRequest(second, flowId)); + Assert.Equal(HttpStatusCode.Accepted, otherApplicant.StatusCode); + } + + [Fact] + public async Task WebhookRegistration_ExhaustedByOnePrincipal_StillAdmitsADifferentPartition() + { + using var factory = CreateFactory(); + using var client = factory.CreateClient(); + + for (var i = 0; i < Limit + 1; i++) + await client.SendAsync(WebhookRequest(ClientA, apiKey: "test-api-key")); + + var limited = await client.SendAsync(WebhookRequest(ClientA, apiKey: "test-api-key")); + Assert.Equal(HttpStatusCode.TooManyRequests, limited.StatusCode); + + // An anonymous caller from another address falls in a different partition, so it reaches + // authorization and is refused there - not by the exhausted operator limiter. + var anonymous = await client.SendAsync(WebhookRequest(ClientB, apiKey: null)); + + Assert.NotEqual(HttpStatusCode.TooManyRequests, anonymous.StatusCode); + } + + [Fact] + public async Task GlobalCeiling_BoundsAFloodSpreadAcrossManyPartitions() + { + const int ceiling = 5; + + using var factory = CreateFactory(new Dictionary + { + // Per-caller budgets far above the ceiling, so only the ceiling can reject. + ["RateLimiting:SessionStartPerMinute"] = "1000", + ["RateLimiting:GlobalCeilingPerMinute"] = ceiling.ToString() + }); + + using var client = factory.CreateClient(); + + var statuses = new List(); + for (var i = 0; i < ceiling + 3; i++) + { + // A different caller each time: nothing but the global ceiling is shared. + var response = await client.SendAsync(StartRequest(Guid.NewGuid(), $"198.51.100.{100 + i}")); + statuses.Add(response.StatusCode); + } + + Assert.Contains(HttpStatusCode.TooManyRequests, statuses); + } + + [Theory] + [InlineData("session-start")] + [InlineData("analytics-ingest")] + [InlineData("webhook-registration")] + [InlineData("general")] + public void EveryPolicy_DerivesADistinctKey_ForTwoDifferentCallers(string policy) + { + Func partitioner = policy switch + { + "session-start" => RateLimitPartitionKeys.ClientAddress, + "analytics-ingest" => RateLimitPartitionKeys.ApplicantSession, + _ => RateLimitPartitionKeys.Principal + }; + + var first = partitioner(CallerContext(ClientA, Guid.NewGuid(), "operator-one")); + var second = partitioner(CallerContext(ClientB, Guid.NewGuid(), "operator-two")); + + Assert.NotEqual(first, second); + } + + [Fact] + public void PartitionKeys_NeverCollideAcrossKinds() + { + // A principal id that reads exactly like an address must not spend the address budget. + var context = CallerContext(ClientA, sessionId: null, principal: ClientA); + + Assert.NotEqual(RateLimitPartitionKeys.ClientAddress(context), RateLimitPartitionKeys.Principal(context)); + } + + [Fact] + public void ApplicantSession_WithoutASessionClaim_FallsBackToThePrincipal() + { + var context = CallerContext(ClientA, sessionId: null, principal: "operator-one"); + + Assert.Equal( + RateLimitPartitionKeys.Principal(context), + RateLimitPartitionKeys.ApplicantSession(context)); + } + + [Fact] + public void Principal_WithoutACredential_FallsBackToTheClientAddress() + { + var context = CallerContext(ClientA, sessionId: null, principal: null); + + Assert.Equal( + RateLimitPartitionKeys.ClientAddress(context), + RateLimitPartitionKeys.Principal(context)); + } + + private static HttpRequestMessage WebhookRequest(string clientAddress, string? apiKey) + { + var request = new HttpRequestMessage(HttpMethod.Post, $"/api/flows/{Guid.NewGuid()}/webhooks") + { + Content = JsonContent.Create(new + { + url = "https://example.com/hook", + secret = "a-secret-long-enough-to-pass" + }) + }; + request.Headers.Add(ClientAddressStartupFilter.HeaderName, clientAddress); + + if (apiKey is not null) + request.Headers.Add("X-Api-Key", apiKey); + + return request; + } + + private static HttpContext CallerContext(string address, Guid? sessionId, string? principal) + { + var claims = new List(); + + if (principal is not null) + claims.Add(new Claim(ClaimTypes.NameIdentifier, principal)); + + if (sessionId is not null) + claims.Add(new Claim(ApplicantSessionAuthenticationDefaults.SessionIdClaim, sessionId.Value.ToString())); + + var context = new DefaultHttpContext + { + User = new ClaimsPrincipal(new ClaimsIdentity(claims, "test")) + }; + context.Connection.RemoteIpAddress = IPAddress.Parse(address); + return context; + } + + private static async Task StartSessionAsync(HttpClient client, Guid flowId) + { + var response = await client.SendAsync(StartRequest(flowId, ClientA)); + response.EnsureSuccessStatusCode(); + return (await response.Content.ReadFromJsonAsync())!; + } + + private static HttpRequestMessage IngestRequest(StartResponse session, Guid flowId) + { + var request = new HttpRequestMessage(HttpMethod.Post, "/api/analytics/events"); + request.Headers.Authorization = new AuthenticationHeaderValue("Bearer", session.ApplicantToken); + request.Headers.Add(ClientAddressStartupFilter.HeaderName, ClientA); + request.Content = JsonContent.Create(new + { + events = new[] + { + new + { + eventType = "step_view", + journeyId = flowId.ToString(), + sessionId = session.SessionId.ToString() + } + } + }); + return request; + } +} diff --git a/src/backend/OpenOnboarding.Application.Tests/TestHelpers/ClientAddressStartupFilter.cs b/src/backend/OpenOnboarding.Application.Tests/TestHelpers/ClientAddressStartupFilter.cs new file mode 100644 index 0000000..3d07084 --- /dev/null +++ b/src/backend/OpenOnboarding.Application.Tests/TestHelpers/ClientAddressStartupFilter.cs @@ -0,0 +1,38 @@ +using System.Net; +using Microsoft.AspNetCore.Builder; +using Microsoft.AspNetCore.Hosting; + +namespace OpenOnboarding.Application.Tests.TestHelpers; + +/// +/// Gives a test control over HttpContext.Connection.RemoteIpAddress, which +/// otherwise leaves null. +/// +/// Rate limit partitioning and the forwarded-headers trust check both read the connection address, +/// so a test cannot pose as two different callers without setting it. The middleware runs ahead of +/// everything in Program.cs - including UseForwardedHeaders - so the address it sets +/// is the one the proxy trust check sees. +/// +/// +internal sealed class ClientAddressStartupFilter : IStartupFilter +{ + /// Header a test sets to choose the address the request appears to come from. + public const string HeaderName = "X-Test-Client-Address"; + + public Action Configure(Action next) + => app => + { + app.Use(async (context, nextMiddleware) => + { + if (context.Request.Headers.TryGetValue(HeaderName, out var address) + && IPAddress.TryParse(address.ToString(), out var parsed)) + { + context.Connection.RemoteIpAddress = parsed; + } + + await nextMiddleware(); + }); + + next(app); + }; +} diff --git a/src/backend/OpenOnboarding.Application.Tests/TestWebAppFactory.cs b/src/backend/OpenOnboarding.Application.Tests/TestWebAppFactory.cs index bfaacd8..7a11130 100644 --- a/src/backend/OpenOnboarding.Application.Tests/TestWebAppFactory.cs +++ b/src/backend/OpenOnboarding.Application.Tests/TestWebAppFactory.cs @@ -5,6 +5,8 @@ using Microsoft.EntityFrameworkCore.Storage; using Microsoft.Extensions.Configuration; using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Hosting; +using OpenOnboarding.Application.Tests.TestHelpers; using OpenOnboarding.Infrastructure.Persistence; namespace OpenOnboarding.Application.Tests; @@ -54,6 +56,11 @@ public static WebApplicationFactory Create( }); builder.ConfigureTestServices(services => { + // Lets a test choose the address a request appears to come from; without it + // TestServer leaves RemoteIpAddress null and every caller shares one rate limit + // partition. Inert unless the test sends the header. + services.AddSingleton(); + // EF Core 10 uses TryAdd for IDbContextOptionsConfiguration, so the Npgsql // registration persists unless we explicitly remove it before adding the InMemory one. // Remove all DbContext-related registrations for OnboardingDbContext. From ed9154e9e689e22f5f5323a726f24e327c65ba48 Mon Sep 17 00:00:00 2001 From: MaximumTrainer Date: Sun, 13 Sep 2026 10:03:02 +0100 Subject: [PATCH 2/5] test(infrastructure): cover RabbitMqSessionEventTransport The adapter that carries session events between replicas had no test of any kind. SessionEventEmitterTests proves the fan-out contract through an in-process fake, so the emitter and the port were covered and the only implementation that runs in a real deployment was not. A defect in it reproduces the failure it exists to prevent: an SSE stream that stays open and delivers nothing. Unit tests cover the wire format, the poison-message path and the lifecycle guard. Broker-backed tests cover the deployment-shaped case - publish through one transport, receive on another - and that a malformed body on the wire does not stop the consumer. The poison path moved out of the consumer lambda into DeliverAsync so it can be tested without a broker. Recovery was assumed from client defaults. It is now stated explicitly and proven: the recovery test drops the connection from the broker side, which is the only way to get one - the client treats an application-initiated close as deliberate and does not recover from it. Connections carry a name so the management UI can identify which replica holds which queue. Broker tests skip with a clear reason when nothing is listening, so dotnet test stays dependency-free. CI declares a rabbitmq service container and sets REQUIRE_BROKER_TESTS=1, which turns a missing broker into a failure rather than a silent skip. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/ci.yml | 21 ++ docker-compose.yml | 16 + docs/runbook.md | 22 ++ .../RabbitMqSessionEventTransportTests.cs | 299 ++++++++++++++++++ .../RequiresBrokerFactAttribute.cs | 159 ++++++++++ .../EventBus/RabbitMqSessionEventTransport.cs | 67 +++- 6 files changed, 567 insertions(+), 17 deletions(-) create mode 100644 src/backend/OpenOnboarding.Application.Tests/RabbitMqSessionEventTransportTests.cs create mode 100644 src/backend/OpenOnboarding.Application.Tests/TestHelpers/RequiresBrokerFactAttribute.cs diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 7201e95..7e66033 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -46,8 +46,29 @@ jobs: --health-timeout 5s --health-retries 10 + # Carries session events between API replicas. The broker-backed transport tests skip + # themselves when nothing is listening, so this is what makes them actually run. The + # management plugin is needed by one of them, which drops a connection broker-side to prove + # the consumer recovers. + rabbitmq: + image: rabbitmq:3-management + env: + RABBITMQ_DEFAULT_USER: guest + RABBITMQ_DEFAULT_PASS: guest + ports: + - 5672:5672 + - 15672:15672 + options: >- + --health-cmd "rabbitmq-diagnostics -q check_running" + --health-interval 10s + --health-timeout 10s + --health-retries 12 + env: ConnectionStrings__OnboardingDb: "Host=localhost;Port=5432;Database=onboarding_test;Username=postgres;Password=postgres" + SESSIONEVENTS__RABBITMQ__URI: "amqp://guest:guest@localhost:5672/" + # A broker exists here, so a missing one must fail the build rather than skip the tests. + REQUIRE_BROKER_TESTS: "1" PERSONA_REPORT_DIR: ${{ github.workspace }}/persona-report steps: diff --git a/docker-compose.yml b/docker-compose.yml index 4aa2790..02d63bc 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -15,6 +15,22 @@ services: timeout: 5s retries: 10 + # Only needed for multi-replica session event fan-out (SessionEvents:Transport=rabbitmq) and + # for the broker-backed tests, which skip themselves when nothing is listening here. + rabbitmq: + image: rabbitmq:3-management + environment: + RABBITMQ_DEFAULT_USER: guest + RABBITMQ_DEFAULT_PASS: guest + ports: + - "5672:5672" + - "15672:15672" + healthcheck: + test: ["CMD", "rabbitmq-diagnostics", "-q", "check_running"] + interval: 10s + timeout: 10s + retries: 10 + api: build: . ports: diff --git a/docs/runbook.md b/docs/runbook.md index 86ab114..6cbb466 100644 --- a/docs/runbook.md +++ b/docs/runbook.md @@ -423,6 +423,28 @@ A non-Development environment running the in-memory emitter logs a startup warni limitation. Events are transient and not persisted: they are only useful to a stream that is open at the time, so an instance that was down missed the stream too. +**Connection recovery.** The transport sets `AutomaticRecoveryEnabled` and `TopologyRecoveryEnabled` +explicitly rather than relying on the RabbitMQ client defaults. On a dropped connection the client +reconnects, re-declares the exclusive queue and its binding, and re-attaches the consumer, so +delivery resumes without restarting the instance. This is covered by a test that drops the +connection from the broker side and asserts events flow again. Each connection reports a name of +`open-onboarding-session-events:`, so the management UI identifies which replica holds +which queue. + +**Testing the transport.** The broker-backed tests skip themselves when nothing is listening, so +`dotnet test` stays dependency-free. To run them: + +```bash +docker compose up -d rabbitmq +dotnet test src/backend/OpenOnboarding.Application.Tests +``` + +Point them elsewhere with `SESSIONEVENTS__RABBITMQ__URI`. One of them — the recovery test — also +needs the management API on port 15672, and skips separately if the broker image does not include +it. CI declares a `rabbitmq:3-management` service container and sets `REQUIRE_BROKER_TESTS=1`, which +turns a missing broker into a failure rather than a skip: a silently skipped test is a green build +that proved nothing. + All other state is in PostgreSQL — safe for horizontal scaling. **Connection pool**: Configure `Maximum Pool Size` in the connection string (default: 100). For multi-instance, ensure total connections < PostgreSQL `max_connections` (default: 100). diff --git a/src/backend/OpenOnboarding.Application.Tests/RabbitMqSessionEventTransportTests.cs b/src/backend/OpenOnboarding.Application.Tests/RabbitMqSessionEventTransportTests.cs new file mode 100644 index 0000000..6f464dd --- /dev/null +++ b/src/backend/OpenOnboarding.Application.Tests/RabbitMqSessionEventTransportTests.cs @@ -0,0 +1,299 @@ +using System.Collections.Concurrent; +using System.Text; +using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Logging.Abstractions; +using OpenOnboarding.Application.Interfaces; +using OpenOnboarding.Application.Tests.TestHelpers; +using OpenOnboarding.Infrastructure.EventBus; +using RabbitMQ.Client; + +namespace OpenOnboarding.Application.Tests; + +/// +/// proves the fan-out contract through an in-process fake, so +/// it covers the emitter and the port but none of the broker adapter. This is the implementation +/// that actually runs in a multi-replica deployment, and a defect in it reproduces exactly the +/// failure the distributed transport exists to prevent: an SSE stream that stays open and delivers +/// nothing. +/// +/// The parts that need no broker are unit tested. The deployment-shaped case needs a real one and +/// skips with a clear reason when none is reachable. +/// +/// +public sealed class RabbitMqSessionEventTransportTests +{ + private static RabbitMqSessionEventTransport CreateTransport(string exchange, string? clientProvidedName = null) + => new(BrokerProbe.BrokerUri, exchange, NullLogger.Instance, clientProvidedName); + + // ── Wire format ──────────────────────────────────────────────────────────── + + [Fact] + public void Serialize_ThenDeserialize_RoundTripsTheWholeEnvelope() + { + var sessionId = Guid.NewGuid(); + var timestamp = new DateTimeOffset(2026, 9, 13, 10, 30, 45, 123, TimeSpan.Zero); + var original = new SessionEvent("step-completed", """{"nodeKey":"identity","attempt":2}""", timestamp); + + var envelope = RabbitMqSessionEventTransport.Deserialize( + RabbitMqSessionEventTransport.Serialize(sessionId, original)); + + Assert.NotNull(envelope); + Assert.Equal(sessionId, envelope!.SessionId); + Assert.Equal(original.EventType, envelope.EventType); + Assert.Equal(original.PayloadJson, envelope.PayloadJson); + Assert.Equal(original.Timestamp, envelope.Timestamp); + + var restored = envelope.ToSessionEvent(); + Assert.Equal(original.EventType, restored.EventType); + Assert.Equal(original.PayloadJson, restored.PayloadJson); + Assert.Equal(original.Timestamp, restored.Timestamp); + } + + [Fact] + public void Deserialize_PreservesTheTimestampOffset() + { + // A session trail read across regions is wrong by hours if the offset is dropped. + var timestamp = new DateTimeOffset(2026, 9, 13, 10, 30, 45, TimeSpan.FromHours(5.5)); + + var envelope = RabbitMqSessionEventTransport.Deserialize( + RabbitMqSessionEventTransport.Serialize(Guid.NewGuid(), new SessionEvent("x", "{}", timestamp))); + + Assert.Equal(timestamp.Offset, envelope!.Timestamp.Offset); + Assert.Equal(timestamp, envelope.Timestamp); + } + + // ── Poison messages ──────────────────────────────────────────────────────── + + [Fact] + public async Task Deliver_MalformedBody_IsLoggedAndSwallowed() + { + var logger = new CapturingLogger(); + var delivered = new List(); + + await RabbitMqSessionEventTransport.DeliverAsync( + Encoding.UTF8.GetBytes("{ this is not json"), + (sessionId, _) => { delivered.Add(sessionId); return Task.CompletedTask; }, + logger); + + Assert.Empty(delivered); + Assert.Contains(logger.Errors, message => message.Contains("Failed to deliver a session event")); + } + + [Fact] + public async Task Deliver_AfterAMalformedBody_KeepsDeliveringSubsequentMessages() + { + var logger = new CapturingLogger(); + var delivered = new List(); + Func handler = (_, evt) => + { + delivered.Add(evt.EventType); + return Task.CompletedTask; + }; + + await RabbitMqSessionEventTransport.DeliverAsync(Encoding.UTF8.GetBytes("not json at all"), handler, logger); + await RabbitMqSessionEventTransport.DeliverAsync( + RabbitMqSessionEventTransport.Serialize( + Guid.NewGuid(), new SessionEvent("step-completed", "{}", DateTimeOffset.UtcNow)), + handler, + logger); + + Assert.Equal(["step-completed"], delivered); + } + + [Fact] + public async Task Deliver_WhenTheHandlerThrows_DoesNotPropagate() + { + // The consumer callback is the only caller; an exception escaping it kills the consumer and + // with it every open stream on this instance. + var logger = new CapturingLogger(); + + await RabbitMqSessionEventTransport.DeliverAsync( + RabbitMqSessionEventTransport.Serialize( + Guid.NewGuid(), new SessionEvent("step-completed", "{}", DateTimeOffset.UtcNow)), + (_, _) => throw new InvalidOperationException("the stream went away"), + logger); + + Assert.Single(logger.Errors); + } + + [Fact] + public async Task Deliver_AJsonNullBody_IsIgnoredWithoutThrowing() + { + var logger = new CapturingLogger(); + var delivered = 0; + + await RabbitMqSessionEventTransport.DeliverAsync( + Encoding.UTF8.GetBytes("null"), + (_, _) => { delivered++; return Task.CompletedTask; }, + logger); + + Assert.Equal(0, delivered); + Assert.Empty(logger.Errors); + } + + // ── Lifecycle ────────────────────────────────────────────────────────────── + + [Fact] + public async Task BroadcastAsync_BeforeStartAsync_Throws() + { + await using var transport = CreateTransport("session-events-test"); + + var error = await Assert.ThrowsAsync( + () => transport.BroadcastAsync( + Guid.NewGuid(), new SessionEvent("step-completed", "{}", DateTimeOffset.UtcNow))); + + Assert.Contains("StartAsync", error.Message); + } + + [Fact] + public async Task DisposeAsync_BeforeStartAsync_DoesNotThrow() + { + var transport = CreateTransport("session-events-test"); + + await transport.DisposeAsync(); + } + + // ── Against a real broker ────────────────────────────────────────────────── + + [RequiresBrokerFact] + public async Task EventPublishedOnOneInstance_ReachesAnother() + { + var exchange = UniqueExchange(); + await using var publisher = CreateTransport(exchange); + await using var subscriber = CreateTransport(exchange); + + var received = new ConcurrentQueue<(Guid SessionId, SessionEvent Event)>(); + var arrived = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + + // Both instances consume, as they do in a real deployment: the publisher receives its own + // broadcast too, which is what makes delivery uniform across instances. + await publisher.StartAsync((_, _) => Task.CompletedTask); + await subscriber.StartAsync((sessionId, evt) => + { + received.Enqueue((sessionId, evt)); + arrived.TrySetResult(); + return Task.CompletedTask; + }); + + var expectedSession = Guid.NewGuid(); + var expectedEvent = new SessionEvent( + "step-completed", """{"nodeKey":"identity"}""", DateTimeOffset.UtcNow); + + await publisher.BroadcastAsync(expectedSession, expectedEvent); + + await WaitForAsync(arrived.Task, "the subscriber never received the broadcast event"); + + Assert.True(received.TryDequeue(out var delivered)); + Assert.Equal(expectedSession, delivered.SessionId); + Assert.Equal(expectedEvent.EventType, delivered.Event.EventType); + Assert.Equal(expectedEvent.PayloadJson, delivered.Event.PayloadJson); + } + + [RequiresBrokerFact] + public async Task AMalformedMessageOnTheWire_DoesNotStopTheConsumer() + { + var exchange = UniqueExchange(); + await using var subscriber = CreateTransport(exchange); + + var good = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + await subscriber.StartAsync((_, _) => { good.TrySetResult(); return Task.CompletedTask; }); + + var factory = new ConnectionFactory { Uri = new Uri(BrokerProbe.BrokerUri) }; + await using var connection = await factory.CreateConnectionAsync(); + await using var channel = await connection.CreateChannelAsync(); + await channel.ExchangeDeclareAsync(exchange, ExchangeType.Fanout, durable: true, autoDelete: false); + + await channel.BasicPublishAsync( + exchange, routingKey: string.Empty, body: Encoding.UTF8.GetBytes("{ not an envelope")); + + await channel.BasicPublishAsync( + exchange, + routingKey: string.Empty, + body: RabbitMqSessionEventTransport.Serialize( + Guid.NewGuid(), new SessionEvent("step-completed", "{}", DateTimeOffset.UtcNow))); + + await WaitForAsync(good.Task, "the consumer stopped after a poison message"); + } + + [RequiresBrokerManagementFact] + public async Task DeliveryResumes_AfterTheBrokerConnectionIsDropped() + { + var exchange = UniqueExchange(); + var subscriberName = $"session-events-test-subscriber-{Guid.NewGuid():N}"; + + await using var publisher = CreateTransport(exchange); + await using var subscriber = CreateTransport(exchange, subscriberName); + + var beforeDrop = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + var afterRecovery = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + + await publisher.StartAsync((_, _) => Task.CompletedTask); + await subscriber.StartAsync((_, evt) => + { + if (evt.EventType == "before-drop") beforeDrop.TrySetResult(); + if (evt.EventType == "after-recovery") afterRecovery.TrySetResult(); + return Task.CompletedTask; + }); + + await publisher.BroadcastAsync(Guid.NewGuid(), new SessionEvent("before-drop", "{}", DateTimeOffset.UtcNow)); + await WaitForAsync(beforeDrop.Task, "the subscriber never received the pre-drop event"); + + // Kill the subscriber connection from the broker side. It has to come from there: the + // client treats an application-initiated close as deliberate and does not recover from it, + // so closing it locally would prove nothing. Automatic recovery must now reconnect, + // re-declare the exclusive queue and its binding, and re-attach the consumer - the topology + // recovery this adapter configures explicitly rather than inheriting from client defaults. + await BrokerProbe.ForceCloseConnectionAsync(subscriberName); + + // Recovery polls on NetworkRecoveryInterval, so keep republishing until it lands or we + // give up; a single publish into the gap would be lost with nothing bound to the exchange. + using var cts = new CancellationTokenSource(TimeSpan.FromSeconds(60)); + while (!afterRecovery.Task.IsCompleted && !cts.IsCancellationRequested) + { + try + { + await publisher.BroadcastAsync( + Guid.NewGuid(), new SessionEvent("after-recovery", "{}", DateTimeOffset.UtcNow)); + } + catch (Exception) + { + // The publisher may be mid-recovery itself; keep trying until the deadline. + } + + await Task.WhenAny(afterRecovery.Task, Task.Delay(TimeSpan.FromSeconds(1))); + } + + Assert.True( + afterRecovery.Task.IsCompletedSuccessfully, + "delivery did not resume within 60s of the broker connection being dropped"); + } + + private static string UniqueExchange() => $"session-events-test-{Guid.NewGuid():N}"; + + private static async Task WaitForAsync(Task task, string because) + { + var completed = await Task.WhenAny(task, Task.Delay(TimeSpan.FromSeconds(20))); + Assert.True(completed == task, because); + await task; + } + + private sealed class CapturingLogger : ILogger + { + public List Errors { get; } = []; + + public IDisposable? BeginScope(TState state) where TState : notnull => null; + + public bool IsEnabled(LogLevel logLevel) => true; + + public void Log( + LogLevel logLevel, + EventId eventId, + TState state, + Exception? exception, + Func formatter) + { + if (logLevel >= LogLevel.Error) + Errors.Add(formatter(state, exception)); + } + } +} diff --git a/src/backend/OpenOnboarding.Application.Tests/TestHelpers/RequiresBrokerFactAttribute.cs b/src/backend/OpenOnboarding.Application.Tests/TestHelpers/RequiresBrokerFactAttribute.cs new file mode 100644 index 0000000..179a220 --- /dev/null +++ b/src/backend/OpenOnboarding.Application.Tests/TestHelpers/RequiresBrokerFactAttribute.cs @@ -0,0 +1,159 @@ +using System.Net.Http.Headers; +using System.Net.Sockets; +using System.Text; +using System.Text.Json; + +namespace OpenOnboarding.Application.Tests.TestHelpers; + +/// +/// A that skips itself when no RabbitMQ broker is reachable. +/// +/// The same bargain the git hooks make: dotnet test must pass on any machine with the SDKs +/// installed, while the suites that need real infrastructure still run where it exists - CI, or a +/// developer with docker compose up. Skipping is decided at discovery so the reachability +/// probe runs once per test class rather than once per test. +/// +/// +public sealed class RequiresBrokerFactAttribute : FactAttribute +{ + public RequiresBrokerFactAttribute() + { + if (!BrokerProbe.IsReachable && !BrokerProbe.BrokerIsRequired) + Skip = BrokerProbe.SkipReason; + } +} + +/// +/// As , but also needs the broker's management HTTP API. +/// +/// Only the connection-recovery test needs it: a dropped connection has to be dropped by the +/// broker, because the client treats an application-initiated close as deliberate and does not +/// recover from one. The management API is how you ask the broker to do that. +/// +/// +public sealed class RequiresBrokerManagementFactAttribute : FactAttribute +{ + public RequiresBrokerManagementFactAttribute() + { + if (BrokerProbe.BrokerIsRequired) + return; + + if (!BrokerProbe.IsReachable) + Skip = BrokerProbe.SkipReason; + else if (!BrokerProbe.IsManagementReachable) + Skip = BrokerProbe.ManagementSkipReason; + } +} + +/// +/// Resolves the broker to test against and reports whether it answers. +/// +public static class BrokerProbe +{ + /// Environment variable naming the broker, so CI can point at its service container. + public const string UriVariable = "SESSIONEVENTS__RABBITMQ__URI"; + + /// Port the management plugin listens on. Only the recovery test needs it. + public const int ManagementPort = 15672; + + /// + /// Set where a broker is guaranteed to exist - CI, which declares one as a service container. + /// There, a missing broker must fail the build rather than skip: a silently skipped test is a + /// green build that proved nothing, which is how this adapter came to have no coverage at all. + /// + public const string RequiredVariable = "REQUIRE_BROKER_TESTS"; + + /// Whether a missing broker should fail instead of skip. + public static bool BrokerIsRequired => + Environment.GetEnvironmentVariable(RequiredVariable) is "1" or "true"; + + private static readonly Lazy Reachable = new(() => Probe(AmqpPort), isThreadSafe: true); + private static readonly Lazy ManagementReachable = + new(() => Probe(ManagementPort), isThreadSafe: true); + + private const int AmqpPort = 5672; + + /// The broker URI under test, defaulting to a local one. + public static string BrokerUri => + Environment.GetEnvironmentVariable(UriVariable) is { Length: > 0 } configured + ? configured + : "amqp://guest:guest@localhost:5672/"; + + public static bool IsReachable => Reachable.Value; + + public static bool IsManagementReachable => ManagementReachable.Value; + + public static string SkipReason => + $"No RabbitMQ broker reachable at {BrokerUri}. Start one (docker compose up rabbitmq) " + + $"or set {UriVariable} to run this test."; + + public static string ManagementSkipReason => + $"The RabbitMQ broker at {BrokerUri} has no management API on port {ManagementPort}. " + + "Run an image that includes it (rabbitmq:3-management) to run this test."; + + /// + /// Asks the broker to close the connection with the given client-provided name, so the client + /// sees a failure and runs its recovery path. + /// + public static async Task ForceCloseConnectionAsync(string clientProvidedName) + { + var broker = new Uri(BrokerUri); + var credentials = broker.UserInfo.Split(':', 2); + + using var http = new HttpClient + { + BaseAddress = new UriBuilder("http", broker.Host, ManagementPort).Uri, + Timeout = TimeSpan.FromSeconds(10) + }; + http.DefaultRequestHeaders.Authorization = new AuthenticationHeaderValue( + "Basic", + Convert.ToBase64String(Encoding.UTF8.GetBytes( + $"{Uri.UnescapeDataString(credentials.ElementAtOrDefault(0) ?? "guest")}:" + + $"{Uri.UnescapeDataString(credentials.ElementAtOrDefault(1) ?? "guest")}"))); + + // The connection may take a moment to register in the management database. + var deadline = DateTime.UtcNow.AddSeconds(20); + while (DateTime.UtcNow < deadline) + { + using var listing = await http.GetAsync("/api/connections"); + listing.EnsureSuccessStatusCode(); + + using var document = JsonDocument.Parse(await listing.Content.ReadAsStringAsync()); + var match = document.RootElement.EnumerateArray().FirstOrDefault(connection => + connection.TryGetProperty("client_properties", out var properties) + && properties.TryGetProperty("connection_name", out var name) + && name.GetString() == clientProvidedName); + + if (match.ValueKind == JsonValueKind.Object) + { + var id = match.GetProperty("name").GetString()!; + using var delete = await http.DeleteAsync($"/api/connections/{Uri.EscapeDataString(id)}"); + delete.EnsureSuccessStatusCode(); + return; + } + + await Task.Delay(TimeSpan.FromSeconds(1)); + } + + throw new InvalidOperationException( + $"The broker never reported a connection named {clientProvidedName}."); + } + + private static bool Probe(int defaultPort) + { + try + { + var uri = new Uri(BrokerUri); + var port = defaultPort == AmqpPort && uri.Port > 0 ? uri.Port : defaultPort; + + using var client = new TcpClient(); + // Short: a missing broker is the normal case locally and must not slow the suite down. + return client.ConnectAsync(uri.Host, port).Wait(TimeSpan.FromSeconds(2)) + && client.Connected; + } + catch + { + return false; + } + } +} diff --git a/src/backend/OpenOnboarding.Infrastructure/EventBus/RabbitMqSessionEventTransport.cs b/src/backend/OpenOnboarding.Infrastructure/EventBus/RabbitMqSessionEventTransport.cs index 457fdff..be6b355 100644 --- a/src/backend/OpenOnboarding.Infrastructure/EventBus/RabbitMqSessionEventTransport.cs +++ b/src/backend/OpenOnboarding.Infrastructure/EventBus/RabbitMqSessionEventTransport.cs @@ -21,19 +21,27 @@ public sealed class RabbitMqSessionEventTransport : ISessionEventTransport private readonly string _connectionString; private readonly string _exchangeName; + private readonly string _clientProvidedName; private readonly ILogger _logger; private IConnection? _connection; private IChannel? _channel; + /// + /// Name this connection reports to the broker. Defaults to one naming the host, so the + /// management UI identifies which replica owns which queue - without it every replica shows up + /// as an anonymous connection and a stuck consumer cannot be traced back to an instance. + /// public RabbitMqSessionEventTransport( string connectionString, string exchangeName, - ILogger logger) + ILogger logger, + string? clientProvidedName = null) { _connectionString = connectionString; _exchangeName = exchangeName; _logger = logger; + _clientProvidedName = clientProvidedName ?? $"open-onboarding-session-events:{Environment.MachineName}"; } /// Wire envelope. Kept separate from so the session id travels with it. @@ -51,9 +59,48 @@ internal static byte[] Serialize(Guid sessionId, SessionEvent evt) internal static Envelope? Deserialize(ReadOnlySpan body) => JsonSerializer.Deserialize(body, JsonOptions); + /// + /// Hands one received message to , swallowing anything it or the + /// deserialiser throws. + /// + /// A single malformed or unhandleable message must never tear down the consumer: this channel + /// carries every session on this instance, so one poison body would silently stop progress + /// reaching every open SSE stream - the exact failure the distributed transport exists to + /// prevent. Extracted from the consumer callback so it can be tested without a broker. + /// + /// + internal static async Task DeliverAsync( + ReadOnlyMemory body, + Func handler, + ILogger logger) + { + try + { + var envelope = Deserialize(body.Span); + if (envelope is not null) + await handler(envelope.SessionId, envelope.ToSessionEvent()); + } + catch (Exception ex) + { + logger.LogError(ex, "Failed to deliver a session event received from RabbitMQ."); + } + } + public async Task StartAsync(Func handler, CancellationToken cancellationToken = default) { - var factory = new ConnectionFactory { Uri = new Uri(_connectionString) }; + // Recovery is stated rather than inherited from the client defaults. On a dropped + // connection the client reconnects and replays the topology - the exclusive queue, its + // binding and the consumer - so delivery resumes without this instance restarting. Left + // implicit, a future client default could turn this off and the only symptom would be SSE + // streams that stay open and deliver nothing. + var factory = new ConnectionFactory + { + Uri = new Uri(_connectionString), + AutomaticRecoveryEnabled = true, + TopologyRecoveryEnabled = true, + NetworkRecoveryInterval = TimeSpan.FromSeconds(5), + ClientProvidedName = _clientProvidedName + }; _connection = await factory.CreateConnectionAsync(cancellationToken); _channel = await _connection.CreateChannelAsync(cancellationToken: cancellationToken); @@ -71,21 +118,7 @@ await _channel.QueueBindAsync( cancellationToken: cancellationToken); var consumer = new AsyncEventingBasicConsumer(_channel); - consumer.ReceivedAsync += async (_, args) => - { - try - { - var envelope = Deserialize(args.Body.Span); - if (envelope is not null) - await handler(envelope.SessionId, envelope.ToSessionEvent()); - } - catch (Exception ex) - { - // A poison message must not tear down the consumer; the stream stays live for - // every other session. - _logger.LogError(ex, "Failed to deliver a session event received from RabbitMQ."); - } - }; + consumer.ReceivedAsync += (_, args) => DeliverAsync(args.Body, handler, _logger); await _channel.BasicConsumeAsync( queue.QueueName, autoAck: true, consumer, cancellationToken: cancellationToken); From 8f1b106b496244c83d8a589a3eaffa2bcc53988e Mon Sep 17 00:00:00 2001 From: MaximumTrainer Date: Sun, 13 Sep 2026 10:40:49 +0100 Subject: [PATCH 3/5] feat(auth): renew applicant tokens and refuse them once a session ends The per-session token was issued once at session start and never renewed or withdrawn, which cost at both ends. A journey left open past the lifetime returned to a live session holding a credential the API no longer accepted, with no way forward but to start again and lose everything entered. Step submission now returns a replacement token: activity extends the credential, so no separate refresh endpoint is needed and nothing to protect is added. A finished session kept a working token until expiry - up to a day, and on a shared machine well past the visit that created it. The ownership requirement now carries read/write intent: writes against a Completed or Abandoned session are refused for an applicant token, reads still serve the completion screen. Analytics ingest applies the same rule. Operators are unaffected. Abandon stays a read so its documented idempotency holds. The browser stores each renewal, drops the token when the journey completes, and distinguishes a spent credential - 401 expired, 403 terminal - from a server fault, showing a "your session has expired, start again" state instead of a generic error banner. Co-Authored-By: Claude Opus 5 (1M context) --- docs/runbook.md | 19 +- .../Authorization/SessionOwnershipHandler.cs | 12 + .../SessionOwnershipRequirement.cs | 26 +- .../Controllers/AnalyticsController.cs | 11 + .../Controllers/WorkflowController.cs | 25 +- .../ApplicantTokenLifecycleTests.cs | 387 ++++++++++++++++++ .../SessionOwnershipHandlerTests.cs | 92 ++++- .../Contracts/SessionStepResponse.cs | 16 + .../src/onboarding/ApplicantJourneyPage.tsx | 21 +- .../api/applicant-token-lifecycle.test.ts | 157 +++++++ .../src/onboarding/api/workflow-api-client.ts | 46 ++- .../onboarding/hooks/useOnboarding.test.ts | 86 ++++ .../src/onboarding/hooks/useOnboarding.ts | 14 +- src/frontend/src/onboarding/types/flow.ts | 12 +- 14 files changed, 905 insertions(+), 19 deletions(-) create mode 100644 src/backend/OpenOnboarding.Application.Tests/ApplicantTokenLifecycleTests.cs create mode 100644 src/frontend/src/onboarding/api/applicant-token-lifecycle.test.ts diff --git a/docs/runbook.md b/docs/runbook.md index 6cbb466..75f8166 100644 --- a/docs/runbook.md +++ b/docs/runbook.md @@ -135,8 +135,25 @@ response carries `applicantToken`, an HMAC-signed JWT the browser sends as - **Scope.** The token names one session. It is rejected for any other session, and for every operator endpoint, including the submissions readout for its own session. -- **Lifetime.** Defaults to `SessionTimeoutMinutes`. A shorter life only strands applicants +- **Lifetime.** Defaults to `SessionTimeoutMinutes` (1440), overridable with + `Authentication__ApplicantToken__LifetimeMinutes`. A shorter life only strands applicants mid-journey; a longer one outlives the session it names, which is abandoned by then anyway. +- **Renewal.** Step submission returns a fresh `applicantToken` and `applicantTokenExpiresAt` + alongside the next step, and the browser stores it in place of the one it held. Activity is what + 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. +- **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 + 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 + terminal session stays idempotent rather than returning `403`, since it changes nothing. +- **Clearing.** The browser drops the token from `sessionStorage` when the journey completes. +- **Expiry, from the applicant's side.** An expired token returns `401`; a token for a finished + session returns `403`. The frontend treats both as an expired credential and shows a distinct + "your session has expired, start again" state rather than a generic error banner — an expired + credential is a dead end, not a fault to retry. - **Signing key.** `Authentication__ApplicantToken__SigningKey` must be set outside Development or startup fails. All replicas need the same key, or a token issued by one is rejected by another. In Development an ephemeral key is generated per process, so tokens stop working on restart. diff --git a/src/backend/OpenOnboarding.Api/Authorization/SessionOwnershipHandler.cs b/src/backend/OpenOnboarding.Api/Authorization/SessionOwnershipHandler.cs index 0d31151..8051827 100644 --- a/src/backend/OpenOnboarding.Api/Authorization/SessionOwnershipHandler.cs +++ b/src/backend/OpenOnboarding.Api/Authorization/SessionOwnershipHandler.cs @@ -2,6 +2,7 @@ using Microsoft.AspNetCore.Authorization; using OpenOnboarding.Api.Authentication; using OpenOnboarding.Application.Contracts; +using OpenOnboarding.Domain.Enums; namespace OpenOnboarding.Api.Authorization; @@ -13,6 +14,14 @@ namespace OpenOnboarding.Api.Authorization; public sealed class SessionOwnershipHandler : AuthorizationHandler { + /// + /// Statuses after which a journey is over. An applicant token is refused for writes against a + /// session in one of these, so the credential stops being useful when the visit ends rather + /// than when it expires up to a day later. + /// + private static bool IsTerminal(SessionStatus status) + => status is SessionStatus.Completed or SessionStatus.Abandoned; + protected override Task HandleRequirementAsync( AuthorizationHandlerContext context, SessionOwnershipRequirement requirement, @@ -24,6 +33,9 @@ protected override Task HandleRequirementAsync( return Task.CompletedTask; } + if (requirement.RequiresActiveSession && IsTerminal(resource.Status)) + return Task.CompletedTask; + // A per-session applicant token names the one session it may act on. This is the path the // public onboarding app takes; it does not depend on a customer profile existing. var sessionId = context.User.FindFirstValue(ApplicantSessionAuthenticationDefaults.SessionIdClaim); diff --git a/src/backend/OpenOnboarding.Api/Authorization/SessionOwnershipRequirement.cs b/src/backend/OpenOnboarding.Api/Authorization/SessionOwnershipRequirement.cs index 0dc1900..0857faa 100644 --- a/src/backend/OpenOnboarding.Api/Authorization/SessionOwnershipRequirement.cs +++ b/src/backend/OpenOnboarding.Api/Authorization/SessionOwnershipRequirement.cs @@ -6,4 +6,28 @@ namespace OpenOnboarding.Api.Authorization; /// Requirement that the current user owns the session (or is an Operator). /// Used with resource-based authorization against SessionDetailDto. /// -public sealed class SessionOwnershipRequirement : IAuthorizationRequirement { } +public sealed class SessionOwnershipRequirement : IAuthorizationRequirement +{ + private SessionOwnershipRequirement(bool requiresActiveSession) + => RequiresActiveSession = requiresActiveSession; + + /// + /// Whether an applicant token is refused once its session has reached a terminal status. + /// Operators are never subject to this; the rule is about a credential outliving the visit that + /// created it, not about who may act on a finished session. + /// + public bool RequiresActiveSession { get; } + + /// + /// For reads. A completed session is still readable by the token that completed it, which is + /// what lets the completion screen render before the applicant closes the tab. + /// + public static SessionOwnershipRequirement Read { get; } = new(requiresActiveSession: false); + + /// + /// For anything that changes the session or writes against it. Once a session is Completed or + /// Abandoned its token stops being accepted here, so a credential left behind on a shared + /// machine cannot be used to write against the journey that ended. + /// + public static SessionOwnershipRequirement Write { get; } = new(requiresActiveSession: true); +} diff --git a/src/backend/OpenOnboarding.Api/Controllers/AnalyticsController.cs b/src/backend/OpenOnboarding.Api/Controllers/AnalyticsController.cs index b36c026..b377acf 100644 --- a/src/backend/OpenOnboarding.Api/Controllers/AnalyticsController.cs +++ b/src/backend/OpenOnboarding.Api/Controllers/AnalyticsController.cs @@ -6,6 +6,7 @@ using OpenOnboarding.Application.Contracts; using OpenOnboarding.Application.Interfaces; using OpenOnboarding.Domain.Entities; +using OpenOnboarding.Domain.Enums; namespace OpenOnboarding.Api.Controllers; @@ -79,6 +80,16 @@ public async Task> Ingest( } } + // A token for a finished journey stops being accepted here. It survives until expiry - up + // to a day - and on a shared machine that outlives the visit that created it. Operators are + // unaffected: they post for sessions they did not start, finished ones included. + if (!isOperator && Guid.TryParse(callerSessionId, out var callerSession)) + { + var session = await sessionAnalyticsService.GetSessionAsync(callerSession, cancellationToken); + if (session is not null && session.Status is SessionStatus.Completed or SessionStatus.Abandoned) + return Forbid(); + } + foreach (var incoming in request.Events) { await telemetryService.TrackAsync( diff --git a/src/backend/OpenOnboarding.Api/Controllers/WorkflowController.cs b/src/backend/OpenOnboarding.Api/Controllers/WorkflowController.cs index 05e5677..58b5c98 100644 --- a/src/backend/OpenOnboarding.Api/Controllers/WorkflowController.cs +++ b/src/backend/OpenOnboarding.Api/Controllers/WorkflowController.cs @@ -75,10 +75,23 @@ public async Task> StartSession([FromBody] St public async Task> SubmitStep([FromRoute] Guid sessionId, [FromRoute] Guid nodeId, [FromBody] SubmitStepRequest request, CancellationToken cancellationToken) { var session = await sessionAnalyticsService.GetSessionAsync(sessionId, cancellationToken); - var authResult = await authorizationService.AuthorizeAsync(User, session, new SessionOwnershipRequirement()); + var authResult = await authorizationService.AuthorizeAsync(User, session, SessionOwnershipRequirement.Write); if (!authResult.Succeeded) return Forbid(); var result = await workflowService.SubmitStepAsync(sessionId, nodeId, request, cancellationToken); + + // Renew on activity. The token is issued once at session start and nothing else extends it, + // so a long journey would otherwise reach a live session holding a credential the API no + // longer accepts. Renewing here needs no separate refresh endpoint: submitting a step is + // the activity that proves the journey is still being worked on. + if (!User.IsInRole(AppRoles.Operator) + && User.HasClaim(c => c.Type == ApplicantSessionAuthenticationDefaults.SessionIdClaim)) + { + var (token, expiresAt) = applicantTokenService.Issue(sessionId, session?.CustomerProfileId); + result.ApplicantToken = token; + result.ApplicantTokenExpiresAt = expiresAt; + } + return Ok(result); } @@ -95,7 +108,7 @@ public async Task> SubmitStep([FromRoute] Guid public async Task> GetNextStep([FromRoute] Guid sessionId, CancellationToken cancellationToken) { var session = await sessionAnalyticsService.GetSessionAsync(sessionId, cancellationToken); - var authResult = await authorizationService.AuthorizeAsync(User, session, new SessionOwnershipRequirement()); + var authResult = await authorizationService.AuthorizeAsync(User, session, SessionOwnershipRequirement.Read); if (!authResult.Succeeded) return Forbid(); var result = await workflowService.GetNextStepAsync(sessionId, cancellationToken); @@ -117,7 +130,9 @@ public async Task> GetNextStep([FromRoute] Gui public async Task> AbandonSession([FromRoute] Guid sessionId, CancellationToken cancellationToken) { var session = await sessionAnalyticsService.GetSessionAsync(sessionId, cancellationToken); - var authResult = await authorizationService.AuthorizeAsync(User, session, new SessionOwnershipRequirement()); + // Read, not Write: abandoning an already-terminal session is a documented no-op, + // so refusing the token here would break that idempotency without closing anything. + var authResult = await authorizationService.AuthorizeAsync(User, session, SessionOwnershipRequirement.Read); if (!authResult.Succeeded) return Forbid(); var result = await workflowService.AbandonSessionAsync(sessionId, cancellationToken); @@ -153,7 +168,7 @@ public async Task>> GetSubmissions([Fr public async Task> GetSession([FromRoute] Guid sessionId, CancellationToken cancellationToken) { var session = await sessionAnalyticsService.GetSessionAsync(sessionId, cancellationToken); - var authResult = await authorizationService.AuthorizeAsync(User, session, new SessionOwnershipRequirement()); + var authResult = await authorizationService.AuthorizeAsync(User, session, SessionOwnershipRequirement.Read); if (!authResult.Succeeded) return Forbid(); return Ok(session); @@ -291,7 +306,7 @@ public async Task StreamEvents([FromRoute] Guid sessionId, CancellationToken can // Same ownership rule as every other session endpoint: a credential for one session must // not open another session's event stream. var session = await sessionAnalyticsService.GetSessionAsync(sessionId, cancellationToken); - var authResult = await authorizationService.AuthorizeAsync(User, session, new SessionOwnershipRequirement()); + var authResult = await authorizationService.AuthorizeAsync(User, session, SessionOwnershipRequirement.Read); if (!authResult.Succeeded) { Response.StatusCode = StatusCodes.Status403Forbidden; diff --git a/src/backend/OpenOnboarding.Application.Tests/ApplicantTokenLifecycleTests.cs b/src/backend/OpenOnboarding.Application.Tests/ApplicantTokenLifecycleTests.cs new file mode 100644 index 0000000..c68b478 --- /dev/null +++ b/src/backend/OpenOnboarding.Application.Tests/ApplicantTokenLifecycleTests.cs @@ -0,0 +1,387 @@ +using System.Net; +using System.Net.Http.Headers; +using System.Net.Http.Json; +using System.Security.Claims; +using System.Text; +using Microsoft.AspNetCore.Mvc.Testing; +using Microsoft.IdentityModel.JsonWebTokens; +using Microsoft.IdentityModel.Tokens; +using OpenOnboarding.Api.Authentication; +using OpenOnboarding.Api.Authorization; + +namespace OpenOnboarding.Application.Tests; + +/// +/// The applicant token is issued once at session start and nothing renews or withdraws it. Two +/// things follow: a long journey outlives its own credential and strands the applicant with no way +/// forward but to start again, and a finished journey leaves a working credential behind for up to +/// a day - on a shared machine, well past the visit that created it. +/// +public sealed class ApplicantTokenLifecycleTests +{ + private const string OperatorApiKey = "test-api-key"; + + /// + /// Fixed so a test can mint its own token - an expired one in particular, which is otherwise + /// only reachable by waiting out the lifetime. + /// + private const string SigningKey = "applicant-token-lifecycle-tests-signing-key-long-enough-for-hmac-sha256"; + + private sealed record StepResponse( + Guid SessionId, + bool IsCompleted, + NodeStub? CurrentNode, + string? ApplicantToken, + DateTimeOffset? ApplicantTokenExpiresAt); + + private sealed record NodeStub(Guid Id, string Key, string Type); + + private sealed record FlowStub(Guid Id); + + private static WebApplicationFactory CreateFactory() + => TestWebAppFactory.Create(configurationOverrides: new Dictionary + { + ["Authentication:ApplicantToken:SigningKey"] = SigningKey + }); + + private static HttpClient CreateOperatorClient(WebApplicationFactory factory) + { + var client = factory.CreateClient(); + client.DefaultRequestHeaders.Add("X-Api-Key", OperatorApiKey); + return client; + } + + /// A one-step flow: submitting its only step completes the session. + private static async Task CreateSingleStepFlowAsync(HttpClient operatorClient) + { + var response = await operatorClient.PostAsJsonAsync("/api/flows", new + { + name = $"Token lifecycle flow {Guid.NewGuid()}", + nodes = new[] + { + new { key = "start", type = "Form", title = "Start", isStartNode = true, jsonContent = "{\"fields\":[]}" } + }, + connections = Array.Empty() + }); + + response.EnsureSuccessStatusCode(); + return (await response.Content.ReadFromJsonAsync())!.Id; + } + + /// A two-step flow, so a journey can continue on the credential the first step returned. + private static async Task CreateTwoStepFlowAsync(HttpClient operatorClient) + { + var first = Guid.NewGuid(); + var second = Guid.NewGuid(); + + var response = await operatorClient.PostAsJsonAsync("/api/flows", new + { + name = $"Token lifecycle flow {Guid.NewGuid()}", + nodes = new object[] + { + new { id = first, key = "start", type = "Form", title = "Start", isStartNode = true, jsonContent = "{\"fields\":[]}" }, + new { id = second, key = "details", type = "Form", title = "Details", isStartNode = false, jsonContent = "{\"fields\":[]}" } + }, + connections = new object[] + { + new { sourceNodeId = first, targetNodeId = second, priority = 0 } + } + }); + + 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 SubmitRequest(Guid sessionId, Guid nodeId, string token) + { + var request = new HttpRequestMessage( + HttpMethod.Post, $"/api/workflow/sessions/{sessionId}/steps/{nodeId}/submit") + { + Content = JsonContent.Create(new { payload = new Dictionary() }) + }; + request.Headers.Authorization = new AuthenticationHeaderValue("Bearer", token); + return request; + } + + private static HttpRequestMessage IngestRequest(Guid sessionId, Guid flowId, string token) + { + var request = new HttpRequestMessage(HttpMethod.Post, "/api/analytics/events") + { + Content = JsonContent.Create(new + { + events = new[] + { + new + { + eventType = "step_view", + journeyId = flowId.ToString(), + sessionId = sessionId.ToString() + } + } + }) + }; + request.Headers.Authorization = new AuthenticationHeaderValue("Bearer", token); + return request; + } + + private static HttpRequestMessage ReadRequest(Guid sessionId, string token) + { + var request = new HttpRequestMessage(HttpMethod.Get, $"/api/workflow/sessions/{sessionId}"); + request.Headers.Authorization = new AuthenticationHeaderValue("Bearer", token); + return request; + } + + /// + /// Mints a token for the session that expired in the past. Signed with the same key the API + /// uses, so the only reason it can be refused is its lifetime. + /// + private static string ExpiredTokenFor(Guid sessionId) + { + var descriptor = new SecurityTokenDescriptor + { + Issuer = ApplicantSessionAuthenticationDefaults.Issuer, + Audience = ApplicantSessionAuthenticationDefaults.Audience, + IssuedAt = DateTime.UtcNow.AddHours(-2), + Expires = DateTime.UtcNow.AddHours(-1), + Claims = new Dictionary + { + [JwtRegisteredClaimNames.Sub] = sessionId.ToString(), + [ApplicantSessionAuthenticationDefaults.SessionIdClaim] = sessionId.ToString(), + [ClaimTypes.Role] = AppRoles.Applicant + }, + SigningCredentials = new SigningCredentials( + new SymmetricSecurityKey(Encoding.UTF8.GetBytes(SigningKey)), + SecurityAlgorithms.HmacSha256) + }; + + return new JsonWebTokenHandler().CreateToken(descriptor); + } + + // ── Renewal ──────────────────────────────────────────────────────────────── + + [Fact] + public async Task SubmittingAStep_ReturnsARefreshedToken() + { + using var factory = CreateFactory(); + using var operatorClient = CreateOperatorClient(factory); + var flowId = await CreateTwoStepFlowAsync(operatorClient); + + using var browser = factory.CreateClient(); + var start = await StartSessionAsync(browser, flowId); + + var response = await browser.SendAsync( + SubmitRequest(start.SessionId, start.CurrentNode!.Id, start.ApplicantToken!)); + response.EnsureSuccessStatusCode(); + + var submitted = (await response.Content.ReadFromJsonAsync())!; + + Assert.False(string.IsNullOrWhiteSpace(submitted.ApplicantToken)); + Assert.True(submitted.ApplicantTokenExpiresAt > DateTimeOffset.UtcNow); + + // Not asserted to differ from the original: a JWT carries its expiry to the second, so two + // issuances inside the same second are byte-identical. What matters is that the response + // carries a usable credential for this session - see AJourneyContinues_OnTheRefreshedTokenAlone. + var claims = new JsonWebTokenHandler().ReadJsonWebToken(submitted.ApplicantToken); + Assert.Equal( + start.SessionId.ToString(), + claims.GetClaim(ApplicantSessionAuthenticationDefaults.SessionIdClaim).Value); + } + + [Fact] + public async Task TheRefreshedToken_ExpiresLaterThanTheOneItReplaces() + { + using var factory = CreateFactory(); + using var operatorClient = CreateOperatorClient(factory); + var flowId = await CreateTwoStepFlowAsync(operatorClient); + + using var browser = factory.CreateClient(); + var start = await StartSessionAsync(browser, flowId); + + var response = await browser.SendAsync( + SubmitRequest(start.SessionId, start.CurrentNode!.Id, start.ApplicantToken!)); + var submitted = (await response.Content.ReadFromJsonAsync())!; + + // This is what keeps a journey alive: each step pushes the deadline out, so the session + // cannot outlive its own credential while it is still being worked on. + Assert.NotNull(submitted.ApplicantTokenExpiresAt); + Assert.True( + submitted.ApplicantTokenExpiresAt > start.ApplicantTokenExpiresAt, + "the refreshed token must expire later than the one it replaces"); + } + + [Fact] + public async Task AJourneyContinues_OnTheRefreshedTokenAlone() + { + using var factory = CreateFactory(); + using var operatorClient = CreateOperatorClient(factory); + var flowId = await CreateTwoStepFlowAsync(operatorClient); + + using var browser = factory.CreateClient(); + var start = await StartSessionAsync(browser, flowId); + + var first = await browser.SendAsync( + SubmitRequest(start.SessionId, start.CurrentNode!.Id, start.ApplicantToken!)); + var afterFirst = (await first.Content.ReadFromJsonAsync())!; + + // The original credential is never used again - the journey finishes on the renewal. + var second = await browser.SendAsync( + SubmitRequest(afterFirst.SessionId, afterFirst.CurrentNode!.Id, afterFirst.ApplicantToken!)); + + Assert.Equal(HttpStatusCode.OK, second.StatusCode); + var afterSecond = (await second.Content.ReadFromJsonAsync())!; + Assert.True(afterSecond.IsCompleted); + } + + [Fact] + public async Task AnOperatorSubmitting_IsNotHandedAnApplicantToken() + { + using var factory = CreateFactory(); + using var operatorClient = CreateOperatorClient(factory); + var flowId = await CreateTwoStepFlowAsync(operatorClient); + + var start = await StartSessionAsync(operatorClient, flowId); + + var response = await operatorClient.PostAsJsonAsync( + $"/api/workflow/sessions/{start.SessionId}/steps/{start.CurrentNode!.Id}/submit", + new { payload = new Dictionary() }); + var submitted = (await response.Content.ReadFromJsonAsync())!; + + // An operator credential already covers the session; minting a second one would be a + // credential nobody asked for. + Assert.Null(submitted.ApplicantToken); + Assert.Null(submitted.ApplicantTokenExpiresAt); + } + + // ── Expiry ───────────────────────────────────────────────────────────────── + + [Fact] + public async Task AnExpiredToken_IsRefused() + { + using var factory = CreateFactory(); + using var operatorClient = CreateOperatorClient(factory); + var flowId = await CreateTwoStepFlowAsync(operatorClient); + + using var browser = factory.CreateClient(); + var start = await StartSessionAsync(browser, flowId); + + var response = await browser.SendAsync( + SubmitRequest(start.SessionId, start.CurrentNode!.Id, ExpiredTokenFor(start.SessionId))); + + // 401, not 403: the credential is not bad, it is over. The frontend tells the two apart to + // show "your session has expired" rather than a generic failure. + Assert.Equal(HttpStatusCode.Unauthorized, response.StatusCode); + } + + // ── Terminal sessions ────────────────────────────────────────────────────── + + [Fact] + public async Task ATokenForACompletedSession_IsRefusedFromStepSubmission() + { + using var factory = CreateFactory(); + using var operatorClient = CreateOperatorClient(factory); + var flowId = await CreateSingleStepFlowAsync(operatorClient); + + using var browser = factory.CreateClient(); + var start = await StartSessionAsync(browser, flowId); + + var completing = await browser.SendAsync( + SubmitRequest(start.SessionId, start.CurrentNode!.Id, start.ApplicantToken!)); + var completed = (await completing.Content.ReadFromJsonAsync())!; + Assert.True(completed.IsCompleted); + + var again = await browser.SendAsync( + SubmitRequest(start.SessionId, start.CurrentNode.Id, completed.ApplicantToken ?? start.ApplicantToken!)); + + Assert.Equal(HttpStatusCode.Forbidden, again.StatusCode); + } + + [Fact] + public async Task ATokenForACompletedSession_IsRefusedFromAnalyticsIngest() + { + using var factory = CreateFactory(); + using var operatorClient = CreateOperatorClient(factory); + var flowId = await CreateSingleStepFlowAsync(operatorClient); + + using var browser = factory.CreateClient(); + var start = await StartSessionAsync(browser, flowId); + + // Accepted while the journey is live. + var whileLive = await browser.SendAsync(IngestRequest(start.SessionId, flowId, start.ApplicantToken!)); + Assert.Equal(HttpStatusCode.Accepted, whileLive.StatusCode); + + var completing = await browser.SendAsync( + SubmitRequest(start.SessionId, start.CurrentNode!.Id, start.ApplicantToken!)); + var completed = (await completing.Content.ReadFromJsonAsync())!; + + var afterCompletion = await browser.SendAsync( + IngestRequest(start.SessionId, flowId, completed.ApplicantToken ?? start.ApplicantToken!)); + + Assert.Equal(HttpStatusCode.Forbidden, afterCompletion.StatusCode); + } + + [Fact] + public async Task ATokenForAnAbandonedSession_IsRefusedFromAnalyticsIngest() + { + using var factory = CreateFactory(); + using var operatorClient = CreateOperatorClient(factory); + var flowId = await CreateTwoStepFlowAsync(operatorClient); + + using var browser = factory.CreateClient(); + var start = await StartSessionAsync(browser, flowId); + + var abandon = new HttpRequestMessage(HttpMethod.Delete, $"/api/workflow/sessions/{start.SessionId}"); + abandon.Headers.Authorization = new AuthenticationHeaderValue("Bearer", start.ApplicantToken!); + (await browser.SendAsync(abandon)).EnsureSuccessStatusCode(); + + var response = await browser.SendAsync(IngestRequest(start.SessionId, flowId, start.ApplicantToken!)); + + Assert.Equal(HttpStatusCode.Forbidden, response.StatusCode); + } + + [Fact] + public async Task ATokenForACompletedSession_StillServesTheCompletionScreenRead() + { + using var factory = CreateFactory(); + using var operatorClient = CreateOperatorClient(factory); + var flowId = await CreateSingleStepFlowAsync(operatorClient); + + using var browser = factory.CreateClient(); + var start = await StartSessionAsync(browser, flowId); + + var completing = await browser.SendAsync( + SubmitRequest(start.SessionId, start.CurrentNode!.Id, start.ApplicantToken!)); + var completed = (await completing.Content.ReadFromJsonAsync())!; + + // The completion screen renders after the session is terminal; refusing this read would + // break the last thing the applicant sees. + var read = await browser.SendAsync( + ReadRequest(start.SessionId, completed.ApplicantToken ?? start.ApplicantToken!)); + + Assert.Equal(HttpStatusCode.OK, read.StatusCode); + } + + [Fact] + public async Task AnOperator_StillReachesACompletedSession() + { + using var factory = CreateFactory(); + using var operatorClient = CreateOperatorClient(factory); + var flowId = await CreateSingleStepFlowAsync(operatorClient); + + using var browser = factory.CreateClient(); + var start = await StartSessionAsync(browser, flowId); + await browser.SendAsync(SubmitRequest(start.SessionId, start.CurrentNode!.Id, start.ApplicantToken!)); + + // The terminal-status rule bounds a stale applicant credential; it says nothing about who + // may act on a finished session. + var read = await operatorClient.GetAsync($"/api/workflow/sessions/{start.SessionId}"); + + Assert.Equal(HttpStatusCode.OK, read.StatusCode); + } +} diff --git a/src/backend/OpenOnboarding.Application.Tests/Authorization/SessionOwnershipHandlerTests.cs b/src/backend/OpenOnboarding.Application.Tests/Authorization/SessionOwnershipHandlerTests.cs index c0e1576..736d5c2 100644 --- a/src/backend/OpenOnboarding.Application.Tests/Authorization/SessionOwnershipHandlerTests.cs +++ b/src/backend/OpenOnboarding.Application.Tests/Authorization/SessionOwnershipHandlerTests.cs @@ -2,12 +2,14 @@ using Microsoft.AspNetCore.Authorization; using OpenOnboarding.Api.Authorization; using OpenOnboarding.Application.Contracts; +using OpenOnboarding.Api.Authentication; +using OpenOnboarding.Domain.Enums; namespace OpenOnboarding.Application.Tests.Authorization; public sealed class SessionOwnershipHandlerTests { - private static readonly SessionOwnershipRequirement Requirement = new(); + private static readonly SessionOwnershipRequirement Requirement = SessionOwnershipRequirement.Read; private static readonly SessionOwnershipHandler Handler = new(); private static ClaimsPrincipal BuildPrincipal(string role, string? customerProfileId = null) @@ -23,8 +25,26 @@ private static ClaimsPrincipal BuildPrincipal(string role, string? customerProfi return new ClaimsPrincipal(new ClaimsIdentity(claims, "Test")); } - private static SessionDetailDto BuildSession(Guid? customerProfileId = null) => - new() { Id = Guid.NewGuid(), FlowId = Guid.NewGuid(), CustomerProfileId = customerProfileId }; + private static SessionDetailDto BuildSession( + Guid? customerProfileId = null, + SessionStatus status = SessionStatus.Started) => + new() + { + Id = Guid.NewGuid(), + FlowId = Guid.NewGuid(), + CustomerProfileId = customerProfileId, + Status = status + }; + + /// A principal carrying a per-session applicant token for the given session. + private static ClaimsPrincipal BuildSessionTokenPrincipal(Guid sessionId) + => new(new ClaimsIdentity( + [ + new Claim(ClaimTypes.NameIdentifier, sessionId.ToString()), + new Claim(ClaimTypes.Role, AppRoles.Applicant), + new Claim(ApplicantSessionAuthenticationDefaults.SessionIdClaim, sessionId.ToString()) + ], + "Test")); [Fact] public async Task HandleRequirementAsync_WhenOperator_Succeeds() @@ -75,4 +95,70 @@ public async Task HandleRequirementAsync_WhenApplicantHasNoCustomerProfileIdClai Assert.False(context.HasSucceeded); } + + // ── Terminal sessions ────────────────────────────────────────────────────── + // + // A token issued at session start stays valid until it expires - up to a day. Once the journey + // it names is over, that credential outlives the visit that created it, which on a shared or + // public machine is exactly the window worth closing. + + [Theory] + [InlineData(SessionStatus.Completed)] + [InlineData(SessionStatus.Abandoned)] + public async Task Write_AgainstATerminalSession_IsRefusedForAnApplicantToken(SessionStatus status) + { + var session = BuildSession(status: status); + var user = BuildSessionTokenPrincipal(session.Id); + + var context = new AuthorizationHandlerContext([SessionOwnershipRequirement.Write], user, session); + await Handler.HandleAsync(context); + + Assert.False(context.HasSucceeded); + } + + [Theory] + [InlineData(SessionStatus.Completed)] + [InlineData(SessionStatus.Abandoned)] + public async Task Read_AgainstATerminalSession_StillServesItsOwnToken(SessionStatus status) + { + // The completion screen renders after the session is Completed; refusing the read would + // break the last thing the applicant sees. + var session = BuildSession(status: status); + var user = BuildSessionTokenPrincipal(session.Id); + + var context = new AuthorizationHandlerContext([SessionOwnershipRequirement.Read], user, session); + await Handler.HandleAsync(context); + + Assert.True(context.HasSucceeded); + } + + [Theory] + [InlineData(SessionStatus.Started)] + [InlineData(SessionStatus.Error)] + public async Task Write_AgainstALiveSession_StillSucceeds(SessionStatus status) + { + var session = BuildSession(status: status); + var user = BuildSessionTokenPrincipal(session.Id); + + var context = new AuthorizationHandlerContext([SessionOwnershipRequirement.Write], user, session); + await Handler.HandleAsync(context); + + Assert.True(context.HasSucceeded); + } + + [Theory] + [InlineData(SessionStatus.Completed)] + [InlineData(SessionStatus.Abandoned)] + public async Task Write_AgainstATerminalSession_IsStillAllowedForAnOperator(SessionStatus status) + { + // The rule is about a stale applicant credential, not about who may act on a finished + // session. An operator reaches every session regardless of status. + var session = BuildSession(status: status); + var user = BuildPrincipal(AppRoles.Operator); + + var context = new AuthorizationHandlerContext([SessionOwnershipRequirement.Write], user, session); + await Handler.HandleAsync(context); + + Assert.True(context.HasSucceeded); + } } diff --git a/src/backend/OpenOnboarding.Application/Contracts/SessionStepResponse.cs b/src/backend/OpenOnboarding.Application/Contracts/SessionStepResponse.cs index d08aa1b..44573b7 100644 --- a/src/backend/OpenOnboarding.Application/Contracts/SessionStepResponse.cs +++ b/src/backend/OpenOnboarding.Application/Contracts/SessionStepResponse.cs @@ -21,4 +21,20 @@ public sealed class SessionStepResponse /// null when the session is completed. /// public NodeDto? CurrentNode { get; set; } + + /// + /// A replacement applicant token, extending the credential for a journey still in progress. + /// + /// Set on step submission for applicant callers only. Without it a compliance-heavy journey + /// left open past the token lifetime returns to a live session and a credential the API no + /// longer accepts, with no way forward but to start again and lose everything entered. + /// + /// null when the caller is an operator, whose own credential already covers the session, + /// and on responses that do not renew - session start carries its token on + /// instead. + /// + public string? ApplicantToken { get; set; } + + /// When stops being accepted. + public DateTimeOffset? ApplicantTokenExpiresAt { get; set; } } diff --git a/src/frontend/src/onboarding/ApplicantJourneyPage.tsx b/src/frontend/src/onboarding/ApplicantJourneyPage.tsx index 4a8b831..b5f0565 100644 --- a/src/frontend/src/onboarding/ApplicantJourneyPage.tsx +++ b/src/frontend/src/onboarding/ApplicantJourneyPage.tsx @@ -16,7 +16,7 @@ import { defaultFlowId } from '../journeys' * The journey comes from the link the applicant was sent (`/?flowId=...`), not from a picker. */ export function ApplicantJourneyPage({ search }: { search: string }) { - const { step, startSession, submitStep, isLoading, error } = useOnboarding() + const { step, startSession, submitStep, isLoading, error, isExpired } = useOnboarding() const [visitedNodeIds, setVisitedNodeIds] = useState>(new Set()) const flowId = new URLSearchParams(search).get('flowId') ?? defaultFlowId @@ -63,7 +63,24 @@ export function ApplicantJourneyPage({ search }: { search: string }) {
- {error ? ( + {isExpired ? ( + // Shown instead of the error banner, not alongside it: an expired credential is not a + // fault to retry past, and the applicant needs the one instruction that works. +
+

Your session has expired.

+

+ For your security, applications left open too long are closed. Please start again. +

+ +
+ ) : null} + {error && !isExpired ? (

{error}

diff --git a/src/frontend/src/onboarding/api/applicant-token-lifecycle.test.ts b/src/frontend/src/onboarding/api/applicant-token-lifecycle.test.ts new file mode 100644 index 0000000..ff5b895 --- /dev/null +++ b/src/frontend/src/onboarding/api/applicant-token-lifecycle.test.ts @@ -0,0 +1,157 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { ExpiredSessionError, getNextStep, startSession, submitStep } from './workflow-api-client' +import { clearApplicantToken, getApplicantToken } from './applicant-session' + +/** + * The applicant credential is issued once at session start and renewed on each step submission. Two + * behaviours matter to the browser: it has to store the renewal, and it has to tell an expired + * credential apart from a server fault — one is a dead end the applicant must be told about, the + * other is a failure they can retry. + */ +describe('applicant token lifecycle', () => { + beforeEach(() => { + clearApplicantToken() + }) + + afterEach(() => { + vi.restoreAllMocks() + clearApplicantToken() + }) + + const jsonResponse = (status: number, body: unknown) => ({ + ok: status >= 200 && status < 300, + status, + json: async () => body, + } as Response) + + describe('renewal', () => { + it('stores the token returned by session start', async () => { + vi.spyOn(globalThis, 'fetch').mockResolvedValue(jsonResponse(200, { + sessionId: 'session-1', + isCompleted: false, + currentNode: null, + applicantToken: 'token-from-start', + })) + + await startSession('', { flowId: 'flow-1' }) + + expect(getApplicantToken()).toBe('token-from-start') + }) + + it('replaces the stored token with the one a step submission returns', async () => { + vi.spyOn(globalThis, 'fetch') + .mockResolvedValueOnce(jsonResponse(200, { + sessionId: 'session-1', + isCompleted: false, + currentNode: null, + applicantToken: 'token-from-start', + })) + .mockResolvedValueOnce(jsonResponse(200, { + sessionId: 'session-1', + isCompleted: false, + currentNode: null, + applicantToken: 'refreshed-token', + })) + + await startSession('', { flowId: 'flow-1' }) + await submitStep('', 'session-1', 'node-1', { payload: {} }) + + // This is what keeps a long journey alive: without it the browser keeps presenting the + // original credential until it expires mid-application. + expect(getApplicantToken()).toBe('refreshed-token') + }) + + it('sends the refreshed token on the next request', async () => { + const fetchSpy = vi.spyOn(globalThis, 'fetch') + .mockResolvedValueOnce(jsonResponse(200, { + sessionId: 'session-1', + isCompleted: false, + currentNode: null, + applicantToken: 'token-from-start', + })) + .mockResolvedValueOnce(jsonResponse(200, { + sessionId: 'session-1', + isCompleted: false, + currentNode: null, + applicantToken: 'refreshed-token', + })) + .mockResolvedValueOnce(jsonResponse(200, { + sessionId: 'session-1', + isCompleted: false, + currentNode: null, + })) + + await startSession('', { flowId: 'flow-1' }) + await submitStep('', 'session-1', 'node-1', { payload: {} }) + await getNextStep('', 'session-1') + + const lastCall = fetchSpy.mock.calls.at(-1)! + const headers = (lastCall[1] as RequestInit).headers as Record + expect(headers.Authorization).toBe('Bearer refreshed-token') + }) + + it('keeps the stored token when a response carries no renewal', async () => { + // An operator-submitted step returns none; the browser must not wipe what it holds. + vi.spyOn(globalThis, 'fetch') + .mockResolvedValueOnce(jsonResponse(200, { + sessionId: 'session-1', + isCompleted: false, + currentNode: null, + applicantToken: 'token-from-start', + })) + .mockResolvedValueOnce(jsonResponse(200, { + sessionId: 'session-1', + isCompleted: false, + currentNode: null, + })) + + await startSession('', { flowId: 'flow-1' }) + await submitStep('', 'session-1', 'node-1', { payload: {} }) + + expect(getApplicantToken()).toBe('token-from-start') + }) + }) + + describe('expired credentials', () => { + it('raises ExpiredSessionError when a step submission is refused with 401', async () => { + vi.spyOn(globalThis, 'fetch').mockResolvedValue(jsonResponse(401, {})) + + await expect(submitStep('', 'session-1', 'node-1', { payload: {} })) + .rejects.toBeInstanceOf(ExpiredSessionError) + }) + + it('raises ExpiredSessionError when the session has reached a terminal status', async () => { + // 403 on a session this browser was completing means its token is no longer accepted for + // writes. From the applicant side that is the same dead end as an expiry. + vi.spyOn(globalThis, 'fetch').mockResolvedValue(jsonResponse(403, {})) + + await expect(submitStep('', 'session-1', 'node-1', { payload: {} })) + .rejects.toBeInstanceOf(ExpiredSessionError) + }) + + it('raises ExpiredSessionError when reading the next step is refused with 401', async () => { + vi.spyOn(globalThis, 'fetch').mockResolvedValue(jsonResponse(401, {})) + + await expect(getNextStep('', 'session-1')).rejects.toBeInstanceOf(ExpiredSessionError) + }) + + it('carries a message the applicant can act on', async () => { + vi.spyOn(globalThis, 'fetch').mockResolvedValue(jsonResponse(401, {})) + + const error = await submitStep('', 'session-1', 'node-1', { payload: {} }).catch((e) => e) + + expect(error.message).toMatch(/expired/i) + }) + + it('leaves a server fault as a generic error, not an expiry', async () => { + // A 500 is a failure the applicant can retry past; telling them to start again would throw + // away a journey that is still perfectly valid. + vi.spyOn(globalThis, 'fetch').mockResolvedValue(jsonResponse(500, {})) + + const error = await submitStep('', 'session-1', 'node-1', { payload: {} }).catch((e) => e) + + expect(error).not.toBeInstanceOf(ExpiredSessionError) + expect(error).toBeInstanceOf(Error) + }) + }) +}) diff --git a/src/frontend/src/onboarding/api/workflow-api-client.ts b/src/frontend/src/onboarding/api/workflow-api-client.ts index 001a1d6..189ea12 100644 --- a/src/frontend/src/onboarding/api/workflow-api-client.ts +++ b/src/frontend/src/onboarding/api/workflow-api-client.ts @@ -7,6 +7,20 @@ export interface ComplianceViolation { ruleId?: string } +/** + * The applicant credential is no longer accepted — it expired, or its session ended. + * + * Distinct from a generic failure on purpose. An expired credential is not a fault the applicant + * can retry past: the only way forward is to start again, and the page has to say so rather than + * showing a server-error banner over a journey that will never resume. + */ +export class ExpiredSessionError extends Error { + constructor() { + super('Your session has expired. Please start again.') + this.name = 'ExpiredSessionError' + } +} + export class ComplianceError extends Error { readonly violations: ComplianceViolation[] @@ -51,11 +65,22 @@ export async function startSession( if (!res.ok) throw new Error(`startSession failed with status ${res.status}`) const session = await res.json() as SessionStartResponse - if (session.applicantToken) setApplicantToken(session.applicantToken) + captureRefreshedToken(session) return session } +/** + * Stores a credential the API handed back, if it handed one back. + * + * Session start always carries one; step submission carries a renewal, which is what keeps a long + * journey from outliving its own token. An operator-submitted step carries none, and the stored + * credential is left alone. + */ +function captureRefreshedToken(response: SessionStepResponse): void { + if (response.applicantToken) setApplicantToken(response.applicantToken) +} + export async function submitStep( baseUrl: string, sessionId: string, @@ -75,8 +100,24 @@ export async function submitStep( const problem = await res.json() as { violations?: ComplianceViolation[] } throw new ComplianceError(problem.violations ?? []) } + if (isCredentialRejection(res.status)) throw new ExpiredSessionError() if (!res.ok) throw new Error(`submitStep failed with status ${res.status}`) - return res.json() as Promise + + const next = await res.json() as SessionStepResponse + captureRefreshedToken(next) + + return next +} + +/** + * Whether a status means the credential is the problem. + * + * 401 is an expired or unreadable token. 403 on a session the browser was completing means the + * session reached a terminal status, after which its own token is refused for writes — from the + * applicant's side the two are the same dead end. + */ +function isCredentialRejection(status: number): boolean { + return status === 401 || status === 403 } export async function getNextStep( @@ -88,6 +129,7 @@ export async function getNextStep( method: 'GET', headers: buildHeaders(), }) + if (res.status === 401) throw new ExpiredSessionError() if (!res.ok) throw new Error(`getNextStep failed with status ${res.status}`) return res.json() as Promise } diff --git a/src/frontend/src/onboarding/hooks/useOnboarding.test.ts b/src/frontend/src/onboarding/hooks/useOnboarding.test.ts index 556fc50..1c1c9db 100644 --- a/src/frontend/src/onboarding/hooks/useOnboarding.test.ts +++ b/src/frontend/src/onboarding/hooks/useOnboarding.test.ts @@ -14,6 +14,12 @@ vi.mock('../api/workflow-api-client', () => ({ this.violations = violations } }, + ExpiredSessionError: class ExpiredSessionError extends Error { + constructor() { + super('Your session has expired. Please start again.') + this.name = 'ExpiredSessionError' + } + }, })) vi.mock('./session-event-source', () => ({ @@ -113,4 +119,84 @@ describe('useOnboarding', () => { expect(result.current.error).toBeNull() }) + + // ── Expired credentials ──────────────────────────────────────────────────── + + it('flags an expired credential apart from a generic error', async () => { + const { ExpiredSessionError } = await import('../api/workflow-api-client') + mockApiSubmitStep.mockRejectedValueOnce(new ExpiredSessionError()) + + const { result } = renderHook(() => useOnboarding()) + + await act(async () => { + try { + await result.current.submitStep('session-abc', 'node-1', { payload: {} }) + } catch { + // expected + } + }) + + expect(result.current.isExpired).toBe(true) + }) + + it('leaves isExpired false for an ordinary failure', async () => { + // A server fault is retryable; telling the applicant to start again would throw away a + // journey that is still valid. + mockApiSubmitStep.mockRejectedValueOnce(new Error('submitStep failed with status 500')) + + const { result } = renderHook(() => useOnboarding()) + + await act(async () => { + try { + await result.current.submitStep('session-abc', 'node-1', { payload: {} }) + } catch { + // expected + } + }) + + expect(result.current.isExpired).toBe(false) + expect(result.current.error).toBe('submitStep failed with status 500') + }) + + it('starts with no expired state', () => { + const { result } = renderHook(() => useOnboarding()) + + expect(result.current.isExpired).toBe(false) + }) + + it('drops the stored credential once the journey completes', async () => { + const { setApplicantToken, getApplicantToken } = await import('../api/applicant-session') + setApplicantToken('token-for-a-finished-journey') + + mockApiSubmitStep.mockResolvedValueOnce({ + sessionId: 'session-abc', + isCompleted: true, + currentNode: null, + }) + + const { result } = renderHook(() => useOnboarding()) + + await act(async () => { + await result.current.submitStep('session-abc', 'node-1', { payload: {} }) + }) + + // Nothing left to authorise, and a credential left in sessionStorage outlives the visit on a + // shared machine. + expect(getApplicantToken()).toBeNull() + }) + + it('keeps the credential while the journey is still in progress', async () => { + const { setApplicantToken, getApplicantToken } = await import('../api/applicant-session') + setApplicantToken('token-mid-journey') + + mockApiSubmitStep.mockResolvedValueOnce(makeStep()) + + const { result } = renderHook(() => useOnboarding()) + + await act(async () => { + await result.current.submitStep('session-abc', 'node-1', { payload: {} }) + }) + + expect(getApplicantToken()).toBe('token-mid-journey') + }) }) diff --git a/src/frontend/src/onboarding/hooks/useOnboarding.ts b/src/frontend/src/onboarding/hooks/useOnboarding.ts index 1dc813f..8a4f196 100644 --- a/src/frontend/src/onboarding/hooks/useOnboarding.ts +++ b/src/frontend/src/onboarding/hooks/useOnboarding.ts @@ -6,9 +6,10 @@ import { getNextStep as apiGetNextStep, resolveWorkflowApiBase, ComplianceError, + ExpiredSessionError, } from '../api/workflow-api-client' import { createSessionEventSource } from './session-event-source' -import { withApplicantTokenQuery } from '../api/applicant-session' +import { clearApplicantToken, withApplicantTokenQuery } from '../api/applicant-session' const serverBase = (import.meta.env.VITE_API_BASE_URL ?? '').replace(/\/$/, '') const workflowApiBase = resolveWorkflowApiBase(serverBase) @@ -18,6 +19,9 @@ export function useOnboarding() { const [isLoading, setIsLoading] = useState(false) const [error, setError] = useState(null) const [isCompleted, setIsCompleted] = useState(false) + // Tracked apart from `error`: an expired credential is a dead end the applicant cannot retry + // past, and saying so is the difference between "start again" and an unexplained failure. + const [isExpired, setIsExpired] = useState(false) const eventSourceRef = useRef(null) const pollingRef = useRef | null>(null) const sessionIdRef = useRef(null) @@ -46,6 +50,7 @@ export function useOnboarding() { setStep(next) return next } catch (requestError) { + if (requestError instanceof ExpiredSessionError) setIsExpired(true) setError(requestError instanceof Error ? requestError.message : 'Unknown onboarding error') throw requestError } finally { @@ -67,6 +72,10 @@ export function useOnboarding() { setIsCompleted(true) setStep((prev) => prev ? { ...prev, isCompleted: true, currentNode: null } : null) eventSourceRef.current = null + // The journey is over, so the credential has nothing left to authorise. Dropping it + // keeps a finished applicant's token off a shared machine rather than leaving it in + // sessionStorage until the tab closes. + clearApplicantToken() }, onAbandoned: () => { eventSourceRef.current = null @@ -110,8 +119,10 @@ export function useOnboarding() { try { const next = await apiSubmitStep(serverBase, sessionId, nodeId, payload) setStep(next) + if (next.isCompleted) clearApplicantToken() return next } catch (requestError) { + if (requestError instanceof ExpiredSessionError) setIsExpired(true) // ComplianceErrors are handled by the form component — don't set global error if (!(requestError instanceof ComplianceError)) { setError(requestError instanceof Error ? requestError.message : 'Unknown onboarding error') @@ -126,6 +137,7 @@ export function useOnboarding() { step, isLoading, error, + isExpired, isCompleted, startSession, submitStep, diff --git a/src/frontend/src/onboarding/types/flow.ts b/src/frontend/src/onboarding/types/flow.ts index c35681f..75b2000 100644 --- a/src/frontend/src/onboarding/types/flow.ts +++ b/src/frontend/src/onboarding/types/flow.ts @@ -17,14 +17,18 @@ export interface SessionStepResponse { sessionId: string isCompleted: boolean currentNode: FlowNode | null -} - -/** Session start also returns the credential used for the rest of the journey. */ -export interface SessionStartResponse extends SessionStepResponse { + /** + * A replacement credential, returned on step submission so a journey still being worked on never + * outlives its own token. Absent when an operator submits: their credential already covers the + * session. + */ applicantToken?: string | null applicantTokenExpiresAt?: string | null } +/** Session start also returns the credential used for the rest of the journey. */ +export type SessionStartResponse = SessionStepResponse + export interface StartSessionRequest { flowId: string customerProfileId?: string From 2fdba1c185b7aa552627b9859d95220b3a3c9a01 Mon Sep 17 00:00:00 2001 From: MaximumTrainer Date: Sun, 13 Sep 2026 11:24:39 +0100 Subject: [PATCH 4/5] feat(frontend): show a session's analytics trail in the operator console The durable event trail had no reader. An operator investigating why an applicant stalled had to query the API by hand; the only reference to the endpoint anywhere in the frontend was a test asserting the applicant page does not call it. SessionDetail now renders the trail beside the submissions it already showed. Submissions say what was entered, the trail says how the applicant got there: events in occurrence order, each with its type, step, absolute timestamp, source, payload summary, and the gap since the previous event - the column that makes a stall visible. Server and client events are distinguished by label as well as styling, since a reader should not have to know the schema to interpret a gap. A gap is never rendered negative: a browser-timestamped client event can land fractionally before the server event it followed, and a negative gap would read as an ordering bug. Both empty states are named rather than left blank. A trail with no client events is normal - an ad blocker or a closed tab stops them reaching the API - and an empty trail may be a session whose events aged past the retention window, which after the sweep is indistinguishable from one that never had any. Read through operatorFetch and the admin session cookie, like every other operator view; the applicant page still never requests it. Co-Authored-By: Claude Opus 5 (1M context) --- docs/runbook.md | 21 ++ src/frontend/src/sessions/SessionDetail.tsx | 7 + .../src/sessions/SessionEventTrail.test.tsx | 190 ++++++++++++++++++ .../src/sessions/SessionEventTrail.tsx | 161 +++++++++++++++ src/frontend/src/sessions/eventTrailFormat.ts | 45 +++++ 5 files changed, 424 insertions(+) create mode 100644 src/frontend/src/sessions/SessionEventTrail.test.tsx create mode 100644 src/frontend/src/sessions/SessionEventTrail.tsx create mode 100644 src/frontend/src/sessions/eventTrailFormat.ts diff --git a/docs/runbook.md b/docs/runbook.md index 75f8166..2ccaca3 100644 --- a/docs/runbook.md +++ b/docs/runbook.md @@ -185,6 +185,27 @@ Events are stored in the `AnalyticsEvents` table and pruned by a background swee which a caller controls. Set `Analytics__DatabaseProvider__Enabled=false` to turn storage off entirely; the application keeps working and events go only to the log. +#### Reading one session's trail + +The operator console shows a session's trail under **Sessions → (a session) → Event trail**, beside +its submissions. Submissions say what the applicant entered; the trail says how they got there — +the order steps were seen in, and the gap between consecutive events, which is what makes a stall +visible. It reads `GET /api/analytics/sessions/{sessionId}/events` with the admin session cookie, +and is never requested from the applicant page. + +Two empty states are worth recognising when reading it: + +- **Server events only.** Client events are self-reported by the applicant's browser and can be + absent entirely — an ad blocker or a tab closed before the batch flushed stops them reaching the + API. The panel says so. It is not evidence of a fault. +- **No events at all.** Either none were raised, or they aged past `Analytics__RetentionDays` and + were deleted. The two are indistinguishable after the sweep, so the panel names both rather than + implying data was lost. Expect this for any session older than the retention window. + +Aggregate drop-off in the flow analytics view is still derived from session status and submissions, +not from this event stream, so per-step timing is currently visible per session but not across a +flow. + ### SAML Single Sign-On (Admin UI) The admin UI signs operators in over SAML 2.0 (`GET /auth/saml/login` → IdP → `POST /auth/saml/callback`). diff --git a/src/frontend/src/sessions/SessionDetail.tsx b/src/frontend/src/sessions/SessionDetail.tsx index 1358dd4..a1b78c5 100644 --- a/src/frontend/src/sessions/SessionDetail.tsx +++ b/src/frontend/src/sessions/SessionDetail.tsx @@ -1,5 +1,6 @@ import { useEffect, useState } from 'react' import { operatorFetch } from '../api/operator-api' +import { SessionEventTrail } from './SessionEventTrail' interface Submission { id: string @@ -74,6 +75,8 @@ export function SessionDetail({ sessionId, onBack }: SessionDetailProps) { {error &&

{error}

} {isLoading &&

Loading submissions…

} +

Submissions

+ {!isLoading && submissions.length === 0 && (

No submissions recorded for this session.

)} @@ -92,6 +95,10 @@ export function SessionDetail({ sessionId, onBack }: SessionDetailProps) { ))} )} + + {/* The complementary view: submissions say what was entered, the trail says how the applicant + got there and where the time went. */} + ) } diff --git a/src/frontend/src/sessions/SessionEventTrail.test.tsx b/src/frontend/src/sessions/SessionEventTrail.test.tsx new file mode 100644 index 0000000..4f27309 --- /dev/null +++ b/src/frontend/src/sessions/SessionEventTrail.test.tsx @@ -0,0 +1,190 @@ +import { render, screen, waitFor } from '@testing-library/react' +import { afterEach, describe, expect, it, vi } from 'vitest' +import { SessionEventTrail } from './SessionEventTrail' +import { formatGap } from './eventTrailFormat' +import type { AnalyticsTrailEvent } from './SessionEventTrail' + +/** + * The persisted trail is what an operator reads to find out why an applicant stalled. Three things + * have to hold for it to be readable: the order has to be the order events happened, a client event + * has to be distinguishable from a server one, and an empty panel must say which kind of empty it + * is — a session with no client events is normal, and one whose events aged out of retention is not + * the same as one that never had any. + */ + +const SESSION_ID = '11111111-1111-1111-1111-111111111111' + +function event(overrides: Partial & { eventId: string }): AnalyticsTrailEvent { + return { + eventType: 'step_view', + journeyId: '22222222-2222-2222-2222-222222222222', + sessionId: SESSION_ID, + stepId: 'identity', + stepIndex: 0, + payload: {}, + occurredAt: '2026-09-13T10:00:00.000Z', + source: 'server', + ...overrides, + } +} + +function mockTrail(events: AnalyticsTrailEvent[]) { + return vi.spyOn(globalThis, 'fetch').mockResolvedValue({ + ok: true, + status: 200, + json: async () => events, + } as Response) +} + +describe('SessionEventTrail', () => { + afterEach(() => { + vi.restoreAllMocks() + }) + + it('renders the trail in occurrence order', async () => { + // Deliberately out of order from the API, so the assertion is about the view, not the fixture. + mockTrail([ + event({ eventId: 'c', eventType: 'journey_complete', occurredAt: '2026-09-13T10:02:00.000Z' }), + event({ eventId: 'a', eventType: 'session_started', occurredAt: '2026-09-13T10:00:00.000Z' }), + event({ eventId: 'b', eventType: 'step_submitted', occurredAt: '2026-09-13T10:01:00.000Z' }), + ]) + + render() + + await waitFor(() => expect(screen.getAllByTestId('trail-event')).toHaveLength(3)) + + const rendered = screen.getAllByTestId('trail-event').map((node) => node.textContent) + expect(rendered[0]).toContain('session_started') + expect(rendered[1]).toContain('step_submitted') + expect(rendered[2]).toContain('journey_complete') + }) + + it('shows event type, step, timestamp and the gap since the previous event', async () => { + mockTrail([ + event({ eventId: 'a', eventType: 'session_started', occurredAt: '2026-09-13T10:00:00.000Z' }), + event({ + eventId: 'b', + eventType: 'step_submitted', + stepId: 'address', + stepIndex: 1, + occurredAt: '2026-09-13T10:02:30.000Z', + }), + ]) + + render() + + await waitFor(() => expect(screen.getAllByTestId('trail-event')).toHaveLength(2)) + + const second = screen.getAllByTestId('trail-event')[1] + expect(second.textContent).toContain('step_submitted') + expect(second.textContent).toContain('address') + expect(second.textContent).toContain('#1') + // The stall signal: two and a half minutes on one step. + expect(second.textContent).toContain('+2m 30s') + expect(second.textContent).toContain(new Date('2026-09-13T10:02:30.000Z').toLocaleString()) + }) + + it('distinguishes client events from server events', async () => { + mockTrail([ + event({ eventId: 'a', source: 'server', occurredAt: '2026-09-13T10:00:00.000Z' }), + event({ eventId: 'b', source: 'client', occurredAt: '2026-09-13T10:00:30.000Z' }), + ]) + + render() + + await waitFor(() => expect(screen.getAllByTestId('trail-event')).toHaveLength(2)) + + const [first, second] = screen.getAllByTestId('trail-event') + expect(first).toHaveAttribute('data-source', 'server') + expect(second).toHaveAttribute('data-source', 'client') + + // Labelled, not only coloured: a reader should not have to know the schema to interpret a gap. + expect(first.textContent).toContain('server') + expect(second.textContent).toContain('client') + }) + + it('says a trail with no client events is not an error', async () => { + mockTrail([ + event({ eventId: 'a', source: 'server' }), + event({ eventId: 'b', source: 'server', occurredAt: '2026-09-13T10:00:10.000Z' }), + ]) + + render() + + await waitFor(() => expect(screen.getAllByTestId('trail-event')).toHaveLength(2)) + + expect(screen.getByText(/Server events only/i)).toBeInTheDocument() + expect(screen.queryByRole('alert')).not.toBeInTheDocument() + }) + + it('does not show the server-only note when client events are present', async () => { + mockTrail([ + event({ eventId: 'a', source: 'server' }), + event({ eventId: 'b', source: 'client', occurredAt: '2026-09-13T10:00:10.000Z' }), + ]) + + render() + + await waitFor(() => expect(screen.getAllByTestId('trail-event')).toHaveLength(2)) + + expect(screen.queryByText(/Server events only/i)).not.toBeInTheDocument() + }) + + it('explains an empty trail rather than rendering a blank panel', async () => { + mockTrail([]) + + render() + + await waitFor(() => expect(screen.getByText(/No events recorded/i)).toBeInTheDocument()) + + // A session old enough to have aged out looks identical to one that never had events, so the + // empty state has to name both possibilities rather than implying data was lost. + expect(screen.getByText(/retention window/i)).toBeInTheDocument() + expect(screen.queryByRole('alert')).not.toBeInTheDocument() + }) + + it('reports a failed load as an error, not as an empty trail', async () => { + vi.spyOn(globalThis, 'fetch').mockResolvedValue({ + ok: false, + status: 500, + json: async () => ({}), + } as Response) + + render() + + await waitFor(() => expect(screen.getByRole('alert')).toBeInTheDocument()) + expect(screen.queryByText(/No events recorded/i)).not.toBeInTheDocument() + }) + + it('requests the trail through the operator credential', async () => { + const fetchSpy = mockTrail([]) + + render() + + await waitFor(() => expect(fetchSpy).toHaveBeenCalled()) + + const [url, init] = fetchSpy.mock.calls[0] + expect(url).toBe(`/api/analytics/sessions/${SESSION_ID}/events`) + // The admin session cookie, like every other operator view. Never an API key. + expect((init as RequestInit).credentials).toBe('include') + }) +}) + +describe('formatGap', () => { + it('shows no gap for the first event', () => { + expect(formatGap(null, '2026-09-13T10:00:00.000Z')).toBe('—') + }) + + it('scales the unit to the size of the gap', () => { + expect(formatGap('2026-09-13T10:00:00.000Z', '2026-09-13T10:00:00.400Z')).toBe('+400ms') + expect(formatGap('2026-09-13T10:00:00.000Z', '2026-09-13T10:00:05.000Z')).toBe('+5.0s') + expect(formatGap('2026-09-13T10:00:00.000Z', '2026-09-13T10:03:20.000Z')).toBe('+3m 20s') + expect(formatGap('2026-09-13T10:00:00.000Z', '2026-09-13T12:30:00.000Z')).toBe('+2h 30m') + }) + + it('does not render a negative gap from clock skew', () => { + // A client event timestamped by the applicant browser can land fractionally before the server + // event it followed. Showing a negative gap would read as an ordering bug. + expect(formatGap('2026-09-13T10:00:05.000Z', '2026-09-13T10:00:04.000Z')).toBe('—') + }) +}) diff --git a/src/frontend/src/sessions/SessionEventTrail.tsx b/src/frontend/src/sessions/SessionEventTrail.tsx new file mode 100644 index 0000000..778f229 --- /dev/null +++ b/src/frontend/src/sessions/SessionEventTrail.tsx @@ -0,0 +1,161 @@ +import { useEffect, useMemo, useState } from 'react' +import { operatorFetch } from '../api/operator-api' +import { formatGap, isClient, payloadSummary } from './eventTrailFormat' + +/** + * One session's persisted analytics trail. + * + * The submissions list next to this shows what an applicant entered. The trail shows how they got + * there: the order steps were seen in, where time went, which validations failed. That is what an + * operator investigating a stalled application actually needs, and until now it could only be had + * by querying the API by hand. + * + * Operator-only, through `operatorFetch` and the admin session cookie, like every other operator + * view. The applicant page never requests it. + */ +export interface AnalyticsTrailEvent { + eventId: string + eventType: string + journeyId: string + sessionId: string + stepId?: string | null + stepIndex?: number | null + payload?: Record | null + occurredAt: string + /** `server` for events the engine emits, `client` for events a browser reported. */ + source: string +} + +interface SessionEventTrailProps { + sessionId: string +} + +export function SessionEventTrail({ sessionId }: SessionEventTrailProps) { + const [events, setEvents] = useState([]) + const [isLoading, setIsLoading] = useState(false) + const [error, setError] = useState(null) + + useEffect(() => { + const controller = new AbortController() + + const load = async () => { + setIsLoading(true) + setError(null) + try { + const res = await operatorFetch( + `/api/analytics/sessions/${sessionId}/events`, + { signal: controller.signal }, + ) + if (!res.ok) throw new Error(`Failed to load the event trail (${res.status})`) + setEvents((await res.json()) as AnalyticsTrailEvent[]) + } catch (err) { + if (err instanceof Error && err.name !== 'AbortError') { + setError(err.message) + } + } finally { + setIsLoading(false) + } + } + + void load() + return () => controller.abort() + }, [sessionId]) + + // The API returns the trail in order; sorting here keeps the view correct if that ever changes, + // and a trail read out of order is worse than useless. + const ordered = useMemo( + () => [...events].sort((a, b) => new Date(a.occurredAt).getTime() - new Date(b.occurredAt).getTime()), + [events], + ) + + const clientEventCount = ordered.filter((event) => isClient(event.source)).length + + return ( +
+
+

+ Event trail +

+ {ordered.length > 0 && ( +

+ {ordered.length} event{ordered.length === 1 ? '' : 's'} +

+ )} +
+ + {error &&

{error}

} + {isLoading &&

Loading event trail…

} + + {!isLoading && !error && ordered.length === 0 && ( +
+

No events recorded for this session.

+

+ Either none were raised, or they have passed the analytics retention window and been + deleted. Older sessions show nothing here even when the journey was completed normally. +

+
+ )} + + {!isLoading && !error && ordered.length > 0 && clientEventCount === 0 && ( +

+ Server events only. Client events are reported by the applicant's browser and can be + absent entirely — an ad blocker or a closed tab stops them reaching the API. This is not an + error. +

+ )} + + {ordered.length > 0 && ( +
    + {ordered.map((event, index) => { + const gap = formatGap(index === 0 ? null : ordered[index - 1].occurredAt, event.occurredAt) + const client = isClient(event.source) + const summary = payloadSummary(event.payload) + + return ( +
  1. +
    + {event.eventType} + + {client ? 'client' : 'server'} + + {gap} +
    + +

    + {new Date(event.occurredAt).toLocaleString()} + · + + Step:{' '} + {event.stepId + ? {event.stepId} + : none} + {typeof event.stepIndex === 'number' ? ` (#${event.stepIndex})` : ''} + +

    + + {summary && ( +

    {summary}

    + )} +
    2. + ) + })} +
+ )} +
+ ) +} diff --git a/src/frontend/src/sessions/eventTrailFormat.ts b/src/frontend/src/sessions/eventTrailFormat.ts new file mode 100644 index 0000000..a462f7a --- /dev/null +++ b/src/frontend/src/sessions/eventTrailFormat.ts @@ -0,0 +1,45 @@ +/** + * Formatting for the session event trail, kept apart from the component so both can be tested and + * so the component file exports only a component. + */ + +/** + * Renders the gap since the previous event. This is the column that makes a stall visible: a + * fifteen-minute pause on one step reads very differently from fifteen seconds. + */ +export function formatGap(previousIso: string | null, currentIso: string): string { + if (previousIso === null) return '—' + + const gapMs = new Date(currentIso).getTime() - new Date(previousIso).getTime() + if (!Number.isFinite(gapMs)) return '—' + + // Clock skew between a browser and the server can put a client event fractionally before the + // server event it followed. Showing a negative gap would read as an ordering bug. + if (gapMs < 0) return '—' + if (gapMs < 1000) return `+${gapMs}ms` + + const seconds = gapMs / 1000 + if (seconds < 60) return `+${seconds.toFixed(1)}s` + + const minutes = Math.floor(seconds / 60) + if (minutes < 60) return `+${minutes}m ${Math.round(seconds % 60)}s` + + const hours = Math.floor(minutes / 60) + return `+${hours}h ${minutes % 60}m` +} + +export function payloadSummary(payload: Record | null | undefined): string | null { + if (!payload) return null + + const entries = Object.entries(payload) + if (entries.length === 0) return null + + return entries + .slice(0, 4) + .map(([key, value]) => `${key}: ${String(value)}`) + .join(', ') + (entries.length > 4 ? ', …' : '') +} + +export function isClient(source: string): boolean { + return source.toLowerCase() === 'client' +} From 3fbd2e33610b9a75ceb3d88989dfcacde45f206d Mon Sep 17 00:00:00 2001 From: MaximumTrainer Date: Sun, 13 Sep 2026 17:53:16 +0100 Subject: [PATCH 5/5] fix(api): attach the general rate limit policy to the endpoints it was for The policy was declared, partitioned and configurable, and attached to nothing. Every controller endpoint bar the three naming a specific policy was unlimited while appearing covered, and #99 asked for per-partition isolation tests on every policy - which was not reachable over HTTP for a policy no endpoint used. It is now added as endpoint metadata wherever no policy is already present. Not via RequireRateLimiting on the controller group: that applies to the whole group and, because the middleware takes the last matching metadata, it overrode the action-level attributes - session-start silently ran under the general budget instead of its own. A test pins that precedence so the two can never start stacking. Health checks stay outside it. Throttling the endpoint a load balancer polls turns a traffic spike into an instance being pulled from service. The three CI jobs that drive the API from a single address - which is one partition - get explicit headroom, keeping their behaviour as it was before the policy was attached. Rate limiting has its own dedicated tests; throttling those suites would only add flake. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/ci.yml | 24 ++++++ docs/runbook.md | 12 ++- src/backend/OpenOnboarding.Api/Program.cs | 16 +++- .../RateLimitPartitioningTests.cs | 85 +++++++++++++++++++ 4 files changed, 135 insertions(+), 2 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 7e66033..744137a 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -192,6 +192,14 @@ jobs: env: ConnectionStrings__OnboardingDb: "Host=localhost;Port=5432;Database=onboarding_test;Username=postgres;Password=postgres" + # These suites drive the API hard from a single address, which is one rate limit partition + # now that the "general" policy covers every controller endpoint. Rate limiting has its own + # dedicated tests; throttling here would only add flake, so the budgets are lifted to keep + # these jobs behaving as they did before the policy was attached. + RateLimiting__GeneralPerMinute: "100000" + RateLimiting__GlobalCeilingPerMinute: "100000" + RateLimiting__SessionStartPerMinute: "100000" + RateLimiting__AnalyticsIngestPerMinute: "100000" steps: - uses: actions/checkout@v4 @@ -284,6 +292,14 @@ jobs: env: ConnectionStrings__OnboardingDb: "Host=localhost;Port=5432;Database=onboarding_test;Username=postgres;Password=postgres" + # This suite drives the API from a single address, which is one rate limit partition now + # that the "general" policy covers every controller endpoint. Rate limiting has its own + # dedicated tests; throttling here would only add flake, so the budgets are lifted to keep + # this job behaving as it did before the policy was attached. + RateLimiting__GeneralPerMinute: "100000" + RateLimiting__GlobalCeilingPerMinute: "100000" + RateLimiting__SessionStartPerMinute: "100000" + RateLimiting__AnalyticsIngestPerMinute: "100000" steps: - uses: actions/checkout@v4 @@ -342,6 +358,14 @@ jobs: ASPNETCORE_URLS: http://localhost:5072 Authentication__ApiKey: playwright-api-key Authentication__JwtAuthority: "" + # These suites drive the API hard from a single address, which is one rate limit partition + # now that the "general" policy covers every controller endpoint. Rate limiting has its own + # dedicated tests; throttling here would only add flake, so the budgets are lifted to keep + # these jobs behaving as they did before the policy was attached. + RateLimiting__GeneralPerMinute: "100000" + RateLimiting__GlobalCeilingPerMinute: "100000" + RateLimiting__SessionStartPerMinute: "100000" + RateLimiting__AnalyticsIngestPerMinute: "100000" steps: - uses: actions/checkout@v4 diff --git a/docs/runbook.md b/docs/runbook.md index 2ccaca3..9a231a7 100644 --- a/docs/runbook.md +++ b/docs/runbook.md @@ -498,12 +498,22 @@ other. Configure the limits under `RateLimiting` in `appsettings.json` or as | `session-start` | `POST /api/workflow/sessions/start` | Client IP — the endpoint is anonymous by design | `RateLimiting:SessionStartPerMinute` | 100 | | `analytics-ingest` | `POST /api/analytics/events` | Applicant session id from the token; operators fall back to their principal | `RateLimiting:AnalyticsIngestPerMinute` | 120 | | `webhook-registration` | `POST /api/flows/{flowId}/webhooks` | Authenticated principal, falling back to client IP | `RateLimiting:WebhookRegistrationPerMinute` | 20 | -| `general` | declared, not yet attached to an endpoint | Authenticated principal, falling back to client IP | `RateLimiting:GeneralPerMinute` | 300 | +| `general` | every other controller endpoint | Authenticated principal, falling back to client IP | `RateLimiting:GeneralPerMinute` | 300 | | global ceiling | every request | nothing — one bucket for the whole instance | `RateLimiting:GlobalCeilingPerMinute` | 3000 | The global ceiling runs **in addition to** the endpoint policy, so a flood spread across thousands of partitions still has a bound. A request rejected by either returns `429` with `Retry-After: 60`. +`general` is the fallback: it is attached to every controller endpoint that does not declare a +policy of its own, so no part of the API is unlimited. An endpoint naming a specific policy is +**not** additionally bound by it — the two do not stack, and `session-start` runs under its own +budget alone. Health check endpoints are outside it entirely, since throttling the endpoint a load +balancer polls turns a traffic spike into an instance being pulled out of service. + +One consequence worth sizing for: callers sharing a credential share a partition. Every integration +using the same API key spends one `general` budget between them, because the partition key is the +principal. Give separate integrations separate credentials, or raise the limit to cover them all. + #### Behind a proxy The partition key is the connection address, which behind a load balancer is the balancer's own diff --git a/src/backend/OpenOnboarding.Api/Program.cs b/src/backend/OpenOnboarding.Api/Program.cs index b909d03..b5f6f3c 100644 --- a/src/backend/OpenOnboarding.Api/Program.cs +++ b/src/backend/OpenOnboarding.Api/Program.cs @@ -410,7 +410,21 @@ await context.Response.WriteAsJsonAsync(new ProblemDetails } }).AllowAnonymous(); -app.MapControllers(); +// Every controller endpoint falls under the "general" policy unless it names a more specific one. +// The policy was previously declared and attached to nothing, so most of the API was unlimited +// while appearing covered. +// +// Added as metadata only where no policy is already present, rather than with +// RequireRateLimiting: that applies to the whole group, and because the middleware takes the last +// matching metadata it would override the action-level attributes - session-start would silently +// run under the general budget instead of its own. Health checks are mapped above and stay outside. +app.MapControllers().Add(endpoint => +{ + if (endpoint.Metadata.OfType().Any()) return; + if (endpoint.Metadata.OfType().Any()) return; + + endpoint.Metadata.Add(new EnableRateLimitingAttribute("general")); +}); app.MapMetrics().RequireAuthorization().DisableRateLimiting(); diff --git a/src/backend/OpenOnboarding.Application.Tests/RateLimitPartitioningTests.cs b/src/backend/OpenOnboarding.Application.Tests/RateLimitPartitioningTests.cs index b117d37..e005d47 100644 --- a/src/backend/OpenOnboarding.Application.Tests/RateLimitPartitioningTests.cs +++ b/src/backend/OpenOnboarding.Application.Tests/RateLimitPartitioningTests.cs @@ -221,6 +221,79 @@ public async Task WebhookRegistration_ExhaustedByOnePrincipal_StillAdmitsADiffer Assert.NotEqual(HttpStatusCode.TooManyRequests, anonymous.StatusCode); } + [Fact] + public async Task General_ExhaustedByOneCaller_StillAdmitsADifferentPartition() + { + // The fallback policy covers every controller endpoint that does not name a tighter one, + // which is most of the API. Partitioning matters here for the same reason it does on + // session-start: one caller must not be able to close the API to everyone else. + using var factory = CreateFactory(new Dictionary + { + ["RateLimiting:GeneralPerMinute"] = Limit.ToString() + }); + + using var client = factory.CreateClient(); + + for (var i = 0; i < Limit + 1; i++) + await client.SendAsync(GeneralRequest(ClientA)); + + var limited = await client.SendAsync(GeneralRequest(ClientA)); + Assert.Equal(HttpStatusCode.TooManyRequests, limited.StatusCode); + + // A different caller reaches authorization and is refused there, not by the exhausted + // caller's limiter. + var other = await client.SendAsync(GeneralRequest(ClientB)); + + Assert.NotEqual(HttpStatusCode.TooManyRequests, other.StatusCode); + } + + [Fact] + public async Task AnEndpointWithItsOwnPolicy_IsNotAlsoBoundByTheGeneralFallback() + { + // Attaching "general" to every controller must not silently stack a second limiter onto the + // endpoints that already declare one - session-start would then be capped by whichever of + // the two is tighter, which is not what either setting says. + using var factory = CreateFactory(new Dictionary + { + ["RateLimiting:GeneralPerMinute"] = "2", + ["RateLimiting:SessionStartPerMinute"] = "1000" + }); + + using var operatorClient = factory.CreateClient(); + operatorClient.DefaultRequestHeaders.Add("X-Api-Key", "test-api-key"); + var flowId = await CreateFlowAsync(operatorClient); + + using var client = factory.CreateClient(); + + for (var i = 0; i < 6; i++) + { + var response = await client.SendAsync(StartRequest(flowId, ClientA)); + Assert.NotEqual(HttpStatusCode.TooManyRequests, response.StatusCode); + } + } + + [Fact] + public async Task TheGeneralFallback_DoesNotApplyToHealthChecks() + { + // Health checks are what a load balancer polls, often several times a second. Rate limiting + // them turns a traffic spike into an instance being marked unhealthy and pulled out. + using var factory = CreateFactory(new Dictionary + { + ["RateLimiting:GeneralPerMinute"] = "2" + }); + + using var client = factory.CreateClient(); + + for (var i = 0; i < 6; i++) + { + var request = new HttpRequestMessage(HttpMethod.Get, "/health/live"); + request.Headers.Add(ClientAddressStartupFilter.HeaderName, ClientA); + var response = await client.SendAsync(request); + + Assert.NotEqual(HttpStatusCode.TooManyRequests, response.StatusCode); + } + } + [Fact] public async Task GlobalCeiling_BoundsAFloodSpreadAcrossManyPartitions() { @@ -295,6 +368,18 @@ public void Principal_WithoutACredential_FallsBackToTheClientAddress() RateLimitPartitionKeys.Principal(context)); } + /// + /// A request to an endpoint that declares no policy of its own, so it falls to the "general" + /// fallback. Sent anonymously: the limiter runs before authorization, so an unauthenticated + /// caller still spends its partition's budget and a refusal tells the two apart by status. + /// + private static HttpRequestMessage GeneralRequest(string clientAddress) + { + var request = new HttpRequestMessage(HttpMethod.Get, "/api/flows"); + request.Headers.Add(ClientAddressStartupFilter.HeaderName, clientAddress); + return request; + } + private static HttpRequestMessage WebhookRequest(string clientAddress, string? apiKey) { var request = new HttpRequestMessage(HttpMethod.Post, $"/api/flows/{Guid.NewGuid()}/webhooks")