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

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<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 @@ final class ObjectToMapStringObjectConverter implements ConditionalGenericConver
 			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.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<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 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<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,7 +205,7 @@ public class ClaimConversionServiceTests {
 	}
 
 	@Test
-	public void convertMapStringObjectWhenMapStringObjectThenReturnSame() {
+	public void convertMapStringObjectWhenMapStringObjectThenReturnNotSameButEqual() {
 		Map<String, Object> mapStringObject = new HashMap<String, Object>() {
 			{
 				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<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.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<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 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