diff --git a/services/web/app/coffee/Features/Authentication/AuthenticationController.coffee b/services/web/app/coffee/Features/Authentication/AuthenticationController.coffee index 443cc1fece..52d20e9194 100644 --- a/services/web/app/coffee/Features/Authentication/AuthenticationController.coffee +++ b/services/web/app/coffee/Features/Authentication/AuthenticationController.coffee @@ -39,7 +39,19 @@ module.exports = AuthenticationController = return next(err) if user # `user` is either a user object or false req.login user, (err) -> - res.json {redir: req._redir} + # Regenerate the session to get a new sessionID (cookie value) to + # protect against session fixation attacks + oldSession = req.session + req.session.destroy() + req.sessionStore.generate(req) + for key, value of oldSession + req.session[key] = value + req.session.save (err) -> + if err? + logger.err {user_id: user._id}, "error saving regenerated session after login" + return next(err) + UserSessionsManager.trackSession(user, req.sessionID, () ->) + res.json {redir: req._redir} else res.json message: info )(req, res, next) @@ -60,9 +72,8 @@ module.exports = AuthenticationController = LoginRateLimiter.recordSuccessfulLogin(email) AuthenticationController._recordSuccessfulLogin(user._id) Analytics.recordEvent(user._id, "user-logged-in") - UserSessionsManager.trackSession(user, req.sessionID, () ->) - req.session.justLoggedIn = true logger.log email: email, user_id: user._id.toString(), "successful log in" + req.session.justLoggedIn = true # capture the request ip for use when creating the session user._login_req_ip = req.ip req._redir = redir diff --git a/services/web/test/UnitTests/coffee/Authentication/AuthenticationControllerTests.coffee b/services/web/test/UnitTests/coffee/Authentication/AuthenticationControllerTests.coffee index 059cd202d0..65591dfbb3 100644 --- a/services/web/test/UnitTests/coffee/Authentication/AuthenticationControllerTests.coffee +++ b/services/web/test/UnitTests/coffee/Authentication/AuthenticationControllerTests.coffee @@ -20,7 +20,7 @@ describe "AuthenticationController", -> "../Security/LoginRateLimiter": @LoginRateLimiter = { processLoginRequest:sinon.stub(), recordSuccessfulLogin:sinon.stub() } "../User/UserHandler": @UserHandler = {setupLoginData:sinon.stub()} "../Analytics/AnalyticsManager": @AnalyticsManager = { recordEvent: sinon.stub() } - "logger-sharelatex": @logger = { log: sinon.stub(), error: sinon.stub() } + "logger-sharelatex": @logger = { log: sinon.stub(), error: sinon.stub(), err: sinon.stub() } "settings-sharelatex": {} "passport": @passport = authenticate: sinon.stub().returns(sinon.stub()) @@ -50,6 +50,10 @@ describe "AuthenticationController", -> @info = null @req.login = sinon.stub().callsArgWith(1, null) @res.json = sinon.stub() + @req.session = @session = {test: 'test'} + @req.session.destroy = sinon.stub() + @req.session.save = sinon.stub().callsArgWith(0, null) + @req.sessionStore = {generate: sinon.stub()} @passport.authenticate.callsArgWith(1, null, @user, @info) it 'should call passport.authenticate', () -> @@ -85,6 +89,18 @@ describe "AuthenticationController", -> @res.json.callCount.should.equal 1 @res.json.calledWith({redir: @req._redir}).should.equal true + describe 'when session.save produces an error', () -> + beforeEach -> + @req.session.save = sinon.stub().callsArgWith(0, new Error('woops')) + + it 'should return next with an error', () -> + @AuthenticationController.passportLogin @req, @res, @next + @next.calledWith(@err).should.equal true + + it 'should not return json', () -> + @AuthenticationController.passportLogin @req, @res, @next + @res.json.callCount.should.equal 0 + describe 'when authenticate does not produce a user', -> beforeEach ->