diff --git a/doc/migrations/authorization-model.md b/doc/migrations/authorization-model.md index 304d2a293..f66009b03 100644 --- a/doc/migrations/authorization-model.md +++ b/doc/migrations/authorization-model.md @@ -99,6 +99,27 @@ gives a warning that is easy to suppress and a runtime failure that is not visib Replace each with the module's `ResourcePermissions` constant and a verb. Classes still referenced by their own modules — `WorkflowPermissions`, `IdentityPermissions` and the rest — are untouched. +## Setting default roles now requires its own permission + +Authoring the `defaultRoleIds` of an unlinked-identity policy now requires +`external-authentication/policies/default-roles:update`. Previously only the subset rule applied — you could +not grant roles carrying permissions you did not hold, but any actor who could edit a connection could decide +what auto-created users receive. + +The permission was enforced only when removing policy references during role deletion, while its sibling +`external-authentication/policies:update` was already enforced on the write path. That asymmetry is what this +closes, and it makes "may configure connections, may not decide what auto-created users receive" expressible. + +**Who this affects.** Anyone who held legacy `external-authentication:roles:assign` already maps to the new +permission and is unaffected. The break is for roles holding `external-authentication:policies:manage` +(→ `policies:view` + `policies:update`) but *not* `roles:assign`, which set default roles today. Grant them +`external-authentication/policies/default-roles:update`, or move that responsibility to a role that has it. + +The permission is required when the default-role set **changes** — adding, removing, clearing, or switching +the policy away from one that creates users, which drops its roles just as surely. Leaving a stored set alone +needs nothing extra, so an administrator without the permission can still edit other fields on a connection +whose default roles someone else configured, enable it, or validate it. + ## Third-party modules Modules outside this repository keep compiling. `ConfigurePermissions(params string[])` remains available but obsolete, and a permission that resolves to no registered descriptor registers an implicit one marked unverified, logs a warning, and appears as such in the catalog. The module keeps working and the gap stays visible. diff --git a/src/modules/Elsa.ExternalAuthentication/Services/IdentityProviderConnectionManagementService.cs b/src/modules/Elsa.ExternalAuthentication/Services/IdentityProviderConnectionManagementService.cs index 9cfc668a4..5ee7fcd9e 100644 --- a/src/modules/Elsa.ExternalAuthentication/Services/IdentityProviderConnectionManagementService.cs +++ b/src/modules/Elsa.ExternalAuthentication/Services/IdentityProviderConnectionManagementService.cs @@ -205,7 +205,7 @@ public sealed partial class IdentityProviderConnectionManagementService( if (!adapters.TryGet(connection.AdapterType, out var adapter) || !IsAllowed(configuredOptions.AllowedAdapterTypes, connection.AdapterType)) errors.Add(new("adapterType", "unavailable", "The selected adapter is not installed or is not allowed by this deployment.")); - await ValidatePolicyAsync(connection, actor, configuredOptions, errors, cancellationToken); + await ValidatePolicyAsync(connection, actor, targetTenantId, configuredOptions, errors, cancellationToken); ValidateGrantSources(connection, configuredOptions, errors); if (connection.PermissionGrantSources.Count != 0) { @@ -490,7 +490,7 @@ public sealed partial class IdentityProviderConnectionManagementService( errors.Add(new("claimProjection.redactedClaimTypes", "invalid", "Redacted claim types must also be allowed claim types.")); } - private async ValueTask ValidatePolicyAsync(IdentityProviderConnection connection, ClaimsPrincipal actor, ExternalAuthenticationOptions configuredOptions, ICollection errors, CancellationToken cancellationToken) + private async ValueTask ValidatePolicyAsync(IdentityProviderConnection connection, ClaimsPrincipal actor, string targetTenantId, ExternalAuthenticationOptions configuredOptions, ICollection errors, CancellationToken cancellationToken) { if (connection.UnlinkedPolicy is not { } policy) return; @@ -500,8 +500,30 @@ public sealed partial class IdentityProviderConnectionManagementService( errors.Add(new("unlinkedPolicy", "unavailable", "The selected unlinked identity policy is not installed or allowed.")); else { - if (UsesCreateUserFallback(policy) && - !await roleAuthorizationService.CanAssignRolesAsync(actor, Policies.CreateUserUnlinkedIdentityPolicy.ReadRoleIds(policy.Settings), cancellationToken)) + // 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.")); if (string.Equals(policy.Type, Policies.MatchExternalUserUnlinkedIdentityPolicy.PolicyType, StringComparison.Ordinal) && @@ -513,6 +535,27 @@ public sealed partial class IdentityProviderConnectionManagementService( } } + /// Whether matches what the connection already assigns. + /// + /// The baseline comes from the registry rather than the database store, because a configuration-owned + /// connection has no database record: looking only there made its configured roles read as newly assigned + /// on every validation, so a caller with view access could not validate one at all. The registry answers + /// for both ownerships, which is the question being asked -- what does this connection assign today. + /// + private async ValueTask DefaultRolesAreUnchangedAsync(IdentityProviderConnection connection, string targetTenantId, IReadOnlyCollection candidateRoleIds, CancellationToken cancellationToken) + { + var existing = string.IsNullOrWhiteSpace(connection.Id) + ? null + : (await registry.FindByIdAsync(targetTenantId, connection.Id, cancellationToken))?.Connection + ?? await store.FindByIdAsync(connection.Id, cancellationToken); + var storedRoleIds = existing?.UnlinkedPolicy is { } storedPolicy && UsesCreateUserFallback(storedPolicy) + ? Policies.CreateUserUnlinkedIdentityPolicy.ReadRoleIds(storedPolicy.Settings) + : []; + + // Order is not meaningful in a role set, so a reordering is not a change. + return storedRoleIds.OrderBy(x => x, StringComparer.Ordinal).SequenceEqual(candidateRoleIds.OrderBy(x => x, StringComparer.Ordinal), StringComparer.Ordinal); + } + private static bool UsesCreateUserFallback(PolicySelection policy) => string.Equals(policy.Type, Policies.CreateUserUnlinkedIdentityPolicy.PolicyType, StringComparison.Ordinal) || string.Equals(policy.Type, Policies.MatchExternalUserUnlinkedIdentityPolicy.PolicyType, StringComparison.Ordinal) && diff --git a/test/integration/Elsa.ExternalAuthentication.IntegrationTests/Connections/ConnectionManagementTests.cs b/test/integration/Elsa.ExternalAuthentication.IntegrationTests/Connections/ConnectionManagementTests.cs index f66d106b3..d51f2c34d 100644 --- a/test/integration/Elsa.ExternalAuthentication.IntegrationTests/Connections/ConnectionManagementTests.cs +++ b/test/integration/Elsa.ExternalAuthentication.IntegrationTests/Connections/ConnectionManagementTests.cs @@ -44,6 +44,15 @@ public class ConnectionManagementTests : IAsyncLifetime private IExternalAuthenticationSessionStore _sessions = null!; private INotificationSender _notifications = null!; private bool _unsafePermissionGranted = true; + + /// + /// Overrides the acting principal's permissions for one test. + /// + /// + /// The default is all-or-nothing, which cannot express "may manage policies but may not decide default + /// roles" -- the separation of duties #7977 is about. A test that needs that distinction sets this. + /// + private string[]? _permissions; private string _tenantId = "tenant-a"; public async Task InitializeAsync() @@ -108,7 +117,8 @@ public class ConnectionManagementTests : IAsyncLifetime _app = builder.Build(); _app.Use(async (context, next) => { - context.User = new ClaimsPrincipal(new ClaimsIdentity([new Claim(PermissionNames.ClaimType, _unsafePermissionGranted ? PermissionNames.All : $"{ExternalAuthenticationResourcePermissions.Connections}:{CoreVerbs.Update}")], "test")); + var granted = _permissions ?? [_unsafePermissionGranted ? PermissionNames.All : $"{ExternalAuthenticationResourcePermissions.Connections}:{CoreVerbs.Update}"]; + context.User = new ClaimsPrincipal(new ClaimsIdentity(granted.Select(x => new Claim(PermissionNames.ClaimType, x)), "test")); await next(context); }); _app.UseAuthorization(); @@ -641,6 +651,154 @@ public class ConnectionManagementTests : IAsyncLifetime Assert.Equal(new[] { "workflow-user" }, _roleAuthorizationService.LastRequestedRoleIds); } + [Fact] + public async Task SettingDefaultRolesRequiresThePolicyDefaultRolesPermission() + { + // The actor may create connections and manage policies, but not decide what auto-created users get. + // Before #7977 that was inexpressible: policies:update guarded the policy while the roles inside it + // were guarded only by the subset rule, so any connection administrator could set them. + _permissions = + [ + $"{ExternalAuthenticationResourcePermissions.Connections}:{CoreVerbs.Create}", + $"{ExternalAuthenticationResourcePermissions.Policies}:{CoreVerbs.Update}" + ]; + + var response = await _client!.PostAsJsonAsync( + "/external-authentication/connections", + CreateRequest("roles-guard", 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 HoldingThePolicyDefaultRolesPermissionClearsThatObjection() + { + _permissions = + [ + $"{ExternalAuthenticationResourcePermissions.Connections}:{CoreVerbs.Create}", + $"{ExternalAuthenticationResourcePermissions.Policies}:{CoreVerbs.Update}", + $"{ExternalAuthenticationResourcePermissions.PolicyDefaultRoles}:{CoreVerbs.Update}" + ]; + + var response = await _client!.PostAsJsonAsync( + "/external-authentication/connections", + CreateRequest("roles-allowed", unlinkedPolicy: CreateMatcherPolicy("allowed-matcher", "create-user"))); + + // The subset rule is a separate question and still applies; only this objection must be gone. + Assert.DoesNotContain("policy default roles update permission", await response.Content.ReadAsStringAsync()); + } + + [Fact] + public async Task LeavingStoredDefaultRolesAloneNeedsNoPermission() + { + // Validation runs on every update, on enabling a connection, and on read-only validate. Keying the + // permission off the roles being present rather than changing meant that once anyone set default + // roles, an administrator without it could no longer edit an unrelated field on that connection. + var created = await _client!.PostAsJsonAsync( + "/external-authentication/connections", + CreateRequest("roles-untouched", unlinkedPolicy: CreateMatcherPolicy("allowed-matcher", "create-user"))); + Assert.Equal(HttpStatusCode.Created, created.StatusCode); + var id = (await created.Content.ReadFromJsonAsync())!.Id; + var revision = created.Headers.ETag!.Tag; + + // Now act as someone who may edit connections and policies, but not decide default roles. + _permissions = + [ + $"{ExternalAuthenticationResourcePermissions.Connections}:{CoreVerbs.Update}", + $"{ExternalAuthenticationResourcePermissions.Policies}:{CoreVerbs.Update}" + ]; + + var request = new HttpRequestMessage(HttpMethod.Put, $"/external-authentication/connections/{id}") + { + Content = JsonContent.Create(CreateRequest("roles-untouched", displayName: "Renamed", unlinkedPolicy: CreateMatcherPolicy("allowed-matcher", "create-user"))) + }; + request.Headers.TryAddWithoutValidation("If-Match", revision); + + var response = await _client.SendAsync(request); + + // Asserting the status, not just the absence of a message: DoesNotContain alone passes for any + // failure response, which would make this test vacuous exactly when it matters. + Assert.True(response.IsSuccessStatusCode, $"expected success, got {(int)response.StatusCode}: {await response.Content.ReadAsStringAsync()}"); + } + + [Fact] + public async Task AbandoningACreateUserPolicyStillCountsAsChangingDefaultRoles() + { + // Turning off a stored create-user fallback removes its automatic role assignments. That is a + // decision about what auto-created users receive, so it needs the same permission as editing the + // list -- checking only create-user candidates would have let it through unguarded. Expressed here by + // changing noMatchAction rather than the policy type, because the test registry only knows match-user. + var created = await _client!.PostAsJsonAsync( + "/external-authentication/connections", + CreateRequest("roles-abandoned", unlinkedPolicy: CreateMatcherPolicy("allowed-matcher", "create-user"))); + Assert.Equal(HttpStatusCode.Created, created.StatusCode); + var id = (await created.Content.ReadFromJsonAsync())!.Id; + var revision = created.Headers.ETag!.Tag; + + _permissions = + [ + $"{ExternalAuthenticationResourcePermissions.Connections}:{CoreVerbs.Update}", + $"{ExternalAuthenticationResourcePermissions.Policies}:{CoreVerbs.Update}" + ]; + + var request = new HttpRequestMessage(HttpMethod.Put, $"/external-authentication/connections/{id}") + { + Content = JsonContent.Create(CreateRequest("roles-abandoned", unlinkedPolicy: CreateMatcherPolicy("allowed-matcher", "reject"))) + }; + request.Headers.TryAddWithoutValidation("If-Match", revision); + + var response = await _client.SendAsync(request); + + Assert.Equal(HttpStatusCode.BadRequest, response.StatusCode); + Assert.Contains("policy default roles update permission", await response.Content.ReadAsStringAsync()); + } + + [Fact] + public async Task ValidatingAConfigurationOwnedConnectionDoesNotReadItsRolesAsNew() + { + // A configuration-owned connection has no database row, so taking the baseline from the database + // store alone made its configured roles look newly assigned every time. Validation only needs + // connections:view, so a caller with exactly that could not validate one at all. + var configuration = ConfigurationConnection("config-roles", isEnabled: true); + configuration.UnlinkedPolicy = CreateMatcherPolicy("allowed-matcher", "create-user"); + _registry.ConfigurationConnection = configuration; + + _permissions = [$"{ExternalAuthenticationResourcePermissions.Connections}:{CoreVerbs.View}"]; + + var response = await _client!.PostAsync($"/external-authentication/connections/{configuration.Id}/validate", null); + + Assert.DoesNotContain("policy default roles update permission", await response.Content.ReadAsStringAsync()); + } + + [Fact] + public async Task APolicyThatSetsNoDefaultRolesNeedsNoExtraPermission() + { + // Creating with none decides nothing, so it needs nothing. Changing a stored set -- including + // clearing it -- is deciding, and is covered by the permission. + _permissions = + [ + $"{ExternalAuthenticationResourcePermissions.Connections}:{CoreVerbs.Create}", + $"{ExternalAuthenticationResourcePermissions.Policies}:{CoreVerbs.Update}" + ]; + + var response = await _client!.PostAsJsonAsync( + "/external-authentication/connections", + CreateRequest("roles-empty", unlinkedPolicy: CreateMatcherPolicyWithoutDefaultRoles("allowed-matcher", "create-user"))); + + Assert.DoesNotContain("policy default roles update permission", await response.Content.ReadAsStringAsync()); + } + + private static PolicySelection CreateMatcherPolicyWithoutDefaultRoles(string matcherType, string noMatchAction) => new( + "match-user", + 1, + JsonSerializer.SerializeToElement(new + { + matcher = new { type = matcherType, settingsVersion = 1, settings = new { } }, + noMatchAction, + defaultRoleIds = Array.Empty() + })); + private static object CreateRequest(string key, object? scope = null, string displayName = "Contoso", object? settings = null, bool confirmUnsafeSettings = false, object? unlinkedPolicy = null, string upstreamLogoutMode = "disabled", bool overridesConfigurationConnection = false) => new { key,