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

Changes applied into the code after CON_APP_KEY database constraint removed #2100

Open
wants to merge 8 commits into
base: feature-con-app-key-removal
Choose a base branch
from

Conversation

Davids-98
Copy link
Contributor

@Davids-98 Davids-98 commented Jun 16, 2023

Currently, when two consecutive token requests arrive, we follow different approaches for opaque tokens and JWT tokens.

Opaque tokens return an existing token if the previous token has not expired, while for JWT tokens, we revoke the existing token and issue a new one for each request. However, the current implementation of token issuing can lead to challenges and a less-than-ideal experience for developers.

While the issue may not be significant for opaque tokens alone, it becomes problematic even with opaque tokens when developers attempt to handle token expiry.

The situation becomes more challenging with JWT tokens, as when multiple instances request tokens, some of the instances might end up with revoked tokens after a whole.

For example, opening the same single-page application (that uses JWT tokens) in multiple tabs, one tab revokes the token in the other tab. This leads to a chain of token refreshes making the application unusable.

To address these challenges and enhance the developer experience, I have analyzed the code base and identified an improvement in the token-issuing logic. The approach is similar to what we do when enabling token hashing, giving new tokens for each token request. I used the same approach to address the above gap and give a new token for each request token.

While examining the token issuing code flow, I discovered a database constraint named 'CON_APP_KEY' in the "IDN_OAUTH2_ACCESS_TOKEN" table. This constraint enforces that only one 'Active' token is allowed for a specific combination of 'CONSUMER_KEY_ID, AUTHZ_USER, TENANT_ID, USER_DOMAIN, USER_TYPE, TOKEN_SCOPE_HASH, TOKEN_STATE_ID, IDP_ID, TOKEN_BINDING_REF' at any given time. Consequently, it is impossible to store two tokens with the same combinations due to violating the 'CON_APP_KEY' database constraint.

To overcome this limitation and ensure new tokens for each token request, I have made the change to remove the 'CON_APP_KEY' database constraint. This change requires relevant code modifications.

Changes;
I have gone through the code base and found what changes should I do with the new assumption after the CON_APP_KEY remove

*Check the token-issuing logic and remove the path that checks the existing token
*Check the refresh grant flow and add relevant changes to it
*Check the token revocation flow and add relevant changes to it.
*Go through the DAO layer and change relevant code scripts that were implemented with the previous assumptions
*Limiting the maximum active tokens per app, user combination

…con app key (remove removing latest token with scopes because now we have a new token for a each token request)
…con app key (remove removing latest token with scopes because now we have a new token for a each token request)
…con app key (remove removing latest token with scopes because now we have a new token for a each token request)
@CLAassistant
Copy link

CLAassistant commented Jun 16, 2023

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@Davids-98 Davids-98 force-pushed the con-app-key-removing branch from d11d9ea to da17112 Compare June 30, 2023 05:45
@Davids-98 Davids-98 marked this pull request as ready for review July 15, 2023 19:28
@Thumimku
Copy link
Contributor

Hi @Davids-98,

⚠️ Notice: This PR has been open for a while. To keep the repository clean and up-to-date, this PR will be closed within the next two weeks if there is no further activity.

Please take one of the following actions:

  • Merge the PR if it is ready.
  • Close the PR if it is no longer relevant.
  • Leave a comment explaining why it should remain open and provide an update on its progress.

Your prompt attention to this matter is greatly appreciated. Thank you for your understanding and collaboration! 🙏

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.

3 participants