Skip to content

Improve authoritiesClaimName validation in JwtGrantedAuthoritiesConverter #17247

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

chanbinme
Copy link
Contributor

Summary

Use StringUtils.hasText() instead of null check in getAuthoritiesClaimName() to properly handle empty strings and whitespace-only strings.

Problem

The current null check (!= null) incorrectly treats empty strings ("") and whitespace-only strings (" ") as valid claim names. While setAuthoritiesClaimName() validates with Assert.hasText(), the field can be set through other means (reflection, constructors, etc.) that bypass this validation.

Changes

  • Replace != null check with StringUtils.hasText()
  • Add comprehensive test coverage for blank claim names

Testing

Added parameterized tests covering empty strings, whitespace strings, and null values using ReflectionTestUtils to simulate edge cases.

Impact

  • Fixes edge case bugs with blank claim names
  • Maintains full backward compatibility
  • Follows defensive programming principles
  • All existing tests pass

This is a straightforward bug fix that improves robustness without breaking changes.

…rter

Use StringUtils.hasText() instead of null check to properly handle empty strings and whitespace-only strings.

Signed-off-by: chanbinme <[email protected]>
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jun 14, 2025
Copy link
Contributor

@jzheaux jzheaux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR, @chanbinme! I've left my feedback inline.

@@ -106,7 +106,7 @@ public void setAuthoritiesClaimName(String authoritiesClaimName) {
}

private String getAuthoritiesClaimName(Jwt jwt) {
if (this.authoritiesClaimName != null) {
if (StringUtils.hasText(this.authoritiesClaimName)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of continuing to check for null, let's remove the null check by doing the following:

  1. Change the authoritiesClaimName to Collection<String> authoritiesClaimNames = WELL_KNOWN_AUTHORITIES_CLAIM_NAMES;
  2. Change setAuthoritiesClaimName to set this.authoritiesClaimNames to a single value
  3. Replace the for loop to use this.authoritiesClaimNames
  4. Remove the null check

In this way, authoritiesClaimNames can never contain an empty claim name.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @jzheaux,

I've implemented the refactoring as suggested, but I noticed an important behavioral difference that I wanted to discuss:

Original behavior:

  • When authoritiesClaimName is explicitly set → return it directly without JWT validation
  • When using default claim names → return the first claim name that exists in the JWT

Initial refactoring attempt:

  • All claim names go through JWT validation, which changes the behavior for explicitly set claim names

My solution:
To preserve the original behavior, I added an isExplicitlySet flag that tracks whether the claim name was explicitly configured. This ensures:

  • Explicitly set claim names are returned without JWT validation (original behavior)
  • Default claim names are validated against the JWT (original behavior)
  • Null checks are eliminated since authoritiesClaimNames is always initialized

Would you prefer this approach, or would you like to intentionally change the behavior to always validate claim names against the JWT? I wanted to confirm the intended behavior before finalizing the implementation.

The changes include:

  1. ✅ Changed authoritiesClaimName to Collection<String> authoritiesClaimNames
  2. ✅ Updated setAuthoritiesClaimName to use Collections.singletonList()
  3. ✅ Modified the for loop to use this.authoritiesClaimNames
  4. ✅ Removed null checks
  5. ➕ Added isExplicitlySet flag to preserve original behavior

Let me know your thoughts on this approach!

- Change authoritiesClaimName field to Collection<String> authoritiesClaimNames
- Add isExplicitlySet flag to preserve original behavior
- Remove null checks by ensuring authoritiesClaimNames is always initialized
- Maintain backward compatibility for explicit vs default claim name handling
- Delete unnecessary test code related to previous null-checking logic

Signed-off-by: chanbinme <[email protected]>
@chanbinme chanbinme force-pushed the improve-authorities-claim-name-check branch from b6b8aa0 to 39b5cf5 Compare June 18, 2025 15:27
@chanbinme
Copy link
Contributor Author

Hi @jzheaux,

Thank you so much for your helpful feedback!
I've incorporated your suggestions and pushed the changes.
When you have a moment, could you please take another look?
I've also left some comments on the inline feedback for further discussion.

Thanks again for your time and support!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: waiting-for-triage An issue we've not yet triaged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants