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/prevent logout in login pages #1889

Merged
merged 1 commit into from
Jun 5, 2024

Conversation

alexanderleegs
Copy link
Contributor

Problem

Possible fix to https://linear.app/ogp/issue/ISOM-1009/sgid-logins-are-failing-silently - cannot verify as issue is hard to reproduce

This PR prevents the calling of the logout endpoints on our login pages, i.e. / and /sgid-redirect - as their interaction could possibly be the source of failed sgid logins. As these pages are also inaccessible if the user is logged in, since they will be redirected to /sites, there is no reason to call /logout from these pages regardless.

Tests

  • Login with sgid
  • Login with email

Copy link
Contributor

@seaerchin seaerchin left a comment

Choose a reason for hiding this comment

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

press x to doubt

if (
error.response &&
error.response.status === 401 &&
!LOGIN_PATHS.includes(pathname)
Copy link
Contributor

Choose a reason for hiding this comment

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

actually on reading this, it looks very suspicous. if i'm on / or /sgid-callback and i get a 401 unauthorized, i'm not logged out because there might be another request in flight to sgid?

Copy link
Contributor Author

@alexanderleegs alexanderleegs May 21, 2024

Choose a reason for hiding this comment

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

My guess is that the unhappy flow on /sgid-callback is -

  1. verify-login is called, and whoami is called, since it's part of the login context
  2. whoami resolves with 401 (expected, since user isn't logged in yet). This triggers logout
  3. verify-login resolves. The cookie with the user data is set
  4. logout resolves. The cookie with user data is removed
  5. User gets redirected to sites. They no longer have a valid session cookie, so they get logged out when whoami triggers again

I think users who aren't affected by this just happen to have steps 3 and 4 swapped.

Logically i don't think it makes sense to call logout from the login pages anyway, since logged in users should be redirected away anyway, and logged out users are already logged out

Copy link
Contributor

Choose a reason for hiding this comment

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

In this case shouldn't verify login be awaited to resolve before whoami is called? Are we able to make it sequential?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this might be possible! I'm opting for the quickest solution here to test it out though, since we can't be sure that this is the actual source of the issue

@kishore03109
Copy link
Contributor

kishore03109 commented May 21, 2024

wait wait sgid had a legit incident, lets not push faulty code here as a knee jerk reaction. well, unless they are not related ah, lmk if I am mistaken

@harishv7
Copy link
Contributor

Don't have full context on the inc, but general thoughts:
Can we identify root cause for the sgid login using traces/logs, or add monitoring to recognise the sgid silent failures before making logic-level changes? For all we know, it might have been just that 1 user, and might have been related to the sgid downtime.

@alexanderleegs
Copy link
Contributor Author

alexanderleegs commented May 21, 2024

Shouldn't be related to the sgid incident - we were having this issue before their spike in cpu util! We've also been having this issue on and off, and sgid + our sgid login endpoints are behaving as expected - the failure seems to be from a generic 401 error from whoami, see here for verify-login succeeding, but the user still being unable to login

My suspicion is because of this strange behaviour on login - notice how we call /verify-login, then
/whoami, but /whoami returns a 401, which triggers the /logout.
Screenshot 2024-05-21 at 9 10 12 PM

Funnily enough this is actually a successful login - whoami gets triggered again later and actually returns 200 this time, so the login was successful

@alexanderleegs alexanderleegs requested a review from seaerchin May 21, 2024 13:29
Copy link
Contributor

@seaerchin seaerchin left a comment

Choose a reason for hiding this comment

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

reading through this, i think it makes sense. if i start in a logged-out state, then it doesn't make sense to call logout (because i'm already logout).

i'm not entirely sure this is the fix going forward but the blast radius is limited here so i'm ok w/ merging this in and trying

@alexanderleegs alexanderleegs merged commit a522ea4 into develop Jun 5, 2024
11 of 12 checks passed
@mergify mergify bot deleted the fix/prevent-logout-in-login-pages branch June 5, 2024 04:48
@alexanderleegs alexanderleegs mentioned this pull request Jun 13, 2024
5 tasks
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.

4 participants