[KEYCLOAK-8281] - Deletion of client with token exchange policy leads…

… to breaking errors
abryson-redhat · Sep 18, 2018 · 609c521 · 609c521
1 parent 0b893d5
commit 609c521
Show file tree

Hide file tree

Showing 5 changed files with 185 additions and 9 deletions.
diff --git a/model/jpa/src/main/java/org/keycloak/authorization/jpa/store/JPAPolicyStore.java b/model/jpa/src/main/java/org/keycloak/authorization/jpa/store/JPAPolicyStore.java
@@ -137,7 +137,9 @@ public List<Policy> findByResourceServer(Map<String, String[]> attributes, Strin
         List<Predicate> predicates = new ArrayList();
         querybuilder.select(root.get("id"));
 
-        predicates.add(builder.equal(root.get("resourceServer").get("id"), resourceServerId));
+        if (resourceServerId != null) {
+            predicates.add(builder.equal(root.get("resourceServer").get("id"), resourceServerId));
+        }
 
         attributes.forEach((name, value) -> {
             if ("permission".equals(name)) {
@@ -156,6 +158,9 @@ public List<Policy> findByResourceServer(Map<String, String[]> attributes, Strin
                 predicates.add(root.join("resources").get("id").in(value));
             } else if ("scope".equals(name)) {
                 predicates.add(root.join("scopes").get("id").in(value));
+            } else if (name.startsWith("config:")) {
+                predicates.add(root.joinMap("config").key().in(name.substring("config:".length())));
+                predicates.add(builder.like(root.joinMap("config").value().as(String.class), "%" + value[0] + "%"));
             } else {
                 predicates.add(builder.like(builder.lower(root.get(name)), "%" + value[0].toLowerCase() + "%"));
             }

diff --git a/...n/java/org/keycloak/authorization/store/syncronization/ClientApplicationSynchronizer.java b/...n/java/org/keycloak/authorization/store/syncronization/ClientApplicationSynchronizer.java
@@ -18,13 +18,21 @@
 
 package org.keycloak.authorization.store.syncronization;
 
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
 import org.keycloak.authorization.AuthorizationProvider;
+import org.keycloak.authorization.model.Policy;
 import org.keycloak.authorization.model.ResourceServer;
+import org.keycloak.authorization.policy.provider.PolicyProviderFactory;
 import org.keycloak.authorization.store.ResourceServerStore;
 import org.keycloak.authorization.store.StoreFactory;
 import org.keycloak.models.KeycloakSessionFactory;
 import org.keycloak.models.RealmModel.ClientRemovedEvent;
 import org.keycloak.provider.ProviderFactory;
+import org.keycloak.representations.idm.authorization.ClientPolicyRepresentation;
 
 /**
  * @author <a href="mailto:psilva@redhat.com">Pedro Igor</a>
@@ -35,16 +43,39 @@ public class ClientApplicationSynchronizer implements Synchronizer<ClientRemoved
     public void synchronize(ClientRemovedEvent event, KeycloakSessionFactory factory) {
         ProviderFactory<AuthorizationProvider> providerFactory = factory.getProviderFactory(AuthorizationProvider.class);
         AuthorizationProvider authorizationProvider = providerFactory.create(event.getKeycloakSession());
+
+        removeFromClientPolicies(event, authorizationProvider);
+    }
+
+    private void removeFromClientPolicies(ClientRemovedEvent event, AuthorizationProvider authorizationProvider) {
         StoreFactory storeFactory = authorizationProvider.getStoreFactory();
         ResourceServerStore store = storeFactory.getResourceServerStore();
         ResourceServer resourceServer = store.findById(event.getClient().getId());
 
         if (resourceServer != null) {
-            String id = resourceServer.getId();
-            //storeFactory.getResourceStore().findByResourceServer(id).forEach(resource -> storeFactory.getResourceStore().delete(resource.getId()));
-            //storeFactory.getScopeStore().findByResourceServer(id).forEach(scope -> storeFactory.getScopeStore().delete(scope.getId()));
-            //storeFactory.getPolicyStore().findByResourceServer(id).forEach(scope -> storeFactory.getPolicyStore().delete(scope.getId()));
-            storeFactory.getResourceServerStore().delete(id);
+            storeFactory.getResourceServerStore().delete(resourceServer.getId());
+        }
+
+        Map<String, String[]> attributes = new HashMap<>();
+
+        attributes.put("type", new String[] {"client"});
+        attributes.put("config:clients", new String[] {event.getClient().getId()});
+
+        List<Policy> search = storeFactory.getPolicyStore().findByResourceServer(attributes, null, -1, -1);
+
+        for (Policy policy : search) {
+            PolicyProviderFactory policyFactory = authorizationProvider.getProviderFactory(policy.getType());
+            ClientPolicyRepresentation representation = ClientPolicyRepresentation.class.cast(policyFactory.toRepresentation(policy, authorizationProvider));
+            Set<String> clients = representation.getClients();
+
+            clients.remove(event.getClient().getId());
+
+            if (clients.isEmpty()) {
+                policyFactory.onRemove(policy, authorizationProvider);
+                authorizationProvider.getStoreFactory().getPolicyStore().delete(policy.getId());
+            } else {
+                policyFactory.onUpdate(policy, representation, authorizationProvider);
+            }
         }
     }
 }
diff --git a/...ivate/src/main/java/org/keycloak/authorization/store/syncronization/UserSynchronizer.java b/...ivate/src/main/java/org/keycloak/authorization/store/syncronization/UserSynchronizer.java
@@ -17,8 +17,15 @@
 
 package org.keycloak.authorization.store.syncronization;
 
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
 import org.keycloak.authorization.AuthorizationProvider;
+import org.keycloak.authorization.model.Policy;
 import org.keycloak.authorization.model.ResourceServer;
+import org.keycloak.authorization.policy.provider.PolicyProviderFactory;
 import org.keycloak.authorization.store.PolicyStore;
 import org.keycloak.authorization.store.ResourceServerStore;
 import org.keycloak.authorization.store.ResourceStore;
@@ -28,6 +35,7 @@
 import org.keycloak.models.UserModel;
 import org.keycloak.models.UserModel.UserRemovedEvent;
 import org.keycloak.provider.ProviderFactory;
+import org.keycloak.representations.idm.authorization.UserPolicyRepresentation;
 
 /**
  * @author <a href="mailto:psilva@redhat.com">Pedro Igor</a>
@@ -38,12 +46,45 @@ public class UserSynchronizer implements Synchronizer<UserRemovedEvent> {
     public void synchronize(UserRemovedEvent event, KeycloakSessionFactory factory) {
         ProviderFactory<AuthorizationProvider> providerFactory = factory.getProviderFactory(AuthorizationProvider.class);
         AuthorizationProvider authorizationProvider = providerFactory.create(event.getKeycloakSession());
+
+        removeUserResources(event, authorizationProvider);
+        removeFromUserPolicies(event, authorizationProvider);
+    }
+
+    private void removeFromUserPolicies(UserRemovedEvent event, AuthorizationProvider authorizationProvider) {
         StoreFactory storeFactory = authorizationProvider.getStoreFactory();
+        PolicyStore policyStore = storeFactory.getPolicyStore();
         UserModel userModel = event.getUser();
-        ResourceStore resourceStore = storeFactory.getResourceStore();
+        Map<String, String[]> attributes = new HashMap<>();
+
+        attributes.put("type", new String[] {"user"});
+        attributes.put("config:users", new String[] {userModel.getId()});
+
+        List<Policy> search = policyStore.findByResourceServer(attributes, null, -1, -1);
+
+        for (Policy policy : search) {
+            PolicyProviderFactory policyFactory = authorizationProvider.getProviderFactory(policy.getType());
+            UserPolicyRepresentation representation = UserPolicyRepresentation.class.cast(policyFactory.toRepresentation(policy, authorizationProvider));
+            Set<String> users = representation.getUsers();
+
+            users.remove(userModel.getId());
+
+            if (users.isEmpty()) {
+                policyFactory.onRemove(policy, authorizationProvider);
+                policyStore.delete(policy.getId());
+            } else {
+                policyFactory.onUpdate(policy, representation, authorizationProvider);
+            }
+        }
+    }
+
+    private void removeUserResources(UserRemovedEvent event, AuthorizationProvider authorizationProvider) {
+        StoreFactory storeFactory = authorizationProvider.getStoreFactory();
         PolicyStore policyStore = storeFactory.getPolicyStore();
+        ResourceStore resourceStore = storeFactory.getResourceStore();
         ResourceServerStore resourceServerStore = storeFactory.getResourceServerStore();
         RealmModel realm = event.getRealm();
+        UserModel userModel = event.getUser();
 
         realm.getClients().forEach(clientModel -> {
             ResourceServer resourceServer = resourceServerStore.findById(clientModel.getId());

diff --git a/...st/java/org/keycloak/testsuite/admin/client/authorization/ClientPolicyManagementTest.java b/...st/java/org/keycloak/testsuite/admin/client/authorization/ClientPolicyManagementTest.java
@@ -27,10 +27,12 @@
 import javax.ws.rs.NotFoundException;
 import javax.ws.rs.core.Response;
 
+import org.junit.Assert;
 import org.junit.Test;
 import org.keycloak.admin.client.resource.AuthorizationResource;
 import org.keycloak.admin.client.resource.ClientPoliciesResource;
 import org.keycloak.admin.client.resource.ClientPolicyResource;
+import org.keycloak.admin.client.resource.ClientsResource;
 import org.keycloak.admin.client.resource.PolicyResource;
 import org.keycloak.representations.idm.ClientRepresentation;
 import org.keycloak.representations.idm.authorization.ClientPolicyRepresentation;
@@ -50,7 +52,10 @@ protected RealmBuilder createTestRealm() {
         return super.createTestRealm()
                 .client(ClientBuilder.create().clientId("Client A"))
                 .client(ClientBuilder.create().clientId("Client B"))
-                .client(ClientBuilder.create().clientId("Client C"));
+                .client(ClientBuilder.create().clientId("Client C"))
+                .client(ClientBuilder.create().clientId("Client D"))
+                .client(ClientBuilder.create().clientId("Client E"))
+                .client(ClientBuilder.create().clientId("Client F"));
     }
 
     @Test
@@ -126,6 +131,51 @@ public void testDelete() {
         }
     }
 
+
+    @Test
+    public void testDeleteClient() {
+        AuthorizationResource authorization = getClient().authorization();
+        ClientPolicyRepresentation representation = new ClientPolicyRepresentation();
+
+        representation.setName("Update Test Client Policy");
+        representation.setDescription("description");
+        representation.setDecisionStrategy(DecisionStrategy.CONSENSUS);
+        representation.setLogic(Logic.NEGATIVE);
+        representation.addClient("Client D");
+        representation.addClient("Client E");
+        representation.addClient("Client F");
+
+        assertCreated(authorization, representation);
+
+        ClientsResource clients = getRealm().clients();
+        ClientRepresentation client = clients.findByClientId("Client D").get(0);
+
+        clients.get(client.getId()).remove();
+
+        representation = authorization.policies().client().findById(representation.getId()).toRepresentation();
+
+        Assert.assertEquals(2, representation.getClients().size());
+        Assert.assertFalse(representation.getClients().contains(client.getId()));
+
+        client = clients.findByClientId("Client E").get(0);
+        clients.get(client.getId()).remove();
+
+        representation = authorization.policies().client().findById(representation.getId()).toRepresentation();
+
+        Assert.assertEquals(1, representation.getClients().size());
+        Assert.assertFalse(representation.getClients().contains(client.getId()));
+
+        client = clients.findByClientId("Client F").get(0);
+        clients.get(client.getId()).remove();
+
+        try {
+            authorization.policies().client().findById(representation.getId()).toRepresentation();
+            fail("Client policy should be removed");
+        } catch (NotFoundException nfe) {
+            // ignore
+        }
+    }
+
     @Test
     public void testGenericConfig() {
         AuthorizationResource authorization = getClient().authorization();

diff --git a/...test/java/org/keycloak/testsuite/admin/client/authorization/UserPolicyManagementTest.java b/...test/java/org/keycloak/testsuite/admin/client/authorization/UserPolicyManagementTest.java
@@ -29,11 +29,13 @@
 import javax.ws.rs.NotFoundException;
 import javax.ws.rs.core.Response;
 
+import org.junit.Assert;
 import org.junit.Test;
 import org.keycloak.admin.client.resource.AuthorizationResource;
 import org.keycloak.admin.client.resource.PolicyResource;
 import org.keycloak.admin.client.resource.UserPoliciesResource;
 import org.keycloak.admin.client.resource.UserPolicyResource;
+import org.keycloak.admin.client.resource.UsersResource;
 import org.keycloak.representations.idm.UserRepresentation;
 import org.keycloak.representations.idm.authorization.DecisionStrategy;
 import org.keycloak.representations.idm.authorization.Logic;
@@ -52,7 +54,10 @@ protected RealmBuilder createTestRealm() {
         return super.createTestRealm()
                 .user(UserBuilder.create().username("User A"))
                 .user(UserBuilder.create().username("User B"))
-                .user(UserBuilder.create().username("User C"));
+                .user(UserBuilder.create().username("User C"))
+                .user(UserBuilder.create().username("User D"))
+                .user(UserBuilder.create().username("User E"))
+                .user(UserBuilder.create().username("User F"));
     }
 
     @Test
@@ -127,6 +132,50 @@ public void testDelete() {
         }
     }
 
+    @Test
+    public void testDeleteUser() {
+        AuthorizationResource authorization = getClient().authorization();
+        UserPolicyRepresentation representation = new UserPolicyRepresentation();
+
+        representation.setName("Realm User Policy");
+        representation.setDescription("description");
+        representation.setDecisionStrategy(DecisionStrategy.CONSENSUS);
+        representation.setLogic(Logic.NEGATIVE);
+        representation.addUser("User D");
+        representation.addUser("User E");
+        representation.addUser("User F");
+
+        assertCreated(authorization, representation);
+
+        UsersResource users = getRealm().users();
+        UserRepresentation user = users.search("User D").get(0);
+
+        users.get(user.getId()).remove();
+
+        representation = authorization.policies().user().findById(representation.getId()).toRepresentation();
+
+        Assert.assertEquals(2, representation.getUsers().size());
+        Assert.assertFalse(representation.getUsers().contains(user.getId()));
+
+        user = users.search("User E").get(0);
+        users.get(user.getId()).remove();
+
+        representation = authorization.policies().user().findById(representation.getId()).toRepresentation();
+
+        Assert.assertEquals(1, representation.getUsers().size());
+        Assert.assertFalse(representation.getUsers().contains(user.getId()));
+
+        user = users.search("User F").get(0);
+        users.get(user.getId()).remove();
+
+        try {
+            authorization.policies().user().findById(representation.getId()).toRepresentation();
+            fail("User policy should be removed");
+        } catch (NotFoundException nfe) {
+            // ignore
+        }
+    }
+
     @Test
     public void testGenericConfig() {
         AuthorizationResource authorization = getClient().authorization();