Adding role based scope issuing for exchange grant #13053
Conversation
CrowleyRajapakse
requested review from
tharindu1st,
AnuGayan,
chamilaadhi,
Arshardh,
dushaniw and
tgtshanika
as code owners
📝 WalkthroughWalkthroughThe pull request updates the token grant handling in the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant SystemScopesIssuer
participant TokenReqMsgCtx
Client->>SystemScopesIssuer: Submit token request
SystemScopesIssuer->>SystemScopesIssuer: Check grant type
alt TOKEN_EXCHANGE
SystemScopesIssuer->>SystemScopesIssuer: Call configureForJWTGrantOrExchangeGrant(true)
SystemScopesIssuer->>SystemScopesIssuer: Extract user attributes and determine roles
else SAML20_BEARER
SystemScopesIssuer->>SystemScopesIssuer: Process SAML assertions
else JWT
SystemScopesIssuer->>SystemScopesIssuer: Call configureForJWTGrantOrExchangeGrant(false)
end
SystemScopesIssuer->>SystemScopesIssuer: Parse subject token using getSignedJWTFromSubjectToken
SystemScopesIssuer-->>Client: Return scopes
Assessment against linked issues
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/issuers/SystemScopesIssuer.java (2)
569-570: Add clarifying Javadoc for the boolean parameter.Consider extending the method’s JavaDoc to clarify what the
isExchangeGrantparameter represents and how it affects token handling. This will improve maintainability for future contributors.
683-706: Consider consolidating with the existing getSignedJWT method.The new
getSignedJWTFromSubjectTokendiffers fromgetSignedJWTonly by looking at a different request parameter key. You could unify them by parameterizing the key to reduce duplicate code.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/issuers/SystemScopesIssuer.java(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: build-product (3, group3)
- GitHub Check: build-product (1, group1)
- GitHub Check: build-product (4, group4)
- GitHub Check: run-benchmark-test
- GitHub Check: build-product (2, group2)
- GitHub Check: build-carbon
🔇 Additional comments (3)
components/apimgt/org.wso2.carbon.apimgt.impl/src/main/java/org/wso2/carbon/apimgt/impl/issuers/SystemScopesIssuer.java (3)
340-347: Confirm the fallback for role claim absence.When the grant type is token exchange and
ROLE_CLAIMisn't set in the token request context, this block may leaveuserRolesas null or empty, potentially omitting role-based scope validation. Please verify if this behavior is intentional or if a fallback logic is required.
353-355: Looks good.The condition for the JWT bearer grant and the associated system property check appears correct and consistent with the existing logic.
576-580: Verify error handling when parsing fails.If the parse operation fails, the code logs the error but continues without a valid JWT. Confirm whether continuing the flow with a null
signedJWTis the desired behavior or if an exception should be thrown to halt processing.
Issue
Fixes wso2/api-manager#3548