From b8175bccd23cf37d3ccdd457747843537ac71a86 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 | 42 ++++++++++++++++--- .../converter/ClaimTypeConverterTests.java | 23 ++++++++-- 4 files changed, 56 insertions(+), 18 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 0597f561fe..c1a70f9651 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 08d4f6ef97..2a6e241812 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 @@ -53,9 +53,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 6a07f8f37f..f77687a428 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 @@ -26,6 +26,8 @@ import java.util.HashMap; import java.util.List; import java.util.Map; +import com.nimbusds.jose.shaded.json.JSONArray; +import com.nimbusds.jose.shaded.json.JSONObject; import org.assertj.core.util.Lists; import org.junit.Test; @@ -145,9 +147,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 @@ -156,6 +158,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"; @@ -169,9 +182,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 @@ -192,7 +205,7 @@ public class ClaimConversionServiceTests { } @Test - public void convertMapStringObjectWhenMapStringObjectThenReturnSame() { + public void convertMapStringObjectWhenMapStringObjectThenReturnNotSameButEqual() { Map mapStringObject = new HashMap() { { put("key1", "value1"); @@ -200,7 +213,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 @@ -222,6 +236,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 0b0d9f0acd..2a1f329df1 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 @@ -23,7 +23,10 @@ import java.util.HashMap; import java.util.List; import java.util.Map; +import com.nimbusds.jose.shaded.json.JSONArray; +import com.nimbusds.jose.shaded.json.JSONObject; import org.assertj.core.util.Lists; +import org.assertj.core.util.Maps; import org.junit.Before; import org.junit.Test; @@ -55,6 +58,10 @@ public class ClaimTypeConverterTests { 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 @@ -115,6 +122,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"); @@ -123,6 +136,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"); assertThat(claims.get(BOOLEAN_CLAIM)).isEqualTo(Boolean.TRUE); @@ -131,6 +146,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 @@ -155,9 +172,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