diff --git a/services/web/app/coffee/Features/Authentication/AuthenticationController.coffee b/services/web/app/coffee/Features/Authentication/AuthenticationController.coffee index 161b0f2078..8d12acfb64 100644 --- a/services/web/app/coffee/Features/Authentication/AuthenticationController.coffee +++ b/services/web/app/coffee/Features/Authentication/AuthenticationController.coffee @@ -62,24 +62,21 @@ module.exports = AuthenticationController = isUserLoggedIn: (req) -> user_id = AuthenticationController.getLoggedInUserId(req) - return user_id? + return user_id != null # TODO: perhaps should produce an error if the current user is not present getLoggedInUserId: (req) -> user = AuthenticationController.getSessionUser(req) - if user? + if user return user._id else return null getSessionUser: (req) -> - # old sessions - if req?.session?.user?._id? + if req?.session?.user? return req.session.user - # new passport sessions - else if req?.session?.passport?.user?._id? + else if req?.session?.passport?.user return req.session.passport.user - # neither else return null @@ -110,7 +107,7 @@ module.exports = AuthenticationController = if req.headers['authorization']? return AuthenticationController.httpAuth(req, res, next) - else if AuthenticationController.isUserLoggedIn()? + else if AuthenticationController.isUserLoggedIn(req) return next() else logger.log url:req.url, "user trying to access endpoint not in global whitelist" diff --git a/services/web/test/UnitTests/coffee/Authentication/AuthenticationControllerTests.coffee b/services/web/test/UnitTests/coffee/Authentication/AuthenticationControllerTests.coffee index 86c5e5840f..3e0812d3ff 100644 --- a/services/web/test/UnitTests/coffee/Authentication/AuthenticationControllerTests.coffee +++ b/services/web/test/UnitTests/coffee/Authentication/AuthenticationControllerTests.coffee @@ -39,35 +39,52 @@ describe "AuthenticationController", -> @callback = @next = sinon.stub() tk.freeze(Date.now()) - afterEach -> tk.reset() - describe "login", -> + describe 'getSessionUser', -> + + it 'should get the user object from session', -> + @req.session = + passport: + user: {_id: 'one'} + user = @AuthenticationController.getSessionUser(@req) + expect(user).to.deep.equal {_id: 'one'} + + it 'should work with legacy sessions', -> + @req.session = + user: {_id: 'one'} + user = @AuthenticationController.getSessionUser(@req) + expect(user).to.deep.equal {_id: 'one'} + + describe "doPassportLogin", -> 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 redir: @redir = "/path/to/redir/to" + @cb = sinon.stub() describe "when the users rate limit", -> - it "should block the request if the limit has been exceeded", (done)-> + beforeEach -> @LoginRateLimiter.processLoginRequest.callsArgWith(1, null, false) - @res = - send: (code)=> - @res.statusCode.should.equal 429 - done() - @AuthenticationController.login(@req, @res) + + it "should block the request if the limit has been exceeded", (done)-> + @AuthenticationController.doPassportLogin(@req, @req.body.email, @req.body.password, @cb) + @cb.callCount.should.equal 1 + @cb.calledWith(null, null).should.equal true + done() describe 'when the user is authenticated', -> beforeEach -> + @cb = sinon.stub() @LoginRateLimiter.processLoginRequest.callsArgWith(1, null, true) @AuthenticationManager.authenticate = sinon.stub().callsArgWith(2, null, @user) - @AuthenticationController.login(@req, @res) + @AuthenticationController.doPassportLogin(@req, @req.body.email, @req.body.password, @cb) it "should attempt to authorise the user", -> @AuthenticationManager.authenticate @@ -78,15 +95,13 @@ describe "AuthenticationController", -> @UserHandler.setupLoginData.calledWith(@user).should.equal true it "should establish the user's session", -> - @AuthenticationController.establishUserSession - .calledWith(@req, @user) - .should.equal true + @cb.calledWith(null, @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 + expect(@req._redir).to.deep.equal @redir it "should record the successful login", -> @AuthenticationController._recordSuccessfulLogin @@ -106,23 +121,22 @@ describe "AuthenticationController", -> .calledWith(@user._id, "user-logged-in") .should.equal true - describe 'when the user is not authenticated', -> beforeEach -> @LoginRateLimiter.processLoginRequest.callsArgWith(1, null, true) @AuthenticationManager.authenticate = sinon.stub().callsArgWith(2, null, null) - @AuthenticationController.login(@req, @res) + @cb = sinon.stub() + @AuthenticationController.doPassportLogin(@req, @req.body.email, @req.body.password, @cb) - it "should return an error", -> + it "should not establish the login", -> + @cb.callCount.should.equal 1 + @cb.calledWith(null, false) # @res.body.should.exist - expect(@res.body.message).to.exist + expect(@cb.lastCall.args[2]).to.contain.all.keys ['message'] # message: # text: 'Your email or password were incorrect. Please try again', # type: 'error' - it "should not establish a session", -> - @AuthenticationController.establishUserSession.called.should.equal false - it "should not setup the user data in the background", -> @UserHandler.setupLoginData.called.should.equal false @@ -139,10 +153,11 @@ describe "AuthenticationController", -> @LoginRateLimiter.processLoginRequest.callsArgWith(1, null, true) @req.body.redir = "http://www.facebook.com/test" @AuthenticationManager.authenticate = sinon.stub().callsArgWith(2, null, @user) - @AuthenticationController.login(@req, @res) + @cb = sinon.stub() + @AuthenticationController.doPassportLogin(@req, @req.body.email, @req.body.password, @cb) it "should only redirect to the local path", -> - expect(@res.body).to.deep.equal redir: "/test" + expect(@req._redir).to.equal "/test" describe "getLoggedInUserId", -> @@ -150,34 +165,42 @@ describe "AuthenticationController", -> @req = session :{} - it "should return the user id from the session", (done)-> + it "should return the user id from the session", ()-> @user_id = "2134" @req.session.user = _id:@user_id - @AuthenticationController.getLoggedInUserId @req, (err, user_id)=> - expect(user_id).to.equal @user_id - done() + result = @AuthenticationController.getLoggedInUserId @req + expect(result).to.equal @user_id - it "should return null if there is no user on the session", (done)-> - @AuthenticationController.getLoggedInUserId @req, (err, user_id)=> - expect(user_id).to.be.null - done() + it 'should return user for passport session', () -> + @user_id = "2134" + @req.session = { + passport: { + user: { + _id:@user_id + } + } + } + result = @AuthenticationController.getLoggedInUserId @req + expect(result).to.equal @user_id - it "should return null if there is no session", (done)-> + it "should return null if there is no user on the session", ()-> + result = @AuthenticationController.getLoggedInUserId @req + expect(result).to.equal null + + it "should return null if there is no session", ()-> @req = {} - @AuthenticationController.getLoggedInUserId @req, (err, user_id)=> - expect(user_id).to.be.null - done() + result = @AuthenticationController.getLoggedInUserId @req + expect(result).to.equal null - it "should return null if there is no req", (done)-> + it "should return null if there is no req", ()-> @req = {} - @AuthenticationController.getLoggedInUserId @req, (err, user_id)=> - expect(user_id).to.be.null - done() + result = @AuthenticationController.getLoggedInUserId @req + expect(result).to.equal null describe "getLoggedInUser", -> beforeEach -> - @UserGetter.getUser = sinon.stub().callsArgWith(1, null, @user) + @UserGetter.getUser = sinon.stub().callsArgWith(2, null, @user) describe "with an established session", -> beforeEach -> @@ -209,9 +232,6 @@ describe "AuthenticationController", -> } @middleware(@req, @res, @next) - it "should set the user property on the request", -> - @req.user.should.deep.equal @user - it "should call the next method in the chain", -> @next.called.should.equal true @@ -367,33 +387,3 @@ describe "AuthenticationController", -> it "should call the callback", -> @callback.called.should.equal true - - describe "establishUserSession", -> - beforeEach -> - @req.session = - save: sinon.stub().callsArg(0) - destroy : sinon.stub() - @req.sessionStore = - generate: sinon.stub() - @req.ip = "1.2.3.4" - @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 - @req.session.user.email.should.equal @user.email - @req.session.user.first_name.should.equal @user.first_name - @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 - @req.session.user.ip_address.should.equal @req.ip - expect(typeof @req.session.user.ip_address).to.equal 'string' - expect(typeof @req.session.user.session_created).to.equal 'string' - - it "should destroy the session", -> - @req.session.destroy.called.should.equal true - - 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