diff --git a/src/modules/Elsa.ExternalAuthentication/Extensions/ServiceCollectionExtensions.cs b/src/modules/Elsa.ExternalAuthentication/Extensions/ServiceCollectionExtensions.cs index f8988cb57..f4fc41415 100644 --- a/src/modules/Elsa.ExternalAuthentication/Extensions/ServiceCollectionExtensions.cs +++ b/src/modules/Elsa.ExternalAuthentication/Extensions/ServiceCollectionExtensions.cs @@ -1,3 +1,4 @@ +using Elsa.Common.Multitenancy; using Elsa.Extensions; using Elsa.ExternalAuthentication.Contracts; using Elsa.ExternalAuthentication.Options; @@ -58,6 +59,11 @@ public static class ServiceCollectionExtensions services.AddRateLimiter(_ => { }); services.TryAddEnumerable(ServiceDescriptor.Singleton, ConfigureExternalAuthenticationRateLimiterOptions>()); + // The module reads the ambient tenant outside the multitenancy feature -- connection scoping and the + // role-deletion contributor's tenant boundary -- so it depends on an accessor whether or not a host + // enabled multitenancy. TryAdd keeps a host's own registration. + services.TryAddSingleton(); + services.TryAddSingleton(); services.TryAddSingleton(); services.TryAddSingleton(); diff --git a/src/modules/Elsa.ExternalAuthentication/Services/ExternalAuthenticationRoleDeletionDependencyContributor.cs b/src/modules/Elsa.ExternalAuthentication/Services/ExternalAuthenticationRoleDeletionDependencyContributor.cs index 71c9fa3fc..7cf02be7c 100644 --- a/src/modules/Elsa.ExternalAuthentication/Services/ExternalAuthenticationRoleDeletionDependencyContributor.cs +++ b/src/modules/Elsa.ExternalAuthentication/Services/ExternalAuthenticationRoleDeletionDependencyContributor.cs @@ -4,6 +4,7 @@ using System.Text; using System.Text.Json; using System.Text.Json.Nodes; using Elsa.Authorization; +using Elsa.Common.Multitenancy; using Elsa.ExternalAuthentication.Contracts; using Elsa.ExternalAuthentication.Models; using Elsa.ExternalAuthentication.Notifications; @@ -11,12 +12,43 @@ using Elsa.ExternalAuthentication.Options; using Elsa.ExternalAuthentication.Permissions; using Elsa.ExternalAuthentication.Policies; using Elsa.Identity.Contracts; +using Elsa.Identity.Entities; using Elsa.Identity.Models; using Microsoft.Extensions.Options; namespace Elsa.ExternalAuthentication.Services; /// Guards Elsa Role deletion against JIT-policy default-role references. +/// +/// Roles are tenant-scoped, so impact and remediation only ever consider connections in the role's own tenant +/// context: the resolved role's own TenantId. The tenant active on is used +/// only as a fallback when the role cannot be resolved, because with multitenancy disabled (the default) the EF +/// Core role store installs no tenant query filter and can resolve a tenant-owned role by ID regardless of the +/// ambient tenant; trusting the ambient tenant instead of the resolved role's tenant would then let this +/// contributor scan the wrong tenant's connections while the coordinator deletes a role belonging to another +/// tenant. A connection carrying another tenant's ID is out of scope in both directions, for impact and for +/// remediation. +/// Host-scoped connections (, and configuration entries that leave the +/// tenant blank, which are materialized at host scope) stay in scope for every tenant. The connection registry +/// resolves the host scope for every signing-in tenant, and a connection's default role IDs are then resolved by +/// ExternalIdentityUserProvisioningService through in the signing-in user's +/// tenant, so a host connection naming role ID X really does reference tenant A's role X. +/// A tenant-agnostic role () is visible from every tenant, so its tenant +/// context is every tenant: impact and remediation for such a role scan every stored connection and every +/// configuration entry regardless of tenant, instead of the single active tenant plus host scope. Authorizing a +/// replacement role, however, is still performed through the ambient tenant's role services, so when the +/// deletion target is agnostic the replacement role must itself be agnostic; a tenant-scoped replacement is +/// rejected rather than being authorized in one tenant and written into every tenant's connections. +/// In EF Core persistence a role's primary key is its ID alone, so a role ID is unique across all tenants there +/// and an agnostic/tenant-scoped collision cannot exist. Only MemoryRoleStore can hold two roles that +/// share an ID (its storage key includes the tenant); resolving a role ID against it can then be genuinely +/// ambiguous. That ambiguity is never resolved by guessing: widening a tenant-scoped deletion would expose +/// another tenant's references, and narrowing an agnostic deletion would leave an agnostic role's references +/// dangling. It fails closed instead. The same collision makes a replacement candidate ambiguous too: a +/// replacement ID that resolves to more than one role (an ambient match and an agnostic match, under +/// MemoryRoleStore) is rejected as not agnostic rather than guessed at, so it is reported as +/// replacement_role_unavailable_or_unauthorized instead of surfacing as an exception. +/// public sealed class ExternalAuthenticationRoleDeletionDependencyContributor( IIdentityProviderConnectionStore store, IOptionsMonitor options, @@ -25,24 +57,34 @@ public sealed class ExternalAuthenticationRoleDeletionDependencyContributor( IConnectionRegistryVersionStore registryVersions, ConnectionRevisionCalculator revisionCalculator, ExternalAuthenticationSecurityNotifier notifier, - IPermissionEvaluator permissionEvaluator) : IRoleDeletionDependencyContributor + IPermissionEvaluator permissionEvaluator, + ITenantAccessor tenantAccessor) : IRoleDeletionDependencyContributor { public const string SourceName = "external-authentication"; public string Source => SourceName; + /// + /// Match the default DI container's direct-service semantics: when persistence replaces the in-memory + /// store, the last registration is the active store. + /// + private IRoleStore? ActiveRoleStore => roleStores.LastOrDefault(); + public async ValueTask InspectAsync(string roleId, CancellationToken cancellationToken = default) { + var roleTenantId = await ResolveRoleTenantIdAsync(roleId, cancellationToken); var dependencies = new List(); var configuredConnections = options.CurrentValue.ConfigurationConnections ?? []; var configurationIndex = 0; foreach (var connection in configuredConnections) { - dependencies.AddRange(GetConfigurationDependencies(connection, configurationIndex, roleId)); + // The index is part of the configuration path an operator edits, so out-of-scope entries are + // skipped without renumbering the entries that remain. + if (IsInRoleTenantScope(GetConfigurationScopeTenantId(connection), roleTenantId)) + dependencies.AddRange(GetConfigurationDependencies(connection, configurationIndex, roleId)); configurationIndex++; } - var databaseConnections = await store.FindAsync(new(), cancellationToken); - foreach (var connection in databaseConnections.Items) + foreach (var connection in await FindConnectionsInRoleTenantScopeAsync(roleTenantId, cancellationToken)) { if (!TryGetRoleReference(connection.UnlinkedPolicy, roleId, out var policyBranch, out _, out var removesLastDefaultRole)) continue; @@ -91,9 +133,10 @@ public sealed class ExternalAuthenticationRoleDeletionDependencyContributor( if (!expectedOwners.IsSubsetOf(currentOwners)) return new RoleReferenceRemovalValidationResult.Conflict("dependency_changed"); + var roleTenantId = await ResolveRoleTenantIdAsync(request.RoleId, cancellationToken); foreach (var dependency in request.Dependencies) { - var connection = await store.FindByIdAsync(dependency.OwnerId, cancellationToken); + var connection = await FindConnectionInRoleTenantScopeAsync(dependency.OwnerId, roleTenantId, cancellationToken); if (connection is null || connection.Revision != dependency.ExpectedRevision || !TryGetRoleReference(connection.UnlinkedPolicy, request.RoleId, out _, out var roleIds, out _)) return new RoleReferenceRemovalValidationResult.Conflict("connection_revision_changed"); @@ -104,6 +147,19 @@ public sealed class ExternalAuthenticationRoleDeletionDependencyContributor( string.Equals(request.ReplacementRoleId, request.RoleId, StringComparison.Ordinal))) return new RoleReferenceRemovalValidationResult.Forbidden("replacement_role_unavailable_or_unauthorized"); + // Authorization below still resolves through the ambient tenant's role services, so an agnostic + // deletion target may only be replaced by another agnostic role; a tenant-scoped replacement would + // otherwise be authorized in this tenant and then written into every other tenant's connections. + // This does not extend to host-scoped connections: IdentityProviderConnectionManagementService + // forces every managed connection to host scope, and in a deployment without multitenancy roles + // are created scoped to the default tenant rather than agnostic, so requiring an agnostic + // replacement for host-scoped connections would make every replacement remediation impossible in + // the default deployment. + if (requiresReplacement && + string.Equals(roleTenantId, Tenant.AgnosticTenantId, StringComparison.Ordinal) && + !await IsAgnosticRoleAsync(request.ReplacementRoleId, cancellationToken)) + return new RoleReferenceRemovalValidationResult.Forbidden("replacement_role_unavailable_or_unauthorized"); + var rolesToAssign = requiresReplacement ? new[] { request.ReplacementRoleId! } : remainingRoleIds; @@ -128,12 +184,13 @@ public sealed class ExternalAuthenticationRoleDeletionDependencyContributor( if (validation is RoleReferenceRemovalValidationResult.Conflict conflict) return new RoleReferenceRemovalResult.Conflict(conflict.Code, []); + var roleTenantId = await ResolveRoleTenantIdAsync(request.RoleId, cancellationToken); var changedOwnerIds = new List(); try { foreach (var dependency in request.Dependencies.OrderBy(x => x.OwnerId, StringComparer.Ordinal)) { - var connection = await store.FindByIdAsync(dependency.OwnerId, cancellationToken); + var connection = await FindConnectionInRoleTenantScopeAsync(dependency.OwnerId, roleTenantId, cancellationToken); if (connection is null || connection.Revision != dependency.ExpectedRevision || !TryGetRoleReference(connection.UnlinkedPolicy, request.RoleId, out _, out _, out var removesLastDefaultRole)) return new RoleReferenceRemovalResult.Conflict("connection_revision_changed", changedOwnerIds); @@ -147,15 +204,26 @@ public sealed class ExternalAuthenticationRoleDeletionDependencyContributor( if (request.SelectedReferences is not null && removesLastDefaultRole) { - // Match the default DI container's direct-service semantics: when persistence - // replaces the in-memory store, the last registration is the active store. - var roleStore = roleStores.LastOrDefault(); + var roleStore = ActiveRoleStore; if (roleStore is null) return new RoleReferenceRemovalResult.Failed("replacement_role_unavailable_or_unauthorized", changedOwnerIds); var replacement = await roleStore.FindAsync(new() { Id = request.ReplacementRoleId }, cancellationToken); if (replacement is null || !await roleAuthorizationService.CanAssignRolesAsync(request.Actor, [replacement.Id], cancellationToken)) return new RoleReferenceRemovalResult.Failed("replacement_role_unavailable_or_unauthorized", changedOwnerIds); + + // Authorization above still resolves through the ambient tenant's role services, so an + // agnostic deletion target may only be replaced by another agnostic role; a tenant-scoped + // replacement would otherwise be authorized in this tenant and then written into every other + // tenant's connections. This does not extend to host-scoped connections: see the matching + // guard in ValidateRemovalAsync for why. The check is re-run through IsAgnosticRoleAsync + // rather than trusting the TenantId on `replacement` from the FindAsync call above, because + // a same-ID tenant-scoped role added after validation could make that lookup ambiguous; + // IsAgnosticRoleAsync resolves the candidate itself and rejects an ambiguous match instead + // of accepting whichever role FindAsync happened to return. + if (string.Equals(roleTenantId, Tenant.AgnosticTenantId, StringComparison.Ordinal) && + !await IsAgnosticRoleAsync(request.ReplacementRoleId, cancellationToken)) + return new RoleReferenceRemovalResult.Failed("replacement_role_unavailable_or_unauthorized", changedOwnerIds); } var update = await store.UpdateAsync(candidate, connection.Revision, cancellationToken); @@ -191,6 +259,90 @@ public sealed class ExternalAuthenticationRoleDeletionDependencyContributor( return new RoleReferenceRemovalResult.Success(changedOwnerIds); } + /// + /// Resolves the tenant context for the role being deleted: the role's own TenantId + /// ( normalized when the role is tenant-agnostic, in which case its + /// tenant context is every tenant). In EF Core persistence a role ID is unique across all tenants (the + /// Roles table keys on Id alone), so this lookup resolves to at most one role there. Only + /// MemoryRoleStore can hold two roles that share an ID because its storage key includes the tenant; + /// if the ID resolves to more than one role, which tenant's role the coordinator's own delete actually + /// targets is already ambiguous, and this method cannot make the operation consistent by guessing in either + /// direction -- widening would expose another tenant's references for what may be a tenant-scoped deletion, + /// and narrowing would leave an agnostic role's references dangling. It fails closed instead. + /// A missing store or no matching role falls back to the ambient tenant on , + /// which is the only case where the ambient tenant is trusted: the role cannot be resolved at all, so there + /// is no resolved tenant to prefer over it. + /// + private async ValueTask ResolveRoleTenantIdAsync(string roleId, CancellationToken cancellationToken) + { + var roles = await FindRolesByIdAsync(roleId, cancellationToken); + return roles.Length switch + { + 0 => tenantAccessor.TenantId.NormalizeTenantId(), + 1 => roles[0].TenantId.NormalizeTenantId(), + _ => throw new InvalidOperationException( + $"Role '{roleId}' resolves to {roles.Length} roles across tenant scopes; the deletion target is ambiguous and its external-authentication dependencies cannot be determined.") + }; + } + + /// + /// Resolves whether a candidate role ID (typically a replacement role) is itself tenant-agnostic. Unlike + /// , an ambiguous candidate -- a role ID that resolves to more than one + /// role, which only MemoryRoleStore can produce -- is not the coordinator's own deletion target, so + /// there is no operation to fail closed on by throwing; it is instead treated the same as an unresolved + /// candidate and reported as not agnostic, since there is no single resolved role to trust as safe to write + /// into every tenant's connections. + /// + private async ValueTask IsAgnosticRoleAsync(string? roleId, CancellationToken cancellationToken) + { + if (string.IsNullOrWhiteSpace(roleId)) + return false; + var roles = await FindRolesByIdAsync(roleId, cancellationToken); + return roles.Length == 1 && string.Equals(roles[0].TenantId.NormalizeTenantId(), Tenant.AgnosticTenantId, StringComparison.Ordinal); + } + + /// Loads every role matching the given ID from the active role store, or an empty result if none is configured. + private async ValueTask FindRolesByIdAsync(string roleId, CancellationToken cancellationToken) + { + var roleStore = ActiveRoleStore; + if (roleStore is null) + return []; + return (await roleStore.FindManyAsync(new() { Id = roleId }, cancellationToken)).ToArray(); + } + + /// + /// Loads every stored connection in a single snapshot and filters it in memory to the role's tenant context. + /// Composing the result from separate per-scope reads instead would let a connection's TenantId change + /// between those reads ( permits it via UpdateAsync), so the + /// connection could fall between the reads and appear in neither result. A single snapshot has no gap to fall + /// through. + /// + private async ValueTask> FindConnectionsInRoleTenantScopeAsync(string roleTenantId, CancellationToken cancellationToken) + { + var connections = (await store.FindAsync(new(), cancellationToken)).Items; + return connections.Where(x => IsInRoleTenantScope(x.TenantId, roleTenantId)).ToArray(); + } + + /// + /// Loads one dependency's connection, reporting a connection outside the role's tenant context as absent so + /// that a caller-supplied owner ID cannot reach across a tenant boundary. An agnostic role's tenant context + /// is every tenant, so any connection loaded by owner ID qualifies. + /// + private async ValueTask FindConnectionInRoleTenantScopeAsync(string ownerId, string roleTenantId, CancellationToken cancellationToken) + { + var connection = await store.FindByIdAsync(ownerId, cancellationToken); + return connection is not null && IsInRoleTenantScope(connection.TenantId, roleTenantId) ? connection : null; + } + + private static bool IsInRoleTenantScope(string? connectionTenantId, string roleTenantId) => + string.Equals(roleTenantId, Tenant.AgnosticTenantId, StringComparison.Ordinal) || + string.Equals(connectionTenantId, ConnectionScope.HostTenantId, StringComparison.Ordinal) || + string.Equals(connectionTenantId.NormalizeTenantId(), roleTenantId, StringComparison.Ordinal); + + /// A configuration entry that leaves the tenant blank is materialized at host scope. + private static string GetConfigurationScopeTenantId(IdentityProviderConnection connection) => + string.IsNullOrWhiteSpace(connection.TenantId) ? ConnectionScope.HostTenantId : connection.TenantId; + private IEnumerable GetConfigurationDependencies(IdentityProviderConnection connection, int connectionIndex, string roleId) { if (!TryGetRoleReference(connection.UnlinkedPolicy, roleId, out var policyBranch, out var roleIds, out var removesLastDefaultRole)) diff --git a/test/integration/Elsa.ExternalAuthentication.IntegrationTests/Persistence/ExternalAuthenticationPersistenceTests.cs b/test/integration/Elsa.ExternalAuthentication.IntegrationTests/Persistence/ExternalAuthenticationPersistenceTests.cs index c85eecdf0..a5a368e6c 100644 --- a/test/integration/Elsa.ExternalAuthentication.IntegrationTests/Persistence/ExternalAuthenticationPersistenceTests.cs +++ b/test/integration/Elsa.ExternalAuthentication.IntegrationTests/Persistence/ExternalAuthenticationPersistenceTests.cs @@ -150,6 +150,25 @@ public sealed class ExternalAuthenticationPersistenceTests : IAsyncLifetime Assert.Equal(2, Assert.IsType(await store.UpdateAsync(created.Connection, 1)).CurrentRevision); } + [Fact] + public async Task ConnectionStoreReturnsOnlyTheRequestedScope() + { + var store = new EFCoreIdentityProviderConnectionStore(_leaseFactory); + Assert.IsType(await store.CreateAsync(CreateConnection())); + Assert.IsType(await store.CreateAsync(CreateConnection("connection-b", "tenant-b"))); + Assert.IsType(await store.CreateAsync(CreateConnection("connection-host", ConnectionScope.HostTenantId))); + + var tenantScoped = await store.FindAsync(new() { Scope = new(ConnectionScopeKind.Tenant, "tenant-a") }); + var hostScoped = await store.FindAsync(new() { Scope = ConnectionScope.Host }); + var unscoped = await store.FindAsync(new()); + + // The store honors ConnectionFilter.Scope, so callers that query by scope get only that scope's rows; + // a store that accepted and ignored the filter would silently widen their reach. + Assert.Equal(["connection-a"], tenantScoped.Items.Select(x => x.Id).ToArray()); + Assert.Equal(["connection-host"], hostScoped.Items.Select(x => x.Id).ToArray()); + Assert.Equal(3, unscoped.Items.Count); + } + [Fact] public async Task DurableStateGrantSessionAndRegistryVersionOperationsAreSingleUseOrCompareAndSwap() { @@ -754,10 +773,10 @@ public sealed class ExternalAuthenticationPersistenceTests : IAsyncLifetime private static IReadOnlyDictionary> EmptyClaims { get; } = new Dictionary>(); - private static IdentityProviderConnection CreateConnection(string id = "connection-a") => new() + private static IdentityProviderConnection CreateConnection(string id = "connection-a", string tenantId = "tenant-a") => new() { Id = id, - TenantId = "tenant-a", + TenantId = tenantId, Key = "contoso", AdapterType = "openid-connect", AdapterSettingsVersion = 1, diff --git a/test/unit/Elsa.ExternalAuthentication.UnitTests/Foundational/ExternalAuthenticationRoleDeletionDependencyContributorTests.cs b/test/unit/Elsa.ExternalAuthentication.UnitTests/Foundational/ExternalAuthenticationRoleDeletionDependencyContributorTests.cs index 90d0243ef..d8f913aa8 100644 --- a/test/unit/Elsa.ExternalAuthentication.UnitTests/Foundational/ExternalAuthenticationRoleDeletionDependencyContributorTests.cs +++ b/test/unit/Elsa.ExternalAuthentication.UnitTests/Foundational/ExternalAuthenticationRoleDeletionDependencyContributorTests.cs @@ -2,7 +2,10 @@ using Elsa.Authorization; using Elsa.Testing.Shared.Multitenancy; using System.Security.Claims; using System.Text.Json; +using Elsa.Common.Models; +using Elsa.Common.Multitenancy; using Elsa.Common.Services; +using Elsa.ExternalAuthentication.Contracts; using Elsa.ExternalAuthentication.Models; using Elsa.ExternalAuthentication.Options; using Elsa.ExternalAuthentication.Permissions; @@ -165,7 +168,8 @@ public class ExternalAuthenticationRoleDeletionDependencyContributorTests new InMemoryConnectionRegistryVersionStore(), new ConnectionRevisionCalculator(), new ExternalAuthenticationSecurityNotifier(services), - new PermissionEvaluator()); + new PermissionEvaluator(), + TestTenantAccessor.Default); var snapshot = await contributor.InspectAsync("workflow-user"); var request = new RoleReferenceRemovalRequest( "workflow-user", @@ -236,7 +240,8 @@ public class ExternalAuthenticationRoleDeletionDependencyContributorTests versions, new ConnectionRevisionCalculator(), new ExternalAuthenticationSecurityNotifier(services), - new PermissionEvaluator()); + new PermissionEvaluator(), + TestTenantAccessor.Default); 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; @@ -311,6 +316,351 @@ public class ExternalAuthenticationRoleDeletionDependencyContributorTests Assert.IsType(result); } + [Fact] + public async Task ImpactExcludesConnectionsOwnedByAnotherTenant() + { + var ownConnection = Connection("own-connection", CreateUserPolicy("workflow-user"), TenantA); + var otherTenantConnection = Connection("other-tenant-connection", CreateUserPolicy("workflow-user"), TenantB); + var otherTenantConfiguration = Connection("other-tenant-configured", CreateUserPolicy("workflow-user"), TenantB); + var (contributor, _, _) = await CreateContributorAsync( + [otherTenantConfiguration], + [ownConnection, otherTenantConnection], + tenantAccessor: new TestTenantAccessor(TenantA)); + + var snapshot = await contributor.InspectAsync("workflow-user"); + + // Neither tenant B's stored connection nor its configuration entry -- which would block deletion + // outright -- is reported, while the tenant's own reference still is, so the filter is not simply + // reporting nothing. + var dependency = Assert.Single(snapshot.Dependencies); + Assert.Equal(ownConnection.Id, dependency.OwnerId); + Assert.Equal(RoleDeletionDependencyOwnership.Database, dependency.Ownership); + } + + [Fact] + public async Task ImpactIncludesHostScopedConnectionsForEveryTenant() + { + var hostConnection = Connection("host-connection", CreateUserPolicy("workflow-user")); + var hostConfiguration = Connection("host-configured", CreateUserPolicy("workflow-user"), ConnectionScope.DefaultTenantId); + var (contributor, _, _) = await CreateContributorAsync( + [hostConfiguration], + [hostConnection], + tenantAccessor: new TestTenantAccessor(TenantA)); + + var snapshot = await contributor.InspectAsync("workflow-user"); + + // A host connection is served to every signing-in tenant and its default roles are resolved in that + // tenant, so it references this tenant's role. A blank configuration tenant is materialized at host scope. + Assert.Equal( + [hostConfiguration.Id, hostConnection.Id], + snapshot.Dependencies.Select(x => x.OwnerId).Order(StringComparer.Ordinal).ToArray()); + } + + [Fact] + public async Task RemediationCannotReachAnotherTenantsConnection() + { + var ownConnection = Connection("own-connection", CreateUserPolicy("workflow-user", "other-role"), TenantA); + var otherTenantConnection = Connection("other-tenant-connection", CreateUserPolicy("workflow-user", "other-role"), TenantB); + var (contributor, store, _) = await CreateContributorAsync( + [], + [ownConnection, otherTenantConnection], + tenantAccessor: new TestTenantAccessor(TenantA)); + var snapshot = await contributor.InspectAsync("workflow-user"); + var request = new RoleReferenceRemovalRequest( + "workflow-user", + Administrator(), + snapshot.Version, + [ + ..snapshot.Dependencies, + // The owner ID of another tenant's connection, as a caller could supply it. + new RoleDeletionDependency( + ExternalAuthenticationRoleDeletionDependencyContributor.SourceName, + otherTenantConnection.Id, + otherTenantConnection.Key, + "create-user", + RoleDeletionDependencyOwnership.Database, + null, + 1, + false) + ]); + + Assert.IsType(await contributor.ValidateRemovalAsync(request)); + var result = Assert.IsType(await contributor.RemoveEditableReferencesAsync(request)); + + // Nothing may half-run: neither the foreign connection nor the tenant's own connection is touched. + Assert.Empty(result.ChangedOwnerIds); + AssertDefaultRoleIds(await store.FindByIdAsync(otherTenantConnection.Id), "workflow-user", "other-role"); + AssertDefaultRoleIds(await store.FindByIdAsync(ownConnection.Id), "workflow-user", "other-role"); + } + + [Fact] + public async Task RemediationStopsWhenTheConnectionLeavesTheRoleTenantAfterValidation() + { + var ownConnection = Connection("own-connection", CreateUserPolicy("workflow-user", "other-role"), TenantA); + var (contributor, store, _) = await CreateContributorAsync( + [], + [ownConnection], + tenantAccessor: new TestTenantAccessor(TenantA), + decorateStore: inner => new ConnectionStoreThatMovesConnectionToAnotherTenant(inner, ownConnection.Id, TenantB, lookupsBeforeMove: 1)); + var snapshot = await contributor.InspectAsync("workflow-user"); + var request = new RoleReferenceRemovalRequest("workflow-user", Administrator(), snapshot.Version, snapshot.Dependencies); + + var result = Assert.IsType(await contributor.RemoveEditableReferencesAsync(request)); + + Assert.Equal("connection_revision_changed", result.Code); + Assert.Empty(result.ChangedOwnerIds); + AssertDefaultRoleIds(await store.FindByIdAsync(ownConnection.Id), "workflow-user", "other-role"); + } + + [Fact] + public async Task ImpactForAnAgnosticRoleIncludesAConnectionOwnedByAnotherTenant() + { + var ownConnection = Connection("own-connection", CreateUserPolicy("agnostic-role"), TenantA); + var otherTenantConnection = Connection("other-tenant-connection", CreateUserPolicy("agnostic-role"), TenantB); + var (contributor, _, _) = await CreateContributorAsync( + [], + [ownConnection, otherTenantConnection], + additionalRoles: [new Role { Id = "agnostic-role", Name = "Agnostic role", TenantId = Tenant.AgnosticTenantId, Permissions = [] }], + tenantAccessor: new TestTenantAccessor(TenantA)); + + var snapshot = await contributor.InspectAsync("agnostic-role"); + + // The role is visible from every tenant, so its tenant context is every tenant: tenant B's reference is + // reported alongside tenant A's, unlike a tenant-scoped role (see ImpactExcludesConnectionsOwnedByAnotherTenant). + Assert.Equal( + [otherTenantConnection.Id, ownConnection.Id], + snapshot.Dependencies.Select(x => x.OwnerId).Order(StringComparer.Ordinal).ToArray()); + } + + [Fact] + public async Task ImpactIsScopedByTheResolvedRolesTenantRatherThanTheAmbientTenant() + { + // The ambient tenant is the default tenant (no tenant pushed), which is what an EF host runs as with + // multitenancy disabled: the EF role store installs no tenant query filter there and can resolve a + // tenant-owned role by ID regardless of the ambient tenant, unlike MemoryRoleStore, which always + // filters by the ambient tenant itself. RoleStoreWithoutAmbientTenantFilter stands in for that EF + // behavior. The role being deleted belongs to tenant A, so impact must be scoped by that resolved + // tenant, not by the unrelated ambient one. + var ownConnection = Connection("own-connection", CreateUserPolicy("tenant-a-role"), TenantA); + var otherTenantConnection = Connection("other-tenant-connection", CreateUserPolicy("tenant-a-role"), TenantB); + var connectionStore = new InMemoryIdentityProviderConnectionStore(); + Assert.IsType(await connectionStore.CreateAsync(ownConnection)); + Assert.IsType(await connectionStore.CreateAsync(otherTenantConnection)); + var roleStore = new RoleStoreWithoutAmbientTenantFilter( + [new Role { Id = "tenant-a-role", Name = "Tenant A role", TenantId = TenantA, Permissions = [] }]); + var roleAuthorizationService = new RoleAuthorizationService(new StoreBasedRoleProvider(roleStore), new PermissionEvaluator()); + var services = new ServiceCollection().BuildServiceProvider(); + var contributor = new ExternalAuthenticationRoleDeletionDependencyContributor( + connectionStore, + new MutableOptionsMonitor(new ExternalAuthenticationOptions()), + [roleAuthorizationService], + [roleStore], + new InMemoryConnectionRegistryVersionStore(), + new ConnectionRevisionCalculator(), + new ExternalAuthenticationSecurityNotifier(services), + new PermissionEvaluator(), + TestTenantAccessor.Default); + + var snapshot = await contributor.InspectAsync("tenant-a-role"); + + // Scoped by the role's own tenant (A): tenant A's connection is reported, tenant B's is not. Scoping by + // the ambient default tenant instead -- what this test is guarding against -- would report neither. + var dependency = Assert.Single(snapshot.Dependencies); + Assert.Equal(ownConnection.Id, dependency.OwnerId); + Assert.Equal(RoleDeletionDependencyOwnership.Database, dependency.Ownership); + } + + [Fact] + public async Task RoleIdThatResolvesToMoreThanOneRoleAcrossTenantScopesFailsClosed() + { + var ownConnection = Connection("own-connection", CreateUserPolicy("workflow-user"), TenantA); + var otherTenantConnection = Connection("other-tenant-connection", CreateUserPolicy("workflow-user"), TenantB); + var (contributor, _, _) = await CreateContributorAsync( + [], + [ownConnection, otherTenantConnection], + additionalRoles: [new Role { Id = "workflow-user", Name = "Agnostic workflow user", TenantId = Tenant.AgnosticTenantId, Permissions = [] }], + tenantAccessor: new TestTenantAccessor(TenantA)); + + // Tenant A's own "workflow-user" role and an agnostic role sharing that same ID both exist in the + // in-memory role store, so the deletion target is ambiguous: the contributor cannot determine whether + // to scope its inspection and remediation to tenant A alone or to every tenant, and must fail closed + // rather than guess in either direction. + await Assert.ThrowsAsync(() => contributor.InspectAsync("workflow-user").AsTask()); + + var request = new RoleReferenceRemovalRequest( + "workflow-user", + Administrator(), + "irrelevant-version", + [ + new RoleDeletionDependency( + ExternalAuthenticationRoleDeletionDependencyContributor.SourceName, + ownConnection.Id, + ownConnection.Key, + "create-user", + RoleDeletionDependencyOwnership.Database, + null, + 1, + false) + ]); + await Assert.ThrowsAsync(() => contributor.ValidateRemovalAsync(request).AsTask()); + await Assert.ThrowsAsync(() => contributor.RemoveEditableReferencesAsync(request).AsTask()); + } + + [Fact] + public async Task RemediationOfAnAgnosticRoleCanRemoveTheReferenceFromAnotherTenantsConnection() + { + var ownConnection = Connection("own-connection", CreateUserPolicy("agnostic-role"), TenantA); + var otherTenantConnection = Connection("other-tenant-connection", CreateUserPolicy("agnostic-role"), TenantB); + var (contributor, store, _) = await CreateContributorAsync( + [], + [ownConnection, otherTenantConnection], + additionalRoles: [new Role { Id = "agnostic-role", Name = "Agnostic role", TenantId = Tenant.AgnosticTenantId, Permissions = [] }], + tenantAccessor: new TestTenantAccessor(TenantA)); + var snapshot = await contributor.InspectAsync("agnostic-role"); + var request = new RoleReferenceRemovalRequest("agnostic-role", Administrator(), snapshot.Version, snapshot.Dependencies); + + Assert.IsType(await contributor.ValidateRemovalAsync(request)); + var result = Assert.IsType(await contributor.RemoveEditableReferencesAsync(request)); + + Assert.Equal( + [otherTenantConnection.Id, ownConnection.Id], + result.ChangedOwnerIds.Order(StringComparer.Ordinal).ToArray()); + AssertDefaultRoleIds(await store.FindByIdAsync(ownConnection.Id)); + AssertDefaultRoleIds(await store.FindByIdAsync(otherTenantConnection.Id)); + } + + [Fact] + public async Task RemediationOfAnAgnosticRoleRejectsATenantScopedReplacementRole() + { + var ownConnection = Connection("own-connection", CreateUserPolicy("agnostic-role"), TenantA); + var otherTenantConnection = Connection("other-tenant-connection", CreateUserPolicy("agnostic-role"), TenantB); + var (contributor, store, _) = await CreateContributorAsync( + [], + [ownConnection, otherTenantConnection], + additionalRoles: + [ + new Role { Id = "agnostic-role", Name = "Agnostic role", TenantId = Tenant.AgnosticTenantId, Permissions = [] }, + new Role { Id = "tenant-a-replacement", Name = "Tenant A replacement", TenantId = TenantA, Permissions = [] } + ], + tenantAccessor: new TestTenantAccessor(TenantA)); + var snapshot = await contributor.InspectAsync("agnostic-role"); + var request = new RoleReferenceRemovalRequest("agnostic-role", Administrator(), snapshot.Version, snapshot.Dependencies) + { + SelectedReferences = snapshot.Dependencies + .Select(x => new RoleDeletionReferenceSelection(ExternalAuthenticationRoleDeletionDependencyContributor.SourceName, x.OwnerId)) + .ToArray(), + ReplacementRoleId = "tenant-a-replacement" + }; + + // Remediation is initiated in tenant A and would resolve the replacement role through tenant A's role + // authorization service alone, even though tenant B's connection is also in scope for this agnostic + // role. Admitting a tenant-A-only replacement would write a role into tenant B's policy that does not + // exist there, so it must be rejected rather than authorized in one tenant and applied to every tenant. + var validation = await contributor.ValidateRemovalAsync(request); + var forbidden = Assert.IsType(validation); + Assert.Equal("replacement_role_unavailable_or_unauthorized", forbidden.Code); + + var result = await contributor.RemoveEditableReferencesAsync(request); + var failed = Assert.IsType(result); + Assert.Equal("replacement_role_unavailable_or_unauthorized", failed.Code); + Assert.Empty(failed.ChangedOwnerIds); + AssertDefaultRoleIds(await store.FindByIdAsync(ownConnection.Id), "agnostic-role"); + AssertDefaultRoleIds(await store.FindByIdAsync(otherTenantConnection.Id), "agnostic-role"); + } + + [Fact] + public async Task RemediationOfAnAgnosticRoleAcceptsAnAgnosticReplacementRole() + { + var ownConnection = Connection("own-connection", CreateUserPolicy("agnostic-role"), TenantA); + var otherTenantConnection = Connection("other-tenant-connection", CreateUserPolicy("agnostic-role"), TenantB); + var (contributor, store, _) = await CreateContributorAsync( + [], + [ownConnection, otherTenantConnection], + additionalRoles: + [ + new Role { Id = "agnostic-role", Name = "Agnostic role", TenantId = Tenant.AgnosticTenantId, Permissions = [] }, + new Role { Id = "agnostic-replacement", Name = "Agnostic replacement", TenantId = Tenant.AgnosticTenantId, Permissions = [] } + ], + tenantAccessor: new TestTenantAccessor(TenantA)); + var snapshot = await contributor.InspectAsync("agnostic-role"); + var request = new RoleReferenceRemovalRequest("agnostic-role", Administrator(), snapshot.Version, snapshot.Dependencies) + { + SelectedReferences = snapshot.Dependencies + .Select(x => new RoleDeletionReferenceSelection(ExternalAuthenticationRoleDeletionDependencyContributor.SourceName, x.OwnerId)) + .ToArray(), + ReplacementRoleId = "agnostic-replacement" + }; + + // An agnostic replacement exists identically in every tenant, so it is safe to write into tenant B's + // policy even though remediation was authorized through tenant A's role services. + Assert.IsType(await contributor.ValidateRemovalAsync(request)); + var result = Assert.IsType(await contributor.RemoveEditableReferencesAsync(request)); + + Assert.Equal( + [otherTenantConnection.Id, ownConnection.Id], + result.ChangedOwnerIds.Order(StringComparer.Ordinal).ToArray()); + AssertDefaultRoleIds(await store.FindByIdAsync(ownConnection.Id), "agnostic-replacement"); + AssertDefaultRoleIds(await store.FindByIdAsync(otherTenantConnection.Id), "agnostic-replacement"); + } + + [Fact] + public async Task RemediationRemovesTheRoleFromAHostScopedConnectionForATenant() + { + var hostConnection = Connection("host-connection", CreateUserPolicy("workflow-user", "other-role")); + var (contributor, store, _) = await CreateContributorAsync( + [], + [hostConnection], + tenantAccessor: new TestTenantAccessor(TenantA)); + var snapshot = await contributor.InspectAsync("workflow-user"); + var request = new RoleReferenceRemovalRequest("workflow-user", Administrator(), snapshot.Version, snapshot.Dependencies); + + Assert.IsType(await contributor.ValidateRemovalAsync(request)); + var result = Assert.IsType(await contributor.RemoveEditableReferencesAsync(request)); + + Assert.Equal([hostConnection.Id], result.ChangedOwnerIds); + AssertDefaultRoleIds(await store.FindByIdAsync(hostConnection.Id), "other-role"); + } + + [Fact] + public async Task ReplacementRoleIdThatResolvesToBothATenantRoleAndAnAgnosticRoleIsRejectedRatherThanThrowing() + { + var ownConnection = Connection("own-connection", CreateUserPolicy("agnostic-role"), TenantA); + var otherTenantConnection = Connection("other-tenant-connection", CreateUserPolicy("agnostic-role"), TenantB); + var (contributor, store, _) = await CreateContributorAsync( + [], + [ownConnection, otherTenantConnection], + additionalRoles: + [ + new Role { Id = "agnostic-role", Name = "Agnostic role", TenantId = Tenant.AgnosticTenantId, Permissions = [] }, + new Role { Id = "ambiguous-replacement", Name = "Ambiguous replacement (tenant A)", TenantId = TenantA, Permissions = [] }, + new Role { Id = "ambiguous-replacement", Name = "Ambiguous replacement (agnostic)", TenantId = Tenant.AgnosticTenantId, Permissions = [] } + ], + tenantAccessor: new TestTenantAccessor(TenantA)); + var snapshot = await contributor.InspectAsync("agnostic-role"); + var request = new RoleReferenceRemovalRequest("agnostic-role", Administrator(), snapshot.Version, snapshot.Dependencies) + { + SelectedReferences = snapshot.Dependencies + .Select(x => new RoleDeletionReferenceSelection(ExternalAuthenticationRoleDeletionDependencyContributor.SourceName, x.OwnerId)) + .ToArray(), + ReplacementRoleId = "ambiguous-replacement" + }; + + // The replacement ID resolves to two roles in the in-memory store (a tenant-A role and an agnostic role + // sharing the same ID), which is exactly the collision ResolveRoleTenantIdAsync fails closed on for a + // deletion target. A replacement candidate is not the coordinator's own deletion target, so this must be + // reported as an ordinary validation failure rather than escape as an exception. + var validation = await contributor.ValidateRemovalAsync(request); + var forbidden = Assert.IsType(validation); + Assert.Equal("replacement_role_unavailable_or_unauthorized", forbidden.Code); + + var result = await contributor.RemoveEditableReferencesAsync(request); + var failed = Assert.IsType(result); + Assert.Equal("replacement_role_unavailable_or_unauthorized", failed.Code); + Assert.Empty(failed.ChangedOwnerIds); + AssertDefaultRoleIds(await store.FindByIdAsync(ownConnection.Id), "agnostic-role"); + AssertDefaultRoleIds(await store.FindByIdAsync(otherTenantConnection.Id), "agnostic-role"); + } + private static Task<(ExternalAuthenticationRoleDeletionDependencyContributor Contributor, InMemoryIdentityProviderConnectionStore Store, InMemoryConnectionRegistryVersionStore Versions)> CreateContributorAsync( IReadOnlyCollection configuredConnections, params IdentityProviderConnection[] databaseConnections) => @@ -319,35 +669,39 @@ public class ExternalAuthenticationRoleDeletionDependencyContributorTests private static async Task<(ExternalAuthenticationRoleDeletionDependencyContributor Contributor, InMemoryIdentityProviderConnectionStore Store, InMemoryConnectionRegistryVersionStore Versions)> CreateContributorAsync( IReadOnlyCollection configuredConnections, IdentityProviderConnection[] databaseConnections, - IReadOnlyCollection? additionalRoles = null) + IReadOnlyCollection? additionalRoles = null, + ITenantAccessor? tenantAccessor = null, + Func? decorateStore = null) { var store = new InMemoryIdentityProviderConnectionStore(); foreach (var connection in databaseConnections) Assert.IsType(await store.CreateAsync(connection)); - var roleStore = new MemoryRoleStore(new MemoryStore(), TestTenantAccessor.Default); - await roleStore.SaveAsync(new Role { Id = "workflow-user", Name = "Workflow user", Permissions = [] }); - await roleStore.SaveAsync(new Role { Id = "other-role", Name = "Other role", Permissions = [] }); + var accessor = tenantAccessor ?? TestTenantAccessor.Default; + var roleStore = new MemoryRoleStore(new MemoryStore(), accessor); + await roleStore.SaveAsync(new Role { Id = "workflow-user", Name = "Workflow user", TenantId = accessor.TenantId, Permissions = [] }); + await roleStore.SaveAsync(new Role { Id = "other-role", Name = "Other role", TenantId = accessor.TenantId, Permissions = [] }); foreach (var role in additionalRoles ?? []) await roleStore.SaveAsync(role); var versions = new InMemoryConnectionRegistryVersionStore(); var services = new ServiceCollection().BuildServiceProvider(); var contributor = new ExternalAuthenticationRoleDeletionDependencyContributor( - store, + decorateStore?.Invoke(store) ?? store, new MutableOptionsMonitor(new ExternalAuthenticationOptions { ConfigurationConnections = configuredConnections.ToList() }), [new RoleAuthorizationService(new StoreBasedRoleProvider(roleStore), new PermissionEvaluator())], [roleStore], versions, new ConnectionRevisionCalculator(), new ExternalAuthenticationSecurityNotifier(services), - new PermissionEvaluator()); + new PermissionEvaluator(), + accessor); return (contributor, store, versions); } - private static IdentityProviderConnection Connection(string id, PolicySelection policy) => new() + private static IdentityProviderConnection Connection(string id, PolicySelection policy, string? tenantId = null) => new() { Id = id, - TenantId = ConnectionScope.HostTenantId, + TenantId = tenantId ?? ConnectionScope.HostTenantId, Key = id, AdapterType = "oidc", AdapterSettingsVersion = 1, @@ -359,6 +713,18 @@ public class ExternalAuthenticationRoleDeletionDependencyContributorTests UpdatedAt = DateTimeOffset.UnixEpoch }; + private static PolicySelection CreateUserPolicy(params string[] defaultRoleIds) => new( + CreateUserUnlinkedIdentityPolicy.PolicyType, + 1, + JsonSerializer.SerializeToElement(new { defaultRoleIds })); + + private static void AssertDefaultRoleIds(IdentityProviderConnection? connection, params string[] expectedRoleIds) => + Assert.Equal( + expectedRoleIds, + Assert.IsType(connection).UnlinkedPolicy!.Settings.GetProperty("defaultRoleIds").EnumerateArray().Select(x => x.GetString()!).ToArray()); + + private const string TenantA = "tenant-a"; + private const string TenantB = "tenant-b"; private const string ConnectionsUpdate = $"{ExternalAuthenticationResourcePermissions.Connections}:{CoreVerbs.Update}"; private const string PoliciesUpdate = $"{ExternalAuthenticationResourcePermissions.Policies}:{CoreVerbs.Update}"; private const string DefaultRolesUpdate = $"{ExternalAuthenticationResourcePermissions.PolicyDefaultRoles}:{CoreVerbs.Update}"; @@ -378,6 +744,57 @@ public class ExternalAuthenticationRoleDeletionDependencyContributorTests .Select(x => new Claim(PermissionNames.ClaimType, x)))); } + /// + /// Reassigns a connection to another tenant once the contributor has read it, which puts the connection + /// outside the role's tenant context between prevalidation and the remediation write. + /// + private sealed class ConnectionStoreThatMovesConnectionToAnotherTenant( + InMemoryIdentityProviderConnectionStore inner, + string connectionId, + string tenantId, + int lookupsBeforeMove) : IIdentityProviderConnectionStore + { + private int _lookups; + + public ValueTask> FindAsync(ConnectionFilter filter, CancellationToken cancellationToken = default) => + inner.FindAsync(filter, cancellationToken); + + public async ValueTask FindByIdAsync(string id, CancellationToken cancellationToken = default) + { + var connection = await inner.FindByIdAsync(id, cancellationToken); + if (connection is not null && string.Equals(id, connectionId, StringComparison.Ordinal) && Interlocked.Increment(ref _lookups) > lookupsBeforeMove) + connection.TenantId = tenantId; + return connection; + } + + public ValueTask CreateAsync(IdentityProviderConnection connection, CancellationToken cancellationToken = default) => + inner.CreateAsync(connection, cancellationToken); + + public ValueTask UpdateAsync(IdentityProviderConnection connection, long expectedRevision, CancellationToken cancellationToken = default) => + inner.UpdateAsync(connection, expectedRevision, cancellationToken); + } + + /// + /// Resolves roles by ID alone, regardless of the ambient tenant, standing in for the EF Core role store + /// with multitenancy disabled: it installs no tenant query filter and can resolve a tenant-owned role by + /// ID no matter which tenant is ambient. cannot exercise that scenario + /// because it always filters by the ambient tenant itself. + /// + private sealed class RoleStoreWithoutAmbientTenantFilter(IReadOnlyCollection roles) : IRoleStore + { + public Task AddAsync(Role role, CancellationToken cancellationToken = default) => throw new NotSupportedException(); + + public Task DeleteAsync(RoleFilter filter, CancellationToken cancellationToken = default) => throw new NotSupportedException(); + + public Task SaveAsync(Role role, CancellationToken cancellationToken = default) => throw new NotSupportedException(); + + public Task FindAsync(RoleFilter filter, CancellationToken cancellationToken = default) => + Task.FromResult(roles.FirstOrDefault(x => x.Id == filter.Id)); + + public Task> FindManyAsync(RoleFilter filter, CancellationToken cancellationToken = default) => + Task.FromResult(roles.Where(x => x.Id == filter.Id)); + } + private sealed class RoleStoreThatRemovesReplacementAfterContributorValidation( MemoryRoleStore inner, string replacementRoleId) : IRoleStore