session icon indicating copy to clipboard operation
session copied to clipboard

feat: Option to allow regenerate() to preserve old session

Open mccleanp opened this issue 8 years ago • 6 comments

Add preserve option to session.regenerate() function for use cases when it is desirable to regenerate the session, but not destroy the old session immediately. For example when there could be multiple ajax requests inflight with the old session id.

Existing functionality is preserved if the option is omitted. Existing test was improved to cover session destuction and new test was added for the preserve case.

This was motivated by the desire to implement middleware to provide session renewal, as recommended by OWASP

mccleanp avatar Jan 27 '17 16:01 mccleanp

I have pushed another commit to address the comments from @dougwilson. Let me know if you want me to squash the commits.

mccleanp avatar Jan 28 '17 12:01 mccleanp

Hi @mccleanp thanks for updating the PR! What ever happened to the preserve: true above? I don't remember you mentioning that you would be implementing it a different way. The reason I ask is because usually in the JS sense, an option that defaults to false is easy to understand, since null and undefined is falsy, which people associate with default values. This implementation defaults the option to true.

dougwilson avatar Mar 01 '17 03:03 dougwilson

@dougwilson: I am happy to change it back to preserve: true if you prefer. I had selected destroy: false simply because the destroy terminology was already used, while preserve is introducing new terminology.

mccleanp avatar Mar 01 '17 09:03 mccleanp

Yea, if you have a better idea for the terminology, that's great, just the key constraint I'm trying to keep here is that the default value should be false, not true, for better mental mapping of undefined / null.

dougwilson avatar Mar 03 '17 06:03 dougwilson

I just rebased the commits. I'm going to work on cleaning it up further to merge 👍

dougwilson avatar Nov 20 '18 22:11 dougwilson

I currently have a problem is that the previous sessionStore got delete after run regenerate() function.

Not sure is it could be solved with this PR. but I think to let the old sessionStore expire itself by cookie.maxAge make more sense.

max8hine avatar Dec 02 '18 05:12 max8hine