From d7bb59461df66d8180d50ce6f53d90b25992afbb Mon Sep 17 00:00:00 2001 From: rmartinc Date: Thu, 5 Oct 2023 12:19:04 +0200 Subject: [PATCH] Escape $ sign when replacing clientId in the role mappers Closes https://github.com/keycloak/keycloak/issues/23692 --- .../oidc/mappers/AbstractUserRoleMappingMapper.java | 10 +++------- .../java/org/keycloak/testsuite/oidc/UserInfoTest.java | 9 +++++---- 2 files changed, 8 insertions(+), 11 deletions(-) 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 2f37151391..dbd61ed442 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 @@ -75,10 +75,7 @@ abstract class AbstractUserRoleMappingMapper extends AbstractOIDCProtocolMapper } - private static final Pattern CLIENT_ID_PATTERN = Pattern.compile("\\$\\{client_id\\}"); - - private static final Pattern DOT_PATTERN = Pattern.compile("\\."); - private static final String DOT_REPLACEMENT = "\\\\\\\\."; + private static final Pattern CLIENT_ID_PATTERN = Pattern.compile(Pattern.quote("${client_id}")); private static void mapClaim(IDToken token, ProtocolMapperModel mappingModel, Object attributeValue, String clientId) { attributeValue = OIDCAttributeMapperHelper.mapAttributeValue(mappingModel, attributeValue); @@ -90,11 +87,10 @@ abstract class AbstractUserRoleMappingMapper extends AbstractOIDCProtocolMapper } if (clientId != null) { - // case when clientId contains dots - clientId = DOT_PATTERN.matcher(clientId).replaceAll(DOT_REPLACEMENT); Matcher matcher = CLIENT_ID_PATTERN.matcher(protocolClaim); if (matcher.find()) { - protocolClaim = matcher.replaceAll(clientId); + // dots and backslashes in clientId should be escaped first for the claim + protocolClaim = matcher.replaceAll(Matcher.quoteReplacement(clientId.replace("\\", "\\\\").replace(".", "\\."))); } } diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oidc/UserInfoTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oidc/UserInfoTest.java index 0fc416d6a2..2ce9fd6a1a 100755 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oidc/UserInfoTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oidc/UserInfoTest.java @@ -235,8 +235,9 @@ public class UserInfoTest extends AbstractKeycloakTest { @Test public void testSuccess_dotsInClientId() throws Exception { // Create client with dot in the name + final String clientId = "my.foo.$\\client\\$"; ClientRepresentation clientRep = org.keycloak.testsuite.util.ClientBuilder.create() - .clientId("my.foo.client") + .clientId(clientId) .addRedirectUri("http://foo.host") .secret("password") .directAccessGrants() @@ -258,11 +259,11 @@ public class UserInfoTest extends AbstractKeycloakTest { userResource.roles().clientLevel(clientUUID).add(Collections.singletonList(fooRole)); // Login to the new client - OAuthClient.AccessTokenResponse accessTokenResponse = oauth.clientId("my.foo.client") + OAuthClient.AccessTokenResponse accessTokenResponse = oauth.clientId(clientId) .doGrantAccessTokenRequest("password", "test-user@localhost", "password"); AccessToken accessToken = oauth.verifyToken(accessTokenResponse.getAccessToken()); - Assert.assertNames(accessToken.getResourceAccess("my.foo.client").getRoles(), "my.foo.role"); + Assert.assertNames(accessToken.getResourceAccess(clientId).getRoles(), "my.foo.role"); events.clear(); @@ -271,7 +272,7 @@ public class UserInfoTest extends AbstractKeycloakTest { try { Response response = UserInfoClientUtil.executeUserInfoRequest_getMethod(client, accessTokenResponse.getAccessToken()); - testSuccessfulUserInfoResponse(response, "my.foo.client"); + testSuccessfulUserInfoResponse(response, clientId); } finally { client.close(); }