Skip to content

Enabling stores to opt into seeing the request #712

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

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 39 additions & 4 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -278,6 +278,12 @@ all the elements will be considered when verifying the signature in requests.

The session store instance, defaults to a new `MemoryStore` instance.

##### passReqToStore

Choose a reason for hiding this comment

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

I wonder if it would be better to leave the decision whether passing the request is supported to the store.

Copy link
Author

@boenrobot boenrobot Nov 14, 2019

Choose a reason for hiding this comment

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

I think it is, but the only ways I see are arity and classes with a instanceof check before every call... @dougwilson is against arity, and a new base class would make implementation significantly more complex... also I don't know for sure if instaneof even works on ancient Node versions.

Copy link
Author

@boenrobot boenrobot Nov 15, 2019

Choose a reason for hiding this comment

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

I've checked out node.green and indeed, instanceof is only supported in Node 6.17.1 and up, meaning it's not an option... for now at least.

I have another idea... there could be a new reccomended method that would probe for a feature, and expect a true/false result, and assume false for everything if the method does not exist.

e.g. store.isSupported('req')

That would also address the concern with future changes to the store API - each change would be a new string, with the middleware expected to handle all possible combinations, or at least some documented ones in a known order.

It also enables the options to not be exposed in the request.

@dougwilson Thoughts?


Pass the request object to required and optional store methods.

By default, the request object is not passed to store methods.

##### unset

Control the result of unsetting `req.session` (through `delete`, setting to `null`,
Expand Down Expand Up @@ -348,6 +354,11 @@ req.session.reload(function(err) {
})
```

#### Session.load(sid, callback)
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this was removed from the docs and should not be merged back in again.

Copy link
Author

@boenrobot boenrobot May 15, 2020

Choose a reason for hiding this comment

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

I thought that omission was an oversight, and since I was asked to add docs anyway, I thought I'll add one for this as well.

Why was it removed from the docs though? It's still part of the Store prototype after all. If it's deprecated for whatever reason, shouldn't it be documented as such?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is part of this module, though. Can you link to where it is in source? I see no load method on our prototype, unless I am just looking past it.

Copy link
Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks. Hmm, I wonder why it is not documented... We should get it documented as a separate PR (and add tests if those are missing too), not bundled into this one.

Copy link
Author

Choose a reason for hiding this comment

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

There is one test that uses it (and in effect, covers it) here:
https://github.com/expressjs/session/blob/master/test/session.js#L1467

I modeled the test at
https://github.com/expressjs/session/pull/712/files#diff-306b67b120079e997613897e45b88194R2329
in accordance with that very test.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yea, I didn't say there were no tests, only that documenting it (and adding tests if those are missing too) just needs to be moved into a separate PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, looking at that test, we indeed do not have store.load tests. It would be around the public usage of that method (which is what documenting it in the README is for) and would be similar in scope to the other documented methods like

describe('.reload()', function () {
or
describe('.destroy()', function(){
or
describe('.save()', function () {
etc.

But regardless of the above, I do wonder: why would a store who is opting for support req passing not then take the req as an argument to this load method like it does for all the others?

Copy link
Contributor

Choose a reason for hiding this comment

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

https://github.com/expressjs/session/blob/master/session/store.js#L67

P.S. That would be documented as Store.load, not Session.load. That is why I didn't see it went it went to look, as it's a method on the Store class and not the Session class.


Loads the the session data for the given `sid` from the store and re-populates the
`req.session` object. Once complete, the `callback` will be invoked.

#### Session.save(callback)

Save the session back to the store, replacing the contents on the store with the
Expand Down Expand Up @@ -420,6 +431,14 @@ To get the ID of the loaded session, access the request property
`req.sessionID`. This is simply a read-only value set when a session
is loaded/created.

### req.sessionOptions

To get the options of the session, access the request property
`req.sessionOptions`.

**NOTE** For performance reasons, this is the original global options object.
Changes to it would take effect for all requests in an undefined manner.

## Session Store Implementation

Every session store _must_ be an `EventEmitter` and implement specific
Expand All @@ -441,14 +460,18 @@ For an example implementation view the [connect-redis](http://github.com/visionm
This optional method is used to get all sessions in the store as an array. The
`callback` should be called as `callback(error, sessions)`.

### store.destroy(sid, callback)
### store.destroy(sid, [req], callback)

Choose a reason for hiding this comment

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

I think it might be good to use an object to pass the request to the store to make future changes easier if something else needs to be passed to the store.

Copy link
Author

Choose a reason for hiding this comment

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

That concern is also the other reason I went for arity initially.

With the session object and options now being in the request, I think whatever else is needed should be covered from the request.

Copy link
Contributor

Choose a reason for hiding this comment

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

As we are only really interested in the host could we not pass an object with specific props. This comment can be applied to all the places we pass req.

Copy link
Author

@boenrobot boenrobot May 15, 2020

Choose a reason for hiding this comment

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

I am interested in the hostname for my use case, but I imagine other people might be interested in some other features in the request to make a decision where to go. Maybe you're fragmenting stores by geo IP, maybe you're fragmenting stores by a custom header, maybe you're fragmenting by URL prefix, or by query string. Whatever you need to do, you could do it. Stores should expose a way for developers to return settings based on whatever they need to do.


**Required**

This required method is used to destroy/delete a session from the store given
a session ID (`sid`). The `callback` should be called as `callback(error)` once
the session is destroyed.

The request is passed into the `req` argument only if the session's options
have `passReqToStore` set to `true`. A store wishing to operate in either mode
should check whether the second argument is a function or an object.

### store.clear(callback)

**Optional**
Expand All @@ -463,7 +486,7 @@ This optional method is used to delete all sessions from the store. The
This optional method is used to get the count of all sessions in the store.
The `callback` should be called as `callback(error, len)`.

### store.get(sid, callback)
### store.get(sid, [req], callback)

**Required**

Expand All @@ -474,15 +497,23 @@ The `session` argument should be a session if found, otherwise `null` or
`undefined` if the session was not found (and there was no error). A special
case is made when `error.code === 'ENOENT'` to act like `callback(null, null)`.

### store.set(sid, session, callback)
The request is passed into the `req` argument only if the session's options
have `passReqToStore` set to `true`. A store wishing to operate in either mode
should check whether the second argument is a function or an object.

### store.set(sid, session, [req], callback)

**Required**

This required method is used to upsert a session into the store given a
session ID (`sid`) and session (`session`) object. The callback should be
called as `callback(error)` once the session has been set in the store.

### store.touch(sid, session, callback)
The request is passed into the `req` argument only if the session's options
have `passReqToStore` set to `true`. A store wishing to operate in either mode
should check whether the third argument is a function or an object.

### store.touch(sid, session, [req], callback)

**Recommended**

Expand All @@ -494,6 +525,10 @@ This is primarily used when the store will automatically delete idle sessions
and this method is used to signal to the store the given session is active,
potentially resetting the idle timer.

The request is passed into the `req` argument only if the session's options
have `passReqToStore` set to `true`. A store wishing to operate in either mode
should check whether the third argument is a function or an object.

## Compatible Session Stores

The following modules implement a session store that is compatible with this
Expand Down
55 changes: 49 additions & 6 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ var defer = typeof setImmediate === 'function'
* @param {Boolean} [options.saveUninitialized] Save uninitialized sessions to the store
* @param {String|Array} [options.secret] Secret for signing session ID
* @param {Object} [options.store=MemoryStore] Session store
* @param {Boolean} [options.passReqToStore] Pass the request to store methods
* @param {String} [options.unset]
* @return {Function} middleware
* @public
Expand All @@ -99,6 +100,8 @@ function session(options) {
// get the session store
var store = opts.store || new MemoryStore()

var passReqToStore = opts.passReqToStore;

// get the trust proxy setting
var trustProxy = opts.proxy

Expand Down Expand Up @@ -201,6 +204,27 @@ function session(options) {
return;
}

/**
* Load a `Session` instance via the given `sid`
* and invoke the callback `fn(err, sess)`.
*
* @param {String} sid
* @param {Function} fn
* @api public
*/
store.load = function(sid, fn){
Copy link
Contributor

Choose a reason for hiding this comment

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

yep, totally get it. But please remember that this is adding complexity.

Copy link
Author

Choose a reason for hiding this comment

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

How is it adding complexity? load() already exists in the prototype. I merely moved it from there to here, so that even stores that don't extend from Store can take advantage of load() without having to explicitly add the req to the argument list or doing some uglier hack.

Copy link
Contributor

Choose a reason for hiding this comment

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

This would override any load method or property a store has defined. We need to validate that this will not conflict with any existing store implementations.

Copy link
Author

Choose a reason for hiding this comment

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

You're asking if stores implemented a previously undocumented method in the Store prototype that won't even be there if this PR is merged as is.... ok 😕

I checked whether the following stores define their own load methods (be it as an override to the prototype one or a custom one). Feel free to double check if you don't believe me...

Unless otherwise noted, all stores inherit from the Store prototype (either explicitly or from a supplied object's Store property), but don't implement load of their own.

aerospike-session-store-expressjs - no
cassandra-store - no
cluster-store - no (doesn't inherit from Store prototype in its memory store, and doesn't define a load() method)
connect-arango - no
connect-azuretables - no
connect-cloudant-store - no
connect-couchbase - no
connect-datacache - no
@google-cloud/connect-datastore - no
connect-db2 - no
connect-dynamodb - no
@google-cloud/connect-firestore - no
connect-hazelcast - no
connect-loki - no
connect-memcached - no
connect-memjs - no
connect-ml - no
connect-monetdb - no
connect-mongo - no
connect-mongodb-session - no
connect-mssql - no (side note: Abandoned; Even if there was an issue, users should move away anyway)
connect-pg-simple - no
connect-redis - no
connect-session-firebase - no
connect-session-knex - no
connect-session-sequelize - no
connect-sqlite3 - no
connect-typeorm - no
couchdb-expression - no
documentdb-session - no (Abandoned due to dep deprecation; Even if there was an issue, users should move away anyway)
dynamodb-store - no
express-etcd - no
express-mysql-session - no
express-nedb-session - no
express-oracle-session - no
express-session-cache-manager - no
express-session-etcd3 - no
express-session-level - no
express-session-rsdb - no
express-sessions - no
firestore-store - no
fortune-session - no
hazelcast-store - no
level-session-store - no
lowdb-session-store - no
medea-session-store - no
memorystore - no
mssql-session-store - no
nedb-session-store - no
restsession - no
sequelstore-connect - no
session-file-store - no
session-pouchdb-store - no
session-rethinkdb - no
sessionstore - no
tch-nedb-session - no

Copy link
Contributor

Choose a reason for hiding this comment

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

So, sorry, excuse me if I'm confused is all 😂 . So if none of the stores are implementing this besides what they get from the store base, why perform this override? Why not simply modify the base method?

Copy link
Author

Choose a reason for hiding this comment

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

The only time when load can even be used (with or without this PR) is in a manual call within the request. It seems redundant to require the request for something that can only be done in the request.

Copy link
Contributor

@dougwilson dougwilson May 15, 2020

Choose a reason for hiding this comment

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

I'm unsure what you mean, but it sounds like this is going to introduce some sort of race condition. I didn't realize you were adding a method to the store (effectively a global) while closing over a request... would not a call to req.sessionStore.load by a user then then up with a different internal req reference?

var getCallback = function(err, sess){
if (err) return fn(err);
if (!sess) return fn();
fn(null, store.createSession(req, sess))
};
if (passReqToStore) {
store.get(sid, req, getCallback);
} else {
store.get(sid, getCallback);
}
};

// backwards compatibility for signed cookies
// req.secret is passed from the cookie parser middleware
var secrets = secret || [req.secret];
Expand All @@ -212,6 +236,8 @@ function session(options) {

// expose store
req.sessionStore = store;
// expose session options
req.sessionOptions = opts;

// get the session ID from the cookie
var cookieId = req.sessionID = getcookie(req, name, secrets);
Expand Down Expand Up @@ -303,14 +329,20 @@ function session(options) {
if (shouldDestroy(req)) {
// destroy session
debug('destroying');
store.destroy(req.sessionID, function ondestroy(err) {
var ondestroy = function(err) {
if (err) {
defer(next, err);
}

debug('destroyed');
writeend();
});
}

if (passReqToStore) {
store.destroy(req.sessionID, req, ondestroy);
} else {
store.destroy(req.sessionID, ondestroy);
}

return writetop();
}
Expand Down Expand Up @@ -340,14 +372,20 @@ function session(options) {
} else if (storeImplementsTouch && shouldTouch(req)) {
// store implements touch method
debug('touching');
store.touch(req.sessionID, req.session, function ontouch(err) {
var ontouch = function(err) {
if (err) {
defer(next, err);
}

debug('touched');
writeend();
});
}

if (passReqToStore) {
store.touch(req.sessionID, req.session, req, ontouch);
} else {
store.touch(req.sessionID, req.session, ontouch);
}

return writetop();
}
Expand Down Expand Up @@ -471,7 +509,7 @@ function session(options) {

// generate the session object
debug('fetching %s', req.sessionID);
store.get(req.sessionID, function(err, sess){
var getHandler = function(err, sess){
// error handling
if (err && err.code !== 'ENOENT') {
debug('error %j', err);
Expand All @@ -493,7 +531,12 @@ function session(options) {
}

next()
});
}
if (passReqToStore) {
store.get(req.sessionID, req, getHandler);
} else {
store.get(req.sessionID, getHandler);
}
};
};

Expand Down
21 changes: 17 additions & 4 deletions session/session.js
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,11 @@ defineMethod(Session.prototype, 'resetMaxAge', function resetMaxAge() {
*/

defineMethod(Session.prototype, 'save', function save(fn) {
this.req.sessionStore.set(this.id, this, fn || function(){});
if (this.req.sessionOptions.passReqToStore) {
this.req.sessionStore.set(this.id, this, this.req, fn || function(){});
} else {
this.req.sessionStore.set(this.id, this, fn || function(){});
}
return this;
});

Expand All @@ -89,12 +93,17 @@ defineMethod(Session.prototype, 'reload', function reload(fn) {
var req = this.req
var store = this.req.sessionStore

store.get(this.id, function(err, sess){
var getCallback = function(err, sess){
if (err) return fn(err);
if (!sess) return fn(new Error('failed to load session'));
store.createSession(req, sess);
fn();
});
};
if (this.req.sessionOptions.passReqToStore) {
store.get(this.id, req, getCallback);
} else {
store.get(this.id, getCallback);
}
return this;
});

Expand All @@ -108,7 +117,11 @@ defineMethod(Session.prototype, 'reload', function reload(fn) {

defineMethod(Session.prototype, 'destroy', function destroy(fn) {
delete this.req.session;
this.req.sessionStore.destroy(this.id, fn);
if (this.req.sessionOptions.passReqToStore) {
this.req.sessionStore.destroy(this.id, this.req, fn);
} else {
this.req.sessionStore.destroy(this.id, fn);
}
return this;
});

Expand Down
28 changes: 7 additions & 21 deletions session/store.js
Original file line number Diff line number Diff line change
Expand Up @@ -49,29 +49,15 @@ util.inherits(Store, EventEmitter)

Store.prototype.regenerate = function(req, fn){
var self = this;
this.destroy(req.sessionID, function(err){
var destroyCallback = function(err){
self.generate(req);
fn(err);
});
};

/**
* Load a `Session` instance via the given `sid`
* and invoke the callback `fn(err, sess)`.
*
* @param {String} sid
* @param {Function} fn
* @api public
*/

Store.prototype.load = function(sid, fn){
var self = this;
this.get(sid, function(err, sess){
if (err) return fn(err);
if (!sess) return fn();
var req = { sessionID: sid, sessionStore: self };
fn(null, self.createSession(req, sess))
});
};
if (req.sessionOptions.passReqToStore) {
this.destroy(req.sessionID, req, destroyCallback);
} else {
this.destroy(req.sessionID, destroyCallback);
}
};

/**
Expand Down
7 changes: 4 additions & 3 deletions test/cookie.js
Original file line number Diff line number Diff line change
Expand Up @@ -62,11 +62,12 @@ describe('new Cookie()', function () {
})

it('should set maxAge', function () {
var expires = new Date(Date.now() + 60000)
var now = Date.now()
var expires = new Date(now + 60000)
var cookie = new Cookie({ expires: expires })

assert.ok(expires.getTime() - Date.now() - 1000 <= cookie.maxAge)
assert.ok(expires.getTime() - Date.now() + 1000 >= cookie.maxAge)
assert.ok(expires.getTime() - now - 1000 <= cookie.maxAge)
assert.ok(expires.getTime() - now + 1000 >= cookie.maxAge)
})
})

Expand Down
Loading