Polish Preserve Null Claim Values

Preserves the original behavior of ClaimTypeConverter so that its
converters can maintain their default behavior of null meaning that
conversion failed.

Issue gh-10135

Author: jzheaux

oauth2/oauth2-core/src/main/java/org/springframework/security/oauth2/core/converter/ClaimTypeConverter.java

@@ -56,7 +56,10 @@ public Map<String, Object> convert(Map<String, Object> claims) {
 		this.claimTypeConverters.forEach((claimName, typeConverter) -> {
 			if (claims.containsKey(claimName)) {
 				Object claim = claims.get(claimName);
-				result.put(claimName, typeConverter.convert(claim));
+				Object mappedClaim = typeConverter.convert(claim);
+				if (mappedClaim != null) {
+					result.put(claimName, mappedClaim);
+				}
 			}
 		});
 		return result;

oauth2/oauth2-core/src/test/java/org/springframework/security/oauth2/core/converter/ClaimTypeConverterTests.java

@@ -62,8 +62,6 @@ public class ClaimTypeConverterTests {
 
 	private static final String JSON_OBJECT_CLAIM = "json-object-claim";
 
-	private static final String NULL_OBJECT_CLAIM = "null-object-claim";
-
 	private ClaimTypeConverter claimTypeConverter;
 
 	@BeforeEach
@@ -79,7 +77,6 @@ public void setup() {
 				TypeDescriptor.collection(List.class, TypeDescriptor.valueOf(String.class)));
 		Converter<Object, ?> mapStringObjectConverter = getConverter(TypeDescriptor.map(Map.class,
 				TypeDescriptor.valueOf(String.class), TypeDescriptor.valueOf(Object.class)));
-		Converter<Object, ?> nullConverter = (value) -> null;
 		Map<String, Converter<Object, ?>> claimTypeConverters = new HashMap<>();
 		claimTypeConverters.put(STRING_CLAIM, stringConverter);
 		claimTypeConverters.put(BOOLEAN_CLAIM, booleanConverter);
@@ -88,7 +85,6 @@ public void setup() {
 		claimTypeConverters.put(COLLECTION_STRING_CLAIM, collectionStringConverter);
 		claimTypeConverters.put(LIST_STRING_CLAIM, listStringConverter);
 		claimTypeConverters.put(MAP_STRING_OBJECT_CLAIM, mapStringObjectConverter);
-		claimTypeConverters.put(NULL_OBJECT_CLAIM, nullConverter);
 		this.claimTypeConverter = new ClaimTypeConverter(claimTypeConverters);
 	}
 
@@ -142,7 +138,6 @@ public void convertWhenAllClaimsRequireConversionThenConvertAll() throws Excepti
 		claims.put(MAP_STRING_OBJECT_CLAIM, mapIntegerObject);
 		claims.put(JSON_ARRAY_CLAIM, jsonArray);
 		claims.put(JSON_OBJECT_CLAIM, jsonObject);
-		claims.put(NULL_OBJECT_CLAIM, instant.toString());
 		claims = this.claimTypeConverter.convert(claims);
 		assertThat(claims.get(STRING_CLAIM)).isEqualTo("true");
 		assertThat(claims.get(BOOLEAN_CLAIM)).isEqualTo(Boolean.TRUE);
@@ -153,7 +148,6 @@ public void convertWhenAllClaimsRequireConversionThenConvertAll() throws Excepti
 		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);
-		assertThat(claims.get(NULL_OBJECT_CLAIM)).isNull();
 	}
 
 	@Test

oauth2/oauth2-jose/src/main/java/org/springframework/security/oauth2/jwt/MappedJwtClaimSetConverter.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2019 the original author or authors.
+ * Copyright 2002-2021 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -52,18 +52,19 @@ public final class MappedJwtClaimSetConverter implements Converter<Map<String, O
 
 	private final Map<String, Converter<Object, ?>> claimTypeConverters;
 
-	private final Converter<Map<String, Object>, Map<String, Object>> delegate;
-
 	/**
 	 * Constructs a {@link MappedJwtClaimSetConverter} with the provided arguments
 	 *
 	 * This will completely replace any set of default converters.
+	 *
+	 * A converter that returns {@code null} removes the claim from the claim set. A
+	 * converter that returns a non-{@code null} value adds or replaces that claim in the
+	 * claim set.
 	 * @param claimTypeConverters The {@link Map} of converters to use
 	 */
 	public MappedJwtClaimSetConverter(Map<String, Converter<Object, ?>> claimTypeConverters) {
 		Assert.notNull(claimTypeConverters, "claimTypeConverters cannot be null");
 		this.claimTypeConverters = claimTypeConverters;
-		this.delegate = new ClaimTypeConverter(claimTypeConverters);
 	}
 
 	/**
@@ -87,6 +88,10 @@ public MappedJwtClaimSetConverter(Map<String, Converter<Object, ?>> claimTypeCon
 	 *
 	 * To completely replace the underlying {@link Map} of converters, see
 	 * {@link MappedJwtClaimSetConverter#MappedJwtClaimSetConverter(Map)}.
+	 *
+	 * A converter that returns {@code null} removes the claim from the claim set. A
+	 * converter that returns a non-{@code null} value adds or replaces that claim in the
+	 * claim set.
 	 * @param claimTypeConverters
 	 * @return An instance of {@link MappedJwtClaimSetConverter} that contains the
 	 * converters provided, plus any defaults that were not overridden.
@@ -143,9 +148,16 @@ private static String convertIssuer(Object source) {
 	@Override
 	public Map<String, Object> convert(Map<String, Object> claims) {
 		Assert.notNull(claims, "claims cannot be null");
-		Map<String, Object> mappedClaims = this.delegate.convert(claims);
-		mappedClaims = removeClaims(mappedClaims);
-		mappedClaims = addClaims(mappedClaims);
+		Map<String, Object> mappedClaims = new HashMap<>(claims);
+		for (Map.Entry<String, Converter<Object, ?>> entry : this.claimTypeConverters.entrySet()) {
+			String claimName = entry.getKey();
+			Converter<Object, ?> converter = entry.getValue();
+			if (converter != null) {
+				Object claim = claims.get(claimName);
+				Object mappedClaim = converter.convert(claim);
+				mappedClaims.compute(claimName, (key, value) -> mappedClaim);
+			}
+		}
 		Instant issuedAt = (Instant) mappedClaims.get(JwtClaimNames.IAT);
 		Instant expiresAt = (Instant) mappedClaims.get(JwtClaimNames.EXP);
 		if (issuedAt == null && expiresAt != null) {
@@ -154,24 +166,4 @@ public Map<String, Object> convert(Map<String, Object> claims) {
 		return mappedClaims;
 	}
 
-	private Map<String, Object> removeClaims(Map<String, Object> claims) {
-		Map<String, Object> result = new HashMap<>();
-		for (Map.Entry<String, Object> entry : claims.entrySet()) {
-			if (entry.getValue() != null) {
-				result.put(entry.getKey(), entry.getValue());
-			}
-		}
-		return result;
-	}
-
-	private Map<String, Object> addClaims(Map<String, Object> claims) {
-		Map<String, Object> result = new HashMap<>(claims);
-		for (Map.Entry<String, Converter<Object, ?>> entry : this.claimTypeConverters.entrySet()) {
-			if (!claims.containsKey(entry.getKey()) && entry.getValue().convert(null) != null) {
-				result.put(entry.getKey(), entry.getValue().convert(null));
-			}
-		}
-		return result;
-	}
-
 }

oauth2/oauth2-jose/src/test/java/org/springframework/security/oauth2/jwt/MappedJwtClaimSetConverterTests.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2018 the original author or authors.
+ * Copyright 2002-2021 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.