diff --git a/src/Modules/Identity/Modules.Identity/Authorization/RequiredPermissionAuthorizationHandler.cs b/src/Modules/Identity/Modules.Identity/Authorization/RequiredPermissionAuthorizationHandler.cs index 4bc389bec4..6d7fa36c8e 100644 --- a/src/Modules/Identity/Modules.Identity/Authorization/RequiredPermissionAuthorizationHandler.cs +++ b/src/Modules/Identity/Modules.Identity/Authorization/RequiredPermissionAuthorizationHandler.cs @@ -23,8 +23,10 @@ protected override async Task HandleRequirementAsync(AuthorizationHandlerContext // IMPORTANT: resolve IRequiredPermissionMetadata from FSH.Framework.Shared.Identity.Authorization (the // interface the attribute implements) — a duplicate would silently fail-open every .RequirePermission(). - var requiredPermissions = endpoint?.Metadata.GetMetadata()?.RequiredPermissions; - if (requiredPermissions == null) + // Read every instance, not just the most specific one: a route group's .RequirePermission(A) and an + // endpoint's .RequirePermission(B) are both gates, so the caller must hold the union of them (#1429). + var permissionMetadata = endpoint?.Metadata.GetOrderedMetadata() ?? []; + if (permissionMetadata.Count == 0) { // there are no permission requirements set by the endpoint // hence, authorize requests @@ -33,11 +35,17 @@ protected override async Task HandleRequirementAsync(AuthorizationHandlerContext } // An empty set comes from a blank RequirePermission("") and must deny, not fall through to Succeed. - if (requiredPermissions.Count == 0 || context.User?.GetUserId() is not { } userId) + if (permissionMetadata.Any(m => m.RequiredPermissions.Count == 0) || context.User?.GetUserId() is not { } userId) { return; } + var requiredPermissions = new HashSet(StringComparer.OrdinalIgnoreCase); + foreach (var metadata in permissionMetadata) + { + requiredPermissions.UnionWith(metadata.RequiredPermissions); + } + var cancellationToken = httpContext?.RequestAborted ?? CancellationToken.None; foreach (var permission in requiredPermissions) { diff --git a/src/Tests/Integration.Tests/Infrastructure/TestPermissionEndpointStartupFilter.cs b/src/Tests/Integration.Tests/Infrastructure/TestPermissionEndpointStartupFilter.cs index 1c80946e2c..2f79b199b3 100644 --- a/src/Tests/Integration.Tests/Infrastructure/TestPermissionEndpointStartupFilter.cs +++ b/src/Tests/Integration.Tests/Infrastructure/TestPermissionEndpointStartupFilter.cs @@ -8,13 +8,14 @@ namespace Integration.Tests.Infrastructure; /// -/// No production endpoint passes more than one permission to .RequirePermission(), so this -/// maps one that does. It appends to the live route builder after next(app) has run +/// No production endpoint passes more than one permission to .RequirePermission() or gates a +/// route group with it, so this maps endpoints that do. It appends to the live route builder after next(app) has run /// UseRouting(), the same way Integration.Middleware.Tests maps /__test/throw. /// public sealed class TestPermissionEndpointStartupFilter : IStartupFilter { public const string TwoPermissionsPath = "/__test/permissions/two"; + public const string GroupGatedPath = "/__test/permissions/group/endpoint"; public const string FirstPermission = IdentityPermissions.Groups.Create; public const string SecondPermission = IdentityPermissions.Groups.Delete; @@ -34,6 +35,14 @@ public Action Configure(Action next) routeBuilder.MapGet(TwoPermissionsPath, () => Results.Ok()) .RequireAuthorization() .RequirePermission(FirstPermission, SecondPermission); + + // Group gate (first) + endpoint gate (second): both must apply, the endpoint's + // metadata must not replace the group's. + routeBuilder.MapGroup("/__test/permissions/group") + .RequireAuthorization() + .RequirePermission(FirstPermission) + .MapGet("/endpoint", () => Results.Ok()) + .RequirePermission(SecondPermission); } }; } diff --git a/src/Tests/Integration.Tests/Tests/Authorization/RequirePermissionAllOfTests.cs b/src/Tests/Integration.Tests/Tests/Authorization/RequirePermissionAllOfTests.cs index 5856fc306d..b3a75acab5 100644 --- a/src/Tests/Integration.Tests/Tests/Authorization/RequirePermissionAllOfTests.cs +++ b/src/Tests/Integration.Tests/Tests/Authorization/RequirePermissionAllOfTests.cs @@ -9,9 +9,10 @@ namespace Integration.Tests.Tests.Authorization; /// -/// .RequirePermission(a, b) must demand every listed permission. Probed through -/// , a test-only endpoint, -/// because no production endpoint lists more than one. +/// .RequirePermission(a, b) must demand every listed permission, and a route group's +/// .RequirePermission(a) must still apply when an endpoint inside it adds .RequirePermission(b). +/// Probed through test-only endpoints from , because no +/// production endpoint lists more than one permission or gates a group. /// [Collection(FshCollectionDefinition.Name)] public sealed class RequirePermissionAllOfTests @@ -51,6 +52,44 @@ public async Task TwoPermissionEndpoint_Should_Return200_When_UserHoldsBothPermi $"Holding every required permission must open the gate: {await response.Content.ReadAsStringAsync()}"); } + [Fact] + public async Task GroupGatedEndpoint_Should_Return403_When_UserHoldsOnlyTheEndpointPermission() + { + using var userClient = await CreateUserClientWithPermissionsAsync( + "group-endpoint-only", TestPermissionEndpointStartupFilter.SecondPermission); + + var response = await userClient.GetAsync(TestPermissionEndpointStartupFilter.GroupGatedPath); + + response.StatusCode.ShouldBe(HttpStatusCode.Forbidden, + "The endpoint's RequirePermission must not replace the route group's gate."); + } + + [Fact] + public async Task GroupGatedEndpoint_Should_Return403_When_UserHoldsOnlyTheGroupPermission() + { + using var userClient = await CreateUserClientWithPermissionsAsync( + "group-group-only", TestPermissionEndpointStartupFilter.FirstPermission); + + var response = await userClient.GetAsync(TestPermissionEndpointStartupFilter.GroupGatedPath); + + response.StatusCode.ShouldBe(HttpStatusCode.Forbidden, + "Holding only the group's permission must not open an endpoint that adds its own gate."); + } + + [Fact] + public async Task GroupGatedEndpoint_Should_Return200_When_UserHoldsGroupAndEndpointPermissions() + { + using var userClient = await CreateUserClientWithPermissionsAsync( + "group-both", + TestPermissionEndpointStartupFilter.FirstPermission, + TestPermissionEndpointStartupFilter.SecondPermission); + + var response = await userClient.GetAsync(TestPermissionEndpointStartupFilter.GroupGatedPath); + + response.StatusCode.ShouldBe(HttpStatusCode.OK, + $"Holding both the group and endpoint permissions must open the gate: {await response.Content.ReadAsStringAsync()}"); + } + // ─── helpers ───────────────────────────────────────────────────── private async Task CreateUserClientWithPermissionsAsync(string handlePrefix, params string[] permissions)