Fix role claim returned for applications with no role claim set - #1155
Fix role claim returned for applications with no role claim set#1155DilshanSenarath wants to merge 1 commit into
Conversation
📝 WalkthroughSummary
WalkthroughThe application model conversion now selects the role claim URI conditionally when an application’s role claim is unset and the local claim dialect is enabled. It reads Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
components/org.wso2.carbon.identity.api.server.application.management/org.wso2.carbon.identity.api.server.application.management.v1/src/main/java/org/wso2/carbon/identity/api/server/application/management/v1/core/functions/application/ServiceProviderToApiModel.java (1)
448-455: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd regression coverage for claim selection.
Cover unset, explicit
true, explicitfalse, and invalid configuration values so both response modes remain stable.🤖 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/org.wso2.carbon.identity.api.server.application.management/org.wso2.carbon.identity.api.server.application.management.v1/src/main/java/org/wso2/carbon/identity/api/server/application/management/v1/core/functions/application/ServiceProviderToApiModel.java` around lines 448 - 455, In getRoleClaimUriForUnsetRoleClaim, add regression tests covering a missing configuration value, explicit true, explicit false, and invalid values; assert each selects the expected claim URI and preserves both legacy and local-group response modes.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.
Inline comments:
In
`@components/org.wso2.carbon.identity.api.server.application.management/org.wso2.carbon.identity.api.server.application.management.v1/src/main/java/org/wso2/carbon/identity/api/server/application/management/v1/core/functions/application/ServiceProviderToApiModel.java`:
- Around line 448-455: Update getRoleClaimUriForUnsetRoleClaim so only a
non-null configuration value whose trimmed text explicitly equals "false"
selects IdentityUtil.getLocalGroupsClaimURI(). Preserve
FrameworkConstants.LOCAL_ROLE_CLAIM_URI for null, malformed, whitespace-padded,
or any other values, without using Boolean.parseBoolean.
---
Nitpick comments:
In
`@components/org.wso2.carbon.identity.api.server.application.management/org.wso2.carbon.identity.api.server.application.management.v1/src/main/java/org/wso2/carbon/identity/api/server/application/management/v1/core/functions/application/ServiceProviderToApiModel.java`:
- Around line 448-455: In getRoleClaimUriForUnsetRoleClaim, add regression tests
covering a missing configuration value, explicit true, explicit false, and
invalid values; assert each selects the expected claim URI and preserves both
legacy and local-group response modes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b41ad8cf-4420-4b46-996e-9fd1ff35f92e
📒 Files selected for processing (2)
components/org.wso2.carbon.identity.api.server.application.management/org.wso2.carbon.identity.api.server.application.management.v1/src/main/java/org/wso2/carbon/identity/api/server/application/management/v1/core/functions/application/ServiceProviderToApiModel.javapom.xml
| private String getRoleClaimUriForUnsetRoleClaim() { | ||
|
|
||
| String returnLegacyRoleClaim = IdentityUtil.getProperty(RETURN_LEGACY_ROLE_CLAIM_IN_APPLICATION_RESPONSE); | ||
| if (returnLegacyRoleClaim != null && !Boolean.parseBoolean(returnLegacyRoleClaim)) { |
There was a problem hiding this comment.
| if (returnLegacyRoleClaim != null && !Boolean.parseBoolean(returnLegacyRoleClaim)) { | |
| if (!Boolean.parseBoolean(returnLegacyRoleClaim)) { |
Purpose
For an application with no role claim set, the API returned the legacy role claim (
http://wso2.org/claims/role), while the authentication runtime resolveshttp://wso2.org/claims/roleswhen group and role separation is enabled. The API therefore returned a claim the runtime never emits, and echoing that value back on an update could fail validation because the legacy claim is filtered out of the local claim list.The returned claim is now selected by the new
ReturnLegacyRoleClaimInApplicationResponseconfig, which is enabled by default so existing behaviour is unchanged. Setting it tofalsereturns the runtime resolved claim:Also points
carbon.identity.framework.versionat7.11.169. Depends on wso2/carbon-identity-framework#8231, which must be released first.Related Issue
wso2/product-is#28185