From 60942346f31a505ec0db748b95dd1b0f4fb1820c Mon Sep 17 00:00:00 2001 From: Martin Hardselius Date: Wed, 14 Jun 2017 10:47:40 +0200 Subject: [PATCH] KEYCLOAK-4924: pairwise clients get duplicate subs in tokens --- .../mappers/AbstractPairwiseSubMapper.java | 26 ++++++++++++------- .../oidc/mappers/SHA256PairwiseSubMapper.java | 4 +-- .../OIDCPairwiseClientRegistrationTest.java | 20 ++++++++++++-- 3 files changed, 36 insertions(+), 14 deletions(-) diff --git a/services/src/main/java/org/keycloak/protocol/oidc/mappers/AbstractPairwiseSubMapper.java b/services/src/main/java/org/keycloak/protocol/oidc/mappers/AbstractPairwiseSubMapper.java index 09b39c4ae2..374581d83a 100644 --- a/services/src/main/java/org/keycloak/protocol/oidc/mappers/AbstractPairwiseSubMapper.java +++ b/services/src/main/java/org/keycloak/protocol/oidc/mappers/AbstractPairwiseSubMapper.java @@ -64,24 +64,32 @@ public abstract class AbstractPairwiseSubMapper extends AbstractOIDCProtocolMapp } @Override - public final IDToken transformIDToken(IDToken token, ProtocolMapperModel mappingModel, KeycloakSession session, UserSessionModel userSession, AuthenticatedClientSessionModel clientSession) { - setSubject(token, generateSub(mappingModel, getSectorIdentifier(clientSession.getClient(), mappingModel), userSession.getUser().getId())); + public IDToken transformIDToken(IDToken token, ProtocolMapperModel mappingModel, KeycloakSession session, UserSessionModel userSession, AuthenticatedClientSessionModel clientSession) { + setIDTokenSubject(token, generateSub(mappingModel, getSectorIdentifier(clientSession.getClient(), mappingModel), userSession.getUser().getId())); return token; } @Override - public final AccessToken transformAccessToken(AccessToken token, ProtocolMapperModel mappingModel, KeycloakSession session, UserSessionModel userSession, AuthenticatedClientSessionModel clientSession) { - setSubject(token, generateSub(mappingModel, getSectorIdentifier(clientSession.getClient(), mappingModel), userSession.getUser().getId())); + public AccessToken transformAccessToken(AccessToken token, ProtocolMapperModel mappingModel, KeycloakSession session, UserSessionModel userSession, AuthenticatedClientSessionModel clientSession) { + setAccessTokenSubject(token, generateSub(mappingModel, getSectorIdentifier(clientSession.getClient(), mappingModel), userSession.getUser().getId())); return token; } @Override - public final AccessToken transformUserInfoToken(AccessToken token, ProtocolMapperModel mappingModel, KeycloakSession session, UserSessionModel userSession, AuthenticatedClientSessionModel clientSession) { - setSubject(token, generateSub(mappingModel, getSectorIdentifier(clientSession.getClient(), mappingModel), userSession.getUser().getId())); + public AccessToken transformUserInfoToken(AccessToken token, ProtocolMapperModel mappingModel, KeycloakSession session, UserSessionModel userSession, AuthenticatedClientSessionModel clientSession) { + setUserInfoTokenSubject(token, generateSub(mappingModel, getSectorIdentifier(clientSession.getClient(), mappingModel), userSession.getUser().getId())); return token; } - private void setSubject(IDToken token, String pairwiseSub) { + protected void setIDTokenSubject(IDToken token, String pairwiseSub) { + token.setSubject(pairwiseSub); + } + + protected void setAccessTokenSubject(IDToken token, String pairwiseSub) { + token.setSubject(pairwiseSub); + } + + protected void setUserInfoTokenSubject(IDToken token, String pairwiseSub) { token.getOtherClaims().put("sub", pairwiseSub); } @@ -115,6 +123,4 @@ public abstract class AbstractPairwiseSubMapper extends AbstractOIDCProtocolMapp public final String getId() { return "oidc-" + getIdPrefix() + PROVIDER_ID_SUFFIX; } -} - - +} \ No newline at end of file diff --git a/services/src/main/java/org/keycloak/protocol/oidc/mappers/SHA256PairwiseSubMapper.java b/services/src/main/java/org/keycloak/protocol/oidc/mappers/SHA256PairwiseSubMapper.java index 28a0b87284..f69f2b12f2 100644 --- a/services/src/main/java/org/keycloak/protocol/oidc/mappers/SHA256PairwiseSubMapper.java +++ b/services/src/main/java/org/keycloak/protocol/oidc/mappers/SHA256PairwiseSubMapper.java @@ -79,7 +79,7 @@ public class SHA256PairwiseSubMapper extends AbstractPairwiseSubMapper { Charset charset = Charset.forName("UTF-8"); byte[] salt = saltStr.getBytes(charset); String pairwiseSub = generateSub(sectorIdentifier, localSub, salt); - logger.infof("local sub = '%s', pairwise sub = '%s'", localSub, pairwiseSub); + logger.tracef("local sub = '%s', pairwise sub = '%s'", localSub, pairwiseSub); return pairwiseSub; } @@ -109,4 +109,4 @@ public class SHA256PairwiseSubMapper extends AbstractPairwiseSubMapper { public String getIdPrefix() { return PROVIDER_ID; } -} +} \ No newline at end of file diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/OIDCPairwiseClientRegistrationTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/OIDCPairwiseClientRegistrationTest.java index c5bb78577b..8666e04aec 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/OIDCPairwiseClientRegistrationTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/OIDCPairwiseClientRegistrationTest.java @@ -18,6 +18,7 @@ package org.keycloak.testsuite.client; +import org.apache.commons.lang.StringUtils; import org.junit.Before; import org.junit.Test; import org.keycloak.admin.client.resource.ClientResource; @@ -44,6 +45,7 @@ import org.keycloak.testsuite.util.UserInfoClientUtil; import javax.ws.rs.client.Client; import javax.ws.rs.core.Response; import java.util.ArrayList; +import java.util.Base64; import java.util.Collections; import java.util.List; @@ -319,11 +321,20 @@ public class OIDCPairwiseClientRegistrationTest extends AbstractClientRegistrati oauth.openLoginForm(); loginResponse = new OAuthClient.AuthorizationEndpointResponse(oauth); accessTokenResponse = oauth.doAccessTokenRequest(loginResponse.getCode(), pairwiseClient.getClientSecret()); + + // Assert token payloads don't contain more than one "sub" + String accessTokenPayload = getPayload(accessTokenResponse.getAccessToken()); + Assert.assertEquals(1, StringUtils.countMatches(accessTokenPayload, "\"sub\"")); + String idTokenPayload = getPayload(accessTokenResponse.getIdToken()); + Assert.assertEquals(1, StringUtils.countMatches(idTokenPayload, "\"sub\"")); + String refreshTokenPayload = getPayload(accessTokenResponse.getRefreshToken()); + Assert.assertEquals(1, StringUtils.countMatches(refreshTokenPayload, "\"sub\"")); + accessToken = oauth.verifyToken(accessTokenResponse.getAccessToken()); Assert.assertEquals("test-user", accessToken.getPreferredUsername()); Assert.assertEquals("test-user@localhost", accessToken.getEmail()); - // Assert pairwise client has different subject like userId + // Assert pairwise client has different subject than userId String pairwiseUserId = accessToken.getSubject(); Assert.assertNotEquals(pairwiseUserId, user.getId()); @@ -339,4 +350,9 @@ public class OIDCPairwiseClientRegistrationTest extends AbstractClientRegistrati jaxrsClient.close(); } } -} + + private String getPayload(String token) { + String payloadBase64 = token.split("\\.")[1]; + return new String(Base64.getDecoder().decode(payloadBase64)); + } +} \ No newline at end of file