From 7d31837af32ee86bd23a32aa7a5a9fe28bcc21cf Mon Sep 17 00:00:00 2001 From: Ovidiu Popa Date: Fri, 27 Nov 2020 09:10:54 +0200 Subject: [PATCH] OidcIdToken cannot be serialized to JSON if token contains claim of type JSONArray or JSONObject ObjectToListStringConverter and ObjectToMapStringObjectConverter were checking if the source object is of type List or Map and if the first element or key is a String. If we have a JSONArray containing Strings the above check will pass, meaning that a JSONArray will be returned which is not serializable (same applies to JSONObject) With this change, even if the check is passing a new List or Map will be returned. Closes gh-9210 --- .../ObjectToListStringConverter.java | 6 --- .../ObjectToMapStringObjectConverter.java | 3 -- .../ClaimConversionServiceTests.java | 45 ++++++++++++++++--- .../converter/ClaimTypeConverterTests.java | 23 +++++++--- 4 files changed, 56 insertions(+), 21 deletions(-) diff --git a/oauth2/oauth2-core/src/main/java/org/springframework/security/oauth2/core/converter/ObjectToListStringConverter.java b/oauth2/oauth2-core/src/main/java/org/springframework/security/oauth2/core/converter/ObjectToListStringConverter.java index daba913f25..ceea009f74 100644 --- a/oauth2/oauth2-core/src/main/java/org/springframework/security/oauth2/core/converter/ObjectToListStringConverter.java +++ b/oauth2/oauth2-core/src/main/java/org/springframework/security/oauth2/core/converter/ObjectToListStringConverter.java @@ -56,12 +56,6 @@ final class ObjectToListStringConverter implements ConditionalGenericConverter { if (source == null) { return null; } - if (source instanceof List) { - List sourceList = (List) source; - if (!sourceList.isEmpty() && sourceList.get(0) instanceof String) { - return source; - } - } if (source instanceof Collection) { Collection results = new ArrayList<>(); for (Object object : ((Collection) source)) { diff --git a/oauth2/oauth2-core/src/main/java/org/springframework/security/oauth2/core/converter/ObjectToMapStringObjectConverter.java b/oauth2/oauth2-core/src/main/java/org/springframework/security/oauth2/core/converter/ObjectToMapStringObjectConverter.java index 6db09f9cf1..23874b71df 100644 --- a/oauth2/oauth2-core/src/main/java/org/springframework/security/oauth2/core/converter/ObjectToMapStringObjectConverter.java +++ b/oauth2/oauth2-core/src/main/java/org/springframework/security/oauth2/core/converter/ObjectToMapStringObjectConverter.java @@ -52,9 +52,6 @@ final class ObjectToMapStringObjectConverter implements ConditionalGenericConver return null; } Map sourceMap = (Map) source; - if (!sourceMap.isEmpty() && sourceMap.keySet().iterator().next() instanceof String) { - return source; - } Map result = new HashMap<>(); sourceMap.forEach((k, v) -> result.put(k.toString(), v)); return result; diff --git a/oauth2/oauth2-core/src/test/java/org/springframework/security/oauth2/core/converter/ClaimConversionServiceTests.java b/oauth2/oauth2-core/src/test/java/org/springframework/security/oauth2/core/converter/ClaimConversionServiceTests.java index c7b8cc3571..109c6e2b8e 100644 --- a/oauth2/oauth2-core/src/test/java/org/springframework/security/oauth2/core/converter/ClaimConversionServiceTests.java +++ b/oauth2/oauth2-core/src/test/java/org/springframework/security/oauth2/core/converter/ClaimConversionServiceTests.java @@ -15,9 +15,10 @@ */ package org.springframework.security.oauth2.core.converter; +import net.minidev.json.JSONArray; +import net.minidev.json.JSONObject; import org.assertj.core.util.Lists; import org.junit.Test; -import org.springframework.core.convert.ConversionService; import java.net.URL; import java.time.Instant; @@ -29,6 +30,8 @@ import java.util.HashMap; import java.util.List; import java.util.Map; +import org.springframework.core.convert.ConversionService; + import static org.assertj.core.api.Assertions.assertThat; /** @@ -141,9 +144,9 @@ public class ClaimConversionServiceTests { } @Test - public void convertCollectionStringWhenListStringThenReturnSame() { + public void convertCollectionStringWhenListStringThenReturnNotSameButEqual() { List list = Lists.list("1", "2", "3", "4"); - assertThat(this.conversionService.convert(list, Collection.class)).isSameAs(list); + assertThat(this.conversionService.convert(list, Collection.class)).isNotSameAs(list).isEqualTo(list); } @Test @@ -152,6 +155,17 @@ public class ClaimConversionServiceTests { .isEqualTo(Lists.list("1", "2", "3", "4")); } + @Test + public void convertListStringWhenJsonArrayThenConverts() { + JSONArray jsonArray = new JSONArray(); + jsonArray.add("1"); + jsonArray.add("2"); + jsonArray.add("3"); + jsonArray.add(null); + assertThat(this.conversionService.convert(jsonArray, List.class)).isNotInstanceOf(JSONArray.class) + .isEqualTo(Lists.list("1", "2", "3")); + } + @Test public void convertCollectionStringWhenNotConvertibleThenReturnSingletonList() { String string = "not-convertible-collection"; @@ -165,9 +179,9 @@ public class ClaimConversionServiceTests { } @Test - public void convertListStringWhenListStringThenReturnSame() { + public void convertListStringWhenListStringThenReturnNotSameButEqual() { List list = Lists.list("1", "2", "3", "4"); - assertThat(this.conversionService.convert(list, List.class)).isSameAs(list); + assertThat(this.conversionService.convert(list, List.class)).isNotSameAs(list).isEqualTo(list); } @Test @@ -189,7 +203,7 @@ public class ClaimConversionServiceTests { } @Test - public void convertMapStringObjectWhenMapStringObjectThenReturnSame() { + public void convertMapStringObjectWhenMapStringObjectThenReturnNotSameButEqual() { Map mapStringObject = new HashMap() { { put("key1", "value1"); @@ -197,7 +211,8 @@ public class ClaimConversionServiceTests { put("key3", "value3"); } }; - assertThat(this.conversionService.convert(mapStringObject, Map.class)).isSameAs(mapStringObject); + assertThat(this.conversionService.convert(mapStringObject, Map.class)).isNotSameAs(mapStringObject) + .isEqualTo(mapStringObject); } @Test @@ -219,6 +234,22 @@ public class ClaimConversionServiceTests { assertThat(this.conversionService.convert(mapIntegerObject, Map.class)).isEqualTo(mapStringObject); } + @Test + public void convertMapStringObjectWhenJsonObjectThenConverts() { + JSONObject jsonObject = new JSONObject(); + jsonObject.put("1", "value1"); + jsonObject.put("2", "value2"); + + Map mapStringObject = new HashMap() { + { + put("1", "value1"); + put("2", "value2"); + } + }; + assertThat(this.conversionService.convert(jsonObject, Map.class)).isNotInstanceOf(JSONObject.class) + .isEqualTo(mapStringObject); + } + @Test public void convertMapStringObjectWhenNotConvertibleThenReturnNull() { List notConvertibleList = Lists.list("1", "2", "3", "4"); diff --git a/oauth2/oauth2-core/src/test/java/org/springframework/security/oauth2/core/converter/ClaimTypeConverterTests.java b/oauth2/oauth2-core/src/test/java/org/springframework/security/oauth2/core/converter/ClaimTypeConverterTests.java index fee193f8e4..d95cb9b366 100644 --- a/oauth2/oauth2-core/src/test/java/org/springframework/security/oauth2/core/converter/ClaimTypeConverterTests.java +++ b/oauth2/oauth2-core/src/test/java/org/springframework/security/oauth2/core/converter/ClaimTypeConverterTests.java @@ -15,7 +15,10 @@ */ package org.springframework.security.oauth2.core.converter; +import net.minidev.json.JSONArray; +import net.minidev.json.JSONObject; import org.assertj.core.util.Lists; +import org.assertj.core.util.Maps; import org.junit.Before; import org.junit.Test; import org.springframework.core.convert.TypeDescriptor; @@ -45,6 +48,8 @@ public class ClaimTypeConverterTests { private static final String COLLECTION_STRING_CLAIM = "collection-string-claim"; private static final String LIST_STRING_CLAIM = "list-string-claim"; private static final String MAP_STRING_OBJECT_CLAIM = "map-string-object-claim"; + private static final String JSON_ARRAY_CLAIM = "json-array-claim"; + private static final String JSON_OBJECT_CLAIM = "json-object-claim"; private ClaimTypeConverter claimTypeConverter; @Before @@ -107,7 +112,12 @@ public class ClaimTypeConverterTests { mapIntegerObject.put(1, "value1"); Map mapStringObject = new HashMap<>(); mapStringObject.put("1", "value1"); - + JSONArray jsonArray = new JSONArray(); + jsonArray.add("1"); + List jsonArrayListString = Lists.list("1"); + JSONObject jsonObject = new JSONObject(); + jsonObject.put("1", "value1"); + Map jsonObjectMap = Maps.newHashMap("1", "value1"); Map claims = new HashMap<>(); claims.put(STRING_CLAIM, Boolean.TRUE); claims.put(BOOLEAN_CLAIM, "true"); @@ -116,7 +126,8 @@ public class ClaimTypeConverterTests { claims.put(COLLECTION_STRING_CLAIM, listNumber); claims.put(LIST_STRING_CLAIM, listNumber); claims.put(MAP_STRING_OBJECT_CLAIM, mapIntegerObject); - + claims.put(JSON_ARRAY_CLAIM, jsonArray); + claims.put(JSON_OBJECT_CLAIM, jsonObject); claims = this.claimTypeConverter.convert(claims); assertThat(claims.get(STRING_CLAIM)).isEqualTo("true"); @@ -126,6 +137,8 @@ public class ClaimTypeConverterTests { assertThat(claims.get(COLLECTION_STRING_CLAIM)).isEqualTo(listString); assertThat(claims.get(LIST_STRING_CLAIM)).isEqualTo(listString); assertThat(claims.get(MAP_STRING_OBJECT_CLAIM)).isEqualTo(mapStringObject); + assertThat(claims.get(JSON_ARRAY_CLAIM)).isEqualTo(jsonArrayListString); + assertThat(claims.get(JSON_OBJECT_CLAIM)).isEqualTo(jsonObjectMap); } @Test @@ -153,9 +166,9 @@ public class ClaimTypeConverterTests { assertThat(claims.get(BOOLEAN_CLAIM)).isSameAs(bool); assertThat(claims.get(INSTANT_CLAIM)).isSameAs(instant); assertThat(claims.get(URL_CLAIM)).isSameAs(url); - assertThat(claims.get(COLLECTION_STRING_CLAIM)).isSameAs(listString); - assertThat(claims.get(LIST_STRING_CLAIM)).isSameAs(listString); - assertThat(claims.get(MAP_STRING_OBJECT_CLAIM)).isSameAs(mapStringObject); + assertThat(claims.get(COLLECTION_STRING_CLAIM)).isNotSameAs(listString).isEqualTo(listString); + assertThat(claims.get(LIST_STRING_CLAIM)).isNotSameAs(listString).isEqualTo(listString); + assertThat(claims.get(MAP_STRING_OBJECT_CLAIM)).isNotSameAs(mapStringObject).isEqualTo(mapStringObject); } @Test