diff --git a/src/modules/Elsa.Identity/Contracts/IRoleStore.cs b/src/modules/Elsa.Identity/Contracts/IRoleStore.cs index 26ded57b4..3fff2e652 100644 --- a/src/modules/Elsa.Identity/Contracts/IRoleStore.cs +++ b/src/modules/Elsa.Identity/Contracts/IRoleStore.cs @@ -19,6 +19,11 @@ public interface IRoleStore /// /// Deletes the role with the specified ID. /// + /// + /// This member reports no affected-row count, so a caller cannot tell a deletion it performed itself apart + /// from one a concurrent caller had already performed. Implementations able to delete atomically should + /// also implement , which callers prefer when it is available. + /// /// The filter. /// The cancellation token. /// The task. diff --git a/src/modules/Elsa.Identity/Contracts/IRoleStoreWithAtomicDelete.cs b/src/modules/Elsa.Identity/Contracts/IRoleStoreWithAtomicDelete.cs new file mode 100644 index 000000000..5931d78fb --- /dev/null +++ b/src/modules/Elsa.Identity/Contracts/IRoleStoreWithAtomicDelete.cs @@ -0,0 +1,28 @@ +namespace Elsa.Identity.Contracts; + +/// +/// An optional capability, implemented alongside , that deletes roles atomically and +/// reports whether the calling request is the one that removed them. +/// +/// +/// This capability is deliberately separate from so that third-party role stores keep +/// compiling and binding against the unchanged signature. Callers that need +/// to act only on a deletion they performed themselves, such as one publishing a security notification, probe for +/// this interface and fall back to when a store does not offer it. +/// +public interface IRoleStoreWithAtomicDelete +{ + /// + /// Deletes the single role with the given ID within the current tenant scope, atomically, and reports whether + /// this call removed it. + /// + /// + /// Implementations must decide the outcome atomically, so that exactly one of two concurrent callers for the + /// same ID observes . Loading the role and then deleting it in a separate step does not + /// satisfy this contract. + /// + /// The ID of the role to delete. + /// The cancellation token. + /// when this call removed the role; otherwise, . + Task TryDeleteAsync(string roleId, CancellationToken cancellationToken = default); +} diff --git a/src/modules/Elsa.Identity/Services/MemoryRoleStore.cs b/src/modules/Elsa.Identity/Services/MemoryRoleStore.cs index c7e2d1cbb..19f104b21 100644 --- a/src/modules/Elsa.Identity/Services/MemoryRoleStore.cs +++ b/src/modules/Elsa.Identity/Services/MemoryRoleStore.cs @@ -9,7 +9,7 @@ namespace Elsa.Identity.Services; /// /// Represents an in-memory role store. /// -public class MemoryRoleStore : IRoleStore +public class MemoryRoleStore : IRoleStore, IRoleStoreWithAtomicDelete { private readonly MemoryStore _store; private readonly ITenantAccessor _tenantAccessor; @@ -38,6 +38,20 @@ public class MemoryRoleStore : IRoleStore return Task.CompletedTask; } + /// + /// + /// The matching role's key is removed through the underlying concurrent dictionary, whose removal is a single + /// compare-and-remove step. Two callers racing on the same role ID therefore see one and + /// one , rather than both concluding they deleted it. + /// + public Task TryDeleteAsync(string roleId, CancellationToken cancellationToken = default) + { + var role = _store.Query(query => Filter(query, new RoleFilter { Id = roleId })).FirstOrDefault(); + var deleted = role is not null && _store.Delete(GetStorageKey(role)); + + return Task.FromResult(deleted); + } + /// public Task SaveAsync(Role role, CancellationToken cancellationToken = default) { diff --git a/src/modules/Elsa.Identity/Services/RoleDeletionCoordinator.cs b/src/modules/Elsa.Identity/Services/RoleDeletionCoordinator.cs index ba3f9875e..15381503f 100644 --- a/src/modules/Elsa.Identity/Services/RoleDeletionCoordinator.cs +++ b/src/modules/Elsa.Identity/Services/RoleDeletionCoordinator.cs @@ -11,9 +11,11 @@ namespace Elsa.Identity.Services; public sealed class RoleDeletionCoordinator( IRoleStore roleStore, IRoleAuthorizationService roleAuthorizationService, - IEnumerable contributors) : IRoleDeletionCoordinator + IEnumerable contributors, + RoleSecurityNotifier securityNotifier) : IRoleDeletionCoordinator { private readonly IReadOnlyDictionary _contributors = contributors.ToDictionary(x => x.Source, StringComparer.Ordinal); + private readonly IRoleStoreWithAtomicDelete? _atomicRoleStore = roleStore as IRoleStoreWithAtomicDelete; /// public async ValueTask InspectAsync(string roleId, ClaimsPrincipal actor, CancellationToken cancellationToken = default) @@ -41,8 +43,7 @@ public sealed class RoleDeletionCoordinator( if (!impact.CanDelete) return new RoleDeletionOperationResult.Blocked(impact); - await roleStore.DeleteAsync(new() { Id = roleId }, cancellationToken); - return new RoleDeletionOperationResult.Deleted([]); + return await DeleteRoleAsync(roleId, actor, [], cancellationToken); } /// @@ -64,10 +65,7 @@ public sealed class RoleDeletionCoordinator( return new RoleDeletionOperationResult.ValidationFailed(impact, selectionError); if (impact.CanDelete) - { - await roleStore.DeleteAsync(new() { Id = command.RoleId }, cancellationToken); - return new RoleDeletionOperationResult.Deleted([]); - } + return await DeleteRoleAsync(command.RoleId, command.Actor, [], cancellationToken); var selectedDependencies = SelectEditableDependencies(impact, command.SelectedReferences); var replacementValidation = await ValidateReplacementRoleAsync(impact, command, selectedDependencies, cancellationToken); @@ -133,8 +131,46 @@ public sealed class RoleDeletionCoordinator( if (!finalImpact.CanDelete) return new RoleDeletionOperationResult.Incomplete(finalImpact, changedOwnerIds.Distinct(StringComparer.Ordinal).ToArray(), "role_dependencies_remain"); - await roleStore.DeleteAsync(new() { Id = command.RoleId }, cancellationToken); - return new RoleDeletionOperationResult.Deleted(changedOwnerIds.Distinct(StringComparer.Ordinal).ToArray()); + return await DeleteRoleAsync(command.RoleId, command.Actor, changedOwnerIds.Distinct(StringComparer.Ordinal).ToArray(), cancellationToken); + } + + /// + /// Deletes the role and publishes the deletion to security subscribers, reporting + /// when this call did not remove it. + /// + /// + /// The snapshot taken before the delete is what the notification carries, because the name and permissions a + /// reviewer needs are gone once the row is. The snapshot alone cannot decide whether to publish: a concurrent + /// request may remove the role between the read and the delete, and both callers would then report a deletion + /// they did not perform. Where the store implements the store's own + /// affected-row verdict decides instead, so exactly one racing caller publishes. Stores that do not implement + /// that capability keep the legacy find-then-delete path and publish once the delete returns, which preserves + /// the notification for third-party stores at the cost of not distinguishing concurrent callers. + /// + private async ValueTask DeleteRoleAsync( + string roleId, + ClaimsPrincipal actor, + IReadOnlyCollection changedOwnerIds, + CancellationToken cancellationToken) + { + var role = await roleStore.FindAsync(new() { Id = roleId }, cancellationToken); + if (role is null) + return new RoleDeletionOperationResult.NotFound(); + + if (_atomicRoleStore is not null) + { + if (!await _atomicRoleStore.TryDeleteAsync(roleId, cancellationToken)) + return new RoleDeletionOperationResult.NotFound(); + } + else + { + await roleStore.DeleteAsync(new() { Id = roleId }, cancellationToken); + } + + // The role is already gone, so the notification is published with a token the request cannot cancel: + // a caller that walks away mid-request must not silence a deletion that has completed. + await securityNotifier.RoleChangedAsync(actor, "deleted", role.Id, role.Name, role.Permissions.ToArray(), CancellationToken.None); + return new RoleDeletionOperationResult.Deleted(changedOwnerIds); } private async ValueTask> InspectContributorsAsync(string roleId, CancellationToken cancellationToken) diff --git a/src/modules/Elsa.Persistence.EFCore/Modules/Identity/RoleStore.cs b/src/modules/Elsa.Persistence.EFCore/Modules/Identity/RoleStore.cs index e0b8cc6ca..0e0e2428e 100644 --- a/src/modules/Elsa.Persistence.EFCore/Modules/Identity/RoleStore.cs +++ b/src/modules/Elsa.Persistence.EFCore/Modules/Identity/RoleStore.cs @@ -8,7 +8,7 @@ namespace Elsa.Persistence.EFCore.Modules.Identity; /// /// An EF Core implementation of . /// -public class EFCoreRoleStore : IRoleStore +public class EFCoreRoleStore : IRoleStore, IRoleStoreWithAtomicDelete { private readonly EntityStore _applicationStore; @@ -38,6 +38,17 @@ public class EFCoreRoleStore : IRoleStore await _applicationStore.DeleteWhereAsync(query => Filter(query, filter), cancellationToken); } + /// + /// + /// The delete is issued as a single DELETE ... WHERE statement and the affected-row count it returns is + /// the database's own verdict on which caller removed the row, so two concurrent callers cannot both observe + /// . + /// + public async Task TryDeleteAsync(string roleId, CancellationToken cancellationToken = default) + { + return await _applicationStore.DeleteWhereAsync(query => Filter(query, new RoleFilter { Id = roleId }), cancellationToken) > 0; + } + /// public async Task FindAsync(RoleFilter filter, CancellationToken cancellationToken = default) { diff --git a/test/unit/Elsa.ExternalAuthentication.UnitTests/Foundational/ExternalAuthenticationRoleDeletionDependencyContributorTests.cs b/test/unit/Elsa.ExternalAuthentication.UnitTests/Foundational/ExternalAuthenticationRoleDeletionDependencyContributorTests.cs index 0743bba4b..90d0243ef 100644 --- a/test/unit/Elsa.ExternalAuthentication.UnitTests/Foundational/ExternalAuthenticationRoleDeletionDependencyContributorTests.cs +++ b/test/unit/Elsa.ExternalAuthentication.UnitTests/Foundational/ExternalAuthenticationRoleDeletionDependencyContributorTests.cs @@ -14,7 +14,9 @@ using Elsa.Identity.Entities; using Elsa.Identity.Models; using Elsa.Identity.Providers; using Elsa.Identity.Services; +using Elsa.Mediator.Contracts; using Microsoft.Extensions.DependencyInjection; +using NSubstitute; namespace Elsa.ExternalAuthentication.UnitTests.Foundational; @@ -235,7 +237,8 @@ public class ExternalAuthenticationRoleDeletionDependencyContributorTests new ConnectionRevisionCalculator(), new ExternalAuthenticationSecurityNotifier(services), new PermissionEvaluator()); - var coordinator = new RoleDeletionCoordinator(roleStore, roleAuthorizationService, [contributor]); + var securityNotifier = new RoleSecurityNotifier(Substitute.For(), TestTenantAccessor.Default, new SystemClock()); + var coordinator = new RoleDeletionCoordinator(roleStore, roleAuthorizationService, [contributor], securityNotifier); var impact = Assert.IsType(await coordinator.InspectAsync("workflow-user", Administrator())).Impact; var result = await coordinator.RemediateAndDeleteAsync(new RoleDeletionRemediationCommand( diff --git a/test/unit/Elsa.Identity.UnitTests/Services/RoleDeletionCoordinatorTests.cs b/test/unit/Elsa.Identity.UnitTests/Services/RoleDeletionCoordinatorTests.cs index 8c700a586..38b91040a 100644 --- a/test/unit/Elsa.Identity.UnitTests/Services/RoleDeletionCoordinatorTests.cs +++ b/test/unit/Elsa.Identity.UnitTests/Services/RoleDeletionCoordinatorTests.cs @@ -5,8 +5,11 @@ using Elsa.Common.Services; using Elsa.Identity.Contracts; using Elsa.Identity.Entities; using Elsa.Identity.Models; +using Elsa.Identity.Notifications; using Elsa.Identity.Providers; using Elsa.Identity.Services; +using Elsa.Mediator.Contracts; +using NSubstitute; namespace Elsa.Identity.UnitTests.Services; @@ -78,15 +81,91 @@ public class RoleDeletionCoordinatorTests [Fact] public async Task OrdinaryDeletionIsBlockedByConfigurationDependency() { + var notificationSender = Substitute.For(); var (store, coordinator) = await CreateCoordinatorAsync( new StubContributor([ Dependency("configuration", RoleDeletionDependencyOwnership.Configuration, configurationPath: "ExternalAuthentication:Connections:0:UnlinkedPolicy:Settings:defaultRoleIds:0") - ])); + ]), + notificationSender); var result = await coordinator.DeleteAsync("workflow-user", Administrator()); Assert.IsType(result); Assert.NotNull(await store.FindAsync(new() { Id = "workflow-user" })); + await AssertNoRoleNotificationAsync(notificationSender); + } + + [Fact] + public async Task SuccessfulDeletionPublishesDeletedRoleNotification() + { + var notificationSender = Substitute.For(); + var (store, coordinator) = await CreateCoordinatorAsync(new StubContributor([]), notificationSender); + // A cancellable request token is what makes the token assertion below mean anything: were the notification + // published with the request's own token, that token could be cancelled and the assertion would fail. + using var request = new CancellationTokenSource(); + + var result = await coordinator.DeleteAsync("workflow-user", Administrator(), request.Token); + + Assert.IsType(result); + Assert.Null(await store.FindAsync(new() { Id = "workflow-user" })); + await AssertRoleDeletedNotificationAsync(notificationSender); + } + + [Fact] + public async Task DeletionThatRemovedNothingReportsNotFoundWithoutPublishing() + { + // What the loser of a race sees: the role is still there to be found, and the delete then removes no row + // because a concurrent request got there first. Publishing here would credit this request with a deletion + // it did not perform, and audit would record the role as deleted twice. + var notificationSender = Substitute.For(); + var (_, coordinator) = await CreateCoordinatorAsync( + new StubContributor([]), + notificationSender, + inner => new RoleStoreThatDeletesNothing(inner)); + + var result = await coordinator.DeleteAsync("workflow-user", Administrator()); + + Assert.IsType(result); + await AssertNoRoleNotificationAsync(notificationSender); + } + + [Fact] + public async Task DeletionThroughStoreWithoutAtomicCapabilityStillPublishesOnce() + { + // A third-party store implementing only IRoleStore reports no affected-row count. The deletion must still + // reach security subscribers over that legacy path rather than being dropped for want of the capability. + var notificationSender = Substitute.For(); + var (store, coordinator) = await CreateCoordinatorAsync( + new StubContributor([]), + notificationSender, + inner => new LegacyRoleStore(inner)); + + var result = await coordinator.DeleteAsync("workflow-user", Administrator()); + + Assert.IsType(result); + Assert.Null(await store.FindAsync(new() { Id = "workflow-user" })); + await AssertRoleDeletedNotificationAsync(notificationSender); + } + + [Fact] + public async Task ConcurrentDeletionsPublishExactlyOneNotification() + { + // Both requests are held until each has already found the role, so neither can be turned away by the + // existence check and the store's own delete is the only thing that can separate them. + var notificationSender = Substitute.For(); + var (store, coordinator) = await CreateCoordinatorAsync( + new StubContributor([]), + notificationSender, + inner => new RoleStoreThatDeletesInLockstep(inner, 2)); + + var results = await Task.WhenAll( + Task.Run(async () => await coordinator.DeleteAsync("workflow-user", Administrator())), + Task.Run(async () => await coordinator.DeleteAsync("workflow-user", Administrator()))); + + Assert.Single(results, result => result is RoleDeletionOperationResult.Deleted); + Assert.Single(results, result => result is RoleDeletionOperationResult.NotFound); + Assert.Null(await store.FindAsync(new() { Id = "workflow-user" })); + await AssertRoleDeletedNotificationAsync(notificationSender); } [Fact] @@ -116,7 +195,8 @@ public class RoleDeletionCoordinatorTests public async Task SuccessfulRemediationRemovesDependenciesBeforeDeletingRole() { var contributor = new StubContributor([Dependency("connection-a", removesLastDefaultRole: true)]); - var (store, coordinator) = await CreateCoordinatorAsync(contributor); + var notificationSender = Substitute.For(); + var (store, coordinator) = await CreateCoordinatorAsync(contributor, notificationSender); var impact = Assert.IsType(await coordinator.InspectAsync("workflow-user", Administrator())).Impact; var result = await coordinator.RemediateAndDeleteAsync(new( @@ -131,6 +211,7 @@ public class RoleDeletionCoordinatorTests Assert.Equal(["connection-a"], deleted.ChangedOwnerIds); Assert.Null(await store.FindAsync(new() { Id = "workflow-user" })); Assert.Empty(contributor.Dependencies); + await AssertRoleDeletedNotificationAsync(notificationSender); } [Fact] @@ -139,7 +220,8 @@ public class RoleDeletionCoordinatorTests var contributor = new StubContributor( [Dependency("connection-a"), Dependency("connection-b")], failAfterFirst: true); - var (store, coordinator) = await CreateCoordinatorAsync(contributor); + var notificationSender = Substitute.For(); + var (store, coordinator) = await CreateCoordinatorAsync(contributor, notificationSender); var impact = Assert.IsType(await coordinator.InspectAsync("workflow-user", Administrator())).Impact; var result = await coordinator.RemediateAndDeleteAsync(new( @@ -154,6 +236,7 @@ public class RoleDeletionCoordinatorTests Assert.Equal(["connection-a"], incomplete.ChangedOwnerIds); Assert.NotNull(await store.FindAsync(new() { Id = "workflow-user" })); Assert.Single(contributor.Dependencies); + await AssertNoRoleNotificationAsync(notificationSender); } [Fact] @@ -288,15 +371,34 @@ public class RoleDeletionCoordinatorTests Assert.Single(contributor.Dependencies); } - private static async Task<(MemoryRoleStore Store, RoleDeletionCoordinator Coordinator)> CreateCoordinatorAsync(IRoleDeletionDependencyContributor contributor) + private static async Task<(MemoryRoleStore Store, RoleDeletionCoordinator Coordinator)> CreateCoordinatorAsync( + IRoleDeletionDependencyContributor contributor, + INotificationSender? notificationSender = null, + Func? storeDecorator = null) { var store = new MemoryRoleStore(new MemoryStore(), TestTenantAccessor.Default); await store.SaveAsync(new Role { Id = "workflow-user", Name = "Workflow user", Permissions = [] }); - var roleProvider = new StoreBasedRoleProvider(store); - var coordinator = new RoleDeletionCoordinator(store, new RoleAuthorizationService(roleProvider, new PermissionEvaluator()), [contributor]); + var roleStore = storeDecorator?.Invoke(store) ?? store; + var roleProvider = new StoreBasedRoleProvider(roleStore); + var securityNotifier = new RoleSecurityNotifier(notificationSender ?? Substitute.For(), TestTenantAccessor.Default, new SystemClock()); + var coordinator = new RoleDeletionCoordinator(roleStore, new RoleAuthorizationService(roleProvider, new PermissionEvaluator()), [contributor], securityNotifier); return (store, coordinator); } + private static async Task AssertRoleDeletedNotificationAsync(INotificationSender notificationSender) => + await notificationSender.Received(1).SendAsync( + Arg.Is(notification => + notification.Operation == "deleted" && + notification.RoleId == "workflow-user" && + notification.RoleName == "Workflow user" && + notification.Permissions.Count == 0), + // The row is already gone by the time this is published, so a request that is cancelled or abandoned + // must not be able to silence it. An uncancellable token is how that is guaranteed. + Arg.Is(token => !token.CanBeCanceled)); + + private static async Task AssertNoRoleNotificationAsync(INotificationSender notificationSender) => + await notificationSender.DidNotReceive().SendAsync(Arg.Any(), Arg.Any()); + private static ClaimsPrincipal Administrator() => new(new ClaimsIdentity([new Claim(PermissionNames.ClaimType, PermissionNames.All)])); private static ClaimsPrincipal PrincipalWith(string permission) => new(new ClaimsIdentity([new Claim(PermissionNames.ClaimType, permission)])); @@ -350,4 +452,40 @@ public class RoleDeletionCoordinatorTests private string Version() => string.Join("|", Dependencies.Select(x => $"{x.OwnerId}:{x.ExpectedRevision}")); } + + /// Offers only , standing in for a store that predates atomic deletion. + private class LegacyRoleStore(MemoryRoleStore inner) : IRoleStore + { + protected MemoryRoleStore Inner { get; } = inner; + + public Task AddAsync(Role role, CancellationToken cancellationToken = default) => Inner.AddAsync(role, cancellationToken); + + public Task DeleteAsync(RoleFilter filter, CancellationToken cancellationToken = default) => Inner.DeleteAsync(filter, cancellationToken); + + public Task SaveAsync(Role role, CancellationToken cancellationToken = default) => Inner.SaveAsync(role, cancellationToken); + + public Task FindAsync(RoleFilter filter, CancellationToken cancellationToken = default) => Inner.FindAsync(filter, cancellationToken); + + public Task> FindManyAsync(RoleFilter filter, CancellationToken cancellationToken = default) => Inner.FindManyAsync(filter, cancellationToken); + } + + /// Finds the role but reports that the delete removed nothing, as the loser of a race would. + private sealed class RoleStoreThatDeletesNothing(MemoryRoleStore inner) : LegacyRoleStore(inner), IRoleStoreWithAtomicDelete + { + public Task TryDeleteAsync(string roleId, CancellationToken cancellationToken = default) => Task.FromResult(false); + } + + /// Holds every caller at the delete until they have all found the role, then lets them race for real. + private sealed class RoleStoreThatDeletesInLockstep(MemoryRoleStore inner, int callers) : LegacyRoleStore(inner), IRoleStoreWithAtomicDelete + { + private readonly Barrier _barrier = new(callers); + + public Task TryDeleteAsync(string roleId, CancellationToken cancellationToken = default) + { + if (!_barrier.SignalAndWait(TimeSpan.FromSeconds(30))) + throw new TimeoutException("The concurrent deletions never met at the barrier."); + + return Inner.TryDeleteAsync(roleId, cancellationToken); + } + } }