Repository navigation
Reject resourceType on user-managed permissions - #54967
Merged
Merged
Conversation
Closes keycloak#54696 Signed-off-by: Stefan Guilhen <sguilhen@redhat.com>
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
The change affects a cross-owner authorization boundary for a reported CVE, warranting security-maintainer review and integration validation of legacy persisted permissions.
No open issues 🎉
What changed in this PR
This PR hardens UMA user-managed permissions against cross-owner access through resource-type policies, addressing #54696.
Changes:
- Rejects
resourceTypeon user-managed permission creation and updates. - Excludes owner-bound policies from resource-type policy evaluation.
- Adds regression tests for request validation and cross-owner access through legacy policies.
| File | Description |
|---|---|
testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/authz/UserManagedPermissionServiceTest.java |
Tests create/update rejection and denial of cross-owner access through a legacy owner-bound policy. |
services/src/main/java/org/keycloak/authorization/protection/policy/UserManagedPermissionService.java |
Returns HTTP 400 when a user-managed permission specifies resourceType. |
server-spi-private/src/main/java/org/keycloak/authorization/policy/evaluation/DefaultPolicyEvaluator.java |
Filters owner-bound policies out of resource-type policy evaluation. |
Review effort: Balanced
ssilvert
approved these changes
Nov 11, 2026
Contributor
There was a problem hiding this comment.
The fix looks correct to me. One thing needs sorting out before this can go in:
Testsuite Deprecation Check is failing (and with it "Status Check - Keycloak CI"). The PR adds ~122 lines to UserManagedPermissionServiceTest.java, and the check allows at most 100 added lines per file in the deprecated testsuite. The tests need trimming by roughly 25 lines, or moving to the new test framework.
What I checked:
- Write path:
checkRequestrejectsresourceTypeon both create and update. Update validates the same payload, parsed as the same class, that the delegate then persists, so the check and the write cannot disagree. - Read path: skipping owner-bound policies in
evaluateResourceTypePoliciesneutralises policies stored before the fix, with no migration needed. The filter sits outside the cache, so cached results are filtered too. - No regression for legitimate sharing: owner-bound policies (Protection API and granted permission tickets) always carry an explicit resource, so they still apply through the per-resource lookup. Admin-created resource-type permissions have no owner and are unaffected.
- No other route in: the scope-only lookup is limited to
scope-type policies without a resource type, so a legacyumapolicy cannot come back that way.
Optional, non-blocking:
createOwnerBoundResourceTypePolicybuilds a policy with no resource attached, which the real API never produced. It still proves the evaluator filter, but a legacy policy with both a resource and a resource type would be closer to real data.- The new
catch (RuntimeException ignore)blocks use the "ignored" exception. This matches the existing style in the file, so feel free to leave it. - The 400 on
resourceTypeis a behaviour change for Protection API clients; is an upgrade note needed on main, or is that already covered by the release-branch notes?
Contributor
Author
|
Thanks, @ssilvert. As you know, we are in the process of migrating tests to the new test framework. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #54696