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

Add proposed changes for token persistence removal #2208

Merged

Conversation

dushaniw
Copy link
Contributor

@dushaniw dushaniw commented Oct 20, 2023

Proposed changes in this pull request

Related wso2/api-manager#1664.
This PR is a continuation of following PRs for token persistence removal feature.

  1. Add extensions for token persistence removal feature #2091
  2. Add extension support for token validation during token introspection and user info endpoints #2176
  3. Add abstractions to indirect revocations due to internal user events #2179

PR #2091 and #2176 introduces new extensions point to plug a custom solution for non persistence feature. Basically it abstracts out the database persistence logic during token generation, refresh token handling, token revocation, token introspection and OIDC user info endpoint response handling. PR #2179 introduces abstractions to handle token revocation events due to user change events. This PR adds the following changes on top of above 3 PRs.

  1. Even if we have abstracted out token persistence during token generation, the OAuth2 Caching logic is still common for both cases. Hence even though tokens are not persisted in the database, the recent token is served from token cache, and it is notified as “revoked” to the gateway. Related to [Removal of Token Persistence] Handle cached behaviour of token revocations wso2/api-manager#2216. With this feature, the behavior of keeping tokens in the tokens cache with user, scope, client, IDP and binding reference as cache key during generation is no longer needed. Tokens handling during introspection happens with the token identifier as the cache key. Hence through a config, I have skipped checking cache and updating the token cache during token generation for cache keys consisting unique user, scope, client, IDP and binding reference.

I am utilizing the following configuration already available in identity.xml but not yet templated. There are no usages of the config other than in AccessTokenDAO (even still the enabling the config is not providing the intended behavior).

deployment.toml

[oauth.token_persistence]
enable=false

identity.xml

        <!-- Configs related to OAuth2 token persistence -->
        <TokenPersistence>
            <Enable>{{oauth.token_persistence.enable}}</Enable>
            <PoolSize>0</PoolSize>
            <RetryCount>5</RetryCount>
        </TokenPersistence>
  1. Introduced a new unique claim entity_id to the JWT access token to identify the principle subject of the token. Eg: User in case of Application_User access tokens, Client Application in case of Application tokens. This is needed to validate the tokens when no persistence is available for the JTI and if there are in direct token revocation events triggered due to client application or user change events (Eg: client secret regeneration, user password change etc).

  2. Along with this feature, to identify if the provided refresh token JWT, a new claim is introduced as is_consented. For that, in Refresh Grant flow, we need to pass this information through RefreshTokenValidationDO to the code where we generate the new token.

  3. Refactored the code in Add extension support for token validation during token introspection and user info endpoints #2176 Add extensions for token persistence removal feature #2091 as follows.

  • Renamed AccessTokenProvider -> TokenProvider
  • Added new method to TokenProvider to verify/validate refresh tokens. This was previously in TokenRevocationProcessor's getRevocableRefreshToken method. As we have introduce AccessTokenProvider (former) to verify access tokens, it make sense to move the refresh token handling to the same interface.
  • isRefreshToken in TokenRevocationProcessor is no longer needed, as we are not keeping the token type in the revoked JTI table with this feature. Refreshtokens can be handled in the same way as access tokens if token type hint is not provided.
  • Currently, when no active refresh token is found when token type hint is refresh_grant, we were querying directly in DB through .getAccessTokenDAO().getAccessToken. This is same as OAuth2Util.findAccessToken(), but latter go through the oauth cache.With the above refactoring, I have change the OAuth2Service.revokeTokenByOAuthClient to go through TokenProvider..getVerifiedAccessToken().

When should this PR be merged

Along with wso2/carbon-identity-framework#5073.

Follow up actions

N/A

Checklist (for reviewing)

General

  • Is this PR explained thoroughly? All code changes must be accounted for in the PR description.
  • Is the PR labeled correctly?

Functionality

  • Are all requirements met? Compare implemented functionality with the requirements specification.
  • Does the UI work as expected? There should be no Javascript errors in the console; all resources should load. There should be no unexpected errors. Deliberately try to break the feature to find out if there are corner cases that are not handled.

Code

  • Do you fully understand the introduced changes to the code? If not ask for clarification, it might uncover ways to solve a problem in a more elegant and efficient way.
  • Does the PR introduce any inefficient database requests? Use the debug server to check for duplicate requests.
  • Are all necessary strings marked for translation? All strings that are exposed to users via the UI must be marked for translation.

Tests

  • Are there sufficient test cases? Ensure that all components are tested individually; models, forms, and serializers should be tested in isolation even if a test for a view covers these components.
  • If this is a bug fix, are tests for the issue in place? There must be a test case for the bug to ensure the issue won’t regress. Make sure that the tests break without the new code to fix the issue.
  • If this is a new feature or a significant change to an existing feature? has the manual testing spreadsheet been updated with instructions for manual testing?

Security

  • Confirm this PR doesn't commit any keys, passwords, tokens, usernames, or other secrets.
  • Are all UI and API inputs run through forms or serializers?
  • Are all external inputs validated and sanitized appropriately?
  • Does all branching logic have a default case?
  • Does this solution handle outliers and edge cases gracefully?
  • Are all external communications secured and restricted to SSL?

Documentation

  • Are changes to the UI documented in the platform docs? If this PR introduces new platform site functionality or changes existing ones, the changes should be documented.
  • Are changes to the API documented in the API docs? If this PR introduces new API functionality or changes existing ones, the changes must be documented.
  • Are reusable components documented? If this PR introduces components that are relevant to other developers (for instance a mixin for a view or a generic form) they should be documented in the Wiki.

@dushaniw dushaniw marked this pull request as ready for review October 26, 2023 09:45
@jenkins-is-staging
Copy link

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

@jenkins-is-staging
Copy link

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

@jenkins-is-staging
Copy link

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

janakamarasena
janakamarasena previously approved these changes Oct 27, 2023
@jenkins-is-staging
Copy link

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

@dushaniw
Copy link
Contributor Author

dushaniw commented Oct 27, 2023

Ran integration tests for the previously failed test OAuth2ServiceJWTGrantTestCase locally and it passed with the lastest changes.

@janakamarasena janakamarasena merged commit 17d460d into wso2-extensions:master Oct 27, 2023
1 check passed
dushaniw added a commit to dushaniw/identity-inbound-auth-oauth that referenced this pull request Jan 30, 2024
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