Added support for providing a JWKSCache implementation #804
Conversation
|
|
|
Issues
======
+ Solved 1
See the complete overview on Codacy |
|
Is there any update on when this will potentially be implemented? Thanks |
| @Bean | ||
| @ConditionalOnMissingBean(JWKSetCache.class) | ||
| public JWKSetCache getJWKSetCache() { | ||
| return null; |
There was a problem hiding this comment.
This method will always return null. It makes no sense.
There was a problem hiding this comment.
Well with my understanding that @ConditionalOnMissingBean annotation makes it so this method will only get called when JWKSetCache is missing? So in that case just return null. So the check above will create the AADAuthenticationFilter without the cache object.
That's what the library currently does, it doesn't include that last argument, it has 2 constructers the old one without jwkSetCache and a new one with it.
But I do not understand where the cache is configured if that bean is present...
|
Hi @briancarneiro, I leave a comment for the method getJWKCache(). Could you or your team respond to the comment? Once it is resolved, the PR can be merged into the master. |
| @@ -60,7 +64,12 @@ public AADAuthenticationFilterAutoConfiguration(AADAuthenticationProperties aadA | |||
| @ConditionalOnExpression("${azure.activedirectory.session-stateless:false} == false") | |||
| public AADAuthenticationFilter azureADJwtTokenFilter() { | |||
| LOG.info("AzureADJwtTokenFilter Constructor."); | |||
| return new AADAuthenticationFilter(aadAuthProps, serviceEndpointsProps, getJWTResourceRetriever()); | |||
| if (getJWKSetCache() != null) { | |||
There was a problem hiding this comment.
This method will always return null. How can you get the specific JWKSetCache customized by yourself?
There was a problem hiding this comment.
I am also wondering about this. Trying to figure this part out
There was a problem hiding this comment.
I think with the below changes I suggested we might not need the null check. We would always add it as the 4th parameter. And we could default the lifespan to 5 minutes. That's what it gets defaulted to if you don't pass it in as per the documentation so that would be the same behavior unless you specify in the properties file
| @@ -80,6 +89,12 @@ public ResourceRetriever getJWTResourceRetriever() { | |||
| aadAuthProps.getJwtSizeLimit()); | |||
| } | |||
|
|
|||
| @Bean | |||
There was a problem hiding this comment.
| @Bean | |
| @Bean | |
| @ConditionalOnMissingBean(JWKSetCache.class) | |
| public JWKSetCache getJWKSetCache() { | |
| return new DefaultJWKSetCache(properties.lifespan, TimeUnit.MILLISECONDS) | |
| } |
There was a problem hiding this comment.
Would this be something like what we would want to do? We would then define the lifespan within a properties file
There was a problem hiding this comment.
Sound good! It is a cool way to customize the cache by properties. If users don't set related configuration, it will use 5 min by default.
|
Is there a way for me to edit/add to his pull request or do I need to create my own? |
|
Please see above new pull request with the comments we discussed @neuqlz #827 |
ok, now I close this PR. |
Summary
Added support for consumers to provide a JWKSetCache implementation to the UserPrincipalManager
Issue Type
Starter Names
Additional Information
Fix for #802