From b92889d515461ab0910beb909ff85fce9858572d 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 0597f561fed..c1a70f96511 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 @@ public Object convert(Object source, TypeDescriptor sourceType, TypeDescriptor t 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 08d4f6ef971..2a6e2418124 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 @@ public Object convert(Object source, TypeDescriptor sourceType, TypeDescriptor t 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 6a07f8f37f4..f77687a4284 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.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 void convertCollectionStringWhenNullThenReturnNull() { } @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 void convertCollectionStringWhenListNumberThenConverts() { .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 void convertListStringWhenNullThenReturnNull() { } @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 void convertMapStringObjectWhenNullThenReturnNull() { } @Test - public void convertMapStringObjectWhenMapStringObjectThenReturnSame() { + public void convertMapStringObjectWhenMapStringObjectThenReturnNotSameButEqual() { Map mapStringObject = new HashMap() { { put("key1", "value1"); @@ -200,7 +213,8 @@ public void convertMapStringObjectWhenMapStringObjectThenReturnSame() { 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 void convertMapStringObjectWhenMapIntegerObjectThenConverts() { 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 0b0d9f0acd7..2a1f329df17 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.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 void convertWhenAllClaimsRequireConversionThenConvertAll() throws Excepti 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 void convertWhenAllClaimsRequireConversionThenConvertAll() throws Excepti 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 void convertWhenAllClaimsRequireConversionThenConvertAll() throws Excepti 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 void convertWhenNoClaimsRequireConversionThenConvertNone() throws Excepti 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