From e53fe1a82d7adb7114c4725667bc6385efc0436a Mon Sep 17 00:00:00 2001 From: iammukeshm Date: Tue, 3 Nov 2026 22:10:08 +0530 Subject: [PATCH] fix(identity): enforce group and endpoint RequirePermission together RequiredPermissionAuthorizationHandler read only the most specific IRequiredPermissionMetadata, so an endpoint's .RequirePermission(B) replaced its route group's .RequirePermission(A) and the group gate failed open. The handler now reads every instance with GetOrderedMetadata, unions their permissions and requires all of them. A blank entry anywhere in the chain still denies. Closes #1429 --- .../RequiredPermissionAuthorizationHandler.cs | 14 ++++-- .../TestPermissionEndpointStartupFilter.cs | 13 +++++- .../RequirePermissionAllOfTests.cs | 45 +++++++++++++++++-- 3 files changed, 64 insertions(+), 8 deletions(-) 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)