diff --git a/docs/documentation/upgrading/topics/keycloak/changes-23_0_0.adoc b/docs/documentation/upgrading/topics/keycloak/changes-23_0_0.adoc index 2627588ab8..28615dfa9d 100644 --- a/docs/documentation/upgrading/topics/keycloak/changes-23_0_0.adoc +++ b/docs/documentation/upgrading/topics/keycloak/changes-23_0_0.adoc @@ -35,3 +35,14 @@ If you are using a different encoding, convert the files to UTF-8. The Oracle Database JDBC driver is no longer part of the Keycloak distribution. Administrators will need to install a version of the Oracle Driver matching their environment as described in the configuring the database {section}. + += Changes to the value format of claims mapped by the realm and client role mappers + +Before this release, both realm (`User Realm Role`) and client (`User Client Role`) protocol mappers +were mapping a stringfied JSON array when the `Multivalued` setting was disabled. + +However, the `Multivalued` setting indicates whether the claim should be mapped as a list or, if disabled, only a single value +from the same list of values. + +In this release, the role and client mappers now map to a single value from the effective roles of a user when +they are marked as single-valued (`Multivalued` disabled). diff --git a/services/src/main/java/org/keycloak/protocol/oidc/mappers/AbstractUserRoleMappingMapper.java b/services/src/main/java/org/keycloak/protocol/oidc/mappers/AbstractUserRoleMappingMapper.java index b59dbb631f..36c23fa597 100755 --- a/services/src/main/java/org/keycloak/protocol/oidc/mappers/AbstractUserRoleMappingMapper.java +++ b/services/src/main/java/org/keycloak/protocol/oidc/mappers/AbstractUserRoleMappingMapper.java @@ -29,6 +29,7 @@ import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.Set; +import java.util.regex.Matcher; import java.util.regex.Pattern; import java.util.stream.Collectors; @@ -70,15 +71,7 @@ abstract class AbstractUserRoleMappingMapper extends AbstractOIDCProtocolMapper realmRoleNames = rolesToAdd; } - Object claimValue = realmRoleNames; - - boolean multiValued = "true".equals(mappingModel.getConfig().get(ProtocolMapperUtils.MULTIVALUED)); - if (!multiValued) { - claimValue = realmRoleNames.toString(); - } - - //OIDCAttributeMapperHelper.mapClaim(token, mappingModel, claimValue); - mapClaim(token, mappingModel, claimValue, clientId); + mapClaim(token, mappingModel, realmRoleNames, clientId); } @@ -99,7 +92,16 @@ abstract class AbstractUserRoleMappingMapper extends AbstractOIDCProtocolMapper if (clientId != null) { // case when clientId contains dots clientId = DOT_PATTERN.matcher(clientId).replaceAll(DOT_REPLACEMENT); - protocolClaim = CLIENT_ID_PATTERN.matcher(protocolClaim).replaceAll(clientId); + Matcher matcher = CLIENT_ID_PATTERN.matcher(protocolClaim); + if (matcher.find()) { + protocolClaim = matcher.replaceAll(clientId); + } + if (!(protocolClaim.endsWith("roles") || protocolClaim.startsWith(clientId) || protocolClaim.endsWith(clientId))) { + // the claim name does not reference the current client, do not map roles + // or if the claim does not end with roles suffix, do not map roles. + // the role suffix is used to move roles to a single location other than the default location (e.g.: realm_access and resource_access claims) + return; + } } List split = splitClaimPath(protocolClaim); diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/OIDCProtocolMappersTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/OIDCProtocolMappersTest.java index 9f19ceac14..67886b92f4 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/OIDCProtocolMappersTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/OIDCProtocolMappersTest.java @@ -79,6 +79,7 @@ import static org.hamcrest.Matchers.arrayContainingInAnyOrder; import static org.hamcrest.Matchers.containsInAnyOrder; import static org.hamcrest.Matchers.hasItem; import static org.hamcrest.Matchers.hasItems; +import static org.hamcrest.Matchers.in; import static org.hamcrest.Matchers.instanceOf; import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.isEmptyOrNullString; @@ -581,8 +582,8 @@ public class OIDCProtocolMappersTest extends AbstractKeycloakTest { // Verify attribute is filled Map roleMappings = (Map)idToken.getOtherClaims().get("roles-custom"); assertThat(roleMappings.keySet(), containsInAnyOrder("realm", "test-app")); - String realmRoleMappings = (String) roleMappings.get("realm"); - String testAppMappings = (String) roleMappings.get("test-app"); + List realmRoleMappings = (List) roleMappings.get("realm"); + List testAppMappings = (List) roleMappings.get("test-app"); assertRolesString(realmRoleMappings, "pref.user", // from direct assignment in user definition "pref.offline_access" // from direct assignment in user definition @@ -808,7 +809,7 @@ public class OIDCProtocolMappersTest extends AbstractKeycloakTest { // Verify attribute is filled AND it is filled only once Map roleMappings = (Map)idToken.getOtherClaims().get("roles-custom"); assertThat(roleMappings.keySet(), containsInAnyOrder(clientId)); - String testAppScopeMappings = (String) roleMappings.get(clientId); + List testAppScopeMappings = (List) roleMappings.get(clientId); assertRolesString(testAppScopeMappings, "customer-user" // from assignment to level2group or level2group2. It is filled just once ); @@ -837,8 +838,8 @@ public class OIDCProtocolMappersTest extends AbstractKeycloakTest { // Verify attribute is filled Map roleMappings = (Map)idToken.getOtherClaims().get("roles-custom"); assertThat(roleMappings.keySet(), containsInAnyOrder("realm", clientId)); - String realmRoleMappings = (String) roleMappings.get("realm"); - String testAppMappings = (String) roleMappings.get(clientId); + List realmRoleMappings = (List) roleMappings.get("realm"); + List testAppMappings = (List) roleMappings.get(clientId); assertRolesString(realmRoleMappings, "pref.admin", // from direct assignment to /roleRichGroup/level2group "pref.user", // from parent group of /roleRichGroup/level2group, i.e. from /roleRichGroup @@ -882,8 +883,8 @@ public class OIDCProtocolMappersTest extends AbstractKeycloakTest { // Verify attribute is filled Map roleMappings = (Map)idToken.getOtherClaims().get("roles-custom"); assertThat(roleMappings.keySet(), containsInAnyOrder("realm")); - String realmRoleMappings = (String) roleMappings.get("realm"); - String testAppAuthzMappings = (String) roleMappings.get(clientId); + List realmRoleMappings = (List) roleMappings.get("realm"); + List testAppAuthzMappings = (List) roleMappings.get(clientId); assertRolesString(realmRoleMappings, "pref.admin", // from direct assignment to /roleRichGroup/level2group "pref.user", // from parent group of /roleRichGroup/level2group, i.e. from /roleRichGroup @@ -915,8 +916,8 @@ public class OIDCProtocolMappersTest extends AbstractKeycloakTest { // Verify attribute is filled Map roleMappings = (Map)idToken.getOtherClaims().get("roles-custom"); assertThat(roleMappings.keySet(), containsInAnyOrder("realm", clientId)); - String realmRoleMappings = (String) roleMappings.get("realm"); - String testAppScopeMappings = (String) roleMappings.get(clientId); + List realmRoleMappings = (List) roleMappings.get("realm"); + List testAppScopeMappings = (List) roleMappings.get(clientId); assertRolesString(realmRoleMappings, "pref.admin", // from direct assignment to /roleRichGroup/level2group "pref.user", // from parent group of /roleRichGroup/level2group, i.e. from /roleRichGroup @@ -949,8 +950,8 @@ public class OIDCProtocolMappersTest extends AbstractKeycloakTest { // Verify attribute is filled Map roleMappings = (Map)idToken.getOtherClaims().get("roles-custom"); assertThat(roleMappings.keySet(), containsInAnyOrder("realm", clientId)); - String realmRoleMappings = (String) roleMappings.get("realm"); - String testAppScopeMappings = (String) roleMappings.get(clientId); + List realmRoleMappings = (List) roleMappings.get("realm"); + List testAppScopeMappings = (List) roleMappings.get(clientId); assertRolesString(realmRoleMappings, "pref.admin", // from direct assignment to /roleRichGroup/level2group "pref.user", // from parent group of /roleRichGroup/level2group, i.e. from /roleRichGroup @@ -965,6 +966,40 @@ public class OIDCProtocolMappersTest extends AbstractKeycloakTest { deleteMappers(protocolMappers); } + @Test + public void testSingleValuedRoleMapping() throws Exception { + String clientId = "test-app-scope"; + ProtocolMapperRepresentation realmMapper = ProtocolMapperUtil.createUserRealmRoleMappingMapper("pref.", "Realm roles mapper", "roles-custom.realm", true, true, false); + ProtocolMapperRepresentation clientMapper = ProtocolMapperUtil.createUserClientRoleMappingMapper(null, null, "Client roles mapper", "roles-custom.test-app-scope", true, true, false); + + ProtocolMappersResource protocolMappers = ApiUtil.findClientResourceByClientId(adminClient.realm("test"), clientId).getProtocolMappers(); + protocolMappers.createMapper(Arrays.asList(realmMapper, clientMapper)); + + // Login user + ClientManager.realm(adminClient.realm("test")).clientId(clientId).directAccessGrant(true); + oauth.clientId(clientId); + OAuthClient.AccessTokenResponse response = browserLogin("password", "rich.roles@redhat.com", "password"); + IDToken idToken = oauth.verifyIDToken(response.getIdToken()); + + // Verify attribute is filled + Map roleMappings = (Map)idToken.getOtherClaims().get("roles-custom"); + assertThat(roleMappings.keySet(), containsInAnyOrder("realm", clientId)); + String realmRoleMappings = (String) roleMappings.get("realm"); + String testAppScopeMappings = (String) roleMappings.get(clientId); + assertSingleValuedRolesString(realmRoleMappings, + "pref.admin", // from direct assignment to /roleRichGroup/level2group + "pref.user", // from parent group of /roleRichGroup/level2group, i.e. from /roleRichGroup + "pref.customer-user-premium" + ); + assertSingleValuedRolesString(testAppScopeMappings, + "test-app-allowed-by-scope", // from direct assignment to roleRichUser, present as scope allows it + "test-app-disallowed-by-scope" // from direct assignment to /roleRichGroup/level2group, present as scope allows it + ); + + // Revert + deleteMappers(protocolMappers); + } + @Test public void testUserGroupRoleToAttributeMappersScopedWithDifferentClient() throws Exception { final String clientId = "test-app-scope"; @@ -988,8 +1023,8 @@ public class OIDCProtocolMappersTest extends AbstractKeycloakTest { Map roleMappings = (Map) idToken.getOtherClaims().get("roles-custom"); assertNotNull(roleMappings); assertThat(roleMappings.keySet(), containsInAnyOrder("realm", diffClient)); - String realmRoleMappings = (String) roleMappings.get("realm"); - String testAppScopeMappings = (String) roleMappings.get(diffClient); + List realmRoleMappings = (List) roleMappings.get("realm"); + List testAppScopeMappings = (List) roleMappings.get(diffClient); assertRolesString(realmRoleMappings, "pref.admin", "pref.user", @@ -1769,26 +1804,12 @@ public class OIDCProtocolMappersTest extends AbstractKeycloakTest { Assert.assertNames(actualRoleList, expectedRoles); } - private void assertRolesString(String actualRoleString, String...expectedRoles) { - - assertThat(actualRoleString.matches("^\\[.*\\]$"), is(true)); - String[] roles = actualRoleString.substring(1, actualRoleString.length() - 1).split(",\\s*"); - - if (expectedRoles == null || expectedRoles.length == 0) { - assertThat(roles, arrayContainingInAnyOrder("")); - } else { - assertThat(roles, arrayContainingInAnyOrder(expectedRoles)); - } + private void assertRolesString(List actualRoleString, String...expectedRoles) { + assertThat(actualRoleString, containsInAnyOrder(expectedRoles)); } - - private ProtocolMapperRepresentation makeMapper(String name, String mapperType, Map config) { - ProtocolMapperRepresentation rep = new ProtocolMapperRepresentation(); - rep.setProtocol(OIDCLoginProtocol.LOGIN_PROTOCOL); - rep.setName(name); - rep.setProtocolMapper(mapperType); - rep.setConfig(config); - return rep; + private void assertSingleValuedRolesString(String actualRoleString, String... expectedRoles) { + assertThat(actualRoleString, is(in(expectedRoles))); } private OAuthClient.AccessTokenResponse browserLogin(String clientSecret, String username, String password) { diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/util/ProtocolMapperUtil.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/util/ProtocolMapperUtil.java index b9f0d6863c..e636ad86f2 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/util/ProtocolMapperUtil.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/util/ProtocolMapperUtil.java @@ -118,7 +118,7 @@ public class ProtocolMapperUtil { String tokenClaimName, boolean accessToken, boolean idToken) { - return createUserRealmRoleMappingMapper(realmRolePrefix, name, tokenClaimName, accessToken, idToken, false); + return createUserRealmRoleMappingMapper(realmRolePrefix, name, tokenClaimName, accessToken, idToken, true); } public static ProtocolMapperRepresentation createUserRealmRoleMappingMapper(String realmRolePrefix, @@ -134,7 +134,7 @@ public class ProtocolMapperUtil { String tokenClaimName, boolean accessToken, boolean idToken) { - return createUserClientRoleMappingMapper(clientId, clientRolePrefix, name, tokenClaimName, accessToken, idToken, false); + return createUserClientRoleMappingMapper(clientId, clientRolePrefix, name, tokenClaimName, accessToken, idToken, true); } public static ProtocolMapperRepresentation createUserClientRoleMappingMapper(String clientId, String clientRolePrefix,