From e487db349c7479959dbea0727dbaa4cb10e3523c Mon Sep 17 00:00:00 2001 From: mposolda Date: Mon, 23 Jan 2017 17:55:45 +0100 Subject: [PATCH] KEYCLOAK-4274 Fix recursive composite role mappings --- .../models/cache/infinispan/RoleAdapter.java | 2 +- .../org/keycloak/models/jpa/RoleAdapter.java | 2 +- .../mongo/keycloak/adapters/RoleAdapter.java | 2 +- .../models/utils/KeycloakModelUtils.java | 23 +++++++++++++------ .../composites/CompositeRoleTest.java | 20 +++++++++++++++- 5 files changed, 38 insertions(+), 11 deletions(-) diff --git a/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/RoleAdapter.java b/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/RoleAdapter.java index 41cb41df50..00e41f3081 100755 --- a/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/RoleAdapter.java +++ b/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/RoleAdapter.java @@ -176,7 +176,7 @@ public class RoleAdapter implements RoleModel { @Override public boolean hasRole(RoleModel role) { - return this.equals(role) || KeycloakModelUtils.searchFor(role, this); + return this.equals(role) || KeycloakModelUtils.searchFor(role, this, new HashSet<>()); } @Override diff --git a/model/jpa/src/main/java/org/keycloak/models/jpa/RoleAdapter.java b/model/jpa/src/main/java/org/keycloak/models/jpa/RoleAdapter.java index 10e625291a..39e24c3bf1 100755 --- a/model/jpa/src/main/java/org/keycloak/models/jpa/RoleAdapter.java +++ b/model/jpa/src/main/java/org/keycloak/models/jpa/RoleAdapter.java @@ -128,7 +128,7 @@ public class RoleAdapter implements RoleModel, JpaModel { @Override public boolean hasRole(RoleModel role) { - return this.equals(role) || KeycloakModelUtils.searchFor(role, this); + return this.equals(role) || KeycloakModelUtils.searchFor(role, this, new HashSet<>()); } @Override diff --git a/model/mongo/src/main/java/org/keycloak/models/mongo/keycloak/adapters/RoleAdapter.java b/model/mongo/src/main/java/org/keycloak/models/mongo/keycloak/adapters/RoleAdapter.java index e141ff8e17..cf3c78fd00 100755 --- a/model/mongo/src/main/java/org/keycloak/models/mongo/keycloak/adapters/RoleAdapter.java +++ b/model/mongo/src/main/java/org/keycloak/models/mongo/keycloak/adapters/RoleAdapter.java @@ -172,7 +172,7 @@ public class RoleAdapter extends AbstractMongoAdapter implement @Override public boolean hasRole(RoleModel role) { - return this.equals(role) || KeycloakModelUtils.searchFor(role, this); + return this.equals(role) || KeycloakModelUtils.searchFor(role, this, new HashSet<>()); } public MongoRoleEntity getRole() { diff --git a/server-spi-private/src/main/java/org/keycloak/models/utils/KeycloakModelUtils.java b/server-spi-private/src/main/java/org/keycloak/models/utils/KeycloakModelUtils.java index a258cd77a6..695fe38c1b 100755 --- a/server-spi-private/src/main/java/org/keycloak/models/utils/KeycloakModelUtils.java +++ b/server-spi-private/src/main/java/org/keycloak/models/utils/KeycloakModelUtils.java @@ -177,14 +177,23 @@ public final class KeycloakModelUtils { * @param visited set of already visited roles (used for recursion) * @return true if "role" is descendant of "composite" */ - public static boolean searchFor(RoleModel role, RoleModel composite) { - return composite.isComposite() && ( - composite.getComposites().contains(role) || - composite.getComposites().stream() - .filter(x -> x.isComposite() && x.hasRole(role)) + public static boolean searchFor(RoleModel role, RoleModel composite, Set visited) { + if (visited.contains(composite.getId())) { + return false; + } + + visited.add(composite.getId()); + + if (!composite.isComposite()) { + return false; + } + + Set compositeRoles = composite.getComposites(); + return compositeRoles.contains(role) || + compositeRoles.stream() + .filter(x -> x.isComposite() && searchFor(role, x, visited)) .findFirst() - .isPresent() - ); + .isPresent(); } /** diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/composites/CompositeRoleTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/composites/CompositeRoleTest.java index e94edce040..4cf3904851 100755 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/composites/CompositeRoleTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/composites/CompositeRoleTest.java @@ -17,7 +17,6 @@ package org.keycloak.testsuite.composites; import org.jboss.arquillian.graphene.page.Page; -import org.junit.Assert; import org.junit.Before; import org.junit.Test; import org.keycloak.OAuth2Constants; @@ -28,6 +27,7 @@ import org.keycloak.common.enums.SslRequired; import org.keycloak.representations.AccessToken; import org.keycloak.representations.idm.RealmRepresentation; import org.keycloak.representations.idm.RoleRepresentation; +import org.keycloak.testsuite.Assert; import org.keycloak.testsuite.admin.ApiUtil; import org.keycloak.testsuite.pages.LoginPage; import org.keycloak.testsuite.util.ClientBuilder; @@ -345,4 +345,22 @@ public class CompositeRoleTest extends AbstractCompositeKeycloakTest { Assert.assertEquals(200, refreshResponse.getStatusCode()); } + + // KEYCLOAK-4274 + @Test + public void testRecursiveComposites() throws Exception { + // This will create recursive composite mappings between "REALM_COMPOSITE_1" and "REALM_ROLE_1" + RoleRepresentation realmComposite1 = testRealm().roles().get("REALM_COMPOSITE_1").toRepresentation(); + testRealm().roles().get("REALM_ROLE_1").addComposites(Collections.singletonList(realmComposite1)); + + UserResource userResource = ApiUtil.findUserByUsernameId(testRealm(), "REALM_COMPOSITE_1_USER"); + List realmRoles = userResource.roles().realmLevel().listEffective(); + Assert.assertNames(realmRoles, "REALM_COMPOSITE_1", "REALM_ROLE_1"); + + userResource = ApiUtil.findUserByUsernameId(testRealm(), "REALM_ROLE_1_USER"); + realmRoles = userResource.roles().realmLevel().listEffective(); + Assert.assertNames(realmRoles, "REALM_COMPOSITE_1", "REALM_ROLE_1"); + + } + }