From 8c05286fd7a7819dc5d1e244297caf4d5f582d1c Mon Sep 17 00:00:00 2001 From: Pedro Igor Date: Thu, 5 Mar 2026 11:20:47 -0300 Subject: [PATCH] Stricter access control for managing permission tickets Closes #46723 Signed-off-by: Pedro Igor --- .../authorization/client/AuthzClient.java | 1 + .../topics/changes/changes-26_5_6.adoc | 11 +++++ .../upgrading/topics/changes/changes.adoc | 8 ++++ .../jpa/store/JPAPermissionTicketStore.java | 25 ++++++++++- .../UserManagedPermissionUtil.java | 2 + .../authorization/model/PermissionTicket.java | 7 ++- .../PolicyEvaluationResponseBuilder.java | 2 + .../AuthorizationTokenService.java | 1 + .../protection/ProtectionService.java | 7 ++- .../permission/PermissionTicketService.java | 4 +- .../account/AbstractRestServiceTest.java | 2 +- .../account/ResourcesRestServiceTest.java | 22 ++++++--- .../authz/AbstractResourceServerTest.java | 5 ++- .../authz/PermissionManagementTest.java | 45 +++++++++++++++++++ 14 files changed, 127 insertions(+), 15 deletions(-) create mode 100644 docs/documentation/upgrading/topics/changes/changes-26_5_6.adoc diff --git a/authz/client/src/main/java/org/keycloak/authorization/client/AuthzClient.java b/authz/client/src/main/java/org/keycloak/authorization/client/AuthzClient.java index 7f903a0fc56c..74d31866f076 100644 --- a/authz/client/src/main/java/org/keycloak/authorization/client/AuthzClient.java +++ b/authz/client/src/main/java/org/keycloak/authorization/client/AuthzClient.java @@ -138,6 +138,7 @@ protected boolean isRetry() { * @return a {@link ProtectionResource} */ public ProtectionResource protection(String userName, String password) { + patSupplier = null; return new ProtectionResource(this.http, this.serverConfiguration, configuration, createPatSupplier(userName, password)); } diff --git a/docs/documentation/upgrading/topics/changes/changes-26_5_6.adoc b/docs/documentation/upgrading/topics/changes/changes-26_5_6.adoc new file mode 100644 index 000000000000..af50dcfddf0a --- /dev/null +++ b/docs/documentation/upgrading/topics/changes/changes-26_5_6.adoc @@ -0,0 +1,11 @@ +// ------------------------ Breaking changes ------------------------ // +== Breaking changes + +Breaking changes are identified as those that might require changes for existing users to their configurations or applications. +In minor or patch releases, {project_name} will only introduce breaking changes to fix bugs. + +=== Stricter access control for managing permission tickets + +In order to improve security, the access control for managing permission tickets has been made stricter where +only users (or service accounts) granted with the `uma-protection` role can manage permission tickets for a given +resource server. The only exception is the resource server itself, which can manage its permission tickets without the `uma-protection` role. diff --git a/docs/documentation/upgrading/topics/changes/changes.adoc b/docs/documentation/upgrading/topics/changes/changes.adoc index 94536a094df4..408c5baf5f06 100644 --- a/docs/documentation/upgrading/topics/changes/changes.adoc +++ b/docs/documentation/upgrading/topics/changes/changes.adoc @@ -1,6 +1,14 @@ [[migration-changes]] == Migration Changes +=== Migrating to 26.5.6 + +include::changes-26_5_6.adoc[leveloffset=2] + +=== Migrating to 26.5.5 + +include::changes-26_5_5.adoc[leveloffset=2] + === Migrating to 26.5.4 include::changes-26_5_4.adoc[leveloffset=2] diff --git a/model/jpa/src/main/java/org/keycloak/authorization/jpa/store/JPAPermissionTicketStore.java b/model/jpa/src/main/java/org/keycloak/authorization/jpa/store/JPAPermissionTicketStore.java index e54b58815bf2..5e65b7e05696 100644 --- a/model/jpa/src/main/java/org/keycloak/authorization/jpa/store/JPAPermissionTicketStore.java +++ b/model/jpa/src/main/java/org/keycloak/authorization/jpa/store/JPAPermissionTicketStore.java @@ -42,6 +42,9 @@ import org.keycloak.authorization.store.PermissionTicketStore; import org.keycloak.authorization.store.ResourceStore; import org.keycloak.common.util.Time; +import org.keycloak.models.ClientModel; +import org.keycloak.models.KeycloakSession; +import org.keycloak.models.UserModel; import org.keycloak.models.utils.KeycloakModelUtils; import static org.keycloak.models.jpa.PaginationUtils.paginateQuery; @@ -82,9 +85,27 @@ private List getPredicates(CriteriaBuilder builder, ResourceServer resourceServer, Map attributes) { List predicates = new ArrayList<>(); + KeycloakSession session = provider.getKeycloakSession(); - if (resourceServer != null) { + if (resourceServer != null && !Boolean.parseBoolean(attributes.get(PermissionTicket.FilterOption.IS_ADMIN))) { predicates.add(builder.equal(root.get("resourceServer").get("id"), resourceServer.getId())); + ClientModel resourceServerClient = session.clients().getClientById(session.getContext().getRealm(), resourceServer.getClientId()); + UserModel currentUser = session.getContext().getUser(); + + if (resourceServerClient.isServiceAccountsEnabled()) { + UserModel serviceAccount = session.users().getServiceAccount(resourceServerClient); + + if (serviceAccount != null && serviceAccount.equals(currentUser)) { + currentUser = null; + } + } + + if (currentUser != null) { + predicates.add(builder.or( + builder.equal(root.get("owner"), currentUser.getId()), + builder.equal(root.get("requester"), currentUser.getId()) + )); + } } attributes.forEach((filterOption, value) -> { @@ -121,6 +142,8 @@ private List getPredicates(CriteriaBuilder builder, case POLICY_IS_NOT_NULL: predicates.add(builder.isNotNull(root.get("policy"))); break; + case IS_ADMIN: + break; default: throw new IllegalArgumentException("Unsupported filter [" + filterOption + "]"); } diff --git a/server-spi-private/src/main/java/org/keycloak/authorization/UserManagedPermissionUtil.java b/server-spi-private/src/main/java/org/keycloak/authorization/UserManagedPermissionUtil.java index a9c3fa9572f3..030115f83f58 100644 --- a/server-spi-private/src/main/java/org/keycloak/authorization/UserManagedPermissionUtil.java +++ b/server-spi-private/src/main/java/org/keycloak/authorization/UserManagedPermissionUtil.java @@ -21,6 +21,7 @@ import java.util.Map; import org.keycloak.authorization.model.PermissionTicket; +import org.keycloak.authorization.model.PermissionTicket.FilterOption; import org.keycloak.authorization.model.Policy; import org.keycloak.authorization.model.ResourceServer; import org.keycloak.authorization.model.Scope; @@ -81,6 +82,7 @@ public static void removePolicy(PermissionTicket ticket, StoreFactory storeFacto filter.put(PermissionTicket.FilterOption.REQUESTER, ticket.getRequester()); filter.put(PermissionTicket.FilterOption.RESOURCE_ID, ticket.getResource().getId()); filter.put(PermissionTicket.FilterOption.GRANTED, Boolean.TRUE.toString()); + filter.put(FilterOption.IS_ADMIN, Boolean.TRUE.toString()); List tickets = storeFactory.getPermissionTicketStore().find(ticket.getResourceServer(), filter, null, null); diff --git a/server-spi-private/src/main/java/org/keycloak/authorization/model/PermissionTicket.java b/server-spi-private/src/main/java/org/keycloak/authorization/model/PermissionTicket.java index 42846a4998d6..2f6eaca450bd 100644 --- a/server-spi-private/src/main/java/org/keycloak/authorization/model/PermissionTicket.java +++ b/server-spi-private/src/main/java/org/keycloak/authorization/model/PermissionTicket.java @@ -32,8 +32,11 @@ public static enum FilterOption { REQUESTER("requester"), REQUESTER_IS_NULL("requester_is_null"), POLICY_IS_NOT_NULL("policy_is_not_null"), - POLICY_ID("policy.id") - ; + POLICY_ID("policy.id"), + /** a special filter option to ignore owner and requester checks in order to return any ticket, including those + * that the user is not the owner or requester + */ + IS_ADMIN("is_admin"); private final String name; diff --git a/services/src/main/java/org/keycloak/authorization/admin/representation/PolicyEvaluationResponseBuilder.java b/services/src/main/java/org/keycloak/authorization/admin/representation/PolicyEvaluationResponseBuilder.java index ce0258c6cf40..fc8ebef0e744 100644 --- a/services/src/main/java/org/keycloak/authorization/admin/representation/PolicyEvaluationResponseBuilder.java +++ b/services/src/main/java/org/keycloak/authorization/admin/representation/PolicyEvaluationResponseBuilder.java @@ -35,6 +35,7 @@ import org.keycloak.authorization.common.KeycloakIdentity; import org.keycloak.authorization.fgap.AdminPermissionsSchema; import org.keycloak.authorization.model.PermissionTicket; +import org.keycloak.authorization.model.PermissionTicket.FilterOption; import org.keycloak.authorization.model.Policy; import org.keycloak.authorization.model.ResourceServer; import org.keycloak.authorization.model.Scope; @@ -194,6 +195,7 @@ private static PolicyEvaluationResponse.PolicyResultRepresentation toRepresentat Map filters = new EnumMap<>(PermissionTicket.FilterOption.class); filters.put(PermissionTicket.FilterOption.POLICY_ID, policy.getId()); + filters.put(FilterOption.IS_ADMIN, Boolean.TRUE.toString()); List tickets = authorization.getStoreFactory().getPermissionTicketStore().find(resourceServer, filters, -1, 1); diff --git a/services/src/main/java/org/keycloak/authorization/authorization/AuthorizationTokenService.java b/services/src/main/java/org/keycloak/authorization/authorization/AuthorizationTokenService.java index bfe00951d878..bf25ceb935fb 100644 --- a/services/src/main/java/org/keycloak/authorization/authorization/AuthorizationTokenService.java +++ b/services/src/main/java/org/keycloak/authorization/authorization/AuthorizationTokenService.java @@ -213,6 +213,7 @@ public Response authorize(KeycloakAuthorizationRequest request) { if (identity != null) { event.user(identity.getId()); + request.getKeycloakSession().getContext().setBearerToken(identity.getAccessToken()); } ResourceServer resourceServer = getResourceServer(ticket, request); diff --git a/services/src/main/java/org/keycloak/authorization/protection/ProtectionService.java b/services/src/main/java/org/keycloak/authorization/protection/ProtectionService.java index 648a4262c8b2..a4472b9b394f 100644 --- a/services/src/main/java/org/keycloak/authorization/protection/ProtectionService.java +++ b/services/src/main/java/org/keycloak/authorization/protection/ProtectionService.java @@ -80,7 +80,7 @@ public Object permission() { @Path("/permission/ticket") public Object ticket() { - KeycloakIdentity identity = createIdentity(false); + KeycloakIdentity identity = createIdentity(true); return new PermissionTicketService(identity, getResourceServer(identity), this.authorization); } @@ -100,6 +100,11 @@ private KeycloakIdentity createIdentity(boolean checkProtectionScope) { ClientModel client = realm.getClientById(resourceServer.getClientId()); if (checkProtectionScope) { + if (identity.isResourceServer()) { + // if the identity is the resource server itself, then we don't need to check for uma_protection scope + return identity; + } + if (!identity.hasClientRole(client.getClientId(), "uma_protection")) { throw new ErrorResponseException(OAuthErrorException.INVALID_SCOPE, "Requires uma_protection scope.", Status.FORBIDDEN); } diff --git a/services/src/main/java/org/keycloak/authorization/protection/permission/PermissionTicketService.java b/services/src/main/java/org/keycloak/authorization/protection/permission/PermissionTicketService.java index 5e780dfaf872..c64aa12c3317 100644 --- a/services/src/main/java/org/keycloak/authorization/protection/permission/PermissionTicketService.java +++ b/services/src/main/java/org/keycloak/authorization/protection/permission/PermissionTicketService.java @@ -86,8 +86,8 @@ public Response create(PermissionTicketRepresentation representation) { Resource resource = rstore.findById(resourceServer, representation.getResource()); if (resource == null ) throw new ErrorResponseException("invalid_resource_id", "Resource set with id [" + representation.getResource() + "] does not exists in this server.", Response.Status.BAD_REQUEST); - if (!resource.getOwner().equals(this.identity.getId())) - throw new ErrorResponseException("not_authorised", "permissions for [" + representation.getResource() + "] can be only created by the owner", Response.Status.FORBIDDEN); + if (!identity.isResourceServer() && !resource.getOwner().equals(this.identity.getId())) + throw new ErrorResponseException("not_authorised", "permissions for [" + representation.getResource() + "] can be only created by the owner or by the resource server itself", Response.Status.FORBIDDEN); if (!resource.isOwnerManagedAccess()) throw new ErrorResponseException("invalid_permission", "permission can only be created for resources with user-managed access enabled", Response.Status.BAD_REQUEST); diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/account/AbstractRestServiceTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/account/AbstractRestServiceTest.java index 2d9350b1c456..36f142a3a283 100755 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/account/AbstractRestServiceTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/account/AbstractRestServiceTest.java @@ -50,7 +50,7 @@ public abstract class AbstractRestServiceTest extends AbstractTestRealmKeycloakTest { @Rule - public TokenUtil tokenUtil = new TokenUtil(); + public TokenUtil tokenUtil = new TokenUtil("test-user@localhost", "password"); @Rule public AssertEvents events = new AssertEvents(this); diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/account/ResourcesRestServiceTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/account/ResourcesRestServiceTest.java index 8eece4bdb00f..c71e0a6cdc54 100755 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/account/ResourcesRestServiceTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/account/ResourcesRestServiceTest.java @@ -61,6 +61,7 @@ import com.fasterxml.jackson.core.type.TypeReference; import org.junit.BeforeClass; +import org.junit.Rule; import org.junit.Test; import static org.keycloak.common.util.Encode.encodePathAsIs; @@ -78,6 +79,9 @@ */ public class ResourcesRestServiceTest extends AbstractRestServiceTest { + @Rule + public TokenUtil tokenUtil = new TokenUtil("test-authz-user@localhost", "password"); + private AuthzClient authzClient; private List userNames = new ArrayList<>(Arrays.asList("alice", "jdoe", "bob")); @@ -89,13 +93,17 @@ public static void enabled() { @Override public void configureTestRealm(RealmRepresentation testRealm) { super.configureTestRealm(testRealm); - RealmRepresentation realmRepresentation = testRealm; - realmRepresentation.setUserManagedAccessAllowed(true); + testRealm.setUserManagedAccessAllowed(true); testRealm.getUsers().add(createUser("alice", "password")); testRealm.getUsers().add(createUser("jdoe", "password")); testRealm.getUsers().add(createUser("bob", "password")); + testRealm.getUsers().add(UserBuilder.create().username("test-authz-user@localhost").password("password") + .addRoles("uma_authorization", "uma_protection") + .role("my-resource-server", "uma_protection") + .role("account", AccountRoles.MANAGE_ACCOUNT) + .build()); ClientRepresentation client = ClientBuilder.create() .clientId("my-resource-server") @@ -149,7 +157,7 @@ public void before() { ticket.setResource(resource.getId()); ticket.setScopeName(scope); - authzClient.protection("test-user@localhost", "password").permission().create(ticket); + authzClient.protection("test-authz-user@localhost", "password").permission().create(ticket); } } } @@ -524,12 +532,12 @@ public void testGetPermissionRequests() { PermissionTicketRepresentation ticket = new PermissionTicketRepresentation(); ticket.setGranted(false); - ticket.setOwner("test-user@localhost"); + ticket.setOwner("test-authz-user@localhost"); ticket.setRequesterName(userName); ticket.setResource(resource.getId()); ticket.setScopeName(scope); - authzClient.protection("test-user@localhost", "password").permission().create(ticket); + authzClient.protection("test-authz-user@localhost", "password").permission().create(ticket); } } @@ -588,12 +596,12 @@ public void testApprovePermissionRequest() throws IOException { PermissionTicketRepresentation ticket = new PermissionTicketRepresentation(); ticket.setGranted(false); - ticket.setOwner("test-user@localhost"); + ticket.setOwner("test-authz-user@localhost"); ticket.setRequesterName(userName); ticket.setResource(resource.getId()); ticket.setScopeName(scope); - authzClient.protection("test-user@localhost", "password").permission().create(ticket); + authzClient.protection("test-authz-user@localhost", "password").permission().create(ticket); } } diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/authz/AbstractResourceServerTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/authz/AbstractResourceServerTest.java index cce24f963ac8..3bf77657d85a 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/authz/AbstractResourceServerTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/authz/AbstractResourceServerTest.java @@ -70,7 +70,10 @@ public void addTestRealms(List testRealms) { .user(UserBuilder.create().username("alice").password("password") .addRoles("uma_authorization", "uma_protection") .role("resource-server-test", "uma_protection")) - .user(UserBuilder.create().username("kolo").password("password")) + .user(UserBuilder.create().username("kolo").password("password") + .addRoles("uma_authorization", "uma_protection") + .role("resource-server-test", "uma_protection") + ) .client(ClientBuilder.create().clientId("resource-server-test") .secret("secret") .authorizationServicesEnabled(true) diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/authz/PermissionManagementTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/authz/PermissionManagementTest.java index 4eb78143b9c4..de405316b1fa 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/authz/PermissionManagementTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/authz/PermissionManagementTest.java @@ -327,6 +327,51 @@ public void testSameTicketForSamePermissionRequest() throws Exception { assertNotNull(response.getTicket()); } + @Test + public void testSearchTicket() throws Exception { + ResourceRepresentation resource = addResource("Resource A", "kolo", true); + AuthzClient authzClient = getAuthzClient(); + PermissionResponse response = authzClient.protection("marta", "password").permission().create(new PermissionRequest(resource.getId())); + AuthorizationRequest request = new AuthorizationRequest(); + request.setTicket(response.getTicket()); + request.setClaimToken(authzClient.obtainAccessToken("marta", "password").getToken()); + try { + authzClient.authorization().authorize(request); + } catch (Exception e) { + + } + List tickets = authzClient.protection("kolo", "password").permission().find(resource.getId(), null, null, null, null, true, -1, -1); + assertEquals(1, tickets.size()); + tickets = authzClient.protection("marta", "password").permission().find(resource.getId(), null, null, null, null, true, -1, -1); + assertEquals(1, tickets.size()); + + response = authzClient.protection("alice", "password").permission().create(new PermissionRequest(resource.getId())); + request = new AuthorizationRequest(); + request.setTicket(response.getTicket()); + request.setClaimToken(authzClient.obtainAccessToken("alice", "password").getToken()); + try { + authzClient.authorization().authorize(request); + } catch (Exception e) { + } + // two tickets open for kolo, one for alice and one for marta + tickets = authzClient.protection("kolo", "password").permission().find(resource.getId(), null, null, null, null, true, -1, -1); + assertEquals(2, tickets.size()); + assertEquals(2L, (long) authzClient.protection("kolo", "password").permission().count(resource.getId(), null, null, null, null, true)); + // one ticket for alice + tickets = authzClient.protection("alice", "password").permission().find(resource.getId(), null, null, null, null, true, -1, -1); + assertEquals(1, tickets.size()); + assertEquals(1L, (long) authzClient.protection("alice", "password").permission().count(resource.getId(), null, null, null, null, true)); + PermissionTicketRepresentation aliceTicket = tickets.get(0); + // one ticket for marta + tickets = authzClient.protection("marta", "password").permission().find(resource.getId(), null, null, null, null, true, -1, -1); + assertEquals(1, tickets.size()); + assertEquals(1L, (long) authzClient.protection("marta", "password").permission().count(resource.getId(), null, null, null, null, true)); + // the ticket for alice is different from the ticket for marta + assertFalse(aliceTicket.getId().equals(tickets.get(0).getId())); + + + } + private void assertPersistence(PermissionResponse response, ResourceRepresentation resource, String... scopeNames) throws Exception { String ticket = response.getTicket(); assertNotNull(ticket);