From de0d9cc19b7f9eab76b09d1483846dd881f074cb Mon Sep 17 00:00:00 2001 From: "Marcelo M. Maciel" <4993482+marcelo-maciel@users.noreply.github.com> Date: Fri, 2 Oct 2026 04:43:13 -0300 Subject: [PATCH 1/6] fix(identity): make the system-role permission sync remove revoked claims --- .../RolePermissionSyncHostedService.cs | 8 +- .../Authorization/RolePermissionSyncer.cs | 44 ++++++-- .../Catalog/RolePermissionSyncerTests.cs | 105 ++++++++++++++++-- .../Roles/PermissionCacheInvalidationTests.cs | 66 +++++++++++ 4 files changed, 197 insertions(+), 26 deletions(-) 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,65 @@ 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(); + + 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( + $"Syncer stripped {missing.Count} root permission(s) from the root tenant's Admin: " + + $"[{string.Join(", ", missing)}]"); + } + // ─── helpers ───────────────────────────────────────────────────── private async Task GetRootTenantAsync() @@ -95,7 +156,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 +195,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 +213,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); + } } From cf333c0fb8fdb263eb8196da39f823864bf2c376 Mon Sep 17 00:00:00 2001 From: "Marcelo M. Maciel" <4993482+marcelo-maciel@users.noreply.github.com> Date: Fri, 2 Oct 2026 05:24:54 -0300 Subject: [PATCH 2/6] test(identity): check root permissions after each of two sync passes --- .../Tests/Catalog/RolePermissionSyncerTests.cs | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/src/Tests/Integration.Tests/Tests/Catalog/RolePermissionSyncerTests.cs b/src/Tests/Integration.Tests/Tests/Catalog/RolePermissionSyncerTests.cs index 85b6a940b9..d858e75660 100644 --- a/src/Tests/Integration.Tests/Tests/Catalog/RolePermissionSyncerTests.cs +++ b/src/Tests/Integration.Tests/Tests/Catalog/RolePermissionSyncerTests.cs @@ -136,13 +136,17 @@ public async Task SyncAsync_Should_Keep_Root_Permissions_On_Root_Tenant_Admin() var rootTenant = await GetRootTenantAsync(); PermissionConstants.Root.ShouldNotBeEmpty(); - await RunSyncerAsync(rootTenant); + // 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( - $"Syncer stripped {missing.Count} root permission(s) from the root tenant's Admin: " + - $"[{string.Join(", ", missing)}]"); + 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 ───────────────────────────────────────────────────── From fd6e9b0b2d835d11f69655be12bed709373c4344 Mon Sep 17 00:00:00 2001 From: "Marcelo M. Maciel" <4993482+marcelo-maciel@users.noreply.github.com> Date: Wed, 14 Oct 2026 12:30:40 -0300 Subject: [PATCH 3/6] build(deps): move the .NET 10 packages to 10.0.13 for the October advisories Advisories published on 2026-10-13 cover every 10.0.x up to 10.0.12 of System.Security.Cryptography.Xml, System.Security.Cryptography.Pkcs and Microsoft.AspNetCore.SignalR.Common, and NuGetAudit under TreatWarningsAsErrors fails restore for every project, main included. SignalR.Common 10.0.13 requires Microsoft.Extensions.* 10.0.13, so a transitive pin alone does not resolve; the whole 10.0.12 line moves to 10.0.13 and Xml/Pkcs stay pinned. --- src/Directory.Packages.props | 68 +++++++++++++++++++----------------- 1 file changed, 35 insertions(+), 33 deletions(-) diff --git a/src/Directory.Packages.props b/src/Directory.Packages.props index 39ac673040..0c7bae922c 100644 --- a/src/Directory.Packages.props +++ b/src/Directory.Packages.props @@ -57,40 +57,40 @@ - - - + + + - - - - - - + + + + + + - - - + + + - - - - - - - - - - - - - - - + + + + + + + + + + + + + + + @@ -104,24 +104,26 @@ - + - - + + - + From 41ad8813294ded77c7cf12a52347aa8cc1818035 Mon Sep 17 00:00:00 2001 From: "Marcelo M. Maciel" <4993482+marcelo-maciel@users.noreply.github.com> Date: Wed, 14 Oct 2026 12:48:34 -0300 Subject: [PATCH 4/6] fix(identity): guard the authoritative permission sync against partial catalogs and races - PermissionSyncOptions:RemoveRevoked (default true) turns removal off for a host that registers IdentityModule without every module, whose partial catalog would otherwise revoke the missing modules' permissions from the built-in roles in every tenant. Missing permissions are still added. - An empty target set never removes anything: it means a missing catalog. - A save that loses a delete race with another instance starting at the same time (DbUpdateConcurrencyException) is logged at Information and its tracked entries are cleared, so the next role in the same scope does not retry them and no false Error reaches the per-tenant catch. --- .../Authorization/PermissionSyncOptions.cs | 18 ++ .../RolePermissionSyncHostedService.cs | 13 +- .../Authorization/RolePermissionSyncer.cs | 72 +++++++- .../Modules.Identity/IdentityModule.cs | 1 + .../Catalog/RolePermissionSyncerTests.cs | 173 ++++++++++++++++++ 5 files changed, 269 insertions(+), 8 deletions(-) create mode 100644 src/Modules/Identity/Modules.Identity/Authorization/PermissionSyncOptions.cs diff --git a/src/Modules/Identity/Modules.Identity/Authorization/PermissionSyncOptions.cs b/src/Modules/Identity/Modules.Identity/Authorization/PermissionSyncOptions.cs new file mode 100644 index 0000000000..602809262f --- /dev/null +++ b/src/Modules/Identity/Modules.Identity/Authorization/PermissionSyncOptions.cs @@ -0,0 +1,18 @@ +namespace FSH.Modules.Identity.Authorization; + +/// +/// Startup reconciliation of the built-in roles' permission claims (config section +/// "PermissionSyncOptions"), run by . +/// +public sealed class PermissionSyncOptions +{ + public const string SectionName = "PermissionSyncOptions"; + + /// + /// When true (the default), the sync also removes permission claims the catalog no longer + /// grants. Set it to false on any host that registers IdentityModule without every + /// module the API loads: its catalog is partial, so removal would revoke the missing modules' + /// permissions from the built-in roles in every tenant. Missing permissions are still added. + /// + public bool RemoveRevoked { get; set; } = true; +} diff --git a/src/Modules/Identity/Modules.Identity/Authorization/RolePermissionSyncHostedService.cs b/src/Modules/Identity/Modules.Identity/Authorization/RolePermissionSyncHostedService.cs index 5fd8cd9390..d4b92c1edc 100644 --- a/src/Modules/Identity/Modules.Identity/Authorization/RolePermissionSyncHostedService.cs +++ b/src/Modules/Identity/Modules.Identity/Authorization/RolePermissionSyncHostedService.cs @@ -4,13 +4,15 @@ using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Hosting; using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Options; namespace FSH.Modules.Identity.Authorization; /// /// 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 — +/// adding the missing ones and removing the ones no longer granted (unless +/// is off). Idempotent and lightweight — /// only writes when something changed, so it's safe to run unconditionally. /// /// @@ -23,6 +25,7 @@ namespace FSH.Modules.Identity.Authorization; /// internal sealed class RolePermissionSyncHostedService( IServiceProvider serviceProvider, + IOptions options, ILogger logger) : BackgroundService { private static readonly TimeSpan PollInterval = TimeSpan.FromSeconds(2); @@ -41,6 +44,14 @@ protected override async Task ExecuteAsync(CancellationToken stoppingToken) return; } + // Once per process, not per tenant and role: a worker host runs with this off on every start. + if (!options.Value.RemoveRevoked && logger.IsEnabled(LogLevel.Information)) + { + logger.LogInformation( + "Role permission sync runs add-only: {Setting} is false, so revoked permissions are not removed", + $"{PermissionSyncOptions.SectionName}:{nameof(PermissionSyncOptions.RemoveRevoked)}"); + } + foreach (var tenant in tenants) { if (stoppingToken.IsCancellationRequested) break; diff --git a/src/Modules/Identity/Modules.Identity/Authorization/RolePermissionSyncer.cs b/src/Modules/Identity/Modules.Identity/Authorization/RolePermissionSyncer.cs index 11bdf319df..4db8947b1e 100644 --- a/src/Modules/Identity/Modules.Identity/Authorization/RolePermissionSyncer.cs +++ b/src/Modules/Identity/Modules.Identity/Authorization/RolePermissionSyncer.cs @@ -9,6 +9,7 @@ using Microsoft.EntityFrameworkCore; using Microsoft.Extensions.Caching.Hybrid; using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Options; namespace FSH.Modules.Identity.Authorization; @@ -17,7 +18,8 @@ namespace FSH.Modules.Identity.Authorization; /// ) 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. +/// Idempotent, so it can run on every startup safely. Removal can be turned off with +/// . /// public sealed class RolePermissionSyncer( IdentityDbContext context, @@ -25,6 +27,7 @@ public sealed class RolePermissionSyncer( IMultiTenantContextAccessor tenantAccessor, HybridCache cache, TimeProvider timeProvider, + IOptions options, ILogger logger) { public async Task SyncAsync(CancellationToken cancellationToken) @@ -48,7 +51,7 @@ public async Task SyncAsync(CancellationToken cancellationToken) } } - private async Task SyncRoleAsync(string roleName, IReadOnlyList targetPermissions, CancellationToken cancellationToken) + internal async Task SyncRoleAsync(string roleName, IReadOnlyList targetPermissions, CancellationToken cancellationToken) { var role = await roleManager.Roles .SingleOrDefaultAsync(r => r.Name == roleName, cancellationToken) @@ -78,10 +81,7 @@ private async Task SyncRoleAsync(string roleName, IReadOnlyList rc.ClaimValue is null || !targetSet.Contains(rc.ClaimValue)) - .ToList(); + var toRemove = SelectRevokedClaims(roleName, existing, targetSet); if (toAdd.Count == 0 && toRemove.Count == 0) { @@ -90,7 +90,18 @@ private async Task SyncRoleAsync(string roleName, IReadOnlyList 0 && logger.IsEnabled(LogLevel.Warning)) { @@ -115,4 +126,51 @@ private async Task SyncRoleAsync(string roleName, IReadOnlyList + /// Saves the pending claim changes. Returns false when another instance starting at the same + /// time deleted the same rows first (a delete matched 0 rows): its write already reached the same + /// end state, so this is not an error. + /// + private async Task TrySaveAsync(CancellationToken cancellationToken) + { + try + { + await context.SaveChangesAsync(cancellationToken).ConfigureAwait(false); + return true; + } + catch (DbUpdateConcurrencyException) + { + // Drop the failed entries, or the next role's save in this scope would retry them. + context.ChangeTracker.Clear(); + return false; + } + } + + private List SelectRevokedClaims(string roleName, List existing, HashSet targetSet) + { + if (!options.Value.RemoveRevoked) + { + return []; + } + + // An empty target means the catalog is missing, not that the role should hold nothing. + if (targetSet.Count == 0) + { + if (logger.IsEnabled(LogLevel.Warning)) + { + logger.LogWarning( + "Skipped permission removal for '{Role}' in tenant '{Tenant}': the permission catalog grants it nothing", + roleName, + tenantAccessor.MultiTenantContext.TenantInfo?.Id); + } + + return []; + } + + // A null value grants nothing, so it is as stale as a retired permission. + return existing + .Where(rc => rc.ClaimValue is null || !targetSet.Contains(rc.ClaimValue)) + .ToList(); + } } diff --git a/src/Modules/Identity/Modules.Identity/IdentityModule.cs b/src/Modules/Identity/Modules.Identity/IdentityModule.cs index 3c64f20eaf..a2405039db 100644 --- a/src/Modules/Identity/Modules.Identity/IdentityModule.cs +++ b/src/Modules/Identity/Modules.Identity/IdentityModule.cs @@ -87,6 +87,7 @@ public void ConfigureServices(IHostApplicationBuilder builder) FSH.Modules.Identity.Contracts.Authorization.IdentityPermissions.All); var services = builder.Services; + services.Configure(builder.Configuration.GetSection(PermissionSyncOptions.SectionName)); services.AddScoped(); services.AddHostedService(); services.AddSingleton(); diff --git a/src/Tests/Integration.Tests/Tests/Catalog/RolePermissionSyncerTests.cs b/src/Tests/Integration.Tests/Tests/Catalog/RolePermissionSyncerTests.cs index d858e75660..cc5e84565c 100644 --- a/src/Tests/Integration.Tests/Tests/Catalog/RolePermissionSyncerTests.cs +++ b/src/Tests/Integration.Tests/Tests/Catalog/RolePermissionSyncerTests.cs @@ -2,6 +2,7 @@ using Finbuckle.MultiTenant.Abstractions; using FSH.Framework.Shared.Constants; using FSH.Framework.Shared.Multitenancy; +using FSH.Framework.Shared.Persistence; using FSH.Modules.Catalog.Contracts.Authorization; using FSH.Modules.Identity.Authorization; using FSH.Modules.Identity.Contracts.Authorization; @@ -10,7 +11,10 @@ using Integration.Tests.Infrastructure; using Microsoft.AspNetCore.Identity; using Microsoft.EntityFrameworkCore; +using Microsoft.EntityFrameworkCore.Diagnostics; using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Hosting; +using Microsoft.Extensions.Options; namespace Integration.Tests.Tests.Catalog; @@ -29,6 +33,9 @@ public sealed class RolePermissionSyncerTests private const string RetiredPermission = "Permissions.Retired.View"; private const string NonPermissionClaimType = "fsh.test.marker"; + // Target for the throwaway-role tests; never registered, so it cannot collide with a real grant. + private static readonly FshPermission ProbePermission = new("Sync probe", "View", "SyncProbe"); + private readonly FshWebApplicationFactory _factory; public RolePermissionSyncerTests(FshWebApplicationFactory factory) @@ -149,8 +156,174 @@ public async Task SyncAsync_Should_Keep_Root_Permissions_On_Root_Tenant_Admin() } } + // The guards below run SyncRoleAsync against a throwaway role with an explicit target, so they + // never touch Basic/Admin, which other tests and the startup sync share. + + [Fact] + public async Task SyncRoleAsync_Should_Add_But_Not_Remove_When_RemoveRevoked_Is_Off() + { + var rootTenant = await GetRootTenantAsync(); + string roleName = await CreateProbeRoleAsync(rootTenant, RetiredPermission); + + try + { + int changed = await RunSyncRoleAsync(rootTenant, roleName, [ProbePermission], removeRevoked: false); + + changed.ShouldBe(1, "Only the missing permission should be written."); + var claims = await GetClaimsAsync(rootTenant, roleName, ClaimConstants.Permission); + claims.ShouldBe([RetiredPermission, ProbePermission.Name], ignoreOrder: true, + "With RemoveRevoked off the sync must add the missing permission and keep the revoked one."); + } + finally + { + await DeleteRoleAsync(rootTenant, roleName); + } + } + + [Fact] + public async Task SyncRoleAsync_Should_Not_Remove_Anything_When_The_Target_Is_Empty() + { + var rootTenant = await GetRootTenantAsync(); + string roleName = await CreateProbeRoleAsync(rootTenant, RetiredPermission); + + try + { + int changed = await RunSyncRoleAsync(rootTenant, roleName, [], removeRevoked: true); + + changed.ShouldBe(0); + var claims = await GetClaimsAsync(rootTenant, roleName, ClaimConstants.Permission); + claims.ShouldContain(RetiredPermission, + "An empty target means a missing catalog; the sync must not strip the role."); + } + finally + { + await DeleteRoleAsync(rootTenant, roleName); + } + } + + [Fact] + public async Task SyncRoleAsync_Should_Treat_A_Lost_Delete_Race_As_Done_And_Keep_Syncing() + { + var rootTenant = await GetRootTenantAsync(); + string racedRole = await CreateProbeRoleAsync(rootTenant, RetiredPermission); + string nextRole = await CreateProbeRoleAsync(rootTenant, RetiredPermission); + + try + { + using var scope = _factory.Services.CreateScope(); + var sp = scope.ServiceProvider; + sp.GetRequiredService() + .MultiTenantContext = new MultiTenantContext(rootTenant); + + // Stands in for a second instance starting at the same time: just before this instance's + // first save, it deletes the same row from its own connection. + var otherInstance = new DeleteBeforeFirstSaveInterceptor( + () => DeleteClaimAsync(rootTenant, racedRole, RetiredPermission)); + var options = new DbContextOptionsBuilder( + sp.GetRequiredService>()) + .AddInterceptors(otherInstance) + .Options; + await using var db = new IdentityDbContext( + sp.GetRequiredService>(), + options, + sp.GetRequiredService>(), + sp.GetRequiredService()); + var syncer = ActivatorUtilities.CreateInstance( + sp, db, Options.Create(new PermissionSyncOptions { RemoveRevoked = true })); + + int racedChanged = await syncer.SyncRoleAsync(racedRole, [ProbePermission], CancellationToken.None); + int nextChanged = await syncer.SyncRoleAsync(nextRole, [ProbePermission], CancellationToken.None); + + otherInstance.Fired.ShouldBeTrue("Pre-condition: the competing delete must have run."); + racedChanged.ShouldBe(0, "A lost race is reported as no change from this instance."); + nextChanged.ShouldBe(2, "The next role in the same scope must still sync (one removal, one addition)."); + var next = await GetClaimsAsync(rootTenant, nextRole, ClaimConstants.Permission); + next.ShouldBe([ProbePermission.Name], ignoreOrder: true); + } + finally + { + await DeleteRoleAsync(rootTenant, racedRole); + await DeleteRoleAsync(rootTenant, nextRole); + } + } + // ─── helpers ───────────────────────────────────────────────────── + private async Task RunSyncRoleAsync(AppTenantInfo tenant, string roleName, IReadOnlyList target, bool removeRevoked) + { + using var scope = _factory.Services.CreateScope(); + scope.ServiceProvider.GetRequiredService() + .MultiTenantContext = new MultiTenantContext(tenant); + + var syncer = ActivatorUtilities.CreateInstance( + scope.ServiceProvider, Options.Create(new PermissionSyncOptions { RemoveRevoked = removeRevoked })); + return await syncer.SyncRoleAsync(roleName, target, CancellationToken.None); + } + + private async Task CreateProbeRoleAsync(AppTenantInfo tenant, string permission) + { + using var scope = _factory.Services.CreateScope(); + scope.ServiceProvider.GetRequiredService() + .MultiTenantContext = new MultiTenantContext(tenant); + + string roleName = $"sync-probe-{Guid.NewGuid():N}"; + var roleManager = scope.ServiceProvider.GetRequiredService>(); + (await roleManager.CreateAsync(new FshRole(roleName))).Succeeded.ShouldBeTrue(); + + await SeedClaimAsync(tenant, roleName, ClaimConstants.Permission, permission); + return roleName; + } + + private async Task DeleteRoleAsync(AppTenantInfo tenant, string roleName) + { + using var scope = _factory.Services.CreateScope(); + scope.ServiceProvider.GetRequiredService() + .MultiTenantContext = new MultiTenantContext(tenant); + + // Role claims cascade with the role. + var roleManager = scope.ServiceProvider.GetRequiredService>(); + var role = await roleManager.FindByNameAsync(roleName); + if (role is not null) + { + (await roleManager.DeleteAsync(role)).Succeeded.ShouldBeTrue(); + } + } + + private async Task DeleteClaimAsync(AppTenantInfo tenant, string roleName, 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(); + int deleted = await db.RoleClaims + .Where(rc => rc.RoleId == role.Id && rc.ClaimValue == claimValue) + .ExecuteDeleteAsync(); + deleted.ShouldBe(1); + } + + private sealed class DeleteBeforeFirstSaveInterceptor(Func competingDelete) : SaveChangesInterceptor + { + public bool Fired { get; private set; } + + public override async ValueTask> SavingChangesAsync( + DbContextEventData eventData, + InterceptionResult result, + CancellationToken cancellationToken = default) + { + if (!Fired) + { + Fired = true; + await competingDelete(); + } + + return result; + } + } + private async Task GetRootTenantAsync() { using var scope = _factory.Services.CreateScope(); From 9a875c748bfb02adc6dd3c7ebd78cdd275b846c0 Mon Sep 17 00:00:00 2001 From: "Marcelo M. Maciel" <4993482+marcelo-maciel@users.noreply.github.com> Date: Wed, 14 Oct 2026 12:48:36 -0300 Subject: [PATCH 5/6] test(identity): remove the seeded cache-probe claim from Basic even when the test fails --- .../Roles/PermissionCacheInvalidationTests.cs | 48 +++++++++++++++---- 1 file changed, 38 insertions(+), 10 deletions(-) diff --git a/src/Tests/Integration.Tests/Tests/Roles/PermissionCacheInvalidationTests.cs b/src/Tests/Integration.Tests/Tests/Roles/PermissionCacheInvalidationTests.cs index 392c37af1b..3f2892aeea 100644 --- a/src/Tests/Integration.Tests/Tests/Roles/PermissionCacheInvalidationTests.cs +++ b/src/Tests/Integration.Tests/Tests/Roles/PermissionCacheInvalidationTests.cs @@ -187,19 +187,27 @@ public async Task RolePermissionSync_Should_EvictCachedPermissions_When_It_Revok await AssignRoleAsync(adminClient, userId, RoleConstants.Basic); await SeedRootRoleClaimAsync(RoleConstants.Basic, retiredPermission); - using var userClient = await _auth.CreateAuthenticatedClientAsync(email, password); + // Basic is shared by every test: if anything below fails before the sync revokes the claim, it must not leak. + try + { + 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."); + 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(); + // 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."); + // 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."); + } + finally + { + await RemoveRootRoleClaimAsync(RoleConstants.Basic, retiredPermission); + } } #endregion @@ -311,6 +319,26 @@ private async Task SeedRootRoleClaimAsync(string roleName, string permission) await db.SaveChangesAsync(); } + // No-op when the sync already revoked it. + private async Task RemoveRootRoleClaimAsync(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(); + await db.RoleClaims + .Where(rc => rc.RoleId == role.Id && rc.ClaimType == ClaimConstants.Permission && rc.ClaimValue == permission) + .ExecuteDeleteAsync(); + } + private async Task RunRootTenantSyncAsync() { using var scope = _factory.Services.CreateScope(); From 1464dacfd11605c0185b01cd22d547a3ac677787 Mon Sep 17 00:00:00 2001 From: "Marcelo M. Maciel" <4993482+marcelo-maciel@users.noreply.github.com> Date: Wed, 14 Oct 2026 13:24:51 -0300 Subject: [PATCH 6/6] ci: rerun after a timing flake in TypingIndicatorTests (passes 6/6 locally)