Implement the self delegation flow with azp claim inside the act/sub claim#54
Implement the self delegation flow with azp claim inside the act/sub claim#54Bin4yi wants to merge 4 commits intowso2-extensions:mainfrom
Conversation
|
|
...ain/java/org/wso2/carbon/identity/oauth2/grant/token/exchange/TokenExchangeGrantHandler.java
Outdated
Show resolved
Hide resolved
...ain/java/org/wso2/carbon/identity/oauth2/grant/token/exchange/TokenExchangeGrantHandler.java
Show resolved
Hide resolved
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 | ||
| #### Log Improvement Suggestion No: 2 |
- Refactor isSelfDelegationRequest() to accept pre-parsed JWTClaimsSet instead of parsing the subject token internally, and remove throws IdentityOAuth2Exception as it no longer performs operations that can throw - Refactor validateSubjectTokenForSelfDelegation() to accept pre-parsed SignedJWT and JWTClaimsSet parameters instead of parsing internally - Parse subject token once in validateGrant() and pass the result down to isSelfDelegationRequest() and validateSubjectTokenForSelfDelegation(), eliminating duplicate parsing of the same token
- Add @BeforeMethod to reset shared OAuth2AccessTokenReqDTO request parameters before each test, preventing state pollution from tests that set actor_token on the shared DTO - Add ACTOR_SUBJECT_ID constant for delegation test data - Add testValidateDelegationRequest() (disabled) for the delegation positive test case, pending handler fix for isImpersonationRequest() to check may_act claim presence - Add delegationNegativeTestData data provider with 5 negative cases covering missing mandatory claims, wrong issuer, and wrong audience for both subject and actor tokens - Add testValidateDelegationRequestNegativeTest() to verify delegation validation fails with IdentityOAuth2Exception for all 5 negative cases - Add getDelegationSubjectToken(), getDelegationActorToken(), getDelegationReqParams(), and prepareTokenUtilsForDelegation() helper methods following the same pattern as impersonation helpers
…release v1.1.17
…for next development iteration
|
Can you update the PR description with sample request, and sample JWT access token? |
...ain/java/org/wso2/carbon/identity/oauth2/grant/token/exchange/TokenExchangeGrantHandler.java
Show resolved
Hide resolved
...ain/java/org/wso2/carbon/identity/oauth2/grant/token/exchange/TokenExchangeGrantHandler.java
Show resolved
Hide resolved
| validateMandatoryClaims(claimsSet, subject); | ||
|
|
||
| // NOTE: We SKIP impersonator validation for self-delegation | ||
| // In self-delegation, there is no may_act claim because the application |
There was a problem hiding this comment.
where are we skipping this impersonator validation for self-delegation. Couldn't find the logic for that
There was a problem hiding this comment.
as we discussed we consider that there is no maay_act claim in delegation or self delegation. may_act is only in impersonation
| * @param tokReqMsgCtx OAuthTokenReqMessageContext | ||
| * @return true if the request is a self-delegation request, false otherwise. | ||
| */ | ||
| private boolean isSelfDelegationRequest(Map<String, String> requestParams, OAuthTokenReqMessageContext tokReqMsgCtx, JWTClaimsSet subjectClaimsSet) { |
| return false; | ||
| } | ||
|
|
||
| if (!requestParams.containsKey(TokenExchangeConstants.SUBJECT_TOKEN) || |
There was a problem hiding this comment.
this is already captured in line 366. Redundant line
| !requestParams.containsKey(TokenExchangeConstants.SUBJECT_TOKEN_TYPE)) { | ||
| return false; | ||
| } | ||
| if (requestParams.containsKey(TokenExchangeConstants.ACTOR_TOKEN)) { |
| } | ||
|
|
||
| // Check if the client_id claim matches the requesting client | ||
| Object clientIdClaim = subjectClaimsSet.getClaim(TokenExchangeConstants.CLIENT_ID); |
There was a problem hiding this comment.
azp is an optional claim. Hence we should be able to stick to client_id claim only. Do you have any reasons to fallback to azp and client_id?
Purpose
Support self-delegation in OAuth2 Token Exchange (token exchange performed by the same actor) and ensure the
azpclaim is included inside theactclaim.Description
This PR improves token-exchange handling for self-delegation and strengthens validation logic:
Self-delegation detection
isSelfDelegationRequest()to verify the token exchange request contains only a subject token, and that the subject token was issued to the same client.Avoid
actclaim duplicationactclaim creation during token exchange by checking whether an existingactclaim is already present.actclaim exists, preserve it instead of recreating/duplicating it.Improve subject token validation
validateSubjectTokenForSelfDelegation()to validate mandatory JWT claims.aud) list.issclaim matches the JWT token’s issuer.Add constants
AZPandCLIENT_IDinConstants.javafor consistent claim handling.Sample (sanitized) token exchange request
POST https://localhost:9443/oauth2/tokengrant_type:urn:ietf:params:oauth:grant-type:token-exchangesubject_token:<JWT_ACCESS_TOKEN>subject_token_type:urn:ietf:params:oauth:token-type:access_tokenscope:booking:readExpected (sanitized) response claims
act.suband ensuresact.azpis present underactactwhen already availableazpand client binding consistent for self-delegation flows