Skip to content

Automatic refresh: Do only once if application open in multiple tabs #1148

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

Open
CedricHg opened this issue Oct 20, 2021 · 7 comments · May be fixed by #1423
Open

Automatic refresh: Do only once if application open in multiple tabs #1148

CedricHg opened this issue Oct 20, 2021 · 7 comments · May be fixed by #1423
Labels
feature-request Improvements and additions to the library.

Comments

@CedricHg
Copy link

CedricHg commented Oct 20, 2021

Is your feature request related to a problem? Please describe.
If automatic refresh is used and a user has the application open in multiple tabs, each tab will do a refresh token call when the timeout factor is passed.

Describe the solution you'd like
Ideally the call should happen only once.

Describe alternatives you've considered
Randomize timeout factors and prior to running refresh token, check that the current one meets some kind of minimum age criteria? But currently we'd have to implement that ourselves, I'd prefer letting the framework handle it

@jeroenheijmans jeroenheijmans added the feature-request Improvements and additions to the library. label Oct 20, 2021
@WolfspiritM
Copy link

I think the default implementation is using session storage which is per tab. That means each tab has their own access and refresh token and needs to refresh them individually. So I think this is only required if a custom OAuthStorage is used (e.g. localStorage instead of sessionStorage), right?

@jeroenheijmans
Copy link
Collaborator

jeroenheijmans commented Nov 5, 2021

Interesting thought. But I think with localStorage multiple tabs will also simultaneously recognize the token is about to expire, and simultaneously kick off silent refreshes? Haven't tested this though.

In fact, I would consider only with cross-tab storage solutions like localStorage it to be a feature if they coordinate things and do the request only once. If you have per-tab isolated OAuthStorage, it makes sense that each tab (even if it's simultaneous) doe its own refresh.

@WolfspiritM
Copy link

Actually for cross-tab storages like localStorage together with code flow and pkce it is a must have that the refresh is only triggered once as this solution is only "safe" with refresh token rotation activated. If both tabs send a token refresh at the same time then all refresh tokens will be revoked cause of token reuse and the user is logged out after the current access token expires.

However AFAIK it's not best-practice to use any long-term storage for refresh tokens anyway. The recommended approach even is to not use sessionStorage but in-memory (js closures) or even better webworkers to store the refresh token to prevent (easy) XSS access to the token. Both solutions mean that they are per-tab and each refresh get a new token.

https://auth0.com/docs/security/data-security/token-storage#browser-in-memory-scenarios

@CedricHg
Copy link
Author

CedricHg commented Nov 5, 2021

Good points above. In our scenario we are trying to limit the amount of requests to our IDP. The nature of the application is such that users tend to have it open in a few dozen tabs, causing a (what seems to be at first glance) excessive amount of refresh token calls. One measurement gave us 2426 calls for 70 users in just one hour.

I wasn't aware that storing in localStorage is not recommended, however it then makes sense that the library does not provide this functionality in order to discourage devs from doing so.

As far as I'm concerned, I'd of course still appreciate this being implemented ;-) But I could agree with this getting closed.

@jeroenheijmans
Copy link
Collaborator

Regardless of specific situations or storage types, I think it could be a good feature to have, but it will become rather messy if you think about all the edge cases:

  • Coordination between tabs should be a configurable option (or else it's a major breaking change)
  • It's only relevant in browser contexts, not in SSR situations
  • A tab can "hold off" on a refresh if another tab announced it was going to do one, but then again the tab should "prepare for the worst", i.e. the case where that other tab crashed or was closed and never completed it's refresh
  • What if accidentally (e.g. because of a race condition) multiple refreshes do go off? What then?
  • What if a browser / tab goes into hibernation (e.g. on a smartphone) during a call? What if the refresh failed?
  • Etc. etc.

In general, I think cross-tab coordination can be quite difficult. And it might be worth considering creating the right hooks in this library, so that app devs can do it themselves if it's important enough to them?

@CedricHg
Copy link
Author

CedricHg commented Nov 5, 2021

The "fix" I currently am using passes a random timeoutFactor to the library per tab, between 0.5 and 0.75. When the token_expires event comes in on any tab, we check that the access token is at least 50% expired. There is no "holding off" based on what another tab announces. If a tab wasn't able to complete the refresh, there will not be a new access token, so any of the other tabs will pass the 50% expired check whenever it's their turn to try to refresh. Likewise, hibernation etc isn't an issue here.

It IS still possible for multiple refreshes to go off simultaneously, but the chance of that is reduced, and when it does happen as long as we're using localStorage it's not a problem - it works the same way as it would before this fix with all tabs refreshing simultaneously.

So far from what we've tested, this accomplishes the objective well enough. I haven't yet noticed any downsides to this that aren't already present without applying this method.

@malek-itani
Copy link

I agree with @jeroenheijmans that making coordination between tabs may be difficult and lot of edge cases to be taken and may show later, but this doesn't mean there should not be a workaround or hack for the localStorage case.
what I suggest is similar to @CedricHg but this has to be handled from the package itself as a clean hack to all Storage providers is to perform an extra check when event token_expires is raised in this function ### setupAutomaticSilentRefresh and before calling refreshInternal truly check if access_token is expiring soon making a similar calculation to ### calcTimeout as if another tab performed the refresh process then the result of calcTimeout inside ### setupAccessTokenTimer is not true in other tabs and should be invalided
hopefully, this can be done as it minimizes the time we take to come up with custom solutions and hacks
cause every person using the localStorage will face an issue later with a concurrent refresh token triggered across multi tabs

@chdanielmueller chdanielmueller linked a pull request Jul 18, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request Improvements and additions to the library.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants