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

Automatically refresh access token before it expires #78

Closed
wants to merge 2 commits into from

Conversation

dabarrell
Copy link

@dabarrell dabarrell commented Apr 19, 2022

Addresses #73

Uses a similar timeout mechanism to gotrue-js's autoRefreshToken function, but simplified.

With the refreshToken available in the frontend after the first commit, it could theoretically be passed to the supabaseClient.auth instance so that we can use its autoRefreshToken function, but the only current interface for setting the refreshToken is supabaseClient.auth.setSession(refreshToken), which triggers a refresh of the token, which we don't want. So the (arguably cleaner) alternative is to handle the refresh within UserProvider.

if (accessToken) {
supabaseClient.auth.setAuth(accessToken);
setAccessToken(accessToken);
}
if (refreshToken && expiresAt) {
setRefreshToken({ token: refreshToken, expiresAt });
}
setUser(user);
if (!user) setIsLoading(false);
Copy link
Author

Choose a reason for hiding this comment

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

Seems like there's some double-handling of isLoading in this component – it's set in checkSession, and in the two useEffect hooks that call checkSession. Seems worth cleaning up, but I didn't want to muddy this PR.

Copy link
Author

Choose a reason for hiding this comment

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

While I'm thinking about it, it seems strange to set isLoading to true on subsequent reloads of the user (after the first load). If you think about how isLoading would be used practically, it'd be to show loading states etc on an initial page load. As a consumer of the hook, I wouldn't expect isLoading to then flick back to false then true again every time the user is refreshed – it'd cause loading states to briefly pop back in, which would be strange

@thorwebdev
Copy link
Member

Thanks for this. I think the setTimeout itself, isn't enough though. Maybe needs to be paired with something like #35 ?

Also, we shouldn't refresh the token on the client-side, but rather on the server-since the cookies are the source of truth and we don't want them to become out of sync. So rather than calling supabaseClient.auth.setSession we should make a get request to api/user which then uses getUser to refresh the token. getUser should be the only method that handles token refreshes. Wdyt, does that make sense?

@dabarrell
Copy link
Author

dabarrell commented Apr 20, 2022

setTimeout feels a little thin to me, but I took inspiration from @supabase/gotrue-js which uses setTimeout for its auto refresh mechanism. In all my testing, it seems to be working really well – haven't had any issues.

I think the issue with #35 is that it allows the token to expire, meaning if the user comes back to the page after it's expired, there's a chance that requests might be sent with an expired token before the refresh is completed.

As an example, my app uses SWR, which I've configured to refresh data when the page becomes visible again (as is a common pattern). This would mean the page data refresh and the token refresh would happen at the same time, and the former would fail.

Also, we shouldn't refresh the token on the client-side, but rather on the server-since the cookies are the source of truth and we don't want them to become out of sync.

With this PR, they won't get out of sync as calling supabaseClient.auth.setSession triggers a SIGNED_IN event, which is picked up by supabaseClient.auth.onAuthStateChange in UserProvider and pushed to the /api/auth/callback route, which updates the cookies with the new tokens.

That said, I could understand the design choice to avoid reading refresh tokens in the client, and instead let the handlers pull them out of cookies and complete the refresh there. As it stands, calling /api/auth/user won't cause a refresh unless the access token has already expired, but perhaps we could add a /api/auth/refresh handler specifically for this use-case? Or we could add a parameter to /api/auth/user to force a refresh? e.g. /api/auth/user?forceRefresh=1

Though we'd still need to know the expiresAt in the frontend, in order to know when to trigger the refresh

@dabarrell
Copy link
Author

the cookies are the source of truth

I'm not sure this PR breaks that principle (which I agree with) – it's reading the refresh token from the cookies then using it in the frontend in the same way that the access token is being used. It's not being updated elsewhere - only in the cookies

@dabarrell
Copy link
Author

Maybe needs to be paired with something like #35 ?

Apologies! I misread this initially – I see now that you're suggesting using both setTimeout and also something like #35. That could certainly make it more reliable!

@silentworks
Copy link
Contributor

I think @GaryAustin1 has a more robust fix as its at the gotrue-js level supabase/supabase#6464

@thorwebdev
Copy link
Member

@silentworks, AFAICS @GaryAustin1 solution is for client-side refreshes when the tokens are managed in localStorage. With the auth helpers, token management is moved to the server-side via cookies, so the refresh would need to happen there so that tokens don't get out of sync.

Since we're moving away from storing tokens in localStorage in the future, I'd prefer to have the auth helpers not rely on this part of gotrue-js if that makes sense?

@GaryAustin1
Copy link

GaryAustin1 commented Apr 20, 2022

All,
This is a multifaceted issue.

  1. detection of expiring token should be a common util or at least formula for all of Supabase. Currently there is no specified minimum token time. It would help if it was something like two minutes. Then a 10 sec number could be used reasonably. A formula could also be used based on the exp life provided back from gotrue. In the end this refresh is just to get thru fixed immediate code though. I’ll note that gotrue.js was changed last year because apparently there was concern for less than 60 sec refresh setting.

  2. A reliable means of refreshing the token that does not depend on the timers is needed. I highly recommend Supabase.js check the token expiration when it sets the authorization bearer header on each call. If about to expire it can refresh it and generate an event that can be monitored to sync severside/local copies. This is the method I am attempting to simulate with fix 1 in my code. @thorwebdev I’m not sure my report in Supabase has anything to do with local storage. It is a timer issue on mobile that will affect any client side timer (timers can be throttled or no code run at all during sleep/background tabs) and storage method of the session data does not matter. Even auth helpers apps will potentially not make a serverside call for hours. My initial report here is just that auth helpers has no means of refresh at all if specific calls are not made within expire time.

  3. All testing should be done with low token expire time set on server and robust use of RLS database calls on both server side and client side.

  4. Edit adding: Realtime also needs to be considered in the token update process as I think it depends on some internal Supabase.js event to update the websocket token.

@thorwebdev
Copy link
Member

Yes, it's happening on a timer because of realtime. See some suggested improvements here: supabase/auth-js#274

@thorwebdev
Copy link
Member

This has been added in #92

@thorwebdev thorwebdev closed this May 10, 2022
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