From 7e9b9cf4d7be02428e963fc729496a45baeea608 Mon Sep 17 00:00:00 2001 From: Jared Hanson Date: Tue, 17 May 2022 08:15:54 -0700 Subject: [PATCH 01/28] Regenerate session on login. --- lib/sessionmanager.js | 30 ++++++++++++++++++++---------- 1 file changed, 20 insertions(+), 10 deletions(-) diff --git a/lib/sessionmanager.js b/lib/sessionmanager.js index 3d5c51ca..8647b9cc 100644 --- a/lib/sessionmanager.js +++ b/lib/sessionmanager.js @@ -10,24 +10,34 @@ function SessionManager(options, serializeUser) { } SessionManager.prototype.logIn = function(req, user, cb) { + console.log('SM: logIn'); + var self = this; - this._serializeUser(user, req, function(err, obj) { + req.session.regenerate(function(err) { if (err) { return cb(err); } - // TODO: Error if session isn't available here. - if (!req.session) { - req.session = {}; - } - if (!req.session[self._key]) { - req.session[self._key] = {}; - } - req.session[self._key].user = obj; - cb(); + + self._serializeUser(user, req, function(err, obj) { + if (err) { + return cb(err); + } + // TODO: Error if session isn't available here. + if (!req.session) { + req.session = {}; + } + if (!req.session[self._key]) { + req.session[self._key] = {}; + } + req.session[self._key].user = obj; + cb(); + }); }); } SessionManager.prototype.logOut = function(req, cb) { + console.log('SM: logOut'); + if (req.session && req.session[this._key]) { delete req.session[this._key].user; } From a7513c4d8175b4485178f14c95d93a9f3d427c2a Mon Sep 17 00:00:00 2001 From: Jared Hanson Date: Tue, 17 May 2022 08:29:59 -0700 Subject: [PATCH 02/28] Fix tests. --- test/http/request.test.js | 31 ++++++++++++++++++++++++++++--- 1 file changed, 28 insertions(+), 3 deletions(-) diff --git a/test/http/request.test.js b/test/http/request.test.js index 29137fd2..251b7e1c 100644 --- a/test/http/request.test.js +++ b/test/http/request.test.js @@ -202,7 +202,11 @@ describe('http.ServerRequest', function() { req._passport = {}; req._passport.instance = passport; req._sessionManager = passport._sm; - req.session = {}; + req.session = { id: '1' }; + req.session.regenerate = function(cb) { + req.session = { id: '2' }; + process.nextTick(cb); + } var error; @@ -224,6 +228,10 @@ describe('http.ServerRequest', function() { expect(req.isUnauthenticated()).to.be.false; }); + it('should regenerate session', function() { + expect(req.session.id).to.equal('2'); + }); + it('should set user', function() { expect(req.user).to.be.an('object'); expect(req.user.id).to.equal('1'); @@ -248,7 +256,11 @@ describe('http.ServerRequest', function() { req._passport = {}; req._passport.instance = passport; req._sessionManager = passport._sm; - req.session = {}; + req.session = { id: '1' }; + req.session.regenerate = function(cb) { + req.session = { id: '2' }; + process.nextTick(cb); + } req._userProperty = 'currentUser'; var error; @@ -271,6 +283,10 @@ describe('http.ServerRequest', function() { expect(req.isUnauthenticated()).to.be.false; }); + it('should regenerate session', function() { + expect(req.session.id).to.equal('2'); + }); + it('should not set user', function() { expect(req.user).to.be.undefined; }); @@ -299,8 +315,13 @@ describe('http.ServerRequest', function() { req._passport = {}; req._passport.instance = passport; req._sessionManager = passport._sm; - req.session = {}; + req.session = { id: '1' }; req.session['passport'] = {}; + req.session.regenerate = function(cb) { + req.session = { id: '2' }; + req.session['passport'] = {}; + process.nextTick(cb); + } var error; @@ -323,6 +344,10 @@ describe('http.ServerRequest', function() { expect(req.isUnauthenticated()).to.be.true; }); + it('should regenerate session', function() { + expect(req.session.id).to.equal('2'); + }); + it('should not set user', function() { expect(req.user).to.be.null; }); From a77271f55f045bd4fd2578a953256406b3621721 Mon Sep 17 00:00:00 2001 From: Jared Hanson Date: Wed, 18 May 2022 07:06:56 -0700 Subject: [PATCH 03/28] Regenerate session in logout. --- lib/http/request.js | 6 ++++-- lib/sessionmanager.js | 16 ++++++++++----- test/http/request.test.js | 42 +++++++++++++++++++++++++++++++++------ 3 files changed, 51 insertions(+), 13 deletions(-) diff --git a/lib/http/request.js b/lib/http/request.js index 01985393..9634dbd1 100644 --- a/lib/http/request.js +++ b/lib/http/request.js @@ -51,12 +51,14 @@ req.logIn = function(user, options, done) { * @api public */ req.logout = -req.logOut = function() { +req.logOut = function(done) { var property = this._userProperty || 'user'; this[property] = null; if (this._sessionManager) { - this._sessionManager.logOut(this); + if (typeof done != 'function') { throw new Error('req#logout requires a callback function'); } + + this._sessionManager.logOut(this, done); } }; diff --git a/lib/sessionmanager.js b/lib/sessionmanager.js index 8647b9cc..0c246be7 100644 --- a/lib/sessionmanager.js +++ b/lib/sessionmanager.js @@ -38,11 +38,17 @@ SessionManager.prototype.logIn = function(req, user, cb) { SessionManager.prototype.logOut = function(req, cb) { console.log('SM: logOut'); - if (req.session && req.session[this._key]) { - delete req.session[this._key].user; - } - - cb && cb(); + var self = this; + req.session.regenerate(function(err) { + if (err) { + return cb(err); + } + + if (req.session && req.session[self._key]) { + delete req.session[self._key].user; + } + cb && cb(); + }); } diff --git a/test/http/request.test.js b/test/http/request.test.js index 251b7e1c..86401e7b 100644 --- a/test/http/request.test.js +++ b/test/http/request.test.js @@ -410,11 +410,26 @@ describe('http.ServerRequest', function() { req._passport = {}; req._passport.instance = passport; req._sessionManager = passport._sm; - req.session = {}; + req.session = { id: '1' }; req.session['passport'] = {}; req.session['passport'].user = '1'; + req.session.regenerate = function(cb) { + req.session = { id: '2' }; + process.nextTick(cb); + } - req.logout(); + var error; + + before(function(done) { + req.logout(function(err) { + error = err; + done(); + }); + }); + + it('should not error', function() { + expect(error).to.be.undefined; + }); it('should not be authenticated', function() { expect(req.isAuthenticated()).to.be.false; @@ -426,7 +441,7 @@ describe('http.ServerRequest', function() { }); it('should clear serialized user', function() { - expect(req.session['passport'].user).to.be.undefined; + expect(req.session['passport']).to.be.undefined; }); }); @@ -442,11 +457,26 @@ describe('http.ServerRequest', function() { req._passport.instance = passport; req._userProperty = 'currentUser'; req._sessionManager = passport._sm; - req.session = {}; + req.session = { id: '1' }; req.session['passport'] = {}; req.session['passport'].user = '1'; + req.session.regenerate = function(cb) { + req.session = { id: '2' }; + process.nextTick(cb); + } - req.logout(); + var error; + + before(function(done) { + req.logout(function(err) { + error = err; + done(); + }); + }); + + it('should not error', function() { + expect(error).to.be.undefined; + }); it('should not be authenticated', function() { expect(req.isAuthenticated()).to.be.false; @@ -458,7 +488,7 @@ describe('http.ServerRequest', function() { }); it('should clear serialized user', function() { - expect(req.session['passport'].user).to.be.undefined; + expect(req.session['passport']).to.be.undefined; }); }); From 9cde80820fe37b1eb27001b98f97ecbad40fc83e Mon Sep 17 00:00:00 2001 From: Jared Hanson Date: Wed, 18 May 2022 07:28:56 -0700 Subject: [PATCH 04/28] Save session on login before invoking callback. --- lib/sessionmanager.js | 13 ++++++++++++- test/http/request.test.js | 8 +++++++- 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/lib/sessionmanager.js b/lib/sessionmanager.js index 0c246be7..6a897f5e 100644 --- a/lib/sessionmanager.js +++ b/lib/sessionmanager.js @@ -13,6 +13,9 @@ SessionManager.prototype.logIn = function(req, user, cb) { console.log('SM: logIn'); var self = this; + + // regenerate the session, which is good practice to help + // guard against forms of session fixation req.session.regenerate(function(err) { if (err) { return cb(err); @@ -29,8 +32,16 @@ SessionManager.prototype.logIn = function(req, user, cb) { if (!req.session[self._key]) { req.session[self._key] = {}; } + // store user information in session, typically a user id req.session[self._key].user = obj; - cb(); + // save the session before redirection to ensure page + // load does not happen before session is saved + req.session.save(function(err) { + if (err) { + return cb(err); + } + cb(); + }); }); }); } diff --git a/test/http/request.test.js b/test/http/request.test.js index 86401e7b..d13b39a7 100644 --- a/test/http/request.test.js +++ b/test/http/request.test.js @@ -205,8 +205,11 @@ describe('http.ServerRequest', function() { req.session = { id: '1' }; req.session.regenerate = function(cb) { req.session = { id: '2' }; + req.session.save = function(cb) { + process.nextTick(cb); + }; process.nextTick(cb); - } + }; var error; @@ -259,6 +262,9 @@ describe('http.ServerRequest', function() { req.session = { id: '1' }; req.session.regenerate = function(cb) { req.session = { id: '2' }; + req.session.save = function(cb) { + process.nextTick(cb); + }; process.nextTick(cb); } req._userProperty = 'currentUser'; From c018deaaa2d04954d2362597aefec33757cf97a3 Mon Sep 17 00:00:00 2001 From: Jared Hanson Date: Wed, 18 May 2022 07:47:37 -0700 Subject: [PATCH 05/28] Clear user from old session on logout. --- lib/sessionmanager.js | 26 +++++++++++++++++++------- 1 file changed, 19 insertions(+), 7 deletions(-) diff --git a/lib/sessionmanager.js b/lib/sessionmanager.js index 6a897f5e..edd2535f 100644 --- a/lib/sessionmanager.js +++ b/lib/sessionmanager.js @@ -50,15 +50,27 @@ SessionManager.prototype.logOut = function(req, cb) { console.log('SM: logOut'); var self = this; - req.session.regenerate(function(err) { + + // clear the user from the session object and save. + // this will ensure that re-using the old session id + // does not have a logged in user + if (req.session && req.session[this._key]) { + delete req.session[this._key].user; + } + + req.session.save(function(err) { if (err) { - return cb(err); - } - - if (req.session && req.session[self._key]) { - delete req.session[self._key].user; + return cb(err) } - cb && cb(); + + // regenerate the session, which is good practice to help + // guard against forms of session fixation + req.session.regenerate(function(err) { + if (err) { + return cb(err); + } + cb && cb(); + }); }); } From fa80b204fb404c124e7382c4f8605206b50ea393 Mon Sep 17 00:00:00 2001 From: Jared Hanson Date: Wed, 18 May 2022 08:08:07 -0700 Subject: [PATCH 06/28] Fix tests. --- test/http/request.test.js | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/test/http/request.test.js b/test/http/request.test.js index d13b39a7..c6521a1e 100644 --- a/test/http/request.test.js +++ b/test/http/request.test.js @@ -419,10 +419,14 @@ describe('http.ServerRequest', function() { req.session = { id: '1' }; req.session['passport'] = {}; req.session['passport'].user = '1'; + req.session.save = function(cb) { + expect(req.session['passport'].user).to.be.undefined; + process.nextTick(cb); + }; req.session.regenerate = function(cb) { req.session = { id: '2' }; process.nextTick(cb); - } + }; var error; @@ -466,10 +470,14 @@ describe('http.ServerRequest', function() { req.session = { id: '1' }; req.session['passport'] = {}; req.session['passport'].user = '1'; + req.session.save = function(cb) { + expect(req.session['passport'].user).to.be.undefined; + process.nextTick(cb); + }; req.session.regenerate = function(cb) { req.session = { id: '2' }; process.nextTick(cb); - } + }; var error; From fa70e2fb31ff92e5fc3039b0e5e8b1f67f5cd944 Mon Sep 17 00:00:00 2001 From: Jared Hanson Date: Wed, 18 May 2022 08:27:52 -0700 Subject: [PATCH 07/28] Error if session isn't available. --- lib/sessionmanager.js | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/lib/sessionmanager.js b/lib/sessionmanager.js index edd2535f..c39bc6e5 100644 --- a/lib/sessionmanager.js +++ b/lib/sessionmanager.js @@ -12,6 +12,8 @@ function SessionManager(options, serializeUser) { SessionManager.prototype.logIn = function(req, user, cb) { console.log('SM: logIn'); + if (!req.session) { return cb(new Error('Login sessions require session support. Did you forget to use `express-session` middleware?')); } + var self = this; // regenerate the session, which is good practice to help @@ -25,10 +27,6 @@ SessionManager.prototype.logIn = function(req, user, cb) { if (err) { return cb(err); } - // TODO: Error if session isn't available here. - if (!req.session) { - req.session = {}; - } if (!req.session[self._key]) { req.session[self._key] = {}; } From 88c1f1bc7fc29baceb119fbcacaa5d9d89192f6c Mon Sep 17 00:00:00 2001 From: Jared Hanson Date: Wed, 18 May 2022 08:34:40 -0700 Subject: [PATCH 08/28] Handle logout without session manager. --- lib/http/request.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lib/http/request.js b/lib/http/request.js index 9634dbd1..58efb3aa 100644 --- a/lib/http/request.js +++ b/lib/http/request.js @@ -59,6 +59,8 @@ req.logOut = function(done) { if (typeof done != 'function') { throw new Error('req#logout requires a callback function'); } this._sessionManager.logOut(this, done); + } else { + done && done(); } }; From 71c54f6169a8d5d5bda0c8559e0889d67128609f Mon Sep 17 00:00:00 2001 From: Jared Hanson Date: Thu, 19 May 2022 06:48:32 -0700 Subject: [PATCH 09/28] Add test. --- test/http/request.test.js | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/test/http/request.test.js b/test/http/request.test.js index c6521a1e..c845ed8e 100644 --- a/test/http/request.test.js +++ b/test/http/request.test.js @@ -525,6 +525,28 @@ describe('http.ServerRequest', function() { }); }); + describe('existing session, but not passing a callback argument', function() { + var passport = new Passport(); + passport.serializeUser(function(user, done) { + done(null, user.id); + }); + + var req = new Object(); + req.logout = request.logout; + req._passport = {}; + req._passport.instance = passport; + req._sessionManager = passport._sm; + req.session = {}; + req.session['passport'] = {}; + req.session['passport'].user = '1'; + + it('should throw an exception', function() { + expect(function() { + req.logout(); + }).to.throw(Error, 'req#logout requires a callback function'); + }); + }); + }); From cc7606c8eefb0a681db161c11351398c9507779c Mon Sep 17 00:00:00 2001 From: Jared Hanson Date: Thu, 19 May 2022 06:52:24 -0700 Subject: [PATCH 10/28] Add tests. --- test/http/request.test.js | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/test/http/request.test.js b/test/http/request.test.js index c845ed8e..eb2d8436 100644 --- a/test/http/request.test.js +++ b/test/http/request.test.js @@ -525,6 +525,36 @@ describe('http.ServerRequest', function() { }); }); + describe('existing session, without passport.initialize() middleware, and invoked with a callback', function() { + var req = new Object(); + req.logout = request.logout; + req.isAuthenticated = request.isAuthenticated; + req.isUnauthenticated = request.isUnauthenticated; + req.user = { id: '1', username: 'root' }; + + var error; + + before(function(done) { + req.logout(function(err) { + error = err; + done(); + }); + }); + + it('should not error', function() { + expect(error).to.be.undefined; + }); + + it('should not be authenticated', function() { + expect(req.isAuthenticated()).to.be.false; + expect(req.isUnauthenticated()).to.be.true; + }); + + it('should clear user', function() { + expect(req.user).to.be.null; + }); + }); + describe('existing session, but not passing a callback argument', function() { var passport = new Passport(); passport.serializeUser(function(user, done) { From ee0bf811aba5dcb25310b6eeb5b4a43d6ec8d86f Mon Sep 17 00:00:00 2001 From: Jared Hanson Date: Thu, 19 May 2022 06:56:12 -0700 Subject: [PATCH 11/28] Add tests. --- test/http/request.test.js | 55 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 55 insertions(+) diff --git a/test/http/request.test.js b/test/http/request.test.js index eb2d8436..72568e16 100644 --- a/test/http/request.test.js +++ b/test/http/request.test.js @@ -308,6 +308,61 @@ describe('http.ServerRequest', function() { }); }); + describe('encountering an error when regenerating session', function() { + var passport = new Passport(); + passport.serializeUser(function(user, done) { + done(null, user.id); + }); + + var req = new Object(); + req.login = request.login; + req.isAuthenticated = request.isAuthenticated; + req.isUnauthenticated = request.isUnauthenticated; + req._passport = {}; + req._passport.instance = passport; + req._sessionManager = passport._sm; + req.session = { id: '1' }; + req.session['passport'] = {}; + req.session.regenerate = function(cb) { + process.nextTick(function(){ + cb(new Error('something went wrong')); + }) + } + + var error; + + before(function(done) { + var user = { id: '1', username: 'root' }; + + req.login(user, function(err) { + error = err; + done(); + }); + }); + + it('should error', function() { + expect(error).to.be.an.instanceOf(Error); + expect(error.message).to.equal('something went wrong'); + }); + + it('should not be authenticated', function() { + expect(req.isAuthenticated()).to.be.false; + expect(req.isUnauthenticated()).to.be.true; + }); + + it('should not regenerate session', function() { + expect(req.session.id).to.equal('1'); + }); + + it('should not set user', function() { + expect(req.user).to.be.null; + }); + + it('should not serialize user', function() { + expect(req.session['passport'].user).to.be.undefined; + }); + }); + describe('encountering an error when serializing to session', function() { var passport = new Passport(); passport.serializeUser(function(user, done) { From cfa8259dc232e053d3d78f25659dae83eca3a813 Mon Sep 17 00:00:00 2001 From: Jared Hanson Date: Thu, 19 May 2022 07:01:08 -0700 Subject: [PATCH 12/28] Add tests. --- test/http/request.test.js | 60 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 60 insertions(+) diff --git a/test/http/request.test.js b/test/http/request.test.js index 72568e16..79e93db2 100644 --- a/test/http/request.test.js +++ b/test/http/request.test.js @@ -418,6 +418,66 @@ describe('http.ServerRequest', function() { }); }); + describe('encountering an error when saving session', function() { + var passport = new Passport(); + passport.serializeUser(function(user, done) { + done(null, user.id); + }); + + var req = new Object(); + req.login = request.login; + req.isAuthenticated = request.isAuthenticated; + req.isUnauthenticated = request.isUnauthenticated; + req._passport = {}; + req._passport.instance = passport; + req._sessionManager = passport._sm; + req.session = { id: '1' }; + req.session['passport'] = {}; + req.session.regenerate = function(cb) { + req.session = { id: '2' }; + req.session['passport'] = {}; + req.session.save = function(cb) { + process.nextTick(function(){ + cb(new Error('something went wrong')); + }); + }; + process.nextTick(cb); + } + + var error; + + before(function(done) { + var user = { id: '1', username: 'root' }; + + req.login(user, function(err) { + error = err; + done(); + }); + }); + + it('should error', function() { + expect(error).to.be.an.instanceOf(Error); + expect(error.message).to.equal('something went wrong'); + }); + + it('should not be authenticated', function() { + expect(req.isAuthenticated()).to.be.false; + expect(req.isUnauthenticated()).to.be.true; + }); + + it('should not regenerate session', function() { + expect(req.session.id).to.equal('2'); + }); + + it('should not set user', function() { + expect(req.user).to.be.null; + }); + + it('should not serialize user', function() { + expect(req.session['passport'].user).to.equal('1'); + }); + }); + /* describe('establishing a session, without passport.initialize() middleware', function() { var req = new Object(); From b395106fef73f2d06cb8cbfed936bcf5f9713e4b Mon Sep 17 00:00:00 2001 From: Jared Hanson Date: Thu, 19 May 2022 07:06:15 -0700 Subject: [PATCH 13/28] Clean up tests. --- test/http/request.test.js | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/test/http/request.test.js b/test/http/request.test.js index 79e93db2..10863824 100644 --- a/test/http/request.test.js +++ b/test/http/request.test.js @@ -380,7 +380,6 @@ describe('http.ServerRequest', function() { req.session['passport'] = {}; req.session.regenerate = function(cb) { req.session = { id: '2' }; - req.session['passport'] = {}; process.nextTick(cb); } @@ -414,7 +413,7 @@ describe('http.ServerRequest', function() { }); it('should not serialize user', function() { - expect(req.session['passport'].user).to.be.undefined; + expect(req.session['passport']).to.be.undefined; }); }); @@ -435,7 +434,6 @@ describe('http.ServerRequest', function() { req.session['passport'] = {}; req.session.regenerate = function(cb) { req.session = { id: '2' }; - req.session['passport'] = {}; req.session.save = function(cb) { process.nextTick(function(){ cb(new Error('something went wrong')); From 30016547df2d1b89de965f2cf3efab04fdb6bfe8 Mon Sep 17 00:00:00 2001 From: Jared Hanson Date: Thu, 19 May 2022 07:13:15 -0700 Subject: [PATCH 14/28] Add tests. --- test/http/request.test.js | 53 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+) diff --git a/test/http/request.test.js b/test/http/request.test.js index 10863824..138788ac 100644 --- a/test/http/request.test.js +++ b/test/http/request.test.js @@ -668,6 +668,59 @@ describe('http.ServerRequest', function() { }); }); + describe('encountering an error saving existing session', function() { + var passport = new Passport(); + + var req = new Object(); + req.logout = request.logout; + req.isAuthenticated = request.isAuthenticated; + req.isUnauthenticated = request.isUnauthenticated; + req.user = { id: '1', username: 'root' }; + req._passport = {}; + req._passport.instance = passport; + req._sessionManager = passport._sm; + req.session = { id: '1' }; + req.session['passport'] = {}; + req.session['passport'].user = '1'; + req.session.save = function(cb) { + expect(req.session['passport'].user).to.be.undefined; + process.nextTick(function() { + cb(new Error('something went wrong')); + }); + }; + req.session.regenerate = function(cb) { + req.session = { id: '2' }; + process.nextTick(cb); + }; + + var error; + + before(function(done) { + req.logout(function(err) { + error = err; + done(); + }); + }); + + it('should error', function() { + expect(error).to.be.an.instanceOf(Error); + expect(error.message).to.equal('something went wrong'); + }); + + it('should not be authenticated', function() { + expect(req.isAuthenticated()).to.be.false; + expect(req.isUnauthenticated()).to.be.true; + }); + + it('should clear user', function() { + expect(req.user).to.be.null; + }); + + it('should clear serialized user', function() { + expect(req.session['passport'].user).to.be.undefined; + }); + }); + describe('existing session, but not passing a callback argument', function() { var passport = new Passport(); passport.serializeUser(function(user, done) { From 80cc4e367f733f34c1d5a754e402e7e27f8a1295 Mon Sep 17 00:00:00 2001 From: Jared Hanson Date: Thu, 19 May 2022 07:18:33 -0700 Subject: [PATCH 15/28] Add tests. --- test/http/request.test.js | 52 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 52 insertions(+) diff --git a/test/http/request.test.js b/test/http/request.test.js index 138788ac..2337b147 100644 --- a/test/http/request.test.js +++ b/test/http/request.test.js @@ -721,6 +721,58 @@ describe('http.ServerRequest', function() { }); }); + describe('encountering an error regenerating session', function() { + var passport = new Passport(); + + var req = new Object(); + req.logout = request.logout; + req.isAuthenticated = request.isAuthenticated; + req.isUnauthenticated = request.isUnauthenticated; + req.user = { id: '1', username: 'root' }; + req._passport = {}; + req._passport.instance = passport; + req._sessionManager = passport._sm; + req.session = { id: '1' }; + req.session['passport'] = {}; + req.session['passport'].user = '1'; + req.session.save = function(cb) { + expect(req.session['passport'].user).to.be.undefined; + process.nextTick(cb); + }; + req.session.regenerate = function(cb) { + process.nextTick(function() { + cb(new Error('something went wrong')); + }); + }; + + var error; + + before(function(done) { + req.logout(function(err) { + error = err; + done(); + }); + }); + + it('should error', function() { + expect(error).to.be.an.instanceOf(Error); + expect(error.message).to.equal('something went wrong'); + }); + + it('should not be authenticated', function() { + expect(req.isAuthenticated()).to.be.false; + expect(req.isUnauthenticated()).to.be.true; + }); + + it('should clear user', function() { + expect(req.user).to.be.null; + }); + + it('should clear serialized user', function() { + expect(req.session['passport'].user).to.be.undefined; + }); + }); + describe('existing session, but not passing a callback argument', function() { var passport = new Passport(); passport.serializeUser(function(user, done) { From 294f22c4d9b9b8f6e1b80d4045fbf7bd4ec1e4d9 Mon Sep 17 00:00:00 2001 From: Jared Hanson Date: Thu, 19 May 2022 07:24:21 -0700 Subject: [PATCH 16/28] Better session detection and exceptions. --- lib/sessionmanager.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/sessionmanager.js b/lib/sessionmanager.js index c39bc6e5..0c43e5b3 100644 --- a/lib/sessionmanager.js +++ b/lib/sessionmanager.js @@ -47,12 +47,14 @@ SessionManager.prototype.logIn = function(req, user, cb) { SessionManager.prototype.logOut = function(req, cb) { console.log('SM: logOut'); + if (!req.session) { return cb(new Error('Login sessions require session support. Did you forget to use `express-session` middleware?')); } + var self = this; // clear the user from the session object and save. // this will ensure that re-using the old session id // does not have a logged in user - if (req.session && req.session[this._key]) { + if (req.session[this._key]) { delete req.session[this._key].user; } From c1991cf745f166efea65dd35bff818d5d35c38ed Mon Sep 17 00:00:00 2001 From: Jared Hanson Date: Thu, 19 May 2022 07:27:48 -0700 Subject: [PATCH 17/28] Add tests. --- test/http/request.test.js | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/test/http/request.test.js b/test/http/request.test.js index 2337b147..8075faea 100644 --- a/test/http/request.test.js +++ b/test/http/request.test.js @@ -513,6 +513,35 @@ describe('http.ServerRequest', function() { }); }); + describe('establishing a session without session support', function() { + var passport = new Passport(); + passport.serializeUser(function(user, done) { + done(null, user.id); + }); + + var req = new Object(); + req.login = request.login; + req._passport = {}; + req._passport.instance = passport; + req._sessionManager = passport._sm; + + var error; + + before(function(done) { + var user = { id: '1', username: 'root' }; + + req.login(user, function(err) { + error = err; + done(); + }); + }); + + it('should error', function() { + expect(error).to.be.an.instanceOf(Error); + expect(error.message).to.equal('Login sessions require session support. Did you forget to use `express-session` middleware?'); + }); + }); + }); From 8825a9a0cd129a332f78124af6268af1f67fdc1b Mon Sep 17 00:00:00 2001 From: Jared Hanson Date: Thu, 19 May 2022 07:29:21 -0700 Subject: [PATCH 18/28] Add tests. --- test/http/request.test.js | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/test/http/request.test.js b/test/http/request.test.js index 8075faea..1f7f46f5 100644 --- a/test/http/request.test.js +++ b/test/http/request.test.js @@ -824,6 +824,33 @@ describe('http.ServerRequest', function() { }); }); + describe('without session support', function() { + var passport = new Passport(); + passport.serializeUser(function(user, done) { + done(null, user.id); + }); + + var req = new Object(); + req.logout = request.logout; + req._passport = {}; + req._passport.instance = passport; + req._sessionManager = passport._sm; + + var error; + + before(function(done) { + req.logout(function(err) { + error = err; + done(); + }); + }); + + it('should error', function() { + expect(error).to.be.an.instanceOf(Error); + expect(error.message).to.equal('Login sessions require session support. Did you forget to use `express-session` middleware?'); + }); + }); + }); From e69834e070662e8972e9eca07cbebf0338cfd226 Mon Sep 17 00:00:00 2001 From: Jared Hanson Date: Thu, 19 May 2022 08:39:31 -0700 Subject: [PATCH 19/28] Add optional options to login and logout. --- lib/http/request.js | 8 +++++++- lib/sessionmanager.js | 16 ++++++++++++++-- 2 files changed, 21 insertions(+), 3 deletions(-) diff --git a/lib/http/request.js b/lib/http/request.js index 58efb3aa..9bb27e42 100644 --- a/lib/http/request.js +++ b/lib/http/request.js @@ -51,7 +51,13 @@ req.logIn = function(user, options, done) { * @api public */ req.logout = -req.logOut = function(done) { +req.logOut = function(options, done) { + if (typeof options == 'function') { + done = options; + options = {}; + } + options = options || {}; + var property = this._userProperty || 'user'; this[property] = null; diff --git a/lib/sessionmanager.js b/lib/sessionmanager.js index 0c43e5b3..30323033 100644 --- a/lib/sessionmanager.js +++ b/lib/sessionmanager.js @@ -9,7 +9,13 @@ function SessionManager(options, serializeUser) { this._serializeUser = serializeUser; } -SessionManager.prototype.logIn = function(req, user, cb) { +SessionManager.prototype.logIn = function(req, user, options, cb) { + if (typeof options == 'function') { + cb = options; + options = {}; + } + options = options || {}; + console.log('SM: logIn'); if (!req.session) { return cb(new Error('Login sessions require session support. Did you forget to use `express-session` middleware?')); } @@ -44,7 +50,13 @@ SessionManager.prototype.logIn = function(req, user, cb) { }); } -SessionManager.prototype.logOut = function(req, cb) { +SessionManager.prototype.logOut = function(req, options, cb) { + if (typeof options == 'function') { + cb = options; + options = {}; + } + options = options || {}; + console.log('SM: logOut'); if (!req.session) { return cb(new Error('Login sessions require session support. Did you forget to use `express-session` middleware?')); } From a349c2bc32e4eba793500bb7b6125f07e0014a99 Mon Sep 17 00:00:00 2001 From: Jared Hanson Date: Thu, 19 May 2022 09:30:59 -0700 Subject: [PATCH 20/28] Add option to keep session data. --- lib/http/request.js | 2 +- lib/sessionmanager.js | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/lib/http/request.js b/lib/http/request.js index 9bb27e42..705110fd 100644 --- a/lib/http/request.js +++ b/lib/http/request.js @@ -36,7 +36,7 @@ req.logIn = function(user, options, done) { if (typeof done != 'function') { throw new Error('req#login requires a callback function'); } var self = this; - this._sessionManager.logIn(this, user, function(err) { + this._sessionManager.logIn(this, user, options, function(err) { if (err) { self[property] = null; return done(err); } done(); }); diff --git a/lib/sessionmanager.js b/lib/sessionmanager.js index 30323033..193f11b4 100644 --- a/lib/sessionmanager.js +++ b/lib/sessionmanager.js @@ -21,6 +21,7 @@ SessionManager.prototype.logIn = function(req, user, options, cb) { if (!req.session) { return cb(new Error('Login sessions require session support. Did you forget to use `express-session` middleware?')); } var self = this; + var prevSession = req.session; // regenerate the session, which is good practice to help // guard against forms of session fixation @@ -33,6 +34,9 @@ SessionManager.prototype.logIn = function(req, user, options, cb) { if (err) { return cb(err); } + if (options.keepSessionData) { + Object.assign(req.session, prevSession); + } if (!req.session[self._key]) { req.session[self._key] = {}; } From 17111d76972462eab90a1ea5dcd9211ceb93c7b0 Mon Sep 17 00:00:00 2001 From: Jared Hanson Date: Thu, 19 May 2022 09:40:59 -0700 Subject: [PATCH 21/28] Add option to keep session data on logout. --- lib/http/request.js | 2 +- lib/sessionmanager.js | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/lib/http/request.js b/lib/http/request.js index 705110fd..cdb5432f 100644 --- a/lib/http/request.js +++ b/lib/http/request.js @@ -64,7 +64,7 @@ req.logOut = function(options, done) { if (this._sessionManager) { if (typeof done != 'function') { throw new Error('req#logout requires a callback function'); } - this._sessionManager.logOut(this, done); + this._sessionManager.logOut(this, options, done); } else { done && done(); } diff --git a/lib/sessionmanager.js b/lib/sessionmanager.js index 193f11b4..c80efda7 100644 --- a/lib/sessionmanager.js +++ b/lib/sessionmanager.js @@ -73,6 +73,7 @@ SessionManager.prototype.logOut = function(req, options, cb) { if (req.session[this._key]) { delete req.session[this._key].user; } + var prevSession = req.session; req.session.save(function(err) { if (err) { @@ -85,6 +86,9 @@ SessionManager.prototype.logOut = function(req, options, cb) { if (err) { return cb(err); } + if (options.keepSessionData) { + Object.assign(req.session, prevSession); + } cb && cb(); }); }); From bfba8a1ab44b658f745e33e3484b389f0751cdc0 Mon Sep 17 00:00:00 2001 From: Jared Hanson Date: Thu, 19 May 2022 09:51:32 -0700 Subject: [PATCH 22/28] Add tests. --- test/http/request.test.js | 124 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 124 insertions(+) diff --git a/test/http/request.test.js b/test/http/request.test.js index 1f7f46f5..94566979 100644 --- a/test/http/request.test.js +++ b/test/http/request.test.js @@ -246,6 +246,130 @@ describe('http.ServerRequest', function() { }); }); + describe('establishing a session and not keeping previous session data', function() { + var passport = new Passport(); + passport.serializeUser(function(user, done) { + done(null, user.id); + }); + + var req = new Object(); + req.login = request.login; + req.isAuthenticated = request.isAuthenticated; + req.isUnauthenticated = request.isUnauthenticated; + req._passport = {}; + req._passport.instance = passport; + req._sessionManager = passport._sm; + req.session = { cart: [ '1', '2', ] }; + Object.defineProperty(req.session, 'id', { value: '1' }); + req.session.regenerate = function(cb) { + req.session = { id: '2' }; + req.session.save = function(cb) { + process.nextTick(cb); + }; + process.nextTick(cb); + }; + + var error; + + before(function(done) { + var user = { id: '1', username: 'root' }; + + req.login(user, function(err) { + error = err; + done(); + }); + }); + + it('should not error', function() { + expect(error).to.be.undefined; + }); + + it('should be authenticated', function() { + expect(req.isAuthenticated()).to.be.true; + expect(req.isUnauthenticated()).to.be.false; + }); + + it('should regenerate session', function() { + expect(req.session.id).to.equal('2'); + }); + + it('should keep session data', function() { + expect(req.session.cart).to.be.undefined; + }); + + it('should set user', function() { + expect(req.user).to.be.an('object'); + expect(req.user.id).to.equal('1'); + expect(req.user.username).to.equal('root'); + }); + + it('should serialize user', function() { + expect(req.session['passport'].user).to.equal('1'); + }); + }); + + describe('establishing a session and keeping previous session data', function() { + var passport = new Passport(); + passport.serializeUser(function(user, done) { + done(null, user.id); + }); + + var req = new Object(); + req.login = request.login; + req.isAuthenticated = request.isAuthenticated; + req.isUnauthenticated = request.isUnauthenticated; + req._passport = {}; + req._passport.instance = passport; + req._sessionManager = passport._sm; + req.session = { cart: [ '1', '2', ] }; + Object.defineProperty(req.session, 'id', { value: '1' }); + req.session.regenerate = function(cb) { + req.session = { id: '2' }; + req.session.save = function(cb) { + process.nextTick(cb); + }; + process.nextTick(cb); + }; + + var error; + + before(function(done) { + var user = { id: '1', username: 'root' }; + + req.login(user, { keepSessionData: true }, function(err) { + error = err; + done(); + }); + }); + + it('should not error', function() { + expect(error).to.be.undefined; + }); + + it('should be authenticated', function() { + expect(req.isAuthenticated()).to.be.true; + expect(req.isUnauthenticated()).to.be.false; + }); + + it('should regenerate session', function() { + expect(req.session.id).to.equal('2'); + }); + + it('should keep session data', function() { + expect(req.session.cart).to.deep.equal([ '1', '2' ]); + }); + + it('should set user', function() { + expect(req.user).to.be.an('object'); + expect(req.user.id).to.equal('1'); + expect(req.user.username).to.equal('root'); + }); + + it('should serialize user', function() { + expect(req.session['passport'].user).to.equal('1'); + }); + }); + describe('establishing a session and setting custom user property', function() { var passport = new Passport(); passport.serializeUser(function(user, done) { From 29a90d68dd5d4bc807bc658cfe49fba968b34d7d Mon Sep 17 00:00:00 2001 From: Jared Hanson Date: Thu, 19 May 2022 09:53:53 -0700 Subject: [PATCH 23/28] No need to guard callback existence. --- lib/sessionmanager.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/sessionmanager.js b/lib/sessionmanager.js index c80efda7..b56f9744 100644 --- a/lib/sessionmanager.js +++ b/lib/sessionmanager.js @@ -89,7 +89,7 @@ SessionManager.prototype.logOut = function(req, options, cb) { if (options.keepSessionData) { Object.assign(req.session, prevSession); } - cb && cb(); + cb(); }); }); } From f8a175f1145c4efdffa7e4c511a642f608e11c0f Mon Sep 17 00:00:00 2001 From: Jared Hanson Date: Thu, 19 May 2022 10:18:27 -0700 Subject: [PATCH 24/28] Add tests. --- test/http/request.test.js | 57 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 56 insertions(+), 1 deletion(-) diff --git a/test/http/request.test.js b/test/http/request.test.js index 94566979..3c98cc5b 100644 --- a/test/http/request.test.js +++ b/test/http/request.test.js @@ -293,7 +293,7 @@ describe('http.ServerRequest', function() { expect(req.session.id).to.equal('2'); }); - it('should keep session data', function() { + it('should not keep session data', function() { expect(req.session.cart).to.be.undefined; }); @@ -721,6 +721,61 @@ describe('http.ServerRequest', function() { }); }); + describe('existing session and keeping session data', function() { + var passport = new Passport(); + + var req = new Object(); + req.logout = request.logout; + req.isAuthenticated = request.isAuthenticated; + req.isUnauthenticated = request.isUnauthenticated; + req.user = { id: '1', username: 'root' }; + req._passport = {}; + req._passport.instance = passport; + req._sessionManager = passport._sm; + req.session = { cart: [ '1', '2', ] }; + Object.defineProperty(req.session, 'id', { value: '1' }); + req.session['passport'] = {}; + req.session['passport'].user = '1'; + req.session.save = function(cb) { + expect(req.session['passport'].user).to.be.undefined; + process.nextTick(cb); + }; + req.session.regenerate = function(cb) { + req.session = { id: '2' }; + process.nextTick(cb); + }; + + var error; + + before(function(done) { + req.logout({ keepSessionData: true }, function(err) { + error = err; + done(); + }); + }); + + it('should not error', function() { + expect(error).to.be.undefined; + }); + + it('should not be authenticated', function() { + expect(req.isAuthenticated()).to.be.false; + expect(req.isUnauthenticated()).to.be.true; + }); + + it('should clear user', function() { + expect(req.user).to.be.null; + }); + + it('should clear serialized user', function() { + expect(req.session['passport'].user).to.be.undefined; + }); + + it('should keep session data', function() { + expect(req.session.cart).to.deep.equal([ '1', '2' ]); + }); + }); + describe('existing session and clearing custom user property', function() { var passport = new Passport(); From 987b1918a2c5056531bbd325a2ff888a3595b2df Mon Sep 17 00:00:00 2001 From: Jared Hanson Date: Thu, 19 May 2022 10:22:35 -0700 Subject: [PATCH 25/28] Add tests. --- test/http/request.test.js | 55 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 55 insertions(+) diff --git a/test/http/request.test.js b/test/http/request.test.js index 3c98cc5b..85c60458 100644 --- a/test/http/request.test.js +++ b/test/http/request.test.js @@ -721,6 +721,61 @@ describe('http.ServerRequest', function() { }); }); + describe('existing session and not keeping session data', function() { + var passport = new Passport(); + + var req = new Object(); + req.logout = request.logout; + req.isAuthenticated = request.isAuthenticated; + req.isUnauthenticated = request.isUnauthenticated; + req.user = { id: '1', username: 'root' }; + req._passport = {}; + req._passport.instance = passport; + req._sessionManager = passport._sm; + req.session = { cart: [ '1', '2', ] }; + Object.defineProperty(req.session, 'id', { value: '1' }); + req.session['passport'] = {}; + req.session['passport'].user = '1'; + req.session.save = function(cb) { + expect(req.session['passport'].user).to.be.undefined; + process.nextTick(cb); + }; + req.session.regenerate = function(cb) { + req.session = { id: '2' }; + process.nextTick(cb); + }; + + var error; + + before(function(done) { + req.logout(function(err) { + error = err; + done(); + }); + }); + + it('should not error', function() { + expect(error).to.be.undefined; + }); + + it('should not be authenticated', function() { + expect(req.isAuthenticated()).to.be.false; + expect(req.isUnauthenticated()).to.be.true; + }); + + it('should clear user', function() { + expect(req.user).to.be.null; + }); + + it('should clear serialized user', function() { + expect(req.session['passport']).to.be.undefined; + }); + + it('should keep session data', function() { + expect(req.session.cart).to.be.undefined; + }); + }); + describe('existing session and keeping session data', function() { var passport = new Passport(); From 46756e56db671a822490f3d6c103a33a6691047d Mon Sep 17 00:00:00 2001 From: Jared Hanson Date: Thu, 19 May 2022 10:54:43 -0700 Subject: [PATCH 26/28] Silence verbose logging. --- lib/sessionmanager.js | 4 ---- 1 file changed, 4 deletions(-) diff --git a/lib/sessionmanager.js b/lib/sessionmanager.js index b56f9744..21989fd6 100644 --- a/lib/sessionmanager.js +++ b/lib/sessionmanager.js @@ -16,8 +16,6 @@ SessionManager.prototype.logIn = function(req, user, options, cb) { } options = options || {}; - console.log('SM: logIn'); - if (!req.session) { return cb(new Error('Login sessions require session support. Did you forget to use `express-session` middleware?')); } var self = this; @@ -61,8 +59,6 @@ SessionManager.prototype.logOut = function(req, options, cb) { } options = options || {}; - console.log('SM: logOut'); - if (!req.session) { return cb(new Error('Login sessions require session support. Did you forget to use `express-session` middleware?')); } var self = this; From 4f6bd5b254454d3f61c3236e8f1dd33472704fd3 Mon Sep 17 00:00:00 2001 From: Jared Hanson Date: Thu, 19 May 2022 14:00:51 -0700 Subject: [PATCH 27/28] Change keepSessionData to keepSessionData. --- lib/sessionmanager.js | 4 ++-- test/http/request.test.js | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/sessionmanager.js b/lib/sessionmanager.js index 21989fd6..f4123def 100644 --- a/lib/sessionmanager.js +++ b/lib/sessionmanager.js @@ -32,7 +32,7 @@ SessionManager.prototype.logIn = function(req, user, options, cb) { if (err) { return cb(err); } - if (options.keepSessionData) { + if (options.keepSessionInfo) { Object.assign(req.session, prevSession); } if (!req.session[self._key]) { @@ -82,7 +82,7 @@ SessionManager.prototype.logOut = function(req, options, cb) { if (err) { return cb(err); } - if (options.keepSessionData) { + if (options.keepSessionInfo) { Object.assign(req.session, prevSession); } cb(); diff --git a/test/http/request.test.js b/test/http/request.test.js index 85c60458..d94dc9f2 100644 --- a/test/http/request.test.js +++ b/test/http/request.test.js @@ -336,7 +336,7 @@ describe('http.ServerRequest', function() { before(function(done) { var user = { id: '1', username: 'root' }; - req.login(user, { keepSessionData: true }, function(err) { + req.login(user, { keepSessionInfo: true }, function(err) { error = err; done(); }); @@ -803,7 +803,7 @@ describe('http.ServerRequest', function() { var error; before(function(done) { - req.logout({ keepSessionData: true }, function(err) { + req.logout({ keepSessionInfo: true }, function(err) { error = err; done(); }); From 8dd79fe5f3f414435c4e0561fc925fb7ab6c8efb Mon Sep 17 00:00:00 2001 From: Jared Hanson Date: Thu, 19 May 2022 18:33:49 -0700 Subject: [PATCH 28/28] Use utils-merge rather than Object.assign for compatibility. --- lib/sessionmanager.js | 6 ++++-- package.json | 3 ++- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/lib/sessionmanager.js b/lib/sessionmanager.js index f4123def..81b59b1d 100644 --- a/lib/sessionmanager.js +++ b/lib/sessionmanager.js @@ -1,3 +1,5 @@ +var merge = require('utils-merge'); + function SessionManager(options, serializeUser) { if (typeof options == 'function') { serializeUser = options; @@ -33,7 +35,7 @@ SessionManager.prototype.logIn = function(req, user, options, cb) { return cb(err); } if (options.keepSessionInfo) { - Object.assign(req.session, prevSession); + merge(req.session, prevSession); } if (!req.session[self._key]) { req.session[self._key] = {}; @@ -83,7 +85,7 @@ SessionManager.prototype.logOut = function(req, options, cb) { return cb(err); } if (options.keepSessionInfo) { - Object.assign(req.session, prevSession); + merge(req.session, prevSession); } cb(); }); diff --git a/package.json b/package.json index 53cbd220..00398a85 100644 --- a/package.json +++ b/package.json @@ -36,7 +36,8 @@ "main": "./lib", "dependencies": { "passport-strategy": "1.x.x", - "pause": "0.0.1" + "pause": "0.0.1", + "utils-merge": "^1.0.1" }, "devDependencies": { "make-node": "0.3.x",