-
-
Notifications
You must be signed in to change notification settings - Fork 154
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
Support refresh token syncing for multiple tabs #444
Conversation
} | ||
|
||
await this._saveSession(syncedSession) | ||
this._notifyAllSubscribers('TOKEN_REFRESHED', syncedSession) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I originally thought about setting the refreshToken sync status inside an onAuthStateChange
listener.
But I didn't see an example of this inside this class, so figured just to leave it as is. The idea required:
- creating a listener for
TOKEN_REFRESHING
andTOKEN_REFRESHED
in the constructor. In there callsetItemAsync({..., status: "TOKEN_REFRESHING"})
- Adding
TOKEN_REFRESHING
to typeAuthChangeEvent
Instead, I went with the change localized to _callRefreshToken
open to suggestions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this implementation is fine, imo TOKEN_REFRESHING
seems like an implementation detail and we don't have to add it to the AuthChangeEvent
type.
resolve() | ||
}, 1500) | ||
}) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I saw there's an exponential backoff for network retry.
I didn't think it was required here. A request to refresh a token takes on average 150-300ms.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hey @miguelespinoza, just to clarify, the purpose of refreshTokenSyncIntervalTimer
is to continue check the storage to see if the status has changed from TOKEN_REFRESHING
to ``TOKEN_REFRESHED` right?
And we have refreshTokenSyncTimeoutTimer
to eventually cleanup the setInterval
. At this point, if the callback in setTimeout
is invoked, the status should still be TOKEN_REFRESHING
right?
|
||
const result = { session: data.session, error: null } | ||
const { data, error } = await this._refreshAccessToken(refreshToken) | ||
if (error) throw error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm looking closer into this case when an error occurs with the request.
So far with a network disconnection, calls resolve appropriately.
I just want to make sure everything is well accounted for, so just sharing what's next on my list to validate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, this.refreshTokenSyncStorageKey
is removed from storage when an error is thrown.
I tested this case using network disconnection. If the request failed we'll delete the refresh token sync status.
This will have a domino effect on all tabs:
- Tabs finish waiting on
TOKEN_REFRESHING
status to change. - Tabs will attempt to make a network request. It will fail
- Once one tab makes a successful network request. The tab waits. If successful avoid network request and use the latestSession
I'll hold on updating any test until we're all cool with these changes. |
Follow-up conversation from Discord: https://discord.com/channels/839993398554656828/1019328795129421944
This is a good assessment. I'd like to avoid any unnecessary work that could use up resources. The chrome documentation talks about event changes. This is what the multiTab option supported. Could we get an explanation for why this was removed? Another add-on could be idling the API request to refresh token if the tab is in the background? Any concerns here? @GaryAustin1 mentions realtime logic takes a break when in the background. We could apply the same behavior to the refresh token logic. |
Just to clarify on realtime in background... It does not take a break. I have code using a visibility handler to make it "take a break" by stopping it before the background timing errors occur (5 minutes). |
There's been an awesome coversation on Discord. Started here
Relying on localStorage events was supported in v1, but removed in v2. I don't know it's history, so it would be great to understand the decision for removing All in all, this information is helping towards finding an optimal solution.
|
Here's my realization. For a chrome extension that uses supabase on multiple tabs. I need to opt out of the
If this works, I wouldn't require the PR changes anymore. But I'm sure others would benefit from a fix on the library because this has been reproduced in a regular project: #442 |
Here's a WIP implementation of refresh token logic for browser extensions. It's not complete, but the core is in the gist. Two gotchas:
This trace would indicate EDIT: Found where fetch calls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @miguelespinoza, thanks for the detailed explanation and taking the time to investigate this issue. I've tested this PR on my end with the JWT_EXPIRY
set to 20s, REUSE_INTERVAL
set to 10s with 4 tabs and it's looking good.
We got rid of the multi-tab property because when we tested the v2 implementation initially, we couldn't reproduce the error. Also, i don't think the multiTab
property would've helped in this scenario to prevent a refresh outside the reuse interval. It was mainly used to add a listener to the storage event.
resolve() | ||
}, 1500) | ||
}) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hey @miguelespinoza, just to clarify, the purpose of refreshTokenSyncIntervalTimer
is to continue check the storage to see if the status has changed from TOKEN_REFRESHING
to ``TOKEN_REFRESHED` right?
And we have refreshTokenSyncTimeoutTimer
to eventually cleanup the setInterval
. At this point, if the callback in setTimeout
is invoked, the status should still be TOKEN_REFRESHING
right?
} | ||
|
||
await this._saveSession(syncedSession) | ||
this._notifyAllSubscribers('TOKEN_REFRESHED', syncedSession) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this implementation is fine, imo TOKEN_REFRESHING
seems like an implementation detail and we don't have to add it to the AuthChangeEvent
type.
@@ -105,6 +113,7 @@ export default class GoTrueClient { | |||
const settings = { ...DEFAULT_OPTIONS, ...options } | |||
this.inMemorySession = null | |||
this.storageKey = settings.storageKey | |||
this.refreshTokenSyncStorageKey = `${this.storageKey}-rts` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it possible to actually storage this information in the same storage used for the session?
One thing i'm trying to figure out is how the client can get to the point where one of the refreshes happen outside the reuse interval. 10 seconds is a long time and i suspect that the reason for this is because the session passed into the |
Hey @miguelespinoza, i think i've figured out the issue, seems like the session passed into the |
@kangmingtay Thanks for submitting a PR. I think that would be a better change than this. Your change is small, and it's a change I advocated for after submitting this PR. I've since gone with a different solution commented here. Others had this issue too, so I'm learning on them to help with testing 😁. While I have your attention, I'm working on another project and had to downgrade from v2 to v1, because of this tangential auth issue: #450. Just sharing just in case, if it's not already on your radar |
hey @miguelespinoza, yup we're gonna look into #450 next, thanks for taking the time and effort to make this PR though! |
What kind of change does this PR introduce?
This change introduces a mechanism to sync refresh tokens at the client level, rather than depending on the backend.
The current behavior of supabase-js v2 unfortunately continues to log out users. This was reproducible with at least three tabs. More information here: #454 and #442
Implementation Details
The sync logic uses
this.storage
to cache the status of the refresh token request. The only properties stored arerefresh_token
andstatus
. CheckSyncTokenRefreshStorage
.TOKEN_REFRESHING
is set on status before the API request is made.TOKEN_REFRESHED
is set on status after the API request resolved.For any tab that attemps to refresh the token.
TOKEN_REFRESHING
. It waits until token has refreshed, then pulls latest session from storageTOKEN_REFRESHED
. it pulls latest session from storageWhat is the current behavior?
The user is logged out with at least three tabs: #454
Notice a burst of network requests to the API. Eventually, this causes a 400 error, logging out the user. Check timestamps.
What is the new behavior?
The tabs depend on only one request to succeed. The remainder tabs wait until the refresh token is retrieved from the API to resolve. Providing a clean POST request of one to the API. Check timestamps.
Additional context
mitmproxy
extremely helpful to view the requestsmitmproxy --mode socks5 --showhost --view-filter="~u supabase.co ~m POST"
Edit: Tue Sept 20 - Included Implementation Details