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/Identity/Modules.Identity/Services/UserPermissionService.cs b/src/Modules/Identity/Modules.Identity/Services/UserPermissionService.cs index 1be976d900..87c3ea9157 100644 --- a/src/Modules/Identity/Modules.Identity/Services/UserPermissionService.cs +++ b/src/Modules/Identity/Modules.Identity/Services/UserPermissionService.cs @@ -1,13 +1,19 @@ +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.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; +using Microsoft.Extensions.DependencyInjection; namespace FSH.Modules.Identity.Services; @@ -15,7 +21,10 @@ internal sealed class UserPermissionService( UserManager userManager, RoleManager roleManager, IdentityDbContext db, - HybridCache cache) : IUserPermissionService + HybridCache cache, + IMultiTenantContextAccessor tenantAccessor, + IHttpContextAccessor httpContextAccessor, + 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 +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, userId); + var state = new FactoryState(userManager, roleManager, db, CallerTenantOtherThanResolved(userId), scopeFactory, userId); return cache.GetOrCreateAsync( CacheKeys.UserPermissions(userId), @@ -63,14 +72,61 @@ 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. 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 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; + } + + return string.Equals(claimTenant, tenantAccessor.MultiTenantContext.TenantInfo?.Id, StringComparison.Ordinal) + ? null + : claimTenant; + } + private static async ValueTask LoadPermissionsAsync(FactoryState s, CancellationToken ct) { - var user = await s.UserManager.FindByIdAsync(s.UserId).ConfigureAwait(false); + if (s.HomeTenantId is not { } homeTenantId) + { + 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 +134,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 +151,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 +167,7 @@ private readonly record struct FactoryState( UserManager UserManager, RoleManager RoleManager, IdentityDbContext Db, + string? HomeTenantId, + IServiceScopeFactory ScopeFactory, string UserId); } 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>(); 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() {