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

[SDK-3594] De-dupe Id token #967

Merged
merged 23 commits into from
Sep 6, 2022
Merged

[SDK-3594] De-dupe Id token #967

merged 23 commits into from
Sep 6, 2022

Conversation

frederikprijck
Copy link
Member

@frederikprijck frederikprijck commented Aug 25, 2022

Changes

Our SDK caches the token response it gets from Auth0. The combination of audience and scope is used to differentiate the cache entries. This results in the fact that we can have multiple entries in our cache, each containing an access token, id token and, optionally, a refresh token. Most importantly, we can have multiple id tokens, while it's expected to only ever have one per client.

With this PR we are changing the cache in such a way that we now store id token and decodedToken separately from the access token, unrelated to scope or audience. This ensures we only have one id token in our cache.

Change in public API
The consequence of that change is that our public API will change, both getUser and getIdTokenClaims methods will no longer accept any arguments.

Cache changes
This PR also reworks our internal cache in such a way that id_token and decodedToken are now stored separately from the rest, using a key in the format @@auth0spajs@@::CLIENT_ID::@@user@@.

Session Management
This PR also removes the check on the ID Token exp claim when retrieving items from the cache, as that was required due to changes in this PR. This also solves SDK-3587.

Backwards compatibility
Even though the public API has changed, and users will be required to update their code as needed, we do provide backwards compatibility in such a way that we try to have users not need to re-authenticate (when using local-storage or a custom cache) after updating to v2.

To do so, the implementation of the above methods (getUser, getIdTokenClaims and isAuthenticated) is changed as follows:

  • Try and return a user based on the id token that's stored separately from the access token
  • If nothing was found, fallback to retrieving the id token using the global scope and audience (those provided to new Auth0Client({...}))

Testing

This PR adds a new file called migration.js to verify a couple of scenario's between v1 and v2. This is not running on Circle CI yet, as it needs a couple of tenant-specific configurations. However, including this here already and will follow up on getting them to run on circle.

  • This change adds unit test coverage
  • This change adds integration test coverage
  • This change has been tested on the latest version of the platform/language

Checklist

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Aug 25, 2022

This pull request introduces 1 alert when merging 90f22d4 into b0541c8 - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Aug 25, 2022

This pull request introduces 1 alert when merging 1febd38 into b0541c8 - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Aug 26, 2022

This pull request introduces 2 alerts when merging 53e6999 into b0541c8 - view on LGTM.com

new alerts:

  • 2 for Unused variable, import, function or class

@frederikprijck frederikprijck marked this pull request as ready for review September 1, 2022 09:57
@frederikprijck frederikprijck requested a review from a team as a code owner September 1, 2022 09:57
@frederikprijck frederikprijck changed the title [DRAFT] [SDK-3594] De-dupe Id token [SDK-3594] De-dupe Id token Sep 1, 2022
Copy link
Contributor

@ewanharris ewanharris left a comment

Choose a reason for hiding this comment

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

Just a few minor comments

__tests__/Auth0Client/getUser.test.ts Outdated Show resolved Hide resolved
src/Auth0Client.ts Outdated Show resolved Hide resolved
src/cache/cache-manager.ts Outdated Show resolved Hide resolved
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.

2 participants