From d7a0597b1df0b2aa227481485ba3613cc1902d1b Mon Sep 17 00:00:00 2001 From: Helge Olav Aarstein Date: Thu, 24 Oct 2019 21:41:38 +0200 Subject: [PATCH] KEYCLOAK-9091 Fix for claims with dots from userInfo (#6312) * KEYCLOAK-9091 Unable to map claim attributes with dots (.) in them when claims are retrieved from userInfo endpoint --- .../AbstractJsonUserAttributeMapper.java | 99 ++++++++++--------- .../AbstractJsonUserAttributeMapperTest.java | 37 +++---- 2 files changed, 71 insertions(+), 65 deletions(-) diff --git a/services/src/main/java/org/keycloak/broker/oidc/mappers/AbstractJsonUserAttributeMapper.java b/services/src/main/java/org/keycloak/broker/oidc/mappers/AbstractJsonUserAttributeMapper.java index bf3a2fc180..6827c21e08 100755 --- a/services/src/main/java/org/keycloak/broker/oidc/mappers/AbstractJsonUserAttributeMapper.java +++ b/services/src/main/java/org/keycloak/broker/oidc/mappers/AbstractJsonUserAttributeMapper.java @@ -26,6 +26,7 @@ import org.keycloak.models.IdentityProviderMapperModel; import org.keycloak.models.KeycloakSession; import org.keycloak.models.RealmModel; import org.keycloak.models.UserModel; +import org.keycloak.protocol.oidc.mappers.OIDCAttributeMapperHelper; import org.keycloak.provider.ProviderConfigProperty; import java.util.ArrayList; @@ -165,51 +166,51 @@ public abstract class AbstractJsonUserAttributeMapper extends AbstractIdentityPr return value; } + public static Object getJsonValue(JsonNode baseNode, String fieldPath) { logger.debug("Going to process JsonNode path " + fieldPath + " on data " + baseNode); if (baseNode != null) { - int idx = fieldPath.indexOf(JSON_PATH_DELIMITER); - - String currentFieldName = fieldPath; - if (idx > 0) { - currentFieldName = fieldPath.substring(0, idx).trim(); - if (currentFieldName.isEmpty()) { - logger.debug("JSON path is invalid " + fieldPath); - return null; - } - } - - String currentNodeName = currentFieldName; - int arrayIndex = -1; - if (currentFieldName.endsWith("]")) { - int bi = currentFieldName.indexOf("["); - if (bi == -1) { - logger.debug("Invalid array index construct in " + currentFieldName); - return null; - } - try { - String is = currentFieldName.substring(bi+1, currentFieldName.length() - 1).trim(); - arrayIndex = Integer.parseInt(is); - } catch (Exception e) { - logger.debug("Invalid array index construct in " + currentFieldName); - return null; - } - currentNodeName = currentFieldName.substring(0,bi).trim(); - } - - JsonNode currentNode = baseNode.get(currentNodeName); - if (arrayIndex > -1 && currentNode.isArray()) { - logger.debug("Going to take array node at index " + arrayIndex); - currentNode = currentNode.get(arrayIndex); - } - - if (currentNode == null) { - logger.debug("JsonNode not found for name " + currentFieldName); + List fields = OIDCAttributeMapperHelper.splitClaimPath(fieldPath); + if (fields.isEmpty() || fieldPath.endsWith(".")) { + logger.debug("JSON path is invalid " + fieldPath); return null; } - if (idx < 0) { + JsonNode currentNode = baseNode; + for (String currentFieldName : fields) { + + // if array path, retrieve field name and index + String currentNodeName = currentFieldName; + int arrayIndex = -1; + if (currentFieldName.endsWith("]")) { + int bi = currentFieldName.indexOf("["); + if (bi == -1) { + logger.debug("Invalid array index construct in " + currentFieldName); + return null; + } + try { + String is = currentFieldName.substring(bi + 1, currentFieldName.length() - 1).trim(); + arrayIndex = Integer.parseInt(is); + if( arrayIndex < 0) throw new ArrayIndexOutOfBoundsException(); + } catch (Exception e) { + logger.debug("Invalid array index construct in " + currentFieldName); + return null; + } + currentNodeName = currentFieldName.substring(0, bi).trim(); + } + + currentNode = currentNode.get(currentNodeName); + if (arrayIndex > -1 && currentNode.isArray()) { + logger.debug("Going to take array node at index " + arrayIndex); + currentNode = currentNode.get(arrayIndex); + } + + if (currentNode == null) { + logger.debug("JsonNode not found for name " + currentFieldName); + return null; + } + if (currentNode.isArray()) { List values = new ArrayList<>(); for (JsonNode childNode : currentNode) { @@ -222,20 +223,22 @@ public abstract class AbstractJsonUserAttributeMapper extends AbstractIdentityPr if (values.isEmpty()) { return null; } - return arrayIndex == idx? values : null; - } - if (currentNode.isNull()) { + return values ; + } else if (currentNode.isNull()) { + logger.debug("JsonNode is null node for name " + currentFieldName); return null; - } else if (!currentNode.isValueNode()) { - return currentNode; + } else if (currentNode.isValueNode()) { + String ret = currentNode.asText(); + if (ret != null && !ret.trim().isEmpty()) + return ret.trim(); + else + return null; + } - String ret = currentNode.asText(); - if (ret != null && !ret.trim().isEmpty()) - return ret.trim(); - } else { - return getJsonValue(currentNode, fieldPath.substring(idx + 1)); + } + return currentNode; } return null; } diff --git a/services/src/test/java/org/keycloak/test/broker/oidc/mappers/AbstractJsonUserAttributeMapperTest.java b/services/src/test/java/org/keycloak/test/broker/oidc/mappers/AbstractJsonUserAttributeMapperTest.java index 49008256b6..aac45ef281 100755 --- a/services/src/test/java/org/keycloak/test/broker/oidc/mappers/AbstractJsonUserAttributeMapperTest.java +++ b/services/src/test/java/org/keycloak/test/broker/oidc/mappers/AbstractJsonUserAttributeMapperTest.java @@ -39,10 +39,11 @@ public class AbstractJsonUserAttributeMapperTest { private JsonNode getJsonNode() throws IOException { if (baseNode == null) - baseNode = mapper.readTree("{ \"value1\" : \"v1 \",\"value_null\" : null,\"value_empty\" : \"\", \"value_b\" : true, \"value_i\" : 454, " + " \"value_array\":[\"a1\",\"a2\"], " +" \"nest1\": {\"value1\": \" fgh \",\"value_null\" : null,\"value_empty\" : \"\", \"nest2\":{\"value_b\" : false, \"value_i\" : 43}}, "+ " \"nesta\": { \"a\":[{\"av1\": \"vala1\"},{\"av1\": \"vala2\"}]}"+" }"); + baseNode = mapper.readTree("{ \"dotted.claim\": \"claimValue\", \"nested.claim\" : { \"claim.with.dots\" : \"nested.claim.with.dots\"}, \"value1\" : \"v1 \",\"value_null\" : null,\"value_empty\" : \"\", \"value_b\" : true, \"value_i\" : 454, " + " \"value_array\":[\"a1\",\"a2\"], " +" \"nest1\": {\"value1\": \" fgh \",\"value_null\" : null,\"value_empty\" : \"\", \"nest2\":{\"value_b\" : false, \"value_i\" : 43}}, "+ " \"nesta\": { \"a\":[{\"av1\": \"vala1\"},{\"av1\": \"vala2\"}]}"+" }"); return baseNode; } + @Test public void getJsonValue_invalidPath() throws IOException { Assert.assertNull(AbstractJsonUserAttributeMapper.getJsonValue(getJsonNode(), ".")); @@ -68,6 +69,8 @@ public class AbstractJsonUserAttributeMapperTest { Assert.assertEquals("true", AbstractJsonUserAttributeMapper.getJsonValue(getJsonNode(), "value_b")); Assert.assertEquals("454", AbstractJsonUserAttributeMapper.getJsonValue(getJsonNode(), "value_i")); + Assert.assertEquals("claimValue", AbstractJsonUserAttributeMapper.getJsonValue(getJsonNode(), "dotted\\.claim")); + Assert.assertEquals("nested.claim.with.dots", AbstractJsonUserAttributeMapper.getJsonValue(getJsonNode(), "nested\\.claim.claim\\.with\\.dots")); } @@ -75,28 +78,28 @@ public class AbstractJsonUserAttributeMapperTest { public void getJsonValue_nestedSimpleValues() throws JsonProcessingException, IOException { // JsonNode if path points to JSON object Assert.assertEquals(mapper.readTree("{\n" - + " \"value1\": \" fgh \",\n" - + " \"value_null\" : null,\n" - + " \"value_empty\" : \"\",\n" - + " \"nest2\":{\n" - + " \"value_b\" : false,\n" - + " \"value_i\" : 43\n" - + " }\n" - + " }"), AbstractJsonUserAttributeMapper.getJsonValue(getJsonNode(), "nest1")); - Assert.assertEquals(mapper.readTree("{\n" - + " \"value_b\" : false,\n" - + " \"value_i\" : 43\n" - + " }"), AbstractJsonUserAttributeMapper.getJsonValue(getJsonNode(), "nest1.nest2")); + + " \"value1\": \" fgh \",\n" + + " \"value_null\" : null,\n" + + " \"value_empty\" : \"\",\n" + + " \"nest2\":{\n" + + " \"value_b\" : false,\n" + + " \"value_i\" : 43\n" + + " }\n" + + " }"), AbstractJsonUserAttributeMapper.getJsonValue(getJsonNode(), "nest1")); + Assert.assertEquals(mapper.readTree("{\n" + + " \"value_b\" : false,\n" + + " \"value_i\" : 43\n" + + " }"), AbstractJsonUserAttributeMapper.getJsonValue(getJsonNode(), "nest1.nest2")); Assert.assertEquals(mapper.readTree("{\"av1\": \"vala1\"}"), AbstractJsonUserAttributeMapper.getJsonValue(getJsonNode(), "nesta.a[0]")); - //unknown field returns null - Assert.assertEquals(null, AbstractJsonUserAttributeMapper.getJsonValue(getJsonNode(), "nest1.value_unknown")); - Assert.assertEquals(null, AbstractJsonUserAttributeMapper.getJsonValue(getJsonNode(), "nest1.nest2.value_unknown")); + //unknown field returns null + Assert.assertEquals(null, AbstractJsonUserAttributeMapper.getJsonValue(getJsonNode(), "nest1.value_unknown")); + Assert.assertEquals(null, AbstractJsonUserAttributeMapper.getJsonValue(getJsonNode(), "nest1.nest2.value_unknown")); // we check value is trimmed also! Assert.assertEquals("fgh", AbstractJsonUserAttributeMapper.getJsonValue(getJsonNode(), "nest1.value1")); // test for KEYCLOAK-4202 bug (null value handling) - Assert.assertEquals(null, AbstractJsonUserAttributeMapper.getJsonValue(getJsonNode(), "nest1.value_null")); + Assert.assertEquals(null, AbstractJsonUserAttributeMapper.getJsonValue(getJsonNode(), "nest1.value_null")); Assert.assertEquals(null, AbstractJsonUserAttributeMapper.getJsonValue(getJsonNode(), "nest1.value_empty")); Assert.assertEquals("false", AbstractJsonUserAttributeMapper.getJsonValue(getJsonNode(), "nest1.nest2.value_b"));