feat(external-auth)!: require a permission to author policy default roles (#7992)
* feat(external-auth)!: require a permission to author policy default roles Setting the defaultRoleIds of an unlinked-identity policy was guarded only by the subset rule -- you could not grant roles carrying permissions you did not hold -- so any actor able to edit a connection could decide what auto-created users receive. The permission named for that decision, external-authentication/policies/default-roles:update, was enforced in one place: removing policy references while deleting a role. The asymmetry is what makes this look like a check that was never wired rather than a deliberate carve-out. Its sibling, policies:update, is already enforced on the write path at both the create and update sites, through the same RequiresPolicyManagement condition that covers the very policy the roles live inside. Demonstrated rather than argued: with the guard stubbed out, a caller holding only connections:create and policies:update creates a connection whose policy assigns "workflow-user", and the response is 201. The subset rule does not object, because it answers a different question -- it prevents escalation, not delegation of the decision. The two checks are now reported independently for that reason. The permission asks whether this actor may decide default roles at all; the subset rule asks whether these particular roles stay inside what they already hold. It applies only when roles are actually being set, so clearing the list, or a policy that assigns none, needs nothing extra. Breaking for roles holding the legacy policies:manage but not roles:assign that set default roles today. Anyone who held roles:assign already maps to the new permission and is unaffected. Documented in the migration guide. Closes #7977 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(external-auth): gate default roles on the set changing, not on it existing Review reproduced the over-reach through the real endpoints: validation runs on every update, on enabling a connection, and on read-only validate, so keying the permission off default roles being present meant that once anyone set them, an administrator without the permission could no longer edit an unrelated field on that connection, enable it, or validate it. The permission now applies when the set changes -- adding, removing, or clearing all count as deciding what auto-created users receive; leaving a stored set alone does not. Order is not treated as meaningful, so reordering is not a change. The test that was supposed to cover this asserted only that a message was absent, which passes for any failure response and made it vacuous exactly when it mattered: it passed with the over-reach still in place, because the request was failing 405 on the wrong verb. It now uses PUT and asserts success, and reverting the fix makes it fail with the 400 review described. Refs #7977 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(external-auth): treat abandoning a create-user policy as a role change The permission check sat inside the create-user branch, so it only ran when the candidate policy still created users. Switching a stored fallback to one that does not -- 'reject', or match-user with a different noMatchAction -- skipped it entirely and dropped the policy's automatic role assignments without the permission that governs them. Review reproduced it. The effective default roles of a policy that does not create users are none, so computing that first and comparing outside the branch makes abandonment a change like any other. The subset rule stays inside the branch, because it only has something to say about roles actually being assigned. The new test expresses abandonment through noMatchAction rather than the policy type, since the fixture's registry only knows match-user. Re-scoping the check to create-user candidates makes it fail with OK instead of the expected BadRequest, which is the bypass. Refs #7977 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(external-auth): take the default-role baseline from the registry A configuration-owned connection has no database row, so comparing against the store alone made its configured default roles look newly assigned on every validation. Validation needs only connections:view, so a caller with exactly that could not validate such a connection at all -- review reproduced it. The baseline now comes from the registry, which answers for both ownerships and is the question actually being asked: what does this connection assign today. The store remains a fallback for a record the registry does not know. The new test gives the fixture's configuration connection an unlinked policy with default roles and validates it as a view-only caller. Reverting to the store-only baseline makes it fail with the permission error, which is the symptom review described. Refs #7977 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
Sipke Schoorstra
GitHub
parent
ee40689ef9
commit
8907eeffd2
|
|
@ -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 `<Module>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.
|
||||
|
|
|
|||
|
|
@ -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<ConnectionValidationError> errors, CancellationToken cancellationToken)
|
||||
private async ValueTask ValidatePolicyAsync(IdentityProviderConnection connection, ClaimsPrincipal actor, string targetTenantId, ExternalAuthenticationOptions configuredOptions, ICollection<ConnectionValidationError> 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(
|
|||
}
|
||||
}
|
||||
|
||||
/// <summary>Whether <paramref name="candidateRoleIds"/> matches what the connection already assigns.</summary>
|
||||
/// <remarks>
|
||||
/// 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.
|
||||
/// </remarks>
|
||||
private async ValueTask<bool> DefaultRolesAreUnchangedAsync(IdentityProviderConnection connection, string targetTenantId, IReadOnlyCollection<string> 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) &&
|
||||
|
|
|
|||
|
|
@ -44,6 +44,15 @@ public class ConnectionManagementTests : IAsyncLifetime
|
|||
private IExternalAuthenticationSessionStore _sessions = null!;
|
||||
private INotificationSender _notifications = null!;
|
||||
private bool _unsafePermissionGranted = true;
|
||||
|
||||
/// <summary>
|
||||
/// Overrides the acting principal's permissions for one test.
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// 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.
|
||||
/// </remarks>
|
||||
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<ConnectionDocument>())!.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<ConnectionDocument>())!.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<string>()
|
||||
}));
|
||||
|
||||
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,
|
||||
|
|
|
|||
Loading…
Reference in a new issue