diff --git a/docs/guide/http/exception-handling.md b/docs/guide/http/exception-handling.md index 081f31327..b7617f566 100644 --- a/docs/guide/http/exception-handling.md +++ b/docs/guide/http/exception-handling.md @@ -248,6 +248,39 @@ methods are optimistic and throw `EventStreamUnexpectedMaxEventIdException`, whi `ConcurrencyException`. ::: +## Unknown Tenants as 404 + +There are two different tenancy failures on an HTTP request, and only one of them was handled: + +| Failure | Meaning | Status | +| --- | --- | --- | +| **Missing** tenant id | "you did not say which tenant" | 400, already handled by `[RequiresTenant]` / `TenantId.AssertExists()` | +| **Unknown** tenant id | "the tenant you named does not exist" | was an unhandled **500** | + +An unknown tenant id throws `JasperFx.MultiTenancy.UnknownTenantIdException` from the store or from +Wolverine's own tenant sources. That is a client side error, so: + +```csharp +app.MapWolverineEndpoints(opts => +{ + opts.MapUnknownTenantToNotFound(); +}); +``` + +maps it to a 404 `ProblemDetails` titled `Unknown tenant`, and stamps `ProducesProblem(404)` on the tenanted +chains so your OpenAPI document advertises it. It applies by default to chains declared `[RequiresTenant]` or +`[MaybeTenanted]` -- a chain that resolves no tenant cannot fail to resolve one -- and takes the same optional +predicate the other mappings do. + +::: tip Why 404 and not 400 +404 reads as "the thing you addressed does not exist", which keeps 400 meaning "you did not say which +tenant". Collapsing both onto one status loses the distinction a caller needs to tell a routing bug from a +provisioning one. +::: + +For **message handlers** the equivalent guidance is `OnException().MoveToErrorQueue()` +-- never retry it, since it is deterministic. + ### Rolling your own If you want different status codes, a different `ProblemDetails` shape, or extra exception types, the diff --git a/src/Http/Wolverine.Http.Tests/unknown_tenant_problem_details_4516.cs b/src/Http/Wolverine.Http.Tests/unknown_tenant_problem_details_4516.cs new file mode 100644 index 000000000..dd9a7b321 --- /dev/null +++ b/src/Http/Wolverine.Http.Tests/unknown_tenant_problem_details_4516.cs @@ -0,0 +1,155 @@ +using Alba; +using JasperFx.MultiTenancy; +using Microsoft.AspNetCore.Builder; +using Microsoft.AspNetCore.Http.Metadata; +using Microsoft.AspNetCore.Mvc; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Hosting; +using Shouldly; +using Xunit; + +namespace Wolverine.Http.Tests; + +/// +/// GH-4516. A missing mandatory tenant id was already handled -- [RequiresTenant] stops the request +/// with a 400 ProblemDetails. An unknown one, present on the request but with no database or +/// registration behind it, threw UnknownTenantIdException, which Wolverine.Http did not catch, so the client +/// got a 500 for what is a client side error. +/// +public class unknown_tenant_problem_details_4516 +{ + [Fact] + public async Task an_unknown_tenant_id_is_a_404_problem_details() + { + await using var host = await startHostAsync(mapUnknownTenant: true); + + var result = await host.Scenario(x => + { + x.Get.Url("/gh4516/tenanted?tenantId=ghost"); + x.StatusCodeShouldBe(404); + }); + + var problem = await result.ReadAsJsonAsync(); + + problem.ShouldNotBeNull(); + problem.Status.ShouldBe(404); + problem.Title.ShouldBe("Unknown tenant"); + problem.Detail.ShouldNotBeNull(); + problem.Detail.ShouldContain("ghost"); + } + + [Fact] + public async Task a_known_tenant_is_untouched() + { + await using var host = await startHostAsync(mapUnknownTenant: true); + + var result = await host.Scenario(x => + { + x.Get.Url("/gh4516/tenanted?tenantId=acme"); + x.StatusCodeShouldBeOk(); + }); + + (await result.ReadAsTextAsync()).ShouldBe("acme"); + } + + [Fact] + public async Task a_missing_tenant_id_still_answers_400_not_404() + { + // The two failures are genuinely different -- "you did not say which tenant" versus "the tenant you + // named does not exist" -- and this mapping must not collapse them onto one status. + await using var host = await startHostAsync(mapUnknownTenant: true); + + await host.Scenario(x => + { + x.Get.Url("/gh4516/tenanted"); + x.StatusCodeShouldBe(400); + }); + } + + [Fact] + public async Task without_the_opt_in_the_unknown_tenant_still_escapes() + { + // Proves the opt in is what changes the answer, rather than something else in the pipeline. + await using var host = await startHostAsync(mapUnknownTenant: false); + + await Should.ThrowAsync(async () => + await host.Scenario(x => + { + x.Get.Url("/gh4516/tenanted?tenantId=ghost"); + x.StatusCodeShouldBe(404); + })); + } + + [Fact] + public async Task the_404_is_advertised_only_on_the_tenanted_chains() + { + await using var host = await startHostAsync(mapUnknownTenant: true); + + var chains = host.Services.GetRequiredService().Endpoints!.Chains; + + var tenanted = chains.Single(x => x.Method.Method.Name == nameof(Gh4516Endpoint.Tenanted)); + tenanted.BuildEndpoint(RouteWarmup.Lazy) + .Metadata.OfType() + .Select(x => x.StatusCode) + .ShouldContain(404); + + // ...and a chain that resolves no tenant cannot fail to resolve one, so it must not advertise it + var notTenanted = chains.Single(x => x.Method.Method.Name == nameof(Gh4516Endpoint.NotTenanted)); + notTenanted.BuildEndpoint(RouteWarmup.Lazy) + .Metadata.OfType() + .Select(x => x.StatusCode) + .ShouldNotContain(404); + } + + private static async Task startHostAsync(bool mapUnknownTenant) + { + var builder = WebApplication.CreateBuilder([]); + + builder.Host.UseWolverine(opts => + { + opts.Durability.Mode = DurabilityMode.MediatorOnly; + opts.Discovery.DisableConventionalDiscovery(); + opts.Discovery.IncludeAssembly(typeof(unknown_tenant_problem_details_4516).Assembly); + }); + + builder.Services.AddWolverineHttp(); + + return await AlbaHost.For(builder, app => app.MapWolverineEndpoints(opts => + { + opts.TenantId.IsQueryStringValue("tenantId"); + + // Without AssertExists(), a missing tenant id binds *DEFAULT* and reaches the handler -- + // which is what the a_missing_tenant_id_still_answers_400_not_404 test is guarding. + opts.TenantId.AssertExists(); + + if (mapUnknownTenant) + { + opts.MapUnknownTenantToNotFound(); + } + + opts.CustomizeHttpEndpointDiscovery(q => + q.Excludes.WithCondition("Not the GH-4516 endpoint", type => type != typeof(Gh4516Endpoint))); + })); + } +} + +public static class Gh4516Endpoint +{ + // Stands in for a store or tenant source that does not know the id. Anything that resolves a tenant + // -- Marten's database source, Wolverine's own tenant sources -- throws this same exception. + [RequiresTenant] + [WolverineGet("/gh4516/tenanted")] + public static string Tenanted(IMessageBus bus) + { + if (bus.TenantId != "acme") + { + throw new UnknownTenantIdException(bus.TenantId!); + } + + return bus.TenantId!; + } + + [NotTenanted] + [WolverineGet("/gh4516/anonymous")] + public static string NotTenanted() => "no tenant here"; +} diff --git a/src/Http/Wolverine.Http/Runtime/TenancyProblemMapping.cs b/src/Http/Wolverine.Http/Runtime/TenancyProblemMapping.cs new file mode 100644 index 000000000..af357b794 --- /dev/null +++ b/src/Http/Wolverine.Http/Runtime/TenancyProblemMapping.cs @@ -0,0 +1,85 @@ +using JasperFx; +using JasperFx.CodeGeneration; +using JasperFx.CodeGeneration.Model; +using JasperFx.MultiTenancy; +using Microsoft.AspNetCore.Builder; +using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Mvc; + +namespace Wolverine.Http.Runtime; + +/// +/// GH-4516. Maps an unknown tenant id onto a 404 ProblemDetails instead of letting it escape as a 500. +/// +/// +/// A missing mandatory tenant id is already handled: [RequiresTenant] stops the request with a +/// 400 ProblemDetails through HttpHandler.WriteTenantIdNotFound. But a tenant id that is present on +/// the request and simply has no database or registration behind it throws +/// from the store or from Wolverine's own tenant sources, which +/// Wolverine.Http did not catch -- so the client got a 500 for what is a client-side error. +/// +/// +/// +/// 404 rather than 400 on purpose: it reads as "the thing you addressed does not exist", and it keeps 400 +/// meaning "you did not say which tenant", which is the distinction the two failures actually have. +/// +/// +public static class UnknownTenantMiddleware +{ + public static ProblemDetails OnException(UnknownTenantIdException ex) + { + return TenancyProblemMapping.ToUnknownTenantProblemDetails(ex); + } +} + +/// +/// GH-4516. The one place the tenancy ProblemDetails are shaped, so they cannot drift from each other or +/// from . +/// +public static class TenancyProblemMapping +{ + public const int UnknownTenantStatusCode = StatusCodes.Status404NotFound; + + public static ProblemDetails ToUnknownTenantProblemDetails(Exception ex) + { + return new ProblemDetails + { + Status = UnknownTenantStatusCode, + Title = "Unknown tenant", + Detail = ex.Message + }; + } + + /// + /// The default set of chains this mapping applies to: the ones that can actually resolve a tenant, and + /// so are the only ones that can fail to. A chain that is explicitly + /// -- or that never declared a tenancy mode at all -- + /// has nothing to report. + /// + public static bool IsTenanted(HttpChain chain) + { + return chain.TenancyMode is TenancyMode.Required or TenancyMode.Maybe; + } +} + +/// +/// GH-4516. Stamps ProducesProblem(404) on the tenanted chains, so the OpenAPI document advertises +/// the unknown-tenant response the same way advertises the conflict. +/// +public class UnknownTenantProblemPolicy : IHttpPolicy +{ + private readonly Func _filter; + + public UnknownTenantProblemPolicy(Func filter) + { + _filter = filter; + } + + public void Apply(IReadOnlyList chains, GenerationRules rules, IServiceContainer container) + { + foreach (var chain in chains.Where(_filter)) + { + chain.Metadata.ProducesProblem(TenancyProblemMapping.UnknownTenantStatusCode); + } + } +} diff --git a/src/Http/Wolverine.Http/WolverineHttpOptions.cs b/src/Http/Wolverine.Http/WolverineHttpOptions.cs index 00b242c2c..a46d83693 100644 --- a/src/Http/Wolverine.Http/WolverineHttpOptions.cs +++ b/src/Http/Wolverine.Http/WolverineHttpOptions.cs @@ -481,6 +481,42 @@ public void MapConcurrencyFailuresToConflict(Func? filter = nul Policies.Add(new ConflictProblemPolicy(filter)); } + /// + /// GH-4516. Map an unknown tenant id onto a 404 ProblemDetails instead of letting it escape as an + /// unhandled 500, and advertise the 404 in the OpenAPI document. + /// + /// + /// A missing mandatory tenant id is already handled without this: [RequiresTenant] stops + /// the request with a 400 ProblemDetails. This covers the other case -- a tenant id that is present on + /// the request and simply has no database or registration behind it, which throws + /// from the store or from Wolverine's own + /// tenant sources. That is a client-side error, and it was answering 500. + /// + /// + /// + /// 404 rather than 400 on purpose: it reads as "the thing you addressed does not exist", and it keeps + /// 400 meaning "you did not say which tenant". + /// + /// + /// + /// A disabled tenant would deserve a 403 -- the tenant exists and access is refused -- but on + /// Marten and Polecat it is indistinguishable from unknown today, and JasperFx has no lifted + /// DisabledTenantException to catch. When it lands, it belongs here. + /// + /// + /// + /// Which chains the mapping applies to. Defaults to the tenanted chains -- those declared + /// [RequiresTenant] or [MaybeTenanted] -- since a chain that resolves no tenant cannot + /// fail to resolve one. + /// + public void MapUnknownTenantToNotFound(Func? filter = null) + { + filter ??= TenancyProblemMapping.IsTenanted; + + AddMiddleware(typeof(UnknownTenantMiddleware), filter); + Policies.Add(new UnknownTenantProblemPolicy(filter)); + } + /// /// Add a new IResourceWriterPolicy for the Wolverine endpoints ///