fix(identity): publish the role security notification after role deletion (#8026)

* Initial plan

* Publish security notification after role deletion

Co-authored-by: sfmskywalker <938393+sfmskywalker@users.noreply.github.com>

* Fix atomic role deletion notifications

Co-authored-by: sfmskywalker <938393+sfmskywalker@users.noreply.github.com>

* refactor(identity): delegate MemoryRoleStore.DeleteAsync to TryDeleteAsync

Mirrors EFCoreRoleStore.DeleteAsync so the deletion logic exists once
instead of being duplicated between DeleteAsync and TryDeleteAsync.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

* fix(identity): scope the atomic role delete capability to a single role ID

Narrow IRoleStoreWithAtomicDelete.TryDeleteAsync to accept a single role
ID instead of a RoleFilter, closing a race where two concurrent deletes
matching multiple roles could each remove one and both report success.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: sfmskywalker <938393+sfmskywalker@users.noreply.github.com>
Co-authored-by: Sipke Schoorstra <sipkeschoorstra@outlook.com>
Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
This commit is contained in:
Copilot 2026-09-06 19:44:36 -07:00 committed by GitHub
parent cbdc3f7e9d
commit 536cee3fc2
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
7 changed files with 253 additions and 18 deletions

View file

@ -19,6 +19,11 @@ public interface IRoleStore
/// <summary>
/// Deletes the role with the specified ID.
/// </summary>
/// <remarks>
/// 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 <see cref="IRoleStoreWithAtomicDelete"/>, which callers prefer when it is available.
/// </remarks>
/// <param name="filter">The filter.</param>
/// <param name="cancellationToken">The cancellation token.</param>
/// <returns>The task.</returns>

View file

@ -0,0 +1,28 @@
namespace Elsa.Identity.Contracts;
/// <summary>
/// An optional capability, implemented alongside <see cref="IRoleStore"/>, that deletes roles atomically and
/// reports whether the calling request is the one that removed them.
/// </summary>
/// <remarks>
/// This capability is deliberately separate from <see cref="IRoleStore"/> so that third-party role stores keep
/// compiling and binding against the unchanged <see cref="IRoleStore.DeleteAsync"/> 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 <see cref="IRoleStore.DeleteAsync"/> when a store does not offer it.
/// </remarks>
public interface IRoleStoreWithAtomicDelete
{
/// <summary>
/// Deletes the single role with the given ID within the current tenant scope, atomically, and reports whether
/// this call removed it.
/// </summary>
/// <remarks>
/// Implementations must decide the outcome atomically, so that exactly one of two concurrent callers for the
/// same ID observes <see langword="true"/>. Loading the role and then deleting it in a separate step does not
/// satisfy this contract.
/// </remarks>
/// <param name="roleId">The ID of the role to delete.</param>
/// <param name="cancellationToken">The cancellation token.</param>
/// <returns><see langword="true"/> when this call removed the role; otherwise, <see langword="false"/>.</returns>
Task<bool> TryDeleteAsync(string roleId, CancellationToken cancellationToken = default);
}

View file

@ -9,7 +9,7 @@ namespace Elsa.Identity.Services;
/// <summary>
/// Represents an in-memory role store.
/// </summary>
public class MemoryRoleStore : IRoleStore
public class MemoryRoleStore : IRoleStore, IRoleStoreWithAtomicDelete
{
private readonly MemoryStore<Role> _store;
private readonly ITenantAccessor _tenantAccessor;
@ -38,6 +38,20 @@ public class MemoryRoleStore : IRoleStore
return Task.CompletedTask;
}
/// <inheritdoc />
/// <remarks>
/// 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 <see langword="true"/> and
/// one <see langword="false"/>, rather than both concluding they deleted it.
/// </remarks>
public Task<bool> 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);
}
/// <inheritdoc />
public Task SaveAsync(Role role, CancellationToken cancellationToken = default)
{

View file

@ -11,9 +11,11 @@ namespace Elsa.Identity.Services;
public sealed class RoleDeletionCoordinator(
IRoleStore roleStore,
IRoleAuthorizationService roleAuthorizationService,
IEnumerable<IRoleDeletionDependencyContributor> contributors) : IRoleDeletionCoordinator
IEnumerable<IRoleDeletionDependencyContributor> contributors,
RoleSecurityNotifier securityNotifier) : IRoleDeletionCoordinator
{
private readonly IReadOnlyDictionary<string, IRoleDeletionDependencyContributor> _contributors = contributors.ToDictionary(x => x.Source, StringComparer.Ordinal);
private readonly IRoleStoreWithAtomicDelete? _atomicRoleStore = roleStore as IRoleStoreWithAtomicDelete;
/// <inheritdoc />
public async ValueTask<RoleDeletionInspectionResult> 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);
}
/// <inheritdoc />
@ -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);
}
/// <summary>
/// Deletes the role and publishes the deletion to security subscribers, reporting
/// <see cref="RoleDeletionOperationResult.NotFound"/> when this call did not remove it.
/// </summary>
/// <remarks>
/// 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 <see cref="IRoleStoreWithAtomicDelete"/> 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.
/// </remarks>
private async ValueTask<RoleDeletionOperationResult> DeleteRoleAsync(
string roleId,
ClaimsPrincipal actor,
IReadOnlyCollection<string> 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<IReadOnlyCollection<RoleDeletionDependencySnapshot>> InspectContributorsAsync(string roleId, CancellationToken cancellationToken)

View file

@ -8,7 +8,7 @@ namespace Elsa.Persistence.EFCore.Modules.Identity;
/// <summary>
/// An EF Core implementation of <see cref="IRoleStore"/>.
/// </summary>
public class EFCoreRoleStore : IRoleStore
public class EFCoreRoleStore : IRoleStore, IRoleStoreWithAtomicDelete
{
private readonly EntityStore<IdentityElsaDbContext, Role> _applicationStore;
@ -38,6 +38,17 @@ public class EFCoreRoleStore : IRoleStore
await _applicationStore.DeleteWhereAsync(query => Filter(query, filter), cancellationToken);
}
/// <inheritdoc />
/// <remarks>
/// The delete is issued as a single <c>DELETE ... WHERE</c> 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
/// <see langword="true"/>.
/// </remarks>
public async Task<bool> TryDeleteAsync(string roleId, CancellationToken cancellationToken = default)
{
return await _applicationStore.DeleteWhereAsync(query => Filter(query, new RoleFilter { Id = roleId }), cancellationToken) > 0;
}
/// <inheritdoc />
public async Task<Role?> FindAsync(RoleFilter filter, CancellationToken cancellationToken = default)
{

View file

@ -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<INotificationSender>(), TestTenantAccessor.Default, new SystemClock());
var coordinator = new RoleDeletionCoordinator(roleStore, roleAuthorizationService, [contributor], securityNotifier);
var impact = Assert.IsType<RoleDeletionInspectionResult.Success>(await coordinator.InspectAsync("workflow-user", Administrator())).Impact;
var result = await coordinator.RemediateAndDeleteAsync(new RoleDeletionRemediationCommand(

View file

@ -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<INotificationSender>();
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<RoleDeletionOperationResult.Blocked>(result);
Assert.NotNull(await store.FindAsync(new() { Id = "workflow-user" }));
await AssertNoRoleNotificationAsync(notificationSender);
}
[Fact]
public async Task SuccessfulDeletionPublishesDeletedRoleNotification()
{
var notificationSender = Substitute.For<INotificationSender>();
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<RoleDeletionOperationResult.Deleted>(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<INotificationSender>();
var (_, coordinator) = await CreateCoordinatorAsync(
new StubContributor([]),
notificationSender,
inner => new RoleStoreThatDeletesNothing(inner));
var result = await coordinator.DeleteAsync("workflow-user", Administrator());
Assert.IsType<RoleDeletionOperationResult.NotFound>(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<INotificationSender>();
var (store, coordinator) = await CreateCoordinatorAsync(
new StubContributor([]),
notificationSender,
inner => new LegacyRoleStore(inner));
var result = await coordinator.DeleteAsync("workflow-user", Administrator());
Assert.IsType<RoleDeletionOperationResult.Deleted>(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<INotificationSender>();
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<INotificationSender>();
var (store, coordinator) = await CreateCoordinatorAsync(contributor, notificationSender);
var impact = Assert.IsType<RoleDeletionInspectionResult.Success>(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<INotificationSender>();
var (store, coordinator) = await CreateCoordinatorAsync(contributor, notificationSender);
var impact = Assert.IsType<RoleDeletionInspectionResult.Success>(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<MemoryRoleStore, IRoleStore>? storeDecorator = null)
{
var store = new MemoryRoleStore(new MemoryStore<Role>(), 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<INotificationSender>(), 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<RoleChanged>(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<CancellationToken>(token => !token.CanBeCanceled));
private static async Task AssertNoRoleNotificationAsync(INotificationSender notificationSender) =>
await notificationSender.DidNotReceive().SendAsync(Arg.Any<RoleChanged>(), Arg.Any<CancellationToken>());
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}"));
}
/// <summary>Offers only <see cref="IRoleStore"/>, standing in for a store that predates atomic deletion.</summary>
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<Role?> FindAsync(RoleFilter filter, CancellationToken cancellationToken = default) => Inner.FindAsync(filter, cancellationToken);
public Task<IEnumerable<Role>> FindManyAsync(RoleFilter filter, CancellationToken cancellationToken = default) => Inner.FindManyAsync(filter, cancellationToken);
}
/// <summary>Finds the role but reports that the delete removed nothing, as the loser of a race would.</summary>
private sealed class RoleStoreThatDeletesNothing(MemoryRoleStore inner) : LegacyRoleStore(inner), IRoleStoreWithAtomicDelete
{
public Task<bool> TryDeleteAsync(string roleId, CancellationToken cancellationToken = default) => Task.FromResult(false);
}
/// <summary>Holds every caller at the delete until they have all found the role, then lets them race for real.</summary>
private sealed class RoleStoreThatDeletesInLockstep(MemoryRoleStore inner, int callers) : LegacyRoleStore(inner), IRoleStoreWithAtomicDelete
{
private readonly Barrier _barrier = new(callers);
public Task<bool> 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);
}
}
}