-
-
Notifications
You must be signed in to change notification settings - Fork 388
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
Race condition on Passport Strategy when using express-session with custom store #146
Comments
Related: expressjs/session#360 |
Hi @pragmaticivan, a quick search in passport shows this is a problem with express-session or passport itself and not each individual strategy. For passport:
The last one suggests there's a fork of passport that handles this, while I don't endorse that it might be something to look into for you, but you should voice your wish for this to be fixed in passport regardless. For express-session, the code suggests that it delays the end of the request until session has been saved. I am unsure as to what you expect to happen in a Strategy (or the hundreds of strategies) implementations. It seems, ultimately, express-session should ensure that the session is saved before completing the request. Edit: I see you've done your research. Sadly i'm not keen to including specific library workarounds in a strategy, this ought to be solved on passport/express-sessions level. |
Hi @panva Thank you very much for your really detailed answer, I really appreciate the time you took providing such a great project. My concern is for anyone that uses Passport.js based on express-session (which seems to be standard). In their doc, they mention that for redirect, you should be performing the I think it should be solved on express-session package, since passport implements session the same way it is implemented int he strategies. |
I understand, but passport doesn't force any one session package, neither does the strategy. So I am unsure how I can help. This issue is the first thing https://github.com/passport-next/passport solved, by explicitly calling save before redirect happens. That being said, I wasn't aware of this and want to thank you for bringing it up. |
Hi, same issue here. @panva are you sure to not implement a fix for this issue? Eventually, can you add an hook to the |
Describe the bug
Using express-session the passport strategy is trying to associate value in the session in memory.
Right after that, it performs a redirect, which doesn't wait for the session to be updated in the store. Causing sometimes to the expected value after the redirect not be available in the store yet.
The express session, for instance, calls
.save
automatically when the request is finished. But recommends calling.save
before redirecting to a different resource. Eg.: https://github.com/expressjs/session#sessionsavecallbackTo Reproduce
It's challenging to reproduce that, because the store connection needs to be experiencing some latency.
I was able to do that by using with postgres + knex as store.
Expected behaviour
A clear and concise description of what you expected to happen.
Environment:
Additional context
Add any other context about the problem here.
The text was updated successfully, but these errors were encountered: