Skip to content

Only touch sessions in the store when rolling sessions are enabled #531

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.

Sign up for GitHub

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

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Only touch sessions in the store when rolling sessions are enabled #531

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
cc345a7
Only touch sessions in the store when rolling sessions are enabled
Dec 19, 2017
64ba3e5
Fix tests broken with rolling session change to touch()
Dec 19, 2017
b6fb975
Add test to get coverage back to 100%
Dec 19, 2017
e313529
Add tests to check touch() suppression when rolling is turned off
Dec 20, 2017
abc7b0a
Add option alwaysTouchUnmodified to force unmodified sessions to alwa…
Dec 20, 2017
25021df
Fix logic error in documentation of new option
Dec 20, 2017
8543451
Revert changes made to tests now that existing behaviour is the default
Dec 20, 2017
8d0180b
Move new touching logic into shouldTouch() where it belongs
Jan 5, 2018
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
11 changes: 11 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,6 +256,17 @@ will add an empty Passport object to the session for use after a user is
authenticated, which will be treated as a modification to the session, causing
it to be saved. *This has been fixed in PassportJS 0.3.0*

##### alwaysTouchUnmodified

If set to `true` and the store supports "touching", unmodified sessions are
always touched in the store at the end of each request. If set to `false`,
unmodified sessions are only touched when `rolling` is enabled.

The default value is `true`. This means the expiration date in the cookie can
differ from that in the store when `rolling` is disabled since the cookie
expiration date is only updated if the session is modified but the session in
the store is updated regardless.

##### secret

**Required option**
Expand Down
11 changes: 10 additions & 1 deletion index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ var defer = typeof setImmediate === 'function'
* @param {Boolean} [options.resave] Resave unmodified sessions back to the store
* @param {Boolean} [options.rolling] Enable/disable rolling session expiration
* @param {Boolean} [options.saveUninitialized] Save uninitialized sessions to the store
* @param {Boolean} [options.alwaysTouchUnmodified] Always touch unmodified sessions
* @param {String|Array} [options.secret] Secret for signing session ID
* @param {Object} [options.store=MemoryStore] Session store
* @param {String} [options.unset]
Expand Down Expand Up @@ -110,6 +111,9 @@ function session(options) {
// get the save uninitialized session option
var saveUninitializedSession = opts.saveUninitialized

// get the save uninitialized session option
var alwaysTouchUnmodified = opts.alwaysTouchUnmodified

// get the cookie signing secret
var secret = opts.secret

Expand All @@ -127,6 +131,10 @@ function session(options) {
saveUninitializedSession = true;
}

if (alwaysTouchUnmodified === undefined) {
alwaysTouchUnmodified = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

I forget looking through the comments: is there an intention to make alwaysTouchUnmodifed the default behavior in the next major? I ask because all default options that are Booleans should be false such that undefined and null are stand in values. The only options that default to true are deprecated options, but this doesn't have a deprecation warning on it.

}

if (opts.unset && opts.unset !== 'destroy' && opts.unset !== 'keep') {
throw new TypeError('unset option must be "destroy" or "keep"');
}
Expand Down Expand Up @@ -432,7 +440,8 @@ function session(options) {
return false;
}

return cookieId === req.sessionID && !shouldSave(req);
return cookieId === req.sessionID && !shouldSave(req) &&
(rollingSessions || alwaysTouchUnmodified);
}

// determine if cookie should be set on response
Expand Down
173 changes: 172 additions & 1 deletion test/session.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@

process.env.NO_DEPRECATION = 'express-session';

var after = require('after')
Expand Down Expand Up @@ -1189,6 +1188,178 @@ describe('session()', function(){
})
});

describe('alwaysTouchUnmodified option', function(){
it('should touch unmodified rolling sessions when set', function(done){
var store = new session.MemoryStore()
var server = createServer({ store: store, alwaysTouchUnmodified: true, rolling: true, resave: false })

request(server)
.get('/')
.expect(shouldSetCookie('connect.sid'))
.expect(200, function(err, res){
if (err) return done(err);

var touched = false
var _touch = store.touch;
store.touch = function touch(sid, sess, callback) {
touched = true;
_touch.call(store, sid, sess, callback);
}

var id = sid(res)
store.get(id, function (err, sess) {
if (err) return done(err);

var expires1 = new Date(sess.cookie.expires);
setTimeout(function () {
request(server)
.get('/')
.set('Cookie', cookie(res))
.expect(shouldSetCookie('connect.sid'))
.expect(200, function (err, res) {
if (err) return done(err);

assert.ok(touched);
store.get(id, function (err, sess) {
if (err) return done(err);
var expires2 = new Date(sess.cookie.expires);
assert(expires1.getTime() < expires2.getTime());
done();
});
});
}, 10);
});
});
});

it('should touch unmodified non-rolling sessions when set', function(done){
var store = new session.MemoryStore()
var server = createServer({ store: store, alwaysTouchUnmodified: true, rolling: false, resave: false })

request(server)
.get('/')
.expect(shouldSetCookie('connect.sid'))
.expect(200, function(err, res){
if (err) return done(err);

var touched = false
var _touch = store.touch;
store.touch = function touch(sid, sess, callback) {
touched = true;
_touch.call(store, sid, sess, callback);
}

var id = sid(res)
store.get(id, function (err, sess) {
if (err) return done(err);

var expires1 = new Date(sess.cookie.expires);
setTimeout(function () {
request(server)
.get('/')
.set('Cookie', cookie(res))
.expect(200, function (err, res) {
if (err) return done(err);

assert.ok(touched);
store.get(id, function (err, sess) {
if (err) return done(err);
var expires2 = new Date(sess.cookie.expires);
assert(expires1.getTime() < expires2.getTime());
done();
});
});
}, 10);
});
});
});

it('should touch unmodified rolling sessions when unset', function(done){
var store = new session.MemoryStore()
var server = createServer({ store: store, alwaysTouchUnmodified: false, rolling: true, resave: false })

request(server)
.get('/')
.expect(shouldSetCookie('connect.sid'))
.expect(200, function(err, res){
if (err) return done(err);

var touched = false
var _touch = store.touch;
store.touch = function touch(sid, sess, callback) {
touched = true;
_touch.call(store, sid, sess, callback);
}

var id = sid(res)
store.get(id, function (err, sess) {
if (err) return done(err);

var expires1 = new Date(sess.cookie.expires);
setTimeout(function () {
request(server)
.get('/')
.set('Cookie', cookie(res))
.expect(shouldSetCookie('connect.sid'))
.expect(200, function (err, res) {
if (err) return done(err);

assert.ok(touched);
store.get(id, function (err, sess) {
if (err) return done(err);
var expires2 = new Date(sess.cookie.expires);
assert(expires1.getTime() < expires2.getTime());
done();
});
});
}, 10);
});
});
});

it('should not touch unmodified non-rolling sessions when unset', function(done){
var store = new session.MemoryStore()
var server = createServer({ store: store, alwaysTouchUnmodified: false, rolling: false, resave: false })

request(server)
.get('/')
.expect(shouldSetCookie('connect.sid'))
.expect(200, function(err, res){
if (err) return done(err);

var touched = false
var _touch = store.touch;
store.touch = function touch(sid, sess, callback) {
touched = true;
_touch.call(store, sid, sess, callback);
}

var id = sid(res)
store.get(id, function (err, sess) {
if (err) return done(err);

var expires1 = new Date(sess.cookie.expires);
setTimeout(function () {
request(server)
.get('/')
.set('Cookie', cookie(res))
.expect(200, function (err, res) {
if (err) return done(err);

assert.ok(!touched);
store.get(id, function (err, sess) {
if (err) return done(err);
var expires2 = new Date(sess.cookie.expires);
assert.equal(expires1.getTime(), expires2.getTime());
done();
});
});
}, 10);
});
});
});
});

describe('secret option', function () {
it('should reject empty arrays', function () {
assert.throws(createServer.bind(null, { secret: [] }), /secret option array/);
Expand Down