supabase / auth-elements

Components to add Supabase Auth to any application
MIT License
11 stars 1 forks source link

The "Remember me" checkbox in the Auth component does NOT do anything #7

Open chipilov opened 3 years ago

chipilov commented 3 years ago

Bug report

Describe the bug

The Auth component currently renders a "Remember me" checkbox, however, this checkbox currently does NOT do anything.

To Reproduce

  1. Use the Auth component from the library to allow users to sign in using email and password
  2. Pass a SupabaseClient object to the supabaseClient prop of the Auth component where the SupabaseClient object was initialized without any optional params
  3. When presented with the email and password UI, avoid clicking on the "Remember me" checkbox

Expected behavior

Since the user did NOT check the "Remember Me" checkbox, refreshing the page should automatically result in signing out the user. However, since a SupabaseClient initialized without any optional params stores the session in localStorage by default, the user remains logged in even if they refresh the page.

Additional context

It seems that an underlying problem for this scenario would be that the SupabaseClient only allows setting the "persistSession" param during initialization of the object. This means that even if the users checks/unchecks the "Remember Me" checkbox there is NOT much the Auth component can do unless SupabaseClient allows changing the "persistSession" option AFTER the client has already been initialized.

i-pip commented 3 years ago

So this doesn't work because it's not implemented in gotrue-js. What's supposed to happen is that the when "Remember me" is checked, gotrue-js is supposed to use localStorage and when "Remember me" is unchecked, use sessionStorage. Currently only localStorage is used.

Getting the expected behavior would require changing the signIn and signUp function signatures - adding an optional parameter for the "remember me" option, this component (supabase ui Auth) can then send that argument on signIn / signUp over here and here

@awalias & @MildTomato, this is something I'd be happy to work on if you think it's a good idea.

P.S. @chipilov - even with "Remember me" unchecked, refreshing the page should still keep the user signed in - its only when the browser window is closed that a signed in user is supposed to be "forgotten" or signed out

chipilov commented 3 years ago

@i-pip While many sites survive a page refresh when you have NOT checked the "Remember me" option, I don't think this is universally true. For example, my bank's site does NOT survive a page refresh - one could definitely argue that this is overkill, but I can also see why some sites with very sensitive info might want to be as conservative as possible.

Hence, I think it would be nice if the introduction of sessionStorage does NOT completely replace the in-memory storage but is instead complementary to it (i.e. the developer can choose whether to use localStorage, sessionStorage or in-memory storage). I guess this in-memory storage would only make sense for SPAs but since they are quite popular currently, it might be worth to keep it. Just my 2 cents, I don't feel strongly about it.

Other than that, I agree that the parameter for whether/how to persist the session should be on the signIn/signUp methods instead of the Supabase constructor.

MildTomato commented 2 years ago

I think we either get this remember me checkbox to work or we remove it from the component, since it makes no sense having a non working input.

I'm not familiar with what the actual technical implementations of 'remember me' functionality actually is, but perhaps we can set the expiry on the cookie to be longer with supabase-js if they select 'remember me'. I'd have assumed that's really what an end user expects. I could be wrong. @chipilov makes some good points that there's a lot of different use cases, like the bank one (killing the session after refreshing the browser just once). I'm guessing if we really wanted to improve this Auth component so it was really flexible we should be covering more use cases.

what do you think @kangmingtay ? Is there a solid way of using supabase-js to implement a 'remember me' behaviour?

kangmingtay commented 2 years ago

Generally, the "remember me" functionality tells the browser to save a cookie so that if the window is close or refreshed, the user will automatically be signed in. Would it be a good idea to add a boolean field in gotrue-js to clear the localStorage / cookie if the "remember me" field is not checked?

Disclaimer: I'm not too familiar with the technical implementation for this - will need to do more research on this

MildTomato commented 2 years ago

Would it be a good idea to add a boolean field in gotrue-js to clear the localStorage / cookie if the "remember me" field is not checked?

Should we do an RFC on gotrue repo to get some comments on it? @kangmingtay

in the meantime, we can remove the remember me checkbox input on this Auth component since there's currently no official support for it.

binajmen commented 2 years ago

Any suggestion to actually implement the "remember me" option ?

FMHO, the check option would suggest to save to localStorage whereas the uncheck option would suggest to save to sessionStorage. A refresh should not destroy a session.

pzduniak commented 2 years ago

We have a client asking us to implement "Remember me", yet it's missing from Supabase. We'd appreciate being able to eg. set a lower session length on demand, or being able to switch between sessionStorage and localStorage on demand.

lumenwrites commented 2 years ago

Are there any updates on this?

"Remember Me" checkbox is required for GDPR compliance.

abrakazinga commented 2 years ago

Would also love an update on this issue.

3v0k4 commented 2 years ago

I believe this issue got stuck because it requires some discussion on how to proceed before writing any code.

I'd like to contribute to Supabase and this would be a nice opportunity. Please let me know if you'd be interested and how you'd like to proceed.