fix(identity): group-derived roles confer permissions, not just JWT claims

iammukeshm · claude · iammukeshm · commit be91d4bde8b1 · 2026-06-20T03:42:55.000+05:30
UserPermissionService computed the effective permission set from a user's
DIRECT roles only, while IdentityService.AddRoleClaimsAsync already unions
direct + group-derived roles when minting the JWT. Result: a user whose only
role comes via a UserGroup saw the role in their token but failed every
.RequirePermission() gate (and GET /identity/permissions under-reported).

Union direct roles with roles reachable via UserGroups -&gt; GroupRoles before
resolving permissions (group mutations already invalidate this cache entry).
Query-only — no schema change.

Tests:
- GroupRolePermissionTests (integration): a group-only user's own-permissions
  include the group role's grants and pass the gated endpoint.
- AuthorizationMetadataTests (architecture): RequiredPermissionAttribute exists
  exactly once implementing IRequiredPermissionMetadata, so gates can't silently
  fail open via a duplicate.

Co-Authored-By: Claude Opus 4.8 (1M context) &lt;noreply@anthropic.com&gt;
diff --git a/src/Modules/Identity/Modules.Identity/Services/UserPermissionService.cs b/src/Modules/Identity/Modules.Identity/Services/UserPermissionService.cs
@@ -70,11 +70,25 @@ private static async ValueTask<PermissionSet> LoadPermissionsAsync(FactoryState
 
         var userRoles = await s.UserManager.GetRolesAsync(user).ConfigureAwait(false);
 
-        var roleIds = await s.RoleManager.Roles
+        var directRoleIds = await s.RoleManager.Roles
             .Where(r => userRoles.Contains(r.Name!))
             .Select(r => r.Id)
             .ToListAsync(ct).ConfigureAwait(false);
 
+        // Group-derived roles confer permissions too — the JWT already unions them
+        // (IdentityService.AddRoleClaimsAsync) and every group mutation invalidates this
+        // cache entry, so the effective set must include roles reachable via UserGroups.
+        var groupRoleIds = await s.Db.GroupRoles
+            .Where(gr => s.Db.UserGroups
+                .Where(ug => ug.UserId == s.UserId)
+                .Select(ug => ug.GroupId)
+                .Contains(gr.GroupId))
+            .Select(gr => gr.RoleId)
+            .Distinct()
+            .ToListAsync(ct).ConfigureAwait(false);
+
+        var roleIds = directRoleIds.Union(groupRoleIds, StringComparer.Ordinal).ToList();
+
         if (roleIds.Count == 0)
         {
             return PermissionSet.Empty;
diff --git a/src/Tests/Architecture.Tests/AuthorizationMetadataTests.cs b/src/Tests/Architecture.Tests/AuthorizationMetadataTests.cs
@@ -0,0 +1,96 @@
+using FSH.Framework.Shared.Identity.Authorization;
+using Shouldly;
+using System.Reflection;
+using Xunit;
+
+namespace Architecture.Tests;
+
+/// <summary>
+/// Guards the permission-metadata contract between <c>RequiredPermissionAttribute</c> and
+/// <c>RequiredPermissionAuthorizationHandler</c>. The handler resolves endpoint permissions via
+/// <see cref="IRequiredPermissionMetadata"/>; if a duplicate <c>RequiredPermissionAttribute</c>
+/// appears in another assembly without implementing that interface, endpoints decorated with it
+/// carry no recognizable metadata and every <c>.RequirePermission()</c> gate silently fails open.
+/// </summary>
+public class AuthorizationMetadataTests
+{
+    private const string AttributeName = "RequiredPermissionAttribute";
+    private const string ExpectedNamespace = "FSH.Framework.Shared.Identity.Authorization";
+
+    [Fact]
+    public void RequiredPermissionAttribute_Should_Exist_Exactly_Once_Across_All_FSH_Assemblies()
+    {
+        var matches = GetAllFshAssemblies()
+            .SelectMany(GetLoadableTypes)
+            .Where(t => string.Equals(t.Name, AttributeName, StringComparison.Ordinal))
+            .ToArray();
+
+        matches.ShouldNotBeEmpty(
+            $"{AttributeName} was not found in any FSH assembly. " +
+            "The permission authorization pipeline depends on it.");
+
+        matches.Length.ShouldBe(1,
+            $"Exactly one {AttributeName} must exist across all FSH assemblies. " +
+            "A duplicate that does not implement IRequiredPermissionMetadata silently disables " +
+            $"every .RequirePermission() gate. Found: {string.Join(", ", matches.Select(t => $"{t.FullName} ({t.Assembly.GetName().Name})"))}");
+
+        matches[0].Namespace.ShouldBe(ExpectedNamespace,
+            $"{AttributeName} must live in {ExpectedNamespace}, where " +
+            "RequiredPermissionAuthorizationHandler resolves its metadata from.");
+    }
+
+    [Fact]
+    public void RequiredPermissionAttribute_Should_Implement_IRequiredPermissionMetadata()
+    {
+        var attributeType = typeof(RequiredPermissionAttribute);
+
+        typeof(IRequiredPermissionMetadata).IsAssignableFrom(attributeType).ShouldBeTrue(
+            $"{attributeType.FullName} must implement IRequiredPermissionMetadata. " +
+            "RequiredPermissionAuthorizationHandler discovers endpoint permissions through that " +
+            "interface; without it, every .RequirePermission() gate silently fails open.");
+    }
+
+    /// <summary>
+    /// Loads every FSH.* assembly deployed alongside the tests so the duplicate sweep covers
+    /// BuildingBlocks, all modules (including Contracts), and host assemblies — not just the
+    /// runtime module assemblies that ModuleAssemblyDiscovery returns.
+    /// </summary>
+    private static Assembly[] GetAllFshAssemblies()
+    {
+        string baseDir = AppContext.BaseDirectory;
+
+        var assemblies = new List<Assembly>();
+
+        foreach (var file in Directory.GetFiles(baseDir, "FSH.*.dll"))
+        {
+            try
+            {
+                var assemblyName = AssemblyName.GetAssemblyName(file);
+                assemblies.Add(Assembly.Load(assemblyName));
+            }
+#pragma warning disable CA1031 // Do not catch general exception types
+            catch (Exception)
+            {
+                // Skip if not a valid .NET assembly or other load error
+            }
+#pragma warning restore CA1031
+        }
+
+        assemblies.ShouldNotBeEmpty(
+            "No FSH.* assemblies were found in the test output directory; the duplicate sweep would be a no-op.");
+
+        return [.. assemblies];
+    }
+
+    private static IEnumerable<Type> GetLoadableTypes(Assembly assembly)
+    {
+        try
+        {
+            return assembly.GetTypes();
+        }
+        catch (ReflectionTypeLoadException ex)
+        {
+            return ex.Types.Where(t => t is not null)!;
+        }
+    }
+}
diff --git a/src/Tests/Integration.Tests/Tests/Groups/GroupRolePermissionTests.cs b/src/Tests/Integration.Tests/Tests/Groups/GroupRolePermissionTests.cs
@@ -0,0 +1,175 @@
+using Finbuckle.MultiTenant;
+using Finbuckle.MultiTenant.Abstractions;
+using FSH.Framework.Shared.Multitenancy;
+using FSH.Modules.Identity.Contracts.Authorization;
+using FSH.Modules.Identity.Domain;
+using Integration.Tests.Infrastructure;
+using Integration.Tests.Infrastructure.Extensions;
+using Microsoft.AspNetCore.Identity;
+
+namespace Integration.Tests.Tests.Groups;
+
+/// <summary>
+/// Proves group-derived roles confer PERMISSIONS, not just JWT role claims.
+/// <c>IdentityService.AddRoleClaimsAsync</c> unions direct roles with
+/// <c>IGroupRoleService.GetUserGroupRolesAsync</c> when minting tokens, and every group
+/// mutation handler invalidates the permission cache — so <c>UserPermissionService</c>
+/// must resolve the same union, or a user whose only role comes via a group fails every
+/// <c>.RequirePermission()</c> gate despite their JWT showing the role.
+/// </summary>
+[Collection(FshCollectionDefinition.Name)]
+public sealed class GroupRolePermissionTests
+{
+    private const string ProbePermission = IdentityPermissions.Groups.Create;
+
+    private readonly FshWebApplicationFactory _factory;
+    private readonly AuthHelper _auth;
+
+    public GroupRolePermissionTests(FshWebApplicationFactory factory)
+    {
+        _factory = factory;
+        _auth = new AuthHelper(factory);
+    }
+
+    [Fact]
+    public async Task GetOwnPermissions_Should_IncludeGroupRolePermissions_When_UserHasNoDirectRoles()
+    {
+        // Arrange — custom role with the probe permission, attached to a GROUP (never to the user).
+        using var adminClient = await _auth.CreateRootAdminClientAsync();
+        var uniqueId = Guid.NewGuid().ToString("N")[..8];
+
+        var role = await CreateRoleWithPermissionAsync(adminClient, $"GrpPermRole-{uniqueId}", ProbePermission);
+        var groupId = await CreateGroupWithRoleAsync(adminClient, $"PermGroup-{uniqueId}", role.Id);
+
+        var (email, password, userId) = await CreateActiveUserAsync($"grpperm-{uniqueId}");
+        await AddUserToGroupAsync(adminClient, groupId, userId);
+
+        using var userClient = await _auth.CreateAuthenticatedClientAsync(email, password);
+
+        // Act — read the user's own effective permissions (auth-only endpoint, backed by UserPermissionService).
+        var response = await userClient.GetAsync($"{TestConstants.IdentityBasePath}/permissions");
+        response.StatusCode.ShouldBe(HttpStatusCode.OK,
+            $"Get current-user permissions failed: {await response.Content.ReadAsStringAsync()}");
+        var permissions = await response.DeserializeAsync<List<string>>();
+
+        // Assert — the group-derived role's permission must be in the effective set.
+        permissions.ShouldContain(ProbePermission,
+            "UserPermissionService only resolves DIRECT role assignments: a role granted via group membership " +
+            "confers no permissions, even though the JWT carries the role claim.");
+    }
+
+    [Fact]
+    public async Task PermissionGate_Should_Open_When_UsersOnlyRoleComesViaGroup()
+    {
+        // Arrange — same setup, but probe through a real .RequirePermission() gate
+        // (POST /groups requires Groups.Create), exercising the authorization handler path.
+        using var adminClient = await _auth.CreateRootAdminClientAsync();
+        var uniqueId = Guid.NewGuid().ToString("N")[..8];
+
+        var role = await CreateRoleWithPermissionAsync(adminClient, $"GrpGateRole-{uniqueId}", ProbePermission);
+        var groupId = await CreateGroupWithRoleAsync(adminClient, $"GateGroup-{uniqueId}", role.Id);
+
+        var (email, password, userId) = await CreateActiveUserAsync($"grpgate-{uniqueId}");
+        await AddUserToGroupAsync(adminClient, groupId, userId);
+
+        using var userClient = await _auth.CreateAuthenticatedClientAsync(email, password);
+
+        // Act — hit the gated endpoint with the group-conferred permission.
+        var response = await userClient.PostAsJsonAsync($"{TestConstants.IdentityBasePath}/groups", new
+        {
+            name = $"grp-by-member-{uniqueId}",
+            description = "created via group-derived permission",
+            isDefault = false,
+            roleIds = new List<string>()
+        });
+
+        // Assert — the gate must honor the group-derived role.
+        response.StatusCode.ShouldNotBe(HttpStatusCode.Forbidden,
+            "RequiredPermissionAuthorizationHandler denied a user whose group membership grants a role holding the required permission.");
+        response.StatusCode.ShouldBe(HttpStatusCode.Created);
+    }
+
+    // ─── helpers ─────────────────────────────────────────────────────
+
+    private static async Task<RoleDto> CreateRoleWithPermissionAsync(HttpClient adminClient, string name, string permission)
+    {
+        var createResponse = await adminClient.PostAsJsonAsync($"{TestConstants.IdentityBasePath}/roles", new
+        {
+            id = string.Empty,
+            name,
+            description = "group-role permission test role"
+        });
+        var role = await createResponse.DeserializeAsync<RoleDto>();
+
+        var permResponse = await adminClient.PutAsJsonAsync(
+            $"{TestConstants.IdentityBasePath}/{role.Id}/permissions", new
+            {
+                roleId = role.Id,
+                permissions = new[] { permission }
+            });
+        permResponse.StatusCode.ShouldBe(HttpStatusCode.OK,
+            $"Set role permissions failed: {await permResponse.Content.ReadAsStringAsync()}");
+
+        return role;
+    }
+
+    private static async Task<Guid> CreateGroupWithRoleAsync(HttpClient adminClient, string name, string roleId)
+    {
+        var response = await adminClient.PostAsJsonAsync($"{TestConstants.IdentityBasePath}/groups", new
+        {
+            name,
+            description = "group-role permission test group",
+            isDefault = false,
+            roleIds = new List<string> { roleId }
+        });
+        response.StatusCode.ShouldBe(HttpStatusCode.Created,
+            $"Create group failed: {await response.Content.ReadAsStringAsync()}");
+        var group = await response.DeserializeAsync<GroupDto>();
+        return group.Id;
+    }
+
+    private static async Task AddUserToGroupAsync(HttpClient adminClient, Guid groupId, string userId)
+    {
+        var response = await adminClient.PostAsJsonAsync(
+            $"{TestConstants.IdentityBasePath}/groups/{groupId}/members",
+            new { userIds = new[] { userId } });
+        response.StatusCode.ShouldBe(HttpStatusCode.OK,
+            $"Add user to group failed: {await response.Content.ReadAsStringAsync()}");
+    }
+
+    /// <summary>
+    /// Seeds a confirmed + active user with NO role assignments. The Finbuckle tenant
+    /// context is set INLINE because it is AsyncLocal — setting it in an awaited helper
+    /// would lose it and the tenant query filter would throw.
+    /// </summary>
+    private async Task<(string Email, string Password, string UserId)> CreateActiveUserAsync(string handle)
+    {
+        const string password = TestConstants.DefaultPassword;
+        var email = $"{handle}@example.com";
+
+        using var scope = _factory.Services.CreateScope();
+
+        var tenant = await scope.ServiceProvider
+            .GetRequiredService<IMultiTenantStore<AppTenantInfo>>()
+            .GetAsync(TestConstants.RootTenantId);
+        scope.ServiceProvider.GetRequiredService<IMultiTenantContextSetter>()
+            .MultiTenantContext = new MultiTenantContext<AppTenantInfo>(tenant);
+
+        var userManager = scope.ServiceProvider.GetRequiredService<UserManager<FshUser>>();
+        var user = new FshUser
+        {
+            FirstName = "GroupRole",
+            LastName = "Probe",
+            Email = email,
+            UserName = handle,
+            EmailConfirmed = true,
+            IsActive = true,
+        };
+
+        var result = await userManager.CreateAsync(user, password);
+        result.Succeeded.ShouldBeTrue(
+            $"Seeding active user failed: {string.Join(", ", result.Errors.Select(e => e.Description))}");
+
+        return (email, password, user.Id);
+    }
+}
