From 5d7fd2a9d8ec99421a398a8eb2978f987c42751c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Timoth=C3=A9e=20Alby?= Date: Tue, 28 Apr 2020 15:12:05 +0200 Subject: [PATCH] Merge pull request #2751 from overleaf/ta-finish-login-private Don't Export `afterLoginSessionSetup` GitOrigin-RevId: 46818a70566b8ec56e1a40c7f0d9758d2ac2c100 --- .../AuthenticationController.js | 131 ++++---- .../AuthenticationControllerTests.js | 311 ++++++++---------- 2 files changed, 203 insertions(+), 239 deletions(-) diff --git a/services/web/app/src/Features/Authentication/AuthenticationController.js b/services/web/app/src/Features/Authentication/AuthenticationController.js index f3a0911f53..e8cdfb5745 100644 --- a/services/web/app/src/Features/Authentication/AuthenticationController.js +++ b/services/web/app/src/Features/Authentication/AuthenticationController.js @@ -45,50 +45,6 @@ const AuthenticationController = (module.exports = { cb(null, user) }, - afterLoginSessionSetup(req, user, callback) { - if (callback == null) { - callback = function() {} - } - req.login(user, function(err) { - if (err) { - logger.warn({ user_id: user._id, err }, 'error from req.login') - 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) { - if (err) { - logger.warn( - { user_id: user._id, err }, - 'error when trying to destroy old session' - ) - 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 (let key in oldSession) { - const value = oldSession[key] - if (key !== '__tmp') { - req.session[key] = value - } - } - req.session.save(function(err) { - if (err) { - logger.warn( - { user_id: user._id }, - 'error saving regenerated session after login' - ) - return callback(err) - } - UserSessionsManager.trackSession(user, req.sessionID, function() {}) - callback(null) - }) - }) - }) - }, - passportLogin(req, res, next) { // This function is middleware which wraps the passport.authenticate middleware, // so we can send back our custom `{message: {text: "", type: ""}}` responses on failure, @@ -133,8 +89,8 @@ const AuthenticationController = (module.exports = { const redir = AuthenticationController._getRedirectFromSession(req) || '/project' - AuthenticationController._loginAsyncHandlers(req, user) - AuthenticationController.afterLoginSessionSetup(req, user, function(err) { + _loginAsyncHandlers(req, user) + _afterLoginSessionSetup(req, user, function(err) { if (err) { return next(err) } @@ -200,26 +156,6 @@ const AuthenticationController = (module.exports = { }) }, - _loginAsyncHandlers(req, user) { - UserHandler.setupLoginData(user, err => { - if (err != null) { - logger.warn({ err }, 'error setting up login data') - } - }) - LoginRateLimiter.recordSuccessfulLogin(user.email) - AuthenticationController._recordSuccessfulLogin(user._id) - AuthenticationController.ipMatchCheck(req, user) - Analytics.recordEvent(user._id, 'user-logged-in', { ip: req.ip }) - Analytics.identifyUser(user._id, req.sessionID) - logger.log( - { email: user.email, user_id: user._id.toString() }, - 'successful log in' - ) - req.session.justLoggedIn = true - // capture the request ip for use when creating the session - return (user._login_req_ip = req.ip) - }, - ipMatchCheck(req, user) { if (req.ip !== user.lastLoginIp) { NotificationsBuilder.ipMatcherAffiliation(user._id).create(req.ip) @@ -485,3 +421,66 @@ const AuthenticationController = (module.exports = { } } }) + +function _afterLoginSessionSetup(req, user, callback) { + if (callback == null) { + callback = function() {} + } + req.login(user, function(err) { + if (err) { + logger.warn({ user_id: user._id, err }, 'error from req.login') + 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) { + if (err) { + logger.warn( + { user_id: user._id, err }, + 'error when trying to destroy old session' + ) + 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 (let key in oldSession) { + const value = oldSession[key] + if (key !== '__tmp') { + req.session[key] = value + } + } + req.session.save(function(err) { + if (err) { + logger.warn( + { user_id: user._id }, + 'error saving regenerated session after login' + ) + return callback(err) + } + UserSessionsManager.trackSession(user, req.sessionID, function() {}) + callback(null) + }) + }) + }) +} +function _loginAsyncHandlers(req, user) { + UserHandler.setupLoginData(user, err => { + if (err != null) { + logger.warn({ err }, 'error setting up login data') + } + }) + LoginRateLimiter.recordSuccessfulLogin(user.email) + AuthenticationController._recordSuccessfulLogin(user._id) + AuthenticationController.ipMatchCheck(req, user) + Analytics.recordEvent(user._id, 'user-logged-in', { ip: req.ip }) + Analytics.identifyUser(user._id, req.sessionID) + logger.log( + { email: user.email, user_id: user._id.toString() }, + 'successful log in' + ) + req.session.justLoggedIn = true + // capture the request ip for use when creating the session + return (user._login_req_ip = req.ip) +} diff --git a/services/web/test/unit/src/Authentication/AuthenticationControllerTests.js b/services/web/test/unit/src/Authentication/AuthenticationControllerTests.js index 4e9cf227fd..41eeff582b 100644 --- a/services/web/test/unit/src/Authentication/AuthenticationControllerTests.js +++ b/services/web/test/unit/src/Authentication/AuthenticationControllerTests.js @@ -36,7 +36,8 @@ describe('AuthenticationController', function() { setupLoginData: sinon.stub() }), '../Analytics/AnalyticsManager': (this.AnalyticsManager = { - recordEvent: sinon.stub() + recordEvent: sinon.stub(), + identifyUser: sinon.stub() }), '../../infrastructure/SessionStoreManager': (this.SessionStoreManager = {}), 'logger-sharelatex': (this.logger = { @@ -248,86 +249,6 @@ describe('AuthenticationController', function() { }) }) - describe('afterLoginSessionSetup', function() { - beforeEach(function() { - this.req.login = sinon.stub().callsArgWith(1, null) - this.req.sessionID = 'thisisacryptographicallysecurerandomid' - 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.sessionStore = { generate: sinon.stub() } - this.UserSessionsManager.trackSession = sinon.stub() - this.call = callback => { - this.AuthenticationController.afterLoginSessionSetup( - this.req, - this.user, - callback - ) - } - }) - - it('should not produce an error', function(done) { - this.call(err => { - expect(err).to.equal(null) - done() - }) - }) - - it('should call req.login', function(done) { - this.call(err => { - if (err) { - return done(err) - } - this.req.login.callCount.should.equal(1) - done() - }) - }) - - it('should call req.session.save', function(done) { - this.call(err => { - if (err) { - return done(err) - } - this.req.session.save.callCount.should.equal(1) - done() - }) - }) - - it('should call UserSessionsManager.trackSession', function(done) { - this.call(err => { - if (err) { - return done(err) - } - this.UserSessionsManager.trackSession.callCount.should.equal(1) - done() - }) - }) - - describe('when req.session.save produces an error', function() { - beforeEach(function() { - this.req.session.save = sinon.stub().callsArgWith(0, new Error('woops')) - }) - - it('should produce an error', function(done) { - this.call(err => { - expect(err).to.not.be.oneOf([null, undefined]) - expect(err).to.be.instanceof(Error) - done() - }) - }) - - it('should not call UserSessionsManager.trackSession', function(done) { - this.call(err => { - expect(err).to.exist - this.UserSessionsManager.trackSession.callCount.should.equal(0) - done() - }) - }) - }) - }) - describe('doPassportLogin', function() { beforeEach(function() { this.AuthenticationController._recordFailedLogin = sinon.stub() @@ -412,58 +333,6 @@ describe('AuthenticationController', function() { }) }) - describe('_loginAsyncHandlers', function() { - beforeEach(function() { - this.UserHandler.setupLoginData = sinon.stub() - this.LoginRateLimiter.recordSuccessfulLogin = sinon.stub() - this.AuthenticationController._recordSuccessfulLogin = sinon.stub() - this.AnalyticsManager.recordEvent = sinon.stub() - this.AnalyticsManager.identifyUser = sinon.stub() - this.AuthenticationController._loginAsyncHandlers(this.req, this.user) - }) - - it('should call identifyUser', function() { - this.AnalyticsManager.identifyUser - .calledWith(this.user._id, this.req.sessionID) - .should.equal(true) - }) - - it('should setup the user data in the background', function() { - this.UserHandler.setupLoginData.calledWith(this.user).should.equal(true) - }) - - it('should set res.session.justLoggedIn', function() { - this.req.session.justLoggedIn.should.equal(true) - }) - - it('should record the successful login', function() { - this.AuthenticationController._recordSuccessfulLogin - .calledWith(this.user._id) - .should.equal(true) - }) - - it('should tell the rate limiter that there was a success for that email', function() { - this.LoginRateLimiter.recordSuccessfulLogin - .calledWith(this.user.email) - .should.equal(true) - }) - - it('should log the successful login', function() { - this.logger.log - .calledWith( - { email: this.user.email, user_id: this.user._id.toString() }, - 'successful log in' - ) - .should.equal(true) - }) - - it('should track the login event', function() { - this.AnalyticsManager.recordEvent - .calledWith(this.user._id, 'user-logged-in') - .should.equal(true) - }) - }) - describe('when the user is not authenticated', function() { beforeEach(function() { this.LoginRateLimiter.processLoginRequest.callsArgWith(1, null, true) @@ -1078,12 +947,24 @@ describe('AuthenticationController', function() { this.AuthenticationController._getRedirectFromSession = sinon .stub() .returns('/some/page') - this.AuthenticationController._loginAsyncHandlers = sinon.stub() - this.AuthenticationController.afterLoginSessionSetup = sinon - .stub() - .callsArgWith(2, null) + + this.req.sessionID = 'thisisacryptographicallysecurerandomid' + 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.sessionStore = { generate: sinon.stub() } + this.req.login = sinon.stub().yields(null) + this.AuthenticationController._clearRedirectFromSession = sinon.stub() this.AuthenticationController._redirectToReconfirmPage = sinon.stub() + this.UserSessionsManager.trackSession = sinon.stub() + this.UserHandler.setupLoginData = sinon.stub() + this.LoginRateLimiter.recordSuccessfulLogin = sinon.stub() + this.AuthenticationController._recordSuccessfulLogin = sinon.stub() + this.AnalyticsManager.recordEvent = sinon.stub() + this.AnalyticsManager.identifyUser = sinon.stub() this.req.headers = { accept: 'application/json, whatever' } this.res.json = sinon.stub() this.res.redirect = sinon.stub() @@ -1106,42 +987,6 @@ describe('AuthenticationController', function() { ).to.equal(true) }) - it('should call the async handlers', function() { - this.AuthenticationController.finishLogin( - this.user, - this.req, - this.res, - this.next - ) - expect( - this.AuthenticationController._loginAsyncHandlers.callCount - ).to.equal(1) - expect( - this.AuthenticationController._loginAsyncHandlers.calledWith( - this.req, - this.user - ) - ).to.equal(true) - }) - - it('should call afterLoginSessionSetup', function() { - this.AuthenticationController.finishLogin( - this.user, - this.req, - this.res, - this.next - ) - expect( - this.AuthenticationController.afterLoginSessionSetup.callCount - ).to.equal(1) - expect( - this.AuthenticationController.afterLoginSessionSetup.calledWith( - this.req, - this.user - ) - ).to.equal(true) - }) - it('should clear redirect from session', function() { this.AuthenticationController.finishLogin( this.user, @@ -1259,5 +1104,125 @@ describe('AuthenticationController', function() { ) }) }) + + describe('_afterLoginSessionSetup', function() { + beforeEach(function() {}) + + it('should call req.login', function() { + this.AuthenticationController.finishLogin( + this.user, + this.req, + this.res, + this.next + ) + this.req.login.callCount.should.equal(1) + }) + + it('should call req.session.save', function() { + this.AuthenticationController.finishLogin( + this.user, + this.req, + this.res, + this.next + ) + this.req.session.save.callCount.should.equal(1) + }) + + it('should call UserSessionsManager.trackSession', function() { + this.AuthenticationController.finishLogin( + this.user, + this.req, + this.res, + this.next + ) + this.UserSessionsManager.trackSession.callCount.should.equal(1) + }) + + describe('when req.session.save produces an error', function() { + beforeEach(function() { + this.req.session.save = sinon + .stub() + .callsArgWith(0, new Error('woops')) + }) + + it('should produce an error', function(done) { + this.AuthenticationController.finishLogin( + this.user, + this.req, + this.res, + err => { + expect(err).to.not.be.oneOf([null, undefined]) + expect(err).to.be.instanceof(Error) + done() + } + ) + }) + + it('should not call UserSessionsManager.trackSession', function(done) { + this.AuthenticationController.finishLogin( + this.user, + this.req, + this.res, + err => { + expect(err).to.exist + this.UserSessionsManager.trackSession.callCount.should.equal(0) + done() + } + ) + }) + }) + }) + + describe('_loginAsyncHandlers', function() { + beforeEach(function() { + this.AuthenticationController.finishLogin( + this.user, + this.req, + this.res, + this.next + ) + }) + + it('should call identifyUser', function() { + this.AnalyticsManager.identifyUser + .calledWith(this.user._id, this.req.sessionID) + .should.equal(true) + }) + + it('should setup the user data in the background', function() { + this.UserHandler.setupLoginData.calledWith(this.user).should.equal(true) + }) + + it('should set res.session.justLoggedIn', function() { + this.req.session.justLoggedIn.should.equal(true) + }) + + it('should record the successful login', function() { + this.AuthenticationController._recordSuccessfulLogin + .calledWith(this.user._id) + .should.equal(true) + }) + + it('should tell the rate limiter that there was a success for that email', function() { + this.LoginRateLimiter.recordSuccessfulLogin + .calledWith(this.user.email) + .should.equal(true) + }) + + it('should log the successful login', function() { + this.logger.log + .calledWith( + { email: this.user.email, user_id: this.user._id.toString() }, + 'successful log in' + ) + .should.equal(true) + }) + + it('should track the login event', function() { + this.AnalyticsManager.recordEvent + .calledWith(this.user._id, 'user-logged-in') + .should.equal(true) + }) + }) }) })