From 060b3b8d0f5dd6ab0a60cc3437cfc6eff16ab1a3 Mon Sep 17 00:00:00 2001 From: Stefan Guilhen Date: Thu, 9 Aug 2018 14:47:02 -0300 Subject: [PATCH] [KEYCLOAK-4902] - Using streams when fetching resources --- .../AbstractPermissionProvider.java | 3 +- .../permission/ScopePolicyProvider.java | 1 + .../StoreFactoryCacheSession.java | 43 ++++++++++- .../jpa/store/JPAResourceStore.java | 50 ++++++------ .../authorization/AuthorizationProvider.java | 10 +++ .../evaluation/DefaultPolicyEvaluator.java | 15 ++-- .../authorization/store/ResourceStore.java | 13 ++++ .../authorization/util/Permissions.java | 27 +++++-- .../testsuite/authz/EntitlementAPITest.java | 77 ++++++++++++++++++- 9 files changed, 196 insertions(+), 43 deletions(-) diff --git a/authz/policy/common/src/main/java/org/keycloak/authorization/policy/provider/permission/AbstractPermissionProvider.java b/authz/policy/common/src/main/java/org/keycloak/authorization/policy/provider/permission/AbstractPermissionProvider.java index bd18379fd0..e806f8b19f 100644 --- a/authz/policy/common/src/main/java/org/keycloak/authorization/policy/provider/permission/AbstractPermissionProvider.java +++ b/authz/policy/common/src/main/java/org/keycloak/authorization/policy/provider/permission/AbstractPermissionProvider.java @@ -44,9 +44,10 @@ public abstract class AbstractPermissionProvider implements PolicyProvider { Map decisions = decisionCache.computeIfAbsent(associatedPolicy, p -> new HashMap<>()); Decision.Effect effect = decisions.get(permission); + defaultEvaluation.setPolicy(associatedPolicy); + if (effect == null) { PolicyProvider policyProvider = authorization.getProvider(associatedPolicy.getType()); - defaultEvaluation.setPolicy(associatedPolicy); policyProvider.evaluate(defaultEvaluation); evaluation.denyIfNoEffect(); decisions.put(permission, defaultEvaluation.getEffect()); diff --git a/authz/policy/common/src/main/java/org/keycloak/authorization/policy/provider/permission/ScopePolicyProvider.java b/authz/policy/common/src/main/java/org/keycloak/authorization/policy/provider/permission/ScopePolicyProvider.java index 930cbbca56..7cc043f7a0 100644 --- a/authz/policy/common/src/main/java/org/keycloak/authorization/policy/provider/permission/ScopePolicyProvider.java +++ b/authz/policy/common/src/main/java/org/keycloak/authorization/policy/provider/permission/ScopePolicyProvider.java @@ -44,6 +44,7 @@ public class ScopePolicyProvider extends AbstractPermissionProvider { if (effect != null) { defaultEvaluation.setEffect(effect); + return; } } diff --git a/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/authorization/StoreFactoryCacheSession.java b/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/authorization/StoreFactoryCacheSession.java index 9d075239d9..687f51ceb1 100644 --- a/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/authorization/StoreFactoryCacheSession.java +++ b/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/authorization/StoreFactoryCacheSession.java @@ -628,6 +628,13 @@ public class StoreFactoryCacheSession implements CachedStoreFactoryProvider { (revision, resources) -> new ResourceListQuery(revision, cacheKey, resources.stream().map(resource -> resource.getId()).collect(Collectors.toSet()), resourceServerId), resourceServerId); } + @Override + public void findByOwner(String ownerId, String resourceServerId, Consumer consumer) { + String cacheKey = getResourceByOwnerCacheKey(ownerId, resourceServerId); + cacheQuery(cacheKey, ResourceListQuery.class, () -> getResourceStoreDelegate().findByOwner(ownerId, resourceServerId), + (revision, resources) -> new ResourceListQuery(revision, cacheKey, resources.stream().map(resource -> resource.getId()).collect(Collectors.toSet()), resourceServerId), resourceServerId, consumer); + } + @Override public List findByUri(String uri, String resourceServerId) { if (uri == null) return null; @@ -667,7 +674,19 @@ public class StoreFactoryCacheSession implements CachedStoreFactoryProvider { (revision, resources) -> new ResourceListQuery(revision, cacheKey, resources.stream().map(resource -> resource.getId()).collect(Collectors.toSet()), resourceServerId), resourceServerId); } + @Override + public void findByType(String type, String resourceServerId, Consumer consumer) { + if (type == null) return; + String cacheKey = getResourceByTypeCacheKey(type, resourceServerId); + cacheQuery(cacheKey, ResourceListQuery.class, () -> getResourceStoreDelegate().findByType(type, resourceServerId), + (revision, resources) -> new ResourceListQuery(revision, cacheKey, resources.stream().map(resource -> resource.getId()).collect(Collectors.toSet()), resourceServerId), resourceServerId, consumer); + } + private List cacheQuery(String cacheKey, Class queryType, Supplier> resultSupplier, BiFunction, Q> querySupplier, String resourceServerId) { + return cacheQuery(cacheKey, queryType, resultSupplier, querySupplier, resourceServerId, null); + } + + private List cacheQuery(String cacheKey, Class queryType, Supplier> resultSupplier, BiFunction, Q> querySupplier, String resourceServerId, Consumer consumer) { Q query = cache.get(cacheKey, queryType); if (query != null) { logger.tracev("cache hit for key: {0}", cacheKey); @@ -679,11 +698,31 @@ public class StoreFactoryCacheSession implements CachedStoreFactoryProvider { if (invalidations.contains(cacheKey)) return model; query = querySupplier.apply(loaded, model); cache.addRevisioned(query, startupRevision); + if (consumer != null) { + for (R resource : model) { + consumer.accept(resource); + } + } return model; } else if (query.isInvalid(invalidations)) { - return resultSupplier.get(); + List result = resultSupplier.get(); + + if (consumer != null) { + for (R resource : result) { + consumer.accept(resource); + } + } + + return result; } else { - return query.getResources().stream().map(resourceId -> (R) findById(resourceId, resourceServerId)).collect(Collectors.toList()); + Set resources = query.getResources(); + + if (consumer != null) { + resources.stream().map(resourceId -> (R) findById(resourceId, resourceServerId)).forEach(consumer); + return Collections.emptyList(); + } + + return resources.stream().map(resourceId -> (R) findById(resourceId, resourceServerId)).collect(Collectors.toList()); } } } diff --git a/model/jpa/src/main/java/org/keycloak/authorization/jpa/store/JPAResourceStore.java b/model/jpa/src/main/java/org/keycloak/authorization/jpa/store/JPAResourceStore.java index c39717e2da..73a77c7d98 100644 --- a/model/jpa/src/main/java/org/keycloak/authorization/jpa/store/JPAResourceStore.java +++ b/model/jpa/src/main/java/org/keycloak/authorization/jpa/store/JPAResourceStore.java @@ -38,6 +38,8 @@ import java.util.ArrayList; import java.util.LinkedList; import java.util.List; import java.util.Map; +import java.util.Objects; +import java.util.function.Consumer; /** * @author Pedro Igor @@ -99,6 +101,15 @@ public class JPAResourceStore implements ResourceStore { @Override public List findByOwner(String ownerId, String resourceServerId) { + List list = new LinkedList<>(); + + findByOwner(ownerId, resourceServerId, list::add); + + return list; + } + + @Override + public void findByOwner(String ownerId, String resourceServerId, Consumer consumer) { String queryName = "findResourceIdByOwner"; if (resourceServerId == null) { @@ -114,19 +125,12 @@ public class JPAResourceStore implements ResourceStore { query.setParameter("serverId", resourceServerId); } - List result = query.getResultList(); - List list = new LinkedList<>(); ResourceStore resourceStore = provider.getStoreFactory().getResourceStore(); - for (String id : result) { - Resource resource = resourceStore.findById(id, resourceServerId); - - if (resource != null) { - list.add(resource); - } - } - - return list; + query.getResultList().stream() + .map(id -> resourceStore.findById(id, resourceServerId)) + .filter(Objects::nonNull) + .forEach(consumer); } @Override @@ -279,6 +283,15 @@ public class JPAResourceStore implements ResourceStore { @Override public List findByType(String type, String resourceServerId) { + List list = new LinkedList<>(); + + findByType(type, resourceServerId, list::add); + + return list; + } + + @Override + public void findByType(String type, String resourceServerId, Consumer consumer) { TypedQuery query = entityManager.createNamedQuery("findResourceIdByType", String.class); query.setFlushMode(FlushModeType.COMMIT); @@ -286,18 +299,11 @@ public class JPAResourceStore implements ResourceStore { query.setParameter("ownerId", resourceServerId); query.setParameter("serverId", resourceServerId); - List result = query.getResultList(); - List list = new LinkedList<>(); ResourceStore resourceStore = provider.getStoreFactory().getResourceStore(); - for (String id : result) { - Resource resource = resourceStore.findById(id, resourceServerId); - - if (resource != null) { - list.add(resource); - } - } - - return list; + query.getResultList().stream() + .map(id -> resourceStore.findById(id, resourceServerId)) + .filter(Objects::nonNull) + .forEach(consumer); } } diff --git a/server-spi-private/src/main/java/org/keycloak/authorization/AuthorizationProvider.java b/server-spi-private/src/main/java/org/keycloak/authorization/AuthorizationProvider.java index 4614fdd31f..26d749644a 100644 --- a/server-spi-private/src/main/java/org/keycloak/authorization/AuthorizationProvider.java +++ b/server-spi-private/src/main/java/org/keycloak/authorization/AuthorizationProvider.java @@ -476,6 +476,11 @@ public final class AuthorizationProvider implements Provider { return delegate.findByOwner(ownerId, resourceServerId); } + @Override + public void findByOwner(String ownerId, String resourceServerId, Consumer consumer) { + delegate.findByOwner(ownerId, resourceServerId, consumer); + } + @Override public List findByUri(String uri, String resourceServerId) { return delegate.findByUri(uri, resourceServerId); @@ -510,6 +515,11 @@ public final class AuthorizationProvider implements Provider { public List findByType(String type, String resourceServerId) { return delegate.findByType(type, resourceServerId); } + + @Override + public void findByType(String type, String resourceServerId, Consumer consumer) { + delegate.findByType(type, resourceServerId, consumer); + } }; } } diff --git a/server-spi-private/src/main/java/org/keycloak/authorization/policy/evaluation/DefaultPolicyEvaluator.java b/server-spi-private/src/main/java/org/keycloak/authorization/policy/evaluation/DefaultPolicyEvaluator.java index 055a5a8827..3d96a1d935 100644 --- a/server-spi-private/src/main/java/org/keycloak/authorization/policy/evaluation/DefaultPolicyEvaluator.java +++ b/server-spi-private/src/main/java/org/keycloak/authorization/policy/evaluation/DefaultPolicyEvaluator.java @@ -18,10 +18,9 @@ package org.keycloak.authorization.policy.evaluation; -import java.util.HashSet; import java.util.List; import java.util.Map; -import java.util.Set; +import java.util.concurrent.atomic.AtomicBoolean; import java.util.function.Consumer; import java.util.stream.Collectors; @@ -61,7 +60,7 @@ public class DefaultPolicyEvaluator implements PolicyEvaluator { return; } - Set verified = new HashSet<>(); + AtomicBoolean verified = new AtomicBoolean(); Consumer policyConsumer = createPolicyEvaluator(permission, authorizationProvider, executionContext, decision, verified, decisionCache); Resource resource = permission.getResource(); @@ -85,7 +84,7 @@ public class DefaultPolicyEvaluator implements PolicyEvaluator { policyStore.findByScopeIds(scopes.stream().map(Scope::getId).collect(Collectors.toList()), null, resourceServer.getId(), policyConsumer); } - if (!verified.isEmpty()) { + if (verified.get()) { decision.onComplete(permission); return; } @@ -97,12 +96,8 @@ public class DefaultPolicyEvaluator implements PolicyEvaluator { } } - private Consumer createPolicyEvaluator(ResourcePermission permission, AuthorizationProvider authorizationProvider, EvaluationContext executionContext, Decision decision, Set verified, Map> decisionCache) { + private Consumer createPolicyEvaluator(ResourcePermission permission, AuthorizationProvider authorizationProvider, EvaluationContext executionContext, Decision decision, AtomicBoolean verified, Map> decisionCache) { return parentPolicy -> { - if (!verified.add(parentPolicy)) { - return; - } - PolicyProvider policyProvider = authorizationProvider.getProvider(parentPolicy.getType()); if (policyProvider == null) { @@ -110,6 +105,8 @@ public class DefaultPolicyEvaluator implements PolicyEvaluator { } policyProvider.evaluate(new DefaultEvaluation(permission, executionContext, parentPolicy, decision, authorizationProvider, decisionCache)); + + verified.compareAndSet(false, true); }; } } diff --git a/server-spi-private/src/main/java/org/keycloak/authorization/store/ResourceStore.java b/server-spi-private/src/main/java/org/keycloak/authorization/store/ResourceStore.java index 78d55db5d2..27bd3e67d2 100644 --- a/server-spi-private/src/main/java/org/keycloak/authorization/store/ResourceStore.java +++ b/server-spi-private/src/main/java/org/keycloak/authorization/store/ResourceStore.java @@ -22,6 +22,7 @@ import org.keycloak.authorization.model.ResourceServer; import java.util.List; import java.util.Map; +import java.util.function.Consumer; /** * A {@link ResourceStore} is responsible to manage the persistence of {@link Resource} instances. @@ -74,6 +75,8 @@ public interface ResourceStore { */ List findByOwner(String ownerId, String resourceServerId); + void findByOwner(String ownerId, String resourceServerId, Consumer consumer); + /** * Finds all {@link Resource} instances with the given uri. * @@ -133,4 +136,14 @@ public interface ResourceStore { * @return a list of resources with the given type */ List findByType(String type, String resourceServerId); + + /** + * Finds all {@link Resource} with the given type. + * + * @param type the type of the resource + * @param resourceServerId the resource server id + * @param consumer the result consumer + * @return a list of resources with the given type + */ + void findByType(String type, String resourceServerId, Consumer consumer); } diff --git a/services/src/main/java/org/keycloak/authorization/util/Permissions.java b/services/src/main/java/org/keycloak/authorization/util/Permissions.java index f25dc12414..7bb38ab497 100644 --- a/services/src/main/java/org/keycloak/authorization/util/Permissions.java +++ b/services/src/main/java/org/keycloak/authorization/util/Permissions.java @@ -25,6 +25,7 @@ import java.util.LinkedList; import java.util.List; import java.util.Map; import java.util.Set; +import java.util.concurrent.atomic.AtomicLong; import java.util.stream.Collectors; import javax.ws.rs.core.Response.Status; @@ -68,17 +69,27 @@ public final class Permissions { StoreFactory storeFactory = authorization.getStoreFactory(); ResourceStore resourceStore = storeFactory.getResourceStore(); Metadata metadata = request.getMetadata(); - long limit = Long.MAX_VALUE; + final AtomicLong limit; if (metadata != null && metadata.getLimit() != null) { - limit = metadata.getLimit(); + limit = new AtomicLong(metadata.getLimit()); + } else { + limit = new AtomicLong(Long.MAX_VALUE); } // obtain all resources where owner is the resource server - resourceStore.findByOwner(resourceServer.getId(), resourceServer.getId()).stream().limit(limit).forEach(resource -> permissions.add(createResourcePermissionsWithScopes(resource, new LinkedList(resource.getScopes()), authorization, request))); + resourceStore.findByOwner(resourceServer.getId(), resourceServer.getId(), resource -> { + if (limit.decrementAndGet() >= 0) { + permissions.add(createResourcePermissionsWithScopes(resource, new LinkedList(resource.getScopes()), authorization, request)); + } + }); // obtain all resources where owner is the current user - resourceStore.findByOwner(identity.getId(), resourceServer.getId()).stream().limit(limit).forEach(resource -> permissions.add(createResourcePermissionsWithScopes(resource, new LinkedList(resource.getScopes()), authorization, request))); + resourceStore.findByOwner(identity.getId(), resourceServer.getId(), resource -> { + if (limit.decrementAndGet() >= 0) { + permissions.add(createResourcePermissionsWithScopes(resource, new LinkedList(resource.getScopes()), authorization, request)); + } + }); // obtain all resources granted to the user via permission tickets (uma) List tickets = storeFactory.getPermissionTicketStore().findGranted(identity.getId(), resourceServer.getId()); @@ -91,10 +102,10 @@ public final class Permissions { if (permission == null) { userManagedPermissions.put(ticket.getResource().getId(), new ResourcePermission(ticket.getResource(), new ArrayList<>(), resourceServer, request.getClaims())); - limit--; + limit.decrementAndGet(); } - if (--limit <= 0) { + if (limit.decrementAndGet() <= 0) { break; } } @@ -117,7 +128,7 @@ public final class Permissions { if (type != null && !resource.getOwner().equals(resourceServer.getId())) { StoreFactory storeFactory = authorization.getStoreFactory(); ResourceStore resourceStore = storeFactory.getResourceStore(); - resourceStore.findByType(type, resourceServer.getId()).forEach(resource1 -> { + resourceStore.findByType(type, resourceServer.getId(), resource1 -> { if (resource1.getOwner().equals(resourceServer.getId())) { for (Scope typeScope : resource1.getScopes()) { if (!scopes.contains(typeScope)) { @@ -137,7 +148,7 @@ public final class Permissions { } return byName; - }).collect(Collectors.toList()); + }).filter(resource.getScopes()::contains).collect(Collectors.toList()); } return new ResourcePermission(resource, scopes, resource.getResourceServer(), request.getClaims()); diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/authz/EntitlementAPITest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/authz/EntitlementAPITest.java index 9287bffdd8..4cd4b0eaf8 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/authz/EntitlementAPITest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/authz/EntitlementAPITest.java @@ -639,7 +639,7 @@ public class EntitlementAPITest extends AbstractAuthzTest { resource = new ResourceRepresentation(); resource.setName(KeycloakModelUtils.generateId()); - resource.addScope("sensors:view", "sensors:update", "sensors:delete"); + resource.addScope("sensors:view", "sensors:update"); resourceIds.add(authorization.resources().create(resource).readEntity(ResourceRepresentation.class).getId()); @@ -695,6 +695,81 @@ public class EntitlementAPITest extends AbstractAuthzTest { } } + @Test + public void testObtainAllEntitlementsForResource() throws Exception { + ClientResource client = getClient(getRealm(), RESOURCE_SERVER_TEST); + AuthorizationResource authorization = client.authorization(); + + JSPolicyRepresentation policy = new JSPolicyRepresentation(); + + policy.setName(KeycloakModelUtils.generateId()); + policy.setCode("$evaluation.grant();"); + + authorization.policies().js().create(policy).close(); + + ResourceRepresentation resource = new ResourceRepresentation(); + + resource.setName(KeycloakModelUtils.generateId()); + resource.addScope("scope:view", "scope:update", "scope:delete"); + + resource = authorization.resources().create(resource).readEntity(ResourceRepresentation.class); + + ResourcePermissionRepresentation permission = new ResourcePermissionRepresentation(); + + permission.setName(KeycloakModelUtils.generateId()); + permission.addResource(resource.getId()); + permission.addPolicy(policy.getName()); + + authorization.permissions().resource().create(permission); + + String accessToken = new OAuthClient().realm("authz-test").clientId(RESOURCE_SERVER_TEST).doGrantAccessTokenRequest("secret", "kolo", "password").getAccessToken(); + AuthzClient authzClient = getAuthzClient(AUTHZ_CLIENT_CONFIG); + AuthorizationRequest request = new AuthorizationRequest(); + + request.addPermission(null, "scope:view", "scope:update", "scope:delete"); + + AuthorizationResponse response = authzClient.authorization(accessToken).authorize(request); + assertNotNull(response.getToken()); + Collection permissions = toAccessToken(response.getToken()).getAuthorization().getPermissions(); + assertEquals(1, permissions.size()); + + for (Permission grantedPermission : permissions) { + assertEquals(resource.getId(), grantedPermission.getResourceId()); + assertEquals(3, grantedPermission.getScopes().size()); + assertTrue(grantedPermission.getScopes().containsAll(Arrays.asList("scope:view"))); + } + + resource.setScopes(new HashSet<>()); + resource.addScope("scope:view", "scope:update"); + + authorization.resources().resource(resource.getId()).update(resource); + + request = new AuthorizationRequest(); + + request.addPermission(null, "scope:view", "scope:update", "scope:delete"); + + response = authzClient.authorization(accessToken).authorize(request); + assertNotNull(response.getToken()); + permissions = toAccessToken(response.getToken()).getAuthorization().getPermissions(); + assertEquals(1, permissions.size()); + + for (Permission grantedPermission : permissions) { + assertEquals(resource.getId(), grantedPermission.getResourceId()); + assertEquals(2, grantedPermission.getScopes().size()); + assertTrue(grantedPermission.getScopes().containsAll(Arrays.asList("scope:view", "scope:update"))); + } + + request = new AuthorizationRequest(); + + request.addPermission(resource.getId(), "scope:view", "scope:update", "scope:delete"); + + for (Permission grantedPermission : permissions) { + assertEquals(resource.getId(), grantedPermission.getResourceId()); + assertEquals(2, grantedPermission.getScopes().size()); + assertTrue(grantedPermission.getScopes().containsAll(Arrays.asList("scope:view", "scope:update"))); + } + } + private void testRptRequestWithResourceName(String configFile) { Metadata metadata = new Metadata();