[KEYCLOAK-4034] - Minor changes to policy enforcer

adapters/oidc/adapter-core/src/main/java/org/keycloak/adapters/authorization/AbstractPolicyEnforcer.java

@@ -17,6 +17,13 @@
  */
 package org.keycloak.adapters.authorization;
 
+import java.net.URI;
+import java.util.Collections;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
 import org.jboss.logging.Logger;
 import org.keycloak.AuthorizationContext;
 import org.keycloak.KeycloakSecurityContext;
@@ -32,13 +39,6 @@ import org.keycloak.representations.adapters.config.PolicyEnforcerConfig.Enforce
 import org.keycloak.representations.adapters.config.PolicyEnforcerConfig.PathConfig;
 import org.keycloak.representations.idm.authorization.Permission;
 
-import java.net.URI;
-import java.util.ArrayList;
-import java.util.Collections;
-import java.util.HashSet;
-import java.util.List;
-import java.util.Set;
-
 /**
  * @author <a href="mailto:psilva@redhat.com">Pedro Igor</a>
  */
@@ -48,7 +48,7 @@ public abstract class AbstractPolicyEnforcer {
     private final PolicyEnforcerConfig enforcerConfig;
     private final PolicyEnforcer policyEnforcer;
 
-    private List<PathConfig> paths;
+    private Map<String, PathConfig> paths;
     private AuthzClient authzClient;
     private PathMatcher pathMatcher;
 
@@ -57,7 +57,7 @@ public abstract class AbstractPolicyEnforcer {
         this.enforcerConfig = policyEnforcer.getEnforcerConfig();
         this.authzClient = policyEnforcer.getClient();
         this.pathMatcher = new PathMatcher();
-        this.paths = new ArrayList<>(policyEnforcer.getPaths());
+        this.paths = policyEnforcer.getPaths();
     }
 
     public AuthorizationContext authorize(OIDCHttpFacade httpFacade) {
@@ -75,8 +75,7 @@ public abstract class AbstractPolicyEnforcer {
             if (accessToken != null) {
                 Request request = httpFacade.getRequest();
                 Response response = httpFacade.getResponse();
-                String pathInfo = URI.create(request.getURI()).getPath().substring(1);
+                String path = getPath(request);
-                String path = pathInfo.substring(pathInfo.indexOf('/'), pathInfo.length());
                 PathConfig pathConfig = this.pathMatcher.matches(path, this.paths);
 
                 LOGGER.debugf("Checking permissions for path [%s] with config [%s].", request.getURI(), pathConfig);
@@ -122,13 +121,10 @@ public abstract class AbstractPolicyEnforcer {
     protected boolean isAuthorized(PathConfig actualPathConfig, Set<String> requiredScopes, AccessToken accessToken, OIDCHttpFacade httpFacade) {
         Request request = httpFacade.getRequest();
         PolicyEnforcerConfig enforcerConfig = getEnforcerConfig();
-        String accessDeniedPath = enforcerConfig.getOnDenyRedirectTo();
 
-        if (accessDeniedPath != null) {
+        if (isDefaultAccessDeniedUri(request, enforcerConfig)) {
-            if (request.getURI().contains(accessDeniedPath)) {
             return true;
         }
-        }
 
         AccessToken.Authorization authorization = accessToken.getAuthorization();
 
@@ -173,6 +169,17 @@ public abstract class AbstractPolicyEnforcer {
         return false;
     }
 
+    private boolean isDefaultAccessDeniedUri(Request request, PolicyEnforcerConfig enforcerConfig) {
+        String accessDeniedPath = enforcerConfig.getOnDenyRedirectTo();
+
+        if (accessDeniedPath != null) {
+            if (request.getURI().contains(accessDeniedPath)) {
+                return true;
+            }
+        }
+        return false;
+    }
+
     private boolean hasResourceScopePermission(Set<String> requiredScopes, Permission permission, PathConfig actualPathConfig) {
         Set<String> allowedScopes = permission.getScopes();
         return (allowedScopes.containsAll(requiredScopes) || allowedScopes.isEmpty());
@@ -220,27 +227,23 @@ public abstract class AbstractPolicyEnforcer {
     }
 
     private PathConfig resolvePathConfig(PathConfig originalConfig, Request request) {
+        String path = getPath(request);
+
         if (originalConfig.hasPattern()) {
-            String pathInfo = URI.create(request.getURI()).getPath().substring(1);
-            String path = pathInfo.substring(pathInfo.indexOf('/'), pathInfo.length());
             ProtectedResource resource = this.authzClient.protection().resource();
             Set<String> search = resource.findByFilter("uri=" + path);
 
             if (!search.isEmpty()) {
                 // resource does exist on the server, cache it
                 ResourceRepresentation targetResource = resource.findById(search.iterator().next()).getResourceDescription();
-                PathConfig config = new PathConfig();
+                PathConfig config = PolicyEnforcer.createPathConfig(targetResource);
 
-                config.setId(targetResource.getId());
-                config.setName(targetResource.getName());
-                config.setType(targetResource.getType());
-                config.setPath(targetResource.getUri());
                 config.setScopes(originalConfig.getScopes());
                 config.setMethods(originalConfig.getMethods());
                 config.setParentConfig(originalConfig);
                 config.setEnforcementMode(originalConfig.getEnforcementMode());
 
-                this.paths.add(config);
+                this.policyEnforcer.addPath(config);
 
                 return config;
             }
@@ -249,6 +252,11 @@ public abstract class AbstractPolicyEnforcer {
         return originalConfig;
     }
 
+    private String getPath(Request request) {
+        String pathInfo = URI.create(request.getURI()).getPath().substring(1);
+        return pathInfo.substring(pathInfo.indexOf('/'), pathInfo.length());
+    }
+
     private Set<String> getRequiredScopes(PathConfig pathConfig, Request request) {
         Set<String> requiredScopes = new HashSet<>();
 

adapters/oidc/adapter-core/src/main/java/org/keycloak/adapters/authorization/KeycloakAdapterPolicyEnforcer.java

@@ -54,7 +54,6 @@ public class KeycloakAdapterPolicyEnforcer extends AbstractPolicyEnforcer {
         int retry = 2;
         AccessToken original = accessToken;
 
-        while (retry > 0) {
         if (super.isAuthorized(pathConfig, requiredScopes, accessToken, httpFacade)) {
             return true;
         }
@@ -80,7 +79,8 @@ public class KeycloakAdapterPolicyEnforcer extends AbstractPolicyEnforcer {
 
         original.setAuthorization(authorization);
 
-            retry--;
+        if (super.isAuthorized(pathConfig, requiredScopes, accessToken, httpFacade)) {
+            return true;
         }
 
         return false;

adapters/oidc/adapter-core/src/main/java/org/keycloak/adapters/authorization/PathMatcher.java

@@ -19,7 +19,7 @@ package org.keycloak.adapters.authorization;
 
 import org.keycloak.representations.adapters.config.PolicyEnforcerConfig.PathConfig;
 
-import java.util.List;
+import java.util.Map;
 
 /**
  * @author <a href="mailto:psilva@redhat.com">Pedro Igor</a>
@@ -28,10 +28,16 @@ class PathMatcher {
 
     private static final String ANY_RESOURCE_PATTERN = "/*";
 
-    PathConfig matches(final String requestedUri, List<PathConfig> paths) {
+    PathConfig matches(final String requestedUri, Map<String, PathConfig> paths) {
+        PathConfig pathConfig = paths.get(requestedUri);
+
+        if (pathConfig != null) {
+            return pathConfig;
+        }
+
         PathConfig actualConfig = null;
 
-        for (PathConfig entry : paths) {
+        for (PathConfig entry : paths.values()) {
             String protectedUri = entry.getPath();
             String selectedUri = null;
 

adapters/oidc/adapter-core/src/main/java/org/keycloak/adapters/authorization/PolicyEnforcer.java

@@ -34,8 +34,10 @@ import org.keycloak.representations.idm.authorization.Permission;
 
 import java.util.ArrayList;
 import java.util.Collections;
+import java.util.HashMap;
 import java.util.HashSet;
 import java.util.List;
+import java.util.Map;
 import java.util.Set;
 
 /**
@@ -48,7 +50,7 @@ public class PolicyEnforcer {
     private final KeycloakDeployment deployment;
     private final AuthzClient authzClient;
     private final PolicyEnforcerConfig enforcerConfig;
-    private final List<PathConfig> paths;
+    private final Map<String, PathConfig> paths;
 
     public PolicyEnforcer(KeycloakDeployment deployment, AdapterConfig adapterConfig) {
         this.deployment = deployment;
@@ -58,7 +60,7 @@ public class PolicyEnforcer {
 
         if (LOGGER.isDebugEnabled()) {
             LOGGER.debug("Initialization complete. Path configurations:");
-            for (PathConfig pathConfig : this.paths) {
+            for (PathConfig pathConfig : this.paths.values()) {
                 LOGGER.debug(pathConfig);
             }
         }
@@ -96,15 +98,19 @@ public class PolicyEnforcer {
         return authzClient;
     }
 
-    public List<PathConfig> getPaths() {
+    public Map<String, PathConfig> getPaths() {
-        return Collections.unmodifiableList(paths);
+        return paths;
+    }
+
+    void addPath(PathConfig pathConfig) {
+        paths.put(pathConfig.getPath(), pathConfig);
     }
 
     KeycloakDeployment getDeployment() {
         return deployment;
     }
 
-    private List<PathConfig> configurePaths(ProtectedResource protectedResource, PolicyEnforcerConfig enforcerConfig) {
+    private Map<String, PathConfig> configurePaths(ProtectedResource protectedResource, PolicyEnforcerConfig enforcerConfig) {
         boolean loadPathsFromServer = true;
 
         for (PathConfig pathConfig : enforcerConfig.getPaths()) {
@@ -123,8 +129,8 @@ public class PolicyEnforcer {
         }
     }
 
-    private List<PathConfig> configureDefinedPaths(ProtectedResource protectedResource, PolicyEnforcerConfig enforcerConfig) {
+    private Map<String, PathConfig> configureDefinedPaths(ProtectedResource protectedResource, PolicyEnforcerConfig enforcerConfig) {
-        List<PathConfig> paths = new ArrayList<>();
+        Map<String, PathConfig> paths = Collections.synchronizedMap(new HashMap<String, PathConfig>());
 
         for (PathConfig pathConfig : enforcerConfig.getPaths()) {
             Set<String> search;
@@ -172,7 +178,7 @@ public class PolicyEnforcer {
 
             PathConfig existingPath = null;
 
-            for (PathConfig current : paths) {
+            for (PathConfig current : paths.values()) {
                 if (current.getId().equals(pathConfig.getId()) && current.getPath().equals(pathConfig.getPath())) {
                     existingPath = current;
                     break;
@@ -180,7 +186,7 @@ public class PolicyEnforcer {
             }
 
             if (existingPath == null) {
-                paths.add(pathConfig);
+                paths.put(pathConfig.getPath(), pathConfig);
             } else {
                 existingPath.getMethods().addAll(pathConfig.getMethods());
                 existingPath.getScopes().addAll(pathConfig.getScopes());
@@ -190,23 +196,24 @@ public class PolicyEnforcer {
         return paths;
     }
 
-    private List<PathConfig> configureAllPathsForResourceServer(ProtectedResource protectedResource) {
+    private Map<String, PathConfig> configureAllPathsForResourceServer(ProtectedResource protectedResource) {
         LOGGER.info("Querying the server for all resources associated with this application.");
-        List<PathConfig> paths = new ArrayList<>();
+        Map<String, PathConfig> paths = Collections.synchronizedMap(new HashMap<String, PathConfig>());
 
         for (String id : protectedResource.findAll()) {
             RegistrationResponse response = protectedResource.findById(id);
             ResourceRepresentation resourceDescription = response.getResourceDescription();
 
             if (resourceDescription.getUri() != null) {
-                paths.add(createPathConfig(resourceDescription));
+                PathConfig pathConfig = createPathConfig(resourceDescription);
+                paths.put(pathConfig.getPath(), pathConfig);
             }
         }
 
         return paths;
     }
 
-    private PathConfig createPathConfig(ResourceRepresentation resourceDescription) {
+    static PathConfig createPathConfig(ResourceRepresentation resourceDescription) {
         PathConfig pathConfig = new PathConfig();
 
         pathConfig.setId(resourceDescription.getId());

core/src/main/java/org/keycloak/AuthorizationContext.java

@@ -24,6 +24,7 @@ import org.keycloak.representations.idm.authorization.Permission;
 
 import java.util.Collections;
 import java.util.List;
+import java.util.Map;
 
 /**
  * @author <a href="mailto:psilva@redhat.com">Pedro Igor</a>
@@ -31,10 +32,10 @@ import java.util.List;
 public class AuthorizationContext {
 
     private final AccessToken authzToken;
-    private final List<PathConfig> paths;
+    private final Map<String, PathConfig> paths;
     private boolean granted;
 
-    public AuthorizationContext(AccessToken authzToken, List<PathConfig> paths) {
+    public AuthorizationContext(AccessToken authzToken, Map<String, PathConfig> paths) {
         this.authzToken = authzToken;
         this.paths = paths;
         this.granted = true;
@@ -57,7 +58,7 @@ public class AuthorizationContext {
         }
 
         for (Permission permission : authorization.getPermissions()) {
-            for (PathConfig pathHolder : this.paths) {
+            for (PathConfig pathHolder : this.paths.values()) {
                 if (pathHolder.getName().equals(resourceName)) {
                     if (pathHolder.getId().equals(permission.getResourceSetId())) {
                         if (permission.getScopes().contains(scopeName)) {
@@ -83,7 +84,7 @@ public class AuthorizationContext {
         }
 
         for (Permission permission : authorization.getPermissions()) {
-            for (PathConfig pathHolder : this.paths) {
+            for (PathConfig pathHolder : this.paths.values()) {
                 if (pathHolder.getName().equals(resourceName)) {
                     if (pathHolder.getId().equals(permission.getResourceSetId())) {
                         return true;

examples/authz/servlet-authz/src/main/webapp/WEB-INF/web.xml

@@ -13,16 +13,8 @@
 		</web-resource-collection>
 		<auth-constraint>
 			<role-name>user</role-name>
-		</auth-constraint>
-	</security-constraint>
-
-	<security-constraint>
-		<web-resource-collection>
-			<web-resource-name>All Resources</web-resource-name>
-			<url-pattern>/*</url-pattern>
-		</web-resource-collection>
-		<auth-constraint>
 			<role-name>admin</role-name>
+			<role-name>user_premium</role-name>
 		</auth-constraint>
 	</security-constraint>
 
@@ -39,6 +31,10 @@
 		<role-name>user</role-name>
 	</security-role>
 
+	<security-role>
+		<role-name>user_premium</role-name>
+	</security-role>
+
 	<error-page>
 		<error-code>403</error-code>
 		<location>/accessDenied.jsp</location>

services/src/main/java/org/keycloak/authorization/admin/representation/PolicyEvaluationResponse.java

@@ -64,7 +64,7 @@ public class PolicyEvaluationResponse {
         AccessToken accessToken = identity.getAccessToken();
         AccessToken.Authorization authorizationData = new AccessToken.Authorization();
 
-        authorizationData.setPermissions(Permissions.permits(results, authorization, resourceServer.getId()));
+        authorizationData.setPermissions(Permissions.allPermits(results, authorization, resourceServer));
         accessToken.setAuthorization(authorizationData);
 
         response.rpt = accessToken;

services/src/main/java/org/keycloak/authorization/entitlement/EntitlementService.java

@@ -179,7 +179,7 @@ public class EntitlementService {
 
                 @Override
                 protected void onComplete(List<Result> results) {
-                    List<Permission> entitlements = Permissions.allPermits(results, authorization, resourceServer);
+                    List<Permission> entitlements = Permissions.permits(results, authorization, resourceServer.getId());
 
                     if (entitlements.isEmpty()) {
                         HashMap<Object, Object> error = new HashMap<>();

testsuite/integration-arquillian/test-apps/servlet-authz/src/main/webapp/WEB-INF/web.xml

@@ -13,16 +13,8 @@
 		</web-resource-collection>
 		<auth-constraint>
 			<role-name>user</role-name>
-		</auth-constraint>
-	</security-constraint>
-
-	<security-constraint>
-		<web-resource-collection>
-			<web-resource-name>All Resources</web-resource-name>
-			<url-pattern>/*</url-pattern>
-		</web-resource-collection>
-		<auth-constraint>
 			<role-name>admin</role-name>
+			<role-name>user_premium</role-name>
 		</auth-constraint>
 	</security-constraint>
 
@@ -39,6 +31,10 @@
 		<role-name>user</role-name>
 	</security-role>
 
+	<security-role>
+		<role-name>user_premium</role-name>
+	</security-role>
+
 	<error-page>
 		<error-code>403</error-code>
 		<location>/accessDenied.jsp</location>

testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/client/authorization/EnforcerConfigTest.java

@@ -27,6 +27,7 @@ import org.keycloak.testsuite.AbstractKeycloakTest;
 import org.keycloak.testsuite.ProfileAssume;
 
 import java.util.List;
+import java.util.Map;
 
 import static org.junit.Assert.assertEquals;
 import static org.keycloak.testsuite.util.IOUtil.loadRealm;
@@ -49,8 +50,8 @@ public class EnforcerConfigTest extends AbstractKeycloakTest {
     public void testMultiplePathsWithSameName() throws Exception{
         KeycloakDeployment deployment = KeycloakDeploymentBuilder.build(getClass().getResourceAsStream("/authorization-test/enforcer-config-paths-same-name.json"));
         PolicyEnforcer policyEnforcer = deployment.getPolicyEnforcer();
-        List<PolicyEnforcerConfig.PathConfig> paths = policyEnforcer.getPaths();
+        Map<String, PolicyEnforcerConfig.PathConfig> paths = policyEnforcer.getPaths();
         assertEquals(1, paths.size());
-        assertEquals(4, paths.get(0).getMethods().size());
+        assertEquals(4, paths.values().iterator().next().getMethods().size());
     }
 }