Merge pull request #4052 from pedroigor/KEYCLOAK-4754

[KEYCLOAK-4754] - Unable to delete realm when using aggregated policies

authz/policy/common/src/main/java/org/keycloak/authorization/policy/provider/client/ClientPolicyProviderFactory.java

@@ -78,9 +78,6 @@ public class ClientPolicyProviderFactory implements PolicyProviderFactory {
 
                         try {
                             if (clients.isEmpty()) {
-                                policyStore.findDependentPolicies(policy.getId(), resourceServer.getId()).forEach(dependentPolicy -> {
-                                    dependentPolicy.removeAssociatedPolicy(policy);
-                                });
                                 policyStore.delete(policy.getId());
                             } else {
                                 policy.getConfig().put("clients", JsonSerialization.writeValueAsString(clients));

authz/policy/common/src/main/java/org/keycloak/authorization/policy/provider/role/RolePolicyProviderFactory.java

@@ -230,12 +230,6 @@ public class RolePolicyProviderFactory implements PolicyProviderFactory<RolePoli
 
                 try {
                     if (roles.isEmpty()) {
-                        policyStore.findDependentPolicies(policy.getId(), resourceServer.getId()).forEach(dependentPolicy -> {
-                            dependentPolicy.removeAssociatedPolicy(policy);
-                            if (dependentPolicy.getAssociatedPolicies().isEmpty()) {
-                                policyStore.delete(dependentPolicy.getId());
-                            }
-                        });
                         policyStore.delete(policy.getId());
                     } else {
                         Map<String, String> config = policy.getConfig();

authz/policy/common/src/main/java/org/keycloak/authorization/policy/provider/user/UserPolicyProviderFactory.java

@@ -180,12 +180,6 @@ public class UserPolicyProviderFactory implements PolicyProviderFactory<UserPoli
 
                             try {
                                 if (users.isEmpty()) {
-                                    policyStore.findDependentPolicies(policy.getId(), resourceServer.getId()).forEach(dependentPolicy -> {
-                                        dependentPolicy.removeAssociatedPolicy(policy);
-                                        if (dependentPolicy.getAssociatedPolicies().isEmpty()) {
-                                            policyStore.delete(dependentPolicy.getId());
-                                        }
-                                    });
                                     policyStore.delete(policy.getId());
                                 } else {
                                     policy.getConfig().put("users", JsonSerialization.writeValueAsString(users));

model/infinispan/src/main/java/org/keycloak/models/authorization/infinispan/CachedPolicyStore.java

@@ -102,6 +102,10 @@ public class CachedPolicyStore implements PolicyStore {
 
     @Override
     public Policy findById(String id, String resourceServerId) {
+        if (resourceServerId == null) {
+            return getDelegate().findById(id, null);
+        }
+
         String cacheKeyForPolicy = getCacheKeyForPolicy(id);
         List<CachedPolicy> cached = resolveResourceServerCache(resourceServerId).get(cacheKeyForPolicy);
 

server-spi-private/src/main/java/org/keycloak/authorization/AuthorizationProvider.java

@@ -128,7 +128,20 @@ public final class AuthorizationProvider implements Provider {
 
                     @Override
                     public void delete(String id) {
-                        policyStore.delete(id);
+                        Policy policy = findById(id, null);
+
+                        if (policy != null) {
+                            ResourceServer resourceServer = policy.getResourceServer();
+
+                            findDependentPolicies(policy.getId(), resourceServer.getId()).forEach(dependentPolicy -> {
+                                dependentPolicy.removeAssociatedPolicy(policy);
+                                if (dependentPolicy.getAssociatedPolicies().isEmpty()) {
+                                    delete(dependentPolicy.getId());
+                                }
+                            });
+
+                            policyStore.delete(id);
+                        }
                     }
 
                     @Override

services/src/main/java/org/keycloak/authorization/admin/PolicyResourceService.java

@@ -96,14 +96,6 @@ public class PolicyResourceService {
 
         resource.onRemove(policy, authorization);
 
-        policyStore.findDependentPolicies(policy.getId(), resourceServer.getId()).forEach(dependentPolicy -> {
-            if (dependentPolicy.getAssociatedPolicies().size() == 1) {
-                policyStore.delete(dependentPolicy.getId());
-            } else {
-                dependentPolicy.removeAssociatedPolicy(policy);
-            }
-        });
-
         policyStore.delete(policy.getId());
 
         return Response.noContent().build();

testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/AuthzCleanupTest.java

@@ -23,20 +23,24 @@ import java.util.List;
 import org.jboss.arquillian.container.test.api.Deployment;
 import org.jboss.shrinkwrap.api.spec.WebArchive;
 import org.junit.Test;
+import org.keycloak.admin.client.resource.ClientsResource;
 import org.keycloak.authorization.AuthorizationProvider;
 import org.keycloak.authorization.model.Policy;
 import org.keycloak.authorization.model.ResourceServer;
 import org.keycloak.models.ClientModel;
 import org.keycloak.models.KeycloakSession;
 import org.keycloak.models.RealmModel;
+import org.keycloak.representations.idm.ClientRepresentation;
 import org.keycloak.representations.idm.RealmRepresentation;
 import org.keycloak.representations.idm.authorization.DecisionStrategy;
 import org.keycloak.representations.idm.authorization.Logic;
+import org.keycloak.representations.idm.authorization.ResourceServerRepresentation;
 import org.keycloak.representations.idm.authorization.RolePolicyRepresentation;
 import org.keycloak.testsuite.AbstractKeycloakTest;
 import org.keycloak.testsuite.runonserver.RunOnServerDeployment;
 import org.keycloak.testsuite.util.ClientBuilder;
 import org.keycloak.testsuite.util.RealmBuilder;
+import org.keycloak.util.JsonSerialization;
 
 /**
  * @author <a href="mailto:bill@burkecentral.com">Bill Burke</a>
@@ -56,7 +60,17 @@ public class AuthzCleanupTest extends AbstractKeycloakTest {
                         .secret("secret")
                         .authorizationServicesEnabled(true)
                         .redirectUris("http://localhost/myclient")
-                        .defaultRoles("client-role-1", "client-role-2").build()).build());
+                        .defaultRoles(
+                                "client-role-1",
+                                "client-role-2",
+                                "Acme administrator",
+                                "Acme viewer",
+                                "tenant administrator",
+                                "tenant viewer",
+                                "tenant user"
+                                )
+                            .build())
+                .build());
     }
 
     public static void setup(KeycloakSession session) {
@@ -84,6 +98,12 @@ public class AuthzCleanupTest extends AbstractKeycloakTest {
 
     @Test
     public void testCreate() throws Exception {
+        ClientsResource clients = getAdminClient().realms().realm(TEST).clients();
+        ClientRepresentation client = clients.findByClientId("myclient").get(0);
+        ResourceServerRepresentation settings = JsonSerialization.readValue(getClass().getResourceAsStream("/authorization-test/acme-resource-server-cleanup-test.json"), ResourceServerRepresentation.class);
+
+        clients.get(client.getId()).authorization().importSettings(settings);
+
         testingClient.server().run(AuthzCleanupTest::setup);
     }
 

testsuite/integration-arquillian/tests/base/src/test/resources/authorization-test/acme-resource-server-cleanup-test.json

@@ -0,0 +1,195 @@
+{
+  "allowRemoteResourceManagement": false,
+  "policyEnforcementMode": "ENFORCING",
+  "resources": [
+    {
+      "name": "Administration resource",
+      "uri": "/admin/*",
+      "type": "http://acme.com/admin",
+      "scopes": [
+        {
+          "name": "urn:acme.com:scopes:admin:manage"
+        },
+        {
+          "name": "urn:acme.com:scopes:admin:view"
+        }
+      ],
+      "typedScopes": []
+    },
+    {
+      "name": "Role resource",
+      "uri": "/{REALM}/roles",
+      "type": "http://acme.com/roles",
+      "scopes": [
+        {
+          "name": "urn:acme.com:scopes:role:view"
+        }
+      ],
+      "typedScopes": []
+    },
+    {
+      "name": "User profile resource",
+      "uri": "/{REALM}/userprofiles/*",
+      "type": "http://acme.com/userprofiles",
+      "scopes": [
+        {
+          "name": "urn:acme.com:scopes:userprofile:manage"
+        },
+        {
+          "name": "urn:acme.com:scopes:userprofile:view"
+        }
+      ],
+      "typedScopes": []
+    },
+    {
+      "name": "Account resource",
+      "uri": "/{REALM}/account/*",
+      "type": "http://acme.com/account",
+      "scopes": [
+        {
+          "name": "urn:acme.com:scopes:account:manage"
+        }
+      ],
+      "typedScopes": []
+    }
+  ],
+  "policies": [
+    {
+      "name": "Acme admin policy",
+      "type": "role",
+      "logic": "POSITIVE",
+      "decisionStrategy": "UNANIMOUS",
+      "config": {
+        "roles": "[{\"id\":\"Acme administrator\",\"required\":true}]"
+      }
+    },
+    {
+      "name": "Acme viewer policy",
+      "type": "role",
+      "logic": "POSITIVE",
+      "decisionStrategy": "UNANIMOUS",
+      "config": {
+        "roles": "[{\"id\":\"Acme viewer\",\"required\":true}]"
+      }
+    },
+    {
+      "name": "Tenant user policy",
+      "type": "role",
+      "logic": "POSITIVE",
+      "decisionStrategy": "UNANIMOUS",
+      "config": {
+        "roles": "[{\"id\":\"tenant user\",\"required\":true}]"
+      }
+    },
+    {
+      "name": "Tenant administrator policy",
+      "type": "role",
+      "logic": "POSITIVE",
+      "decisionStrategy": "UNANIMOUS",
+      "config": {
+        "roles": "[{\"id\":\"tenant administrator\",\"required\":true}]"
+      }
+    },
+    {
+      "name": "Tenant viewer policy",
+      "type": "role",
+      "logic": "POSITIVE",
+      "decisionStrategy": "UNANIMOUS",
+      "config": {
+        "roles": "[{\"id\":\"tenant viewer\",\"required\":true}]"
+      }
+    },
+    {
+      "name": "Any user policy",
+      "description": "Defines that only users from well known clients are allowed to access",
+      "type": "aggregate",
+      "logic": "POSITIVE",
+      "decisionStrategy": "AFFIRMATIVE",
+      "config": {
+        "applyPolicies": "[\"Tenant user policy\",\"Acme admin policy\",\"Acme viewer policy\",\"Tenant viewer policy\",\"Tenant administrator policy\"]"
+      }
+    },
+    {
+      "name": "Super tenant admin permission",
+      "type": "scope",
+      "logic": "POSITIVE",
+      "decisionStrategy": "UNANIMOUS",
+      "config": {
+        "scopes": "[\"urn:acme.com:scopes:admin:manage\"]",
+        "applyPolicies": "[\"Acme admin policy\"]"
+      }
+    },
+    {
+      "name": "Super tenant admin read permission",
+      "type": "scope",
+      "logic": "POSITIVE",
+      "decisionStrategy": "AFFIRMATIVE",
+      "config": {
+        "scopes": "[\"urn:acme.com:scopes:admin:view\"]",
+        "applyPolicies": "[\"Acme admin policy\",\"Acme viewer policy\"]"
+      }
+    },
+    {
+      "name": "View tenant role permission",
+      "type": "scope",
+      "logic": "POSITIVE",
+      "decisionStrategy": "AFFIRMATIVE",
+      "config": {
+        "resources": "[\"Role resource\"]",
+        "scopes": "[\"urn:acme.com:scopes:role:view\"]",
+        "applyPolicies": "[\"Any user policy\"]"
+      }
+    },
+    {
+      "name": "Manage account permission",
+      "type": "scope",
+      "logic": "POSITIVE",
+      "decisionStrategy": "AFFIRMATIVE",
+      "config": {
+        "resources": "[\"Account resource\"]",
+        "scopes": "[\"urn:acme.com:scopes:account:manage\"]",
+        "applyPolicies": "[\"Any user policy\"]"
+      }
+    },
+    {
+      "name": "Manage user profile permission",
+      "type": "scope",
+      "logic": "POSITIVE",
+      "decisionStrategy": "AFFIRMATIVE",
+      "config": {
+        "scopes": "[\"urn:acme.com:scopes:userprofile:manage\"]",
+        "applyPolicies": "[\"Acme admin policy\",\"Tenant administrator policy\"]"
+      }
+    },
+    {
+      "name": "View user profile permission",
+      "type": "scope",
+      "logic": "POSITIVE",
+      "decisionStrategy": "AFFIRMATIVE",
+      "config": {
+        "scopes": "[\"urn:acme.com:scopes:userprofile:view\"]",
+        "applyPolicies": "[\"Acme admin policy\",\"Acme viewer policy\",\"Tenant viewer policy\",\"Tenant administrator policy\"]"
+      }
+    }
+  ],
+  "scopes": [
+    {
+      "name": "urn:acme.com:scopes:admin:manage"
+    },
+    {
+      "name": "urn:acme.com:scopes:admin:view"
+    },
+    {
+      "name": "urn:acme.com:scopes:role:view"
+    },
+    {
+      "name": "urn:acme.com:scopes:account:manage"
+    },
+    {
+      "name": "urn:acme.com:scopes:userprofile:view"
+    },
+    {
+      "name": "urn:acme.com:scopes:userprofile:manage"
+    }
+  ]
+}