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: Logout Glitch #2473

Merged
merged 3 commits into from
Oct 30, 2023
Merged

Fix: Logout Glitch #2473

merged 3 commits into from
Oct 30, 2023

Conversation

rajatmohan22
Copy link
Contributor

@rajatmohan22 rajatmohan22 commented Sep 23, 2023

Fixes: #2449

@lindapaiste I noticed your comments on the issue. You mentioned that you're not an expert on cookies, and I'm in the same boat 😄. However, after some investigation, here's what I've discovered:

1. Why doesn't the error occur on localhost?
Ans: Seemingly, browsers have different policies regarding cookies for different domains. Therefore, cookies and session management may behave differently compared to an external server ( This is actually evident if you open the networks tab on both localhost and the prod site and check the status code of the session.) This could lead to differences in how sessions are managed and why the issue doesn't occur locally.

2. How do we fix it?
Ans: Deleting the session. What's happening now is that, on logout, the session Id is changing. However, we want it gone!
Apparently Passport provides another function for this. It is called: session.destroy.

Please check it out, and let me know if any changes is required.

Here is a brief video/gif:

with req.session.destroy
screen-capture (1).webm

with req.logout:
screen-capture (3).webm

Changes:

I have verified that this pull request:

  • has no linting errors (npm run lint)
  • has no test errors (npm run test)
  • is from a uniquely-named feature branch and is up to date with the develop branch.
  • is descriptively named and links to an issue number, i.e. Fixes #123

@rajatmohan22
Copy link
Contributor Author

rajatmohan22 commented Sep 24, 2023

@lindapaiste Have a look at both the videos:

  • Using req.logout alone creates a new session.

  • Using req.session.destroy deletes the session permanently.

@rajatmohan22 rajatmohan22 changed the title Deleting Session After Logout. Fix: Logout Glitch Sep 24, 2023
@lindapaiste
Copy link
Collaborator

Thanks for researching this! At first glance it looks good.

@rajatmohan22
Copy link
Contributor Author

@lindapaiste, this works. I deployed a test app. Repo link: https://github.com/rajatmohan22/AuthTest1.

Additionally, we can include HTML tags to clear the browser cache. Do you want me to do anything else to test it out particularly?

@rajatmohan22
Copy link
Contributor Author

@raclim could you verify this?

Copy link
Collaborator

@raclim raclim left a comment

Choose a reason for hiding this comment

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

Thanks for working on this! I also think this looks good to me for now.

@raclim raclim merged commit a6e1462 into processing:develop Oct 30, 2023
1 check passed
@rajatmohan22
Copy link
Contributor Author

@raclim Thank you! appreciate it.

@rajatmohan22 rajatmohan22 deleted the logoutGlitchFix branch November 2, 2023 02:48
@lindapaiste
Copy link
Collaborator

@raclim We need to roll this back.

I had not run it before (FYI, if I say something like "At first glance it looks good." that usually means I looked at the code but didn't run it). Now that I am running it I am getting a 500 error from the API when logging out, detailed here.

The passport logout function throws an error if there is no req.session. This PR is calling req.session.destroy first and req.logout second. Therefore the logout fails because the session is gone.

@rajatmohan22
Copy link
Contributor Author

Refer to #2473 for detailed error explanations. I'm also super open to providing a line-by-line breakdown. I believe the code is error-free. However, we can also consider using the 'destroy' method without Passport's 'logout' if desired.

@rajatmohan22 rajatmohan22 mentioned this pull request Nov 3, 2023
4 tasks
@raclim
Copy link
Collaborator

raclim commented Nov 3, 2023

After giving another look at it I'm also getting the same 500 error here. Sorry I think I didn't give it as thorough of a glance as I should've at the time!

I'm going to revert this PR for now, and we can take a look at the new one submitted for this.

@raclim raclim mentioned this pull request Nov 3, 2023
4 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.

Login Session Still Persists Despite Logout
3 participants