diff --git a/services/web/app/coffee/Features/Authentication/AuthenticationController.coffee b/services/web/app/coffee/Features/Authentication/AuthenticationController.coffee index e1dc5d9d1f..9653e302b5 100644 --- a/services/web/app/coffee/Features/Authentication/AuthenticationController.coffee +++ b/services/web/app/coffee/Features/Authentication/AuthenticationController.coffee @@ -6,6 +6,7 @@ Metrics = require('../../infrastructure/Metrics') logger = require("logger-sharelatex") querystring = require('querystring') Url = require("url") +uid = require "uid" module.exports = AuthenticationController = login: (req, res, next = (error) ->) -> @@ -25,8 +26,9 @@ module.exports = AuthenticationController = if user? LoginRateLimiter.recordSuccessfulLogin email AuthenticationController._recordSuccessfulLogin user._id - AuthenticationController._establishUserSession req, user, (error) -> + AuthenticationController.establishUserSession req, user, (error) -> return next(error) if error? + req.session.justLoggedIn = true logger.log email: email, user_id: user._id.toString(), "successful log in" res.send redir: redir else @@ -118,7 +120,7 @@ module.exports = AuthenticationController = Metrics.inc "user.login.failed" callback() - _establishUserSession: (req, user, callback = (error) ->) -> + establishUserSession: (req, user, callback = (error) ->) -> lightUser = _id: user._id first_name: user.first_name @@ -126,6 +128,12 @@ module.exports = AuthenticationController = isAdmin: user.isAdmin email: user.email referal_id: user.referal_id + # Regenerate the session to get a new sessionID (cookie value) to + # protect against session fixation attacks + oldSession = req.session + req.sessionStore.generate(req) + for key, value of oldSession + req.session[key] = value + req.session.user = lightUser - req.session.justLoggedIn = true - req.session.save callback + callback() diff --git a/services/web/app/coffee/Features/User/UserController.coffee b/services/web/app/coffee/Features/User/UserController.coffee index 18f0e6de18..0c2626baf6 100644 --- a/services/web/app/coffee/Features/User/UserController.coffee +++ b/services/web/app/coffee/Features/User/UserController.coffee @@ -89,17 +89,18 @@ module.exports = next(err) else metrics.inc "user.register.success" - req.session.user = user - req.session.justRegistered = true ReferalAllocator.allocate req.session.referal_id, user._id, req.session.referal_source, req.session.referal_medium SubscriptionDomainAllocator.autoAllocate(user) - res.send - redir:redir - id:user._id.toString() - first_name: user.first_name - last_name: user.last_name - email: user.email - created: Date.now() + AuthenticationController.establishUserSession req, user, (error) -> + return callback(error) if error? + req.session.justRegistered = true + res.send + redir:redir + id:user._id.toString() + first_name: user.first_name + last_name: user.last_name + email: user.email + created: Date.now() changePassword : (req, res, next = (error) ->)-> diff --git a/services/web/package.json b/services/web/package.json index 18b3e5ab5d..cec26f13e7 100644 --- a/services/web/package.json +++ b/services/web/package.json @@ -39,6 +39,7 @@ "settings-sharelatex": "git+https://github.com/sharelatex/settings-sharelatex.git#v1.0.0", "socket.io": "0.9.16", "translations-sharelatex": "git+https://github.com/sharelatex/translations-sharelatex.git#master", + "uid": "0.0.2", "underscore": "1.6.0", "underscore.string": "^3.0.2", "v8-profiler": "^5.2.3", diff --git a/services/web/test/UnitTests/coffee/Authentication/AuthenticationControllerTests.coffee b/services/web/test/UnitTests/coffee/Authentication/AuthenticationControllerTests.coffee index cdb75494d9..41b4694d4e 100644 --- a/services/web/test/UnitTests/coffee/Authentication/AuthenticationControllerTests.coffee +++ b/services/web/test/UnitTests/coffee/Authentication/AuthenticationControllerTests.coffee @@ -40,7 +40,7 @@ describe "AuthenticationController", -> beforeEach -> @AuthenticationController._recordFailedLogin = sinon.stub() @AuthenticationController._recordSuccessfulLogin = sinon.stub() - @AuthenticationController._establishUserSession = sinon.stub().callsArg(2) + @AuthenticationController.establishUserSession = sinon.stub().callsArg(2) @req.body = email: @email password: @password @@ -68,9 +68,12 @@ describe "AuthenticationController", -> .should.equal true it "should establish the user's session", -> - @AuthenticationController._establishUserSession + @AuthenticationController.establishUserSession .calledWith(@req, @user) .should.equal true + + it "should set res.session.justLoggedIn", -> + @req.session.justLoggedIn.should.equal true it "should redirect the user to the specified location", -> expect(@res.body).to.deep.equal redir: @redir @@ -103,7 +106,7 @@ describe "AuthenticationController", -> # type: 'error' it "should not establish a session", -> - @AuthenticationController._establishUserSession.called.should.equal false + @AuthenticationController.establishUserSession.called.should.equal false it "should record a failed login", -> @AuthenticationController._recordFailedLogin.called.should.equal true @@ -272,48 +275,42 @@ describe "AuthenticationController", -> .calledWith(@req, {allow_auth_token: true}) .should.equal true + describe "_redirectToLoginOrRegisterPage", -> + beforeEach -> + @middleware = @AuthenticationController.requireLogin(@options = { load_from_db: false }) + @req.session = {} + @AuthenticationController._redirectToRegisterPage = sinon.stub() + @AuthenticationController._redirectToLoginPage = sinon.stub() + @req.query = {} - - describe "_redirectToLoginOrRegisterPage", -> - - beforeEach -> - @middleware = @AuthenticationController.requireLogin(@options = { load_from_db: false }) - @req.session = {} - @AuthenticationController._redirectToRegisterPage = sinon.stub() - @AuthenticationController._redirectToLoginPage = sinon.stub() + describe "they have come directly to the url", -> + beforeEach -> @req.query = {} + @middleware(@req, @res, @next) - describe "they have come directly to the url", -> - beforeEach -> - @req.query = {} - @middleware(@req, @res, @next) + it "should redirect to the login page", -> + @AuthenticationController._redirectToRegisterPage.calledWith(@req, @res).should.equal false + @AuthenticationController._redirectToLoginPage.calledWith(@req, @res).should.equal true - it "should redirect to the login page", -> - @AuthenticationController._redirectToRegisterPage.calledWith(@req, @res).should.equal false - @AuthenticationController._redirectToLoginPage.calledWith(@req, @res).should.equal true + describe "they have come via a templates link", -> - describe "they have come via a templates link", -> - - beforeEach -> - @req.query.zipUrl = "something" - @middleware(@req, @res, @next) - - it "should redirect to the register page", -> - @AuthenticationController._redirectToRegisterPage.calledWith(@req, @res).should.equal true - @AuthenticationController._redirectToLoginPage.calledWith(@req, @res).should.equal false - - describe "they have been invited to a project", -> - - beforeEach -> - @req.query.project_name = "something" - @middleware(@req, @res, @next) - - it "should redirect to the register page", -> - @AuthenticationController._redirectToRegisterPage.calledWith(@req, @res).should.equal true - @AuthenticationController._redirectToLoginPage.calledWith(@req, @res).should.equal false + beforeEach -> + @req.query.zipUrl = "something" + @middleware(@req, @res, @next) + it "should redirect to the register page", -> + @AuthenticationController._redirectToRegisterPage.calledWith(@req, @res).should.equal true + @AuthenticationController._redirectToLoginPage.calledWith(@req, @res).should.equal false + describe "they have been invited to a project", -> + + beforeEach -> + @req.query.project_name = "something" + @middleware(@req, @res, @next) + it "should redirect to the register page", -> + @AuthenticationController._redirectToRegisterPage.calledWith(@req, @res).should.equal true + @AuthenticationController._redirectToLoginPage.calledWith(@req, @res).should.equal false describe "_redirectToRegisterPage", -> beforeEach -> @@ -371,11 +368,13 @@ describe "AuthenticationController", -> it "should call the callback", -> @callback.called.should.equal true - describe "_establishUserSession", -> + describe "establishUserSession", -> beforeEach -> @req.session = save: sinon.stub().callsArg(0) - @AuthenticationController._establishUserSession @req, @user, @callback + @req.sessionStore = + generate: sinon.stub() + @AuthenticationController.establishUserSession @req, @user, @callback it "should set the session user to a basic version of the user", -> @req.session.user._id.should.equal @user._id @@ -384,6 +383,9 @@ describe "AuthenticationController", -> @req.session.user.last_name.should.equal @user.last_name @req.session.user.referal_id.should.equal @user.referal_id @req.session.user.isAdmin.should.equal @user.isAdmin + + it "should regenerate the session to protect against session fixation", -> + @req.sessionStore.generate.calledWith(@req).should.equal true it "should return the callback", -> @callback.called.should.equal true diff --git a/services/web/test/UnitTests/coffee/User/UserControllerTests.coffee b/services/web/test/UnitTests/coffee/User/UserControllerTests.coffee index c16ce4a7ae..090f063a44 100644 --- a/services/web/test/UnitTests/coffee/User/UserControllerTests.coffee +++ b/services/web/test/UnitTests/coffee/User/UserControllerTests.coffee @@ -29,7 +29,8 @@ describe "UserController", -> unsubscribe: sinon.stub().callsArgWith(1) @UserRegistrationHandler = registerNewUser: sinon.stub() - @AuthenticationController = {} + @AuthenticationController = + establishUserSession: sinon.stub().callsArg(2) @AuthenticationManager = authenticate: sinon.stub() setUserPassword: sinon.stub() @@ -181,7 +182,9 @@ describe "UserController", -> it "should put the user on the session and mark them as justRegistered", (done)-> @UserRegistrationHandler.registerNewUser.callsArgWith(1, null, @user) @res.send = => - assert.deepEqual @user, @req.session.user + @AuthenticationController.establishUserSession + .calledWith(@req, @user) + .should.equal true assert.equal @req.session.justRegistered, true done() @UserController.register @req, @res