Conversation
![wso2-engineering[bot]](https://avatars.githubusercontent.com/in/1856828?s=60&v=4){kind=small}
| if (OAuthServerConfiguration.getInstance().isOAuthResponseJspPageAvailable() | ||
| && !OAuthConstants.ResponseModes.FORM_POST_JWT.equals(oauth2Params.getResponseMode())) { | ||
| String params = buildParams(authorizationResponseDTO.getSuccessResponseDTO().getFormPostBody(), | ||
| authenticatedIdPs, sessionStateValue); |
There was a problem hiding this comment.
Log Improvement Suggestion No: 1
| if (OAuthServerConfiguration.getInstance().isOAuthResponseJspPageAvailable() | |
| && !OAuthConstants.ResponseModes.FORM_POST_JWT.equals(oauth2Params.getResponseMode())) { | |
| String params = buildParams(authorizationResponseDTO.getSuccessResponseDTO().getFormPostBody(), | |
| authenticatedIdPs, sessionStateValue); | |
| if (OAuthServerConfiguration.getInstance().isOAuthResponseJspPageAvailable() | |
| && !OAuthConstants.ResponseModes.FORM_POST_JWT.equals(oauth2Params.getResponseMode())) { | |
| log.debug("OAuth response JSP page is available and response mode is not FORM_POST_JWT, building params for JSP response."); | |
| String params = buildParams(authorizationResponseDTO.getSuccessResponseDTO().getFormPostBody(), |
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 |
📝 WalkthroughWalkthroughThe pull request fixes JARM Form Post JWT functionality by modifying Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
components/org.wso2.carbon.identity.oauth.endpoint/src/test/java/org/wso2/carbon/identity/oauth/endpoint/util/AuthzUtilTest.java (1)
2855-2929: Optional: extract shared setup to a helper for these two tests.Both tests repeat the same reflection and common OAuth2/session DTO arrangement. Pulling shared setup into a private helper would reduce duplication and future edit risk.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/org.wso2.carbon.identity.oauth.endpoint/src/test/java/org/wso2/carbon/identity/oauth/endpoint/util/AuthzUtilTest.java` around lines 2855 - 2929, Extract the repeated reflection and DTO/mock setup used in testHandleFormPostResponseModeDoesNotUseJSPForFormPostJwt and testHandleFormPostResponseModeUsesJSPForFormPost into a private helper method (e.g., prepareHandleFormPostTest) that performs the AuthzUtil.class.getDeclaredMethod(... "handleFormPostResponseMode"), sets it accessible, constructs and returns the OAuth2Parameters (responseMode, redirectURI, scopes), SessionDataCacheEntry mock (with getoAuth2Parameters and getAuthenticatedIdPs), AuthorizationResponseDTO with formPostBody set, OIDCSessionState, and any common OAuthServerConfiguration static mocking stubs; update both tests to call this helper and use its returned objects before invoking handleFormPostResponseMode so duplication is removed while preserving per-test-specific mocks like servlet request/dispatcher behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@components/org.wso2.carbon.identity.oauth.endpoint/src/test/java/org/wso2/carbon/identity/oauth/endpoint/util/AuthzUtilTest.java`:
- Around line 2855-2929: Extract the repeated reflection and DTO/mock setup used
in testHandleFormPostResponseModeDoesNotUseJSPForFormPostJwt and
testHandleFormPostResponseModeUsesJSPForFormPost into a private helper method
(e.g., prepareHandleFormPostTest) that performs the
AuthzUtil.class.getDeclaredMethod(... "handleFormPostResponseMode"), sets it
accessible, constructs and returns the OAuth2Parameters (responseMode,
redirectURI, scopes), SessionDataCacheEntry mock (with getoAuth2Parameters and
getAuthenticatedIdPs), AuthorizationResponseDTO with formPostBody set,
OIDCSessionState, and any common OAuthServerConfiguration static mocking stubs;
update both tests to call this helper and use its returned objects before
invoking handleFormPostResponseMode so duplication is removed while preserving
per-test-specific mocks like servlet request/dispatcher behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: de57d903-1f2c-4dce-a665-15827a30ea4d
📒 Files selected for processing (2)
components/org.wso2.carbon.identity.oauth.endpoint/src/main/java/org/wso2/carbon/identity/oauth/endpoint/util/AuthzUtil.javacomponents/org.wso2.carbon.identity.oauth.endpoint/src/test/java/org/wso2/carbon/identity/oauth/endpoint/util/AuthzUtilTest.java
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3194 +/- ##
============================================
+ Coverage 57.27% 59.26% +1.99%
- Complexity 10310 10338 +28
============================================
Files 710 711 +1
Lines 59205 56715 -2490
Branches 14156 13740 -416
============================================
- Hits 33911 33614 -297
+ Misses 20787 18625 -2162
+ Partials 4507 4476 -31
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
PR builder started |
hwupathum
left a comment
hwupathum
left a comment
There was a problem hiding this comment.
Did we check negative cases as well?
I check all 4 cases JWT Form Post with consent is there anything else I have to check as negative cases |
|
PR builder completed |
jenkins-is-staging
left a comment
jenkins-is-staging
left a comment
There was a problem hiding this comment.
Approving the pull request based on the successful pr build https://github.com/wso2/product-is/actions/runs/24228699255
|
3 participants
Proposed changes in this pull request
Fixes wso2/product-is#27528
This pull request improves the handling of OAuth2 response modes, specifically ensuring that the
form_post.jwtresponse mode does not trigger forwarding to the JSP page, even when the JSP is available cause JSP page only handle form_post here its formpost.jwt so we have to skip JSP and give the response to responseMode Provider.. Additionally, new unit tests are added to verify the correct behavior for bothform_post.jwtand standardform_postresponse modes.Tested cases
Summary by CodeRabbit
Bug Fixes
Tests