fix(identity): enforce one row per role claim grant

src/Host/FSH.Starter.Migrations.PostgreSQL/Identity/20261103170127_AddRoleClaimsUniqueIndex.cs

@@ -0,0 +1,54 @@
+using Microsoft.EntityFrameworkCore.Migrations;
+
+#nullable disable
+
+namespace FSH.Starter.Migrations.PostgreSQL.Identity
+{
+    /// <inheritdoc />
+    public partial class AddRoleClaimsUniqueIndex : Migration
+    {
+        /// <inheritdoc />
+        protected override void Up(MigrationBuilder migrationBuilder)
+        {
+            // Remove the duplicate grants concurrent permission writers left behind, keeping the lowest Id
+            // per (RoleId, ClaimType, ClaimValue). Must run before the unique index below is created.
+            // Plain equality matches the index: rows with a NULL ClaimType or ClaimValue never conflict.
+            migrationBuilder.Sql(
+                """
+                DELETE FROM "identity"."RoleClaims" AS dup
+                USING "identity"."RoleClaims" AS kept
+                WHERE dup."RoleId" = kept."RoleId"
+                  AND dup."ClaimType" = kept."ClaimType"
+                  AND dup."ClaimValue" = kept."ClaimValue"
+                  AND dup."Id" > kept."Id";
+                """);
+
+            migrationBuilder.DropIndex(
+                name: "IX_RoleClaims_RoleId",
+                schema: "identity",
+                table: "RoleClaims");
+
+            migrationBuilder.CreateIndex(
+                name: "IX_RoleClaims_RoleId_ClaimType_ClaimValue",
+                schema: "identity",
+                table: "RoleClaims",
+                columns: new[] { "RoleId", "ClaimType", "ClaimValue" },
+                unique: true);
+        }
+
+        /// <inheritdoc />
+        protected override void Down(MigrationBuilder migrationBuilder)
+        {
+            migrationBuilder.DropIndex(
+                name: "IX_RoleClaims_RoleId_ClaimType_ClaimValue",
+                schema: "identity",
+                table: "RoleClaims");
+
+            migrationBuilder.CreateIndex(
+                name: "IX_RoleClaims_RoleId",
+                schema: "identity",
+                table: "RoleClaims",
+                column: "RoleId");
+        }
+    }
+}

src/Host/FSH.Starter.Migrations.PostgreSQL/Identity/IdentityDbContextModelSnapshot.cs

@@ -17,7 +17,7 @@ protected override void BuildModel(ModelBuilder modelBuilder)
         {
 #pragma warning disable 612, 618
             modelBuilder
-                .HasAnnotation("ProductVersion", "10.0.8")
+                .HasAnnotation("ProductVersion", "10.0.13")
                 .HasAnnotation("Relational:MaxIdentifierLength", 63);
 
             NpgsqlModelBuilderExtensions.UseIdentityByDefaultColumns(modelBuilder);
@@ -87,7 +87,9 @@ protected override void BuildModel(ModelBuilder modelBuilder)
 
                     b.HasKey("Id");
 
-                    b.HasIndex("RoleId");
+                    b.HasIndex("RoleId", "ClaimType", "ClaimValue")
+                        .IsUnique()
+                        .HasDatabaseName("IX_RoleClaims_RoleId_ClaimType_ClaimValue");
 
                     b.ToTable("RoleClaims", "identity");
 

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

@@ -90,17 +90,21 @@ internal async Task<int> SyncRoleAsync(string roleName, IReadOnlyList<FshPermiss
 
         context.RoleClaims.RemoveRange(toRemove);
         await context.RoleClaims.AddRangeAsync(toAdd, cancellationToken).ConfigureAwait(false);
-        if (!await TrySaveAsync(cancellationToken).ConfigureAwait(false))
+        switch (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;
+            case SaveOutcome.LostDeleteRace:
+                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;
+            case SaveOutcome.AlreadyInserted:
+                logger.LogRoleClaimsAlreadyInserted(roleName, tenantAccessor.MultiTenantContext.TenantInfo?.Id);
+                return 0;
         }
 
         if (toRemove.Count > 0 && logger.IsEnabled(LogLevel.Warning))
@@ -127,23 +131,35 @@ internal async Task<int> SyncRoleAsync(string roleName, IReadOnlyList<FshPermiss
         return toAdd.Count + toRemove.Count;
     }
 
+    private enum SaveOutcome
+    {
+        Saved,
+        LostDeleteRace,
+        AlreadyInserted,
+    }
+
     /// <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.
+    /// Saves the pending claim changes. Another instance starting at the same time can get there first:
+    /// it deleted the same rows (a delete matched 0 rows) or inserted the same grant (the unique index on
+    /// role claims rejected ours). Its write already reached the same end state, so neither is an error.
     /// </summary>
-    private async Task<bool> TrySaveAsync(CancellationToken cancellationToken)
+    private async Task<SaveOutcome> TrySaveAsync(CancellationToken cancellationToken)
     {
         try
         {
             await context.SaveChangesAsync(cancellationToken).ConfigureAwait(false);
-            return true;
+            return SaveOutcome.Saved;
         }
         catch (DbUpdateConcurrencyException)
         {
             // Drop the failed entries, or the next role's save in this scope would retry them.
             context.ChangeTracker.Clear();
-            return false;
+            return SaveOutcome.LostDeleteRace;
+        }
+        catch (DbUpdateException ex) when (RoleClaimUniqueness.IsDuplicateRoleClaim(ex))
+        {
+            context.ChangeTracker.Clear();
+            return SaveOutcome.AlreadyInserted;
         }
     }
 

src/Modules/Identity/Modules.Identity/Data/IdentityConfigurations.cs

@@ -51,6 +51,13 @@ public void Configure(EntityTypeBuilder<FshRoleClaim> builder)
         builder
             .ToTable("RoleClaims", IdentityModuleConstants.SchemaName)
             .IsMultiTenant();
+
+        // One row per grant. ClaimValue is nullable and PostgreSQL treats NULLs as distinct here,
+        // which is fine: a null-valued claim grants nothing and the permission sync removes it.
+        builder
+            .HasIndex(rc => new { rc.RoleId, rc.ClaimType, rc.ClaimValue })
+            .IsUnique()
+            .HasDatabaseName(RoleClaimUniqueness.IndexName);
     }
 }
 

src/Modules/Identity/Modules.Identity/Data/IdentityDbInitializer.cs

@@ -92,12 +92,24 @@ private async Task AssignPermissionsToRoleAsync(IdentityDbContext dbContext, IRe
             await dbContext.RoleClaims.AddAsync(claim, cancellationToken);
         }
 
-        // Save changes to the database context
-        if (newClaims.Count != 0)
+        if (newClaims.Count == 0)
+        {
+            return;
+        }
+
+        try
         {
             await dbContext.SaveChangesAsync(cancellationToken);
+            return;
+        }
+        catch (DbUpdateException ex) when (RoleClaimUniqueness.IsDuplicateRoleClaim(ex))
+        {
+            // Another writer (the startup permission sync or a second instance) inserted the same grant
+            // first, so the role already holds it. Drop the rejected entries so later saves don't retry them.
+            dbContext.ChangeTracker.Clear();
         }
 
+        logger.LogRoleClaimsAlreadyInserted(role.Name, multiTenantContextAccessor.MultiTenantContext.TenantInfo?.Id);
     }
 
     private async Task SeedSystemGroupsAsync(CancellationToken cancellationToken = default)

src/Modules/Identity/Modules.Identity/Data/RoleClaimUniqueness.cs

@@ -0,0 +1,28 @@
+using Microsoft.EntityFrameworkCore;
+using Microsoft.Extensions.Logging;
+using Npgsql;
+
+namespace FSH.Modules.Identity.Data;
+
+/// <summary>
+/// The unique index on <c>identity.RoleClaims (RoleId, ClaimType, ClaimValue)</c> and how the permission
+/// writers recognise a conflict on it: when two writers (two instances starting at once, or the seeder and
+/// the startup sync) insert the same grant, the second insert is rejected and the row already exists.
+/// </summary>
+internal static partial class RoleClaimUniqueness
+{
+    internal const string IndexName = "IX_RoleClaims_RoleId_ClaimType_ClaimValue";
+
+    /// <summary>True when the save failed only because another writer already inserted the same role claim.</summary>
+    internal static bool IsDuplicateRoleClaim(DbUpdateException exception) =>
+        exception.InnerException is PostgresException
+        {
+            SqlState: PostgresErrorCodes.UniqueViolation,
+            ConstraintName: IndexName,
+        };
+
+    [LoggerMessage(
+        Level = LogLevel.Information,
+        Message = "Permission claims for '{Role}' in tenant '{Tenant}' were already inserted by another writer; skipping")]
+    internal static partial void LogRoleClaimsAlreadyInserted(this ILogger logger, string? role, string? tenant);
+}

src/Modules/Identity/Modules.Identity/Features/v1/Roles/RoleService.cs

@@ -224,6 +224,7 @@ private async Task AddNewPermissionsAsync(FshRole role, IList<System.Security.Cl
     {
         var newPermissions = permissions
             .Where(p => !string.IsNullOrEmpty(p) && !currentClaims.Any(c => c.Value == p))
+            .Distinct(StringComparer.Ordinal)
             .ToList();
 
         foreach (string permission in newPermissions)

src/Tests/Integration.Tests/Infrastructure/Dtos.cs

@@ -39,6 +39,7 @@ public sealed class RoleDto
     public string Id { get; set; } = default!;
     public string Name { get; set; } = default!;
     public string? Description { get; set; }
+    public IReadOnlyCollection<string>? Permissions { get; set; }
 }
 
 public sealed class GroupDto

src/Tests/Integration.Tests/Tests/Catalog/RolePermissionSyncerTests.cs

@@ -15,6 +15,7 @@
 using Microsoft.Extensions.DependencyInjection;
 using Microsoft.Extensions.Hosting;
 using Microsoft.Extensions.Options;
+using Npgsql;
 
 namespace Integration.Tests.Tests.Catalog;
 
@@ -217,17 +218,9 @@ public async Task SyncRoleAsync_Should_Treat_A_Lost_Delete_Race_As_Done_And_Keep
 
             // 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(
+            var otherInstance = new CompetingWriteBeforeFirstSaveInterceptor(
                 () => DeleteClaimAsync(rootTenant, racedRole, RetiredPermission));
-            var options = new DbContextOptionsBuilder<IdentityDbContext>(
-                    sp.GetRequiredService<DbContextOptions<IdentityDbContext>>())
-                .AddInterceptors(otherInstance)
-                .Options;
-            await using var db = new IdentityDbContext(
-                sp.GetRequiredService<IMultiTenantContextAccessor<AppTenantInfo>>(),
-                options,
-                sp.GetRequiredService<IOptions<DatabaseOptions>>(),
-                sp.GetRequiredService<IHostEnvironment>());
+            await using var db = CreateIdentityDbContext(sp, otherInstance);
             var syncer = ActivatorUtilities.CreateInstance<RolePermissionSyncer>(
                 sp, db, Options.Create(new PermissionSyncOptions { RemoveRevoked = true }));
 
@@ -247,8 +240,99 @@ public async Task SyncRoleAsync_Should_Treat_A_Lost_Delete_Race_As_Done_And_Keep
         }
     }
 
+    [Fact]
+    public async Task RoleClaims_Should_Reject_The_Same_Permission_Twice_On_A_Role()
+    {
+        var rootTenant = await GetRootTenantAsync();
+        string roleName = await CreateProbeRoleAsync(rootTenant, RetiredPermission);
+
+        try
+        {
+            var ex = await Should.ThrowAsync<DbUpdateException>(
+                () => SeedClaimAsync(rootTenant, roleName, ClaimConstants.Permission, RetiredPermission));
+
+            var pg = ex.InnerException.ShouldBeOfType<PostgresException>();
+            pg.SqlState.ShouldBe(PostgresErrorCodes.UniqueViolation);
+            pg.ConstraintName.ShouldBe("IX_RoleClaims_RoleId_ClaimType_ClaimValue");
+
+            var count = await CountClaimRowsAsync(rootTenant, roleName, RetiredPermission);
+            count.ShouldBe(1, "The database must hold one row per grant.");
+        }
+        finally
+        {
+            await DeleteRoleAsync(rootTenant, roleName);
+        }
+    }
+
+    [Fact]
+    public async Task SyncRoleAsync_Should_Treat_A_Lost_Insert_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<IMultiTenantContextSetter>()
+                .MultiTenantContext = new MultiTenantContext<AppTenantInfo>(rootTenant);
+
+            // Stands in for a second instance starting at the same time: just before this instance's
+            // first save, it inserts the same missing permission from its own connection.
+            var otherInstance = new CompetingWriteBeforeFirstSaveInterceptor(
+                () => SeedClaimAsync(rootTenant, racedRole, ClaimConstants.Permission, ProbePermission.Name));
+            await using var db = CreateIdentityDbContext(sp, otherInstance);
+            var syncer = ActivatorUtilities.CreateInstance<RolePermissionSyncer>(
+                sp, db, Options.Create(new PermissionSyncOptions { RemoveRevoked = false }));
+
+            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 insert must have run.");
+            racedChanged.ShouldBe(0, "A lost insert race is reported as no change from this instance.");
+            (await CountClaimRowsAsync(rootTenant, racedRole, ProbePermission.Name))
+                .ShouldBe(1, "The other writer's row is the only one; ours must not be retried.");
+            nextChanged.ShouldBe(1, "The next role in the same scope must still sync.");
+            var next = await GetClaimsAsync(rootTenant, nextRole, ClaimConstants.Permission);
+            next.ShouldBe([RetiredPermission, ProbePermission.Name], ignoreOrder: true);
+        }
+        finally
+        {
+            await DeleteRoleAsync(rootTenant, racedRole);
+            await DeleteRoleAsync(rootTenant, nextRole);
+        }
+    }
+
     // ─── helpers ─────────────────────────────────────────────────────
 
+    private static IdentityDbContext CreateIdentityDbContext(IServiceProvider sp, IInterceptor interceptor)
+    {
+        var options = new DbContextOptionsBuilder<IdentityDbContext>(
+                sp.GetRequiredService<DbContextOptions<IdentityDbContext>>())
+            .AddInterceptors(interceptor)
+            .Options;
+        return new IdentityDbContext(
+            sp.GetRequiredService<IMultiTenantContextAccessor<AppTenantInfo>>(),
+            options,
+            sp.GetRequiredService<IOptions<DatabaseOptions>>(),
+            sp.GetRequiredService<IHostEnvironment>());
+    }
+
+    private async Task<int> CountClaimRowsAsync(AppTenantInfo tenant, string roleName, string claimValue)
+    {
+        using var scope = _factory.Services.CreateScope();
+        scope.ServiceProvider.GetRequiredService<IMultiTenantContextSetter>()
+            .MultiTenantContext = new MultiTenantContext<AppTenantInfo>(tenant);
+
+        var roleManager = scope.ServiceProvider.GetRequiredService<RoleManager<FshRole>>();
+        var role = await roleManager.Roles.SingleAsync(r => r.Name == roleName);
+
+        var db = scope.ServiceProvider.GetRequiredService<IdentityDbContext>();
+        return await db.RoleClaims.CountAsync(rc =>
+            rc.RoleId == role.Id && rc.ClaimType == ClaimConstants.Permission && rc.ClaimValue == claimValue);
+    }
+
     private async Task<int> RunSyncRoleAsync(AppTenantInfo tenant, string roleName, IReadOnlyList<FshPermission> target, bool removeRevoked)
     {
         using var scope = _factory.Services.CreateScope();
@@ -305,7 +389,7 @@ private async Task DeleteClaimAsync(AppTenantInfo tenant, string roleName, strin
         deleted.ShouldBe(1);
     }
 
-    private sealed class DeleteBeforeFirstSaveInterceptor(Func<Task> competingDelete) : SaveChangesInterceptor
+    private sealed class CompetingWriteBeforeFirstSaveInterceptor(Func<Task> competingWrite) : SaveChangesInterceptor
     {
         public bool Fired { get; private set; }
 
@@ -317,7 +401,7 @@ public override async ValueTask<InterceptionResult<int>> SavingChangesAsync(
             if (!Fired)
             {
                 Fired = true;
-                await competingDelete();
+                await competingWrite();
             }
 
             return result;