Add configuration for SaaS app role return in ID token#8181
Conversation
📝 WalkthroughWalkthroughA new server configuration flag ChangesSaaS App Roles in ID Token Gate
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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/authentication-framework/org.wso2.carbon.identity.application.authentication.framework/src/main/java/org/wso2/carbon/identity/application/authentication/framework/handler/claims/impl/DefaultClaimHandler.java (1)
433-438: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a guarded DEBUG log for the cross-tenant role-skip branch.
This new branch makes a meaningful business decision silently; a guarded debug log will improve troubleshooting without adding noise.
♻️ Proposed change
if (!StringUtils.equals(serviceProvider.getTenantDomain(), authenticatedUser.getTenantDomain()) && !Boolean.parseBoolean(IdentityUtil.getProperty(RETURN_ROLES_IN_SAAS_APPS_IN_ID_TOKEN))) { + if (log.isDebugEnabled()) { + log.debug("Skipping app-associated role resolution for cross-tenant user since " + + RETURN_ROLES_IN_SAAS_APPS_IN_ID_TOKEN + " is disabled."); + } return new ArrayList<>(); }As per path instructions, add logs around key decision points and guard DEBUG logs with
if (log.isDebugEnabled()).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/authentication-framework/org.wso2.carbon.identity.application.authentication.framework/src/main/java/org/wso2/carbon/identity/application/authentication/framework/handler/claims/impl/DefaultClaimHandler.java` around lines 433 - 438, The cross-tenant role-skip branch in DefaultClaimHandler currently returns an empty role list without any trace, so add a guarded DEBUG log before the return that explains the application and authenticated user tenant domains and that role claims are being skipped because ReturnRolesInSaaSAppsInIDToken is disabled. Use the existing log pattern in DefaultClaimHandler and wrap the message with if (log.isDebugEnabled()) so the new decision point is observable without adding runtime noise.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@components/authentication-framework/org.wso2.carbon.identity.application.authentication.framework/src/main/java/org/wso2/carbon/identity/application/authentication/framework/handler/claims/impl/DefaultClaimHandler.java`:
- Around line 433-438: The cross-tenant role-skip branch in DefaultClaimHandler
currently returns an empty role list without any trace, so add a guarded DEBUG
log before the return that explains the application and authenticated user
tenant domains and that role claims are being skipped because
ReturnRolesInSaaSAppsInIDToken is disabled. Use the existing log pattern in
DefaultClaimHandler and wrap the message with if (log.isDebugEnabled()) so the
new decision point is observable without adding runtime noise.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 8902f18a-af6e-45a1-a57c-3217e30cd823
📒 Files selected for processing (5)
components/authentication-framework/org.wso2.carbon.identity.application.authentication.framework/src/main/java/org/wso2/carbon/identity/application/authentication/framework/handler/claims/impl/DefaultClaimHandler.javacomponents/authentication-framework/org.wso2.carbon.identity.application.authentication.framework/src/main/java/org/wso2/carbon/identity/application/authentication/framework/util/FrameworkConstants.javafeatures/identity-core/org.wso2.carbon.identity.core.server.feature/resources/identity.xmlfeatures/identity-core/org.wso2.carbon.identity.core.server.feature/resources/identity.xml.j2features/identity-core/org.wso2.carbon.identity.core.server.feature/resources/org.wso2.carbon.identity.core.server.feature.default.json
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (0.00%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## master #8181 +/- ##
============================================
+ Coverage 52.93% 52.97% +0.03%
- Complexity 20987 20994 +7
============================================
Files 2197 2197
Lines 129206 129323 +117
Branches 19237 19265 +28
============================================
+ Hits 68398 68504 +106
- Misses 52469 52473 +4
- Partials 8339 8346 +7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Integration test run - https://github.com/SujanSanjula96/product-is/actions/runs/28081082974 |
|
2 participants
Proposed changes in this pull request
Add configuration for the change #8167