Jwt Claim Mapping #5753

jzheaux · 2018-08-28T22:03:49Z

This introduces a hook for users to customize standard Jwt Claim
values in cases where the JWT issuer isn't spec compliant.

Fixes: gh-5223

jzheaux · 2018-08-28T22:05:39Z

@rwinch What do you think of this as an API for doing claims mapping? I like it because it feels similar to JwtAuthenticationConverter.

A little unsure about the name, but I didn't like JwtBuilder since it doesn't really follow the builder pattern (just like JwtAuthenticationConverter doesn't).

rwinch · 2018-08-31T16:12:42Z

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

+ * @author Josh Cummings
+ * @since 5.1
+ */
+public class JwtCreator {


I"m not very keen on using a class for conversion because the whole point is to allow the user to customize the conversion which means they will likely want a new implementation. For that reason, our main hook should be an interface.

I don't like the idea of introducing a custom interface just for this either.

Perhaps the basis could be Converter<Jtw, Jwt> such that we take in the default Jwt but then they can convert it into one that has converted types on headers or claims that they want.

jzheaux · 2018-09-04T23:51:25Z

@rwinch I've implemented two different approaches here, just continuing the conversation we were having on slack.

Here are the tradeoffs I see with the mapped approach (MappedJwtClaimSetConverter):

Pro: It is flexible enough to handle any claim, including custom ones.
Con: We commit to a constructor that presumes a mapping approach. I don't really know another way around this other than to expose a setter method for each claim.
Con: We lose the ability to communicate a type contract for each of the claims, meaning if they performed an incompatible conversion, they would find out at runtime instead of compile time.

And with the sub-class approach (JwtClaimSetConverter):

Pro: The contract is clear.
Con: We expose a larger API. However, we can expose these passively on an as-needed basis.
Con: It doesn't handle custom claims, though I would imagine that a user could easily use a delegate pattern to chain converters, the first being the standard one and the second being their custom claims (and custom implementation of Converter).

Personally, I lean slightly towards JwtClaimSetConverter because I like the type safety, and I also think that the API is simple enough that I'm not worried about exposing a method per standard claim. I also find the mapped converter a bit abstract for my taste.

jzheaux · 2018-09-04T23:53:12Z

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

+		Map<String, Object> mappedClaims = new HashMap<>(source);
+		JwtClaimAccessor claimAccessor = () -> mappedClaims;
+
+		for (Map.Entry<String, Converter<JwtClaimAccessor, ?>> entry : this.attributeConverters.entrySet()) {


Iterating through the converters instead of the claims because this allows converters to add and remove values as well as convert them.

jzheaux · 2018-09-04T23:53:55Z

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

+		return mappedClaims;
+	}
+
+	public static class ConverterEntry {


I liked this because it made the constructor very simple. I also find working with var args much simpler than having to build a map and then pass it in.

jzheaux · 2018-09-04T23:54:45Z

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

+	private class IssuedAtConverter implements Converter<JwtClaimAccessor, Instant> {
+
+		@Override
+		public Instant convert(JwtClaimAccessor claimAccessor) {


This is necessary so that we continue to convert issuedAt in the same way we have currently released (e.g. falling back to expiresAt).

jzheaux · 2018-09-05T23:33:24Z

@rwinch This is ready for your review

rwinch · 2018-09-06T13:35:10Z

...g/annotation/web/configurers/oauth2/server/resource/OAuth2ResourceServerConfigurerTests.java

@@ -907,7 +909,7 @@ public void requestWhenClockSkewSetButJwtStillTooLateThenReportsExpired()
 				.andExpect(invalidTokenHeader("Jwt expired at"));
 	}

-	// -- converter
+	// -- jwt authentication converter


These tests shouldn't be added because they are much slower than unit tests and they do not have anything to do with Java Configuration. We should only have tests added to NimbusJwtDecoderJwkSupport with a Mock converter and tests in MappedJwtClaimSetConverterTests

Just curious, would these be appropriate here if they were fast and didn't use a mock web server? A number of the tests in this class are there for user demonstration and also verification of a documentable feature.

Adding or changing a claim do not seem like corner cases to me, and so where would we place a test that verifies this configuration combination (or why would we not want to verify it)?

Would it be appropriate to create a ticket to add these tests after we have a NimbusJwtDecoder implementation that can easily mock the Nimbus JwtProcessor (thus making these tests fast and not require a mock web server)?

rwinch · 2018-09-06T13:36:57Z

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

+	 *   plus any defaults that were not overridden.
+	 */
+	public static MappedJwtClaimSetConverter withDefaults
+			(Map<String, Converter<Map<String, Object>, ?>> attributeConverters) {


I prefer to name a map based upon the key and value. For example, claimIdToConverter

I had imagined the converters to operate on an individual key value pair. By doing this, we could have a standard Instant converter, List converter, etc. Is there a reason/benefit to having it operate on the entire Map?

The reason that they take a Map (the claim set) is so that the implementer can access all attributes in making their conversion decision. Taking a Map doesn't prevent having a standard Instant converter, etc., unless I misunderstand your thinking. The PR has TemporalConverter and StringConverter already, for example. Are these less than ideal, and if so, why?

To connect this to another comment that you made (apologies for any noise, but I believe they go together), I think that the decision to have issuedAt fall back to one second before expiresAt is implementation-specific and likely to be overridden. As a user, I would personally prefer to override this to simply accept and convert whatever issuedAt is, and so it's isolated to the conversion of that property.

Given that, an example of where having the entire map available is with issuedAt where we fall back to expiresAt, and thus need to look up expiresAt to correctly derive issuedAt. (See my later comments for more discussion on this specific matter. I only include it here, too, to hopefully make it easier to tie the two points together.)

I agree that converters that simply take and return a single attribute would be simpler. What are your thoughts on future passivity if converters need to look at more than one attribute to make their conversion decision? (Presuming hypothetically that we can find a more elegant way to allow users to override our defaulting of issuedAt, removing the immediate need for this requirement).

As a side note, these originally looked something like this:

withDefaults(Map<String, Converter<JwtClaimAccessor, ?>> overrides) { // ... withDefaults.put(JwtClaimNames.SUB, accessor -> accessor.getSubject()); withDefaults.put(JwtClaimNames.JTI, accessor -> accessor.getId()); // ... }

which may be something still considering, though I think we'd still want the conversion logic to reside here in the converter, even if we use JwtClaimAccessor as the conversion source.

rwinch · 2018-09-06T13:41:52Z

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

+		}
+	}
+
+	private static class IssuedAtConverter extends TemporalConverter {


Favor composition to inheritance since Java only lets you inherit from a single class. Instead the temporal converter could accept an actual value (instead of the entire Map).

rwinch · 2018-09-06T13:44:01Z

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

+			return expiresAt.minusSeconds(1);
+		}
+
+		public void setAttributeConverters


This should likely be a constructor argument since is it is required. A few alternatives to consider is injecting only the required converter or perhaps the converted value.

rwinch · 2018-09-06T13:46:44Z

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

+				return issuedAt;
+			}
+
+			Instant expiresAt = getExpiresAt(source);


I really view this as something distinct from conversion of the attributes. It is really defaulting the value. I think it might be best to place this in MappedJwtClaimSetConverter directly. This will also allow simplification of the API as suggested above

I agree that this is more about defaults than conversion. I also think that we should focus just on conversion and not concern ourselves with defaulting properties in the first place. My motive, then, for placing this logic in the converter is to make it easy for users to override this defaulting.

Since this has the potential to change the contract (as discussed above), I'd like to hear your thoughts on other ways that users could easily override this defaulting of issuedAt. My concern is that I cannot do the fallback before conversion since it includes date arithmetic. And so, if it is after the set of conversions, then the user needs to completely reimplement the converter (or the JwtDecoder) in order to remove that defaulting.

jzheaux · 2018-09-06T18:37:12Z

@rwinch I simplified the contract to focus on individual attributes. If a user needs the entire map to make a decision, they can just wire an additional converter that delegates to this one and then does whatever other work it needs to do. Let me know what you think.

jzheaux · 2018-09-06T18:44:05Z

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

+			}
+		}
+
+		Instant issuedAt = (Instant) mappedClaims.get(JwtClaimNames.IAT);


Separate ticket maybe, but I like the idea of also having IssuedAtDefaultingClaimSetConverter which delegates to MappedJwtSetConverter and performs these steps on the map, e.g.:

public class IssuedAtDefaultingClaimSetConverter implements Converter<...> { private final Conveter<...> delegate = MappedJwtClaimSetConverter.withDefaults(...); public Map<String, Object> convert(Map<String, Object> claims) { Map<String, Object> mappedClaims = this.delegate.convert(claims); Instant issuedAt = (Instant) mappedClaims.get(JwtClaimNames.IAT); Instant expiresAt = (Instant) mappedClaims.get(JwtClaimNames.EXP); if (issuedAt == null && expiresAt != null) { mappedClaims.put(JwtClaimNames.IAT, expiresAt.minusSeconds(1)); } return mappedClaims; }

This way, users can easily do

Converter<...> converter = MappedJwtClaimSetConverter.withDefaults(...); NimbusJwtDecoder#setClaimSetConverter(converter);

rwinch · 2018-09-06T19:17:10Z

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

+		}
+	}
+
+	private static class StringConverter implements Converter<Object, String> {


Aren't the inputs almost always a String anyways? Is this necessary?

rwinch · 2018-09-06T19:19:28Z

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

+			try {
+				return issuer.toURL();
+			} catch (MalformedURLException e) {
+				throw new IllegalStateException("Could not coerce iss value to URL");


Please include the value in the exception message

rwinch · 2018-09-06T19:19:41Z

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

+			try {
+				return new URL(issuer);
+			} catch (MalformedURLException e) {
+				throw new IllegalStateException("Could not coerce iss value to URL");


Please include the value in the error message

rwinch · 2018-09-06T19:20:56Z

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

+			try {
+				return Instant.ofEpochSecond(Long.parseLong(temporal.toString()));
+			} catch (Exception e) {
+				throw new IllegalStateException("Could not coerce this value into Instant");


Please include the value and the caused by

rwinch · 2018-09-06T19:24:51Z

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

+	 */
+	public MappedJwtClaimSetConverter(Map<String, Converter<Object, ?>> attributeConverters) {
+		Assert.notNull(attributeConverters, "attributeConverters cannot be null");
+		this.attributeConverters.putAll(attributeConverters);


I'd prefer to make this more explicit that the attributeConverters does not contain any values already and keep the initialization logic in one place. Please change this to new HashMap<>(attributeConverters)

rwinch · 2018-09-06T19:26:01Z

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

+import org.springframework.util.Assert;
+
+/**
+ * Converts a JWT claim set attribute by attribute. Can be configured with custom converters


by attribute

to

by attribute name.

rwinch · 2018-09-06T19:27:56Z

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

+		Map<String, Object> mappedClaims = new HashMap<>(claims);
+
+		for (Map.Entry<String, Converter<Object, ?>> entry : this.attributeConverters.entrySet()) {
+			Converter<Object, ?> converter = this.attributeConverters.get(entry.getKey());


You can get the converter using entry.getValue() which will be slightly better performance.

rwinch · 2018-09-06T19:28:29Z

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

+		Map<String, Object> mappedClaims = new HashMap<>(claims);
+
+		for (Map.Entry<String, Converter<Object, ?>> entry : this.attributeConverters.entrySet()) {
+			Converter<Object, ?> converter = this.attributeConverters.get(entry.getKey());


entry.getKey() is used in multiple places and is not very readable. Please assign to something like attributeName

rwinch · 2018-09-06T19:37:26Z

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

+		}
+	}
+
+	private void put(Map<String, Object> map, String key, Object value) {


remove this and use Map.compute directly

rwinch · 2018-09-06T19:40:41Z

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

+		}
+	}
+
+	private static class TemporalConverter implements Converter<Object, Instant> {


Is there a reason this is called TemporalConverter? It converts to an Instant which is what the other converters are named after.

This introduces a hook for users to customize standard Jwt Claim values in cases where the JWT issuer isn't spec compliant or where the user needs to add or remove claims. Fixes: spring-projectsgh-5223

jzheaux · 2018-09-06T20:52:17Z

Awesome, @rwinch, thanks for the feedback. I've responded with code changes.

rwinch requested changes Aug 31, 2018

View reviewed changes

jzheaux force-pushed the gh-5223 branch from b087ee6 to 162c7dc Compare September 4, 2018 23:35

jzheaux added the wip label Sep 4, 2018

jzheaux commented Sep 4, 2018

View reviewed changes

jzheaux force-pushed the gh-5223 branch 4 times, most recently from 3011097 to 479f5af Compare September 5, 2018 23:30

jzheaux removed the wip label Sep 5, 2018

jzheaux force-pushed the gh-5223 branch 2 times, most recently from 02cd97f to 70f2aeb Compare September 5, 2018 23:58

rwinch requested changes Sep 6, 2018

View reviewed changes

jzheaux force-pushed the gh-5223 branch from 70f2aeb to 3125c8f Compare September 6, 2018 17:49

jzheaux commented Sep 6, 2018

View reviewed changes

jzheaux force-pushed the gh-5223 branch from 3125c8f to a8584fc Compare September 6, 2018 18:46

rwinch requested changes Sep 6, 2018

View reviewed changes

jzheaux force-pushed the gh-5223 branch 2 times, most recently from 6cacbfb to d1820d2 Compare September 6, 2018 20:34

Jwt Claim Mapping

1acdbbe

This introduces a hook for users to customize standard Jwt Claim values in cases where the JWT issuer isn't spec compliant or where the user needs to add or remove claims. Fixes: spring-projectsgh-5223

jzheaux force-pushed the gh-5223 branch from d1820d2 to 1acdbbe Compare September 6, 2018 20:46

rwinch self-assigned this Sep 7, 2018

rwinch added status: duplicate A duplicate of another issue in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) labels Sep 7, 2018

rwinch added this to the 5.1.0.RC2 milestone Sep 7, 2018

rwinch approved these changes Sep 7, 2018

View reviewed changes

rwinch merged commit 9e0f171 into spring-projects:master Sep 7, 2018

Jwt Claim Mapping #5753

Jwt Claim Mapping #5753

Uh oh!

Conversation

jzheaux commented Aug 28, 2018

Uh oh!

jzheaux commented Aug 28, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jzheaux commented Sep 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jzheaux commented Sep 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jzheaux commented Sep 6, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jzheaux commented Sep 6, 2018

Uh oh!

Uh oh!