supabase / supabase-js

An isomorphic Javascript client for Supabase. Query your Supabase database, subscribe to realtime events, upload and download files, browse typescript examples, invoke postgres functions via rpc, invoke supabase edge functions, query pgvector.
https://supabase.com
MIT License
2.86k stars 220 forks source link

`supabase.auth.logout()` throws "Invalid user" error. #15

Closed thorwebdev closed 3 years ago

thorwebdev commented 3 years ago

I found this to only happen sometimes, but couldn't figure out why: image

@kiwicopple have you seen this before?

kiwicopple commented 3 years ago

The only thing I could think is that the token is expired when you try to log out. A couple of questions:

thorwebdev commented 3 years ago

Is the browser left open, and then you try to log out after >3 hours?

I'm able to reproduce this multiple times within a time window of less than 1h

There is a logout network call about 10 requests above the erroneous network call. Did you hit the button twice? Perhaps you're logged out already and the redirect to the home screen failed for some reason

Yes, there are times where the logout call is successful. You can see in the network comms though that after the logout there is a login (token?grant_types=password, right?). So don't think it's that.

awalias commented 3 years ago

@thorwebdev are you just using the OG slack clone example here?

thorwebdev commented 3 years ago

@awalias yes, I'm able to observe this also on the official https://supabase-slack-clone.vercel.app/

slack-clone-logout-error

awalias commented 3 years ago

could be some race-condition here? https://github.com/supabase/supabase/blob/1e49eaeb7e0540c9cc1e4dad627e779a33b0500f/examples/slack-clone/pages/_app.js#L43

awalias commented 3 years ago

issue is when a user instantiates two or more supabase clients - fix is to always check localStorage before adding this.accessToken as an auth bearer token to see if there is one on the browser

phamhieu commented 3 years ago

It's gotrue logout api bug. The cookie is cleared before calling getUserFromClaims.

Screenshot 2020-08-17 at 10 50 15 AM

Screenshot 2020-08-17 at 10 51 54 AM Screenshot 2020-08-17 at 10 52 23 AM

phamhieu commented 3 years ago

On slack-clone app, after an Invalid user error fires, I can login and logout successfully!!! no idea why the logout can goes through getUserFromClaims check

However the next login/logout will trigger Invalid user error again.

awalias commented 3 years ago

Oh we didn't update this issue after meeting @thorwebdev last week about this.

The issue here is that Slack-clone app uses two different supabase client instances, and supabase-js only reads from local storage on initiation, and manages it's own state of which user is logged in after that point - so multiple clients can easily get out of sync on current user state

Our options here are:

1) supabase-js should check local storage before each call to see if there has been a change in user auth by a different client instance. This may be expensive

2) we direct people to only init a single supabase client if they're using auth

3) we make createClient return a singleton

any other?

phamhieu commented 3 years ago

@awalias what you describe seems to be another issue related to front-end.

The original error reported by @thorwebdev is from gotrue logout api. It can be reproduced easily.

  1. start your test supabase project, run slack clone quick start to create required tables
  2. pull slack clone app, update env with your test project keys and run it
  3. login and logout multiple times slack-clone client (with the same acc and client) you can see the error

Screenshot 2020-08-18 at 1 54 51 PM

I already check logout api on gotrue. The last commit on logout.go breaks the check mechanism.

Before it works properly, because the claim is retrieved before clearing token https://github.com/netlify/gotrue/blob/47cc9ce137a24c96985ee3e742b0f0adfb6f146c/api/logout.go Screenshot 2020-08-18 at 2 02 43 PM

After https://github.com/netlify/gotrue/blob/8304885327eb93a7346f4b27658f470499c39107/api/logout.go Screenshot 2020-08-18 at 2 04 32 PM

awalias commented 3 years ago

I think the response from gotrue is actually correct in this instance, if you look at the request headers on the slack clone app, you will see that the apikey and auth bearer headers are the same, in this case the jwt being (mistakenly) sent is the anon key:

image image

for comparison here is an example of a successful logout, with decoded jwt below: image image

The bug seems to be that the supabase-js client calling logout does not have the current user token, since it was already cleared from the client by the other instance

the "double logout" seems to be coming from here: https://github.com/supabase/supabase/blob/fed822f48c5e441eb867fa756443e362ac47423f/examples/slack-clone/components/Layout.js#L59

and here: https://github.com/supabase/supabase/blob/fed822f48c5e441eb867fa756443e362ac47423f/examples/slack-clone/pages/channels/%5Bid%5D.js#L16

awalias commented 3 years ago

also as a side note - we actually don't make use of the cookies set by go-true, we manage these ourselves using local storage inside supabase-js

phamhieu commented 3 years ago

Thanks @awalias for clarification. After modifying slack clone example to use a single Supabase client, everything works properly.

phamhieu commented 3 years ago

Currently, supabase-js persists accessToken, refreshToken and currentUser to localstorage while also keeps them as class params inside supabase.auth. When we have multi supabase clients, these params can be out of sync.

We should use localstorage as the source of true and don't keep them as class params. It's the same as how we get authHeader to supply PostgrestClient. What do you think? @kiwicopple

awalias commented 3 years ago

1) is it inefficient to fetch from local storage every time? 2) what about server-side / node

maybe keep track of them internally but always check local storage first (if it exists?)

phamhieu commented 3 years ago
  1. right now everytime you call a request with postgrest client, it will read localstorage for accessToken to include in the header. It works ok until now. So i think it's efficient enough. Screenshot 2020-08-18 at 6 37 24 PM
  2. for server-side, accessToken should included in the header. To get refreshToken and currentUser we can call gotrue /user endpoint with accessToken