-
Notifications
You must be signed in to change notification settings - Fork 6k
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
Remove Deprecated Usages of RemoteJWKSet #16296
base: main
Are you sure you want to change the base?
Conversation
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, @kwondh5217, for the PR! I've left some feedback inline.
private static final class SpringJWKSetCache implements JWKSetCache { | ||
private static final class SpringURLBasedJWKSource<C extends SecurityContext> implements JWKSource<C> { | ||
|
||
private final URLBasedJWKSetSource urlBasedJWKSetSource; |
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 we are doing our own implementation of JWKSource
, it should be possible to use RestOperations
and Cache
directly instead of implementing additional Nimbus interfaces.
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.
Hi @jzheaux, I have a couple of questions regarding the feedback:
-
Regarding "Since we are doing our own implementation of
JWKSource
, it should be possible to useRestOperations
andCache
directly instead of implementing additional Nimbus interfaces":Does this mean we should replace the custom
SpringJWKSetCache
implementation with direct usage of Spring'sCache
? -
I am also considering whether to streamline the code by either:
- Using
RestOperations
directly, or - Using
RestOperationsResourceRetriever
- Using
Below are the two possible approaches:
-
Direct
RestOperations
:private static final class SpringURLBasedJWKSource<C extends SecurityContext> implements JWKSource<C> { private final RestOperations restOperations; // Implementation logic uses restOperations directly }
-
Using
ResourceRetriever
:private static final class SpringURLBasedJWKSource<C extends SecurityContext> implements JWKSource<C> { private final ResourceRetriever resourceRetriever; // Implementation logic continues to leverage resourceRetriever }
Would it be better to reuse the already implemented RestOperationsResourceRetriever
, or to use RestOperations
directly within the class?
If we go with the direct RestOperations
approach, should the RestOperationsResourceRetriever
implementation be removed entirely?
Thank you for your feedback!
if (this.cache == null) { | ||
return new RemoteJWKSet<>(toURL(jwkSetUri), jwkSetRetriever); | ||
URLBasedJWKSetSource urlBasedJWKSetSource = new URLBasedJWKSetSource(toURL(jwkSetUri), jwkSetRetriever); | ||
if(this.cache == null) { |
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.
Let's update this so that the member variable is set to NoOpCache
. In this way, the null checking here and downstream is unnecessary.
if (this.jwkSetCache != null) { | ||
JWKSet jwkSet = this.jwkSetCache.get(); | ||
if (this.jwkSetCache.requiresRefresh() || jwkSet == null) { | ||
synchronized (this) { |
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.
Could you please replace this synchronized
statement with an ReentrantLock
. Otherwise this could cause thread pinning with virtual threads.
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.
@holzerch Thank you for the feedback!
I’ll replace the synchronized block with a ReentrantLock as suggested.
close gh-16251