search

soofstad / react-oauth2-pkce

Provider agnostic OAuth2 Authorization Code flow with PKCE for React
MIT License
125 stars 53 forks source link

Refactor Token Management: Introducing getTokenSilently for Multi-Tab Consistency and Enhanced Authentication Reliability #201

Closed kleysonfiretail closed 1 week ago

kleysonfiretail commented 1 week ago

Why is this pull request needed?

Limitations of the Current Approach

The existing token management strategy, which relies on periodic checks via setInterval and application context state, introduces critical issues, especially in multi-tab environments. Without a unified storage mechanism, each tab may store and update different token values, leading to conflicting states and unreliable authentication.

Additionally, this approach does not effectively support refresh token rotation. With rotation enabled, each token refresh generates a unique new token, which can lead to errors when multiple tabs hold outdated or mismatched tokens. Although using static refresh tokens reduces the chance of errors by sharing the same token across tabs, it is less secure, as a single compromised token could grant unauthorized access.

Solution: getTokenSilently

To solve these issues, a new approach using getTokenSilently was introduced. This method ensures only one token refresh occurs at a time across all tabs, preventing conflicts by using a mutex to control access and check token expiration before refreshing. By synchronizing token updates, getTokenSilently effectively supports refresh token rotation, keeping authentication states consistent and reducing errors across tabs.

What does this pull request change?

  1. Prevent state conflicts with multiple tabs: The removal of setInterval and introduction of a mutex-based strategy ensures that token refreshing does not conflict between tabs.
  2. Improved token retrieval strategy: The new getTokenSilently function ensures consistent access to the token and handles expiration more reliably, reducing errors.
  3. Unified state management: Combining logoutInProgress and loginInProgress into isLoading simplifies the logic and usage within the application.
  4. Better authentication state tracking: Introducing isAuthenticated provides a clear and direct way to check if the user is authenticated.
  5. Consistent state storage: Refactoring useBrowserStorage to use only browser storage eliminates state synchronization issues during token refreshes.
  6. Manual event dispatch: Ensures that storage events are handled correctly within the current tab, maintaining consistency.
  7. Flexible storage change handling: Adding an onChange callback offers more flexibility in responding to storage changes.
  8. Simplified error handling: Removing handleExpiredRefreshToken shifts error handling to the application, providing more control to developers.
  9. Post-logout actions: Adding postLogout callback enhances the customization of the logout process.
  10. Streamlined context: Removing the token and loginInProgress from the context reduces redundancy and encourages the use of the new mechanisms.
  11. Preventing batch requests: Using async-mutex ensures that only one refresh token request is processed at a time, avoiding potential rate limiting issues.

Moving Forward

I understand that not all features in this pull request may be merged as-is, but moving in this direction could significantly improve the system’s reliability and address the issues pointed out, especially around multi-tab environments and token rotation.

Please note that this is a breaking change, which may require adjustments in other parts of the application. I’m open to discussing any aspect of this proposal to ensure we find the best solution for stable, secure, and consistent token management.

sebastianvitterso commented 1 week ago

Hey @kleysonfiretail ! Thanks for contributing to the library!

I'm still reviewing the contents of the PR, so I have some preliminary comments/questions. When it comes to whether this is a direction we want to go with the library, I'll have to discuss with @soofstad.

I see that you list 11 separate changes you've made with this PR, and I'm wondering if it would be too much to ask to make 11 separate PR's from main?

The problem with a PR like this is the sheer volume of change, and even if we wish to take in some of the changes, we might not want to take in all of them, and in such a case, accepting the PR may be troublesome.

Another comment, that I'm sure @soofstad will make as well, is that this PR actually adds a dependency to "async-mutex": ">=0.5.0", while the library strives to be dependency-free.

I see in types.ts that in addition to adding some keys to IAuthContext (which is fine), the PR also removes loginInProgress. This is a breaking change, and we don't want any of those if we can avoid them. Any way to keep the loginInProgress property? I know that we use that in our application to support some messages in the user's UI.

If we introduce this breaking change, we'll have to bump the major version and library users will have to change their code to continue using the latest version of the library. Not something we take lightly.

I also see that both our linter and our unit testing library complain about the code. The code will need to go through our pre-commit hooks and the unit tests need to pass for us to accept the code.

Again, I realize this may be overwhelmingly negative, but that's just to ensure that we deliver quality. Thanks for contributing, and we'll get back with some more topical feedback for you!

soofstad commented 1 week ago

Totally support @sebastianvitterso. Thanks for the effort and interest @kleysonfiretail. Lets discuss these changes in separate PR's/Issues :)

soofstad commented 1 week ago

Hi @kleysonfiretail Hope you haven't lost interest in helping to improve the library. Could you please elaborate on:

each tab may store and update different token values, leading to conflicting states and unreliable authentication.

Have you been testing with the latest version, and if so, how can we replicate any issues you have been having?