From a4c4c00d00a747d93bb17762117caab606a3ec17 Mon Sep 17 00:00:00 2001 From: rmartinc Date: Mon, 8 Nov 2021 10:04:04 +0100 Subject: [PATCH] [KEYCLOAK-14309] Duplicate sub claim at JSON level --- .../keycloak/protocol/oidc/TokenManager.java | 68 +++++++++- .../mappers/OIDCAttributeMapperHelper.java | 122 ++++++++++++++++-- .../oauth/OIDCProtocolMappersTest.java | 75 ++++++++++- 3 files changed, 249 insertions(+), 16 deletions(-) diff --git a/services/src/main/java/org/keycloak/protocol/oidc/TokenManager.java b/services/src/main/java/org/keycloak/protocol/oidc/TokenManager.java index 8b01dae6015..c8085c8fc80 100755 --- a/services/src/main/java/org/keycloak/protocol/oidc/TokenManager.java +++ b/services/src/main/java/org/keycloak/protocol/oidc/TokenManager.java @@ -717,7 +717,73 @@ public class TokenManager { public Map generateUserInfoClaims(AccessToken userInfo, UserModel userModel) { Map claims = new HashMap<>(); - claims.put("sub", userModel.getId()); + claims.put("sub", userInfo.getSubject() == null? userModel.getId() : userInfo.getSubject()); + if (userInfo.getIssuer() != null) { + claims.put("iss", userInfo.getIssuer()); + } + if (userInfo.getAudience()!= null) { + claims.put("aud", userInfo.getAudience()); + } + if (userInfo.getName() != null) { + claims.put("name", userInfo.getName()); + } + if (userInfo.getGivenName() != null) { + claims.put("given_name", userInfo.getGivenName()); + } + if (userInfo.getFamilyName() != null) { + claims.put("family_name", userInfo.getFamilyName()); + } + if (userInfo.getMiddleName() != null) { + claims.put("middle_name", userInfo.getMiddleName()); + } + if (userInfo.getNickName() != null) { + claims.put("nickname", userInfo.getNickName()); + } + if (userInfo.getPreferredUsername() != null) { + claims.put("preferred_username", userInfo.getPreferredUsername()); + } + if (userInfo.getProfile() != null) { + claims.put("profile", userInfo.getProfile()); + } + if (userInfo.getPicture() != null) { + claims.put("picture", userInfo.getPicture()); + } + if (userInfo.getWebsite() != null) { + claims.put("website", userInfo.getWebsite()); + } + if (userInfo.getEmail() != null) { + claims.put("email", userInfo.getEmail()); + } + if (userInfo.getEmailVerified() != null) { + claims.put("email_verified", userInfo.getEmailVerified()); + } + if (userInfo.getGender() != null) { + claims.put("gender", userInfo.getGender()); + } + if (userInfo.getBirthdate() != null) { + claims.put("birthdate", userInfo.getBirthdate()); + } + if (userInfo.getZoneinfo() != null) { + claims.put("zoneinfo", userInfo.getZoneinfo()); + } + if (userInfo.getLocale() != null) { + claims.put("locale", userInfo.getLocale()); + } + if (userInfo.getPhoneNumber() != null) { + claims.put("phone_number", userInfo.getPhoneNumber()); + } + if (userInfo.getPhoneNumberVerified() != null) { + claims.put("phone_number_verified", userInfo.getPhoneNumberVerified()); + } + if (userInfo.getAddress() != null) { + claims.put("address", userInfo.getAddress()); + } + if (userInfo.getUpdatedAt() != null) { + claims.put("updated_at", userInfo.getUpdatedAt()); + } + if (userInfo.getClaimsLocales() != null) { + claims.put("claims_locales", userInfo.getClaimsLocales()); + } claims.putAll(userInfo.getOtherClaims()); if (userInfo.getRealmAccess() != null) { diff --git a/services/src/main/java/org/keycloak/protocol/oidc/mappers/OIDCAttributeMapperHelper.java b/services/src/main/java/org/keycloak/protocol/oidc/mappers/OIDCAttributeMapperHelper.java index 0813f8171f9..ab1c271f1ea 100755 --- a/services/src/main/java/org/keycloak/protocol/oidc/mappers/OIDCAttributeMapperHelper.java +++ b/services/src/main/java/org/keycloak/protocol/oidc/mappers/OIDCAttributeMapperHelper.java @@ -18,6 +18,7 @@ package org.keycloak.protocol.oidc.mappers; import com.fasterxml.jackson.databind.JsonNode; +import org.jboss.logging.Logger; import org.keycloak.models.ProtocolMapperModel; import org.keycloak.protocol.ProtocolMapper; import org.keycloak.protocol.ProtocolMapperUtils; @@ -59,11 +60,81 @@ public class OIDCAttributeMapperHelper { public static final String INCLUDE_IN_USERINFO_LABEL = "includeInUserInfo.label"; public static final String INCLUDE_IN_USERINFO_HELP_TEXT = "includeInUserInfo.tooltip"; + private static final Logger logger = Logger.getLogger(OIDCAttributeMapperHelper.class); + + /** + * Interface for a token property setter in a class T that accept claims. + * @param The token class for the property + */ + private static interface PropertySetter { + void set(String claim, String mapperName, T token, Object value); + } + + /** + * Setters for claims in IDToken/AccessToken that will not use the other claims map. + */ + private static final Map> tokenPropertySetters; + + /** + * Setters for claims in AccessTokenResponse that will not use the other claims map. + */ + private static final Map> responsePropertySetters; + + static { + // allowed claims that can be set in the IDToken/AccessToken object + Map> tmpToken = new HashMap<>(); + tmpToken.put("sub", (claim, mapperName, token, value) -> { + token.setSubject(value.toString()); + }); + tmpToken.put("azp", (claim, mapperName, token, value) -> { + token.issuedFor(value.toString()); + }); + tmpToken.put("aud", (claim, mapperName, token, value) -> { + if (value instanceof Collection) { + String[] audiences = ((Collection) value).stream().map(Object::toString).toArray(String[]::new); + token.audience(audiences); + } else { + token.audience(value.toString()); + } + }); + // not allowed claims that are set by the server and can generate duplicates + PropertySetter notAllowedInToken = (claim, mapperName, token, value) -> { + logger.warnf("Claim '%s' is non-modifiable in IDToken. Ignoring the assignment for mapper '%s'.", claim, mapperName); + }; + tmpToken.put("jti", notAllowedInToken); + tmpToken.put("typ", notAllowedInToken); + tmpToken.put("iat", notAllowedInToken); + tmpToken.put("exp", notAllowedInToken); + tmpToken.put("iss", notAllowedInToken); + tmpToken.put("scope", notAllowedInToken); + tmpToken.put(IDToken.NONCE, notAllowedInToken); + tmpToken.put(IDToken.ACR, notAllowedInToken); + tmpToken.put(IDToken.AUTH_TIME, notAllowedInToken); + tmpToken.put(IDToken.SESSION_STATE, notAllowedInToken); + tokenPropertySetters = Collections.unmodifiableMap(tmpToken); + + // in the AccessTokenResponse do not allow modifications for server assigned properties + Map> tmpResponse = new HashMap<>(); + PropertySetter notAllowedInResponse = (claim, mapperName, token, value) -> { + logger.warnf("Claim '%s' is non-modifiable in AccessTokenResponse. Ignoring the assignment for mapper '%s'.", claim, mapperName); + }; + tmpResponse.put("access_token", notAllowedInResponse); + tmpResponse.put("token_type", notAllowedInResponse); + tmpResponse.put("session_state", notAllowedInResponse); + tmpResponse.put("expires_in", notAllowedInResponse); + tmpResponse.put("id_token", notAllowedInResponse); + tmpResponse.put("refresh_token", notAllowedInResponse); + tmpResponse.put("refresh_expires_in", notAllowedInResponse); + tmpResponse.put("not-before-policy", notAllowedInResponse); + tmpResponse.put("scope", notAllowedInResponse); + responsePropertySetters = Collections.unmodifiableMap(tmpResponse); + } + public static Object mapAttributeValue(ProtocolMapperModel mappingModel, Object attributeValue) { if (attributeValue == null) return null; if (attributeValue instanceof Collection) { - Collection valueAsList = (Collection) attributeValue; + Collection valueAsList = (Collection) attributeValue; if (valueAsList.isEmpty()) return null; if (isMultivalued(mappingModel)) { @@ -100,34 +171,34 @@ public class OIDCAttributeMapperHelper { Boolean booleanObject = getBoolean(attributeValue); if (booleanObject != null) return booleanObject; if (attributeValue instanceof List) { - return transform((List) attributeValue, OIDCAttributeMapperHelper::getBoolean); + return transform((List) attributeValue, OIDCAttributeMapperHelper::getBoolean); } throw new RuntimeException("cannot map type for token claim"); case "String": if (attributeValue instanceof String) return attributeValue; if (attributeValue instanceof List) { - return transform((List) attributeValue, OIDCAttributeMapperHelper::getString); + return transform((List) attributeValue, OIDCAttributeMapperHelper::getString); } return attributeValue.toString(); case "long": Long longObject = getLong(attributeValue); if (longObject != null) return longObject; if (attributeValue instanceof List) { - return transform((List) attributeValue, OIDCAttributeMapperHelper::getLong); + return transform((List) attributeValue, OIDCAttributeMapperHelper::getLong); } throw new RuntimeException("cannot map type for token claim"); case "int": Integer intObject = getInteger(attributeValue); if (intObject != null) return intObject; if (attributeValue instanceof List) { - return transform((List) attributeValue, OIDCAttributeMapperHelper::getInteger); + return transform((List) attributeValue, OIDCAttributeMapperHelper::getInteger); } throw new RuntimeException("cannot map type for token claim"); case "JSON": JsonNode jsonNodeObject = getJsonNode(attributeValue); if (jsonNodeObject != null) return jsonNodeObject; if (attributeValue instanceof List) { - return transform((List) attributeValue, OIDCAttributeMapperHelper::getJsonNode); + return transform((List) attributeValue, OIDCAttributeMapperHelper::getJsonNode); } throw new RuntimeException("cannot map type for token claim"); default: @@ -200,22 +271,49 @@ public class OIDCAttributeMapperHelper { } public static void mapClaim(IDToken token, ProtocolMapperModel mappingModel, Object attributeValue) { - mapClaim(mappingModel, attributeValue, token.getOtherClaims()); + mapClaim(token, mappingModel, attributeValue, tokenPropertySetters, token.getOtherClaims()); } public static void mapClaim(AccessTokenResponse token, ProtocolMapperModel mappingModel, Object attributeValue) { - mapClaim(mappingModel, attributeValue, token.getOtherClaims()); + mapClaim(token, mappingModel, attributeValue, responsePropertySetters, token.getOtherClaims()); } - private static void mapClaim(ProtocolMapperModel mappingModel, Object attributeValue, Map jsonObject) { + private static void mapClaim(T token, ProtocolMapperModel mappingModel, Object attributeValue, + Map> setters, Map jsonObject) { attributeValue = mapAttributeValue(mappingModel, attributeValue); - if (attributeValue == null) return; + if (attributeValue == null) { + return; + } String protocolClaim = mappingModel.getConfig().get(TOKEN_CLAIM_NAME); if (protocolClaim == null) { return; } + List split = splitClaimPath(protocolClaim); + if (split.isEmpty()) { + return; + } + + String firstClaim = split.iterator().next(); + PropertySetter setter = setters.get(firstClaim); + if (setter != null) { + // assign using the property setters over the token + if (split.size() > 1) { + logger.warnf("Claim '%s' contains more than one level in a setter. Ignoring the assignment for mapper '%s'.", + protocolClaim, mappingModel.getName()); + return; + } + + setter.set(protocolClaim, mappingModel.getName(), token, attributeValue); + return; + } + + // map value to the other claims map + mapClaim(split, attributeValue, jsonObject); + } + + private static void mapClaim(List split, Object attributeValue, Map jsonObject) { final int length = split.size(); int i = 0; for (String component : split) { @@ -253,7 +351,7 @@ public class OIDCAttributeMapperHelper { mapper.setName(name); mapper.setProtocolMapper(mapperId); mapper.setProtocol(OIDCLoginProtocol.LOGIN_PROTOCOL); - Map config = new HashMap(); + Map config = new HashMap<>(); config.put(ProtocolMapperUtils.USER_ATTRIBUTE, userAttribute); config.put(TOKEN_CLAIM_NAME, tokenClaimName); config.put(JSON_TYPE, claimType); @@ -311,7 +409,7 @@ public class OIDCAttributeMapperHelper { ProviderConfigProperty property = new ProviderConfigProperty(); property.setName(JSON_TYPE); property.setLabel(JSON_TYPE); - List types = new ArrayList(5); + List types = new ArrayList<>(5); types.add("String"); types.add("long"); types.add("int"); 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 1f6ef01c9d5..0abec356ee3 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 @@ -37,6 +37,7 @@ import org.keycloak.protocol.oidc.mappers.OIDCAttributeMapperHelper; import org.keycloak.representations.AccessToken; import org.keycloak.representations.AddressClaimSet; import org.keycloak.representations.IDToken; +import org.keycloak.representations.UserInfo; import org.keycloak.representations.idm.ClientRepresentation; import org.keycloak.representations.idm.ClientScopeRepresentation; import org.keycloak.representations.idm.GroupRepresentation; @@ -48,13 +49,17 @@ import org.keycloak.testsuite.Assert; import org.keycloak.testsuite.AssertEvents; import org.keycloak.testsuite.admin.ApiUtil; import org.keycloak.testsuite.arquillian.annotation.AuthServerContainerExclude; +import org.keycloak.testsuite.arquillian.annotation.AuthServerContainerExclude.AuthServer; import org.keycloak.testsuite.arquillian.annotation.EnableFeature; import org.keycloak.testsuite.updaters.ClientAttributeUpdater; import org.keycloak.testsuite.updaters.ProtocolMappersUpdater; +import org.keycloak.testsuite.util.AdminClientUtil; import org.keycloak.testsuite.util.ClientManager; import org.keycloak.testsuite.util.OAuthClient; import org.keycloak.testsuite.util.ProtocolMapperUtil; +import org.keycloak.testsuite.util.UserInfoClientUtil; +import javax.ws.rs.client.Client; import javax.ws.rs.core.Response; import java.nio.charset.StandardCharsets; @@ -64,6 +69,7 @@ import java.util.HashMap; import java.util.List; import java.util.Map; +import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.anyOf; import static org.hamcrest.Matchers.arrayContainingInAnyOrder; import static org.hamcrest.Matchers.containsInAnyOrder; @@ -71,17 +77,15 @@ import static org.hamcrest.Matchers.hasItems; import static org.hamcrest.Matchers.instanceOf; import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.isEmptyOrNullString; -import static org.hamcrest.Matchers.notNullValue; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotEquals; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertNull; -import static org.junit.Assert.assertThat; import static org.junit.Assert.assertTrue; import static org.keycloak.testsuite.admin.AbstractAdminTest.loadJson; import static org.keycloak.testsuite.admin.ApiUtil.findClientByClientId; import static org.keycloak.testsuite.admin.ApiUtil.findClientResourceByClientId; import static org.keycloak.testsuite.admin.ApiUtil.findUserByUsernameId; -import org.keycloak.testsuite.arquillian.annotation.AuthServerContainerExclude.AuthServer; import static org.keycloak.testsuite.util.ProtocolMapperUtil.createAddressMapper; import static org.keycloak.testsuite.util.ProtocolMapperUtil.createClaimMapper; import static org.keycloak.testsuite.util.ProtocolMapperUtil.createHardcodedClaim; @@ -335,6 +339,71 @@ public class OIDCProtocolMappersTest extends AbstractKeycloakTest { events.clear(); } + @Test + @AuthServerContainerExclude(AuthServer.REMOTE) + public void testTokenPropertiesMapping() throws Exception { + UserResource userResource = findUserByUsernameId(adminClient.realm("test"), "test-user@localhost"); + UserRepresentation user = userResource.toRepresentation(); + user.singleAttribute("userid", "123456789"); + user.getAttributes().put("useraud", Arrays.asList("test-app", "other")); + userResource.update(user); + + // create a user attr mapping for some claims that exist as properties in the tokens + ClientResource app = findClientResourceByClientId(adminClient.realm("test"), "test-app"); + app.getProtocolMappers().createMapper(createClaimMapper("userid-as-sub", "userid", "sub", "String", true, true, false)).close(); + app.getProtocolMappers().createMapper(createClaimMapper("useraud", "useraud", "aud", "String", true, true, true)).close(); + app.getProtocolMappers().createMapper(createHardcodedClaim("website-hardcoded", "website", "http://localhost", "String", true, true)).close(); + app.getProtocolMappers().createMapper(createHardcodedClaim("iat-hardcoded", "iat", "123", "long", true, false)).close(); + + // login + OAuthClient.AccessTokenResponse response = browserLogin("password", "test-user@localhost", "password"); + + // assert mappers work as expected + IDToken idToken = oauth.verifyIDToken(response.getIdToken()); + assertEquals(user.firstAttribute("userid"), idToken.getSubject()); + assertEquals("http://localhost", idToken.getWebsite()); + assertNotNull(idToken.getAudience()); + assertThat(Arrays.asList(idToken.getAudience()), hasItems("test-app", "other")); + + AccessToken accessToken = oauth.verifyToken(response.getAccessToken()); + assertEquals(user.firstAttribute("userid"), accessToken.getSubject()); + assertEquals("http://localhost", accessToken.getWebsite()); + assertNotNull(accessToken.getAudience()); + assertThat(Arrays.asList(accessToken.getAudience()), hasItems("test-app", "other")); + assertNotEquals(123L, accessToken.getIat().longValue()); // iat should not be modified + + // assert that tokens are also OK in the UserInfo response (hardcoded mappers in IDToken are in UserInfo) + Client client = AdminClientUtil.createResteasyClient(); + try { + Response userInfoResponse = UserInfoClientUtil.executeUserInfoRequest_getMethod(client, response.getAccessToken()); + UserInfo userInfo = userInfoResponse.readEntity(UserInfo.class); + assertEquals(user.firstAttribute("userid"), userInfo.getSubject()); + assertEquals(user.getEmail(), userInfo.getEmail()); + assertEquals(user.getUsername(), userInfo.getPreferredUsername()); + assertEquals(user.getLastName(), userInfo.getFamilyName()); + assertEquals(user.getFirstName(), userInfo.getGivenName()); + assertEquals("http://localhost", userInfo.getWebsite()); + assertNotNull(accessToken.getAudience()); + assertThat(Arrays.asList(accessToken.getAudience()), hasItems("test-app", "other")); + } finally { + client.close(); + } + + // logout + oauth.openLogout(); + + // undo mappers + app = findClientByClientId(adminClient.realm("test"), "test-app"); + ClientRepresentation clientRepresentation = app.toRepresentation(); + for (ProtocolMapperRepresentation model : clientRepresentation.getProtocolMappers()) { + if (model.getName().equals("userid-as-sub") || model.getName().equals("website-hardcoded") + || model.getName().equals("iat-hardcoded") || model.getName().equals("useraud")) { + app.getProtocolMappers().delete(model.getId()); + } + } + events.clear(); + } + @Test @AuthServerContainerExclude(AuthServer.REMOTE) public void testNullOrEmptyTokenMapping() throws Exception {