From d51d40e0061ea672f23ea57d06d433b915626c0b Mon Sep 17 00:00:00 2001 From: "Marcelo M. Maciel" <4993482+marcelo-maciel@users.noreply.github.com> Date: Mon, 28 Sep 2026 01:51:31 -0300 Subject: [PATCH 1/3] fix(identity): load a user's permissions under their own tenant, so a cold cache can't 401 a root operator The permission set is cached under perm:u:{userId}, a key with no tenant, and on a miss it was loaded through the request-scoped IdentityDbContext. For a root operator whose request the tenant-header override had moved to another tenant, that context could not see the root user, so FindByIdAsync returned null and the check threw UnauthorizedException: 401 on every permission-gated endpoint, whenever the entry happened to be cold (every 2 minutes without Redis, after an hour, a startup sync or a role/group invalidation with it). The loader now reads the user's TenantId ignoring query filters and, when it is not the resolved tenant, runs the unchanged load in a child scope set to the user's own tenant. Same-tenant requests take the old path. The regression test evicts the root admin's entry and sends the cross-tenant search: 401 before this change, 200 after. It is also why RootOperator_Should_TargetOtherTenant_When_HeaderProvided failed at random in CI. --- .../Services/UserPermissionService.cs | 64 ++++++++++++++++--- .../Multitenancy/TenantHeaderOverrideTests.cs | 31 +++++++++ 2 files changed, 86 insertions(+), 9 deletions(-) diff --git a/src/Modules/Identity/Modules.Identity/Services/UserPermissionService.cs b/src/Modules/Identity/Modules.Identity/Services/UserPermissionService.cs index 1be976d900..c0eb67c807 100644 --- a/src/Modules/Identity/Modules.Identity/Services/UserPermissionService.cs +++ b/src/Modules/Identity/Modules.Identity/Services/UserPermissionService.cs @@ -1,6 +1,9 @@ +using Finbuckle.MultiTenant; +using Finbuckle.MultiTenant.Abstractions; using FSH.Framework.Caching; using FSH.Framework.Core.Exceptions; using FSH.Framework.Shared.Constants; +using FSH.Framework.Shared.Multitenancy; using FSH.Modules.Identity.Caching; using FSH.Modules.Identity.Contracts.Services; using FSH.Modules.Identity.Data; @@ -8,6 +11,7 @@ using Microsoft.AspNetCore.Identity; using Microsoft.EntityFrameworkCore; using Microsoft.Extensions.Caching.Hybrid; +using Microsoft.Extensions.DependencyInjection; namespace FSH.Modules.Identity.Services; @@ -15,7 +19,9 @@ internal sealed class UserPermissionService( UserManager userManager, RoleManager roleManager, IdentityDbContext db, - HybridCache cache) : IUserPermissionService + HybridCache cache, + IMultiTenantContextAccessor tenantAccessor, + IServiceScopeFactory scopeFactory) : IUserPermissionService { // Hoisted to avoid per-call allocations. Small payload (< 4 KB after base64), so compression // CPU beats the marginal network savings — disable it for this hot path. @@ -52,7 +58,7 @@ private ValueTask GetOrLoadAsync(string userId, CancellationToken { // Stateless factory overload — the factory is a static method group, so the runtime // reuses a cached delegate and no closure is allocated per call (including L1 hits). - var state = new FactoryState(userManager, roleManager, db, userId); + var state = new FactoryState(userManager, roleManager, db, tenantAccessor, scopeFactory, userId); return cache.GetOrCreateAsync( CacheKeys.UserPermissions(userId), @@ -63,14 +69,52 @@ private ValueTask GetOrLoadAsync(string userId, CancellationToken cancellationToken: cancellationToken); } + // The cache key carries no tenant, and a root operator's request can resolve to another tenant through + // the tenant-header override. Loading under the request's tenant would not find the user there, so a cold + // entry turned a valid cross-tenant request into a 401. The set is always loaded under the user's own tenant. private static async ValueTask LoadPermissionsAsync(FactoryState s, CancellationToken ct) { - var user = await s.UserManager.FindByIdAsync(s.UserId).ConfigureAwait(false); + var homeTenantId = await s.Db.Users + .IgnoreQueryFilters() + .Where(u => u.Id == s.UserId) + .Select(u => EF.Property(u, "TenantId")) + .FirstOrDefaultAsync(ct).ConfigureAwait(false) + ?? throw new UnauthorizedException(); + + if (string.Equals(homeTenantId, s.TenantAccessor.MultiTenantContext.TenantInfo?.Id, StringComparison.Ordinal)) + { + return await LoadInCurrentTenantAsync(s.UserManager, s.RoleManager, s.Db, s.UserId, ct).ConfigureAwait(false); + } + + await using var scope = s.ScopeFactory.CreateAsyncScope(); + var services = scope.ServiceProvider; + var homeTenant = await services.GetRequiredService>() + .GetAsync(homeTenantId).ConfigureAwait(false) + ?? throw new UnauthorizedException(); + services.GetRequiredService() + .MultiTenantContext = new MultiTenantContext(homeTenant); + + return await LoadInCurrentTenantAsync( + services.GetRequiredService>(), + services.GetRequiredService>(), + services.GetRequiredService(), + s.UserId, + ct).ConfigureAwait(false); + } + + private static async Task LoadInCurrentTenantAsync( + UserManager userManager, + RoleManager roleManager, + IdentityDbContext db, + string userId, + CancellationToken ct) + { + var user = await userManager.FindByIdAsync(userId).ConfigureAwait(false); _ = user ?? throw new UnauthorizedException(); - var userRoles = await s.UserManager.GetRolesAsync(user).ConfigureAwait(false); + var userRoles = await userManager.GetRolesAsync(user).ConfigureAwait(false); - var directRoleIds = await s.RoleManager.Roles + var directRoleIds = await roleManager.Roles .Where(r => userRoles.Contains(r.Name!)) .Select(r => r.Id) .ToListAsync(ct).ConfigureAwait(false); @@ -78,9 +122,9 @@ private static async ValueTask LoadPermissionsAsync(FactoryState // 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) + var groupRoleIds = await db.GroupRoles + .Where(gr => db.UserGroups + .Where(ug => ug.UserId == userId) .Select(ug => ug.GroupId) .Contains(gr.GroupId)) .Select(gr => gr.RoleId) @@ -95,7 +139,7 @@ private static async ValueTask LoadPermissionsAsync(FactoryState } // Single query across all role IDs — cheaper than the old N+1 loop. - var perms = await s.Db.RoleClaims + var perms = await db.RoleClaims .Where(rc => roleIds.Contains(rc.RoleId) && rc.ClaimType == ClaimConstants.Permission) .Select(rc => rc.ClaimValue!) .Distinct() @@ -111,5 +155,7 @@ private readonly record struct FactoryState( UserManager UserManager, RoleManager RoleManager, IdentityDbContext Db, + IMultiTenantContextAccessor TenantAccessor, + IServiceScopeFactory ScopeFactory, string UserId); } diff --git a/src/Tests/Integration.Tests/Tests/Multitenancy/TenantHeaderOverrideTests.cs b/src/Tests/Integration.Tests/Tests/Multitenancy/TenantHeaderOverrideTests.cs index f9df4f2290..1e5859d396 100644 --- a/src/Tests/Integration.Tests/Tests/Multitenancy/TenantHeaderOverrideTests.cs +++ b/src/Tests/Integration.Tests/Tests/Multitenancy/TenantHeaderOverrideTests.cs @@ -1,8 +1,13 @@ #pragma warning disable S1144 // Unused private members — populated by JSON #pragma warning disable S3459 // Unassigned members — populated by JSON +using System.IdentityModel.Tokens.Jwt; using System.Net.Http.Json; +using System.Security.Claims; using System.Text.Json; +using FSH.Framework.Caching; using Integration.Tests.Infrastructure; +using Microsoft.Extensions.Caching.Hybrid; +using Microsoft.Extensions.DependencyInjection; namespace Integration.Tests.Tests.Multitenancy; @@ -86,6 +91,32 @@ public async Task RootOperator_Should_TargetOtherTenant_When_HeaderProvided() page.Items.ShouldNotContain(u => u.Email == _tenantBAdminEmail); } + [Fact] + public async Task RootOperator_Should_TargetOtherTenant_When_PermissionCacheIsCold() + { + // Arrange — evict the root admin's cached permission set, as its 2-minute local expiry (or any + // role/group invalidation) does, so the next permission check has to load it again. The cache key + // carries no tenant, and this request resolves to tenant A. + var rootToken = await _auth.GetRootAdminTokenAsync(); + var rootUserId = new JwtSecurityTokenHandler().ReadJwtToken(rootToken.AccessToken) + .Claims.First(c => c.Type is ClaimTypes.NameIdentifier or "nameid").Value; + await _factory.Services.GetRequiredService() + .RemoveAsync(CacheKeys.UserPermissions(rootUserId)); + + using var client = _factory.CreateClient(); + client.DefaultRequestHeaders.Authorization = new("Bearer", rootToken.AccessToken); + client.DefaultRequestHeaders.Add("tenant", _tenantA); + + // Act + var response = await client.GetAsync($"{TestConstants.IdentityBasePath}/users/search?PageNumber=1&PageSize=50"); + + // Assert — the root user does not exist in tenant A, so a load under the request's tenant 401s. + response.StatusCode.ShouldBe(HttpStatusCode.OK, await response.Content.ReadAsStringAsync()); + var page = await response.Content.ReadFromJsonAsync>(Json); + page.ShouldNotBeNull(); + page.Items.ShouldContain(u => u.Email == _tenantAAdminEmail); + } + [Fact] public async Task RootOperator_Should_UseOwnTenant_When_NoHeaderSent() { From c64b2e500e30b352a07cb1918bc1f23b33fce1bd Mon Sep 17 00:00:00 2001 From: "Marcelo M. Maciel" <4993482+marcelo-maciel@users.noreply.github.com> Date: Mon, 28 Sep 2026 01:51:33 -0300 Subject: [PATCH 2/3] docs(multitenancy): the claim strategy authenticates on its own, it is not a pre-auth no-op Finbuckle 10.1.0's ClaimStrategy runs the default scheme's handler itself when User is not authenticated yet, so an authenticated caller resolves to its tenant claim and the tenant header only decides for anonymous requests. TenantAdmin_HeaderOverride_Should_BeIgnored depends on exactly that. Three comments in MultitenancyModule and the architecture and multitenancy rules said the opposite. --- .agents/rules/architecture.md | 2 +- .agents/rules/modules/multitenancy.md | 2 +- .../Modules.Multitenancy/MultitenancyModule.cs | 15 +++++++++------ 3 files changed, 11 insertions(+), 8 deletions(-) diff --git a/.agents/rules/architecture.md b/.agents/rules/architecture.md index f74b68e860..f98d24b042 100644 --- a/.agents/rules/architecture.md +++ b/.agents/rules/architecture.md @@ -84,7 +84,7 @@ In `src/BuildingBlocks/Web/Extensions.cs` (`UseHeroPlatform`): 5. **`UseModuleMiddlewares`** — each module's `ConfigureMiddleware`, runs **after** auth 6. RateLimiting → Quotas → `UseAuthorization` → `MapModules` -`app.UseHeroMultiTenantDatabases()` (Finbuckle `UseMultiTenant()`) runs in `Program.cs` **before** `UseHeroPlatform`, i.e. **before `UseAuthentication`** — so tenant resolution is header-driven, not claim-driven. See `modules/multitenancy.md`. +`app.UseHeroMultiTenantDatabases()` (Finbuckle `UseMultiTenant()`) runs in `Program.cs` **before** `UseHeroPlatform`, i.e. **before `UseAuthentication`**. Finbuckle's `ClaimStrategy` authenticates on its own, so authenticated requests resolve by tenant claim and anonymous ones by the `tenant` header. See `modules/multitenancy.md`. ## Static/global state diff --git a/.agents/rules/modules/multitenancy.md b/.agents/rules/modules/multitenancy.md index 189933c848..59d7b66393 100644 --- a/.agents/rules/modules/multitenancy.md +++ b/.agents/rules/modules/multitenancy.md @@ -7,7 +7,7 @@ Tenant catalog, provisioning, activation/upgrade, per-tenant theming (Finbuckle. ## Gotchas -- **Finbuckle pipeline ordering** — strategy chain Claim → Header → `?tenant=` → DistributedCache → EFCoreStore, but `UseMultiTenant()` runs **before `UseAuthentication()`**, so the claim strategy no-ops (User is anonymous at resolution time). Resolution is effectively **header-driven** (`MultitenancyConstants.Identifier`). +- **Finbuckle pipeline ordering** — strategy chain Claim → Header → `?tenant=` → DistributedCache → EFCoreStore. `UseMultiTenant()` runs **before `UseAuthentication()`**, but Finbuckle's `ClaimStrategy` runs the default auth scheme's handler itself, so an authenticated caller resolves to its **tenant claim** and a `tenant` header cannot override it. The header (`MultitenancyConstants.Identifier`) decides only for anonymous requests (login, refresh); the root-operator override is post-auth middleware in `MultitenancyModule.ConfigureMiddleware`. - **Root-operator cross-tenant override is a post-auth middleware** in `ConfigureMiddleware` (not a Finbuckle strategy). Gate: caller's JWT tenant claim == `MultitenancyConstants.Root.Id` **and** a `tenant` header != root; it re-resolves via `IMultiTenantContextSetter`. Claim-aware tenant logic must go here, never in a strategy. - **`ITenantInitialPasswordBuffer`** (singleton) — the tenant admin password is **operator-supplied**, not a constant. `CreateTenantCommandHandler` calls `Store(tenantId, password)` **before** kicking off provisioning; the background seed step `TryConsume`s it (`ConcurrentDictionary`, consume = remove). - **Provisioning** runs 4 steps (Database → Migrations → Seeding → CacheWarm) via a Hangfire `TenantProvisioningJob`, falling back to inline execution if Hangfire storage is unavailable. **Activation is gated on `Status == Completed`.** diff --git a/src/Modules/Multitenancy/Modules.Multitenancy/MultitenancyModule.cs b/src/Modules/Multitenancy/Modules.Multitenancy/MultitenancyModule.cs index 096b032136..fb290123da 100644 --- a/src/Modules/Multitenancy/Modules.Multitenancy/MultitenancyModule.cs +++ b/src/Modules/Multitenancy/Modules.Multitenancy/MultitenancyModule.cs @@ -101,8 +101,10 @@ public void ConfigureServices(IHostApplicationBuilder builder) }; }) // ── Strategy chain — first non-null identifier wins (registration order) ── - // ClaimStrategy no-ops here: UseMultiTenant() runs BEFORE UseAuthentication(), so User is - // anonymous at resolution. Tenant stays header-driven; root override is post-auth middleware below. + // ClaimStrategy authenticates on its own: UseMultiTenant() runs before UseAuthentication(), so Finbuckle + // runs the default scheme's handler itself. An authenticated caller resolves to its tenant claim and a + // `tenant` header cannot move it; the header decides only for anonymous requests (login, refresh). + // The root-operator override is post-auth middleware below. .WithClaimStrategy(ClaimConstants.Tenant) .WithHeaderStrategy(MultitenancyConstants.Identifier) .WithDelegateStrategy(async context => @@ -132,8 +134,9 @@ public void ConfigureMiddleware(IApplicationBuilder app) ArgumentNullException.ThrowIfNull(app); // ── Root-operator header override ────────────────────────────── - // A "root"-claim caller scopes one request to another tenant via the `tenant` header (post-auth, since - // Finbuckle's pre-auth chain has no User). Gated on claim==root + header set != root + target exists. + // A "root"-claim caller scopes one request to another tenant via the `tenant` header. The claim strategy has + // already resolved that caller to root, so the switch happens here, post-auth. Gated on claim==root + + // header set != root + target exists. app.Use(async (ctx, next) => { var callerTenant = ctx.User?.FindFirstValue(ClaimConstants.Tenant); @@ -167,8 +170,8 @@ public void ConfigureMiddleware(IApplicationBuilder app) var accessor = ctx.RequestServices.GetRequiredService>(); var tenant = accessor.MultiTenantContext?.TenantInfo; - // Claim strategy no-ops pre-auth, so a JWT-only (no header) request may have no resolved - // tenant here — fall back to the caller's claim. + // An authenticated request normally resolves through the claim strategy; if nothing resolved, + // fall back to the caller's claim. if (tenant is null && !string.IsNullOrEmpty(callerTenant)) { var store = ctx.RequestServices.GetRequiredService>(); From d58cd497710e48046f2ab740b315cec6a7fbbaad Mon Sep 17 00:00:00 2001 From: "Marcelo M. Maciel" <4993482+marcelo-maciel@users.noreply.github.com> Date: Mon, 28 Sep 2026 02:09:21 -0300 Subject: [PATCH 3/3] fix(identity): take the root operator's home tenant from its claim, not an extra query The previous commit read every user's TenantId with IgnoreQueryFilters through the request-scoped context on each cache miss. That added a query to every miss, and with a per-tenant connection string (IdentityDbContext.OnConfiguring) the request context points at the target tenant's database, where the root row is absent, so the 401 stayed. Now the child scope opens only when the checked user is the caller and the caller's tenant claim differs from the resolved tenant; the scope is set to the claim's tenant, so it reaches that tenant's connection. The caller comes from IHttpContextAccessor because ICurrentUser is populated after authorization. Every other call takes the old path with no extra query. --- .../Services/UserPermissionService.cs | 34 +++++++++++++------ 1 file changed, 23 insertions(+), 11 deletions(-) diff --git a/src/Modules/Identity/Modules.Identity/Services/UserPermissionService.cs b/src/Modules/Identity/Modules.Identity/Services/UserPermissionService.cs index c0eb67c807..87c3ea9157 100644 --- a/src/Modules/Identity/Modules.Identity/Services/UserPermissionService.cs +++ b/src/Modules/Identity/Modules.Identity/Services/UserPermissionService.cs @@ -3,11 +3,13 @@ using FSH.Framework.Caching; using FSH.Framework.Core.Exceptions; using FSH.Framework.Shared.Constants; +using FSH.Framework.Shared.Identity.Claims; using FSH.Framework.Shared.Multitenancy; using FSH.Modules.Identity.Caching; using FSH.Modules.Identity.Contracts.Services; using FSH.Modules.Identity.Data; using FSH.Modules.Identity.Domain; +using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Identity; using Microsoft.EntityFrameworkCore; using Microsoft.Extensions.Caching.Hybrid; @@ -21,6 +23,7 @@ internal sealed class UserPermissionService( IdentityDbContext db, HybridCache cache, IMultiTenantContextAccessor tenantAccessor, + IHttpContextAccessor httpContextAccessor, IServiceScopeFactory scopeFactory) : IUserPermissionService { // Hoisted to avoid per-call allocations. Small payload (< 4 KB after base64), so compression @@ -58,7 +61,7 @@ private ValueTask GetOrLoadAsync(string userId, CancellationToken { // Stateless factory overload — the factory is a static method group, so the runtime // reuses a cached delegate and no closure is allocated per call (including L1 hits). - var state = new FactoryState(userManager, roleManager, db, tenantAccessor, scopeFactory, userId); + var state = new FactoryState(userManager, roleManager, db, CallerTenantOtherThanResolved(userId), scopeFactory, userId); return cache.GetOrCreateAsync( CacheKeys.UserPermissions(userId), @@ -71,17 +74,26 @@ private ValueTask GetOrLoadAsync(string userId, CancellationToken // The cache key carries no tenant, and a root operator's request can resolve to another tenant through // the tenant-header override. Loading under the request's tenant would not find the user there, so a cold - // entry turned a valid cross-tenant request into a 401. The set is always loaded under the user's own tenant. - private static async ValueTask LoadPermissionsAsync(FactoryState s, CancellationToken ct) + // entry turned a valid cross-tenant request into a 401. When the checked user is the caller and the caller's + // tenant claim differs from the resolved tenant, the set is loaded under the claim's tenant instead. + private string? CallerTenantOtherThanResolved(string userId) { - var homeTenantId = await s.Db.Users - .IgnoreQueryFilters() - .Where(u => u.Id == s.UserId) - .Select(u => EF.Property(u, "TenantId")) - .FirstOrDefaultAsync(ct).ConfigureAwait(false) - ?? throw new UnauthorizedException(); + var caller = httpContextAccessor.HttpContext?.User; + if (caller?.Identity?.IsAuthenticated != true + || !string.Equals(caller.GetUserId(), userId, StringComparison.Ordinal) + || caller.GetTenant() is not { Length: > 0 } claimTenant) + { + return null; + } - if (string.Equals(homeTenantId, s.TenantAccessor.MultiTenantContext.TenantInfo?.Id, StringComparison.Ordinal)) + return string.Equals(claimTenant, tenantAccessor.MultiTenantContext.TenantInfo?.Id, StringComparison.Ordinal) + ? null + : claimTenant; + } + + private static async ValueTask LoadPermissionsAsync(FactoryState s, CancellationToken ct) + { + if (s.HomeTenantId is not { } homeTenantId) { return await LoadInCurrentTenantAsync(s.UserManager, s.RoleManager, s.Db, s.UserId, ct).ConfigureAwait(false); } @@ -155,7 +167,7 @@ private readonly record struct FactoryState( UserManager UserManager, RoleManager RoleManager, IdentityDbContext Db, - IMultiTenantContextAccessor TenantAccessor, + string? HomeTenantId, IServiceScopeFactory ScopeFactory, string UserId); }