Skip to content
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

Property "renew_token_without_revoking_existing" not being honored causing stuck threads #2630

Conversation

KD23243
Copy link
Contributor

@KD23243 KD23243 commented Nov 20, 2024

Proposed changes in this pull request

  • This PR addresses issues related to token renewal and token type handling to enhance the stability and configurability of the token issuance process. When the renew_without_revoking_existing flag is enabled and the binding type is set to "none," the implementation now skips checking for existing tokens. Instead, it generates a random UUID as the binding reference, which helps to prevent thread-stuck issues in high-concurrency scenarios.

  • A new approach is introduced for retrieving the token type from custom token issuers. The OauthTokenIssuer interface now includes a default method, getAccessTokenType(), which returns "Opaque" by default. In the JWTTokenIssuer class, this method is overridden to return "JWT." This allows the system to correctly identify the token type without requiring additional configurations, making the implementation more flexible and easier to extend.

Related Issues

Follow up actions

Update the migration documentation to reflect the changes in token type handling. Specifically, note that a previously used Deployment.toml configuration for specifying the token type is no longer required, and custom JWT issuers that do not extend the JWTTokenIssuer class must override the getter method to specify the token type explicitly. Similarly, if implementing the OauthTokenIssuer interface, the method must be overridden to ensure compatibility.

Copy link

codecov bot commented Nov 20, 2024

Codecov Report

Attention: Patch coverage is 53.33333% with 14 lines in your changes missing coverage. Please review.

Project coverage is 55.77%. Comparing base (1eb13c1) to head (704533b).
Report is 6 commits behind head on master.

Files with missing lines Patch % Lines
...dlers/grant/AbstractAuthorizationGrantHandler.java 55.55% 4 Missing and 8 partials ⚠️
...g/wso2/carbon/identity/oauth2/OAuth2Constants.java 0.00% 1 Missing ⚠️
...2/carbon/identity/oauth2/token/JWTTokenIssuer.java 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master    #2630      +/-   ##
============================================
+ Coverage     55.76%   55.77%   +0.01%     
+ Complexity     8239     8194      -45     
============================================
  Files           632      632              
  Lines         47478    47502      +24     
  Branches       8385     8392       +7     
============================================
+ Hits          26477    26496      +19     
  Misses        17211    17211              
- Partials       3790     3795       +5     
Flag Coverage Δ
unit 38.68% <53.33%> (+0.03%) ⬆️

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.


🚨 Try these New Features:

@KD23243 KD23243 force-pushed the renew_token_without_revoking_existing1 branch from 5f067e1 to eaba6be Compare November 21, 2024 04:49
@jenkins-is-staging
Copy link

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

@jenkins-is-staging
Copy link

PR builder completed
Link: https://github.com/wso2/product-is/actions/runs/11966760205
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/11966760205

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.

5 participants