diff --git a/src/Modules/Identity/Modules.Identity/Authorization/RolePermissionSyncHostedService.cs b/src/Modules/Identity/Modules.Identity/Authorization/RolePermissionSyncHostedService.cs index 817255a716..5fd8cd9390 100644 --- a/src/Modules/Identity/Modules.Identity/Authorization/RolePermissionSyncHostedService.cs +++ b/src/Modules/Identity/Modules.Identity/Authorization/RolePermissionSyncHostedService.cs @@ -8,10 +8,10 @@ namespace FSH.Modules.Identity.Authorization; /// -/// Runs once on host startup: iterates every tenant and adds any permission claims that -/// have been registered via -/// but are missing from the role claims table for that tenant. Idempotent and lightweight — -/// only writes when there's something new, so it's safe to run unconditionally. +/// Runs once on host startup: iterates every tenant and reconciles the built-in roles' permission +/// claims with the ones registered via , +/// adding the missing ones and removing the ones no longer granted. Idempotent and lightweight — +/// only writes when something changed, so it's safe to run unconditionally. /// /// /// Implemented as a so it does not block host startup. diff --git a/src/Modules/Identity/Modules.Identity/Authorization/RolePermissionSyncer.cs b/src/Modules/Identity/Modules.Identity/Authorization/RolePermissionSyncer.cs index d1148c9045..11bdf319df 100644 --- a/src/Modules/Identity/Modules.Identity/Authorization/RolePermissionSyncer.cs +++ b/src/Modules/Identity/Modules.Identity/Authorization/RolePermissionSyncer.cs @@ -13,9 +13,11 @@ namespace FSH.Modules.Identity.Authorization; /// -/// Adds missing permission claims to the built-in roles (, -/// ) for the current Finbuckle tenant context. Idempotent — -/// only inserts claims that don't already exist, so it can run on every startup safely. +/// Reconciles the permission claims of the built-in roles (, +/// ) with the permission catalog for the current Finbuckle tenant +/// context: adds the missing ones and removes the ones the catalog no longer grants. Authoritative +/// because those roles are locked against manual edits, so no operator change can be lost. +/// Idempotent, so it can run on every startup safely. /// public sealed class RolePermissionSyncer( IdentityDbContext context, @@ -30,17 +32,17 @@ public async Task SyncAsync(CancellationToken cancellationToken) var tenantId = tenantAccessor.MultiTenantContext.TenantInfo?.Id; bool isRoot = tenantId == MultitenancyConstants.Root.Id; - int basicAdded = await SyncRoleAsync(RoleConstants.Basic, PermissionConstants.Basic, cancellationToken).ConfigureAwait(false); + int basicChanged = await SyncRoleAsync(RoleConstants.Basic, PermissionConstants.Basic, cancellationToken).ConfigureAwait(false); // Admin gets all non-root permissions; the root tenant's Admin additionally gets Root permissions. var adminPermissions = isRoot ? PermissionConstants.Admin.Concat(PermissionConstants.Root).Distinct().ToList() : PermissionConstants.Admin.ToList(); - int adminAdded = await SyncRoleAsync(RoleConstants.Admin, adminPermissions, cancellationToken).ConfigureAwait(false); + int adminChanged = await SyncRoleAsync(RoleConstants.Admin, adminPermissions, cancellationToken).ConfigureAwait(false); // If we wrote anything, drop the per-user permission cache so already-logged-in - // sessions see the new perms on their next request rather than waiting for TTL. - if (basicAdded + adminAdded > 0) + // sessions see the change on their next request rather than waiting for TTL. + if (basicChanged + adminChanged > 0) { await cache.RemoveByTagAsync(CacheKeys.Tags.Permissions, cancellationToken).ConfigureAwait(false); } @@ -59,10 +61,10 @@ private async Task SyncRoleAsync(string roleName, IReadOnlyList rc.RoleId == role.Id && rc.ClaimType == ClaimConstants.Permission) - .Select(rc => rc.ClaimValue!) .ToListAsync(cancellationToken) .ConfigureAwait(false); - var existingSet = existing.ToHashSet(StringComparer.Ordinal); + var existingSet = existing.Select(rc => rc.ClaimValue).ToHashSet(StringComparer.Ordinal); + var targetSet = targetPermissions.Select(p => p.Name).ToHashSet(StringComparer.Ordinal); var toAdd = targetPermissions .Where(p => !existingSet.Contains(p.Name)) @@ -76,15 +78,33 @@ private async Task SyncRoleAsync(string roleName, IReadOnlyList rc.ClaimValue is null || !targetSet.Contains(rc.ClaimValue)) + .ToList(); + + if (toAdd.Count == 0 && toRemove.Count == 0) { return 0; } + context.RoleClaims.RemoveRange(toRemove); await context.RoleClaims.AddRangeAsync(toAdd, cancellationToken).ConfigureAwait(false); await context.SaveChangesAsync(cancellationToken).ConfigureAwait(false); - if (logger.IsEnabled(LogLevel.Information)) + if (toRemove.Count > 0 && logger.IsEnabled(LogLevel.Warning)) + { + foreach (var claim in toRemove) + { + logger.LogWarning( + "Removed permission claim '{Permission}' from '{Role}' for tenant '{Tenant}': the permission catalog no longer grants it", + claim.ClaimValue, + roleName, + tenantAccessor.MultiTenantContext.TenantInfo?.Id); + } + } + + if (toAdd.Count > 0 && logger.IsEnabled(LogLevel.Information)) { logger.LogInformation( "Synced {Count} new permission claim(s) to '{Role}' for tenant '{Tenant}'", @@ -93,6 +113,6 @@ private async Task SyncRoleAsync(string roleName, IReadOnlyListIDbInitializer.SeedAsync flow does not run again /// for already-provisioned tenants in production. This is the path that produced -/// 401s in dev when the Catalog module was added. +/// 401s in dev when the Catalog module was added. Also verifies the reverse path: +/// a permission the catalog stops granting is revoked from the built-in roles. /// [Collection(FshCollectionDefinition.Name)] public sealed class RolePermissionSyncerTests { + // Well-formed but never registered: stands in for a permission a release removed from the catalog. + private const string RetiredPermission = "Permissions.Retired.View"; + private const string NonPermissionClaimType = "fsh.test.marker"; + private readonly FshWebApplicationFactory _factory; public RolePermissionSyncerTests(FshWebApplicationFactory factory) @@ -39,9 +45,9 @@ public async Task SyncAsync_Should_Restore_Missing_Permission_Claims_For_Admin_R // 1. Wipe Admin's Catalog.* claims directly. Simulates "perms registered in a // later release that never made it into this tenant's RoleClaims table." - await WipeClaimsAsync(rootTenant, "Admin", catalogPermissions); + await WipeClaimsAsync(rootTenant, "Admin", ClaimConstants.Permission, catalogPermissions); - var afterWipe = await GetClaimsAsync(rootTenant, "Admin"); + var afterWipe = await GetClaimsAsync(rootTenant, "Admin", ClaimConstants.Permission); afterWipe.Intersect(catalogPermissions).ShouldBeEmpty( "Pre-condition: Admin must not have any Catalog claims after the wipe."); @@ -56,7 +62,7 @@ public async Task SyncAsync_Should_Restore_Missing_Permission_Claims_For_Admin_R } // 3. All Catalog perms should be back on the Admin role. - var afterSync = await GetClaimsAsync(rootTenant, "Admin"); + var afterSync = await GetClaimsAsync(rootTenant, "Admin", ClaimConstants.Permission); var missing = catalogPermissions.Where(p => !afterSync.Contains(p)).ToList(); missing.ShouldBeEmpty( $"Syncer failed to restore {missing.Count} catalog permission(s): " + @@ -67,7 +73,7 @@ public async Task SyncAsync_Should_Restore_Missing_Permission_Claims_For_Admin_R public async Task SyncAsync_Should_Be_Idempotent_When_Claims_Already_Exist() { var rootTenant = await GetRootTenantAsync(); - var before = await GetClaimsAsync(rootTenant, "Admin"); + var before = await GetClaimsAsync(rootTenant, "Admin", ClaimConstants.Permission); // Running the syncer twice in a row must not duplicate claims or throw. using (var scope = _factory.Services.CreateScope()) @@ -80,10 +86,69 @@ public async Task SyncAsync_Should_Be_Idempotent_When_Claims_Already_Exist() await syncer.SyncAsync(CancellationToken.None); } - var after = await GetClaimsAsync(rootTenant, "Admin"); + var after = await GetClaimsAsync(rootTenant, "Admin", ClaimConstants.Permission); after.Count.ShouldBe(before.Count, "Syncer must not duplicate existing permission claims."); } + [Fact] + public async Task SyncAsync_Should_Remove_Permission_Claims_The_Catalog_No_Longer_Grants() + { + var rootTenant = await GetRootTenantAsync(); + + // An Admin-only permission left on Basic is what narrowing IsBasic leaves behind on existing tenants. + const string narrowedPermission = IdentityPermissions.Roles.Delete; + PermissionConstants.Basic.ShouldNotContain(p => p.Name == narrowedPermission); + PermissionConstants.Admin.ShouldContain(p => p.Name == narrowedPermission); + + await SeedClaimAsync(rootTenant, "Basic", ClaimConstants.Permission, RetiredPermission); + await SeedClaimAsync(rootTenant, "Basic", ClaimConstants.Permission, narrowedPermission); + await SeedClaimAsync(rootTenant, "Basic", NonPermissionClaimType, "kept"); + await SeedClaimAsync(rootTenant, "Admin", ClaimConstants.Permission, RetiredPermission); + + try + { + await RunSyncerAsync(rootTenant); + + var basic = await GetClaimsAsync(rootTenant, "Basic", ClaimConstants.Permission); + basic.ShouldBe( + PermissionConstants.Basic.Select(p => p.Name).ToHashSet(StringComparer.Ordinal), + ignoreOrder: true, + "Basic must hold exactly the catalog's IsBasic permissions after the sync."); + + var admin = await GetClaimsAsync(rootTenant, "Admin", ClaimConstants.Permission); + admin.ShouldNotContain(RetiredPermission); + admin.ShouldContain(narrowedPermission); + + var nonPermission = await GetClaimsAsync(rootTenant, "Basic", NonPermissionClaimType); + nonPermission.ShouldContain("kept", "The sync must only reconcile permission claims."); + } + finally + { + await WipeClaimsAsync(rootTenant, "Basic", ClaimConstants.Permission, [RetiredPermission, narrowedPermission]); + await WipeClaimsAsync(rootTenant, "Basic", NonPermissionClaimType, ["kept"]); + await WipeClaimsAsync(rootTenant, "Admin", ClaimConstants.Permission, [RetiredPermission]); + } + } + + [Fact] + public async Task SyncAsync_Should_Keep_Root_Permissions_On_Root_Tenant_Admin() + { + var rootTenant = await GetRootTenantAsync(); + PermissionConstants.Root.ShouldNotBeEmpty(); + + // Checked after two passes: a sync that adds and removes against different targets flips the claims on each run. + for (int pass = 1; pass <= 2; pass++) + { + await RunSyncerAsync(rootTenant); + + var admin = await GetClaimsAsync(rootTenant, "Admin", ClaimConstants.Permission); + var missing = PermissionConstants.Root.Select(p => p.Name).Where(p => !admin.Contains(p)).ToList(); + missing.ShouldBeEmpty( + $"Sync pass {pass} stripped {missing.Count} root permission(s) from the root tenant's Admin: " + + $"[{string.Join(", ", missing)}]"); + } + } + // ─── helpers ───────────────────────────────────────────────────── private async Task GetRootTenantAsync() @@ -95,7 +160,31 @@ private async Task GetRootTenantAsync() return tenant; } - private async Task WipeClaimsAsync(AppTenantInfo tenant, string roleName, IReadOnlyCollection claimValues) + private async Task RunSyncerAsync(AppTenantInfo tenant) + { + using var scope = _factory.Services.CreateScope(); + scope.ServiceProvider.GetRequiredService() + .MultiTenantContext = new MultiTenantContext(tenant); + + var syncer = scope.ServiceProvider.GetRequiredService(); + await syncer.SyncAsync(CancellationToken.None); + } + + private async Task SeedClaimAsync(AppTenantInfo tenant, string roleName, string claimType, string claimValue) + { + using var scope = _factory.Services.CreateScope(); + scope.ServiceProvider.GetRequiredService() + .MultiTenantContext = new MultiTenantContext(tenant); + + var roleManager = scope.ServiceProvider.GetRequiredService>(); + var role = await roleManager.Roles.SingleAsync(r => r.Name == roleName); + + var db = scope.ServiceProvider.GetRequiredService(); + db.RoleClaims.Add(new FshRoleClaim { RoleId = role.Id, ClaimType = claimType, ClaimValue = claimValue }); + await db.SaveChangesAsync(); + } + + private async Task WipeClaimsAsync(AppTenantInfo tenant, string roleName, string claimType, IReadOnlyCollection claimValues) { using var scope = _factory.Services.CreateScope(); scope.ServiceProvider.GetRequiredService() @@ -110,14 +199,14 @@ private async Task WipeClaimsAsync(AppTenantInfo tenant, string roleName, IReadO // ExecuteDeleteAsync would be cleaner but the in-memory enumeration is // negligible for a handful of claims and avoids parameter-list edge cases. var toRemove = await db.RoleClaims - .Where(rc => rc.RoleId == role.Id && rc.ClaimType == ClaimConstants.Permission) + .Where(rc => rc.RoleId == role.Id && rc.ClaimType == claimType) .ToListAsync(); db.RoleClaims.RemoveRange(toRemove.Where(rc => rc.ClaimValue is not null && values.Contains(rc.ClaimValue))); await db.SaveChangesAsync(); } - private async Task> GetClaimsAsync(AppTenantInfo tenant, string roleName) + private async Task> GetClaimsAsync(AppTenantInfo tenant, string roleName, string claimType) { using var scope = _factory.Services.CreateScope(); scope.ServiceProvider.GetRequiredService() @@ -128,7 +217,7 @@ private async Task> GetClaimsAsync(AppTenantInfo tenant, string var db = scope.ServiceProvider.GetRequiredService(); var claims = await db.RoleClaims - .Where(rc => rc.RoleId == role.Id && rc.ClaimType == ClaimConstants.Permission) + .Where(rc => rc.RoleId == role.Id && rc.ClaimType == claimType) .Select(rc => rc.ClaimValue!) .ToListAsync(); diff --git a/src/Tests/Integration.Tests/Tests/Roles/PermissionCacheInvalidationTests.cs b/src/Tests/Integration.Tests/Tests/Roles/PermissionCacheInvalidationTests.cs index 39c1b55b4f..392c37af1b 100644 --- a/src/Tests/Integration.Tests/Tests/Roles/PermissionCacheInvalidationTests.cs +++ b/src/Tests/Integration.Tests/Tests/Roles/PermissionCacheInvalidationTests.cs @@ -1,7 +1,10 @@ using Finbuckle.MultiTenant; using Finbuckle.MultiTenant.Abstractions; +using FSH.Framework.Shared.Constants; using FSH.Framework.Shared.Multitenancy; +using FSH.Modules.Identity.Authorization; using FSH.Modules.Identity.Contracts.Authorization; +using FSH.Modules.Identity.Data; using FSH.Modules.Identity.Domain; using Integration.Tests.Infrastructure; using Integration.Tests.Infrastructure.Extensions; @@ -170,6 +173,37 @@ public async Task AssignUserRoles_Should_EvictCachedPermissions_When_RoleRemoved #endregion + #region Role permission sync + + [Fact] + public async Task RolePermissionSync_Should_EvictCachedPermissions_When_It_RevokesAClaim() + { + // Arrange — a Basic user whose role carries a permission the catalog no longer registers. + const string retiredPermission = "Permissions.RetiredCacheProbe.View"; + using var adminClient = await _auth.CreateRootAdminClientAsync(); + var uniqueId = Guid.NewGuid().ToString("N")[..8]; + + var (email, password, userId) = await CreateActiveUserAsync($"syncuser-{uniqueId}"); + await AssignRoleAsync(adminClient, userId, RoleConstants.Basic); + await SeedRootRoleClaimAsync(RoleConstants.Basic, retiredPermission); + + using var userClient = await _auth.CreateAuthenticatedClientAsync(email, password); + + var warmed = await GetOwnPermissionsAsync(userClient); + warmed.ShouldContain(retiredPermission, + "Pre-condition: the user must hold the retired permission via Basic before the sync."); + + // Act — the startup sync revokes it from Basic and must drop the warmed entry with it. + await RunRootTenantSyncAsync(); + + // Assert + var afterSync = await GetOwnPermissionsAsync(userClient); + afterSync.ShouldNotContain(retiredPermission, + "Cache was NOT invalidated: a permission the sync revoked is still being served from the stale cache entry."); + } + + #endregion + // ─── helpers ───────────────────────────────────────────────────── private static async Task CreateRoleAsync(HttpClient adminClient, string name) @@ -257,4 +291,36 @@ private static async Task> GetOwnPermissionsAsync(HttpClient userCl return (email, password, user.Id); } + + // System roles reject permission edits through the API, so the claim goes in directly. + private async Task SeedRootRoleClaimAsync(string roleName, string permission) + { + using var scope = _factory.Services.CreateScope(); + + var tenant = await scope.ServiceProvider + .GetRequiredService>() + .GetAsync(TestConstants.RootTenantId); + scope.ServiceProvider.GetRequiredService() + .MultiTenantContext = new MultiTenantContext(tenant); + + var roleManager = scope.ServiceProvider.GetRequiredService>(); + var role = await roleManager.Roles.SingleAsync(r => r.Name == roleName); + + var db = scope.ServiceProvider.GetRequiredService(); + db.RoleClaims.Add(new FshRoleClaim { RoleId = role.Id, ClaimType = ClaimConstants.Permission, ClaimValue = permission }); + await db.SaveChangesAsync(); + } + + private async Task RunRootTenantSyncAsync() + { + using var scope = _factory.Services.CreateScope(); + + var tenant = await scope.ServiceProvider + .GetRequiredService>() + .GetAsync(TestConstants.RootTenantId); + scope.ServiceProvider.GetRequiredService() + .MultiTenantContext = new MultiTenantContext(tenant); + + await scope.ServiceProvider.GetRequiredService().SyncAsync(CancellationToken.None); + } }