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

Improvement: DO-3034 support seamless handling of refresh tokens #345

Conversation

krzysztof-causalens
Copy link
Collaborator

@krzysztof-causalens krzysztof-causalens commented Jul 23, 2024

Motivation and Context

This PR introduces handling of refresh tokens to the framework core.

When any request fails with a 401 or 403 status code, we initiate a token refresh process.
If the current auth config supports token refresh, it can choose to use the previous expired token and attached dara_refresh_token cookie to produce a new session token.
If successful, the token is replaced in a new global store and the request is retried.
If unsuccessful, the original failed response is returned to the caller.

Implementation Description

Client-side

Session token has been moved out of a React context into a singleton global store. This gives us more control over refreshing tokens. For example, once a token refresh is already initiated, other attempts to retrieve the token will block and wait for the in-flight refresh to go through.

The above is achieved utilizing a simple store with a locking mechanism, where in-flight refreshes are represented by promises.

Auth internals like the useSessionToken hook have been updated to pull/subscribe to the store instead of the context.

Backend

The refresh-token endpoint is now included by default in dara core. The base config provides an implementation of a refresh_token method which raises a 400 - refresh not supported - error.

An auth config can be configured to support token refresh by:

  • implement the aforementioned method to sign a new token, reusing the previous session_id for continuity
  • add some mechanism to set-cookie with the dara_refresh_token used, e.g. via custom components_config and endpoints such as sso-callback for SSO auth

The above is utilized in the internal SSO auth package.

Any new dependencies Introduced

How Has This Been Tested?

Unit tests added
Manually tested with internal SSO auth implementation

PR Checklist:

  • I have implemented all requirements? (see JIRA, project documentation).
  • I am not affecting someone else's work, If I am, they are included as a reviewer.
  • I have added relevant tests (unit, integration or regression).
  • I have added comments to all the bits that are hard to follow.
  • I have added/updated Documentation.
  • I have updated the appropriate changelog with a line for my changes.

Screenshots (if appropriate):

@krzysztof-causalens krzysztof-causalens self-assigned this Jul 23, 2024
@krzysztof-causalens krzysztof-causalens changed the title WIP: DO-3034 support seamless handling of refresh tokens Improvement: DO-3034 support seamless handling of refresh tokens Oct 21, 2024
@krzysztof-causalens krzysztof-causalens marked this pull request as ready for review October 21, 2024 10:33
@krzysztof-causalens krzysztof-causalens requested review from sam-causalens and a team October 21, 2024 10:33
Copy link
Contributor

@patricia-causalens patricia-causalens left a comment

Choose a reason for hiding this comment

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

LGTM, just missing changelog entry

packages/dara-components/changelog.md Outdated Show resolved Hide resolved
Copy link
Contributor

@sam-causalens sam-causalens left a comment

Choose a reason for hiding this comment

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

Looks Good Krys, left a couple of comments to think about.

packages/dara-core/js/auth/auth-wrapper.tsx Outdated Show resolved Hide resolved
packages/dara-core/js/auth/auth-wrapper.tsx Outdated Show resolved Hide resolved
packages/dara-core/js/shared/global-state-store.tsx Outdated Show resolved Hide resolved
@krzysztof-causalens
Copy link
Collaborator Author

  • removed periodic token refresh, is not necessary with expected refresh token times and is redundant as we now gracefully handle failed requests by retrying them after refreshing
  • removed jwt-decode dependency as we no longer have to decode the token on the client side to check it's expiry date

@krzysztof-causalens
Copy link
Collaborator Author

I realized there was an issue where in-progress live WS connections would have stale ContextVar values, such as ID_TOKEN if one is used by an auth config.

ContextVars can't be updated from the outside, so I've added a new memory object stream into the WS handler object:

  • The WS handler logic now checks for pending token data updates in the receive stream and updates the context whenever one is found
  • The refresh endpoint sends a token data update into the send stream

Added a test to verify the above behaviour.

Tagging another alpha version - I'll test how that interacts with multi-tab tomorrow, I have a feeling we might run into issues when two tabs decide to refresh their tokens simultaneously

krzysztof-causalens and others added 3 commits October 21, 2024 18:36
…ad. Update GlobalState to wrap localStorage instead of in-memory for cross-tab syncing
@krzysztof-causalens
Copy link
Collaborator Author

Looking more into the cross-tab issues, we've decided to take a different approach.

  1. The client-side GlobalStore used to store the session token is instead a wrapper around localStorage. This means the values are shared cross-tab, and subscriptions use storage events so one tab performing a refresh will automatically notify all active tabs.
  2. To update live WS connections, the client sends a special token_update WS message for the backend to update its in-memory ContextVars.
  3. To prevent multiple tabs from performing concurrent token refreshes, the calls to actual token refresh logic are wrapped with a short-lived deduping async cache. Simultaneous attempts to refresh the same token will be deduped and resolve to the same value (due to underlying locks), and subsequent ones within a short window (5s) will also return the same cached value.

@krzysztof-causalens
Copy link
Collaborator Author

Tested the new alpha locally, seems to work well - both when trying to recreate multiple tabs refreshing at the same time, and token updates are immediately propagated cross-tab.

packages/dara-core/dara/core/auth/utils.py Outdated Show resolved Hide resolved
Copy link
Contributor

@sam-causalens sam-causalens left a comment

Choose a reason for hiding this comment

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

This LGTM, thanks for this Krys much appreciated.

@krzysztof-causalens krzysztof-causalens merged commit 9628b09 into master Oct 25, 2024
3 checks passed
@krzysztof-causalens krzysztof-causalens deleted the DO-3034-make-dara-use-refresh-tokens-in-a-sensible-way branch October 25, 2024 10:42
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