-
Notifications
You must be signed in to change notification settings - Fork 3.7k
[fix][broker] Fix MultiRolesTokenAuthorizationProvider error when subscription prefix doesn't match. #25121
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
Conversation
lhotari
left a comment
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.
LGTM
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.
Pull request overview
This PR fixes an authorization failure in MultiRolesTokenAuthorizationProvider that occurs when JWT tokens contain multiple roles and one role fails the subscription prefix check. Previously, a single role's prefix mismatch exception would cause the entire authorization to fail, even when another role had valid permissions.
Key Changes:
- Modified exception handling in the
authorizemethod to differentiate between single-role and multi-role scenarios - For multiple roles, exceptions are now converted to authorization failures (false), allowing
FutureUtil.waitForAnyto succeed if any role is authorized - For single roles, the original exception propagation behavior is preserved for backward compatibility
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| MultiRolesTokenAuthorizationProvider.java | Added conditional exception handling based on role count - single roles propagate exceptions while multiple roles convert exceptions to false |
| MultiRolesTokenAuthorizationProviderTest.java | Added comprehensive test cases covering multi-role scenarios with exceptions and single-role exception propagation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #25121 +/- ##
============================================
- Coverage 74.82% 74.47% -0.35%
+ Complexity 33836 33659 -177
============================================
Files 1899 1899
Lines 149656 149658 +2
Branches 17393 17394 +1
============================================
- Hits 111979 111457 -522
- Misses 28892 29329 +437
- Partials 8785 8872 +87
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
…scription prefix doesn't match. (#25121) Co-authored-by: druidliu <[email protected]>
…scription prefix doesn't match. (#25121) Co-authored-by: druidliu <[email protected]> (cherry picked from commit 133fe20)
…scription prefix doesn't match. (#25121) Co-authored-by: druidliu <[email protected]> (cherry picked from commit 133fe20)
Motivation
When using
MultiRolesTokenAuthorizationProviderwith multiple roles in a JWT token, if one of the roles fails the subscription prefix check inPulsarAuthorizationProvider#canConsumeAsync, it throws aPulsarServerExceptionwith the message "The subscription name needs to be prefixed by the authentication role".This exception propagates up and causes the entire authorization to fail, even if another role in the token has valid permissions. This is problematic in multi-role scenarios where:
["user-a", "user-b"])FutureUtil.waitForAnymechanism should return success as soon as any role is authorizedModifications
Modified
MultiRolesTokenAuthorizationProvider#authorizemethod to handle exceptions differently based on the number of roles:false(authorization failed). This allowsFutureUtil.waitForAnyto work correctly - if any role succeeds, the overall authorization succeeds; only if all roles fail (returnfalseor throw exceptions), the authorization fails.Verifying this change
This change added new test cases in
MultiRolesTokenAuthorizationProviderTest:testMultiRolesAuthzWithSubscriptionPrefixMismatchException: Tests multi-role scenarios where:truefalsetestSingleRoleAuthzWithSubscriptionPrefixMismatchException: Tests single-role scenario where:Does this pull request potentially affect one of the following parts:
If the box was checked, please highlight the changes
Documentation
docdoc-requireddoc-not-neededdoc-completeMatching PR in forked repository
PR in forked repository: dragonls#12