From 90b2a8c56a00a48603be12fcdda9a28e9a730b17 Mon Sep 17 00:00:00 2001 From: Michal Hajas Date: Fri, 29 Apr 2022 13:20:26 +0200 Subject: [PATCH] Update composite roles on child role removal Closes #11769 --- .github/workflows/ci.yml | 3 +- .../storage/hotRod/role/HotRodRoleEntity.java | 1 + .../jpa/role/JpaRoleModelCriteriaBuilder.java | 10 ++++ .../role/LdapRoleModelCriteriaBuilder.java | 3 + .../models/map/role/MapRoleProvider.java | 8 +++ .../map/role/MapRoleProviderFactory.java | 3 + .../map/storage/chm/MapFieldPredicates.java | 9 +++ .../java/org/keycloak/models/RoleModel.java | 1 + .../testsuite/model/role/RoleModelTest.java | 56 +++++++++++++++++++ 9 files changed, 92 insertions(+), 2 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 2f7a02c65c3c..4bd790f3421d 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -143,8 +143,7 @@ jobs: fetch-depth: 2 - name: Check whether HEAD^ contains HotRod storage relevant changes - run: echo "GIT_HOTROD_RELEVANT_DIFF=$( git diff --name-only HEAD^ | egrep -ic -e 'non-existent-folder' )" >> $GITHUB_ENV -# run: echo "GIT_HOTROD_RELEVANT_DIFF=$( git diff --name-only HEAD^ | egrep -ic -e '^model/hot-rod|^model/map|^model/build-processor|^testsuite/model' )" >> $GITHUB_ENV + run: echo "GIT_HOTROD_RELEVANT_DIFF=$( git diff --name-only HEAD^ | egrep -ic -e '^model/hot-rod|^model/map|^model/build-processor|^testsuite/model' )" >> $GITHUB_ENV - name: Cache Maven packages if: ${{ github.event_name != 'pull_request' || matrix.server != 'undertow-map-hot-rod' || env.GIT_HOTROD_RELEVANT_DIFF != 0 }} diff --git a/model/map-hot-rod/src/main/java/org/keycloak/models/map/storage/hotRod/role/HotRodRoleEntity.java b/model/map-hot-rod/src/main/java/org/keycloak/models/map/storage/hotRod/role/HotRodRoleEntity.java index 75cda08e0c4d..3ad5c0d2dfa4 100644 --- a/model/map-hot-rod/src/main/java/org/keycloak/models/map/storage/hotRod/role/HotRodRoleEntity.java +++ b/model/map-hot-rod/src/main/java/org/keycloak/models/map/storage/hotRod/role/HotRodRoleEntity.java @@ -93,6 +93,7 @@ public void setName(String name) { @ProtoField(number = 8) public String clientId; + @ProtoDoc("@Field(index = Index.YES, store = Store.YES)") @ProtoField(number = 9) public Set compositeRoles; diff --git a/model/map-jpa/src/main/java/org/keycloak/models/map/storage/jpa/role/JpaRoleModelCriteriaBuilder.java b/model/map-jpa/src/main/java/org/keycloak/models/map/storage/jpa/role/JpaRoleModelCriteriaBuilder.java index 8a251fa732a1..30506589efd2 100644 --- a/model/map-jpa/src/main/java/org/keycloak/models/map/storage/jpa/role/JpaRoleModelCriteriaBuilder.java +++ b/model/map-jpa/src/main/java/org/keycloak/models/map/storage/jpa/role/JpaRoleModelCriteriaBuilder.java @@ -35,6 +35,7 @@ import org.keycloak.models.map.common.StringKeyConverter.UUIDKey; import org.keycloak.models.map.storage.CriterionNotSupportedException; import org.keycloak.models.map.storage.jpa.JpaModelCriteriaBuilder; +import org.keycloak.models.map.storage.jpa.hibernate.jsonb.JsonbType; import org.keycloak.models.map.storage.jpa.role.entity.JpaRoleEntity; import org.keycloak.storage.SearchableModelField; @@ -61,6 +62,15 @@ public JpaRoleModelCriteriaBuilder compare(SearchableModelField cb.equal(root.get(modelField.getName()), value[0]) ); + } else if (modelField == SearchableFields.COMPOSITE_ROLE){ + validateValue(value, modelField, op, String.class); + + return new JpaRoleModelCriteriaBuilder((cb, root) -> + cb.isTrue(cb.function("@>", + Boolean.TYPE, + cb.function("->", JsonbType.class, root.get("metadata"), cb.literal("fCompositeRoles")), + cb.literal(convertToJson(value[0])))) + ); } else { throw new CriterionNotSupportedException(modelField, op); } diff --git a/model/map-ldap/src/main/java/org/keycloak/models/map/storage/ldap/role/LdapRoleModelCriteriaBuilder.java b/model/map-ldap/src/main/java/org/keycloak/models/map/storage/ldap/role/LdapRoleModelCriteriaBuilder.java index 962e93b347de..0005db8c2b9e 100644 --- a/model/map-ldap/src/main/java/org/keycloak/models/map/storage/ldap/role/LdapRoleModelCriteriaBuilder.java +++ b/model/map-ldap/src/main/java/org/keycloak/models/map/storage/ldap/role/LdapRoleModelCriteriaBuilder.java @@ -140,6 +140,9 @@ public LdapRoleModelCriteriaBuilder compare(SearchableModelField equal(field, value[0], LdapMapEscapeStrategy.DEFAULT, false)); + } else if (modelField == RoleModel.SearchableFields.COMPOSITE_ROLE) { + // Not supported at the moment + return new LdapRoleModelCriteriaBuilder(roleMapperConfig, StringBuilder::new); } else { throw new CriterionNotSupportedException(modelField, op); } 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 767d700b10f3..cb3157f60ebf 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 @@ -279,6 +279,14 @@ public void preRemove(RealmModel realm) { tx.delete(withCriteria(mcb)); } + public void preRemove(RealmModel realm, RoleModel role) { + // Remove reference from all composite roles + DefaultModelCriteria mcb = criteria(); + mcb = mcb.compare(SearchableFields.REALM_ID, Operator.EQ, realm.getId()) + .compare(SearchableFields.COMPOSITE_ROLE, Operator.EQ, role.getId()); + tx.read(withCriteria(mcb)).forEach(mapRoleEntity -> mapRoleEntity.removeCompositeRole(role.getId())); + } + @Override public void close() { } diff --git a/model/map/src/main/java/org/keycloak/models/map/role/MapRoleProviderFactory.java b/model/map/src/main/java/org/keycloak/models/map/role/MapRoleProviderFactory.java index 74b7420ee0ef..7f729caf63e7 100644 --- a/model/map/src/main/java/org/keycloak/models/map/role/MapRoleProviderFactory.java +++ b/model/map/src/main/java/org/keycloak/models/map/role/MapRoleProviderFactory.java @@ -28,6 +28,7 @@ import static org.keycloak.models.map.common.AbstractMapProviderFactory.MapProviderObjectType.CLIENT_BEFORE_REMOVE; import static org.keycloak.models.map.common.AbstractMapProviderFactory.MapProviderObjectType.REALM_BEFORE_REMOVE; import static org.keycloak.models.map.common.AbstractMapProviderFactory.MapProviderObjectType.ROLE_AFTER_REMOVE; +import static org.keycloak.models.map.common.AbstractMapProviderFactory.MapProviderObjectType.ROLE_BEFORE_REMOVE; public class MapRoleProviderFactory extends AbstractMapProviderFactory implements RoleProviderFactory, InvalidationHandler { @@ -51,6 +52,8 @@ public void invalidate(KeycloakSession session, InvalidableObjectType type, Obje create(session).preRemove((RealmModel) params[0]); } else if (type == CLIENT_BEFORE_REMOVE) { create(session).removeRoles((ClientModel) params[1]); + } else if (type == ROLE_BEFORE_REMOVE) { + create(session).preRemove((RealmModel) params[0], (RoleModel) params[1]); } else if (type == ROLE_AFTER_REMOVE) { session.getKeycloakSessionFactory().publish(new RoleContainerModel.RoleRemovedEvent() { @Override public RoleModel getRole() { return (RoleModel) params[1]; } diff --git a/model/map/src/main/java/org/keycloak/models/map/storage/chm/MapFieldPredicates.java b/model/map/src/main/java/org/keycloak/models/map/storage/chm/MapFieldPredicates.java index 082d0dec1eed..a4d5c02179bd 100644 --- a/model/map/src/main/java/org/keycloak/models/map/storage/chm/MapFieldPredicates.java +++ b/model/map/src/main/java/org/keycloak/models/map/storage/chm/MapFieldPredicates.java @@ -122,6 +122,7 @@ public class MapFieldPredicates { put(ROLE_PREDICATES, RoleModel.SearchableFields.DESCRIPTION, MapRoleEntity::getDescription); put(ROLE_PREDICATES, RoleModel.SearchableFields.NAME, MapRoleEntity::getName); put(ROLE_PREDICATES, RoleModel.SearchableFields.IS_CLIENT_ROLE, MapRoleEntity::isClientRole); + put(ROLE_PREDICATES, RoleModel.SearchableFields.COMPOSITE_ROLE, MapFieldPredicates::checkCompositeRoles); put(USER_PREDICATES, UserModel.SearchableFields.REALM_ID, MapUserEntity::getRealmId); put(USER_PREDICATES, UserModel.SearchableFields.USERNAME, MapUserEntity::getUsername); @@ -326,6 +327,14 @@ private static MapModelCriteriaBuilder che return mcb.fieldCompare(Boolean.TRUE::equals, getter); } + private static MapModelCriteriaBuilder checkCompositeRoles(MapModelCriteriaBuilder mcb, Operator op, Object[] values) { + String roleIdS = ensureEqSingleValue(RoleModel.SearchableFields.COMPOSITE_ROLE, "composite_role_id", op, values); + Function getter; + getter = re -> Optional.ofNullable(re.getCompositeRoles()).orElseGet(Collections::emptySet).contains(roleIdS); + + return mcb.fieldCompare(Boolean.TRUE::equals, getter); + } + private static MapModelCriteriaBuilder checkGrantedUserRole(MapModelCriteriaBuilder mcb, Operator op, Object[] values) { String roleIdS = ensureEqSingleValue(UserModel.SearchableFields.ASSIGNED_ROLE, "role_id", op, values); Function getter; diff --git a/server-spi/src/main/java/org/keycloak/models/RoleModel.java b/server-spi/src/main/java/org/keycloak/models/RoleModel.java index e0289a922a21..cc36641ea7da 100755 --- a/server-spi/src/main/java/org/keycloak/models/RoleModel.java +++ b/server-spi/src/main/java/org/keycloak/models/RoleModel.java @@ -38,6 +38,7 @@ public static class SearchableFields { public static final SearchableModelField NAME = new SearchableModelField<>("name", String.class); public static final SearchableModelField DESCRIPTION = new SearchableModelField<>("description", String.class); public static final SearchableModelField IS_CLIENT_ROLE = new SearchableModelField<>("isClientRole", Boolean.class); + public static final SearchableModelField COMPOSITE_ROLE = new SearchableModelField<>("compositeRoles", Boolean.class); } String getName(); diff --git a/testsuite/model/src/test/java/org/keycloak/testsuite/model/role/RoleModelTest.java b/testsuite/model/src/test/java/org/keycloak/testsuite/model/role/RoleModelTest.java index cfd2ef73b523..19526d55aaf1 100644 --- a/testsuite/model/src/test/java/org/keycloak/testsuite/model/role/RoleModelTest.java +++ b/testsuite/model/src/test/java/org/keycloak/testsuite/model/role/RoleModelTest.java @@ -16,6 +16,7 @@ import java.util.Collection; import java.util.List; import java.util.UUID; +import java.util.concurrent.atomic.AtomicReference; import java.util.stream.Collectors; import java.util.stream.IntStream; import java.util.stream.Stream; @@ -227,6 +228,61 @@ public void testSearchRolesByDescription() { }); } + @Test + public void testCompositeRolesUpdateOnChildRoleRemoval() { + final AtomicReference parentRealmRoleId = new AtomicReference<>(); + final AtomicReference parentClientRoleId = new AtomicReference<>(); + + final AtomicReference childRealmRoleId = new AtomicReference<>(); + final AtomicReference childClientRoleId = new AtomicReference<>(); + + withRealm(realmId, (session, realm) -> { + // Create realm role + RoleModel parentRealmRole = session.roles().addRealmRole(realm, "parentRealmRole"); + parentRealmRoleId.set(parentRealmRole.getId()); + + // Create client role + ClientModel client = session.clients().addClient(realm,"clientWithRole"); + + RoleModel parentClientRole = session.roles().addClientRole(client, "parentClientRole"); + parentClientRoleId.set(parentClientRole.getId()); + + // Create realm child role + RoleModel childRealmRole = session.roles().addRealmRole(realm, "childRealmRole"); + childRealmRoleId.set(childRealmRole.getId()); + + RoleModel childClientRole = session.roles().addClientRole(client, "childClientRole"); + childClientRoleId.set(childClientRole.getId()); + + // Add composites + parentRealmRole.addCompositeRole(childRealmRole); + parentRealmRole.addCompositeRole(childClientRole); + + parentClientRole.addCompositeRole(childRealmRole); + parentClientRole.addCompositeRole(childClientRole); + return null; + }); + + withRealm(realmId, (session, realm) -> { + RoleModel parentRealmRole = session.roles().getRoleById(realm, parentRealmRoleId.get()); + RoleModel parentClientRole = session.roles().getRoleById(realm, parentClientRoleId.get()); + assertThat(parentRealmRole.getCompositesStream().collect(Collectors.toSet()), hasSize(2)); + assertThat(parentClientRole.getCompositesStream().collect(Collectors.toSet()), hasSize(2)); + + session.roles().removeRole(session.roles().getRoleById(realm, childRealmRoleId.get())); + session.roles().removeRole(session.roles().getRoleById(realm, childClientRoleId.get())); + return null; + }); + + withRealm(realmId, (session, realm) -> { + RoleModel parentRealmRole = session.roles().getRoleById(realm, parentRealmRoleId.get()); + RoleModel parentClientRole = session.roles().getRoleById(realm, parentClientRoleId.get()); + assertThat(parentRealmRole.getCompositesStream().collect(Collectors.toSet()), empty()); + assertThat(parentClientRole.getCompositesStream().collect(Collectors.toSet()), empty()); + return null; + }); + } + public void testRolesWithIdsPaginationSearchQueries(GetResult resultProvider) { // test all parameters together List result = resultProvider.getResult("1", 4, 3);