Do not cache policies if they no longer exist (#12797)

Closes #12657

Co-authored-by: Michal Hajas <mhajas@redhat.com>

Co-authored-by: Michal Hajas <mhajas@redhat.com>

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

@@ -19,7 +19,7 @@ package org.keycloak.authorization.policy.provider.client;
 
 import java.io.IOException;
 import java.util.ArrayList;
-import java.util.Arrays;
+import java.util.Collections;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.List;
@@ -67,7 +67,7 @@ public class ClientPolicyProviderFactory implements PolicyProviderFactory<Client
     @Override
     public ClientPolicyRepresentation toRepresentation(Policy policy, AuthorizationProvider authorization) {
         ClientPolicyRepresentation representation = new ClientPolicyRepresentation();
-        representation.setClients(new HashSet<>(Arrays.asList(getClients(policy))));
+        representation.setClients(getClients(policy));
         return representation;
     }
 
@@ -88,7 +88,7 @@ public class ClientPolicyProviderFactory implements PolicyProviderFactory<Client
 
     @Override
     public void onImport(Policy policy, PolicyRepresentation representation, AuthorizationProvider authorization) {
-        updateClients(policy, new HashSet<>(Arrays.asList(getClients(policy))), authorization);
+        updateClients(policy, getClients(policy), authorization);
     }
 
     @Override
@@ -193,17 +193,17 @@ public class ClientPolicyProviderFactory implements PolicyProviderFactory<Client
         }
     }
 
-    private String[] getClients(Policy policy) {
+    private Set<String> getClients(Policy policy) {
         String clients = policy.getConfig().get("clients");
 
         if (clients != null) {
             try {
-                return JsonSerialization.readValue(clients.getBytes(), String[].class);
+                return JsonSerialization.readValue(clients, Set.class);
             } catch (IOException e) {
                 throw new RuntimeException("Could not parse clients [" + clients + "] from policy config [" + policy.getName() + "].", e);
             }
         }
 
-        return new String[]{};
+        return Collections.emptySet();
     }
 }

authz/policy/common/src/main/java/org/keycloak/authorization/policy/provider/clientscope/ClientScopePolicyProviderFactory.java

@@ -19,6 +19,7 @@ package org.keycloak.authorization.policy.provider.clientscope;
 import java.io.IOException;
 import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.Collections;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.List;
@@ -151,9 +152,15 @@ public class ClientScopePolicyProviderFactory implements PolicyProviderFactory<C
         ClientScopePolicyRepresentation representation = new ClientScopePolicyRepresentation();
 
         try {
+            String clientScopes = policy.getConfig().get("clientScopes");
+
+            if (clientScopes == null) {
+                representation.setClientScopes(Collections.emptySet());
+            } else {
                 representation
-                .setClientScopes(new HashSet<>(Arrays.asList(JsonSerialization.readValue(policy.getConfig().get("clientScopes"),
+                        .setClientScopes(new HashSet<>(Arrays.asList(JsonSerialization.readValue(clientScopes,
                                 ClientScopePolicyRepresentation.ClientScopeDefinition[].class))));
+            }
         } catch (IOException e) {
             throw new RuntimeException("Failed to deserialize client scopes", e);
         }

authz/policy/common/src/main/java/org/keycloak/authorization/policy/provider/group/GroupPolicyProviderFactory.java

@@ -19,6 +19,7 @@ package org.keycloak.authorization.policy.provider.group;
 
 import java.io.IOException;
 import java.util.Arrays;
+import java.util.Collections;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.List;
@@ -213,7 +214,13 @@ public class GroupPolicyProviderFactory implements PolicyProviderFactory<GroupPo
         policy.setConfig(config);
     }
 
-    private HashSet<GroupPolicyRepresentation.GroupDefinition> getGroupsDefinition(Map<String, String> config) throws IOException {
+    private Set<GroupPolicyRepresentation.GroupDefinition> getGroupsDefinition(Map<String, String> config) throws IOException {
-        return new HashSet<>(Arrays.asList(JsonSerialization.readValue(config.get("groups"), GroupPolicyRepresentation.GroupDefinition[].class)));
+        String groups = config.get("groups");
+
+        if (groups == null) {
+            return Collections.emptySet();
+        }
+
+        return new HashSet<>(Arrays.asList(JsonSerialization.readValue(groups, GroupPolicyRepresentation.GroupDefinition[].class)));
     }
 }

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

@@ -40,6 +40,7 @@ import org.keycloak.util.JsonSerialization;
 import java.io.IOException;
 import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.Collections;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.List;
@@ -78,7 +79,14 @@ public class RolePolicyProviderFactory implements PolicyProviderFactory<RolePoli
         RolePolicyRepresentation representation = new RolePolicyRepresentation();
 
         try {
-            representation.setRoles(new HashSet<>(Arrays.asList(JsonSerialization.readValue(policy.getConfig().get("roles"), RolePolicyRepresentation.RoleDefinition[].class))));
+            String roles = policy.getConfig().get("roles");
+
+            if (roles == null) {
+                representation.setRoles(Collections.emptySet());
+            } else {
+                representation.setRoles(new HashSet<>(
+                        Arrays.asList(JsonSerialization.readValue(roles, RolePolicyRepresentation.RoleDefinition[].class))));
+            }
         } catch (IOException cause) {
             throw new RuntimeException("Failed to deserialize roles", cause);
         }

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

@@ -19,6 +19,7 @@
 package org.keycloak.authorization.policy.provider.user;
 
 import java.io.IOException;
+import java.util.Collections;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.Map;
@@ -71,7 +72,13 @@ public class UserPolicyProviderFactory implements PolicyProviderFactory<UserPoli
         UserPolicyRepresentation representation = new UserPolicyRepresentation();
 
         try {
-            representation.setUsers(JsonSerialization.readValue(policy.getConfig().get("users"), Set.class));
+            String users = policy.getConfig().get("users");
+
+            if (users == null) {
+                representation.setUsers(Collections.emptySet());
+            } else {
+                representation.setUsers(JsonSerialization.readValue(users, Set.class));
+            }
         } catch (IOException cause) {
             throw new RuntimeException("Failed to deserialize roles", cause);
         }

model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/authorization/PolicyAdapter.java

@@ -206,10 +206,14 @@ public class PolicyAdapter implements Policy, CachedModel<Policy> {
         }
         if (associatedPolicies != null) return associatedPolicies;
         associatedPolicies = new HashSet<>();
-        PolicyStore policyStore = cacheSession.getPolicyStore();
+        PolicyStore policyStore = cacheSession.getPolicyStoreDelegate();
         String resourceServerId = cached.getResourceServerId();
         for (String id : cached.getAssociatedPoliciesIds(modelSupplier)) {
             Policy policy = policyStore.findById(InfinispanCacheStoreFactoryProviderFactory.NULL_REALM, cacheSession.getResourceServerStore().findById(InfinispanCacheStoreFactoryProviderFactory.NULL_REALM, resourceServerId), id);
+            if (policy == null) {
+                // probably because the policy was removed
+                continue;
+            }
             cacheSession.cachePolicy(policy);
             associatedPolicies.add(policy);
         }

model/jpa/src/main/java/org/keycloak/authorization/jpa/store/JPAPolicyStore.java

@@ -199,8 +199,9 @@ public class JPAPolicyStore implements PolicyStore {
 
         List<String> result = paginateQuery(query, firstResult, maxResults).getResultList();
         List<Policy> list = new LinkedList<>();
+        PolicyStore policyStore = provider.getStoreFactory().getPolicyStore();
         for (String id : result) {
-            Policy policy = provider.getStoreFactory().getPolicyStore().findById(JPAAuthorizationStoreFactory.NULL_REALM, resourceServer, id);
+            Policy policy = policyStore.findById(JPAAuthorizationStoreFactory.NULL_REALM, resourceServer, id);
             if (Objects.nonNull(policy)) {
                 list.add(policy);
             }

model/map/src/main/java/org/keycloak/models/map/authorization/MapPolicyStore.java

@@ -162,9 +162,7 @@ public class MapPolicyStore implements PolicyStore {
         }
 
         return tx.read(withCriteria(mcb).pagination(firstResult, maxResults, SearchableFields.NAME))
-            .map(MapPolicyEntity::getId)
+                .map(entityToAdapterFunc(realm, resourceServer))
-            // We need to go through cache
-            .map(id -> authorizationProvider.getStoreFactory().getPolicyStore().findById(realm, resourceServer, id))
                 .collect(Collectors.toList());
     }
 

testsuite/model/src/test/java/org/keycloak/testsuite/model/KeycloakModelTest.java

@@ -22,6 +22,9 @@ import org.junit.Assert;
 import org.keycloak.Config.Scope;
 import org.keycloak.authorization.AuthorizationSpi;
 import org.keycloak.authorization.DefaultAuthorizationProviderFactory;
+import org.keycloak.authorization.policy.provider.PolicyProviderFactory;
+import org.keycloak.authorization.policy.provider.PolicySpi;
+import org.keycloak.authorization.policy.provider.permission.UMAPolicyProviderFactory;
 import org.keycloak.authorization.store.StoreFactorySpi;
 import org.keycloak.cluster.ClusterSpi;
 import org.keycloak.common.util.Time;
@@ -213,6 +216,7 @@ public abstract class KeycloakModelTest {
 
     private static final Set<Class<? extends Spi>> ALLOWED_SPIS = ImmutableSet.<Class<? extends Spi>>builder()
       .add(AuthorizationSpi.class)
+      .add(PolicySpi.class)
       .add(ClientScopeSpi.class)
       .add(ClientSpi.class)
       .add(ComponentFactorySpi.class)
@@ -235,6 +239,7 @@ public abstract class KeycloakModelTest {
     private static final Set<Class<? extends ProviderFactory>> ALLOWED_FACTORIES = ImmutableSet.<Class<? extends ProviderFactory>>builder()
       .add(ComponentFactoryProviderFactory.class)
       .add(DefaultAuthorizationProviderFactory.class)
+      .add(PolicyProviderFactory.class)
       .add(DefaultExecutorsProviderFactory.class)
       .add(DeploymentStateProviderFactory.class)
       .add(DatastoreProviderFactory.class)

testsuite/model/src/test/java/org/keycloak/testsuite/model/authz/ConcurrentAuthzTest.java

@@ -0,0 +1,188 @@
+/*
+ * Copyright 2022 Red Hat, Inc. and/or its affiliates
+ * and other contributors as indicated by the @author tags.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.keycloak.testsuite.model.authz;
+
+import org.junit.Test;
+import org.keycloak.authorization.AuthorizationProvider;
+import org.keycloak.authorization.model.Policy;
+import org.keycloak.authorization.model.ResourceServer;
+import org.keycloak.authorization.store.StoreFactory;
+import org.keycloak.models.ClientModel;
+import org.keycloak.models.ClientProvider;
+import org.keycloak.models.Constants;
+import org.keycloak.models.KeycloakSession;
+import org.keycloak.models.RealmModel;
+import org.keycloak.models.RealmProvider;
+import org.keycloak.models.UserModel;
+import org.keycloak.models.utils.KeycloakModelUtils;
+import org.keycloak.models.utils.ModelToRepresentation;
+import org.keycloak.representations.idm.authorization.UmaPermissionRepresentation;
+import org.keycloak.representations.idm.authorization.UserPolicyRepresentation;
+import org.keycloak.testsuite.model.KeycloakModelTest;
+import org.keycloak.testsuite.model.RequireProvider;
+
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Set;
+import java.util.stream.Collectors;
+import java.util.stream.IntStream;
+
+import static org.hamcrest.CoreMatchers.not;
+import static org.hamcrest.CoreMatchers.nullValue;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.contains;
+
+@RequireProvider(AuthorizationProvider.class)
+@RequireProvider(RealmProvider.class)
+@RequireProvider(ClientProvider.class)
+public class ConcurrentAuthzTest extends KeycloakModelTest {
+
+    private String realmId;
+    private String resourceServerId;
+    private String resourceId;
+    private String adminId;
+
+    @Override
+    protected void createEnvironment(KeycloakSession s) {
+        RealmModel realm = s.realms().createRealm("test");
+        realm.setDefaultRole(s.roles().addRealmRole(realm, Constants.DEFAULT_ROLES_ROLE_PREFIX + "-" + realm.getName()));
+
+        realmId = realm.getId();
+
+        ClientModel client = s.clients().addClient(realm, "my-server");
+
+        AuthorizationProvider authorization = s.getProvider(AuthorizationProvider.class);
+        StoreFactory aStore = authorization.getStoreFactory();
+
+        ResourceServer rs = aStore.getResourceServerStore().create(client);
+        resourceServerId = rs.getId();
+        resourceId =  aStore.getResourceStore().create(rs, "myResource", client.getClientId()).getId();
+        aStore.getScopeStore().create(rs, "read");
+
+        adminId = s.users().addUser(realm, "admin").getId();
+    }
+
+    @Override
+    protected void cleanEnvironment(KeycloakSession s) {
+        s.realms().removeRealm(realmId);
+    }
+
+    @Override
+    protected boolean isUseSameKeycloakSessionFactoryForAllThreads() {
+        return true;
+    }
+
+    @Test
+    public void testPermissionRemoved() {
+        IntStream.range(0, 500).parallel().forEach(index -> {
+            String permissionId = withRealm(realmId, (session, realm) -> {
+                AuthorizationProvider authorization = session.getProvider(AuthorizationProvider.class);
+                StoreFactory aStore = authorization.getStoreFactory();
+                ResourceServer rs = aStore.getResourceServerStore().findById(realm, resourceServerId);
+
+                UserModel u = session.users().addUser(realm, "user" + index);
+
+                UmaPermissionRepresentation permission = new UmaPermissionRepresentation();
+                permission.setName(KeycloakModelUtils.generateId());
+                permission.addUser(u.getUsername());
+                permission.addScope("read");
+
+                permission.addResource(resourceId);
+                permission.setOwner(adminId);
+                return aStore.getPolicyStore().create(rs, permission).getId();
+            });
+
+            withRealm(realmId, (session, realm) -> {
+                AuthorizationProvider authorization = session.getProvider(AuthorizationProvider.class);
+                StoreFactory aStore = authorization.getStoreFactory();
+
+                aStore.getPolicyStore().delete(realm, permissionId);
+                return null;
+            });
+
+            withRealm(realmId, (session, realm) -> {
+                AuthorizationProvider authorization = session.getProvider(AuthorizationProvider.class);
+                StoreFactory aStore = authorization.getStoreFactory();
+                ResourceServer rs = aStore.getResourceServerStore().findById(realm, resourceServerId);
+
+                Map<Policy.FilterOption, String[]> searchMap = new HashMap<>();
+                searchMap.put(Policy.FilterOption.TYPE, new String[]{"uma"});
+                searchMap.put(Policy.FilterOption.OWNER, new String[]{adminId});
+                searchMap.put(Policy.FilterOption.PERMISSION, new String[] {"true"});
+                Set<String> s = aStore.getPolicyStore().find(realm, rs, searchMap, 0, 500).stream().map(Policy::getId).collect(Collectors.toSet());
+                assertThat(s, not(contains(permissionId)));
+                return null;
+            });
+        });
+    }
+
+    @Test
+    public void testStaleCacheConcurrent() {
+        String permissionId = withRealm(realmId, (session, realm) -> {
+            AuthorizationProvider authorization = session.getProvider(AuthorizationProvider.class);
+            StoreFactory aStore = authorization.getStoreFactory();
+            UserModel u = session.users().getUserById(realm, adminId);
+            ResourceServer rs = aStore.getResourceServerStore().findById(realm, resourceServerId);
+
+
+            UmaPermissionRepresentation permission = new UmaPermissionRepresentation();
+            permission.setName(KeycloakModelUtils.generateId());
+            permission.addUser(u.getUsername());
+            permission.addScope("read");
+
+            permission.addResource(resourceId);
+            permission.setOwner(adminId);
+            return aStore.getPolicyStore().create(rs, permission).getId();
+        });
+
+        IntStream.range(0, 500).parallel().forEach(index -> {
+            String createdPolicyId = withRealm(realmId, (session, realm) -> {
+                AuthorizationProvider authorization = session.getProvider(AuthorizationProvider.class);
+                StoreFactory aStore = authorization.getStoreFactory();
+                ResourceServer rs = aStore.getResourceServerStore().findById(realm, resourceServerId);
+                Policy permission = aStore.getPolicyStore().findById(realm, rs, permissionId);
+
+                UserPolicyRepresentation userRep = new UserPolicyRepresentation();
+                userRep.setName("isAdminUser" + index);
+                userRep.addUser("admin");
+                Policy associatedPolicy = aStore.getPolicyStore().create(rs, userRep);
+                permission.addAssociatedPolicy(associatedPolicy);
+                return associatedPolicy.getId();
+            });
+
+            withRealm(realmId, (session, realm) -> {
+                AuthorizationProvider authorization = session.getProvider(AuthorizationProvider.class);
+                StoreFactory aStore = authorization.getStoreFactory();
+                ResourceServer rs = aStore.getResourceServerStore().findById(realm, resourceServerId);
+                Policy permission = aStore.getPolicyStore().findById(realm, rs, permissionId);
+
+                assertThat(permission.getAssociatedPolicies(), not(contains(nullValue())));
+                ModelToRepresentation.toRepresentation(permission, authorization);
+
+                return null;
+            });
+
+            withRealm(realmId, (session, realm) -> {
+                AuthorizationProvider authorization = session.getProvider(AuthorizationProvider.class);
+                StoreFactory aStore = authorization.getStoreFactory();
+                aStore.getPolicyStore().delete(realm, createdPolicyId);
+                return null;
+            });
+        });
+    }
+}

testsuite/model/src/test/java/org/keycloak/testsuite/model/parameters/Infinispan.java

@@ -24,6 +24,8 @@ import org.keycloak.keys.infinispan.InfinispanCachePublicKeyProviderFactory;
 import org.keycloak.keys.infinispan.InfinispanPublicKeyStorageProviderFactory;
 import org.keycloak.models.ActionTokenStoreSpi;
 import org.keycloak.models.SingleUseObjectSpi;
+import org.keycloak.models.cache.authorization.CachedStoreFactorySpi;
+import org.keycloak.models.cache.infinispan.authorization.InfinispanCacheStoreFactoryProviderFactory;
 import org.keycloak.models.cache.CachePublicKeyProviderSpi;
 import org.keycloak.models.session.UserSessionPersisterSpi;
 import org.keycloak.models.sessions.infinispan.InfinispanActionTokenStoreProviderFactory;
@@ -61,6 +63,7 @@ public class Infinispan extends KeycloakModelParameters {
     static final Set<Class<? extends Spi>> ALLOWED_SPIS = ImmutableSet.<Class<? extends Spi>>builder()
       .add(AuthenticationSessionSpi.class)
       .add(CacheRealmProviderSpi.class)
+      .add(CachedStoreFactorySpi.class)
       .add(CacheUserProviderSpi.class)
       .add(InfinispanConnectionSpi.class)
       .add(StickySessionEncoderSpi.class)
@@ -77,6 +80,7 @@ public class Infinispan extends KeycloakModelParameters {
     static final Set<Class<? extends ProviderFactory>> ALLOWED_FACTORIES = ImmutableSet.<Class<? extends ProviderFactory>>builder()
       .add(InfinispanAuthenticationSessionProviderFactory.class)
       .add(InfinispanCacheRealmProviderFactory.class)
+      .add(InfinispanCacheStoreFactoryProviderFactory.class)
       .add(InfinispanClusterProviderFactory.class)
       .add(InfinispanConnectionProviderFactory.class)
       .add(InfinispanUserCacheProviderFactory.class)