Add audience handling in token request message context#47
Add audience handling in token request message context#47Lashen1227 wants to merge 2 commits intowso2-extensions:mainfrom
Conversation
...ain/java/org/wso2/carbon/identity/oauth2/grant/token/exchange/TokenExchangeGrantHandler.java
Show resolved
Hide resolved
...ain/java/org/wso2/carbon/identity/oauth2/grant/token/exchange/TokenExchangeGrantHandler.java
Show resolved
Hide resolved
There was a problem hiding this comment.
AI Agent Log Improvement Checklist
- The log-related comments and suggestions in this review were generated by an AI tool to assist with identifying potential improvements. Purpose of reviewing the code for log improvements is to improve the troubleshooting capabilities of our products.
- Please make sure to manually review and validate all suggestions before applying any changes. Not every code suggestion would make sense or add value to our purpose. Therefore, you have the freedom to decide which of the suggestions are helpful.
✅ Before merging this pull request:
- Review all AI-generated comments for accuracy and relevance.
- Complete and verify the table below. We need your feedback to measure the accuracy of these suggestions and the value they add. If you are rejecting a certain code suggestion, please mention the reason briefly in the suggestion for us to capture it.
| Comment | Accepted (Y/N) | Reason |
|---|---|---|
| #### Log Improvement Suggestion No: 1 | ||
| #### Log Improvement Suggestion No: 2 |
There was a problem hiding this comment.
Pull request overview
This pull request adds logic to parse and store audience values in the token request message context during token exchange operations. The implementation parses comma-separated audience values from the request parameter and sets them on the token message context for downstream processing.
Key changes:
- Adds audience parsing and storage logic in the
handleJWTSubjectTokenmethod after audience validation - Splits comma-separated audience values and trims whitespace before storing them in the message context
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (requestedAudience != null && !requestedAudience.isEmpty()) { | ||
| List<String> audienceList = Arrays.stream(requestedAudience.split(",")) | ||
| .map(String::trim) | ||
| .collect(Collectors.toList()); | ||
| tokReqMsgCtx.setAudiences(audienceList); |
There was a problem hiding this comment.
The PR description states "store the audience values from the JWT subject token", but the implementation stores the requestedAudience parameter from the HTTP request (line 134) rather than the audiences extracted from the JWT claims (line 666). If the intent is to store JWT audiences, the code should use the 'audiences' variable (claimsSet.getAudience()) instead of 'requestedAudience'. If the intent is to store the request parameter audience, the PR description should be updated to reflect this.
| if (requestedAudience != null && !requestedAudience.isEmpty()) { | ||
| List<String> audienceList = Arrays.stream(requestedAudience.split(",")) | ||
| .map(String::trim) | ||
| .collect(Collectors.toList()); | ||
| tokReqMsgCtx.setAudiences(audienceList); | ||
| } |
There was a problem hiding this comment.
The new audience handling logic lacks test coverage. The test file TokenExchangeGrantHandlerTest.java has comprehensive tests for other functionality in this handler, but there are no tests verifying that tokReqMsgCtx.setAudiences() is called correctly with the parsed audience list. Consider adding tests to verify this new behavior, including edge cases like comma-separated audiences, audiences with whitespace, and the scenario where requestedAudience is null or empty.
| // set the audiences in the token request message context | ||
| if (requestedAudience != null && !requestedAudience.isEmpty()) { | ||
| List<String> audienceList = Arrays.stream(requestedAudience.split(",")) | ||
| .map(String::trim) |
There was a problem hiding this comment.
The string splitting logic doesn't filter out empty strings after trimming. If the requestedAudience contains consecutive commas (e.g., "aud1,,aud2") or commas with only whitespace between them (e.g., "aud1, , aud2"), the resulting list will contain empty strings. This could lead to invalid audience values being set in the token request message context. Consider adding a filter to remove empty strings after trimming.
| .map(String::trim) | |
| .map(String::trim) | |
| .filter(StringUtils::isNotEmpty) |
This pull request adds logic to handle and store the audience values from the JWT subject token in the token request message context. This ensures that audience values provided in the request are properly parsed and set for downstream processing.
Related PRs: