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

Functions that call _removeSession internally do not trigger SIGNED_OUT event when the function fails #853

Closed
2 tasks done
bombillazo opened this issue Feb 15, 2024 · 6 comments · Fixed by #915
Closed
2 tasks done
Labels
bug Something isn't working

Comments

@bombillazo
Copy link
Contributor

bombillazo commented Feb 15, 2024

Bug report

  • I confirm this is a bug with Supabase, not with my own application.
  • I confirm I have searched the Docs, GitHub Discussions, and Discord.

Describe the bug

When using the client and auth functions, some internally call a _removeSession function, presumably to clear the session before attempting any auth-related logic. This effectively clears out the local session data used to keep the user logged in. In many cases the function runs and restores or resets the session data eventually. However, if the function fails, this does not trigger the SIGNED_OUT to notify any even handlers and hooks that the session data is no longer present, causing inconsistent auth data states.

To Reproduce

Steps to reproduce the behavior, please provide code snippets or a repository:

  1. Create a supabase client instance.
  2. In your app, use the auth-helpers-react lib and use the useSession hook to get the session data.
  3. Trigger a function like supabase.auth.resend with invalid params to trigger an error and session remove.
  4. This causes the session data to be erased, but the context data of the useSession hook is not updated and retains the old values.

Expected behavior

Any auth function that fails and deletes the auth session data should trigger a SIGNED_OUT event to notify any other serives.

System information

  • Version of supabase-js: 2.64.2
@bombillazo
Copy link
Contributor Author

Any update on this issue?

@kangmingtay
Copy link
Member

@bombillazo hmm there shouldn't be a session return by useSession when the supabase.auth.resend method is called because no user is actually signed in yet

@bombillazo
Copy link
Contributor Author

bombillazo commented May 9, 2024

We can call resend to change email or phone. I dont expect users to be signed out for this

@kangmingtay
Copy link
Member

@bombillazo yeah we handle that case already by not removing the session if the type passed indicates an email change or phone change

@bombillazo
Copy link
Contributor Author

bombillazo commented May 9, 2024

This issue is more related to what happens when something fails with the auth calls while using the useSession react hook that returns the session. Since this library isn't properly triggering the SIGNED_OUT event, the react hook keeps returning the cached session when an auth call fails and erases the session from the local client.

The PR I created fixes this:
https://github.com/supabase/auth-js/pull/854/files#diff-3522461172efd6058d6b8da62fc2d30d8b524d2b64894ea2c67218c52f7fdff5R1960

kangmingtay added a commit that referenced this issue Jun 7, 2024
## What kind of change does this PR introduce?
* Replaces #854 
* Fixes #853, #904 
* We don't need to remove the existing session prematurely. This causes
some issues when users want to implement some sort of switch-account
functionality since the existing session will always be removed
regardless of whether the signup / sign-in attempt succeeds.
* It's safe to remove `_removeSession` since calling `_saveSession`
multiple times will just replace the existing session
@bombillazo
Copy link
Contributor Author

This is still open and should be fixed with #854

kangmingtay pushed a commit that referenced this issue Oct 14, 2024
## What kind of change does this PR introduce?

This adds the SIGNED_OUT event missing in some logic that clears/logs
out the session.

## What is the current behavior?

#853
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants