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

Problem using with passport 0.6.0: session.regenerate is not a function #166

Closed
showaltb opened this issue May 21, 2022 · 7 comments
Closed
Labels

Comments

@showaltb
Copy link

I've been using cookie-session in conjunction with passport successfully. But passport-0.6.0 now calls session.regenerate(), which is part of the API for express-session, but not for cookie-session.

I'm not sure exactly how all this works together, but should regenerate() be added to cookie-session or should passport be checking for existence before attempting to use it?

@dougwilson
Copy link
Contributor

Hi @showaltb sorry about that! I took a look and it appears regenerate is meant to generate a new session ID for a session. But this module stores the session in the cookie directly, no session ID. I'm not sure what a regenerate would actually do if added to this module, though. Also adding something to req.session would be a backwards incompatible change, as right now someone could be storing data in that key.

@Ylianst
Copy link

Ylianst commented May 23, 2022

Arg. Passport now assumes that a session has regenerate() and save() methods (see here). As a result, Password now throws an exception when used with cookie-session, (see here).

Would be great if dummy versions of these methods where implemented.

@dougwilson
Copy link
Contributor

Yea, I thought about that too, but it seems like dummy methods would be misleading and then not actually perform what is intended. It likely makes more sense if, for example, Passport wants to use other modules that express-session, then it should probably have some sort of adapter so you can say what kind of session module you are using and it can use the appropriate APIs for that module.

@dougwilson
Copy link
Contributor

It also would mean that if dummy regenerate method was added, then there would no longer be a way for something like passport to detect that the session is not capable of regenerating the session id.

@Ylianst
Copy link

Ylianst commented May 23, 2022

Ok, filed with with Password: jaredhanson/passport#904

@dougwilson
Copy link
Contributor

Cool. I don't want to close this for now as just want to see what is decided around. I forgot to mention as well that adding, even a dummy method, would end up as semver major, as it would now prevent the usage of the key regenerate in the user-set data. And it's a hard thing to work around with a client side store model like this, because you cannot just update the key in your db; the clients have the data blob :(

@dougwilson
Copy link
Contributor

Ok, so I'm going to close this issue has it has been sitting here for a few months. I looked over the linked Passport.js issue and it seems the question was answered in jaredhanson/passport#904 (comment)

The author of Passport.js seems pretty clear that cookie-session was never a supported session to use, which makes sense why no issue was ever noticed in the Passport.js change before it was released. The author outlined their thoughts there about how to add cookie-session as a supported session system to Passport.js as well. I'm not sure if they have made any progress towards it, but if not, I'm sure they would be willing to accept someone who did contribute such a change to Passport.js.

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

No branches or pull requests

3 participants