Skip to content

Conversation

Thumimku
Copy link
Contributor

@Thumimku Thumimku commented Oct 3, 2025

Proposed changes in this pull request

Issue: wso2/product-is#25778

This pull request introduces enhancements and refactoring to the impersonated refresh token feature in the OAuth2 implementation. The main focus is on improving how impersonation requests are detected and handled, ensuring consistent propagation of impersonation information, and making the configuration for enabling impersonated refresh tokens more robust and clear.

Impersonation Feature Improvements:

  • Added a new utility method isImpersonatedRefreshTokenEnabled() in OAuth2Util to centralize and simplify checking whether impersonated refresh tokens are enabled, with support for a default value.

Impersonation Information Propagation:

  • Added propagateImpersonationInfo() in RefreshGrantHandler to detect and mark impersonation requests by inspecting extended attributes, ensuring impersonation context is set correctly during token generation. [1] [2]

Refactoring and Code Consistency:

  • Updated AbstractAuthorizationGrantHandler to use the new utility method for checking impersonated refresh token enablement, replacing direct property access for improved code clarity and maintainability. [1] [2]

Token Attribute Handling:

  • Ensured that extended attributes are properly set and propagated in DefaultRefreshTokenGrantProcessor, improving how custom token attributes are handled during token creation. [1] [2]
  • Modified JWTTokenIssuer to remove the impersonating actor from custom claims before issuing JWTs, preventing unintended exposure of impersonation details.

Todo
[x] - unit test
[ ] - integration test

Tested Flows

  • SSO impersonation
  • Refresh grant after SSO impersonation

Can found the flows in here

Copy link

@wso2-engineering wso2-engineering bot left a 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

⚠️ Warning: AI-Generated Review Comments

  • 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: 3
#### Log Improvement Suggestion No: 4
#### Log Improvement Suggestion No: 5
#### Log Improvement Suggestion No: 6
#### Log Improvement Suggestion No: 7

Copy link

codecov bot commented Oct 3, 2025

Codecov Report

❌ Patch coverage is 25.00000% with 24 lines in your changes missing coverage. Please review.
✅ Project coverage is 57.56%. Comparing base (57c1177) to head (4b0a6e1).
⚠️ Report is 11 commits behind head on master.

Files with missing lines Patch % Lines
...uth2/token/handlers/grant/RefreshGrantHandler.java 23.07% 8 Missing and 2 partials ⚠️
...enprocessor/DefaultRefreshTokenGrantProcessor.java 0.00% 7 Missing ⚠️
...dlers/grant/AbstractAuthorizationGrantHandler.java 0.00% 3 Missing ⚠️
...2/carbon/identity/oauth2/token/JWTTokenIssuer.java 33.33% 1 Missing and 1 partial ⚠️
...g/wso2/carbon/identity/oauth2/util/OAuth2Util.java 66.66% 1 Missing and 1 partial ⚠️

❌ Your patch status has failed because the patch coverage (25.00%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files
@@             Coverage Diff              @@
##             master    #2950      +/-   ##
============================================
- Coverage     58.21%   57.56%   -0.66%     
- Complexity     9057     9175     +118     
============================================
  Files           669      669              
  Lines         50211    51374    +1163     
  Branches      11430    11711     +281     
============================================
+ Hits          29231    29574     +343     
- Misses        16947    17673     +726     
- Partials       4033     4127      +94     
Flag Coverage Δ
unit 40.73% <25.00%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@mpmadhavig
Copy link
Contributor

LGTM

*
* @return True if impersonated refresh token is enabled.
*/
public static boolean isImpersonatedRefreshTokenEnabled() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be a app level config?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we are not introducing app level config with this PR, cause for app level config we have to change API, UI and integration tests and it would take a week to do it

@jenkins-is-staging
Copy link

PR builder started
Link: https://github.com/wso2/product-is/actions/runs/18279649320

@jenkins-is-staging
Copy link

PR builder completed
Link: https://github.com/wso2/product-is/actions/runs/18279649320
Status: failure

@Thumimku
Copy link
Contributor Author

Thumimku commented Oct 6, 2025

Tested scenarios

MyAccount Impersonation

Screen.Recording.2025-10-06.at.18.07.14.mov

App SSO impersonation

3.mp4

Refresh grant flow

2.mp4

@jenkins-is-staging
Copy link

PR builder started
Link: https://github.com/wso2/product-is/actions/runs/18289047668

@jenkins-is-staging
Copy link

PR builder completed
Link: https://github.com/wso2/product-is/actions/runs/18289047668
Status: cancelled

@jenkins-is-staging
Copy link

PR builder started
Link: https://github.com/wso2/product-is/actions/runs/18290073469

@jenkins-is-staging
Copy link

PR builder completed
Link: https://github.com/wso2/product-is/actions/runs/18290073469
Status: success

Copy link

@jenkins-is-staging jenkins-is-staging left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving the pull request based on the successful pr build https://github.com/wso2/product-is/actions/runs/18290073469

@Thumimku Thumimku merged commit bf7c47b into wso2-extensions:master Oct 6, 2025
3 of 4 checks passed
@Thumimku Thumimku mentioned this pull request Oct 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants