diff --git a/package-lock.json b/package-lock.json index b671bbbdad..23411d2656 100644 --- a/package-lock.json +++ b/package-lock.json @@ -30573,15 +30573,20 @@ } }, "node_modules/passport": { - "version": "0.4.1", - "resolved": "https://registry.npmjs.org/passport/-/passport-0.4.1.tgz", - "integrity": "sha512-IxXgZZs8d7uFSt3eqNjM9NQ3g3uQCW5avD8mRNoXV99Yig50vjuaez6dQK2qC0kVWPRTujxY0dWgGfT09adjYg==", + "version": "0.6.0", + "resolved": "https://registry.npmjs.org/passport/-/passport-0.6.0.tgz", + "integrity": "sha512-0fe+p3ZnrWRW74fe8+SvCyf4a3Pb2/h7gFkQ8yTJpAO50gDzlfjZUZTO1k5Eg9kUct22OxHLqDZoKUWRHOh9ug==", "dependencies": { "passport-strategy": "1.x.x", - "pause": "0.0.1" + "pause": "0.0.1", + "utils-merge": "^1.0.1" }, "engines": { "node": ">= 0.4.0" + }, + "funding": { + "type": "github", + "url": "https://github.com/sponsors/jaredhanson" } }, "node_modules/passport-google-oauth20": { @@ -43525,7 +43530,7 @@ "otplib": "^12.0.1", "p-limit": "^2.3.0", "parse-data-url": "^2.0.0", - "passport": "^0.4.1", + "passport": "^0.6.0", "passport-google-oauth20": "^2.0.0", "passport-ldapauth": "^2.1.4", "passport-local": "^1.0.0", @@ -52267,7 +52272,7 @@ "otplib": "^12.0.1", "p-limit": "^2.3.0", "parse-data-url": "^2.0.0", - "passport": "^0.4.1", + "passport": "^0.6.0", "passport-google-oauth20": "^2.0.0", "passport-ldapauth": "^2.1.4", "passport-local": "^1.0.0", @@ -70997,12 +71002,13 @@ } }, "passport": { - "version": "0.4.1", - "resolved": "https://registry.npmjs.org/passport/-/passport-0.4.1.tgz", - "integrity": "sha512-IxXgZZs8d7uFSt3eqNjM9NQ3g3uQCW5avD8mRNoXV99Yig50vjuaez6dQK2qC0kVWPRTujxY0dWgGfT09adjYg==", + "version": "0.6.0", + "resolved": "https://registry.npmjs.org/passport/-/passport-0.6.0.tgz", + "integrity": "sha512-0fe+p3ZnrWRW74fe8+SvCyf4a3Pb2/h7gFkQ8yTJpAO50gDzlfjZUZTO1k5Eg9kUct22OxHLqDZoKUWRHOh9ug==", "requires": { "passport-strategy": "1.x.x", - "pause": "0.0.1" + "pause": "0.0.1", + "utils-merge": "^1.0.1" } }, "passport-google-oauth20": { diff --git a/services/web/app/src/Features/Authentication/AuthenticationController.js b/services/web/app/src/Features/Authentication/AuthenticationController.js index 3ad4709032..56db9e8f89 100644 --- a/services/web/app/src/Features/Authentication/AuthenticationController.js +++ b/services/web/app/src/Features/Authentication/AuthenticationController.js @@ -80,30 +80,36 @@ const AuthenticationController = { // This function is middleware which wraps the passport.authenticate middleware, // so we can send back our custom `{message: {text: "", type: ""}}` responses on failure, // and send a `{redir: ""}` response on success - passport.authenticate('local', function (err, user, info) { - if (err) { - return next(err) - } - if (user) { - // `user` is either a user object or false - AuthenticationController.setAuditInfo(req, { method: 'Password login' }) - return AuthenticationController.finishLogin(user, req, res, next) - } else { - if (info.redir != null) { - return res.json({ redir: info.redir }) + passport.authenticate( + 'local', + { keepSessionInfo: true }, + function (err, user, info) { + if (err) { + return next(err) + } + if (user) { + // `user` is either a user object or false + AuthenticationController.setAuditInfo(req, { + method: 'Password login', + }) + return AuthenticationController.finishLogin(user, req, res, next) } else { - res.status(info.status || 200) - delete info.status - const body = { message: info } - const { errorReason } = info - if (errorReason) { - body.errorReason = errorReason - delete info.errorReason + if (info.redir != null) { + return res.json({ redir: info.redir }) + } else { + res.status(info.status || 200) + delete info.status + const body = { message: info } + const { errorReason } = info + if (errorReason) { + body.errorReason = errorReason + delete info.errorReason + } + return res.json(body) } - return res.json(body) } } - })(req, res, next) + )(req, res, next) }, finishLogin(user, req, res, next) { @@ -557,55 +563,34 @@ const AuthenticationController = { } function _afterLoginSessionSetup(req, user, callback) { - if (callback == null) { - callback = function () {} - } - req.login(user, function (err) { + req.login(user, { keepSessionInfo: true }, function (err) { if (err) { OError.tag(err, 'error from req.login', { user_id: user._id, }) return callback(err) } - // Regenerate the session to get a new sessionID (cookie value) to - // protect against session fixation attacks - const oldSession = req.session - req.session.destroy(function (err) { + delete req.session.__tmp + delete req.session.csrfSecret + req.session.save(function (err) { if (err) { - OError.tag(err, 'error when trying to destroy old session', { + OError.tag(err, 'error saving regenerated session after login', { user_id: user._id, }) return callback(err) } - req.sessionStore.generate(req) - // Note: the validation token is not writable, so it does not get - // transferred to the new session below. - for (const key in oldSession) { - const value = oldSession[key] - if (key !== '__tmp' && key !== 'csrfSecret') { - req.session[key] = value - } + UserSessionsManager.trackSession(user, req.sessionID, function () {}) + if (!req.deviceHistory) { + // Captcha disabled or SSO-based login. + return callback() } - req.session.save(function (err) { - if (err) { - OError.tag(err, 'error saving regenerated session after login', { - user_id: user._id, - }) - return callback(err) - } - UserSessionsManager.trackSession(user, req.sessionID, function () {}) - if (!req.deviceHistory) { - // Captcha disabled or SSO-based login. - return callback() - } - req.deviceHistory.add(user.email) - req.deviceHistory - .serialize(req.res) - .catch(err => { - logger.err({ err }, 'cannot serialize deviceHistory') - }) - .finally(() => callback()) - }) + req.deviceHistory.add(user.email) + req.deviceHistory + .serialize(req.res) + .catch(err => { + logger.err({ err }, 'cannot serialize deviceHistory') + }) + .finally(() => callback()) }) }) } diff --git a/services/web/app/src/Features/User/UserController.js b/services/web/app/src/Features/User/UserController.js index f2a6070f50..d557c85c84 100644 --- a/services/web/app/src/Features/User/UserController.js +++ b/services/web/app/src/Features/User/UserController.js @@ -266,7 +266,8 @@ async function tryDeleteUser(req, res, next) { const sessionId = req.sessionID if (typeof req.logout === 'function') { - req.logout() + const logout = promisify(req.logout) + await logout() } const destroySession = promisify(req.session.destroy.bind(req.session)) @@ -431,9 +432,10 @@ async function doLogout(req) { logger.debug({ user }, 'logging out') const sessionId = req.sessionID - // passport logout if (typeof req.logout === 'function') { - req.logout() + // passport logout + const logout = promisify(req.logout.bind(req)) + await logout() } const destroySession = promisify(req.session.destroy.bind(req.session)) diff --git a/services/web/package.json b/services/web/package.json index 1777a9f5e8..68140cce7b 100644 --- a/services/web/package.json +++ b/services/web/package.json @@ -137,7 +137,7 @@ "otplib": "^12.0.1", "p-limit": "^2.3.0", "parse-data-url": "^2.0.0", - "passport": "^0.4.1", + "passport": "^0.6.0", "passport-google-oauth20": "^2.0.0", "passport-ldapauth": "^2.1.4", "passport-local": "^1.0.0", diff --git a/services/web/test/unit/src/Authentication/AuthenticationControllerTests.js b/services/web/test/unit/src/Authentication/AuthenticationControllerTests.js index 15bd16199f..cc0b08cbf5 100644 --- a/services/web/test/unit/src/Authentication/AuthenticationControllerTests.js +++ b/services/web/test/unit/src/Authentication/AuthenticationControllerTests.js @@ -77,7 +77,7 @@ describe('AuthenticationController', function () { '../User/UserSessionsManager': (this.UserSessionsManager = { trackSession: sinon.stub(), untrackSession: sinon.stub(), - revokeAllUserSessions: sinon.stub().callsArgWith(1, null), + revokeAllUserSessions: sinon.stub().yields(null), }), '../../infrastructure/Modules': (this.Modules = { hooks: { fire: sinon.stub().yields(null, []) }, @@ -184,17 +184,17 @@ describe('AuthenticationController', function () { describe('passportLogin', function () { beforeEach(function () { this.info = null - this.req.login = sinon.stub().callsArgWith(1, null) + this.req.login = sinon.stub().yields(null) this.res.json = sinon.stub() this.req.session = { passport: { user: this.user }, postLoginRedirect: '/path/to/redir/to', } - this.req.session.destroy = sinon.stub().callsArgWith(0, null) - this.req.session.save = sinon.stub().callsArgWith(0, null) + this.req.session.destroy = sinon.stub().yields(null) + this.req.session.save = sinon.stub().yields(null) this.req.sessionStore = { generate: sinon.stub() } this.AuthenticationController.finishLogin = sinon.stub() - this.passport.authenticate.callsArgWith(1, null, this.user, this.info) + this.passport.authenticate.yields(null, this.user, this.info) this.err = new Error('woops') }) @@ -205,7 +205,7 @@ describe('AuthenticationController', function () { describe('when authenticate produces an error', function () { beforeEach(function () { - this.passport.authenticate.callsArgWith(1, this.err) + this.passport.authenticate.yields(this.err) }) it('should return next with an error', function () { @@ -221,7 +221,7 @@ describe('AuthenticationController', function () { describe('when authenticate produces a user', function () { beforeEach(function () { this.req.session.postLoginRedirect = 'some_redirect' - this.passport.authenticate.callsArgWith(1, null, this.user, this.info) + this.passport.authenticate.yields(null, this.user, this.info) }) afterEach(function () { @@ -244,7 +244,7 @@ describe('AuthenticationController', function () { describe('when authenticate does not produce a user', function () { beforeEach(function () { this.info = { text: 'a', type: 'b' } - this.passport.authenticate.callsArgWith(1, null, false, this.info) + this.passport.authenticate.yields(null, false, this.info) }) it('should not call finishLogin', function () { @@ -273,8 +273,7 @@ describe('AuthenticationController', function () { beforeEach(function () { this.AuthenticationController._recordFailedLogin = sinon.stub() this.AuthenticationController._recordSuccessfulLogin = sinon.stub() - this.Modules.hooks.fire = sinon.stub().callsArgWith(3, null, []) - // @AuthenticationController.establishUserSession = sinon.stub().callsArg(2) + this.Modules.hooks.fire = sinon.stub().yields(null, []) this.req.body = { email: this.email, password: this.password, @@ -290,7 +289,7 @@ describe('AuthenticationController', function () { beforeEach(function () { this.Modules.hooks.fire = sinon .stub() - .callsArgWith(3, null, [null, { redir: '/somewhere' }, null]) + .yields(null, [null, { redir: '/somewhere' }, null]) }) it('should stop early and call done with this info object', function (done) { @@ -311,7 +310,7 @@ describe('AuthenticationController', function () { describe('when the users rate limit', function () { beforeEach(function () { - this.LoginRateLimiter.processLoginRequest.callsArgWith(1, null, false) + this.LoginRateLimiter.processLoginRequest.yields(null, false) }) it('should block the request if the limit has been exceeded', function (done) { @@ -330,10 +329,10 @@ describe('AuthenticationController', function () { describe('when the user is authenticated', function () { beforeEach(function () { this.cb = sinon.stub() - this.LoginRateLimiter.processLoginRequest.callsArgWith(1, null, true) + this.LoginRateLimiter.processLoginRequest.yields(null, true) this.AuthenticationManager.authenticate = sinon .stub() - .callsArgWith(3, null, this.user) + .yields(null, this.user) this.req.sessionID = Math.random() }) @@ -361,7 +360,7 @@ describe('AuthenticationController', function () { beforeEach(function () { this.AuthenticationManager.authenticate = sinon .stub() - .callsArgWith(3, new AuthenticationErrors.ParallelLoginError()) + .yields(new AuthenticationErrors.ParallelLoginError()) this.AuthenticationController.doPassportLogin( this.req, this.req.body.email, @@ -437,10 +436,10 @@ describe('AuthenticationController', function () { describe('when the user is not authenticated', function () { beforeEach(function () { - this.LoginRateLimiter.processLoginRequest.callsArgWith(1, null, true) + this.LoginRateLimiter.processLoginRequest.yields(null, true) this.AuthenticationManager.authenticate = sinon .stub() - .callsArgWith(3, null, null) + .yields(null, null) this.cb = sinon.stub() this.AuthenticationController.doPassportLogin( this.req, @@ -937,7 +936,7 @@ describe('AuthenticationController', function () { describe('_recordSuccessfulLogin', function () { beforeEach(function () { - this.UserUpdater.updateUser = sinon.stub().callsArg(2) + this.UserUpdater.updateUser = sinon.stub().yields() this.AuthenticationController._recordSuccessfulLogin( this.user._id, this.callback @@ -1078,8 +1077,8 @@ describe('AuthenticationController', function () { this.req.session = { passport: { user: { _id: 'one' } }, } - this.req.session.destroy = sinon.stub().callsArgWith(0, null) - this.req.session.save = sinon.stub().callsArgWith(0, null) + this.req.session.destroy = sinon.stub().yields(null) + this.req.session.save = sinon.stub().yields(null) this.req.sessionStore = { generate: sinon.stub() } this.req.login = sinon.stub().yields(null) @@ -1345,9 +1344,7 @@ describe('AuthenticationController', function () { describe('when req.session.save produces an error', function () { beforeEach(function () { - this.req.session.save = sinon - .stub() - .callsArgWith(0, new Error('woops')) + this.req.session.save = sinon.stub().yields(new Error('woops')) }) it('should produce an error', function (done) { diff --git a/services/web/test/unit/src/User/UserControllerTests.js b/services/web/test/unit/src/User/UserControllerTests.js index d6b4f86333..8409163aa1 100644 --- a/services/web/test/unit/src/User/UserControllerTests.js +++ b/services/web/test/unit/src/User/UserControllerTests.js @@ -173,7 +173,7 @@ describe('UserController', function () { describe('tryDeleteUser', function () { beforeEach(function () { this.req.body.password = 'wat' - this.req.logout = sinon.stub() + this.req.logout = sinon.stub().yields() this.req.session.destroy = sinon.stub().yields() this.SessionManager.getLoggedInUserId = sinon .stub()