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

fix: don't return session_not_found on logout, make it idempotent #1828

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

hf
Copy link
Contributor

@hf hf commented Nov 1, 2024

If you call POST /logout with a JWT whose session cannot be found in the database, it means that one of these cases occurred:

  • The API call was retried, such as by the user clicking the button multiple times too fast.
  • The user signed out from device A and then proceeds to sign out from device B within the JWT expiry period.
  • The session was deleted outside of the logout flow, such as by manually running a DELETE FROM sessions query.

If this was done, the logout request failed with a session_not_found error code. With this change, the logout request succeeds.

@hf hf requested a review from a team as a code owner November 1, 2024 11:09
@hf hf force-pushed the hf/fix-logout-session-missing branch from c76b5de to 88f9aa1 Compare November 1, 2024 11:11
@hf hf changed the title fix: don't return on logout, make it idempotent fix: don't return session_not_found on logout, make it idempotent Nov 1, 2024
@hf hf force-pushed the hf/fix-logout-session-missing branch 2 times, most recently from 92eab02 to 8172723 Compare November 1, 2024 11:16
@hf hf force-pushed the hf/fix-logout-session-missing branch from 8172723 to e39473f Compare November 1, 2024 12:05
Copy link
Member

@kangmingtay kangmingtay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm ok with the idea of making the logout action idempotent but i'm not a fan of how we handle it in the middleware function conditionally

i'd prefer if we have a new middleware function which maybe calls requireAuthentication internally but ignores the SessionNotFound error

Copy link
Contributor

@cstockton cstockton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe we should keep the behavior as is, first it seems like the most correct behavior. Second and most importantly it's the behavior we have now, it could potentially break application flows that depend on being able to make a positive assertion on if a logout session previously existed or not.

If we do decide to do this, I agree with @kangmingtay that the change should be in Logout. We probably want to add some basic tests too, as implemented this change would just panic with an NPE here for local & others scope instead of returning a better error:
https://github.com/supabase/auth/blob/master/internal/api/logout.go#L52

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants