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

Author: ovidiupopa07

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<String> results = new ArrayList<>();
 			for (Object object : ((Collection<?>) source)) {

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<String, Object> result = new HashMap<>();
 		sourceMap.forEach((k, v) -> result.put(k.toString(), v));
 		return result;

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<String> 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<String> 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,15 +205,16 @@ public void convertMapStringObjectWhenNullThenReturnNull() {
 	}
 
 	@Test
-	public void convertMapStringObjectWhenMapStringObjectThenReturnSame() {
+	public void convertMapStringObjectWhenMapStringObjectThenReturnNotSameButEqual() {
 		Map<String, Object> mapStringObject = new HashMap<String, Object>() {
 			{
 				put("key1", "value1");
 				put("key2", "value2");
 				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<String, Object> mapStringObject = new HashMap<String, Object>() {
+			{
+				put("1", "value1");
+				put("2", "value2");
+			}
+		};
+		assertThat(this.conversionService.convert(jsonObject, Map.class)).isNotInstanceOf(JSONObject.class)
+				.isEqualTo(mapStringObject);
+	}
+
 	@Test
 	public void convertMapStringObjectWhenNotConvertibleThenReturnNull() {
 		List<String> notConvertibleList = Lists.list("1", "2", "3", "4");

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<String, Object> mapStringObject = new HashMap<>();
 		mapStringObject.put("1", "value1");
+		JSONArray jsonArray = new JSONArray();
+		jsonArray.add("1");
+		List<String> jsonArrayListString = Lists.list("1");
+		JSONObject jsonObject = new JSONObject();
+		jsonObject.put("1", "value1");
+		Map<String, Object> jsonObjectMap = Maps.newHashMap("1", "value1");
 		Map<String, Object> 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