Add versioning support for pre-issue access token action and extend grant types#3000
Conversation
…n-action-token-exchange
![wso2-engineering[bot]](https://avatars.githubusercontent.com/in/1856828?s=60&v=4){kind=small}
.../wso2/carbon/identity/oauth/action/execution/PreIssueAccessTokenActionVersioningHandler.java
Show resolved
Hide resolved
.../wso2/carbon/identity/oauth/action/execution/PreIssueAccessTokenActionVersioningHandler.java
Show resolved
Hide resolved
.../java/org/wso2/carbon/identity/oauth/action/execution/PreIssueAccessTokenRequestBuilder.java
Show resolved
Hide resolved
...ain/java/org/wso2/carbon/identity/oauth/action/versioning/ActionTriggerEvaluatorFactory.java
Show resolved
Hide resolved
...ain/java/org/wso2/carbon/identity/oauth/action/versioning/ActionTriggerEvaluatorFactory.java
Show resolved
Hide resolved
.../java/org/wso2/carbon/identity/oauth/action/versioning/ActionTriggerEvaluatorForVersion.java
Outdated
Show resolved
Hide resolved
.../java/org/wso2/carbon/identity/oauth/action/versioning/ActionTriggerEvaluatorForVersion.java
Outdated
Show resolved
Hide resolved
...g/wso2/carbon/identity/oauth/action/versioning/PreIssueAccessTokenRequestBuilderFactory.java
Show resolved
Hide resolved
...g/wso2/carbon/identity/oauth/action/versioning/PreIssueAccessTokenRequestBuilderFactory.java
Show resolved
Hide resolved
...org/wso2/carbon/identity/oauth/action/versioning/v1/PreIssueAccessTokenRequestBuilderV1.java
Show resolved
Hide resolved
...org/wso2/carbon/identity/oauth/action/versioning/v1/PreIssueAccessTokenRequestBuilderV1.java
Show resolved
Hide resolved
.../org/wso2/carbon/identity/oauth/action/versioning/v2/ActionTriggerEvaluatorForVersionV2.java
Outdated
Show resolved
Hide resolved
...org/wso2/carbon/identity/oauth/action/versioning/v2/PreIssueAccessTokenRequestBuilderV2.java
Show resolved
Hide resolved
...org/wso2/carbon/identity/oauth/action/versioning/v2/PreIssueAccessTokenRequestBuilderV2.java
Show resolved
Hide resolved
...ntity.oauth/src/main/java/org/wso2/carbon/identity/oauth/internal/OAuthServiceComponent.java
Show resolved
Hide resolved
...ntity.oauth/src/main/java/org/wso2/carbon/identity/oauth/internal/OAuthServiceComponent.java
Show resolved
Hide resolved
.../org/wso2/carbon/identity/oauth2/token/handlers/grant/AbstractAuthorizationGrantHandler.java
Outdated
Show resolved
Hide resolved
.../org/wso2/carbon/identity/oauth2/token/handlers/grant/AbstractAuthorizationGrantHandler.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.
…nRequestBuilder classes
| OAuthTokenReqMessageContext tokenMessageContext = | ||
| flowContext.getValue("tokenMessageContext", OAuthTokenReqMessageContext.class); | ||
| if (OAuthConstants.GrantTypes.TOKEN_EXCHANGE.equals( | ||
| tokenMessageContext.getOauth2AccessTokenReqDTO().getGrantType())) { |
There was a problem hiding this comment.
Handle possible NPE cases
|
|
||
| OAuthTokenReqMessageContext tokenMessageContext = | ||
| flowContext.getValue("tokenMessageContext", OAuthTokenReqMessageContext.class); | ||
| if (OAuthConstants.GrantTypes.TOKEN_EXCHANGE.equals( |
There was a problem hiding this comment.
Can you explain how does this logic allows token exchange grant type in action version 2.0 ?
There was a problem hiding this comment.
In v1 the evaluator checks the grant type and returns false for TOKEN_EXCHANGE, so the action is not triggered for token exchange. And in v2 this check is removed and isTriggerableForTokenExchangeGrant() always returns true, so the action is allowed for all grant types including token exchange
There was a problem hiding this comment.
Pull request overview
This pull request introduces a versioning mechanism for the "Pre Issue Access Token" action in the OAuth component, enabling support for multiple action versions (v1 and v2) and extending token exchange grant type support. The changes establish a factory pattern for version-specific request builders and evaluators, refactor the existing request builder to delegate to versioned implementations, and update grant type checking logic.
Key changes:
- Introduced versioning infrastructure with V1 and V2 action implementations supporting different grant type behaviors
- Extended pre-issue access token action support to include token exchange grant type
- Refactored request builder to use factory pattern for version-specific delegation
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
ActionVersioningHandlerFactory.java |
New factory class to obtain action versioning handlers by action type and version |
AbstractAuthorizationGrantHandler.java |
Added token exchange grant type to supported grant types and conditional audience setting |
OAuthServiceComponent.java |
Registered new PreIssueAccessTokenActionVersioningHandler as OSGi service |
PreIssueAccessTokenRequestBuilderV2.java |
New V2 request builder with enhanced audience handling for token exchange grant |
ActionTriggerEvaluatorForVersionV2.java |
V2 evaluator that allows token exchange grant triggering |
PreIssueAccessTokenRequestBuilderV1.java |
V1 request builder maintaining backward-compatible behavior |
PreIssueAccessTokenRequestBuilderFactory.java |
Factory to instantiate correct request builder based on action version |
ActionTriggerEvaluatorForVersion.java |
Base evaluator class blocking token exchange grant by default (V1 behavior) |
ActionTriggerEvaluatorFactory.java |
Factory to obtain version-specific trigger evaluators |
PreIssueAccessTokenRequestBuilder.java |
Refactored to delegate to versioned request builders via factory |
PreIssueAccessTokenActionVersioningHandler.java |
Version handler for pre-issue access token action using trigger evaluators |
PreIssueAccessTokenActionConstants.java |
Constants defining action version identifiers (v1, v2) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
...ava/org/wso2/carbon/identity/oauth2/token/handlers/grant/ActionVersioningHandlerFactory.java
Show resolved
Hide resolved
| public ActionVersioningHandler getVersionTriggerEvaluator(ActionType actionType, String actionVersion) { | ||
|
|
||
| // Delegate to the main OAuth ActionVersioningHandlerFactory to get the correct version handler. | ||
| return ActionVersioningHandlerFactory.getInstance().getVersionTriggerEvaluator(actionType, actionVersion); |
There was a problem hiding this comment.
This line creates infinite recursion: the factory calls its own getInstance() method and then calls a method that doesn't exist in this class. The intention appears to be delegating to a different factory class (possibly from the action.versioning package), but this will cause a stack overflow or compilation error.
| return ActionVersioningHandlerFactory.getInstance().getVersionTriggerEvaluator(actionType, actionVersion); | |
| return org.wso2.carbon.identity.action.versioning.ActionVersioningHandlerFactory | |
| .getInstance().getVersionTriggerEvaluator(actionType, actionVersion); |
| boolean isAuthorizedForUser = isAccessTokenAuthorizedForUser(tokenReqDTO.getGrantType(), tokenMessageContext); | ||
| if (isAuthorizedForUser) { | ||
| setUserForEventBuilder(eventBuilder, authorizedUser, tokenReqDTO.getClientId(), tokenReqDTO.getGrantType()); | ||
| eventBuilder.userStore(new UserStore(authorizedUser.getUserStoreDomain())); |
There was a problem hiding this comment.
Setting user store without checking if authorizedUser or authorizedUser.getUserStoreDomain() is null. In V2 (line 129-131 of PreIssueAccessTokenRequestBuilderV2), there's a null check for the user store domain. V1 should have the same null safety check to prevent potential NullPointerException.
| eventBuilder.userStore(new UserStore(authorizedUser.getUserStoreDomain())); | |
| if (authorizedUser != null && StringUtils.isNotBlank(authorizedUser.getUserStoreDomain())) { | |
| eventBuilder.userStore(new UserStore(authorizedUser.getUserStoreDomain())); | |
| } |
| if (tokenMessageContext.isPreIssueAccessTokenActionsExecuted()) { | ||
| return tokenMessageContext.getAudiences(); | ||
| } else { | ||
| return OAuth2Util.getOIDCAudience(oAuthAppDO.getOauthConsumerKey(), oAuthAppDO); | ||
| } |
There was a problem hiding this comment.
V1 uses isPreIssueAccessTokenActionsExecuted() to determine audience source, while V2 (lines 312-317 of PreIssueAccessTokenRequestBuilderV2) uses CollectionUtils.isNotEmpty() to prioritize context audiences. This inconsistency creates different behaviors between versions. Consider documenting why these versions have different audience resolution strategies, or ensure both use the same logic if the behavior should be consistent.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #3000 +/- ##
============================================
- Coverage 58.77% 58.66% -0.11%
+ Complexity 9933 9892 -41
============================================
Files 701 709 +8
Lines 54577 54924 +347
Branches 12697 12735 +38
============================================
+ Hits 32078 32223 +145
- Misses 18256 18430 +174
- Partials 4243 4271 +28
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…introduce base test class for PreIssueAccessTokenRequestBuilder
Lashen1227
changed the title
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
Lashen1227
changed the title
…pport in access token processing
This pull request introduces a versioning mechanism for the "Pre Issue Access Token" action in the OAuth component, enabling support for multiple action versions and their corresponding execution logic. It adds new factories and handlers to determine the correct version-specific behavior.
Key changes include:
Versioning infrastructure for Pre Issue Access Token actions:
PreIssueAccessTokenActionConstantsto define supported action versions (v1,v2).PreIssueAccessTokenActionVersioningHandler, which determines if the pre-issue access token action can be executed based on version-specific logic.ActionTriggerEvaluatorFactoryandActionTriggerEvaluatorForVersionto provide version-specific evaluators for action triggering, supporting both V1 and V2 behaviors.PreIssueAccessTokenRequestBuilderFactoryto instantiate the correct request builder based on action version.token_exchange,device_code,organization_switch,jwt_bearer&saml2_bearer.Related PRs: