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 <sipkeschoorstra@outlook.com> * 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 <sipkeschoorstra@outlook.com> --------- Co-authored-by: Cursor Agent <cursoragent@cursor.com>
This commit is contained in:
Sipke Schoorstra
GitHub
parent
373f99d163
commit
21e12b3fbd
|
|
@ -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;
|
|||
/// <summary>
|
||||
/// Represents an in-memory application store.
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// Ambient tenant is applied here rather than in callers.
|
||||
/// EF owns that via <c>SetTenantIdFilter</c> / <c>ApplyTenantId</c>; Memory must compensate.
|
||||
/// Application contracts have no TenantAgnostic flag, so isolation always applies (EF query filter).
|
||||
/// </remarks>
|
||||
public class MemoryApplicationStore : IApplicationStore
|
||||
{
|
||||
private readonly MemoryStore<Application> _store;
|
||||
private readonly ITenantAccessor _tenantAccessor;
|
||||
|
||||
/// <summary>
|
||||
/// Initializes a new instance of the <see cref="MemoryApplicationStore"/> class.
|
||||
/// </summary>
|
||||
public MemoryApplicationStore(MemoryStore<Application> store)
|
||||
public MemoryApplicationStore(MemoryStore<Application> store, ITenantAccessor tenantAccessor)
|
||||
{
|
||||
_store = store;
|
||||
_tenantAccessor = tenantAccessor;
|
||||
}
|
||||
|
||||
/// <inheritdoc />
|
||||
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;
|
||||
}
|
||||
|
||||
/// <inheritdoc />
|
||||
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<Application> Filter(IQueryable<Application> queryable, ApplicationFilter filter) => filter.Apply(queryable);
|
||||
}
|
||||
|
||||
private IQueryable<Application> Filter(IQueryable<Application> 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;
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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;
|
|||
/// <summary>
|
||||
/// Represents an in-memory user store.
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// Ambient tenant is applied here rather than in callers.
|
||||
/// EF owns that via <c>SetTenantIdFilter</c> / <c>ApplyTenantId</c>; Memory must compensate.
|
||||
/// Null tenant IDs are visible only to the default tenant.
|
||||
/// </remarks>
|
||||
public class MemoryUserStore : IUserStore
|
||||
{
|
||||
private readonly MemoryStore<User> _store;
|
||||
|
|
@ -26,15 +32,21 @@ public class MemoryUserStore : IUserStore
|
|||
/// <inheritdoc />
|
||||
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;
|
||||
}
|
||||
|
||||
/// <inheritdoc />
|
||||
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);
|
||||
}
|
||||
|
||||
/// <remarks>
|
||||
/// 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.
|
||||
/// </remarks>
|
||||
private IQueryable<User> Filter(IQueryable<User> 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<User> Filter(IQueryable<User> 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;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -147,7 +147,7 @@ public class DefaultSecretHasherTests
|
|||
{
|
||||
var apiKeyGenerator = new DefaultApiKeyGeneratorAndParser();
|
||||
var apiKey = apiKeyGenerator.Generate("client-1");
|
||||
var applicationStore = new MemoryApplicationStore(new MemoryStore<Application>());
|
||||
var applicationStore = new MemoryApplicationStore(new MemoryStore<Application>(), TestTenantAccessor.Default);
|
||||
var legacyHash = CreateLegacyHash(apiKey);
|
||||
await applicationStore.SaveAsync(new Application
|
||||
{
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
|
||||
/// <summary>
|
||||
/// Memory applications must honor ambient tenant the same way EF does via
|
||||
/// <c>SetTenantIdFilter</c> / <c>ApplyTenantId</c>.
|
||||
/// </summary>
|
||||
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<Application>();
|
||||
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<Application>();
|
||||
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<Application>(), new TestTenantAccessor(tenantId));
|
||||
|
||||
private static MemoryApplicationStore StoreWithPreassignedRows(string ambientTenantId)
|
||||
{
|
||||
var backing = new MemoryStore<Application>();
|
||||
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
|
||||
};
|
||||
}
|
||||
|
|
@ -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;
|
||||
|
||||
/// <summary>
|
||||
/// Memory users must honor ambient tenant the same way EF does via
|
||||
/// <c>SetTenantIdFilter</c> / <c>ApplyTenantId</c>.
|
||||
/// </summary>
|
||||
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<User>();
|
||||
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<User>();
|
||||
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<User>(), new TestTenantAccessor(tenantId));
|
||||
|
||||
private static MemoryUserStore StoreWithPreassignedRows(string ambientTenantId)
|
||||
{
|
||||
var backing = new MemoryStore<User>();
|
||||
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
|
||||
};
|
||||
}
|
||||
Loading…
Reference in a new issue