From 0a0caa07d6b22b1a67d7eb7607e1587f75db9578 Mon Sep 17 00:00:00 2001 From: rmartinc Date: Wed, 3 Mar 2021 12:16:16 +0100 Subject: [PATCH] KEYCLOAK-17215 Slowness issue while hitting /auth/admin/realms/$REALM/clients?viewableOnly=true after DELETE a role --- .../cache/infinispan/RealmCacheSession.java | 5 - .../keycloak/models/jpa/JpaRealmProvider.java | 24 ++-- .../models/jpa/JpaRealmProviderFactory.java | 35 ++++- .../MapRootAuthenticationSessionProvider.java | 2 +- .../models/map/client/MapClientProvider.java | 15 ++- .../map/client/MapClientProviderFactory.java | 39 +++++- .../clientscope/MapClientScopeProvider.java | 2 +- .../models/map/group/MapGroupProvider.java | 3 +- .../map/group/MapGroupProviderFactory.java | 40 +++++- .../models/map/role/MapRoleProvider.java | 5 +- .../map/storage/MapFieldPredicates.java | 8 ++ .../models/map/storage/MapStorage.java | 3 +- .../storage/chm/ConcurrentHashMapStorage.java | 7 +- .../models/map/user/MapUserProvider.java | 2 +- .../java/org/keycloak/models/ClientModel.java | 7 +- .../org/keycloak/models/GroupProvider.java | 8 -- .../keycloak/storage/GroupStorageManager.java | 5 - .../keycloak/testsuite/admin/ClientTest.java | 51 ++++++++ .../testsuite/model/ClientModelTest.java | 122 ++++++++++++++++++ 19 files changed, 332 insertions(+), 51 deletions(-) create mode 100644 testsuite/model/src/test/java/org/keycloak/testsuite/model/ClientModelTest.java diff --git a/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/RealmCacheSession.java b/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/RealmCacheSession.java index acecf05e54..c5d71cfa70 100755 --- a/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/RealmCacheSession.java +++ b/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/RealmCacheSession.java @@ -1062,11 +1062,6 @@ public class RealmCacheSession implements CacheRealmProvider { } - @Override - public void preRemove(RealmModel realm, RoleModel role) { - getGroupDelegate().preRemove(realm, role); - } - private void addGroupEventIfAbsent(InvalidationEvent eventToAdd) { String groupId = eventToAdd.getId(); diff --git a/model/jpa/src/main/java/org/keycloak/models/jpa/JpaRealmProvider.java b/model/jpa/src/main/java/org/keycloak/models/jpa/JpaRealmProvider.java index e64f9f0e63..fb50cb5b55 100644 --- a/model/jpa/src/main/java/org/keycloak/models/jpa/JpaRealmProvider.java +++ b/model/jpa/src/main/java/org/keycloak/models/jpa/JpaRealmProvider.java @@ -52,6 +52,7 @@ import org.keycloak.models.ModelException; import org.keycloak.models.RealmModel; import org.keycloak.models.RealmProvider; import org.keycloak.models.RoleContainerModel; +import org.keycloak.models.RoleContainerModel.RoleRemovedEvent; import org.keycloak.models.RoleModel; import org.keycloak.models.RoleProvider; import org.keycloak.models.jpa.entities.ClientEntity; @@ -335,14 +336,20 @@ public class JpaRealmProvider implements RealmProvider, ClientProvider, ClientSc } String compositeRoleTable = JpaUtils.getTableNameForNativeQuery("COMPOSITE_ROLE", em); em.createNativeQuery("delete from " + compositeRoleTable + " where CHILD_ROLE = :role").setParameter("role", roleEntity).executeUpdate(); - realm.getClientsStream().forEach(c -> c.deleteScopeMapping(role)); em.createNamedQuery("deleteClientScopeRoleMappingByRole").setParameter("role", roleEntity).executeUpdate(); - session.groups().preRemove(realm, role); em.flush(); em.remove(roleEntity); - session.getKeycloakSessionFactory().publish(new RoleContainerModel.RoleRemovedEvent() { + session.getKeycloakSessionFactory().publish(roleRemovedEvent(role)); + + em.flush(); + return true; + + } + + public RoleRemovedEvent roleRemovedEvent(RoleModel role) { + return new RoleContainerModel.RoleRemovedEvent() { @Override public RoleModel getRole() { return role; @@ -352,11 +359,7 @@ public class JpaRealmProvider implements RealmProvider, ClientProvider, ClientSc public KeycloakSession getKeycloakSession() { return session; } - }); - - em.flush(); - return true; - + }; } @Override @@ -584,11 +587,14 @@ public class JpaRealmProvider implements RealmProvider, ClientProvider, ClientSc subGroup.setParent(null); } - @Override public void preRemove(RealmModel realm, RoleModel role) { // GroupProvider method implementation starts here em.createNamedQuery("deleteGroupRoleMappingsByRole").setParameter("roleId", role.getId()).executeUpdate(); // GroupProvider method implementation ends here + + // ClientProvider implementation + String clientScopeMapping = JpaUtils.getTableNameForNativeQuery("SCOPE_MAPPING", em); + em.createNativeQuery("delete from " + clientScopeMapping + " where ROLE_ID = :role").setParameter("role", role.getId()).executeUpdate(); } @Override diff --git a/model/jpa/src/main/java/org/keycloak/models/jpa/JpaRealmProviderFactory.java b/model/jpa/src/main/java/org/keycloak/models/jpa/JpaRealmProviderFactory.java index 843d61982d..6a60fed198 100755 --- a/model/jpa/src/main/java/org/keycloak/models/jpa/JpaRealmProviderFactory.java +++ b/model/jpa/src/main/java/org/keycloak/models/jpa/JpaRealmProviderFactory.java @@ -21,16 +21,24 @@ import org.keycloak.Config; import org.keycloak.connections.jpa.JpaConnectionProvider; import org.keycloak.models.KeycloakSession; import org.keycloak.models.KeycloakSessionFactory; -import org.keycloak.models.RealmProvider; import org.keycloak.models.RealmProviderFactory; import javax.persistence.EntityManager; +import org.keycloak.models.ClientModel; +import org.keycloak.models.RealmModel; +import org.keycloak.models.RoleContainerModel; +import org.keycloak.models.RoleContainerModel.RoleRemovedEvent; +import org.keycloak.models.RoleModel; +import org.keycloak.provider.ProviderEvent; +import org.keycloak.provider.ProviderEventListener; /** * @author Bill Burke * @version $Revision: 1 $ */ -public class JpaRealmProviderFactory implements RealmProviderFactory { +public class JpaRealmProviderFactory implements RealmProviderFactory, ProviderEventListener { + + private Runnable onClose; @Override public void init(Config.Scope config) { @@ -38,7 +46,8 @@ public class JpaRealmProviderFactory implements RealmProviderFactory { @Override public void postInit(KeycloakSessionFactory factory) { - + factory.register(this); + onClose = () -> factory.unregister(this); } @Override @@ -47,13 +56,31 @@ public class JpaRealmProviderFactory implements RealmProviderFactory { } @Override - public RealmProvider create(KeycloakSession session) { + public JpaRealmProvider create(KeycloakSession session) { EntityManager em = session.getProvider(JpaConnectionProvider.class).getEntityManager(); return new JpaRealmProvider(session, em); } @Override public void close() { + onClose.run(); } + @Override + public void onEvent(ProviderEvent event) { + if (event instanceof RoleContainerModel.RoleRemovedEvent) { + RoleRemovedEvent e = (RoleContainerModel.RoleRemovedEvent) event; + RoleModel role = e.getRole(); + RoleContainerModel container = role.getContainer(); + RealmModel realm; + if (container instanceof RealmModel) { + realm = (RealmModel) container; + } else if (container instanceof ClientModel) { + realm = ((ClientModel) container).getRealm(); + } else { + return; + } + create(e.getKeycloakSession()).preRemove(realm, role); + } + } } diff --git a/model/map/src/main/java/org/keycloak/models/map/authSession/MapRootAuthenticationSessionProvider.java b/model/map/src/main/java/org/keycloak/models/map/authSession/MapRootAuthenticationSessionProvider.java index bcabc99115..587f8d2c51 100644 --- a/model/map/src/main/java/org/keycloak/models/map/authSession/MapRootAuthenticationSessionProvider.java +++ b/model/map/src/main/java/org/keycloak/models/map/authSession/MapRootAuthenticationSessionProvider.java @@ -58,7 +58,7 @@ public class MapRootAuthenticationSessionProvider implements AuthenticationSessi public MapRootAuthenticationSessionProvider(KeycloakSession session, MapStorage sessionStore) { this.session = session; this.sessionStore = sessionStore; - this.tx = sessionStore.createTransaction(); + this.tx = sessionStore.createTransaction(session); session.getTransactionManager().enlistAfterCompletion(tx); } diff --git a/model/map/src/main/java/org/keycloak/models/map/client/MapClientProvider.java b/model/map/src/main/java/org/keycloak/models/map/client/MapClientProvider.java index b4bb27d627..d561b52879 100644 --- a/model/map/src/main/java/org/keycloak/models/map/client/MapClientProvider.java +++ b/model/map/src/main/java/org/keycloak/models/map/client/MapClientProvider.java @@ -25,6 +25,7 @@ import org.keycloak.models.ClientProvider; import org.keycloak.models.KeycloakSession; import org.keycloak.models.ModelDuplicateException; import org.keycloak.models.RealmModel; +import org.keycloak.models.RoleModel; import org.keycloak.models.map.storage.MapKeycloakTransaction; import org.keycloak.models.map.common.Serialization; @@ -64,7 +65,7 @@ public class MapClientProvider implements ClientProvider { this.session = session; this.clientStore = clientStore; this.clientRegisteredNodesStore = clientRegisteredNodesStore; - this.tx = clientStore.createTransaction(); + this.tx = clientStore.createTransaction(session); session.getTransactionManager().enlist(tx); } @@ -321,6 +322,18 @@ public class MapClientProvider implements ClientProvider { .collect(Collectors.toMap(ClientScopeModel::getName, Function.identity())); } + public void preRemove(RealmModel realm, RoleModel role) { + ModelCriteriaBuilder mcb = clientStore.createCriteriaBuilder() + .compare(SearchableFields.REALM_ID, Operator.EQ, realm.getId()) + .compare(SearchableFields.SCOPE_MAPPING_ROLE, Operator.EQ, role.getId()); + try (Stream toRemove = tx.getUpdatedNotRemoved(mcb)) { + toRemove + .map(clientEntity -> session.clients().getClientById(realm, clientEntity.getId().toString())) + .filter(Objects::nonNull) + .forEach(clientModel -> clientModel.deleteScopeMapping(role)); + } + } + @Override public void close() { diff --git a/model/map/src/main/java/org/keycloak/models/map/client/MapClientProviderFactory.java b/model/map/src/main/java/org/keycloak/models/map/client/MapClientProviderFactory.java index 20e030f12a..24afd9bcb1 100644 --- a/model/map/src/main/java/org/keycloak/models/map/client/MapClientProviderFactory.java +++ b/model/map/src/main/java/org/keycloak/models/map/client/MapClientProviderFactory.java @@ -22,31 +22,66 @@ import org.keycloak.models.ClientProvider; import org.keycloak.models.ClientProviderFactory; import org.keycloak.models.KeycloakSession; import org.keycloak.models.KeycloakSessionFactory; +import org.keycloak.models.RealmModel; +import org.keycloak.models.RoleContainerModel; +import org.keycloak.models.RoleContainerModel.RoleRemovedEvent; +import org.keycloak.models.RoleModel; import java.util.UUID; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ConcurrentMap; import org.keycloak.models.map.storage.MapStorageProvider; import org.keycloak.models.map.storage.MapStorage; +import org.keycloak.provider.ProviderEvent; +import org.keycloak.provider.ProviderEventListener; /** * * @author hmlnarik */ -public class MapClientProviderFactory extends AbstractMapProviderFactory implements ClientProviderFactory { +public class MapClientProviderFactory extends AbstractMapProviderFactory implements ClientProviderFactory, ProviderEventListener { private final ConcurrentHashMap> REGISTERED_NODES_STORE = new ConcurrentHashMap<>(); private MapStorage store; + private Runnable onClose; + @Override public void postInit(KeycloakSessionFactory factory) { MapStorageProvider sp = (MapStorageProvider) factory.getProviderFactory(MapStorageProvider.class); this.store = sp.getStorage("clients", UUID.class, MapClientEntity.class, ClientModel.class); + + factory.register(this); + onClose = () -> factory.unregister(this); } @Override - public ClientProvider create(KeycloakSession session) { + public MapClientProvider create(KeycloakSession session) { return new MapClientProvider(session, store, REGISTERED_NODES_STORE); } + + @Override + public void close() { + super.close(); + onClose.run(); + } + + @Override + public void onEvent(ProviderEvent event) { + if (event instanceof RoleContainerModel.RoleRemovedEvent) { + RoleRemovedEvent e = (RoleContainerModel.RoleRemovedEvent) event; + RoleModel role = e.getRole(); + RoleContainerModel container = role.getContainer(); + RealmModel realm; + if (container instanceof RealmModel) { + realm = (RealmModel) container; + } else if (container instanceof ClientModel) { + realm = ((ClientModel) container).getRealm(); + } else { + return; + } + create(e.getKeycloakSession()).preRemove(realm, role); + } + } } diff --git a/model/map/src/main/java/org/keycloak/models/map/clientscope/MapClientScopeProvider.java b/model/map/src/main/java/org/keycloak/models/map/clientscope/MapClientScopeProvider.java index ebf52c7f6b..7b996043f6 100644 --- a/model/map/src/main/java/org/keycloak/models/map/clientscope/MapClientScopeProvider.java +++ b/model/map/src/main/java/org/keycloak/models/map/clientscope/MapClientScopeProvider.java @@ -53,7 +53,7 @@ public class MapClientScopeProvider implements ClientScopeProvider { public MapClientScopeProvider(KeycloakSession session, MapStorage clientScopeStore) { this.session = session; this.clientScopeStore = clientScopeStore; - this.tx = clientScopeStore.createTransaction(); + this.tx = clientScopeStore.createTransaction(session); session.getTransactionManager().enlist(tx); } diff --git a/model/map/src/main/java/org/keycloak/models/map/group/MapGroupProvider.java b/model/map/src/main/java/org/keycloak/models/map/group/MapGroupProvider.java index 2a805c3d2d..3269855b8a 100644 --- a/model/map/src/main/java/org/keycloak/models/map/group/MapGroupProvider.java +++ b/model/map/src/main/java/org/keycloak/models/map/group/MapGroupProvider.java @@ -51,7 +51,7 @@ public class MapGroupProvider implements GroupProvider { public MapGroupProvider(KeycloakSession session, MapStorage groupStore) { this.session = session; this.groupStore = groupStore; - this.tx = groupStore.createTransaction(); + this.tx = groupStore.createTransaction(session); session.getTransactionManager().enlist(tx); } @@ -295,7 +295,6 @@ public class MapGroupProvider implements GroupProvider { subGroup.setParent(null); } - @Override public void preRemove(RealmModel realm, RoleModel role) { LOG.tracef("preRemove(%s, %s)%s", realm, role, getShortStackTrace()); ModelCriteriaBuilder mcb = groupStore.createCriteriaBuilder() diff --git a/model/map/src/main/java/org/keycloak/models/map/group/MapGroupProviderFactory.java b/model/map/src/main/java/org/keycloak/models/map/group/MapGroupProviderFactory.java index 3e1b7ec525..36ce727f95 100644 --- a/model/map/src/main/java/org/keycloak/models/map/group/MapGroupProviderFactory.java +++ b/model/map/src/main/java/org/keycloak/models/map/group/MapGroupProviderFactory.java @@ -17,34 +17,70 @@ package org.keycloak.models.map.group; +import org.keycloak.models.ClientModel; import org.keycloak.models.GroupModel; import org.keycloak.models.GroupProvider; import org.keycloak.models.GroupProviderFactory; import org.keycloak.models.KeycloakSession; import org.keycloak.models.KeycloakSessionFactory; +import org.keycloak.models.RealmModel; +import org.keycloak.models.RoleContainerModel; +import org.keycloak.models.RoleContainerModel.RoleRemovedEvent; +import org.keycloak.models.RoleModel; import org.keycloak.models.map.common.AbstractMapProviderFactory; import org.keycloak.models.map.storage.MapStorage; import org.keycloak.models.map.storage.MapStorageProvider; +import org.keycloak.provider.ProviderEvent; +import org.keycloak.provider.ProviderEventListener; import java.util.UUID; /** * * @author mhajas */ -public class MapGroupProviderFactory extends AbstractMapProviderFactory implements GroupProviderFactory { +public class MapGroupProviderFactory extends AbstractMapProviderFactory implements GroupProviderFactory, ProviderEventListener { private MapStorage store; + private Runnable onClose; + @Override public void postInit(KeycloakSessionFactory factory) { MapStorageProvider sp = (MapStorageProvider) factory.getProviderFactory(MapStorageProvider.class); this.store = sp.getStorage("groups", UUID.class, MapGroupEntity.class, GroupModel.class); + + factory.register(this); + onClose = () -> factory.unregister(this); } @Override - public GroupProvider create(KeycloakSession session) { + public MapGroupProvider create(KeycloakSession session) { return new MapGroupProvider(session, store); } + + @Override + public void close() { + super.close(); + onClose.run(); + } + + @Override + public void onEvent(ProviderEvent event) { + if (event instanceof RoleContainerModel.RoleRemovedEvent) { + RoleRemovedEvent e = (RoleContainerModel.RoleRemovedEvent) event; + RoleModel role = e.getRole(); + RoleContainerModel container = role.getContainer(); + RealmModel realm; + if (container instanceof RealmModel) { + realm = (RealmModel) container; + } else if (container instanceof ClientModel) { + realm = ((ClientModel) container).getRealm(); + } else { + return; + } + create(e.getKeycloakSession()).preRemove(realm, role); + } + } } diff --git a/model/map/src/main/java/org/keycloak/models/map/role/MapRoleProvider.java b/model/map/src/main/java/org/keycloak/models/map/role/MapRoleProvider.java index dd436080ef..457ce30ad3 100644 --- a/model/map/src/main/java/org/keycloak/models/map/role/MapRoleProvider.java +++ b/model/map/src/main/java/org/keycloak/models/map/role/MapRoleProvider.java @@ -30,7 +30,6 @@ import java.util.Comparator; import java.util.Objects; import java.util.UUID; import java.util.function.Function; -import java.util.function.Predicate; import java.util.stream.Collectors; import java.util.stream.Stream; import org.keycloak.models.map.storage.MapStorage; @@ -67,7 +66,7 @@ public class MapRoleProvider implements RoleProvider { public MapRoleProvider(KeycloakSession session, MapStorage roleStore) { this.session = session; this.roleStore = roleStore; - this.tx = roleStore.createTransaction(); + this.tx = roleStore.createTransaction(session); session.getTransactionManager().enlist(tx); } @@ -210,8 +209,6 @@ public class MapRoleProvider implements RoleProvider { } }); - session.groups().preRemove(realm, role); - // TODO: Sending an event should be extracted to store layer session.getKeycloakSessionFactory().publish(new RoleContainerModel.RoleRemovedEvent() { @Override diff --git a/model/map/src/main/java/org/keycloak/models/map/storage/MapFieldPredicates.java b/model/map/src/main/java/org/keycloak/models/map/storage/MapFieldPredicates.java index ef8cc1448e..cb44ee52ff 100644 --- a/model/map/src/main/java/org/keycloak/models/map/storage/MapFieldPredicates.java +++ b/model/map/src/main/java/org/keycloak/models/map/storage/MapFieldPredicates.java @@ -62,6 +62,7 @@ public class MapFieldPredicates { static { put(CLIENT_PREDICATES, ClientModel.SearchableFields.REALM_ID, AbstractClientEntity::getRealmId); put(CLIENT_PREDICATES, ClientModel.SearchableFields.CLIENT_ID, AbstractClientEntity::getClientId); + put(CLIENT_PREDICATES, ClientModel.SearchableFields.SCOPE_MAPPING_ROLE, MapFieldPredicates::checkScopeMappingRole); put(CLIENT_SCOPE_PREDICATES, ClientScopeModel.SearchableFields.REALM_ID, AbstractClientScopeEntity::getRealmId); put(CLIENT_SCOPE_PREDICATES, ClientScopeModel.SearchableFields.NAME, AbstractClientScopeEntity::getName); @@ -136,6 +137,13 @@ public class MapFieldPredicates { return s; } + private static MapModelCriteriaBuilder, ClientModel> checkScopeMappingRole(MapModelCriteriaBuilder, ClientModel> mcb, Operator op, Object[] values) { + String roleIdS = ensureEqSingleValue(ClientModel.SearchableFields.SCOPE_MAPPING_ROLE, "role_id", op, values); + Function, ?> getter; + getter = ce -> ce.getScopeMappings().contains(roleIdS); + return mcb.fieldCompare(Boolean.TRUE::equals, getter); + } + private static MapModelCriteriaBuilder, GroupModel> checkGrantedGroupRole(MapModelCriteriaBuilder, GroupModel> mcb, Operator op, Object[] values) { String roleIdS = ensureEqSingleValue(GroupModel.SearchableFields.ASSIGNED_ROLE, "role_id", op, values); Function, ?> getter; diff --git a/model/map/src/main/java/org/keycloak/models/map/storage/MapStorage.java b/model/map/src/main/java/org/keycloak/models/map/storage/MapStorage.java index 879e223ef7..d571b5ceef 100644 --- a/model/map/src/main/java/org/keycloak/models/map/storage/MapStorage.java +++ b/model/map/src/main/java/org/keycloak/models/map/storage/MapStorage.java @@ -16,6 +16,7 @@ */ package org.keycloak.models.map.storage; +import org.keycloak.models.KeycloakSession; import org.keycloak.models.map.common.AbstractEntity; import java.util.stream.Stream; @@ -127,6 +128,6 @@ public interface MapStorage, M> { * * @return See description. */ - public MapKeycloakTransaction createTransaction(); + public MapKeycloakTransaction createTransaction(KeycloakSession session); } diff --git a/model/map/src/main/java/org/keycloak/models/map/storage/chm/ConcurrentHashMapStorage.java b/model/map/src/main/java/org/keycloak/models/map/storage/chm/ConcurrentHashMapStorage.java index 50746ab40c..760fad0959 100644 --- a/model/map/src/main/java/org/keycloak/models/map/storage/chm/ConcurrentHashMapStorage.java +++ b/model/map/src/main/java/org/keycloak/models/map/storage/chm/ConcurrentHashMapStorage.java @@ -16,6 +16,7 @@ */ package org.keycloak.models.map.storage.chm; +import org.keycloak.models.KeycloakSession; import org.keycloak.models.map.storage.MapModelCriteriaBuilder; import org.keycloak.models.map.common.AbstractEntity; import org.keycloak.models.map.storage.MapFieldPredicates; @@ -101,8 +102,10 @@ public class ConcurrentHashMapStorage, M> impleme } @Override - public MapKeycloakTransaction createTransaction() { - return new MapKeycloakTransaction<>(this); + @SuppressWarnings("unchecked") + public MapKeycloakTransaction createTransaction(KeycloakSession session) { + MapKeycloakTransaction sessionTransaction = session.getAttribute("map-transaction-" + hashCode(), MapKeycloakTransaction.class); + return sessionTransaction == null ? new MapKeycloakTransaction<>(this) : (MapKeycloakTransaction) sessionTransaction; } diff --git a/model/map/src/main/java/org/keycloak/models/map/user/MapUserProvider.java b/model/map/src/main/java/org/keycloak/models/map/user/MapUserProvider.java index 50fb0aebbb..c8663b1109 100644 --- a/model/map/src/main/java/org/keycloak/models/map/user/MapUserProvider.java +++ b/model/map/src/main/java/org/keycloak/models/map/user/MapUserProvider.java @@ -83,7 +83,7 @@ public class MapUserProvider implements UserProvider.Streams, UserCredentialStor public MapUserProvider(KeycloakSession session, MapStorage store) { this.session = session; this.userStore = store; - this.tx = userStore.createTransaction(); + this.tx = userStore.createTransaction(session); session.getTransactionManager().enlist(tx); } diff --git a/server-spi/src/main/java/org/keycloak/models/ClientModel.java b/server-spi/src/main/java/org/keycloak/models/ClientModel.java index 3571bae196..eeb1b83c6a 100755 --- a/server-spi/src/main/java/org/keycloak/models/ClientModel.java +++ b/server-spi/src/main/java/org/keycloak/models/ClientModel.java @@ -38,9 +38,10 @@ public interface ClientModel extends ClientScopeModel, RoleContainerModel, Prot String X509CERTIFICATE = "X509Certificate"; public static class SearchableFields { - public static final SearchableModelField ID = new SearchableModelField<>("id", String.class); - public static final SearchableModelField REALM_ID = new SearchableModelField<>("realmId", String.class); - public static final SearchableModelField CLIENT_ID = new SearchableModelField<>("clientId", String.class); + public static final SearchableModelField ID = new SearchableModelField<>("id", String.class); + public static final SearchableModelField REALM_ID = new SearchableModelField<>("realmId", String.class); + public static final SearchableModelField CLIENT_ID = new SearchableModelField<>("clientId", String.class); + public static final SearchableModelField SCOPE_MAPPING_ROLE = new SearchableModelField<>("scopeMappingRole", String.class); } interface ClientCreationEvent extends ProviderEvent { diff --git a/server-spi/src/main/java/org/keycloak/models/GroupProvider.java b/server-spi/src/main/java/org/keycloak/models/GroupProvider.java index b462afc428..2373b3591a 100644 --- a/server-spi/src/main/java/org/keycloak/models/GroupProvider.java +++ b/server-spi/src/main/java/org/keycloak/models/GroupProvider.java @@ -287,12 +287,4 @@ public interface GroupProvider extends Provider, GroupLookupProvider { * @throws ModelDuplicateException If there is already a top level group name with the same name */ void addTopLevelGroup(RealmModel realm, GroupModel subGroup); - - /** - * This function is called when a role is removed; this serves for removing references from groups to roles. - * - * @param realm Realm. - * @param role Role which will be removed. - */ - void preRemove(RealmModel realm, RoleModel role); } diff --git a/services/src/main/java/org/keycloak/storage/GroupStorageManager.java b/services/src/main/java/org/keycloak/storage/GroupStorageManager.java index daad4e0de9..24ea6b4f29 100644 --- a/services/src/main/java/org/keycloak/storage/GroupStorageManager.java +++ b/services/src/main/java/org/keycloak/storage/GroupStorageManager.java @@ -124,11 +124,6 @@ public class GroupStorageManager extends AbstractStorageManager protocolMappers = mappersResource.getMappers(); diff --git a/testsuite/model/src/test/java/org/keycloak/testsuite/model/ClientModelTest.java b/testsuite/model/src/test/java/org/keycloak/testsuite/model/ClientModelTest.java new file mode 100644 index 0000000000..6344d57199 --- /dev/null +++ b/testsuite/model/src/test/java/org/keycloak/testsuite/model/ClientModelTest.java @@ -0,0 +1,122 @@ +/* + * Copyright 2021 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; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.is; +import org.junit.Test; +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.RoleModel; +import org.keycloak.models.RoleProvider; + +/** + * + * @author rmartinc + */ +@RequireProvider(RealmProvider.class) +@RequireProvider(ClientProvider.class) +@RequireProvider(RoleProvider.class) +public class ClientModelTest extends KeycloakModelTest { + + private String realmId; + + @Override + public void createEnvironment(KeycloakSession s) { + RealmModel realm = s.realms().createRealm("realm"); + realm.setDefaultRole(s.roles().addRealmRole(realm, Constants.DEFAULT_ROLES_ROLE_PREFIX + "-" + realm.getName())); + this.realmId = realm.getId(); + } + + @Override + public void cleanEnvironment(KeycloakSession s) { + s.realms().removeRealm(realmId); + } + + @Test + public void testScopeMappingRoleRemoval() { + // create two clients, one realm role and one client role and assign both to one of the clients + inComittedTransaction(1, (session , i) -> { + final RealmModel realm = session.realms().getRealm(realmId); + ClientModel client1 = session.clients().addClient(realm, "client1"); + ClientModel client2 = session.clients().addClient(realm, "client2"); + RoleModel realmRole = session.roles().addRealmRole(realm, "realm-role"); + RoleModel client2Role = session.roles().addClientRole(client2, "client2-role"); + client1.addScopeMapping(realmRole); + client1.addScopeMapping(client2Role); + return null; + }); + + // check everything is OK + inComittedTransaction(1, (session, i) -> { + final RealmModel realm = session.realms().getRealm(realmId); + final ClientModel client1 = session.clients().getClientByClientId(realm, "client1"); + assertThat(client1.getScopeMappingsStream().count(), is(2L)); + assertThat(client1.getScopeMappingsStream().filter(r -> r.getName().equals("realm-role")).count(), is(1L)); + assertThat(client1.getScopeMappingsStream().filter(r -> r.getName().equals("client2-role")).count(), is(1L)); + return null; + }); + + // remove the realm role + inComittedTransaction(1, (session, i) -> { + final RealmModel realm = session.realms().getRealm(realmId); + final RoleModel role = session.roles().getRealmRole(realm, "realm-role"); + session.roles().removeRole(role); + return null; + }); + + // check it is removed + inComittedTransaction(1, (session, i) -> { + final RealmModel realm = session.realms().getRealm(realmId); + final ClientModel client1 = session.clients().getClientByClientId(realm, "client1"); + assertThat(client1.getScopeMappingsStream().count(), is(1L)); + assertThat(client1.getScopeMappingsStream().filter(r -> r.getName().equals("client2-role")).count(), is(1L)); + return null; + }); + + // remove client role + inComittedTransaction(1, (session, i) -> { + final RealmModel realm = session.realms().getRealm(realmId); + final ClientModel client2 = session.clients().getClientByClientId(realm, "client2"); + final RoleModel role = session.roles().getClientRole(client2, "client2-role"); + session.roles().removeRole(role); + return null; + }); + + // check both clients are removed + inComittedTransaction(1, (session, i) -> { + final RealmModel realm = session.realms().getRealm(realmId); + final ClientModel client1 = session.clients().getClientByClientId(realm, "client1"); + assertThat(client1.getScopeMappingsStream().count(), is(0L)); + return null; + }); + + // remove clients + inComittedTransaction(1, (session , i) -> { + final RealmModel realm = session.realms().getRealm(realmId); + final ClientModel client1 = session.clients().getClientByClientId(realm, "client1"); + final ClientModel client2 = session.clients().getClientByClientId(realm, "client2"); + session.clients().removeClient(realm, client1.getId()); + session.clients().removeClient(realm, client2.getId()); + return null; + }); + } +}