-
Notifications
You must be signed in to change notification settings - Fork 37
[Accelerator 4] Add FSJWTAccessTokenClaimProvider to set consent-id as an access token claim #888
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
...financial/services/accelerator/identity/extensions/claims/FSJWTAccessTokenClaimProvider.java
Outdated
Show resolved
Hide resolved
...financial/services/accelerator/identity/extensions/claims/FSJWTAccessTokenClaimProvider.java
Outdated
Show resolved
Hide resolved
...al/services/accelerator/identity/extensions/internal/IdentityExtensionsServiceComponent.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 | ||
| #### Log Improvement Suggestion No: 4 |
WalkthroughThis PR removes explicit claims-separation flags from three WSO2 IS deployment.toml files and adds a new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant OAuth as OAuth Token Endpoint
participant Provider as FSJWTAccessTokenClaimProvider
participant ScopeUtil as Scope/Claims Processor
participant Token as JWT Access Token
Client->>OAuth: Token request (with scopes)
OAuth->>Provider: getAdditionalClaims(TokenReqContext)
Provider->>ScopeUtil: extractConsentIdFromScopes(scopes)
ScopeUtil-->>Provider: consentId (optional)
Provider->>ScopeUtil: removeConsentIdScope(scopes)
ScopeUtil-->>Provider: filteredScopes
Provider->>Provider: build claims map (add consentId if present)
Provider-->>OAuth: additional claims + filteredScopes
OAuth->>Token: issue JWT with claims and filtered scopes
Token-->>Client: JWT Access Token
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@financial-services-accelerator/components/org.wso2.financial.services.accelerator.identity.extensions/src/main/java/org/wso2/financial/services/accelerator/identity/extensions/claims/FSJWTAccessTokenClaimProvider.java`:
- Around line 80-92: In addConsentIDClaim, change scope matching to use
startsWith instead of contains to avoid accidental matches: when finding the
FS_PREFIX use Arrays.stream(scopes).filter(scope ->
scope.startsWith(IdentityCommonConstants.FS_PREFIX)) and when finding
consentIdClaimName use .filter(scope -> scope.startsWith(consentIdClaimName));
keep the subsequent replace/remove logic (replace FS_PREFIX or
consentIdClaimName with StringUtils.EMPTY) and the existing fallback behavior
for empty values so the claim extraction remains unchanged except for stricter
prefix matching.
- Around line 56-65: The null-check order can cause NPEs when evaluating
context.getOauth2AccessTokenReqDTO().getClientId(); update the conditional in
FSJWTAccessTokenClaimProvider to first verify context != null, then
context.getOauth2AccessTokenReqDTO() != null, then optionally clientId != null,
and finally context.getScope() != null before calling
IdentityCommonUtils.isRegulatoryApp(...) and using context.getScope(); ensure
all uses of context.getScope() (including the log.debug call and calls to
addConsentIDClaim/removeConsentIdScope) are only executed after these null
guards.
🧹 Nitpick comments (1)
financial-services-accelerator/components/org.wso2.financial.services.accelerator.identity.extensions/src/main/java/org/wso2/financial/services/accelerator/identity/extensions/claims/FSJWTAccessTokenClaimProvider.java (1)
44-44: Static initialization ofconsentIdClaimNamemay cause issues.If the configuration is not yet loaded when this class is initialized, or if
getConfiguredConsentIdClaimName()returnsnull, subsequent usages at lines 87, 89, 95, and 98 will throwNullPointerException. Additionally, any runtime configuration changes won't be reflected.Consider fetching the claim name dynamically within methods or adding a null guard:
Suggested approach
- private static final String consentIdClaimName = IdentityCommonUtils.getConfiguredConsentIdClaimName(); + private static String getConsentIdClaimName() { + String claimName = IdentityCommonUtils.getConfiguredConsentIdClaimName(); + return claimName != null ? claimName : "consent_id"; + }Then replace usages of
consentIdClaimNamewithgetConsentIdClaimName().
Add FSJWTAccessTokenClaimProvider to put consent-id as an access token claim
Issue link: #887
Doc Issue: Optional, link issue from documentation repository
Applicable Labels: Spec, product, version, type (specify requested labels)
Development Checklist
Secure Development Checklist
Testing Checklist
Automation Test Details
Conformance Tests Details
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.