-
Notifications
You must be signed in to change notification settings - Fork 694
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
Comments
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? |
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. |
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 |
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. |
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:
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? |
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. |
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. |
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
The text was updated successfully, but these errors were encountered: