Externalize coercion in ClaimAccessor #6799

jgrandja · 2019-04-18T19:39:04Z

jgrandja · 2019-04-18T19:53:21Z

...e/src/test/java/org/springframework/security/oauth2/jwt/MappedJwtClaimSetConverterTests.java

 		Map<String, Object> source = new HashMap<>();
+		source.put(JwtClaimNames.EXP, exp);


@jzheaux The original test did not pass in any claims but the resulting claim set added iss claim. This does not seem right. If no claims are passed in than I would expect nothing to happen. See #6800

Thanks for pointing this out. Did you mean the iat claim? This was to remain backward compatible with NimbusJwtDecoderJwkSupport, though there are perhaps other ways to achieve this.

jgrandja · 2019-04-18T19:55:03Z

...e/src/test/java/org/springframework/security/oauth2/jwt/MappedJwtClaimSetConverterTests.java

@@ -140,30 +138,6 @@ public void convertWhenUsingCustomConverterThenAllOtherDefaultsAreStillUsed() th
 		assertThat(target.get(JwtClaimNames.SUB)).isEqualTo("1234");
 	}

-	@Test
-	public void convertWhenConverterReturnsNullThenClaimIsRemoved() {


@jzheaux I removed this test as described in #6800

jgrandja · 2019-04-18T19:55:18Z

...e/src/test/java/org/springframework/security/oauth2/jwt/MappedJwtClaimSetConverterTests.java

-	}
-
-	@Test
-	public void convertWhenConverterReturnsValueWhenEntryIsMissingThenEntryIsAdded() {


@jzheaux I removed this test as described in #6800

I'm not keen on removing these test until we come to an agreement on #6800. This PR isn't about changing how MappedJwtClaimSetConverter behaves.

jzheaux

Glad to see us taking this step forward! I've left some feedback inline.

jzheaux · 2019-04-18T20:18:25Z

core/src/main/java/org/springframework/security/converter/ObjectConverters.java

+	 *
+	 * @return an {@code Object} to {@code String} {@link Converter}
+	 */
+	public static Converter<Object, String> stringConverter() {


What are your thoughts around

public static final Converter<Object, String> STRING_CONVERTER = ObjectConverters::asString

It seems like a constant lookup would perform faster than a method invocation.

It seems like a constant lookup would perform faster than a method invocation

Not sure about this? Do you think the static method approach would be too slow?

Also keep in mind that this follows the same pattern as RsaKeyConverters.pkcs8() and RsaKeyConverters.x509 and being consistent is key.

The reason in RsaKeyConverters is so that each gets its own instance of KeyFactory, but I didn't see any state in the ObjectConverters.

There is a bit of a tradeoff. Holding static values improves performance a bit as it won't need to be garbage collected. However, it cannot be garbage collected when there continues to be a reference to it, so that means there is additional memory overhead.

@rwinch It's not clear to me whether you are requesting changes here? If you are, can you please provide more detail? With the current design, garbage collection won't happen after the class is loaded. For example, using stringConverter() as an example, which is static and it references the static method asString. Are you suggesting that stringConverter() be changed to non-static, more like a factory method?

I didn't really suggest one way or the other, but pointed out the tradeoffs.

With the current design, garbage collection won't happen after the class is loaded.

It will happen every time stringConverter() is used and the reference is no longer held. For example, here GC could happen as soon as foo() completed:

private void foo() { Converter<Object, String> toString = stringConverter(); ... }

If foo() is invoked a lot then there is going to be a lot of GC that can happen.

On the flip side ObjectConverters creates static members and returns the same instance, there would not be GC happening and multiple invocations of stringConverter() would return the same object. This is a plus. However, the static values will never be GC'd which is a tradeoff if we anticipate the values only being used at startup.

Given this is likely to be used outside of startup, I would suggest using the static members as @jzheaux suggested.

jzheaux · 2019-04-18T20:27:04Z

oauth2/oauth2-core/src/main/java/org/springframework/security/oauth2/core/ClaimAccessor.java

@@ -49,7 +46,7 @@
 	 */
 	default Boolean containsClaim(String claim) {


Now that these are converted up front, what are your thoughts on adding a method:

default <T> T getClaim(String claim)

It'd be really nice for users to be able to do something like:

List<String> scopes = jwt.getClaim("scope");

I'm not sure how we would implement it with the latest implementation that uses a ClaimConversionService. We would need to supply the targetType using ClaimConversionService.getSharedInstance().convert(claimValue, targetType), which must be captured via the return type of caller. Off the top of my head, I'm not aware how we can capture the generic return type? If it's even possible?

Either way, this would be a feature enhancement as it's not required for this PR. Feel free to log a ticket if you would like this enhancement.

@jgrandja I think this can be a separate issue. However, I agree that this would be a nice feature.

Correct me if I'm wrong @jzheaux but I think the idea would be that it presumes it is already converted to the type. No conversion is done when getClaim is invoked.

Agreed that it can be a separate issue. Yes, @rwinch this would simply do:

default <T> T getClaim(String claim) { return (T) this.getClaims().get(claim); }

I now understand what you are suggesting @jzheaux. As @rwinch mentioned

the idea would be that it presumes it is already converted to the type

I agree that this would be a nice improvement as long as the user is aware that the claim has already been converted otherwise a ClassCastException will occur.

I'll add a new ticket for this.

jzheaux · 2019-04-18T20:30:32Z

oauth2/oauth2-core/src/main/java/org/springframework/security/oauth2/core/ClaimAccessor.java

-		((List<?>) this.getClaims().get(claim)).forEach(e -> claimValues.add(e.toString()));
-		return claimValues;
+		Object claimValue = getClaims().get(claim);
+		List<String> convertedValue = ObjectConverters.listStringConverter().convert(claimValue);


I think Optional is just a bit more readable in situations like these:

return Optional.ofNullable(ObjectConverters.listStringConverter().convert(claimValue)) .orElseThrow(() -> new IllegalArgumentException...

I think this is a matter of preference. The current implementation is easily readable as well and it has the benefit of not creating an extra object (Optional) as part of the check.

I agree this is a matter of preference. Personally, I find Optional harder to read. For me this is especially the case when doing more than one thing on a single line. It is unfortunate, but this often happens because the way IDEs try to format the code with method chaining.

Note that using Optional throughout the code base adds quite a bit of object creation which means it can impact GC.

jzheaux · 2019-04-18T20:36:44Z

...e/src/test/java/org/springframework/security/oauth2/jwt/MappedJwtClaimSetConverterTests.java

 		Map<String, Object> source = new HashMap<>();
+		source.put(JwtClaimNames.EXP, exp);


Thanks for pointing this out. Did you mean the iat claim? This was to remain backward compatible with NimbusJwtDecoderJwkSupport, though there are perhaps other ways to achieve this.

jzheaux · 2019-04-18T20:37:41Z

...e/src/test/java/org/springframework/security/oauth2/jwt/MappedJwtClaimSetConverterTests.java

-	}
-
-	@Test
-	public void convertWhenConverterReturnsValueWhenEntryIsMissingThenEntryIsAdded() {


I'm not keen on removing these test until we come to an agreement on #6800. This PR isn't about changing how MappedJwtClaimSetConverter behaves.

jzheaux · 2019-04-18T20:50:34Z

docs/manual/src/docs/asciidoc/_includes/servlet/preface/java-configuration.adoc

@@ -819,56 +817,6 @@ JwtDecoder jwtDecoder() {
 ```
 This will keep all the defaults, except it will override the default claim converter for `sub`.

-[[oauth2resourceserver-claimsetmapping-add]]


It doesn't seem to me that this PR is about changing how MappedClaimSetConverter works. We should defer changing its documentation to maybe #6800.

rwinch · 2019-04-30T14:02:49Z

core/src/main/java/org/springframework/security/converter/ObjectConverters.java

+ * @since 5.2
+ * @see Converter
+ */
+public final class ObjectConverters {


Have we looked into using built in Spring types (i.e. ConversionService)? It seems we should probably try to use that over writing the conversion ourselves.

Yes, I did originally check Converter impl's in framework when I first wrote ClaimAccessor but did not find anything that met the needs. I will check once again to make sure.

@rwinch I checked again and the reason I did not reuse Converter(s) in framework is because most (if not all) Converter implementations in org.springframework.core.convert.support are package-private.

Also, I don't feel it's necessary to reuse ConversionService (or GenericConversionService) as it provides additional API's that I won't use so I'd like to keep things as simple as possible.

I worry about adding these converters as we will likely need to support more complex things like timestamps that already have support for conversion. What's more is this is rather general, so it means that as we need more types of conversions we need to support them directly vs relying on ConversionService. Perhaps we could still leverage ConversionService, but the API itself accept a Converter that was adapted from ConversionService

rwinch · 2019-04-30T14:04:17Z

core/src/main/java/org/springframework/security/converter/ObjectConverters.java

+	 *
+	 * @return an {@code Object} to {@code String} {@link Converter}
+	 */
+	public static Converter<Object, String> stringConverter() {


There is a bit of a tradeoff. Holding static values improves performance a bit as it won't need to be garbage collected. However, it cannot be garbage collected when there continues to be a reference to it, so that means there is additional memory overhead.

Fixes spring-projectsgh-6245

jgrandja · 2019-05-13T00:23:43Z

@rwinch @jzheaux I applied all changes and it's ready for next review. FYI, I force pushed as I had to remove code related to #6800 that should be excluded. @rwinch I also made use of ConversionService via the new ClaimConversionService and removed ObjectConverters. Thanks for persisting on that as this is a much better solution.

rwinch

I commented inline. I'm not sure we need changes, but am going to mark as request changes until we discuss the point.

rwinch · 2019-05-15T15:21:55Z

...gframework/security/oauth2/client/oidc/authentication/ReactiveOidcIdTokenDecoderFactory.java

@@ -61,17 +70,55 @@
 			put(MacAlgorithm.HS512, "HmacSHA512");
 		}
 	};
+	private static final Converter<Map<String, Object>, Map<String, Object>> DEFAULT_CLAIM_TYPE_CONVERTER =


Thinking out loud a bit, but this API doesn't account for the fact that conversion may be a blocking operation. For example, if we have an InputStream that needs converted to an Object (i.e. reading in JSON) that is blocking and the API does not return a reactive type.

I'm wondering if in practice this is a concern in this situation since we have likely already done all the IO (it probably is converting from a String not an InputStream). What do we think the likelihood is that we will need to do a blocking operation during conversion? Are there other reasons we might need to block during conversion?

@rwinch

I'm wondering if in practice this is a concern in this situation since we have likely already done all the IO

The source of claims passed into the Converter should have already been parsed from it's JSON representation. For the ID Token use case, Nimbus's JSON parser would have read/parsed all claims in the raw ID Token (including custom claims) into an instance of JWTClaimsSet. Even for cases where there is a custom claim and it's a JSON object, Nimbus would parse it into a Map.

Are there other reasons we might need to block during conversion?

At this point, I do not see a use case where a blocking operation could happen with the default NimbusReactiveJwtDecoder.

jzheaux · 2019-05-22T22:00:15Z

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


 		return new MappedJwtClaimSetConverter(claimNameToConverter);
 	}

+	private static Converter<Object, ?> getConverter(TypeDescriptor targetDescriptor) {
+		final TypeDescriptor sourceDescriptor = TypeDescriptor.valueOf(Object.class);
+		return source -> ClaimConversionService.getSharedInstance().convert(source, sourceDescriptor, targetDescriptor);


Since ClaimAccessor is an interface, its methods need to retrieve the instance each time.

However, could this class instead keep it as state (e.g. private final ConversionService conversionService = ClaimConversionService.getSharedInstance())? I believe it would make things more readable.

jzheaux · 2019-05-22T22:02:47Z

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


 		return new MappedJwtClaimSetConverter(claimNameToConverter);
 	}

+	private static Converter<Object, ?> getConverter(TypeDescriptor targetDescriptor) {
+		final TypeDescriptor sourceDescriptor = TypeDescriptor.valueOf(Object.class);


Could we move these TypeDescriptor declarations out to the class level, e.g. private static final TypeDescriptor OBJECT = TypeDescriptor.valueOf(Object.class)?

jgrandja · 2019-06-04T23:35:30Z

Thanks @jzheaux. I applied your feedback.

jgrandja requested review from jzheaux and rwinch April 18, 2019 19:39

jgrandja commented Apr 18, 2019

View reviewed changes

jzheaux requested changes Apr 18, 2019

View reviewed changes

rwinch requested changes Apr 30, 2019

View reviewed changes

Externalize coercion in ClaimAccessor

bc5c91d

Fixes spring-projectsgh-6245

jgrandja force-pushed the gh-6245-claim-accessor branch from f112b72 to bc5c91d Compare May 12, 2019 23:56

rwinch requested changes May 15, 2019

View reviewed changes

jgrandja mentioned this pull request Jun 3, 2019

Add generic getClaim() method in ClaimAccessor #6947

Closed

rwinch approved these changes Jun 4, 2019

View reviewed changes

jzheaux reviewed Jun 4, 2019

View reviewed changes

jzheaux approved these changes Jun 4, 2019

View reviewed changes

jgrandja added in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) status: duplicate A duplicate of another issue labels Jun 4, 2019

jgrandja added this to the 5.2.0.M3 milestone Jun 4, 2019

jgrandja closed this Jun 4, 2019

jgrandja deleted the gh-6245-claim-accessor branch June 4, 2019 23:35

		Map<String, Object> source = new HashMap<>();
		source.put(JwtClaimNames.EXP, exp);

		@@ -49,7 +46,7 @@
		*/
		default Boolean containsClaim(String claim) {

Externalize coercion in ClaimAccessor #6799

Externalize coercion in ClaimAccessor #6799

Conversation

jgrandja commented Apr 18, 2019

jgrandja Apr 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jzheaux left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rwinch May 7, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgrandja commented May 13, 2019

rwinch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgrandja commented Jun 4, 2019

jgrandja Apr 18, 2019 •

edited

Loading

rwinch May 7, 2019 •

edited

Loading