fix: Retry access token refresh when offline #71
Conversation
|
What if the user never goes online back? It'll be signed out anyways |
|
@bdlukaa Yeah, that is true, but I think that is another problem. The problem that I am attempting to solve here is the case where a user's network fails for a short period time like going though a tunnel or something. |
|
Hello @dshukertjr, thanks for the fix! Is the larger fix for “user logged out if app is closed during a token refresh” expected to come any soon? Here at Presence we really need it 🙏 |
|
Hi @iosephmagno! Yes, a fix for that is in the works as well, but might take a bit more time, since it will involve some change on the backend as well. |
There was a problem hiding this comment.
I still don't get it how this fixes the mentioned issue. If the user is offline, he/she will stay offline.
If user is not connected to internet and open app after a hour or two
This is the case when the user is without wifi and without mobile data. Wouldn't efreshing the token while still offline cause any StackOverflow error?
|
@dshukertjr what the secuity approach behind this other fix will be? As user, we would expect the server to refresh token after its expire date the first time user log in after that date. And if user wont login for 1yr, the token shouldnt be refreshed. Is that correct? |
|
I think the biggest and most annoying problem that the users are facing is that if the device happened to be offline, the user is signed out immediately. This PR aims to solve that problem. I agree with you that this PR does not fix a case where the app user opens the app somewhere completely offline. In that case, the user will still be null and it would appear if the user has signed out. If the offline is temporary, they user will get signed back in because of token refresh retries. I have changed the PR description to make this PR a
Could you please educate me on where StackOverflow error could occur? |
That is a great question. The Supabase client and the server will indeed issue a new access token once the access token expires, but currently the access token will be refreshed even after inactivity for a year I believe. If you would like the ability to revoke the refresh token after a year, please feel free to open an issue on https://github.com/supabase/gotrue. |
|
@dshukertjr Thanks for the info! I think user should never be signed off. As far as I know, we could stop opening twitter, whatsapp or facebook app for two years in a row and we wouldn't be kicked off. I know this seems a security issue but disconnecting user from mobile app is weird (app would be considered buggy) and costly (a new otp verification would be required). Also, some choosy people might give 1 star to the app and leave a bad review only for that :) So yes, it would also harm the app reputation. |
|
@bdlukaa Feel free to comment if there is anything that is unclear! |
|
Some points:
|
| _refreshTokenRetryCount++; | ||
| if (_refreshTokenRetryCount < 720) { | ||
| _refreshTokenTimer = Timer(timerDuration, () { | ||
| _callRefreshToken(); | ||
| }); | ||
| } |
There was a problem hiding this comment.
This is a very arbitrary value, but I said it will retry for a duration of hour (720 retries every 5 seconds). Do you think this is an appropriate value?
| if (response.data == null) { | ||
| final error = GotrueError('Invalid session data.'); | ||
| return GotrueSessionResponse(error: error); | ||
| } | ||
| _refreshTokenRetryCount = 0; |
There was a problem hiding this comment.
Here I am resetting the retry count upon successful token refresh.
|
Thanks @bdlukaa! I will take a look at it. |
|
@dshukertjr just informing we still get signed out out of blue |
Currently when accessToken refresh fails for any reason, gotrue-dart never retries to update access token and the user is signed out. This PR fixes that by retrying to refresh the token if it fails due to network error.
It will also not sign a user out just because a token refresh failed. Instead, it will retry every 5 seconds to get a new token.
Related supabase/supabase-flutter#33