Update composite roles on child role removal

Closes #11769
keycloak · May 5, 2022 · fc974fc · fc974fc
1 parent 491b326
commit fc974fc
Show file tree

Hide file tree

Showing 9 changed files with 92 additions and 2 deletions.
diff --git 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/...p-hot-rod/src/main/java/org/keycloak/models/map/storage/hotRod/role/HotRodRoleEntity.java b/...p-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<String> compositeRoles;
 

diff --git a/...a/src/main/java/org/keycloak/models/map/storage/jpa/role/JpaRoleModelCriteriaBuilder.java b/...a/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<? super RoleMode
                     return new JpaRoleModelCriteriaBuilder((cb, root) -> 
                         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/...src/main/java/org/keycloak/models/map/storage/ldap/role/LdapRoleModelCriteriaBuilder.java b/...src/main/java/org/keycloak/models/map/storage/ldap/role/LdapRoleModelCriteriaBuilder.java
@@ -140,6 +140,9 @@ public LdapRoleModelCriteriaBuilder compare(SearchableModelField<? super RoleMod
                     String field = modelFieldNameToLdap(roleMapperConfig, modelField);
                     return new LdapRoleModelCriteriaBuilder(roleMapperConfig, 
                             () -> 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
@@ -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<RoleModel> 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
@@ -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<MapRoleProvider, MapRoleEntity, RoleModel> implements RoleProviderFactory<MapRoleProvider>, 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
@@ -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);
@@ -332,6 +333,14 @@ private static MapModelCriteriaBuilder<Object, MapClientEntity, ClientModel> che
         return mcb.fieldCompare(Boolean.TRUE::equals, getter);
     }
 
+    private static MapModelCriteriaBuilder<Object, MapRoleEntity, RoleModel> checkCompositeRoles(MapModelCriteriaBuilder<Object, MapRoleEntity, RoleModel> mcb, Operator op, Object[] values) {
+        String roleIdS = ensureEqSingleValue(RoleModel.SearchableFields.COMPOSITE_ROLE, "composite_role_id", op, values);
+        Function<MapRoleEntity, ?> getter;
+        getter = re -> Optional.ofNullable(re.getCompositeRoles()).orElseGet(Collections::emptySet).contains(roleIdS);
+
+        return mcb.fieldCompare(Boolean.TRUE::equals, getter);
+    }
+
     private static MapModelCriteriaBuilder<Object, MapUserEntity, UserModel> checkGrantedUserRole(MapModelCriteriaBuilder<Object, MapUserEntity, UserModel> mcb, Operator op, Object[] values) {
         String roleIdS = ensureEqSingleValue(UserModel.SearchableFields.ASSIGNED_ROLE, "role_id", op, values);
         Function<MapUserEntity, ?> 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
@@ -38,6 +38,7 @@ public static class SearchableFields {
         public static final SearchableModelField<RoleModel> NAME                = new SearchableModelField<>("name", String.class);
         public static final SearchableModelField<RoleModel> DESCRIPTION         = new SearchableModelField<>("description", String.class);
         public static final SearchableModelField<RoleModel> IS_CLIENT_ROLE      = new SearchableModelField<>("isClientRole", Boolean.class);
+        public static final SearchableModelField<RoleModel> 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
@@ -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<String> parentRealmRoleId = new AtomicReference<>();
+        final AtomicReference<String> parentClientRoleId = new AtomicReference<>();
+
+        final AtomicReference<String> childRealmRoleId = new AtomicReference<>();
+        final AtomicReference<String> 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<RoleModel> result = resultProvider.getResult("1", 4, 3);