From 21e12b3fbdf9b1d8802ce9ac57a0f2c92cd7c9f3 Mon Sep 17 00:00:00 2001 From: Sipke Schoorstra Date: Sun, 13 Sep 2026 14:27:51 +0200 Subject: [PATCH] fix(identity): honor tenant isolation in Memory user and application stores (#8107) * fix(identity): honor tenant isolation in Memory user and application stores Align MemoryUserStore and MemoryApplicationStore with EF SetTenantIdFilter via TenantVisibility: null TenantId is default-tenant-only, and applications filter by ambient tenant. Stamp missing TenantId on write like ApplyTenantId. Co-authored-by: Sipke Schoorstra * fix(identity): delete memory users and applications under tenant lock DeleteAsync now removes under MemoryStore.Sync with TenantVisibility in the same predicate, so a same-Id replacement from another tenant cannot be wiped. Co-authored-by: Sipke Schoorstra --------- Co-authored-by: Cursor Agent --- .../Services/MemoryApplicationStore.cs | 40 ++++- .../Elsa.Identity/Services/MemoryUserStore.cs | 42 +++-- .../Services/DefaultSecretHasherTests.cs | 2 +- ...oryApplicationStoreTenantIsolationTests.cs | 145 +++++++++++++++++ .../MemoryUserStoreTenantIsolationTests.cs | 148 ++++++++++++++++++ 5 files changed, 354 insertions(+), 23 deletions(-) create mode 100644 test/unit/Elsa.Identity.UnitTests/Services/MemoryApplicationStoreTenantIsolationTests.cs create mode 100644 test/unit/Elsa.Identity.UnitTests/Services/MemoryUserStoreTenantIsolationTests.cs diff --git a/src/modules/Elsa.Identity/Services/MemoryApplicationStore.cs b/src/modules/Elsa.Identity/Services/MemoryApplicationStore.cs index 5c1ad691f..2c7ce54c1 100644 --- a/src/modules/Elsa.Identity/Services/MemoryApplicationStore.cs +++ b/src/modules/Elsa.Identity/Services/MemoryApplicationStore.cs @@ -1,3 +1,5 @@ +using Elsa.Common.Entities; +using Elsa.Common.Multitenancy; using Elsa.Common.Services; using Elsa.Identity.Contracts; using Elsa.Identity.Entities; @@ -8,30 +10,43 @@ namespace Elsa.Identity.Services; /// /// Represents an in-memory application store. /// +/// +/// Ambient tenant is applied here rather than in callers. +/// EF owns that via SetTenantIdFilter / ApplyTenantId; Memory must compensate. +/// Application contracts have no TenantAgnostic flag, so isolation always applies (EF query filter). +/// public class MemoryApplicationStore : IApplicationStore { private readonly MemoryStore _store; + private readonly ITenantAccessor _tenantAccessor; /// /// Initializes a new instance of the class. /// - public MemoryApplicationStore(MemoryStore store) + public MemoryApplicationStore(MemoryStore store, ITenantAccessor tenantAccessor) { _store = store; + _tenantAccessor = tenantAccessor; } /// public Task SaveAsync(Application application, CancellationToken cancellationToken = default) { - _store.Save(application, x => x.Id); + ApplyCurrentTenant(application); + lock (_store.Sync) + _store.Save(application, x => x.Id); return Task.CompletedTask; } /// public Task DeleteAsync(ApplicationFilter filter, CancellationToken cancellationToken = default) { - var ids = _store.Query(query => Filter(query, filter)).Select(x => x.Id).Distinct().ToList(); - _store.DeleteWhere(x => ids.Contains(x.Id)); + lock (_store.Sync) + { + _store.DeleteWhere(application => + IsVisible(application) && filter.Apply(new[] { application }.AsQueryable()).Any()); + } + return Task.CompletedTask; } @@ -41,6 +56,17 @@ public class MemoryApplicationStore : IApplicationStore var result = _store.Query(query => Filter(query, filter)).FirstOrDefault(); return Task.FromResult(result); } - - private IQueryable Filter(IQueryable queryable, ApplicationFilter filter) => filter.Apply(queryable); -} \ No newline at end of file + + private IQueryable Filter(IQueryable queryable, ApplicationFilter filter) => + filter.Apply(queryable.WhereVisibleToTenant(_tenantAccessor.TenantId)); + + private bool IsVisible(Entity entity) => TenantVisibility.IsVisible(entity.TenantId, _tenantAccessor.TenantId); + + private void ApplyCurrentTenant(Entity entity) + { + if (entity.TenantId == Tenant.AgnosticTenantId) + return; + + entity.TenantId ??= _tenantAccessor.TenantId; + } +} diff --git a/src/modules/Elsa.Identity/Services/MemoryUserStore.cs b/src/modules/Elsa.Identity/Services/MemoryUserStore.cs index 0cd492925..aa05bd881 100644 --- a/src/modules/Elsa.Identity/Services/MemoryUserStore.cs +++ b/src/modules/Elsa.Identity/Services/MemoryUserStore.cs @@ -1,3 +1,4 @@ +using Elsa.Common.Entities; using Elsa.Common.Multitenancy; using Elsa.Common.Services; using Elsa.Identity.Contracts; @@ -9,6 +10,11 @@ namespace Elsa.Identity.Services; /// /// Represents an in-memory user store. /// +/// +/// Ambient tenant is applied here rather than in callers. +/// EF owns that via SetTenantIdFilter / ApplyTenantId; Memory must compensate. +/// Null tenant IDs are visible only to the default tenant. +/// public class MemoryUserStore : IUserStore { private readonly MemoryStore _store; @@ -26,15 +32,21 @@ public class MemoryUserStore : IUserStore /// public Task SaveAsync(User user, CancellationToken cancellationToken = default) { - _store.Save(user, x => x.Id); + ApplyCurrentTenant(user); + lock (_store.Sync) + _store.Save(user, x => x.Id); return Task.CompletedTask; } /// public Task DeleteAsync(UserFilter filter, CancellationToken cancellationToken = default) { - var ids = _store.Query(query => Filter(query, filter)).Select(x => x.Id).Distinct().ToList(); - _store.DeleteWhere(x => ids.Contains(x.Id)); + lock (_store.Sync) + { + _store.DeleteWhere(user => + IsVisible(user) && filter.Apply(new[] { user }.AsQueryable()).Any()); + } + return Task.CompletedTask; } @@ -51,17 +63,17 @@ public class MemoryUserStore : IUserStore var result = _store.Query(query => Filter(query, filter)).FirstOrDefault(); return Task.FromResult(result); } - - /// - /// The ambient tenant is applied here rather than left to callers. Isolation previously existed only - /// on the Entity Framework path, and only when multitenancy was enabled, so a deployment running the - /// default in-memory stores had none at all. - /// - private IQueryable Filter(IQueryable queryable, UserFilter filter) - { - var tenantId = _tenantAccessor.TenantId; - queryable = queryable.Where(x => x.TenantId == tenantId || x.TenantId == Tenant.AgnosticTenantId || x.TenantId == null); - return filter.Apply(queryable); + private IQueryable Filter(IQueryable queryable, UserFilter filter) => + filter.Apply(queryable.WhereVisibleToTenant(_tenantAccessor.TenantId)); + + private bool IsVisible(Entity entity) => TenantVisibility.IsVisible(entity.TenantId, _tenantAccessor.TenantId); + + private void ApplyCurrentTenant(Entity entity) + { + if (entity.TenantId == Tenant.AgnosticTenantId) + return; + + entity.TenantId ??= _tenantAccessor.TenantId; } -} \ No newline at end of file +} diff --git a/test/unit/Elsa.Identity.UnitTests/Services/DefaultSecretHasherTests.cs b/test/unit/Elsa.Identity.UnitTests/Services/DefaultSecretHasherTests.cs index 6e982efd1..96cbe9636 100644 --- a/test/unit/Elsa.Identity.UnitTests/Services/DefaultSecretHasherTests.cs +++ b/test/unit/Elsa.Identity.UnitTests/Services/DefaultSecretHasherTests.cs @@ -147,7 +147,7 @@ public class DefaultSecretHasherTests { var apiKeyGenerator = new DefaultApiKeyGeneratorAndParser(); var apiKey = apiKeyGenerator.Generate("client-1"); - var applicationStore = new MemoryApplicationStore(new MemoryStore()); + var applicationStore = new MemoryApplicationStore(new MemoryStore(), TestTenantAccessor.Default); var legacyHash = CreateLegacyHash(apiKey); await applicationStore.SaveAsync(new Application { diff --git a/test/unit/Elsa.Identity.UnitTests/Services/MemoryApplicationStoreTenantIsolationTests.cs b/test/unit/Elsa.Identity.UnitTests/Services/MemoryApplicationStoreTenantIsolationTests.cs new file mode 100644 index 000000000..5b9c1e75b --- /dev/null +++ b/test/unit/Elsa.Identity.UnitTests/Services/MemoryApplicationStoreTenantIsolationTests.cs @@ -0,0 +1,145 @@ +using Elsa.Common.Multitenancy; +using Elsa.Common.Services; +using Elsa.Identity.Entities; +using Elsa.Identity.Models; +using Elsa.Identity.Services; +using Elsa.Testing.Shared.Multitenancy; + +namespace Elsa.Identity.UnitTests.Services; + +/// +/// Memory applications must honor ambient tenant the same way EF does via +/// SetTenantIdFilter / ApplyTenantId. +/// +public class MemoryApplicationStoreTenantIsolationTests +{ + [Fact(DisplayName = "FindAsync hides other tenants and keeps * visible")] + public async Task FindAsync_HidesOtherTenants() + { + var store = CreateStore("tenant-a"); + await SeedMixedTenantsAsync(store); + + var own = await store.FindAsync(new ApplicationFilter { Id = "app-a" }); + var star = await store.FindAsync(new ApplicationFilter { Id = "app-star" }); + var other = await store.FindAsync(new ApplicationFilter { Id = "app-b" }); + + Assert.NotNull(own); + Assert.Equal("app-a", own.Id); + Assert.NotNull(star); + Assert.Equal("app-star", star.Id); + Assert.Null(other); + } + + [Fact(DisplayName = "FindAsync on a named tenant hides null TenantId applications")] + public async Task FindAsync_WhenAmbientIsNamed_HidesNullTenantId() + { + var store = StoreWithPreassignedRows("tenant-a"); + + var found = await store.FindAsync(new ApplicationFilter { Id = "app-null" }); + var own = await store.FindAsync(new ApplicationFilter { Id = "app-a" }); + + Assert.Null(found); + Assert.NotNull(own); + Assert.Equal("app-a", own.Id); + } + + [Fact(DisplayName = "FindAsync on the default tenant includes null TenantId applications")] + public async Task FindAsync_WhenAmbientIsDefault_IncludesNullTenantId() + { + var store = StoreWithPreassignedRows(Tenant.DefaultTenantId); + + var found = await store.FindAsync(new ApplicationFilter { Id = "app-null" }); + + Assert.NotNull(found); + Assert.Equal("app-null", found.Id); + Assert.Null(found.TenantId); + } + + [Fact(DisplayName = "DeleteAsync does not remove another tenant's applications")] + public async Task DeleteAsync_DoesNotDeleteOtherTenantRows() + { + var backing = new MemoryStore(); + var tenantA = new MemoryApplicationStore(backing, new TestTenantAccessor("tenant-a")); + var tenantB = new MemoryApplicationStore(backing, new TestTenantAccessor("tenant-b")); + await SeedMixedTenantsAsync(tenantA); + + await tenantA.DeleteAsync(new ApplicationFilter { Id = "app-b" }); + var remaining = await tenantB.FindAsync(new ApplicationFilter { Id = "app-b" }); + + Assert.NotNull(remaining); + Assert.Equal("app-b", remaining.Id); + } + + [Fact(DisplayName = "DeleteAsync leaves a same-ID row after another tenant replaces it")] + public async Task DeleteAsync_WhenSameIdWasReplacedByOtherTenant_LeavesReplacement() + { + var backing = new MemoryStore(); + var tenantA = new MemoryApplicationStore(backing, new TestTenantAccessor("tenant-a")); + var tenantB = new MemoryApplicationStore(backing, new TestTenantAccessor("tenant-b")); + await tenantA.SaveAsync(CreateApplication("shared", "tenant-a")); + await tenantB.SaveAsync(CreateApplication("shared", "tenant-b")); + + await tenantA.DeleteAsync(new ApplicationFilter { Id = "shared" }); + var remaining = await tenantB.FindAsync(new ApplicationFilter { Id = "shared" }); + + Assert.NotNull(remaining); + Assert.Equal("tenant-b", remaining.TenantId); + } + + [Fact(DisplayName = "SaveAsync stamps the ambient tenant when TenantId is unset")] + public async Task SaveAsync_WhenTenantIdUnset_StampsAmbientTenant() + { + var store = CreateStore("tenant-a"); + var application = CreateApplication("app-new", tenantId: null); + + await store.SaveAsync(application); + + Assert.Equal("tenant-a", application.TenantId); + Assert.Equal("tenant-a", (await store.FindAsync(new ApplicationFilter { Id = "app-new" }))!.TenantId); + } + + [Fact(DisplayName = "SaveAsync does not overwrite * or an explicit TenantId")] + public async Task SaveAsync_DoesNotOverwriteAgnosticOrExplicitTenantId() + { + var store = CreateStore("tenant-a"); + var agnostic = CreateApplication("app-star", Tenant.AgnosticTenantId); + var explicitTenant = CreateApplication("app-a", "tenant-a"); + + await store.SaveAsync(agnostic); + await store.SaveAsync(explicitTenant); + + Assert.Equal(Tenant.AgnosticTenantId, agnostic.TenantId); + Assert.Equal("tenant-a", explicitTenant.TenantId); + } + + private static MemoryApplicationStore CreateStore(string tenantId) => + new(new MemoryStore(), new TestTenantAccessor(tenantId)); + + private static MemoryApplicationStore StoreWithPreassignedRows(string ambientTenantId) + { + var backing = new MemoryStore(); + backing.Save(CreateApplication("app-null", tenantId: null), x => x.Id); + backing.Save(CreateApplication("app-a", "tenant-a"), x => x.Id); + return new MemoryApplicationStore(backing, new TestTenantAccessor(ambientTenantId)); + } + + private static async Task SeedMixedTenantsAsync(MemoryApplicationStore store) + { + await store.SaveAsync(CreateApplication("app-a", "tenant-a")); + await store.SaveAsync(CreateApplication("app-b", "tenant-b")); + await store.SaveAsync(CreateApplication("app-star", Tenant.AgnosticTenantId)); + } + + private static Application CreateApplication(string id, string? tenantId) => + new() + { + Id = id, + ClientId = id, + Name = id, + HashedApiKey = "", + HashedApiKeySalt = "", + HashedClientSecret = "", + HashedClientSecretSalt = "", + TenantId = tenantId + }; +} diff --git a/test/unit/Elsa.Identity.UnitTests/Services/MemoryUserStoreTenantIsolationTests.cs b/test/unit/Elsa.Identity.UnitTests/Services/MemoryUserStoreTenantIsolationTests.cs new file mode 100644 index 000000000..688317869 --- /dev/null +++ b/test/unit/Elsa.Identity.UnitTests/Services/MemoryUserStoreTenantIsolationTests.cs @@ -0,0 +1,148 @@ +using Elsa.Common.Multitenancy; +using Elsa.Common.Services; +using Elsa.Identity.Entities; +using Elsa.Identity.Models; +using Elsa.Identity.Services; +using Elsa.Testing.Shared.Multitenancy; + +namespace Elsa.Identity.UnitTests.Services; + +/// +/// Memory users must honor ambient tenant the same way EF does via +/// SetTenantIdFilter / ApplyTenantId. +/// +public class MemoryUserStoreTenantIsolationTests +{ + [Fact(DisplayName = "FindManyAsync hides other tenants and keeps * visible")] + public async Task FindManyAsync_HidesOtherTenants() + { + var store = CreateStore("tenant-a"); + await SeedMixedTenantsAsync(store); + + var found = (await store.FindManyAsync(new UserFilter())).ToList(); + + Assert.Equal(2, found.Count); + Assert.Contains(found, x => x.Id == "user-a"); + Assert.Contains(found, x => x.Id == "user-star"); + Assert.DoesNotContain(found, x => x.Id == "user-b"); + } + + [Fact(DisplayName = "FindAsync does not return another tenant's user")] + public async Task FindAsync_WhenOtherTenant_ReturnsNull() + { + var store = CreateStore("tenant-a"); + await SeedMixedTenantsAsync(store); + + var found = await store.FindAsync(new UserFilter { Id = "user-b" }); + + Assert.Null(found); + } + + [Fact(DisplayName = "FindAsync on a named tenant hides null TenantId users")] + public async Task FindAsync_WhenAmbientIsNamed_HidesNullTenantId() + { + var store = StoreWithPreassignedRows("tenant-a"); + + var found = await store.FindAsync(new UserFilter { Id = "user-null" }); + var own = await store.FindAsync(new UserFilter { Id = "user-a" }); + + Assert.Null(found); + Assert.NotNull(own); + Assert.Equal("user-a", own.Id); + } + + [Fact(DisplayName = "FindAsync on the default tenant includes null TenantId users")] + public async Task FindAsync_WhenAmbientIsDefault_IncludesNullTenantId() + { + var store = StoreWithPreassignedRows(Tenant.DefaultTenantId); + + var found = await store.FindAsync(new UserFilter { Id = "user-null" }); + + Assert.NotNull(found); + Assert.Equal("user-null", found.Id); + Assert.Null(found.TenantId); + } + + [Fact(DisplayName = "DeleteAsync does not remove another tenant's users")] + public async Task DeleteAsync_DoesNotDeleteOtherTenantRows() + { + var backing = new MemoryStore(); + var tenantA = new MemoryUserStore(backing, new TestTenantAccessor("tenant-a")); + var tenantB = new MemoryUserStore(backing, new TestTenantAccessor("tenant-b")); + await SeedMixedTenantsAsync(tenantA); + + await tenantA.DeleteAsync(new UserFilter { Id = "user-b" }); + var remaining = await tenantB.FindAsync(new UserFilter { Id = "user-b" }); + + Assert.NotNull(remaining); + Assert.Equal("user-b", remaining.Id); + } + + [Fact(DisplayName = "DeleteAsync leaves a same-ID row after another tenant replaces it")] + public async Task DeleteAsync_WhenSameIdWasReplacedByOtherTenant_LeavesReplacement() + { + var backing = new MemoryStore(); + var tenantA = new MemoryUserStore(backing, new TestTenantAccessor("tenant-a")); + var tenantB = new MemoryUserStore(backing, new TestTenantAccessor("tenant-b")); + await tenantA.SaveAsync(CreateUser("shared", "tenant-a")); + await tenantB.SaveAsync(CreateUser("shared", "tenant-b")); + + await tenantA.DeleteAsync(new UserFilter { Id = "shared" }); + var remaining = await tenantB.FindAsync(new UserFilter { Id = "shared" }); + + Assert.NotNull(remaining); + Assert.Equal("tenant-b", remaining.TenantId); + } + + [Fact(DisplayName = "SaveAsync stamps the ambient tenant when TenantId is unset")] + public async Task SaveAsync_WhenTenantIdUnset_StampsAmbientTenant() + { + var store = CreateStore("tenant-a"); + var user = CreateUser("user-new", tenantId: null); + + await store.SaveAsync(user); + + Assert.Equal("tenant-a", user.TenantId); + Assert.Equal("tenant-a", (await store.FindAsync(new UserFilter { Id = "user-new" }))!.TenantId); + } + + [Fact(DisplayName = "SaveAsync does not overwrite * or an explicit TenantId")] + public async Task SaveAsync_DoesNotOverwriteAgnosticOrExplicitTenantId() + { + var store = CreateStore("tenant-a"); + var agnostic = CreateUser("user-star", Tenant.AgnosticTenantId); + var explicitTenant = CreateUser("user-a", "tenant-a"); + + await store.SaveAsync(agnostic); + await store.SaveAsync(explicitTenant); + + Assert.Equal(Tenant.AgnosticTenantId, agnostic.TenantId); + Assert.Equal("tenant-a", explicitTenant.TenantId); + } + + private static MemoryUserStore CreateStore(string tenantId) => + new(new MemoryStore(), new TestTenantAccessor(tenantId)); + + private static MemoryUserStore StoreWithPreassignedRows(string ambientTenantId) + { + var backing = new MemoryStore(); + backing.Save(CreateUser("user-null", tenantId: null), x => x.Id); + backing.Save(CreateUser("user-a", "tenant-a"), x => x.Id); + return new MemoryUserStore(backing, new TestTenantAccessor(ambientTenantId)); + } + + private static async Task SeedMixedTenantsAsync(MemoryUserStore store) + { + await store.SaveAsync(CreateUser("user-a", "tenant-a")); + await store.SaveAsync(CreateUser("user-b", "tenant-b")); + await store.SaveAsync(CreateUser("user-star", Tenant.AgnosticTenantId)); + } + + private static User CreateUser(string id, string? tenantId) => + new() + { + Id = id, + Name = id, + TenantId = tenantId + }; +}