diff --git a/src/modules/Elsa.ExternalAuthentication/Services/IdentityProviderConnectionManagementService.cs b/src/modules/Elsa.ExternalAuthentication/Services/IdentityProviderConnectionManagementService.cs index 5ee7fcd9e..08493eff5 100644 --- a/src/modules/Elsa.ExternalAuthentication/Services/IdentityProviderConnectionManagementService.cs +++ b/src/modules/Elsa.ExternalAuthentication/Services/IdentityProviderConnectionManagementService.cs @@ -492,6 +492,32 @@ public sealed partial class IdentityProviderConnectionManagementService( private async ValueTask ValidatePolicyAsync(IdentityProviderConnection connection, ClaimsPrincipal actor, string targetTenantId, ExternalAuthenticationOptions configuredOptions, ICollection errors, CancellationToken cancellationToken) { + // The roles the candidate would actually assign. A policy that does not create users assigns none, + // and so does no policy at all -- which is what makes switching away from, or clearing, a stored + // create-user fallback a change rather than a no-op. + var defaultRoleIds = connection.UnlinkedPolicy is { } candidate && UsesCreateUserFallback(candidate) + ? Policies.CreateUserUnlinkedIdentityPolicy.ReadRoleIds(candidate.Settings) + : []; + + // Two independent checks, reported separately because they answer different questions. The + // permission asks whether this actor may decide what auto-created users receive; the subset rule + // asks whether these particular roles stay within what the actor already holds. Only the second + // existed, which left the sibling resource guarded on the write path while the roles inside it + // were not -- see #7977. + // + // Gated on the effective set *changing*, not on it being non-empty, and evaluated before the + // null-policy early return. Validation runs on every update, on enabling a connection, and on + // read-only validate, so keying off presence would stop an administrator without this permission + // from editing an unrelated field once anyone had set roles. Evaluating it only for non-null + // policies would be worse: omitting unlinkedPolicy from an update clears a stored fallback and + // drops its role assignments, the same decision as switching it to 'reject'. + // + // The cheap in-memory permission check goes first: the unchanged-roles comparison rebuilds the + // effective registry, and its answer is irrelevant for an actor who holds the permission. + if (!permissionEvaluator.HasPermission(actor, ExternalAuthenticationResourcePermissions.PolicyDefaultRoles, CoreVerbs.Update) + && !await DefaultRolesAreUnchangedAsync(connection, targetTenantId, defaultRoleIds, cancellationToken)) + errors.Add(new("unlinkedPolicy.defaultRoleIds", "forbidden", "Changing the default roles for an unlinked identity policy requires the policy default roles update permission.")); + if (connection.UnlinkedPolicy is not { } policy) return; if (!configuredOptions.UnlinkedIdentityPolicy.AllowDatabaseConnectionOverride) @@ -500,28 +526,6 @@ public sealed partial class IdentityProviderConnectionManagementService( errors.Add(new("unlinkedPolicy", "unavailable", "The selected unlinked identity policy is not installed or allowed.")); else { - // The roles this policy would actually assign. A policy that does not create users assigns none, - // which is what makes switching away from a create-user fallback a change rather than a no-op. - var defaultRoleIds = UsesCreateUserFallback(policy) - ? Policies.CreateUserUnlinkedIdentityPolicy.ReadRoleIds(policy.Settings) - : []; - - // Two independent checks, reported separately because they answer different questions. The - // permission asks whether this actor may decide what auto-created users receive; the subset rule - // asks whether these particular roles stay within what the actor already holds. Only the second - // existed, which left the sibling resource guarded on the write path while the roles inside it - // were not -- see #7977. - // - // Gated on the effective set *changing*, not on it being non-empty, and evaluated outside the - // create-user branch. Validation runs on every update, on enabling a connection, and on - // read-only validate, so keying off presence would stop an administrator without this permission - // from editing an unrelated field once anyone had set roles. Evaluating it only for create-user - // policies would be worse: switching a stored fallback to 'reject' drops its roles, which is a - // decision about what auto-created users receive made without the permission that governs it. - if (!await DefaultRolesAreUnchangedAsync(connection, targetTenantId, defaultRoleIds, cancellationToken) - && !permissionEvaluator.HasPermission(actor, ExternalAuthenticationResourcePermissions.PolicyDefaultRoles, CoreVerbs.Update)) - errors.Add(new("unlinkedPolicy.defaultRoleIds", "forbidden", "Changing the default roles for an unlinked identity policy requires the policy default roles update permission.")); - // The subset rule only has something to say about roles actually being assigned. if (UsesCreateUserFallback(policy) && !await roleAuthorizationService.CanAssignRolesAsync(actor, defaultRoleIds, cancellationToken)) errors.Add(new("unlinkedPolicy.defaultRoleIds", "forbidden", "The selected default roles are unavailable or grant permissions the actor cannot delegate.")); diff --git a/test/integration/Elsa.ExternalAuthentication.IntegrationTests/Connections/ConnectionManagementTests.cs b/test/integration/Elsa.ExternalAuthentication.IntegrationTests/Connections/ConnectionManagementTests.cs index d51f2c34d..895890677 100644 --- a/test/integration/Elsa.ExternalAuthentication.IntegrationTests/Connections/ConnectionManagementTests.cs +++ b/test/integration/Elsa.ExternalAuthentication.IntegrationTests/Connections/ConnectionManagementTests.cs @@ -754,6 +754,81 @@ public class ConnectionManagementTests : IAsyncLifetime Assert.Contains("policy default roles update permission", await response.Content.ReadAsStringAsync()); } + [Fact] + public async Task OmittingAStoredCreateUserPolicyStillCountsAsChangingDefaultRoles() + { + // The abandonment guard above works by switching noMatchAction, but a PUT can drop the stored + // fallback more quietly: omit unlinkedPolicy altogether. Normalization does not carry the stored + // policy forward, so a null candidate clears it -- and its role assignments with it. That is the + // same decision as switching to 'reject', so it needs the same permission. + var (id, revision) = await CreateConnectionAsync( + CreateRequest("roles-omitted", unlinkedPolicy: CreateMatcherPolicy("allowed-matcher", "create-user"))); + + _permissions = UpdateWithoutDefaultRolesPermission; + + var response = await PutConnectionAsync(id, revision, CreateRequest("roles-omitted")); + + Assert.Equal(HttpStatusCode.BadRequest, response.StatusCode); + Assert.Contains("policy default roles update permission", await response.Content.ReadAsStringAsync()); + } + + [Fact] + public async Task IntroducingACreateUserPolicyOnAPolicylessConnectionRequiresThePermission() + { + // The reverse transition: the stored connection has no policy, so the baseline role set is empty, + // and an update that introduces a create-user fallback with roles is deciding what auto-created + // users receive. + var (id, revision) = await CreateConnectionAsync(CreateRequest("roles-introduced")); + + _permissions = UpdateWithoutDefaultRolesPermission; + + var response = await PutConnectionAsync(id, revision, + CreateRequest("roles-introduced", unlinkedPolicy: CreateMatcherPolicy("allowed-matcher", "create-user"))); + + Assert.Equal(HttpStatusCode.BadRequest, response.StatusCode); + Assert.Contains("policy default roles update permission", await response.Content.ReadAsStringAsync()); + } + + [Fact] + public async Task ClearingAPolicyThatAssignsNoRolesNeedsNoPermission() + { + // Clearing a create-user fallback whose role list is already empty changes nothing about what + // auto-created users receive, so the guard must stay quiet -- it keys off the effective set + // changing, not off the policy disappearing. + var (id, revision) = await CreateConnectionAsync( + CreateRequest("no-roles-cleared", unlinkedPolicy: CreateMatcherPolicyWithoutDefaultRoles("allowed-matcher", "create-user"))); + + _permissions = UpdateWithoutDefaultRolesPermission; + + var response = await PutConnectionAsync(id, revision, CreateRequest("no-roles-cleared")); + + Assert.True(response.IsSuccessStatusCode, $"expected success, got {(int)response.StatusCode}: {await response.Content.ReadAsStringAsync()}"); + } + + /// May edit connections and policies, but not decide default roles -- the #7977 separation. + private static readonly string[] UpdateWithoutDefaultRolesPermission = + [ + $"{ExternalAuthenticationResourcePermissions.Connections}:{CoreVerbs.Update}", + $"{ExternalAuthenticationResourcePermissions.Policies}:{CoreVerbs.Update}" + ]; + + private async Task<(string Id, string Revision)> CreateConnectionAsync(object request) + { + var created = await _client!.PostAsJsonAsync("/external-authentication/connections", request); + Assert.Equal(HttpStatusCode.Created, created.StatusCode); + return ((await created.Content.ReadFromJsonAsync())!.Id, created.Headers.ETag!.Tag); + } + + private async Task PutConnectionAsync(string id, string revision, object request) + { + using var message = new HttpRequestMessage(HttpMethod.Put, $"/external-authentication/connections/{id}") + { + Content = JsonContent.Create(request) + }; + message.Headers.TryAddWithoutValidation("If-Match", revision); + return await _client!.SendAsync(message); + } + [Fact] public async Task ValidatingAConfigurationOwnedConnectionDoesNotReadItsRolesAsNew() {