Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Give the session store more control about creation of sessions. #257

Closed
wants to merge 13 commits into
from

Conversation

Projects
None yet
5 participants

fceller commented Apr 15, 2011

This commit introduces a couple of functions that can optionally be added to a session-store implementation, to allow more control over the sessions.

.isSecretRequired()
.generate(secrect, fingerprint, callback)
.create(sid, callback)
.verify(sid, secrect, fingerprint, callback)

kioopi commented Apr 15, 2011

The changes reflect (some of) the special needs of sessions-stores that need more control about how sessions work.

The store can publish methods to control different parts of session creation:

  • isSecretRequired - if a secret is used for authentication
  • generate - how a session-id is generated
  • create - how the session is created. This enables the session-store to pre-populate session data.
  • verifiy - a way for the store to check if a session is valid.

These functions are all optional. In order to avoid the conditionals checking if these functions exists stubs could be added in lib/middleware/session/store.js

Since the changes are similar to @ncb000gt's changes to suppport more flexible hashing ( #252 ), the style is kept similar.

Member

tj commented Apr 15, 2011

in my opinion most of these should be configurable at the middleware level, unless like you say we specifically need them for a store, although most stores should be able to store json no problem.

@tj tj commented on an outdated diff Apr 15, 2011

lib/middleware/session.js
*/
+// -----------------------------------------------------------------------------
+// Module declarations
+// -----------------------------------------------------------------------------
+
@tj

tj Apr 15, 2011

Member

I dont like stuff like this :p it looks funny, and is not consistent with the rest of the project

@tj tj commented on an outdated diff Apr 15, 2011

lib/middleware/session.js
@@ -183,7 +221,7 @@ exports.ignore = ['/favicon.ico'];
* @api public
*/
-function session(options){
+function session (options){
@tj

tj Apr 15, 2011

Member

please don't alter style...

@tj tj commented on the diff Apr 15, 2011

lib/middleware/session.js
if ('production' == env && store instanceof MemoryStore) {
console.warn(warning);
}
- // ensure secret is present
+ // ensure secret is present if needed
@tj

tj Apr 15, 2011

Member

the hashing secret should always be involved

@tj tj commented on an outdated diff Apr 15, 2011

lib/middleware/session.js
// self-awareness
- if (req.session) return next();
+ if (req.session) { return next(); }

kioopi commented Apr 16, 2011

hey, thanks for the comments, appreciate it. Will try to alter the code accordingly.

Next week we probably will release a store implementation for a session-specific database, that uses features like pre-populating the session object from another database.

Member

tj commented Apr 17, 2011

I'll admit that things are slightly bound to JSON for now, but why not just utilize the store API as-is? what's the point of having the store populate the session, and not just implementing the get() method? not dissing the idea, but I'm hesitant to add features without a good reason

fceller commented Apr 18, 2011

Hi,

we also like to stick to json. Please let me elaborate a bit about the motivation behind the changes we propose. We would like to use the session handling with a secure session store, see https://github.com/triAGENS/SessionVoc-OPEN for instance. This has a few advantages and a few restrictions.

For instance, the session-id can no longer be generated by the "session" function but must be generated by the store itself. Independent of the usage of an external store I believe it is a good idea to allow the store to generate the session id. Generating a random identifier can either block (if using /dev/random) or it is not random (if using /dev/urandom) or it requires an external source of entropy (e. g. some usb device or an external server). So, if one separates the generation of session-identifier from the creating of sessions it would to possible to use the system with the current functionally or to easily modify the session-identifier generator (e.g., make it longer then 24 characters, use an external source for entropy).

If you have an external session database - here I mean a server dealing with the session handling, not a database for storing the sessions -, then it is easily possible to do a cross-technology session management (i. e. have parts of the application in PHP and parts in another language). In this case the user login / login, session timeouts and so on are handled by the session database. To give easy access to these methods it would be nice if the session store can decide to return a sub-class of the session object (I know sub-class is not the correct JavaScript term, but I hope you know what I mean), which provides methods like "login" and "logout". That is the reason we would like to add some callbacks to the store. If these callbacks are not present the currently provided functions are used, but enough hocks would be provided to tweak the session handling to external session stores.

cheers Frank

kioopi commented Apr 18, 2011

I think JSON is just fine for everything javascript.
The idea of having the store pre-fill the session store-side is that it makes it possible to pull data out of a persistent database and just copy it into the session object. The logic of getting the user-data for, say, a customer of an online-store and the last shopping-cart would move into the session-server.
I'm not sure if that is beyond the scope of the connect-session-middleware. Maybe it should live in a fork or some monkey-patchy-thing, but i think these changes add a lot of flexibility. If there are very few use-cases for this kind of flexibility, it might not be worth the overhead, though.

Member

tj commented Apr 18, 2011

I'm fine with the odd hook here and there, or perhaps emitting an even "session loaded" etc to monkey-patch the session object with ad hoc data etc, but in generally I dont want to complicate the middleware much more. Also if needed you can patch connect.session.Session, doesn't really matter as long as you are well aware of your changes. As far as the sid generation it would be a few character change to allow for patching that, but IMO it's still more of a session() option than a session store specific thing

I am looking for a way to generate my own session-ids, but is seems the generator is hardcoded in connect.
Being able to a) override the id generator function or b) provide it as a parameter would be nice.
Otherwise, it would also be nice to be able to pick up the cookie if it already exists with the right name and path, and if the session does not exist generate it with the existing cookie id (I tried setting the cookie on the first request to try to generate the session with my own id, didn't work).

Contributor

jonathanong commented Oct 26, 2013

closing. see #295 (comment)

also, it seems you've commited temporary rebase files. i don't know what's going on here!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment