Preserve Null Claim Values

Prior to this commit ClaimTypeConverter returned the claims with the
original value for all the claims with a null converted value.
The changes allows ClaimTypeConverter to overwrite and return claims
with converted value of null.

Closes gh-10135

Author: Fabio Guenci

oauth2/oauth2-core/src/main/java/org/springframework/security/oauth2/core/converter/ClaimTypeConverter.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.

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.
@@ -123,8 +123,18 @@ public void convertWhenUsingCustomConverterThenAllOtherDefaultsAreStillUsed() {
 		assertThat(target.get(JwtClaimNames.SUB)).isEqualTo("1234");
 	}
 
+	// gh-10135
 	@Test
 	public void convertWhenConverterReturnsNullThenClaimIsRemoved() {
+		MappedJwtClaimSetConverter converter = MappedJwtClaimSetConverter
+				.withDefaults(Collections.singletonMap(JwtClaimNames.NBF, (nbfClaimValue) -> null));
+		Map<String, Object> source = Collections.singletonMap(JwtClaimNames.NBF, Instant.now());
+		Map<String, Object> target = converter.convert(source);
+		assertThat(target).doesNotContainKey(JwtClaimNames.NBF);
+	}
+
+	@Test
+	public void convertWhenClaimValueIsNullThenClaimIsRemoved() {
 		MappedJwtClaimSetConverter converter = MappedJwtClaimSetConverter.withDefaults(Collections.emptyMap());
 		Map<String, Object> source = Collections.singletonMap(JwtClaimNames.ISS, null);
 		Map<String, Object> target = converter.convert(source);