From 327991bd738d3d1f4e6003de7ee4313020abd378 Mon Sep 17 00:00:00 2001 From: Pedro Igor Date: Thu, 1 Nov 2018 12:37:33 -0300 Subject: [PATCH] [KEYCLOAK-8716] - Issue with caching resolved roles in KeycloakSession --- .../oidc/endpoints/TokenEndpoint.java | 6 +- .../org/keycloak/utils/RoleResolveUtil.java | 32 ++++----- .../authorization/PolicyEnforcerTest.java | 65 ++++++++++++++++--- 3 files changed, 74 insertions(+), 29 deletions(-) diff --git a/services/src/main/java/org/keycloak/protocol/oidc/endpoints/TokenEndpoint.java b/services/src/main/java/org/keycloak/protocol/oidc/endpoints/TokenEndpoint.java index 2447afa115..ed4817ad68 100644 --- a/services/src/main/java/org/keycloak/protocol/oidc/endpoints/TokenEndpoint.java +++ b/services/src/main/java/org/keycloak/protocol/oidc/endpoints/TokenEndpoint.java @@ -1066,6 +1066,8 @@ public class TokenEndpoint { claimTokenFormat = AuthorizationTokenService.CLAIM_TOKEN_FORMAT_ID_TOKEN; } + String subjectToken = formParams.getFirst("subject_token"); + if (accessTokenString == null) { // in case no bearer token is provided, we force client authentication checkClient(); @@ -1073,6 +1075,8 @@ public class TokenEndpoint { // if a claim token is provided, we check if the format is a OpenID Connect IDToken and assume the token represents the identity asking for permissions if (AuthorizationTokenService.CLAIM_TOKEN_FORMAT_ID_TOKEN.equalsIgnoreCase(claimTokenFormat)) { accessTokenString = claimToken; + } else if (subjectToken != null) { + accessTokenString = subjectToken; } else { // Clients need to authenticate in order to obtain a RPT from the server. // In order to support cases where the client is obtaining permissions on its on behalf, we issue a temporary access token @@ -1100,7 +1104,7 @@ public class TokenEndpoint { authorizationRequest.setScope(formParams.getFirst("scope")); authorizationRequest.setAudience(formParams.getFirst("audience")); - authorizationRequest.setSubjectToken(formParams.getFirst("subject_token") != null ? formParams.getFirst("subject_token") : accessTokenString); + authorizationRequest.setSubjectToken(accessTokenString); String submitRequest = formParams.getFirst("submit_request"); diff --git a/services/src/main/java/org/keycloak/utils/RoleResolveUtil.java b/services/src/main/java/org/keycloak/utils/RoleResolveUtil.java index a674bd3084..f6233c98aa 100644 --- a/services/src/main/java/org/keycloak/utils/RoleResolveUtil.java +++ b/services/src/main/java/org/keycloak/utils/RoleResolveUtil.java @@ -49,7 +49,7 @@ public class RoleResolveUtil { * @return can return null (just in case that createIfMissing is false) */ public static AccessToken.Access getResolvedRealmRoles(KeycloakSession session, ClientSessionContext clientSessionCtx, boolean createIfMissing) { - AccessToken rolesToken = getAllCompositeRoles(session, clientSessionCtx); + AccessToken rolesToken = getAndCacheResolvedRoles(session, clientSessionCtx); AccessToken.Access access = rolesToken.getRealmAccess(); if (access == null && createIfMissing) { access = new AccessToken.Access(); @@ -72,7 +72,7 @@ public class RoleResolveUtil { * @return can return null (just in case that createIfMissing is false) */ public static AccessToken.Access getResolvedClientRoles(KeycloakSession session, ClientSessionContext clientSessionCtx, String clientId, boolean createIfMissing) { - AccessToken rolesToken = getAllCompositeRoles(session, clientSessionCtx); + AccessToken rolesToken = getAndCacheResolvedRoles(session, clientSessionCtx); AccessToken.Access access = rolesToken.getResourceAccess(clientId); if (access == null && createIfMissing) { @@ -93,31 +93,25 @@ public class RoleResolveUtil { * @return not-null object (can return empty map) */ public static Map getAllResolvedClientRoles(KeycloakSession session, ClientSessionContext clientSessionCtx) { - return getAllCompositeRoles(session, clientSessionCtx).getResourceAccess(); + return getAndCacheResolvedRoles(session, clientSessionCtx).getResourceAccess(); } + private static AccessToken getAndCacheResolvedRoles(KeycloakSession session, ClientSessionContext clientSessionCtx) { + ClientModel client = clientSessionCtx.getClientSession().getClient(); + String resolvedRolesAttrName = RESOLVED_ROLES_ATTR + ":" + clientSessionCtx.getClientSession().getUserSession().getId() + ":" + client.getId(); + AccessToken token = session.getAttribute(resolvedRolesAttrName, AccessToken.class); - private static AccessToken getAllCompositeRoles(KeycloakSession session, ClientSessionContext clientSessionCtx) { - AccessToken resolvedRoles = session.getAttribute(RESOLVED_ROLES_ATTR, AccessToken.class); - if (resolvedRoles == null) { - resolvedRoles = loadCompositeRoles(session, clientSessionCtx); - session.setAttribute(RESOLVED_ROLES_ATTR, resolvedRoles); + if (token == null) { + token = new AccessToken(); + for (RoleModel role : clientSessionCtx.getRoles()) { + addToToken(token, role); + } + session.setAttribute(resolvedRolesAttrName, token); } - return resolvedRoles; - } - - - private static AccessToken loadCompositeRoles(KeycloakSession session, ClientSessionContext clientSessionCtx) { - Set requestedRoles = clientSessionCtx.getRoles(); - AccessToken token = new AccessToken(); - for (RoleModel role : requestedRoles) { - addToToken(token, role); - } return token; } - private static void addToToken(AccessToken token, RoleModel role) { AccessToken.Access access = null; if (role.getContainer() instanceof RealmModel) { diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/client/authorization/PolicyEnforcerTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/client/authorization/PolicyEnforcerTest.java index 4f792f1ecd..42e9726d91 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/client/authorization/PolicyEnforcerTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/client/authorization/PolicyEnforcerTest.java @@ -72,6 +72,7 @@ import org.keycloak.representations.idm.authorization.AuthorizationResponse; import org.keycloak.representations.idm.authorization.JSPolicyRepresentation; import org.keycloak.representations.idm.authorization.ResourcePermissionRepresentation; import org.keycloak.representations.idm.authorization.ResourceRepresentation; +import org.keycloak.representations.idm.authorization.RolePolicyRepresentation; import org.keycloak.representations.idm.authorization.ScopePermissionRepresentation; import org.keycloak.representations.idm.authorization.ScopeRepresentation; import org.keycloak.testsuite.AbstractKeycloakTest; @@ -98,9 +99,10 @@ public class PolicyEnforcerTest extends AbstractKeycloakTest { .roles(RolesBuilder.create() .realmRole(RoleBuilder.create().name("uma_authorization").build()) .realmRole(RoleBuilder.create().name("uma_protection").build()) + .realmRole(RoleBuilder.create().name("user").build()) ) .user(UserBuilder.create().username("marta").password("password") - .addRoles("uma_authorization", "uma_protection") + .addRoles("uma_authorization", "uma_protection", "user") .role("resource-server-test", "uma_protection")) .user(UserBuilder.create().username("kolo").password("password")) .client(ClientBuilder.create().clientId("resource-server-uma-test") @@ -393,19 +395,62 @@ public class PolicyEnforcerTest extends AbstractKeycloakTest { assertFalse(context.isGranted()); } + @Test + public void testUsingSubjectToken() { + ClientResource clientResource = getClientResource(RESOURCE_SERVER_CLIENT_ID); + ResourceRepresentation resource = createResource(clientResource, "Resource Subject Token", "/api/check-subject-token"); + + ResourcePermissionRepresentation permission = new ResourcePermissionRepresentation(); + + permission.setName(resource.getName() + " Permission"); + permission.addResource(resource.getName()); + permission.addPolicy("Only User Policy"); + + PermissionsResource permissions = clientResource.authorization().permissions(); + permissions.resource().create(permission).close(); + + KeycloakDeployment deployment = KeycloakDeploymentBuilder.build(getAdapterConfiguration("enforcer-bearer-only.json")); + PolicyEnforcer policyEnforcer = deployment.getPolicyEnforcer(); + OIDCHttpFacade httpFacade = createHttpFacade("/api/check-subject-token"); + AuthorizationContext context = policyEnforcer.enforce(httpFacade); + + assertFalse(context.isGranted()); + assertEquals(403, TestResponse.class.cast(httpFacade.getResponse()).getStatus()); + + oauth.realm(REALM_NAME); + oauth.clientId("public-client-test"); + oauth.doLogin("marta", "password"); + + String code = oauth.getCurrentQuery().get(OAuth2Constants.CODE); + OAuthClient.AccessTokenResponse response = oauth.doAccessTokenRequest(code, null); + String token = response.getAccessToken(); + + httpFacade = createHttpFacade("/api/check-subject-token", token); + + context = policyEnforcer.enforce(httpFacade); + assertTrue(context.isGranted()); + } + private void initAuthorizationSettings(ClientResource clientResource) { if (clientResource.authorization().resources().findByName("Resource A").isEmpty()) { - JSPolicyRepresentation policy = new JSPolicyRepresentation(); + JSPolicyRepresentation jsPolicy = new JSPolicyRepresentation(); - policy.setName("Always Grant Policy"); + jsPolicy.setName("Always Grant Policy"); StringBuilder code = new StringBuilder(); code.append("$evaluation.grant();"); - policy.setCode(code.toString()); + jsPolicy.setCode(code.toString()); - clientResource.authorization().policies().js().create(policy); + clientResource.authorization().policies().js().create(jsPolicy).close(); + + RolePolicyRepresentation rolePolicy = new RolePolicyRepresentation(); + + rolePolicy.setName("Only User Policy"); + rolePolicy.addRole("user"); + + clientResource.authorization().policies().role().create(rolePolicy).close(); createResource(clientResource, "Resource A", "/api/resourcea"); @@ -413,9 +458,9 @@ public class PolicyEnforcerTest extends AbstractKeycloakTest { permission.setName("Resource A Permission"); permission.addResource("Resource A"); - permission.addPolicy(policy.getName()); + permission.addPolicy(jsPolicy.getName()); - clientResource.authorization().permissions().resource().create(permission); + clientResource.authorization().permissions().resource().create(permission).close(); } if (clientResource.authorization().resources().findByName("Resource B").isEmpty()) { @@ -429,7 +474,7 @@ public class PolicyEnforcerTest extends AbstractKeycloakTest { policy.setCode(code.toString()); - clientResource.authorization().policies().js().create(policy); + clientResource.authorization().policies().js().create(policy).close(); createResource(clientResource, "Resource B", "/api/resourceb"); @@ -439,7 +484,7 @@ public class PolicyEnforcerTest extends AbstractKeycloakTest { permission.addResource("Resource B"); permission.addPolicy(policy.getName()); - clientResource.authorization().permissions().resource().create(permission); + clientResource.authorization().permissions().resource().create(permission).close(); } } @@ -458,6 +503,8 @@ public class PolicyEnforcerTest extends AbstractKeycloakTest { representation.setId(response.readEntity(ResourceRepresentation.class).getId()); + response.close(); + return representation; }