fix(identity): make the system-role permission sync remove revoked claims

.agents/rules/modules/identity.md

@@ -24,6 +24,7 @@ Auth (JWT + ASP.NET Identity), users, roles, permissions, sessions, impersonatio
 ## Hosted services (background)
 
 - `RolePermissionSyncHostedService` — best-effort sync of the permission catalog; loops, catches `Exception` *with* an `OperationCanceledException` filter, logs and continues.
+  The sync is authoritative for the built-in `Basic` and `Admin` roles: it adds missing permissions **and removes revoked ones** (root `Admin` keeps `Root` permissions), logging each removal. `PermissionSyncOptions:RemoveRevoked=false` makes it add-only — use it for hosts that load only some modules. Custom roles are never touched.
 - `SessionCleanupHostedService` — hourly expired-session purge; OCE handled by a preceding catch.
 
 These are the model for background loops: stay alive, log with context, never swallow cancellation. See `api-conventions.md`.

src/Host/FSH.Starter.Api/appsettings.json

@@ -78,6 +78,9 @@
     "PasswordExpiryWarningDays": 14,
     "EnforcePasswordExpiry": true
   },
+  "PermissionSyncOptions": {
+    "RemoveRevoked": true
+  },
   "TwoFactorOptions": {
     "Issuer": "FullStackHero"
   },

src/Modules/Identity/Modules.Identity/Authorization/PermissionSyncOptions.cs

@@ -0,0 +1,18 @@
+namespace FSH.Modules.Identity.Authorization;
+
+/// <summary>
+/// Startup reconciliation of the built-in roles' permission claims (config section
+/// <c>"PermissionSyncOptions"</c>), run by <see cref="RolePermissionSyncer"/>.
+/// </summary>
+public sealed class PermissionSyncOptions
+{
+    public const string SectionName = "PermissionSyncOptions";
+
+    /// <summary>
+    /// When <c>true</c> (the default), the sync also removes permission claims the catalog no longer
+    /// grants. Set it to <c>false</c> on any host that registers <c>IdentityModule</c> 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.
+    /// </summary>
+    public bool RemoveRevoked { get; set; } = true;
+}

src/Modules/Identity/Modules.Identity/Authorization/RolePermissionSyncHostedService.cs

@@ -4,14 +4,16 @@
 using Microsoft.Extensions.DependencyInjection;
 using Microsoft.Extensions.Hosting;
 using Microsoft.Extensions.Logging;
+using Microsoft.Extensions.Options;
 
 namespace FSH.Modules.Identity.Authorization;
 
 /// <summary>
-/// Runs once on host startup: iterates every tenant and adds any permission claims that
-/// have been registered via <see cref="FSH.Framework.Shared.Constants.PermissionConstants"/>
-/// 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 <see cref="FSH.Framework.Shared.Constants.PermissionConstants"/>,
+/// adding the missing ones and removing the ones no longer granted (unless
+/// <see cref="PermissionSyncOptions.RemoveRevoked"/> is off). Idempotent and lightweight —
+/// only writes when something changed, so it's safe to run unconditionally.
 /// </summary>
 /// <remarks>
 /// Implemented as a <see cref="BackgroundService"/> so it does not block host startup.
@@ -23,6 +25,7 @@ namespace FSH.Modules.Identity.Authorization;
 /// </remarks>
 internal sealed class RolePermissionSyncHostedService(
     IServiceProvider serviceProvider,
+    IOptions<PermissionSyncOptions> options,
     ILogger<RolePermissionSyncHostedService> 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;

src/Modules/Identity/Modules.Identity/Authorization/RolePermissionSyncer.cs

@@ -9,44 +9,49 @@
 using Microsoft.EntityFrameworkCore;
 using Microsoft.Extensions.Caching.Hybrid;
 using Microsoft.Extensions.Logging;
+using Microsoft.Extensions.Options;
 
 namespace FSH.Modules.Identity.Authorization;
 
 /// <summary>
-/// Adds missing permission claims to the built-in roles (<see cref="RoleConstants.Admin"/>,
-/// <see cref="RoleConstants.Basic"/>) 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 (<see cref="RoleConstants.Admin"/>,
+/// <see cref="RoleConstants.Basic"/>) 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. Removal can be turned off with
+/// <see cref="PermissionSyncOptions.RemoveRevoked"/>.
 /// </summary>
 public sealed class RolePermissionSyncer(
     IdentityDbContext context,
     RoleManager<FshRole> roleManager,
     IMultiTenantContextAccessor<AppTenantInfo> tenantAccessor,
     HybridCache cache,
     TimeProvider timeProvider,
+    IOptions<PermissionSyncOptions> options,
     ILogger<RolePermissionSyncer> logger)
 {
     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);
         }
     }
 
-    private async Task<int> SyncRoleAsync(string roleName, IReadOnlyList<FshPermission> targetPermissions, CancellationToken cancellationToken)
+    internal async Task<int> SyncRoleAsync(string roleName, IReadOnlyList<FshPermission> targetPermissions, CancellationToken cancellationToken)
     {
         var role = await roleManager.Roles
             .SingleOrDefaultAsync(r => r.Name == roleName, cancellationToken)
@@ -59,10 +64,10 @@ private async Task<int> SyncRoleAsync(string roleName, IReadOnlyList<FshPermissi
 
         var existing = await context.RoleClaims
             .Where(rc => 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 +81,41 @@ private async Task<int> SyncRoleAsync(string roleName, IReadOnlyList<FshPermissi
             })
             .ToList();
 
-        if (toAdd.Count == 0)
+        var toRemove = SelectRevokedClaims(roleName, existing, targetSet);
+
+        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 (!await TrySaveAsync(cancellationToken).ConfigureAwait(false))
+        {
+            if (logger.IsEnabled(LogLevel.Information))
+            {
+                logger.LogInformation(
+                    "Permission sync for '{Role}' in tenant '{Tenant}' lost a race with another instance; its changes are already applied",
+                    roleName,
+                    tenantAccessor.MultiTenantContext.TenantInfo?.Id);
+            }
+
+            return 0;
+        }
 
-        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 +124,53 @@ private async Task<int> SyncRoleAsync(string roleName, IReadOnlyList<FshPermissi
                 tenantAccessor.MultiTenantContext.TenantInfo?.Id);
         }
 
-        return toAdd.Count;
+        return toAdd.Count + toRemove.Count;
+    }
+
+    /// <summary>
+    /// Saves the pending claim changes. Returns <c>false</c> 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.
+    /// </summary>
+    private async Task<bool> 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<FshRoleClaim> SelectRevokedClaims(string roleName, List<FshRoleClaim> existing, HashSet<string> 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();
     }
 }

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<PermissionSyncOptions>(builder.Configuration.GetSection(PermissionSyncOptions.SectionName));
         services.AddScoped<RolePermissionSyncer>();
         services.AddHostedService<RolePermissionSyncHostedService>();
         services.AddSingleton<IAuthorizationMiddlewareResultHandler, PathAwareAuthorizationHandler>();