From 7d8f2d7af4218ecada5323ebbffc03e5aac359c8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=A9sar=20Castro?= Date: Sun, 17 May 2026 21:11:13 +0200 Subject: [PATCH 1/7] feat(caching): introduce ITenantCacheService structural guardrail for tenant isolation Adds TenantHybridCache, a scoped decorator of HybridCache that automatically prefixes every cache key and tag with the active tenant ID (t:{tenantId}:). This prevents cross-tenant cache collisions without requiring callers to manage prefixes manually. Changes: - BuildingBlocks/Caching: ITenantCacheService interface, TenantHybridCache implementation, registered as Scoped in Extensions.AddHeroCaching() - Identity: UserPermissionService and RolePermissionSyncer migrated from HybridCache to ITenantCacheService - Multitenancy: TenantThemeService migrated; DefaultTheme (cross-tenant by design) keeps direct HybridCache - Tests: 4 new unit tests verifying tenant isolation, null context guards, and Scoped DI registration Architecture rule: business module code must inject ITenantCacheService, not HybridCache directly. --- src/BuildingBlocks/Caching/Caching.csproj | 9 +- src/BuildingBlocks/Caching/Extensions.cs | 16 +++ .../Caching/ITenantCacheService.cs | 54 +++++++ .../Caching/TenantHybridCache.cs | 127 +++++++++++++++++ .../Authorization/RolePermissionSyncer.cs | 3 +- .../Services/UserPermissionService.cs | 2 +- .../Services/TenantThemeService.cs | 26 ++-- src/Tests/Caching.Tests/Caching.Tests.csproj | 2 + .../HeroCachingRegistrationTests.cs | 16 +++ .../Caching.Tests/TenantCacheServiceTests.cs | 133 ++++++++++++++++++ 10 files changed, 374 insertions(+), 14 deletions(-) create mode 100644 src/BuildingBlocks/Caching/ITenantCacheService.cs create mode 100644 src/BuildingBlocks/Caching/TenantHybridCache.cs create mode 100644 src/Tests/Caching.Tests/TenantCacheServiceTests.cs diff --git a/src/BuildingBlocks/Caching/Caching.csproj b/src/BuildingBlocks/Caching/Caching.csproj index e326c179eb..fd575afa55 100644 --- a/src/BuildingBlocks/Caching/Caching.csproj +++ b/src/BuildingBlocks/Caching/Caching.csproj @@ -1,4 +1,4 @@ - + FSH.Framework.Caching @@ -6,6 +6,12 @@ FullStackHero.Framework.Caching + + + <_Parameter1>Caching.Tests + + + @@ -17,6 +23,7 @@ + diff --git a/src/BuildingBlocks/Caching/Extensions.cs b/src/BuildingBlocks/Caching/Extensions.cs index 1dd28108ad..736621be15 100644 --- a/src/BuildingBlocks/Caching/Extensions.cs +++ b/src/BuildingBlocks/Caching/Extensions.cs @@ -1,3 +1,4 @@ +using Finbuckle.MultiTenant.Abstractions; using Microsoft.AspNetCore.DataProtection; using Microsoft.Extensions.Caching.Hybrid; using Microsoft.Extensions.Configuration; @@ -85,6 +86,21 @@ public static IServiceCollection AddHeroCaching(this IServiceCollection services // factory that builds the inner via the original descriptor and returns our wrapper. DecorateHybridCache(services); + // Register the tenant-scoped cache service. Scoped lifetime because the tenant context + // (and therefore the key prefix) is per-request. Module code must inject + // ITenantCacheService — injecting HybridCache directly is prohibited in module assemblies + // and enforced by architecture tests. + services.AddScoped(sp => + new TenantHybridCache( + sp.GetRequiredService(), + sp.GetRequiredService())); + + // Register the global (cross-tenant) cache service. Singleton lifetime because it + // carries no per-request state. Use this ONLY for data shared across all tenants, + // e.g. system defaults, global lookup tables, shared configuration. + services.AddSingleton(sp => + new GlobalHybridCache(sp.GetRequiredService())); + return services; } diff --git a/src/BuildingBlocks/Caching/ITenantCacheService.cs b/src/BuildingBlocks/Caching/ITenantCacheService.cs new file mode 100644 index 0000000000..86ccee7a50 --- /dev/null +++ b/src/BuildingBlocks/Caching/ITenantCacheService.cs @@ -0,0 +1,54 @@ +using Microsoft.Extensions.Caching.Hybrid; + +namespace FSH.Framework.Caching; + +/// +/// Tenant-scoped wrapper over . +/// All keys and tags are automatically prefixed with the current tenant identifier, +/// preventing cross-tenant cache collisions without requiring callers to manage +/// the prefix themselves. +/// +/// +/// Register as Scoped because the tenant context (and therefore the prefix) +/// changes per HTTP request. Inject in module +/// code — do NOT inject directly from business modules. +/// +public interface ITenantCacheService +{ + /// + /// Gets an existing cache entry or creates a new one using the provided factory, + /// automatically scoping the key and tags to the current tenant. + /// + ValueTask GetOrCreateAsync( + string key, + TState state, + Func> factory, + HybridCacheEntryOptions? options = null, + IEnumerable? tags = null, + CancellationToken cancellationToken = default); + + /// + /// Sets a cache entry for the given key, scoped to the current tenant. + /// + ValueTask SetAsync( + string key, + T value, + HybridCacheEntryOptions? options = null, + IEnumerable? tags = null, + CancellationToken cancellationToken = default); + + /// + /// Removes a cache entry by key, scoped to the current tenant. + /// + ValueTask RemoveAsync(string key, CancellationToken cancellationToken = default); + + /// + /// Removes all cache entries carrying the given tag, scoped to the current tenant. + /// + ValueTask RemoveByTagAsync(string tag, CancellationToken cancellationToken = default); + + /// + /// Removes all cache entries carrying any of the given tags, scoped to the current tenant. + /// + ValueTask RemoveByTagAsync(IEnumerable tags, CancellationToken cancellationToken = default); +} diff --git a/src/BuildingBlocks/Caching/TenantHybridCache.cs b/src/BuildingBlocks/Caching/TenantHybridCache.cs new file mode 100644 index 0000000000..2abcbc2941 --- /dev/null +++ b/src/BuildingBlocks/Caching/TenantHybridCache.cs @@ -0,0 +1,127 @@ +using Finbuckle.MultiTenant; +using Finbuckle.MultiTenant.Abstractions; +using Microsoft.Extensions.Caching.Hybrid; + +namespace FSH.Framework.Caching; + +/// +/// implementation that wraps +/// and automatically scopes every key and tag with the active tenant identifier. +/// +/// +/// This class must be registered as Scoped so that each HTTP request gets +/// a fresh instance bound to the correct tenant context. The underlying +/// remains Singleton and is shared across tenants — +/// only the key/tag prefixing is tenant-specific. +/// +internal sealed class TenantHybridCache : ITenantCacheService +{ + private readonly HybridCache _cache; + private readonly IMultiTenantContextAccessor _tenantAccessor; + + public TenantHybridCache(HybridCache cache, IMultiTenantContextAccessor tenantAccessor) + { + ArgumentNullException.ThrowIfNull(cache); + ArgumentNullException.ThrowIfNull(tenantAccessor); + _cache = cache; + _tenantAccessor = tenantAccessor; + } + + /// + /// Internal factory for test projects (via InternalsVisibleTo). Prefer DI for production code. + /// + internal static ITenantCacheService Create(HybridCache cache, IMultiTenantContextAccessor accessor) + => new TenantHybridCache(cache, accessor); + + // ----------------------------------------------------------------------- + // Key/tag scoping helpers + // ----------------------------------------------------------------------- + + private string GetTenantId() + { + var tenantId = _tenantAccessor.MultiTenantContext?.TenantInfo?.Id; + if (string.IsNullOrEmpty(tenantId)) + { + throw new InvalidOperationException( + "No active tenant context. TenantHybridCache requires a resolved tenant. " + + "Ensure the request passes through the Finbuckle middleware before the cache is accessed."); + } + + return tenantId; + } + + /// Scopes a logical key to the current tenant: t:{tenantId}:{key}. + private string ScopeKey(string key) => $"t:{GetTenantId()}:{key}"; + + /// + /// Prepends a per-tenant tag (tenant:{tenantId}) to any caller-supplied tags. + /// This guarantees that a on the + /// tag correctly purges all entries for a tenant. + /// + private static IEnumerable ScopeTags(string tenantId, IEnumerable? callerTags) + { + yield return CacheKeys.Tags.Tenant(tenantId); + + if (callerTags is null) yield break; + foreach (var tag in callerTags) yield return tag; + } + + // ----------------------------------------------------------------------- + // ITenantCacheService implementation + // ----------------------------------------------------------------------- + + /// + public ValueTask GetOrCreateAsync( + string key, + TState state, + Func> factory, + HybridCacheEntryOptions? options = null, + IEnumerable? tags = null, + CancellationToken cancellationToken = default) + { + var tenantId = GetTenantId(); + return _cache.GetOrCreateAsync( + ScopeKey(key), + state, + factory, + options, + ScopeTags(tenantId, tags), + cancellationToken); + } + + /// + public ValueTask SetAsync( + string key, + T value, + HybridCacheEntryOptions? options = null, + IEnumerable? tags = null, + CancellationToken cancellationToken = default) + { + var tenantId = GetTenantId(); + return _cache.SetAsync( + ScopeKey(key), + value, + options, + ScopeTags(tenantId, tags), + cancellationToken); + } + + /// + public ValueTask RemoveAsync(string key, CancellationToken cancellationToken = default) + => _cache.RemoveAsync(ScopeKey(key), cancellationToken); + + /// + public ValueTask RemoveByTagAsync(string tag, CancellationToken cancellationToken = default) + { + var tenantId = GetTenantId(); + // Scope the tag so only entries for this tenant are evicted. + return _cache.RemoveByTagAsync($"t:{tenantId}:{tag}", cancellationToken); + } + + /// + public ValueTask RemoveByTagAsync(IEnumerable tags, CancellationToken cancellationToken = default) + { + var tenantId = GetTenantId(); + return _cache.RemoveByTagAsync(tags.Select(t => $"t:{tenantId}:{t}"), cancellationToken); + } +} diff --git a/src/Modules/Identity/Modules.Identity/Authorization/RolePermissionSyncer.cs b/src/Modules/Identity/Modules.Identity/Authorization/RolePermissionSyncer.cs index d1148c9045..290599c3c9 100644 --- a/src/Modules/Identity/Modules.Identity/Authorization/RolePermissionSyncer.cs +++ b/src/Modules/Identity/Modules.Identity/Authorization/RolePermissionSyncer.cs @@ -7,7 +7,6 @@ using Finbuckle.MultiTenant.Abstractions; using Microsoft.AspNetCore.Identity; using Microsoft.EntityFrameworkCore; -using Microsoft.Extensions.Caching.Hybrid; using Microsoft.Extensions.Logging; namespace FSH.Modules.Identity.Authorization; @@ -21,7 +20,7 @@ public sealed class RolePermissionSyncer( IdentityDbContext context, RoleManager roleManager, IMultiTenantContextAccessor tenantAccessor, - HybridCache cache, + ITenantCacheService cache, TimeProvider timeProvider, ILogger logger) { diff --git a/src/Modules/Identity/Modules.Identity/Services/UserPermissionService.cs b/src/Modules/Identity/Modules.Identity/Services/UserPermissionService.cs index 48477c6f36..7d5999c27c 100644 --- a/src/Modules/Identity/Modules.Identity/Services/UserPermissionService.cs +++ b/src/Modules/Identity/Modules.Identity/Services/UserPermissionService.cs @@ -15,7 +15,7 @@ internal sealed class UserPermissionService( UserManager userManager, RoleManager roleManager, IdentityDbContext db, - HybridCache cache) : IUserPermissionService + ITenantCacheService cache) : 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. diff --git a/src/Modules/Multitenancy/Modules.Multitenancy/Services/TenantThemeService.cs b/src/Modules/Multitenancy/Modules.Multitenancy/Services/TenantThemeService.cs index e21fc9e4fe..3fab220981 100644 --- a/src/Modules/Multitenancy/Modules.Multitenancy/Services/TenantThemeService.cs +++ b/src/Modules/Multitenancy/Modules.Multitenancy/Services/TenantThemeService.cs @@ -26,7 +26,8 @@ public sealed class TenantThemeService : ITenantThemeService private static readonly string[] DefaultThemeTags = [CacheKeys.Tags.Themes]; - private readonly HybridCache _cache; + private readonly ITenantCacheService _cache; + private readonly HybridCache _globalCache; // for cross-tenant entries like DefaultTheme private readonly TenantDbContext _dbContext; private readonly IMultiTenantContextAccessor _tenantAccessor; private readonly IStorageService _storageService; @@ -34,7 +35,8 @@ public sealed class TenantThemeService : ITenantThemeService private readonly ICurrentUser _currentUser; public TenantThemeService( - HybridCache cache, + ITenantCacheService cache, + HybridCache globalCache, TenantDbContext dbContext, IMultiTenantContextAccessor tenantAccessor, IStorageService storageService, @@ -42,6 +44,7 @@ public TenantThemeService( ICurrentUser currentUser) { _cache = cache; + _globalCache = globalCache; _dbContext = dbContext; _tenantAccessor = tenantAccessor; _storageService = storageService; @@ -60,9 +63,9 @@ public Task GetThemeAsync(string tenantId, CancellationToken ct { ArgumentException.ThrowIfNullOrWhiteSpace(tenantId); - // Per-tenant tag array — one small alloc per call is unavoidable because the tag is - // parameterized by tenantId. Keeping the array allocation local (not LOH) and short-lived. - var tags = new[] { CacheKeys.Tags.Themes, CacheKeys.Tags.Tenant(tenantId) }; + // Tags are relative — TenantHybridCache automatically adds the tenant prefix. + // CacheKeys.Tags.Themes is enough; the per-tenant tag is injected structurally. + var tags = new[] { CacheKeys.Tags.Themes }; // Stateless factory via a static method group — no closure allocation even on L1 hits. var state = new TenantFactoryState(_dbContext, tenantId); @@ -77,7 +80,9 @@ public Task GetThemeAsync(string tenantId, CancellationToken ct public Task GetDefaultThemeAsync(CancellationToken ct = default) { - return _cache.GetOrCreateAsync( + // DefaultTheme is intentionally cross-tenant (same for all tenants). + // We use the underlying HybridCache (not the tenant-scoped wrapper) for this entry. + return _globalCache.GetOrCreateAsync( CacheKeys.DefaultTheme, _dbContext, LoadDefaultThemeAsync, @@ -246,8 +251,8 @@ public async Task SetAsDefaultThemeAsync(string tenantId, CancellationToken ct = entity.IsDefault = true; await _dbContext.SaveChangesAsync(ct).ConfigureAwait(false); - // Invalidate default theme cache - await _cache.RemoveAsync(CacheKeys.DefaultTheme, ct).ConfigureAwait(false); + // Invalidate default theme cache (global cache, not tenant-scoped) + await _globalCache.RemoveAsync(CacheKeys.DefaultTheme, ct).ConfigureAwait(false); if (_logger.IsEnabled(LogLevel.Information)) { @@ -257,9 +262,10 @@ public async Task SetAsDefaultThemeAsync(string tenantId, CancellationToken ct = public async Task InvalidateCacheAsync(string tenantId, CancellationToken ct = default) { - // Purge both the tenant-specific entry and anything tagged for this tenant. + // With TenantHybridCache the key is already prefixed, so removing the logical key + // purges the tenant-scoped entry. RemoveByTagAsync similarly purges only this tenant's data. await _cache.RemoveAsync(CacheKeys.TenantTheme(tenantId), ct).ConfigureAwait(false); - await _cache.RemoveByTagAsync(CacheKeys.Tags.Tenant(tenantId), ct).ConfigureAwait(false); + await _cache.RemoveByTagAsync(CacheKeys.Tags.Themes, ct).ConfigureAwait(false); } private static TenantThemeDto MapEntityToDto(TenantTheme entity) diff --git a/src/Tests/Caching.Tests/Caching.Tests.csproj b/src/Tests/Caching.Tests/Caching.Tests.csproj index 63ad564cbd..8b13c5efb3 100644 --- a/src/Tests/Caching.Tests/Caching.Tests.csproj +++ b/src/Tests/Caching.Tests/Caching.Tests.csproj @@ -22,6 +22,8 @@ runtime; build; native; contentfiles; analyzers; buildtransitive + + diff --git a/src/Tests/Caching.Tests/HeroCachingRegistrationTests.cs b/src/Tests/Caching.Tests/HeroCachingRegistrationTests.cs index af3fd046c4..f227eb5d7c 100644 --- a/src/Tests/Caching.Tests/HeroCachingRegistrationTests.cs +++ b/src/Tests/Caching.Tests/HeroCachingRegistrationTests.cs @@ -64,4 +64,20 @@ public void AddHeroCaching_Should_Throw_When_ConfigurationIsNull() Should.Throw(() => services.AddHeroCaching(null!)); } + + [Fact] + public void AddHeroCaching_Should_RegisterITenantCacheService() + { + // Arrange + var services = new ServiceCollection(); + var config = new ConfigurationBuilder().Build(); + + // Act + services.AddHeroCaching(config); + + // Assert — ITenantCacheService must be registered (descriptor exists) + var descriptor = services.FirstOrDefault(d => d.ServiceType == typeof(ITenantCacheService)); + descriptor.ShouldNotBeNull(); + descriptor.Lifetime.ShouldBe(ServiceLifetime.Scoped); + } } diff --git a/src/Tests/Caching.Tests/TenantCacheServiceTests.cs b/src/Tests/Caching.Tests/TenantCacheServiceTests.cs new file mode 100644 index 0000000000..9b9695b9e7 --- /dev/null +++ b/src/Tests/Caching.Tests/TenantCacheServiceTests.cs @@ -0,0 +1,133 @@ +using Finbuckle.MultiTenant; +using Finbuckle.MultiTenant.Abstractions; +using FSH.Framework.Caching; +using FSH.Framework.Shared.Multitenancy; +using Microsoft.Extensions.Caching.Hybrid; +using Microsoft.Extensions.Configuration; +using Microsoft.Extensions.DependencyInjection; +using NSubstitute; + +namespace Caching.Tests; + +/// +/// Verifies that automatically scopes every cache +/// key and tag with the current tenant's identifier, preventing cross-tenant data leaks. +/// +public sealed class TenantCacheServiceTests +{ + private static AppTenantInfo BuildTenant(string id) => + new(id, id, string.Empty, $"{id}@test.com", issuer: null); + + private static IMultiTenantContextAccessor BuildAccessor(AppTenantInfo tenant) + { + var accessor = Substitute.For(); + accessor.MultiTenantContext.Returns(new MultiTenantContext(tenant)); + return accessor; + } + + private static (ITenantCacheService sut, HybridCache innerCache) CreateSut(string tenantId) + { + var tenant = BuildTenant(tenantId); + var accessor = BuildAccessor(tenant); + + var services = new ServiceCollection(); + services.AddHeroCaching(new ConfigurationBuilder().Build()); + var provider = services.BuildServiceProvider(); + var innerCache = provider.GetRequiredService(); + + return (TenantHybridCache.Create(innerCache, accessor), innerCache); + } + + [Fact] + public async Task GetOrCreateAsync_DifferentTenants_ShouldNotShareEntries() + { + // Arrange — two caches sharing the same underlying HybridCache but with different tenants + var services = new ServiceCollection(); + services.AddHeroCaching(new ConfigurationBuilder().Build()); + var provider = services.BuildServiceProvider(); + var innerCache = provider.GetRequiredService(); + + var sutA = TenantHybridCache.Create(innerCache, BuildAccessor(BuildTenant("tenant-a"))); + var sutB = TenantHybridCache.Create(innerCache, BuildAccessor(BuildTenant("tenant-b"))); + + int factoryCallCount = 0; + + // Act — write through tenant A's cache + await sutA.GetOrCreateAsync("shared-key", "state", (_, _) => + { + factoryCallCount++; + return ValueTask.FromResult("value-for-a"); + }); + + // Act — read through tenant B's cache (same logical key, different tenant scope) + var resultForB = await sutB.GetOrCreateAsync("shared-key", "state", (_, _) => + { + factoryCallCount++; + return ValueTask.FromResult("value-for-b"); + }); + + // Assert — factory must be called TWICE: tenant isolation is working + factoryCallCount.ShouldBe(2); + resultForB.ShouldBe("value-for-b"); + } + + [Fact] + public async Task SetAsync_RemoveAsync_Should_ScopeKey_To_Tenant() + { + // Arrange + var (sut, _) = CreateSut("tenant-set"); + await sut.SetAsync("mykey", "myvalue"); + + // Act — remove the tenant-scoped entry + await sut.RemoveAsync("mykey"); + + // Re-read — factory should run again after eviction + int factoryCount = 0; + await sut.GetOrCreateAsync("mykey", "state", (_, _) => + { + factoryCount++; + return ValueTask.FromResult("fresh"); + }); + + // Assert — factory ran, confirming removal operated on the correct scoped key + factoryCount.ShouldBe(1); + } + + [Fact] + public async Task GetOrCreateAsync_ShouldThrow_When_TenantContextIsNull() + { + // Arrange — accessor returns null context (no tenant resolved) + var accessor = Substitute.For(); + accessor.MultiTenantContext.Returns((IMultiTenantContext?)null!); + + var services = new ServiceCollection(); + services.AddHeroCaching(new ConfigurationBuilder().Build()); + var innerCache = services.BuildServiceProvider().GetRequiredService(); + + var sut = TenantHybridCache.Create(innerCache, accessor); + + // Act & Assert + await Should.ThrowAsync( + () => sut.GetOrCreateAsync("key", "state", + (_, _) => ValueTask.FromResult("v")).AsTask()); + } + + [Fact] + public async Task GetOrCreateAsync_ShouldThrow_When_TenantIdIsEmpty() + { + // Arrange — tenant context present but TenantId is empty + var tenant = BuildTenant(string.Empty); + var accessor = BuildAccessor(tenant); + + var services = new ServiceCollection(); + services.AddHeroCaching(new ConfigurationBuilder().Build()); + var innerCache = services.BuildServiceProvider().GetRequiredService(); + + var sut = TenantHybridCache.Create(innerCache, accessor); + + // Act & Assert + await Should.ThrowAsync( + () => sut.GetOrCreateAsync("key", "state", + (_, _) => ValueTask.FromResult("v")).AsTask()); + } +} From f80bba87d44919386a8714fea5976b9fc226281c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=A9sar=20Castro?= Date: Sun, 17 May 2026 21:19:22 +0200 Subject: [PATCH 2/7] test(arch): add CachingArchitectureTests to enforce ITenantCacheService guardrail 4 architecture rules enforced via NetArchTest: 1. ModuleClasses_Should_Not_Depend_On_HybridCache_Directly: scans all module assemblies via ModuleAssemblyDiscovery and fails if any type (excluding the intentional TenantThemeService exception) has a direct dependency on Microsoft.Extensions.Caching.Hybrid. 2. ITenantCacheService_Should_Be_Public: verifies the interface is public and accessible from modules. 3. TenantHybridCache_Implementation_Should_Be_Internal: verifies the implementation cannot be newed up directly from outside the Caching assembly. 4. CachingArchitectureTests_Should_HaveAtLeastOneModuleToScan: guard to prevent vacuous pass with empty assembly list. --- .../Architecture.Tests.csproj | 1 + .../CachingArchitectureTests.cs | 127 ++++++++++++++++++ 2 files changed, 128 insertions(+) create mode 100644 src/Tests/Architecture.Tests/CachingArchitectureTests.cs diff --git a/src/Tests/Architecture.Tests/Architecture.Tests.csproj b/src/Tests/Architecture.Tests/Architecture.Tests.csproj index ff69562c3d..1ff49d2e19 100644 --- a/src/Tests/Architecture.Tests/Architecture.Tests.csproj +++ b/src/Tests/Architecture.Tests/Architecture.Tests.csproj @@ -24,6 +24,7 @@ + diff --git a/src/Tests/Architecture.Tests/CachingArchitectureTests.cs b/src/Tests/Architecture.Tests/CachingArchitectureTests.cs new file mode 100644 index 0000000000..a4379cfa71 --- /dev/null +++ b/src/Tests/Architecture.Tests/CachingArchitectureTests.cs @@ -0,0 +1,127 @@ +using FSH.Framework.Caching; +using Microsoft.Extensions.Caching.Hybrid; +using NetArchTest.Rules; +using System.Reflection; + +namespace Architecture.Tests; + +/// +/// Architecture guardrails for the caching layer. +/// These tests prevent developers from accidentally injecting +/// directly into business module code, which would bypass the automatic per-tenant +/// key scoping provided by . +/// +/// The rule: module assemblies must inject , +/// not directly. is allowed only in +/// BuildingBlocks (the implementation layer) and in designated cross-tenant infrastructure. +/// +public sealed class CachingArchitectureTests +{ + /// + /// All FSH module assemblies discovered at test run time. + /// The list grows automatically as new modules are added to the test project. + /// + private static readonly Assembly[] ModuleAssemblies = + ModuleAssemblyDiscovery.GetModuleAssemblies(); + + // ------------------------------------------------------------------------- + // Rule 1 — No direct HybridCache injection in module constructors + // ------------------------------------------------------------------------- + + /// + /// Verifies that no class in any business module has a constructor parameter + /// of type . All module code must inject + /// so tenant isolation is enforced structurally. + /// + [Fact] + public void ModuleClasses_Should_Not_Depend_On_HybridCache_Directly() + { + // NetArchTest detects HybridCache as a dependency via the assembly's metadata. + // A constructor injection of HybridCache creates a dependency on its declaring + // namespace: Microsoft.Extensions.Caching.Hybrid. + foreach (var assembly in ModuleAssemblies) + { + var result = Types + .InAssembly(assembly) + .ShouldNot() + .HaveDependencyOn("Microsoft.Extensions.Caching.Hybrid") + .GetResult(); + + var failingTypes = result.FailingTypeNames ?? []; + + // TenantThemeService is a known, intentional exception: + // it holds a secondary HybridCache reference for the cross-tenant DefaultTheme entry. + // All other violations are real bugs. + var realViolations = failingTypes + .Where(t => !t.EndsWith("TenantThemeService", StringComparison.Ordinal)) + .ToList(); + + realViolations.ShouldBeEmpty( + $"Module '{assembly.GetName().Name}' contains types that depend directly on " + + $"HybridCache. Use ITenantCacheService instead to guarantee per-tenant key scoping. " + + $"Violating types: {string.Join(", ", realViolations)}"); + } + } + + // ------------------------------------------------------------------------- + // Rule 2 — ITenantCacheService is registered (DI contract) + // ------------------------------------------------------------------------- + + /// + /// Verifies that the type is publicly accessible + /// from the Caching assembly. If someone accidentally changes its visibility, + /// module code would break at DI resolution time rather than compile time. + /// + [Fact] + public void ITenantCacheService_Should_Be_Public() + { + var type = typeof(ITenantCacheService); + + type.IsInterface.ShouldBeTrue( + $"{type.FullName} must be an interface so modules can depend on the abstraction."); + + type.IsPublic.ShouldBeTrue( + $"{type.FullName} must be public so module assemblies can reference it."); + } + + // ------------------------------------------------------------------------- + // Rule 3 — TenantHybridCache implementation remains internal (encapsulation) + // ------------------------------------------------------------------------- + + /// + /// Verifies that the concrete TenantHybridCache implementation is internal, + /// preventing modules from bypassing the interface and newing it up directly. + /// + [Fact] + public void TenantHybridCache_Implementation_Should_Be_Internal() + { + var cachingAssembly = typeof(ITenantCacheService).Assembly; + + var implType = cachingAssembly + .GetTypes() + .FirstOrDefault(t => t.Name == "TenantHybridCache"); + + implType.ShouldNotBeNull( + "TenantHybridCache implementation type must exist in the Caching assembly."); + + implType!.IsPublic.ShouldBeFalse( + "TenantHybridCache should be internal so module code cannot instantiate it directly. " + + "All consumption must go through ITenantCacheService resolved via DI."); + } + + // ------------------------------------------------------------------------- + // Rule 4 — Guard: at least one module was scanned + // ------------------------------------------------------------------------- + + /// + /// Prevents the tenant isolation check from silently passing with an empty assembly list + /// (which would make rules 1 vacuously true and miss real violations in new modules). + /// + [Fact] + public void CachingArchitectureTests_Should_HaveAtLeastOneModuleToScan() + { + ModuleAssemblies.ShouldNotBeEmpty( + "At least one module assembly must be loaded for the caching architecture " + + "rules to be meaningful. Check Architecture.Tests.csproj project references."); + } +} From 5ccb242ddd47d33fc0fbaecc490748d5afef3864 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=A9sar=20Castro?= Date: Sun, 17 May 2026 21:31:06 +0200 Subject: [PATCH 3/7] fix(caching): add IGlobalCacheService, fix CachingArchitectureTests CI build failure Fixes: - CachingArchitectureTests.cs was missing 'using Xunit;' (CS0246 FactAttribute not found in CI) - Refines architecture rule to check for the HybridCache class specifically (not the whole namespace) so HybridCacheEntryOptions stays allowed as a pure options type New: - IGlobalCacheService: public interface for intentionally cross-tenant cache entries - GlobalHybridCache: internal sealed implementation (Singleton), thin delegate to HybridCache - Registered as Singleton in AddHeroCaching() - TenantThemeService: migrated _globalCache from HybridCache to IGlobalCacheService Architecture test now has 5 clean rules with zero exceptions: 1. No HybridCache class injection in any module 2. ITenantCacheService is a public interface 3. IGlobalCacheService is a public interface 4. Both implementations (TenantHybridCache, GlobalHybridCache) are internal 5. Guard: at least one module assembly scanned --- .../Caching/GlobalHybridCache.cs | 46 +++++++ .../Caching/IGlobalCacheService.cs | 38 ++++++ .../Services/TenantThemeService.cs | 4 +- .../CachingArchitectureTests.cs | 116 +++++++++++------- 4 files changed, 159 insertions(+), 45 deletions(-) create mode 100644 src/BuildingBlocks/Caching/GlobalHybridCache.cs create mode 100644 src/BuildingBlocks/Caching/IGlobalCacheService.cs diff --git a/src/BuildingBlocks/Caching/GlobalHybridCache.cs b/src/BuildingBlocks/Caching/GlobalHybridCache.cs new file mode 100644 index 0000000000..781aa423a0 --- /dev/null +++ b/src/BuildingBlocks/Caching/GlobalHybridCache.cs @@ -0,0 +1,46 @@ +using Microsoft.Extensions.Caching.Hybrid; + +namespace FSH.Framework.Caching; + +/// +/// implementation that delegates directly to +/// without any tenant scoping. +/// Registered as Singleton — safe because no per-request state is involved. +/// +internal sealed class GlobalHybridCache : IGlobalCacheService +{ + private readonly HybridCache _cache; + + public GlobalHybridCache(HybridCache cache) + { + ArgumentNullException.ThrowIfNull(cache); + _cache = cache; + } + + /// + public ValueTask GetOrCreateAsync( + string key, + TState state, + Func> factory, + HybridCacheEntryOptions? options = null, + IEnumerable? tags = null, + CancellationToken cancellationToken = default) + => _cache.GetOrCreateAsync(key, state, factory, options, tags, cancellationToken); + + /// + public ValueTask SetAsync( + string key, + T value, + HybridCacheEntryOptions? options = null, + IEnumerable? tags = null, + CancellationToken cancellationToken = default) + => _cache.SetAsync(key, value, options, tags, cancellationToken); + + /// + public ValueTask RemoveAsync(string key, CancellationToken cancellationToken = default) + => _cache.RemoveAsync(key, cancellationToken); + + /// + public ValueTask RemoveByTagAsync(string tag, CancellationToken cancellationToken = default) + => _cache.RemoveByTagAsync(tag, cancellationToken); +} diff --git a/src/BuildingBlocks/Caching/IGlobalCacheService.cs b/src/BuildingBlocks/Caching/IGlobalCacheService.cs new file mode 100644 index 0000000000..a7df2671b1 --- /dev/null +++ b/src/BuildingBlocks/Caching/IGlobalCacheService.cs @@ -0,0 +1,38 @@ +using Microsoft.Extensions.Caching.Hybrid; + +namespace FSH.Framework.Caching; + +/// +/// Cross-tenant cache service for entries that are intentionally shared across all tenants. +/// Examples: system defaults, global configuration, shared lookup tables. +/// +/// +/// Use this service ONLY for data that is genuinely identical for every tenant. +/// For any per-tenant data use instead. +/// Registered as Singleton because there is no per-request tenant context involved. +/// +public interface IGlobalCacheService +{ + /// Gets or creates a cross-tenant cache entry. + ValueTask GetOrCreateAsync( + string key, + TState state, + Func> factory, + HybridCacheEntryOptions? options = null, + IEnumerable? tags = null, + CancellationToken cancellationToken = default); + + /// Sets a cross-tenant cache entry. + ValueTask SetAsync( + string key, + T value, + HybridCacheEntryOptions? options = null, + IEnumerable? tags = null, + CancellationToken cancellationToken = default); + + /// Removes a cross-tenant cache entry by key. + ValueTask RemoveAsync(string key, CancellationToken cancellationToken = default); + + /// Removes all cross-tenant entries tagged with the given tag. + ValueTask RemoveByTagAsync(string tag, CancellationToken cancellationToken = default); +} diff --git a/src/Modules/Multitenancy/Modules.Multitenancy/Services/TenantThemeService.cs b/src/Modules/Multitenancy/Modules.Multitenancy/Services/TenantThemeService.cs index 3fab220981..041538f26d 100644 --- a/src/Modules/Multitenancy/Modules.Multitenancy/Services/TenantThemeService.cs +++ b/src/Modules/Multitenancy/Modules.Multitenancy/Services/TenantThemeService.cs @@ -27,7 +27,7 @@ public sealed class TenantThemeService : ITenantThemeService private static readonly string[] DefaultThemeTags = [CacheKeys.Tags.Themes]; private readonly ITenantCacheService _cache; - private readonly HybridCache _globalCache; // for cross-tenant entries like DefaultTheme + private readonly IGlobalCacheService _globalCache; // for cross-tenant entries like DefaultTheme private readonly TenantDbContext _dbContext; private readonly IMultiTenantContextAccessor _tenantAccessor; private readonly IStorageService _storageService; @@ -36,7 +36,7 @@ public sealed class TenantThemeService : ITenantThemeService public TenantThemeService( ITenantCacheService cache, - HybridCache globalCache, + IGlobalCacheService globalCache, TenantDbContext dbContext, IMultiTenantContextAccessor tenantAccessor, IStorageService storageService, diff --git a/src/Tests/Architecture.Tests/CachingArchitectureTests.cs b/src/Tests/Architecture.Tests/CachingArchitectureTests.cs index a4379cfa71..99ac7ca43e 100644 --- a/src/Tests/Architecture.Tests/CachingArchitectureTests.cs +++ b/src/Tests/Architecture.Tests/CachingArchitectureTests.cs @@ -1,7 +1,8 @@ using FSH.Framework.Caching; -using Microsoft.Extensions.Caching.Hybrid; using NetArchTest.Rules; +using Shouldly; using System.Reflection; +using Xunit; namespace Architecture.Tests; @@ -11,9 +12,12 @@ namespace Architecture.Tests; /// directly into business module code, which would bypass the automatic per-tenant /// key scoping provided by . /// -/// The rule: module assemblies must inject , -/// not directly. is allowed only in -/// BuildingBlocks (the implementation layer) and in designated cross-tenant infrastructure. +/// The two-cache pattern: +/// +/// — per-tenant scoped, for all business data. +/// — cross-tenant singleton, for shared system defaults. +/// +/// Neither role requires module code to inject HybridCache directly. /// public sealed class CachingArchitectureTests { @@ -25,55 +29,56 @@ public sealed class CachingArchitectureTests ModuleAssemblyDiscovery.GetModuleAssemblies(); // ------------------------------------------------------------------------- - // Rule 1 — No direct HybridCache injection in module constructors + // Rule 1 — No direct HybridCache dependency in any module class // ------------------------------------------------------------------------- /// - /// Verifies that no class in any business module has a constructor parameter - /// of type . All module code must inject - /// so tenant isolation is enforced structurally. + /// Verifies that no class in any business module injects directly. + /// All module code must inject either (tenant-scoped data) + /// or (intentionally cross-tenant data). + /// + /// Note: HybridCacheEntryOptions (a pure data/options type from the same namespace) + /// is intentionally allowed — it carries no state and is safe to use as a method argument. + /// We check for the cache service class by its full name, not the namespace. + /// /// [Fact] public void ModuleClasses_Should_Not_Depend_On_HybridCache_Directly() { - // NetArchTest detects HybridCache as a dependency via the assembly's metadata. - // A constructor injection of HybridCache creates a dependency on its declaring - // namespace: Microsoft.Extensions.Caching.Hybrid. + // We check by fully-qualified class name rather than namespace because: + // - HybridCache (the abstract class) is what we want to ban from constructors. + // - HybridCacheEntryOptions (a data record in the same namespace) is legitimately + // used in module code as options arguments to ITenantCacheService / IGlobalCacheService. + const string hybridCacheClass = "Microsoft.Extensions.Caching.Hybrid.HybridCache"; + foreach (var assembly in ModuleAssemblies) { var result = Types .InAssembly(assembly) .ShouldNot() - .HaveDependencyOn("Microsoft.Extensions.Caching.Hybrid") + .HaveDependencyOn(hybridCacheClass) .GetResult(); var failingTypes = result.FailingTypeNames ?? []; - // TenantThemeService is a known, intentional exception: - // it holds a secondary HybridCache reference for the cross-tenant DefaultTheme entry. - // All other violations are real bugs. - var realViolations = failingTypes - .Where(t => !t.EndsWith("TenantThemeService", StringComparison.Ordinal)) - .ToList(); - - realViolations.ShouldBeEmpty( - $"Module '{assembly.GetName().Name}' contains types that depend directly on " + - $"HybridCache. Use ITenantCacheService instead to guarantee per-tenant key scoping. " + - $"Violating types: {string.Join(", ", realViolations)}"); + failingTypes.ShouldBeEmpty( + $"Module '{assembly.GetName().Name}' contains types that inject HybridCache directly. " + + $"Use ITenantCacheService for per-tenant data or IGlobalCacheService for " + + $"cross-tenant shared data. Violating types: {string.Join(", ", failingTypes)}"); } } // ------------------------------------------------------------------------- - // Rule 2 — ITenantCacheService is registered (DI contract) + // Rule 2 — ITenantCacheService is a public interface (DI contract) // ------------------------------------------------------------------------- /// - /// Verifies that the type is publicly accessible - /// from the Caching assembly. If someone accidentally changes its visibility, - /// module code would break at DI resolution time rather than compile time. + /// Verifies that is a public interface so that + /// module assemblies can depend on it. If someone accidentally changes its + /// visibility, module code would break at DI resolution time rather than compile time. /// [Fact] - public void ITenantCacheService_Should_Be_Public() + public void ITenantCacheService_Should_Be_A_Public_Interface() { var type = typeof(ITenantCacheService); @@ -85,37 +90,62 @@ public void ITenantCacheService_Should_Be_Public() } // ------------------------------------------------------------------------- - // Rule 3 — TenantHybridCache implementation remains internal (encapsulation) + // Rule 3 — IGlobalCacheService is a public interface (DI contract) + // ------------------------------------------------------------------------- + + /// + /// Verifies that is a public interface, enabling + /// modules that legitimately need cross-tenant caching (e.g. system defaults) + /// to depend on it explicitly and intentionally. + /// + [Fact] + public void IGlobalCacheService_Should_Be_A_Public_Interface() + { + var type = typeof(IGlobalCacheService); + + type.IsInterface.ShouldBeTrue( + $"{type.FullName} must be an interface so modules can depend on the abstraction."); + + type.IsPublic.ShouldBeTrue( + $"{type.FullName} must be public so module assemblies can reference it."); + } + + // ------------------------------------------------------------------------- + // Rule 4 — Implementations stay internal (prevent module bypass) // ------------------------------------------------------------------------- /// - /// Verifies that the concrete TenantHybridCache implementation is internal, - /// preventing modules from bypassing the interface and newing it up directly. + /// Verifies that the concrete implementations (TenantHybridCache and + /// GlobalHybridCache) are internal, preventing modules from + /// newing them up directly and bypassing the DI contract. /// [Fact] - public void TenantHybridCache_Implementation_Should_Be_Internal() + public void CacheService_Implementations_Should_Be_Internal() { var cachingAssembly = typeof(ITenantCacheService).Assembly; - var implType = cachingAssembly - .GetTypes() - .FirstOrDefault(t => t.Name == "TenantHybridCache"); + foreach (var implName in new[] { "TenantHybridCache", "GlobalHybridCache" }) + { + var implType = cachingAssembly + .GetTypes() + .FirstOrDefault(t => t.Name == implName); - implType.ShouldNotBeNull( - "TenantHybridCache implementation type must exist in the Caching assembly."); + implType.ShouldNotBeNull( + $"{implName} implementation must exist in the Caching assembly."); - implType!.IsPublic.ShouldBeFalse( - "TenantHybridCache should be internal so module code cannot instantiate it directly. " + - "All consumption must go through ITenantCacheService resolved via DI."); + implType!.IsPublic.ShouldBeFalse( + $"{implName} should be internal so module code cannot instantiate it directly. " + + $"All consumption must go through the interface resolved via DI."); + } } // ------------------------------------------------------------------------- - // Rule 4 — Guard: at least one module was scanned + // Rule 5 — Guard: at least one module was scanned (prevents vacuous pass) // ------------------------------------------------------------------------- /// - /// Prevents the tenant isolation check from silently passing with an empty assembly list - /// (which would make rules 1 vacuously true and miss real violations in new modules). + /// Prevents Rule 1 from silently passing with an empty assembly list, + /// which would make it vacuously true and miss real violations in new modules. /// [Fact] public void CachingArchitectureTests_Should_HaveAtLeastOneModuleToScan() From 493fefe9f8870dbb4a55053fae7abbb670b2aa77 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=A9sar=20Castro?= Date: Sun, 17 May 2026 21:41:25 +0200 Subject: [PATCH 4/7] refactor(caching): standardize Web Idempotency caching to use ITenantCacheService - Migrated IdempotencyEndpointFilter from direct HybridCache usage to ITenantCacheService - Updated CacheKeys to expose logical IdempotencyEntry and full IdempotencyEntryFull (for distributed cache read probe) - Added using directive to CachingArchitectureTests.cs to fix XML doc cref reference compilation issue - Updated Caching.Tests (CacheKeysTests and HeroCachingRegistrationTests) to match new signatures and test registration --- src/BuildingBlocks/Caching/CacheKeys.cs | 16 ++++++++++---- .../Idempotency/IdempotencyEndpointFilter.cs | 22 ++++++++++++------- .../CachingArchitectureTests.cs | 1 + src/Tests/Caching.Tests/CacheKeysTests.cs | 10 +++++++-- .../HeroCachingRegistrationTests.cs | 19 +++++++++++++++- 5 files changed, 53 insertions(+), 15 deletions(-) diff --git a/src/BuildingBlocks/Caching/CacheKeys.cs b/src/BuildingBlocks/Caching/CacheKeys.cs index 20e732e4e1..1687704127 100644 --- a/src/BuildingBlocks/Caching/CacheKeys.cs +++ b/src/BuildingBlocks/Caching/CacheKeys.cs @@ -2,8 +2,9 @@ namespace FSH.Framework.Caching; /// /// Cache key conventions and tag constants used across the FullStackHero starter kit. -/// Keys should be tenant-scoped where applicable; tags enable bulk invalidation via -/// . +/// Tenant scoping is applied automatically by — +/// keys returned by these methods are the logical (un-prefixed) key. +/// Tags enable bulk invalidation via HybridCache's tag system. /// public static class CacheKeys { @@ -35,8 +36,15 @@ public static class Tags /// Key for the system-wide default theme. public const string DefaultTheme = "theme:default"; - /// Key for an idempotency replay entry, scoped by tenant. - public static string IdempotencyEntry(string tenantId, string key) => $"idem:t:{tenantId}:{key}"; + /// Logical key for an idempotency replay entry (no tenant prefix — applied by ITenantCacheService). + public static string IdempotencyEntry(string key) => $"idem:{key}"; + + /// + /// Fully-qualified idempotency key including the tenant segment, used when reading + /// directly from + /// to match the key written by (which uses prefix t:{tenantId}:). + /// + public static string IdempotencyEntryFull(string tenantId, string key) => $"t:{tenantId}:idem:{key}"; /// /// Key for the impersonation-grant revocation marker, indexed by JWT id. diff --git a/src/BuildingBlocks/Web/Idempotency/IdempotencyEndpointFilter.cs b/src/BuildingBlocks/Web/Idempotency/IdempotencyEndpointFilter.cs index 0a79d00b2f..a46be8e814 100644 --- a/src/BuildingBlocks/Web/Idempotency/IdempotencyEndpointFilter.cs +++ b/src/BuildingBlocks/Web/Idempotency/IdempotencyEndpointFilter.cs @@ -19,8 +19,11 @@ namespace FSH.Framework.Web.Idempotency; /// /// /// Uses directly for the probe read (bypassing -/// 's factory-mandatory API) and -/// for the write path so replays benefit from L1 and the regular tag invalidation story. +/// 's factory-mandatory API) and +/// for the write path so replays +/// benefit from L1 and the regular tag invalidation story. Tenant scoping is +/// applied automatically by — no manual key +/// prefixing required in the write path. /// Using HybridCache with DisableUnderlyingData as a "get-only probe" is a /// known anti-pattern tracked at dotnet/aspnetcore#57191. /// @@ -49,18 +52,20 @@ public sealed class IdempotencyEndpointFilter : IEndpointFilter } var distributedCache = httpContext.RequestServices.GetRequiredService(); - var hybridCache = httpContext.RequestServices.GetRequiredService(); + var tenantCache = httpContext.RequestServices.GetRequiredService(); var logger = httpContext.RequestServices.GetRequiredService>(); - // Include tenant context in cache key for isolation + // ITenantCacheService prefixes keys with t:{tenantId}: automatically. + // For the IDistributedCache probe-read we reconstruct the full key to match. var tenantId = httpContext.User.FindFirst("tenant")?.Value ?? "global"; - var cacheKey = CacheKeys.IdempotencyEntry(tenantId, idempotencyKey); - var tags = new[] { CacheKeys.Tags.Idempotency, CacheKeys.Tags.Tenant(tenantId) }; + var logicalKey = CacheKeys.IdempotencyEntry(idempotencyKey); + var fullKey = CacheKeys.IdempotencyEntryFull(tenantId, idempotencyKey); + var tags = new[] { CacheKeys.Tags.Idempotency }; // tenant tag injected automatically by ITenantCacheService // Probe-only read: IDistributedCache has a real GetAsync that returns null on miss, // unlike HybridCache which requires a factory. We bypass L1 here because idempotency // replays are rare relative to first-calls and L1 warmth has little value. - var cachedBytes = await distributedCache.GetAsync(cacheKey, httpContext.RequestAborted).ConfigureAwait(false); + var cachedBytes = await distributedCache.GetAsync(fullKey, httpContext.RequestAborted).ConfigureAwait(false); if (cachedBytes is not null && cachedBytes.Length > 0) { var cached = JsonSerializer.Deserialize(cachedBytes, JsonOpts); @@ -105,7 +110,8 @@ public sealed class IdempotencyEndpointFilter : IEndpointFilter Expiration = options.DefaultTtl, LocalCacheExpiration = options.DefaultTtl < TimeSpan.FromMinutes(2) ? options.DefaultTtl : TimeSpan.FromMinutes(2), }; - await hybridCache.SetAsync(cacheKey, responseToCache, setOptions, tags, httpContext.RequestAborted).ConfigureAwait(false); + // ITenantCacheService.SetAsync prefixes the logical key and adds the tenant tag automatically. + await tenantCache.SetAsync(logicalKey, responseToCache, setOptions, tags, httpContext.RequestAborted).ConfigureAwait(false); } // Best-effort caching: idempotency replay is a convenience, not a correctness requirement catch (Exception ex) when (ex is not OperationCanceledException) diff --git a/src/Tests/Architecture.Tests/CachingArchitectureTests.cs b/src/Tests/Architecture.Tests/CachingArchitectureTests.cs index 99ac7ca43e..3a047b7021 100644 --- a/src/Tests/Architecture.Tests/CachingArchitectureTests.cs +++ b/src/Tests/Architecture.Tests/CachingArchitectureTests.cs @@ -1,4 +1,5 @@ using FSH.Framework.Caching; +using Microsoft.Extensions.Caching.Hybrid; using NetArchTest.Rules; using Shouldly; using System.Reflection; diff --git a/src/Tests/Caching.Tests/CacheKeysTests.cs b/src/Tests/Caching.Tests/CacheKeysTests.cs index 03d346e601..de71acd3ab 100644 --- a/src/Tests/Caching.Tests/CacheKeysTests.cs +++ b/src/Tests/Caching.Tests/CacheKeysTests.cs @@ -27,9 +27,15 @@ public void DefaultTheme_Should_BeConstant() } [Fact] - public void IdempotencyEntry_Should_ScopeByTenant() + public void IdempotencyEntry_Should_ReturnLogicalKey() { - CacheKeys.IdempotencyEntry("t1", "req-42").ShouldBe("idem:t:t1:req-42"); + CacheKeys.IdempotencyEntry("req-42").ShouldBe("idem:req-42"); + } + + [Fact] + public void IdempotencyEntryFull_Should_ScopeByTenant() + { + CacheKeys.IdempotencyEntryFull("t1", "req-42").ShouldBe("t:t1:idem:req-42"); } [Fact] diff --git a/src/Tests/Caching.Tests/HeroCachingRegistrationTests.cs b/src/Tests/Caching.Tests/HeroCachingRegistrationTests.cs index f227eb5d7c..f502b8efcd 100644 --- a/src/Tests/Caching.Tests/HeroCachingRegistrationTests.cs +++ b/src/Tests/Caching.Tests/HeroCachingRegistrationTests.cs @@ -75,9 +75,26 @@ public void AddHeroCaching_Should_RegisterITenantCacheService() // Act services.AddHeroCaching(config); - // Assert — ITenantCacheService must be registered (descriptor exists) + // Assert — ITenantCacheService must be registered as Scoped (per-request tenant context) var descriptor = services.FirstOrDefault(d => d.ServiceType == typeof(ITenantCacheService)); descriptor.ShouldNotBeNull(); descriptor.Lifetime.ShouldBe(ServiceLifetime.Scoped); } + + [Fact] + public void AddHeroCaching_Should_RegisterIGlobalCacheService_AsSingleton() + { + // Arrange + var services = new ServiceCollection(); + var config = new ConfigurationBuilder().Build(); + + // Act + services.AddHeroCaching(config); + + // Assert — IGlobalCacheService must be registered as Singleton (no per-request state) + var descriptor = services.FirstOrDefault(d => d.ServiceType == typeof(IGlobalCacheService)); + descriptor.ShouldNotBeNull("IGlobalCacheService should be registered by AddHeroCaching"); + descriptor.Lifetime.ShouldBe(ServiceLifetime.Singleton, + "IGlobalCacheService carries no per-request state and should live as Singleton"); + } } From 26984fdc1b8853a0919743469fdcb3557c7e975b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=A9sar=20Castro?= Date: Tue, 19 May 2026 23:44:05 +0200 Subject: [PATCH 5/7] fix(caching): correct tag-format mismatch in TenantHybridCache.ScopeTags MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Blocker 1 from PR #1243 review. ScopeTags() was tagging entries with bare caller tags (e.g. 'themes') but RemoveByTagAsync was looking up 't:{tenantId}:{tag}' — these never matched, making all tag-based invalidation a silent no-op. Fix: prefix every caller tag with 't:{tenantId}:' in ScopeTags() so both the SET and REMOVE paths agree on the tag format. The whole-tenant purge tag 'tenant:{tenantId}' is retained unchanged. Concrete effect: TenantThemeService.InvalidateCacheAsync('themes') and RolePermissionSyncer.RemoveByTagAsync('permissions') now actually evict the intended cache entries. Add two round-trip regression tests (single-tag and multi-tag overload) that would have caught this bug: set with tag -> remove by tag -> re-read must miss (factory called again). Co-authored-by: PR reviewer feedback --- .../Caching/TenantHybridCache.cs | 18 +++- .../Caching.Tests/TenantCacheServiceTests.cs | 83 +++++++++++++++++++ 2 files changed, 97 insertions(+), 4 deletions(-) diff --git a/src/BuildingBlocks/Caching/TenantHybridCache.cs b/src/BuildingBlocks/Caching/TenantHybridCache.cs index 2abcbc2941..9898f21a15 100644 --- a/src/BuildingBlocks/Caching/TenantHybridCache.cs +++ b/src/BuildingBlocks/Caching/TenantHybridCache.cs @@ -54,16 +54,26 @@ private string GetTenantId() private string ScopeKey(string key) => $"t:{GetTenantId()}:{key}"; /// - /// Prepends a per-tenant tag (tenant:{tenantId}) to any caller-supplied tags. - /// This guarantees that a on the - /// tag correctly purges all entries for a tenant. + /// Returns the full tag set for a cache entry: + /// + /// tenant:{tenantId} — whole-tenant purge tag (used by ). + /// t:{tenantId}:{callerTag} for every caller-supplied tag — scoped so + /// can look up the same + /// prefixed tag and actually find the stored entries. + /// + /// Without the per-tag prefix the SET and REMOVE paths would use different tag strings, + /// making all tag-based invalidation a silent no-op. /// private static IEnumerable ScopeTags(string tenantId, IEnumerable? callerTags) { + // Whole-tenant purge tag — lets callers blow away an entire tenant's cache. yield return CacheKeys.Tags.Tenant(tenantId); if (callerTags is null) yield break; - foreach (var tag in callerTags) yield return tag; + + // Per-tag scoped form — must match the lookup key built in RemoveByTagAsync. + foreach (var tag in callerTags) + yield return $"t:{tenantId}:{tag}"; } // ----------------------------------------------------------------------- diff --git a/src/Tests/Caching.Tests/TenantCacheServiceTests.cs b/src/Tests/Caching.Tests/TenantCacheServiceTests.cs index 9b9695b9e7..f21aeef86f 100644 --- a/src/Tests/Caching.Tests/TenantCacheServiceTests.cs +++ b/src/Tests/Caching.Tests/TenantCacheServiceTests.cs @@ -130,4 +130,87 @@ await Should.ThrowAsync( () => sut.GetOrCreateAsync("key", "state", (_, _) => ValueTask.FromResult("v")).AsTask()); } + + /// + /// Regression test for the tag-format mismatch bug: entries tagged with a logical + /// tag (e.g. "themes") must be evictable via + /// using the same logical tag string. Without the t:{tenantId}: prefix applied + /// consistently on both SET and REMOVE paths, this round-trip silently fails. + /// + [Fact] + public async Task RemoveByTagAsync_RoundTrip_Should_EvictTaggedEntry() + { + // Arrange + var (sut, _) = CreateSut("tenant-tag-rt"); + int factoryCallCount = 0; + + // Prime the cache — entry is stored with tag "my-tag" + await sut.GetOrCreateAsync( + "tagged-key", + "state", + (_, _) => + { + factoryCallCount++; + return ValueTask.FromResult("original-value"); + }, + tags: ["my-tag"]); + + factoryCallCount.ShouldBe(1, "factory should have run once to prime the cache"); + + // Act — evict by the same logical tag string + await sut.RemoveByTagAsync("my-tag"); + + // Re-read: entry must be gone, so factory runs again + var result = await sut.GetOrCreateAsync( + "tagged-key", + "state", + (_, _) => + { + factoryCallCount++; + return ValueTask.FromResult("refreshed-value"); + }, + tags: ["my-tag"]); + + // Assert + factoryCallCount.ShouldBe(2, "factory must run a second time — RemoveByTagAsync should have evicted the entry"); + result.ShouldBe("refreshed-value"); + } + + /// + /// Same round-trip guarantee for the multi-tag overload. + /// + [Fact] + public async Task RemoveByTagAsync_MultiTag_RoundTrip_Should_EvictTaggedEntry() + { + // Arrange + var (sut, _) = CreateSut("tenant-multitag-rt"); + int factoryCallCount = 0; + + await sut.GetOrCreateAsync( + "multi-tagged-key", + "state", + (_, _) => + { + factoryCallCount++; + return ValueTask.FromResult("initial"); + }, + tags: ["tag-a", "tag-b"]); + + factoryCallCount.ShouldBe(1); + + // Act — evict via the multi-tag overload using one of the stored tags + await sut.RemoveByTagAsync(["tag-a", "tag-b"]); + + // Assert — must miss + await sut.GetOrCreateAsync( + "multi-tagged-key", + "state", + (_, _) => + { + factoryCallCount++; + return ValueTask.FromResult("re-fetched"); + }); + + factoryCallCount.ShouldBe(2, "multi-tag removal must evict the entry"); + } } From 26ad1c7e25386569b40b5830c8559da80134b05b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=A9sar=20Castro?= Date: Tue, 19 May 2026 23:44:15 +0200 Subject: [PATCH 6/7] fix(idempotency): resolve tenant from IMultiTenantContextAccessor in both paths Non-blocker from PR #1243 review. IdempotencyEndpointFilter was reading tenantId from the JWT claim ('tenant') for the probe-read key construction, but writing via ITenantCacheService which internally uses IMultiTenantContextAccessor. During root-operator impersonation the claim says 'root' while the resolved tenant is the override target, so the probe-read key and write key would diverge and idempotency replay would never match. Fix: resolve tenantId from IMultiTenantContextAccessor in both paths. --- .../Web/Idempotency/IdempotencyEndpointFilter.cs | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/src/BuildingBlocks/Web/Idempotency/IdempotencyEndpointFilter.cs b/src/BuildingBlocks/Web/Idempotency/IdempotencyEndpointFilter.cs index a46be8e814..95392f1a97 100644 --- a/src/BuildingBlocks/Web/Idempotency/IdempotencyEndpointFilter.cs +++ b/src/BuildingBlocks/Web/Idempotency/IdempotencyEndpointFilter.cs @@ -1,6 +1,7 @@ using System.Security.Cryptography; using System.Text; using System.Text.Json; +using Finbuckle.MultiTenant.Abstractions; using FSH.Framework.Caching; using Microsoft.AspNetCore.Builder; using Microsoft.AspNetCore.Http; @@ -24,6 +25,10 @@ namespace FSH.Framework.Web.Idempotency; /// benefit from L1 and the regular tag invalidation story. Tenant scoping is /// applied automatically by — no manual key /// prefixing required in the write path. +/// Both the probe-read key construction and the write path resolve the tenant +/// from so they agree even during +/// root-operator impersonation, where the JWT claim may differ from the +/// middleware-resolved tenant. /// Using HybridCache with DisableUnderlyingData as a "get-only probe" is a /// known anti-pattern tracked at dotnet/aspnetcore#57191. /// @@ -54,10 +59,12 @@ public sealed class IdempotencyEndpointFilter : IEndpointFilter var distributedCache = httpContext.RequestServices.GetRequiredService(); var tenantCache = httpContext.RequestServices.GetRequiredService(); var logger = httpContext.RequestServices.GetRequiredService>(); + var tenantAccessor = httpContext.RequestServices.GetRequiredService(); - // ITenantCacheService prefixes keys with t:{tenantId}: automatically. - // For the IDistributedCache probe-read we reconstruct the full key to match. - var tenantId = httpContext.User.FindFirst("tenant")?.Value ?? "global"; + // Resolve tenant from IMultiTenantContextAccessor — must match ITenantCacheService which + // also uses the accessor internally. Using the JWT claim would diverge from the write path + // during root-operator impersonation (claim = "root", resolved tenant = override target). + var tenantId = tenantAccessor.MultiTenantContext?.TenantInfo?.Id ?? "global"; var logicalKey = CacheKeys.IdempotencyEntry(idempotencyKey); var fullKey = CacheKeys.IdempotencyEntryFull(tenantId, idempotencyKey); var tags = new[] { CacheKeys.Tags.Idempotency }; // tenant tag injected automatically by ITenantCacheService From 19cb6966237609f29e5b77d284d8ac75eed26511 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=A9sar=20Castro?= Date: Wed, 20 May 2026 00:21:52 +0200 Subject: [PATCH 7/7] fix(multitenancy): resolve CI test failures - add adminPassword to TenantCreationTests and TenantActivationTests payloads to match recent upstream validation changes - revert UserPermissionService and RolePermissionSyncer to IGlobalCacheService to allow root operators to resolve globally cached permissions when scoping to another tenant via header override --- .../Modules.Identity/Authorization/RolePermissionSyncer.cs | 2 +- .../Modules.Identity/Services/UserPermissionService.cs | 2 +- .../Tests/Multitenancy/TenantActivationTests.cs | 2 ++ .../Tests/Multitenancy/TenantCreationTests.cs | 3 +++ 4 files changed, 7 insertions(+), 2 deletions(-) diff --git a/src/Modules/Identity/Modules.Identity/Authorization/RolePermissionSyncer.cs b/src/Modules/Identity/Modules.Identity/Authorization/RolePermissionSyncer.cs index 290599c3c9..3ab8be20cf 100644 --- a/src/Modules/Identity/Modules.Identity/Authorization/RolePermissionSyncer.cs +++ b/src/Modules/Identity/Modules.Identity/Authorization/RolePermissionSyncer.cs @@ -20,7 +20,7 @@ public sealed class RolePermissionSyncer( IdentityDbContext context, RoleManager roleManager, IMultiTenantContextAccessor tenantAccessor, - ITenantCacheService cache, + IGlobalCacheService cache, TimeProvider timeProvider, ILogger logger) { diff --git a/src/Modules/Identity/Modules.Identity/Services/UserPermissionService.cs b/src/Modules/Identity/Modules.Identity/Services/UserPermissionService.cs index 7d5999c27c..8bce7a97b4 100644 --- a/src/Modules/Identity/Modules.Identity/Services/UserPermissionService.cs +++ b/src/Modules/Identity/Modules.Identity/Services/UserPermissionService.cs @@ -15,7 +15,7 @@ internal sealed class UserPermissionService( UserManager userManager, RoleManager roleManager, IdentityDbContext db, - ITenantCacheService cache) : IUserPermissionService + IGlobalCacheService cache) : 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. diff --git a/src/Tests/Integration.Tests/Tests/Multitenancy/TenantActivationTests.cs b/src/Tests/Integration.Tests/Tests/Multitenancy/TenantActivationTests.cs index e4d4448b6d..b919d54daf 100644 --- a/src/Tests/Integration.Tests/Tests/Multitenancy/TenantActivationTests.cs +++ b/src/Tests/Integration.Tests/Tests/Multitenancy/TenantActivationTests.cs @@ -25,6 +25,7 @@ public async Task ChangeTenantActivation_Should_ReturnOk_When_DeactivatingTenant name = $"Deactivate Tenant {uniqueId}", connectionString = (string?)null, adminEmail = $"deact-{uniqueId}@tenant.com", + adminPassword = TestConstants.DefaultPassword, issuer = "deact.issuer" }); createResponse.StatusCode.ShouldBe(HttpStatusCode.Created); @@ -50,6 +51,7 @@ public async Task ChangeTenantActivation_Should_DeactivateAndReactivate_When_Ten name = $"Toggle Tenant {uniqueId}", connectionString = (string?)null, adminEmail = $"toggle-{uniqueId}@tenant.com", + adminPassword = TestConstants.DefaultPassword, issuer = "toggle.issuer" }); diff --git a/src/Tests/Integration.Tests/Tests/Multitenancy/TenantCreationTests.cs b/src/Tests/Integration.Tests/Tests/Multitenancy/TenantCreationTests.cs index 37b8bf8dad..5a47a1cce9 100644 --- a/src/Tests/Integration.Tests/Tests/Multitenancy/TenantCreationTests.cs +++ b/src/Tests/Integration.Tests/Tests/Multitenancy/TenantCreationTests.cs @@ -28,6 +28,7 @@ public async Task CreateTenant_Should_Return201WithId_When_DataIsValid() name = $"Test Tenant {uniqueId}", connectionString = (string?)null, adminEmail = $"admin-{uniqueId}@tenant.com", + adminPassword = TestConstants.DefaultPassword, issuer = "test.issuer" }); @@ -47,6 +48,7 @@ public async Task CreateTenant_Should_Reject_When_IdAlreadyExists() name = $"Dup Tenant {uniqueId}", connectionString = (string?)null, adminEmail = $"dupadmin-{uniqueId}@tenant.com", + adminPassword = TestConstants.DefaultPassword, issuer = "dup.issuer" }; @@ -70,6 +72,7 @@ public async Task CreateTenant_Should_Return401_When_NotAuthenticated() name = "No Auth Tenant", connectionString = (string?)null, adminEmail = "noauth@tenant.com", + adminPassword = TestConstants.DefaultPassword, issuer = "noauth.issuer" });