supabase / supabase-flutter

Flutter integration for Supabase. This package makes it simple for developers to build secure and scalable products.
https://supabase.com/
MIT License
703 stars 167 forks source link

[auth] Bug: Auth currentSession is null #860

Closed iosephmagno closed 5 months ago

iosephmagno commented 6 months ago

Describe the bug Hello @dshukertjr just reporting that we were hit but the null auth session bug. The code is standard one:

 await Supabase.initialize(
      url: _url,
      anonKey: _publicAnonKey,
      debug: kDebugMode, // optional
    );
 debugPrint('$_debugPrefix Auth.currentSession is: ${auth.currentSession}');

auth.currentSession is null.

To fix issue, I had to uninstall app and reinstall it from scratch. No errors were found on supabase console.

To Reproduce Unfortunately, there are no clear steps to reproduce the bug. It occurs randomly out of blue. In our case, usually once every 2 months and doesnt autoresolve with app launch, user must delete app.

Expected behavior Client should always be capable to auto-resolve any auth issue while fetching a new session from GoTrue server.

Version (please complete the following information): On Linux/macOS Please run dart pub deps | grep -E "supabase|gotrue|postgrest|storage_client|realtime_client|functions_client" in your project directory and paste the output here.

── supabase_flutter 2.3.4
│   ├── supabase 2.0.8
│   │   ├── functions_client 2.0.0
│   │   ├── gotrue 2.5.1
│   │   ├── postgrest 2.1.1
│   │   ├── realtime_client 2.0.1
│   │   ├── storage_client 2.0.1

Additional context Issue resolves by uninstalling the app, hence cache is involved. I mean the issue is caused by what the plugin has saved to cache and revert as soon as cache is cleared.

Potential workaround Since client knows when the currentSession is null, can it clear the cache if currentSession is null after X retry? Thats doesnt fix the root problem but should workaround the issue.

iosephmagno commented 6 months ago

Forgot to mention, on android we cleared app's cache without deleting app and issue resolved also this way.

iosephmagno commented 6 months ago

This also may help. After initialize()n we listen auth event and also print eventual error. But when issue occurs we get no data from the listen and no error from onError.

// Listen on auth change event and take action
  auth.onAuthStateChange.listen((data) async {
    if (data.event == AuthChangeEvent.tokenRefreshed) {
     …
    }
    if (data.event == AuthChangeEvent.signedIn) {
    …
    }
    if (data.event == AuthChangeEvent.userUpdated) {
    …
    }
    if (data.event == AuthChangeEvent.initialSession) {
    …
    }
    if (data.event == AuthChangeEvent.signedOut) {
   …
    }
  }).onError((error) {
    debugPrint('$_debugPrefix  Error within onAuthStateChange: $error');
  });
}

At the moment I have one client stuck in this situation. If you want me to run some code against it, I can do it.

iosephmagno commented 6 months ago

Another thing which may be important to know is that this time the issue occurred when we restored a supabase backup. Supabase's support email said that the two things should be unrelated, but maybe it is not a coincidence. If so, that is worth a deeper investigation coz might potentially lead to entire user-base being forced to uninstall app in case we restore a database backup.

iosephmagno commented 6 months ago

I did run code:

await auth.refreshSession();

And then also:

await auth.reauthenticate();

LOG: [ERROR:flutter/runtime/dart_vm_initializer.cc(41)] Unhandled Exception: AuthException(message: Not logged in., statusCode: null)

0 GoTrueClient.refreshSession (package:gotrue/src/gotrue_client.dart:586:7)

1 PresenceAuth.initialize (package:presence/api/auth.dart:108:16)

#2 main (package:presence/main.dart:281:3) I tried also to dispose instance after `Supabase.initialize`: ``` await Supabase.instance.dispose(); ``` LOG ERROR:flutter/runtime/dart_vm_initializer.cc(41)] Unhandled Exception: 'package:supabase_flutter/src/supabase.dart': Failed assertion: line 36 pos 7: '_instance._initialized': You must initialize the supabase instance before calling Supabase.instance #0 _AssertionError._doThrowNew (dart:core-patch/errors_patch.dart:51:61) #1 _AssertionError._throwNew (dart:core-patch/errors_patch.dart:40:5) #2 Supabase.instance (package:supabase_flutter/src/supabase.dart:36:7) #3 PresenceAuth.auth (package:presence/api/auth.dart:40:44) #4 PresenceAuth.initialize (package:presence/api/auth.dart:112:9) I see event `initialSession` when launching app, and it is stuck there, `currentSession` and `user` is still null. flutter: **** onAuthStateChange: AuthChangeEvent.initialSession Hope this helps.
dshukertjr commented 6 months ago

So the issue is that you were signed in on the app, but one day when you opened the app, you were signed out, correct?

You say you had to uninstall the app and install it again to fix it (or delete app cache on Android), but if you were signed out, couldn't you have just signed in again (understanding that being signed out randomly is annoying, and should be fixed)?

iosephmagno commented 6 months ago

I also thought that it could be a signout issue but didnt try to sign in back via signInWithOtp(). Coz even if it worked, we should bring user to app initial onboarding screen and that would panic user in our case (Presence is like Whatsapp).

I've tried in the past to suggest an ultimate workaround to this problem. Coz it can be solved with a set of recovery tokens issued by Gotrue at signup (and maybe refreshed over time). Simply put: if currentSession is null for whatever reason, client will sign in with one of its recoveryToken. But Supabase Auth team didn't like this option and suggested that Auth is put up in a way that it can work perfectly as it is. On the other hand, recovery tokens is a quite common practise. I mean in case of whatever Armageddon, a valid registered signedIn user won't be signed out and client can always auto-resolve any auth issue.

In any event, can you guys please brainstorm on the flutter client and maybe inspect the code all together. Coz it is a fact that the bug is hard to fix and couldn't be fixed in over one year. We are getting close to our launch date and cannot launch app with this issue.

dshukertjr commented 6 months ago

Okay, the cause of this issue is most likely the refresh token being invalid for some reason (network connection being interrupted before getting the auth response back when refreshing the session or something). The cause is similar to what's causing this or other JWT issues.

We have a fix coming up on the client that would hopefully resolve it. Thanks for your patients.

iosephmagno commented 6 months ago

Another thing which may be important to know is that this time the issue occurred when we restored a supabase backup. Supabase's support email said that the two things should be unrelated, but maybe it is not a coincidence. If so, that is worth a deeper investigation coz might potentially lead to entire user-base being forced to uninstall app in case we restore a database backup.

Thx! Could you please also check this? Coz issue also occurred (on all our phones) after we restored an old backup. If that is also an issue, this is serious too. To reproduce you could just restore backup of 1day ago and check if any issue with refresh token.

dshukertjr commented 6 months ago

@iosephmagno Yeah, restoring backups will wipe out the auth schema as well, so any users who have refreshed their session (pretty much meaning pretty much any users that were active) between the time of when the backup was taken, and the backup was applied will be signed out as their refresh token will be invalid.

iosephmagno commented 6 months ago

@dshukertjr thx for confirming this. Can you please get Auth team's attention on that? In a generic successful scenario, restoring a backup would cause million of users being forced to uninstall app, and they wont even get informed from the company (coz app is dead).

I dont want to sound pushy on my idea, whatever the fix is okay for us, but recoveryToken would be a fix for this issue as well. Also, the consequences of auth issue in terms of brand damage, loss of users, etc is so high that any risky solution is not worth it. We really need to be care-free on that. If we dont give the client a disaster recovery door (whatever it is), backend alone cannot be 100% safe in mantaining the users in.

dshukertjr commented 6 months ago

@iosephmagno

restoring a backup would cause million of users being forced to uninstall app, and they wont even get informed from the company (coz app is dead).

No, they would just be signed out, and they can signed back in. There is not much the auth team can do here. That's how backups work. It restores the entire state of your database to a given point in time. It's the last resort in a case of catastrophic data loss, and you as the developer have to understand the consequences.

iosephmagno commented 6 months ago

Yea I understand this.

But let me better explain my thought here.

In terms of affected users we can assess they will be most part of entire users base, due to token refresh timing and app nature (Messenger).

Ideal case would be that we dont kick off the users who are registered to Auth table. We now kick them off coz we dont have a disaster recovery flow at the moment, but as long as user is inside Auth table and we enable the client to reconnect somehow, kick-off wouldnt be required).

Also, considering the very case where user is signed out for some uncovered reason. We should first display a screen where we explain to user the situation and then navigate to signin screen. But, if I remember well, session is temporarily null during app launch, so we would appreciate to have a sample explaining how to do it correctly.

dshukertjr commented 6 months ago

Ideal case would be that we dont kick off the users who are registered to Auth table. We now kick them off coz we dont have a disaster recovery flow at the moment, but as long as user is inside Auth table and we enable the client to reconnect somehow, kick-off wouldnt be required).

You performed the backup, and the session info is gone from the database. As far as the database is concerned, the session tokens that those users have are invalid, and there is no way for the users to re-obtain the session other than to re-authenticating.

Also, considering the very case where user is signed out for some uncovered reason. We should first display a screen where we explain to user the situation and then navigate to signin screen.

This I agree. There should be an error passed to the onAuthStateChange if the session recovery fails.

But, if I remember well, the session is temporarily null during app launch, so we would appreciate to have a sample explaining how to do it correctly.

The session is not null if the user is signed in. There will be a session available with an initialSession event on onAuthStateChange. If the token is invalid, signOut event will be emitted after that. I believe failing to recover the session is the only situation where initialSession event is emitted and then signOut event is emitted right after, so you could probably use that.

iosephmagno commented 6 months ago

Can you guys please debug the flow by simulating the two cases and add specific errors inside onAuthStateChanged?

This is the alert we can show in case user gets logged out without real reason. As mentioned, we must avoid this bug 99.999%, but we also must be prepared to deal with it if required. The message is generic and tries to not panic user.

Alert: To protect your user's account to the fullest, we might sometime ask you to signin back again. This event is rare and doesnt require to upgrade app.


This is the alert we should display in case of a backup restore, if Auth wont enable the client to get a new session anyway through a special flow. The user will be informed properly and wont panic.

Alert Due to a fix to a minor disservice, we must require you to signin back again. This event is extremely rare and with no consequences on your chat history. However, some of your recent profile's stories might get lost. Please check your profile and repost them, if required.

Does it make sense?

dshukertjr commented 6 months ago

We will get to it whenever we can 👍

iosephmagno commented 6 months ago

Thx! As of now, it seems we cannot code a patch for this coz:

For the first panel: we could check if currentSession is null, but it is null during the signup process and panel would appear when not required (unless we cache a property userLoggedIn and show panel if userLoggedIn && currentSession is null). But maybe this would work bad in some edge cases?

For the second panel: it is not clear whether we should grab the event initialSession null. Maybe this would conflict too with normal flow.

We should need supabase client throwing either specific errors or state events. It is also unclear whether supabase client can get to distinguish the two events, which both kickoff user but require a different panel with different message. Maybe Gotrue server should return a specific error for the two cases.

dshukertjr commented 5 months ago

We have shipped an update that will hopefully make things better with this issue. If you could try out v2.5.0 of supabase_flutter and see if things are better, that would be great.

iosephmagno commented 5 months ago

Great! Will test it and lyk.

iosephmagno commented 5 months ago

Thx! As of now, it seems we cannot code a patch for this coz:

For the first panel: we could check if currentSession is null, but it is null during the signup process and panel would appear when not required (unless we cache a property userLoggedIn and show panel if userLoggedIn && currentSession is null). But maybe this would work bad in some edge cases?

For the second panel: it is not clear whether we should grab the event initialSession null. Maybe this would conflict too with normal flow.

We should need supabase client throwing either specific errors or state events. It is also unclear whether supabase client can get to distinguish the two events, which both kickoff user but require a different panel with different message. Maybe Gotrue server should return a specific error for the two cases.

@dshukertjr any news about how to grab these events such to show correct info panel to user?

dshukertjr commented 5 months ago

@iosephmagno That is a separate issue, so feel free to open a new issue to discuss the matter.

iosephmagno commented 5 months ago

@dshukertjr user is not added to User table after otp code is verified.

dshukertjr commented 5 months ago

@iosephmagno I'm going to need more context of what exactly your issue is to look into it. A new row is inserted into the auth.users table not when they verify the OTP, but when they first signUp using auth.signUp() method. If it's a separate issue from what the original issue is about, please open a new issue.

I'm going to close this issue as a fix has been shipped. If the original issue still persists, please feel free to reopen this one.

iosephmagno commented 5 months ago

@dshukertjr Hi Tyler, it was an Auth backend issue. Thought it was due to this coz I was testing it right when issue occurred.

iosephmagno commented 2 months ago

@dshukertjr just informing that this occurred again while using latest plugin version ^2.5.6. Since no changes have been done related to this specific issue, we can assume that this is not a regression and Supabase auth flow is still such that currentSession can be null for some reason and client has no way to solve this issue when it occurs.

Please discuss internally about a "session-recovery-flow" for this case. If my idea to use a "restoreToken" is not liked, you might come up with something different. But as of now, it is almost certain that this issue will occur at least 2-3 times / year and it is not acceptable on mobile apps (users will freak out).

Meantime a production-ready solution is implemented, can you guys please instruct us with a flow that can be used to help user navigate through this issue? Maybe kick user off and ask him/her to signin again with some fake reason, like: "This is a security procedure, please signin again". No idea about what else we can do without sounding bad to users. Thx

iosephmagno commented 2 months ago

Can we please also re-open it so we can track it? There isn't new data for us to submit with a new issue.

dshukertjr commented 2 months ago

If the currentSession is null, that means that the user is signed out. This could happen when the session refresh is triggered and the request is sent off from the client, but the network is shut off before the response comes back.

I believe you are overreacting to this issue. Although getting signed out 2-3 times a year might not be ideal, it's not something users would freak out. We are constantly trying to make our auth better, and I could only ask you to just patiently wait.

iosephmagno commented 2 months ago

Yes should be an edge case related to network state during auth flow, which makes debug harder.

@dshukertjr, I appreciate much the work done so far on this bug. And I do understand it is tricky to fix (this is why I tried to put on the table that recoverytToken idea to, at least, let devs workaround issue until a final fix is available).

We are not overestimating the severity of the issue. Being getting kicked off from a mobile app (twitter, whatsapp and even the little niche apps) is never happened to nobody. With chat apps, there is extra panic related to the fact that average user cannot distinguish a kick-off from other bad events, so if they see the onboarding screen asking to login back, they just freak out. Those who don't freak out might consider the app to be cheap and might be just tempted to trash it. Please guys do not consider Auth production-ready on mobile until this bug is fixed. LMK if we can help with debugging somehow. Now we are in private beta on both android and appstore, we won't open to the public until this bug is fixed.

iosephmagno commented 2 months ago

If fixing bug might require long time, can you please assess again our idea with the Auth team?

Flow would be like:

  1. mobile client signs up via otp. The api is the same but client would receive an extra token named recoveryToken.
  2. client saves recoveryToken to encrypted sharedprefs (flutter_secure_storage is a bit buggy and might fail at time, we are now testing flutter_keychain). Encrypted sharedprefs should be the safer plugin for that scope at the moment.
  3. if client fails to get an auth session after the retry loop, it will send to Auth server the recoveryToken which would be like a new login without otp code. For security reason, client might send recoveryToken + last 2 used tokens (also saved to encrypted sharedPrefs). A new recoveryToken will be created on Auth server only after the client used it successful with a 200 code.

With this minor improvement, it can never happen that a mobile user cannot connect. Also, recoveryToken can be refreshed over time. The key point here is, whatever is the network condition that impaired the auth procedure, client still has a way to connect after retry loop.