diff --git a/authz/policy/common/src/main/java/org/keycloak/authorization/policy/provider/client/ClientPolicyProviderFactory.java b/authz/policy/common/src/main/java/org/keycloak/authorization/policy/provider/client/ClientPolicyProviderFactory.java index 8f7e631c7f..a0e78746a5 100644 --- a/authz/policy/common/src/main/java/org/keycloak/authorization/policy/provider/client/ClientPolicyProviderFactory.java +++ b/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)); diff --git a/authz/policy/common/src/main/java/org/keycloak/authorization/policy/provider/role/RolePolicyProviderFactory.java b/authz/policy/common/src/main/java/org/keycloak/authorization/policy/provider/role/RolePolicyProviderFactory.java index 90047bba84..d5d39175da 100644 --- a/authz/policy/common/src/main/java/org/keycloak/authorization/policy/provider/role/RolePolicyProviderFactory.java +++ b/authz/policy/common/src/main/java/org/keycloak/authorization/policy/provider/role/RolePolicyProviderFactory.java @@ -230,12 +230,6 @@ public class RolePolicyProviderFactory implements PolicyProviderFactory { - dependentPolicy.removeAssociatedPolicy(policy); - if (dependentPolicy.getAssociatedPolicies().isEmpty()) { - policyStore.delete(dependentPolicy.getId()); - } - }); policyStore.delete(policy.getId()); } else { Map config = policy.getConfig(); diff --git a/authz/policy/common/src/main/java/org/keycloak/authorization/policy/provider/user/UserPolicyProviderFactory.java b/authz/policy/common/src/main/java/org/keycloak/authorization/policy/provider/user/UserPolicyProviderFactory.java index 2d3c219111..0fd54df2b1 100644 --- a/authz/policy/common/src/main/java/org/keycloak/authorization/policy/provider/user/UserPolicyProviderFactory.java +++ b/authz/policy/common/src/main/java/org/keycloak/authorization/policy/provider/user/UserPolicyProviderFactory.java @@ -180,12 +180,6 @@ public class UserPolicyProviderFactory implements PolicyProviderFactory { - dependentPolicy.removeAssociatedPolicy(policy); - if (dependentPolicy.getAssociatedPolicies().isEmpty()) { - policyStore.delete(dependentPolicy.getId()); - } - }); policyStore.delete(policy.getId()); } else { policy.getConfig().put("users", JsonSerialization.writeValueAsString(users)); diff --git a/model/infinispan/src/main/java/org/keycloak/models/authorization/infinispan/CachedPolicyStore.java b/model/infinispan/src/main/java/org/keycloak/models/authorization/infinispan/CachedPolicyStore.java index 5bb365153c..b517098951 100644 --- a/model/infinispan/src/main/java/org/keycloak/models/authorization/infinispan/CachedPolicyStore.java +++ b/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 cached = resolveResourceServerCache(resourceServerId).get(cacheKeyForPolicy); 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 76e6894943..c5b75d38db 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 @@ -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 diff --git a/services/src/main/java/org/keycloak/authorization/admin/PolicyResourceService.java b/services/src/main/java/org/keycloak/authorization/admin/PolicyResourceService.java index e2db57eda8..85e0943221 100644 --- a/services/src/main/java/org/keycloak/authorization/admin/PolicyResourceService.java +++ b/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(); diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/AuthzCleanupTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/AuthzCleanupTest.java index 69d8d6b075..4c472fba5f 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/AuthzCleanupTest.java +++ b/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 Bill Burke @@ -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); } diff --git a/testsuite/integration-arquillian/tests/base/src/test/resources/authorization-test/acme-resource-server-cleanup-test.json b/testsuite/integration-arquillian/tests/base/src/test/resources/authorization-test/acme-resource-server-cleanup-test.json new file mode 100644 index 0000000000..3c1f55f1f9 --- /dev/null +++ b/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" + } + ] +} \ No newline at end of file