SemianiakaVY/elsa-core
1
0
Fork 0
Code Issues Pull requests Actions 1 Packages Projects Releases Wiki Activity

fix(external-auth): keep the default-roles guard when an update omits the policy (#7996)

Browse source 
* fix(external-auth): keep the default-roles guard when an update omits the policy

A PUT that omitted unlinkedPolicy hit ValidatePolicyAsync's null-policy early
return before the PolicyDefaultRoles guard, so an actor holding only
connections:update could clear a stored create-user policy and silently drop
its default-role assignments (refs #7977, #7992). The candidate role set is
now computed before the early return -- empty when the policy is omitted --
so clearing, adding, or switching a policy all count as changing default
roles. The cheap permission check also runs before the registry-backed role
comparison, so the common permitted path skips building the registry.

Adds integration coverage for the omitted-policy transition in both
directions and for clearing a policy that assigns no roles.

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

* test(external-auth): dispose the request message in PutConnectionAsync

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
Sipke Schoorstra 2026-08-27 11:45:44 +02:00 committed by GitHub
parent cf38f0e119
commit 80a5892ecd
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
2 changed files with 101 additions and 22 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

48
src/modules/Elsa.ExternalAuthentication/Services/IdentityProviderConnectionManagementService.cs
View file

@ -492,6 +492,32 @@ public sealed partial class IdentityProviderConnectionManagementService(
private async ValueTask ValidatePolicyAsync(IdentityProviderConnection connection, ClaimsPrincipal actor, string targetTenantId, ExternalAuthenticationOptions configuredOptions, ICollection<ConnectionValidationError> 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."));

75
test/integration/Elsa.ExternalAuthentication.IntegrationTests/Connections/ConnectionManagementTests.cs
View file

@ -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()}");
}
/// <summary>May edit connections and policies, but not decide default roles -- the #7977 separation.</summary>
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<ConnectionDocument>())!.Id, created.Headers.ETag!.Tag);
}
private async Task<HttpResponseMessage> 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()
{