Add NimbusJwtDecoder #5936

jzheaux · 2018-10-10T18:41:11Z

Introduces a JwtDecoder which takes a raw Nimbus JWTProcessor
configuration.

Fixes: gh-5648

jgrandja · 2018-10-15T13:10:05Z

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

This cannot be introduced in a patch version. See Semantic Versioning and Apache Versioning

rwinch

If we deprecate an API we should do our best to remove the usage within our code base. Can you find usage of NimbusJwtDecoderJwkSupport and replace it?
NimbusJwtDecoderJwkSupport was deprecated, but not all the functionality is replaced. For example, there is no easy way for users to leverage RestOperations with NimbusJwtDecoder. I think we should add simple builder methods that simplify creating a NimbusJwtDecoder from a jwkSetUrl and allows setting a RestOperations for usage.

jzheaux · 2018-10-31T19:01:13Z

@rwinch That sounds reasonable. Regarding the builder, there is a separate ticket for providing this capability in a separate class (not directly on NimbusJwtDecoder). Would it be better to add that builder functionality onto NimbusJwtDecoder itself and in this PR?

rwinch · 2018-11-02T18:32:01Z

@jzheaux Yes. Let's merge with this issue.

jzheaux · 2018-11-05T22:28:02Z

@rwinch I added the builder, which exposes rest operations functionality.

This should achieve parity with what NimbusJwtDecoderJwkSupport was providing.

rwinch · 2018-11-08T20:18:43Z

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

Let's rename this to be more precise. When we add support for building with a static key, we want to ensure that it isn't confusing that support isn't on this builder. Perhaps a name of JwkSetUriBuilder?

It is a bit surprising to me that the Builder only provides the properties to build the JWTProcessor for the constructor to NimbusJwtDecoder. After using the Builder to create NimbusJwtDecoder, I'm not sure I would think to look for properties on NimbusJwtDecoder (i.e. setClaimSetConverter) for additional customization.

I wonder if the Builder should either

Have all the properties on it

Build a JWTProcessor instead

Other options are welcome

I like the idea of introducing additional properties, and in fact, I would prefer that when constructing these myself. I prefer treating my objects as immutable after using a builder.

I think that building a JWTProcessor could work, but it also might encourage a very Nimbus-like API. While this is indeed a Nimbus-specific class, I'm not sure I want to position this as an authoritative way to configure Nimbus primitives.

rwinch · 2018-11-08T20:19:17Z

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

Let's move this method outside of the builder to the class level.

I think we should add validation on jwkSetUri. At minimum it should ensure that the String is not null or empty.

rwinch · 2018-11-08T20:36:45Z

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

I think if we have done things well in NimbusJwtDecoder then we should be able to delete more of this class. For example, I don't think we should need RestOperationsResourceRetriever. A way to minimize this would be to have a member of the Builder and a member of the NimbusJwtDecoder If a setter is invoked, it updates the builder and then builds a new delegate NimbusJwtDecoder

Agreed with @rwinch here as far as member variable for Builder that builds a new NimbusJwtDecoder when a setter is invoked. I also think all the code in the constructor should be deleted and simply use the NimbusJwtDecoder.Builder to build the delegate.

Also add @Deprecated at class level

jgrandja · 2018-11-09T20:30:19Z

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

This should return JwtDecoder?

Note, @jgrandja in the latest PR that the builder now returns JWTProcessor instead.

Though, I was debating the same thing as you at the time.

jgrandja · 2018-11-09T20:34:15Z

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

final is not really necessary here since the class is marked final

jgrandja · 2018-11-09T20:34:23Z

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

final is not really necessary here since the class is marked final

jgrandja · 2018-11-09T20:36:51Z

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

Is there a test if issuedAt == null?

Added https://github.com/spring-projects/spring-security/pull/5936/files#diff-bf492c57db9d7f365bc14fb30572ed16R102

jgrandja · 2018-11-09T20:40:00Z

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

This should be Assert.hasText

jgrandja · 2018-11-09T20:42:43Z

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

Why is the parameter type Object instead of String?

I can't remember now. It was needed in an early draft, but it appears I forgot to clean it up.

jgrandja · 2018-11-09T20:48:12Z

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

Agreed with @rwinch here as far as member variable for Builder that builds a new NimbusJwtDecoder when a setter is invoked. I also think all the code in the constructor should be deleted and simply use the NimbusJwtDecoder.Builder to build the delegate.

jgrandja · 2018-11-09T20:58:09Z

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

Also add @Deprecated at class level

Introduces a JwtDecoder which takes a raw Nimbus JWTProcessor configuration. Fixes: spring-projectsgh-5648

jzheaux · 2018-11-12T18:01:39Z

@rwinch, @jgrandja Thank you both for the valuable feedback. I've updated based on it.

A Builder to simply common construction patterns for NimbusJwtDecoder Issue: spring-projectsgh-6010

rwinch · 2018-11-14T21:54:02Z

Thanks for the PR @jzheaux! This is now merged into master

jzheaux added status: duplicate A duplicate of another issue type: enhancement A general enhancement in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) labels Oct 10, 2018

jgrandja reviewed Oct 15, 2018

View reviewed changes

jzheaux force-pushed the gh-5648 branch from b80c568 to 43f6615 Compare October 26, 2018 20:36

jzheaux requested a review from rwinch October 30, 2018 18:50

rwinch requested changes Oct 31, 2018

View reviewed changes

jzheaux force-pushed the gh-5648 branch from 43f6615 to 1ff55a7 Compare November 5, 2018 22:21

rwinch requested changes Nov 8, 2018

View reviewed changes

jgrandja reviewed Nov 9, 2018

View reviewed changes

jzheaux force-pushed the gh-5648 branch from 1ff55a7 to 6b46e62 Compare November 9, 2018 22:02

Low-level Nimbus Jwt Decoder

a74276e

Introduces a JwtDecoder which takes a raw Nimbus JWTProcessor configuration. Fixes: spring-projectsgh-5648

jzheaux force-pushed the gh-5648 branch from 6b46e62 to 1252a9d Compare November 12, 2018 17:53

jzheaux force-pushed the gh-5648 branch from 1252a9d to 2c6f8f4 Compare November 12, 2018 18:11

NimbusJwtDecoder Builder

6112403

A Builder to simply common construction patterns for NimbusJwtDecoder Issue: spring-projectsgh-6010

jzheaux force-pushed the gh-5648 branch from 2c6f8f4 to 6112403 Compare November 12, 2018 19:05

rwinch changed the title ~~Low-level Nimbus Jwt Decoder~~ Add NimbusJwtDecoder Nov 14, 2018

rwinch added this to the 5.2.0.M1 milestone Nov 14, 2018

rwinch self-assigned this Nov 14, 2018

rwinch merged commit d28e32b into spring-projects:master Nov 14, 2018

Add NimbusJwtDecoder #5936

Add NimbusJwtDecoder #5936

Uh oh!

Conversation

jzheaux commented Oct 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rwinch left a comment

Choose a reason for hiding this comment

Uh oh!

jzheaux commented Oct 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rwinch commented Nov 2, 2018

Uh oh!

jzheaux commented Nov 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!

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 Nov 12, 2018

Uh oh!

rwinch commented Nov 14, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jzheaux commented Oct 31, 2018 •

edited

Loading