Support JWK Selection Strategy#4
Conversation
Closes spring-projectsgh-16170 Signed-off-by: douxiaofeng99 <18600127780@163.com>
Reviewer's Guide by SourceryThis pull request introduces a JWK selection strategy to the Updated class diagram for NimbusJwtEncoderclassDiagram
class NimbusJwtEncoder {
-JWKSource jwkSource
-Converter~List~JWK~, JWK~ jwkSelector
+NimbusJwtEncoder(JWKSource jwkSource)
+setJwkSelector(Converter~List~JWK~, JWK~ jwkSelector)
+Jwt encode(JwtEncoderParameters parameters)
-JWK selectJwk(JwsHeader headers)
-String serialize(JwsHeader headers, JwtClaimsSet claims, JWK jwk)
}
NimbusJwtEncoder -- JWKSource : has
NimbusJwtEncoder -- Converter : uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Qodo Merge was enabled for this repository. To continue using it, please link your Git account with your Qodo account here. PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
|
Qodo Merge was enabled for this repository. To continue using it, please link your Git account with your Qodo account here. PR Code Suggestions ✨Explore these optional code suggestions:
|
||||||||||||
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @GuusArts - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider using a functional interface instead of
Converter<List<JWK>, JWK>forjwkSelectorto improve readability. - The exception message in the default
jwkSelectorcould be improved to suggest configuring aJwkSelector.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| public void setJwkSelector(Converter<List<JWK>, JWK> jwkSelector) { | ||
| if(null!=jwkSelector) | ||
| this.jwkSelector = jwkSelector; | ||
| } |
There was a problem hiding this comment.
suggestion: Clarify handling of a null parameter in setJwkSelector.
Currently, passing a null value to setJwkSelector results in no change. It may be clearer to either explicitly disallow null (by throwing an exception) or document that null is a no-op to avoid unintentional mistakes by clients.
| public void setJwkSelector(Converter<List<JWK>, JWK> jwkSelector) { | |
| if(null!=jwkSelector) | |
| this.jwkSelector = jwkSelector; | |
| } | |
| public void setJwkSelector(Converter<List<JWK>, JWK> jwkSelector) { | |
| if(jwkSelector == null) { | |
| throw new IllegalArgumentException("jwkSelector cannot be null"); | |
| } | |
| this.jwkSelector = jwkSelector; | |
| } |
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| public void setJwkSelector(Converter<List<JWK>, JWK> jwkSelector) { | ||
| if(null!=jwkSelector) | ||
| this.jwkSelector = jwkSelector; | ||
| } |
There was a problem hiding this comment.
Suggestion: Improve code style consistency
| public void setJwkSelector(Converter<List<JWK>, JWK> jwkSelector) { | |
| if(null!=jwkSelector) | |
| this.jwkSelector = jwkSelector; | |
| } | |
| public void setJwkSelector(Converter<List<JWK>, JWK> jwkSelector) { | |
| if (jwkSelector != null) { | |
| this.jwkSelector = jwkSelector; | |
| } | |
| } |
2 participants
User description
Closes spring-projectsgh-16170
PR Type
Enhancement
Description
Introduced a customizable JWK selection strategy in
NimbusJwtEncoder.Added a new
setJwkSelectormethod to configure JWK selection logic.Replaced hardcoded JWK selection logic with a configurable converter.
Updated error handling for JWK selection to improve flexibility.
Changes walkthrough 📝
NimbusJwtEncoder.java
Introduced customizable JWK selection strategyoauth2/oauth2-jose/src/main/java/org/springframework/security/oauth2/jwt/NimbusJwtEncoder.java
Converter, JWK>for JWK selection.setJwkSelectormethod for custom JWK selection logic.