-
Notifications
You must be signed in to change notification settings - Fork 6.1k
Add possibility to customize JwkSource of NimbusJwtDecoder #17046
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
d3f0b27
to
7da453e
Compare
Thanks for the PR, @marbon87! I'm curious if it would be easier to construct your own If so, this PR instead might add something similar to |
Hi @jzheaux, thanks for your feedback! If i unterstand the current implementation correctly, adding a separate builder-creation method like Furthermore i cannot use SpringJWKSource because it's private nor can i customize it because it's final. Overall I have to write a lot more code as a user of spring-security, if i want to increase the timeouts. |
Possibly, though I wonder if it is as much as you are thinking (perhaps it's also more than I'm thinking). As for the URI, I think that Nimbus has an API that's quite adept at this: AuthorizationServerMetadata metadata = AuthorizationServerMetadata
.resolve(new Issuer("https://example.org/issuer"));
String jwkSetUri = metadata.getJwkSetUri();
// ... It seems to me that this would result in the following: @Bean
JwtDecoder jwtDecoder(String issuer) {
AuthorizationServerMetadata metadata = AuthorizationServerMetadata
.resolve(new Issuer("https://example.org/issuer"));
String jwkSetUri = metadata.getJwkSetUri();
JWKSource<SecurityContext> source = JWKSourceBuilder.create(new URL(jwkSetUri))
// your caching settings
.build();
return NimbusJwtDecoder.withJwkSource(source)
// your decoding settings
.build();
} Alternatively, Spring Cache is also quite adept at timeouts and other caching functions if you are open to working with that instead. What complexities am I failing to consider? |
Your suggestion should work but i am not sure what's missing to get it "completed". Regarding to spring cache, i did not find an simple solutation for a refresh or/and fault tolerant ahead cache. |
Gotcha, I can see how the code gives that impression. It uses a custom If you want to look further into Spring Cache, I think Spring's JCache integration would make sense since you can integrate in this way with Hazelcast and other caches that support refresh-ahead. That said, you know your requirements best and whether the ease of Nimbus's in-memory caching will suit your needs.
Am I understanding correctly that you are asking for how the PR would change? To add |
Thanks for your offer, but i think i can manage that. Should i close this pr and create a new one referencing this? |
Up to you, @marbon87. Honestly, the title of the PR still seems to work well with what we are talking about now and I'd be fine with reusing it. |
Hi @jzheaux, i updated the pr. Please have a look. |
8b1a66c
to
afa71a6
Compare
Signed-off-by: Mark Bonnekessel <[email protected]>
PR spring-projectsgh-17046 Signed-off-by: Mark Bonnekessel <[email protected]>
- Since JWKSource supercedes the usage of RestOperations and Cache, this commit creates a new builder that only exposes the remaining configuration values PR spring-projectsgh-17046
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your patience, @marbon87! I realized once your PR was submitted that #17181 was needed first. With that completed, I've left some inline feedback for you.
In addition to that feedback, will you please help me with your commit message as well? It looks like there isn't a blank line between your title and your sign-off message which can mess with how certain tools render the commit.
Finally, some of these changes I'd normally do for the contributor, however this is trickier when the PR is from your main
branch, which I don't want to force push to. In the future, if you first cut a branch and then submit, it's a little easier for me to apply some of the needed changes.
* @param jwkSetUri the JWK Set uri to use | ||
* @return a {@link JwkSetUriJwtDecoderBuilder} for further configurations | ||
*/ | ||
public static JwkSetUriJwtDecoderBuilder withJwkSource(JWKSource<SecurityContext> jwkSetUri) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check the parameter name here. It's probably a left-over from the copy-paste.
* Use the given <a href="https://tools.ietf.org/html/rfc7517#section-5">JWK Set</a> | ||
* uri. | ||
* @param jwkSetUri the JWK Set uri to use | ||
* @return a {@link JwkSetUriJwtDecoderBuilder} for further configurations |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add @since 7.0
@@ -274,7 +284,7 @@ public static final class JwkSetUriJwtDecoderBuilder { | |||
private static final JOSEObjectTypeVerifier<SecurityContext> NO_TYPE_VERIFIER = (header, context) -> { | |||
}; | |||
|
|||
private final Function<RestOperations, String> jwkSetUri; | |||
private Function<RestOperations, String> jwkSetUri; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since providing a JWKSource
supercedes a RestOperations
and a Cache
, please create JwkSourceJwtDecoderBuilder
instead of repurposing JwkSetUriJwtDecoderBuilder
. In that way, people won't provide a JWKSource
and then also try and provide a RestOperations
or Cache
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that JwkSourceJwtDecoderBuilder
will not need the validateTypes
flag since that was added as part of migrating from 6.x to 7.0. Since this class will be added in 7.0, it doesn't need the same transition flag.
// gh-7056 | ||
@Test | ||
public void decodeWhenUsingJwkSource() throws Exception { | ||
JWKSource<SecurityContext> source = (a, b) -> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use Mockito instead of an inline stub. That will allow you to use verify
at the end of the test to confirm its usage
@@ -559,6 +560,22 @@ public void decodeWhenUsingSecretKeyWithKidThenStillUsesKey() throws Exception { | |||
// @formatter:on | |||
} | |||
|
|||
// gh-7056 | |||
@Test | |||
public void decodeWhenUsingJwkSource() throws Exception { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To follow the naming convention in this file, will you please follow this format for the method name:
{methodName}When{Condition}Then{Expectation}
For example:
withJwkSourceWhenDefaultsThenUsesProvidedJwkSource
@@ -559,6 +560,22 @@ public void decodeWhenUsingSecretKeyWithKidThenStillUsesKey() throws Exception { | |||
// @formatter:on | |||
} | |||
|
|||
// gh-7056 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like it might be a stray line from a copy-paste. Since this test wasn't added as part of #7056, let's please leave it off.
We want to increase the cache-ttl of the NimbusJwtDecoder respectively the underlying JWKSource. Furthermore we want to use the refresh-ahead caching of JWKSource.
The com.nimbusds.jose.jwk.source.JWKSourceBuilder provides a lot of features to customize how jwks are cached / update etc. but currently there is no way to customize it.
The implemented possiiblity to customiz the used JWKSourceBuilder in NimbusJwtDecoder allows to use all features from JWKSourceBuilder.