From b4f81082774966dea2fde8a859069a6fb081b091 Mon Sep 17 00:00:00 2001 From: Shane Kilkelly Date: Fri, 13 Jul 2018 11:51:11 +0100 Subject: [PATCH 1/5] Move the pre-login async code into a helper function --- .../AuthenticationController.coffee | 21 +++++++++++-------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/services/web/app/coffee/Features/Authentication/AuthenticationController.coffee b/services/web/app/coffee/Features/Authentication/AuthenticationController.coffee index dfa19ed8a8..ab18b21a56 100644 --- a/services/web/app/coffee/Features/Authentication/AuthenticationController.coffee +++ b/services/web/app/coffee/Features/Authentication/AuthenticationController.coffee @@ -83,21 +83,24 @@ module.exports = AuthenticationController = return done(error) if error? if user? # async actions - UserHandler.setupLoginData(user, ()->) - LoginRateLimiter.recordSuccessfulLogin(email) - AuthenticationController._recordSuccessfulLogin(user._id) - Analytics.recordEvent(user._id, "user-logged-in", {ip:req.ip}) - Analytics.identifyUser(user._id, req.sessionID) - 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 + AuthenticationController._loginAsyncHandlers(req, email, user) return done(null, user) else AuthenticationController._recordFailedLogin() logger.log email: email, "failed log in" return done(null, false, {text: req.i18n.translate("email_or_password_wrong_try_again"), type: 'error'}) + _loginAsyncHandlers: (req, email, user) -> + UserHandler.setupLoginData(user, ()->) + LoginRateLimiter.recordSuccessfulLogin(email) + AuthenticationController._recordSuccessfulLogin(user._id) + Analytics.recordEvent(user._id, "user-logged-in", {ip:req.ip}) + Analytics.identifyUser(user._id, req.sessionID) + 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 + setInSessionUser: (req, props) -> for key, value of props if req?.session?.passport?.user? From 299de369e538a748e9d74e5b3db5d3d28784ad23 Mon Sep 17 00:00:00 2001 From: Shane Kilkelly Date: Tue, 17 Jul 2018 16:27:24 +0100 Subject: [PATCH 2/5] Refactor the way logins are finished off and sessions established --- .../AuthenticationController.coffee | 26 ++++++++++++------- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/services/web/app/coffee/Features/Authentication/AuthenticationController.coffee b/services/web/app/coffee/Features/Authentication/AuthenticationController.coffee index ab18b21a56..832f01043f 100644 --- a/services/web/app/coffee/Features/Authentication/AuthenticationController.coffee +++ b/services/web/app/coffee/Features/Authentication/AuthenticationController.coffee @@ -62,16 +62,23 @@ module.exports = AuthenticationController = if err? return next(err) if user # `user` is either a user object or false - redir = AuthenticationController._getRedirectFromSession(req) || "/project" - AuthenticationController.afterLoginSessionSetup req, user, (err) -> - if err? - return next(err) - AuthenticationController._clearRedirectFromSession(req) - res.json {redir: redir} + AuthenticationController.finishLogin(user, req, res, next) else res.json message: info )(req, res, next) + finishLogin: (user, req, res, next) -> + redir = AuthenticationController._getRedirectFromSession(req) || "/project" + AuthenticationController._loginAsyncHandlers(req, user) + AuthenticationController.afterLoginSessionSetup req, user, (err) -> + if err? + return next(err) + AuthenticationController._clearRedirectFromSession(req) + if req.headers?['accept']?.match(/^application\/json.*$/) + res.json {redir: redir} + else + res.redirect(redir) + doPassportLogin: (req, username, password, done) -> email = username.toLowerCase() LoginRateLimiter.processLoginRequest email, (err, isAllowed)-> @@ -83,20 +90,19 @@ module.exports = AuthenticationController = return done(error) if error? if user? # async actions - AuthenticationController._loginAsyncHandlers(req, email, user) return done(null, user) else AuthenticationController._recordFailedLogin() logger.log email: email, "failed log in" return done(null, false, {text: req.i18n.translate("email_or_password_wrong_try_again"), type: 'error'}) - _loginAsyncHandlers: (req, email, user) -> + _loginAsyncHandlers: (req, user) -> UserHandler.setupLoginData(user, ()->) - LoginRateLimiter.recordSuccessfulLogin(email) + LoginRateLimiter.recordSuccessfulLogin(user.email) AuthenticationController._recordSuccessfulLogin(user._id) Analytics.recordEvent(user._id, "user-logged-in", {ip:req.ip}) Analytics.identifyUser(user._id, req.sessionID) - logger.log email: email, user_id: user._id.toString(), "successful log in" + 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 user._login_req_ip = req.ip From dae9506f84c9e1c4e8e16aba1a3f5b4930a40ec7 Mon Sep 17 00:00:00 2001 From: Shane Kilkelly Date: Wed, 18 Jul 2018 09:57:05 +0100 Subject: [PATCH 3/5] Un-break unit tests --- .../AuthenticationControllerTests.coffee | 75 ++++++++----------- 1 file changed, 30 insertions(+), 45 deletions(-) diff --git a/services/web/test/unit/coffee/Authentication/AuthenticationControllerTests.coffee b/services/web/test/unit/coffee/Authentication/AuthenticationControllerTests.coffee index f631dfe9a9..c48a3bc65c 100644 --- a/services/web/test/unit/coffee/Authentication/AuthenticationControllerTests.coffee +++ b/services/web/test/unit/coffee/Authentication/AuthenticationControllerTests.coffee @@ -98,6 +98,7 @@ describe "AuthenticationController", -> @req.session.destroy = sinon.stub().callsArgWith(0, null) @req.session.save = sinon.stub().callsArgWith(0, null) @req.sessionStore = {generate: sinon.stub()} + @AuthenticationController.finishLogin = sinon.stub() @passport.authenticate.callsArgWith(1, null, @user, @info) @err = new Error('woops') @@ -123,27 +124,10 @@ describe "AuthenticationController", -> afterEach -> delete @req.session.postLoginRedirect - it 'should call req.login', () -> + it 'should call finishLogin', () -> @AuthenticationController.passportLogin @req, @res, @next - @req.login.callCount.should.equal 1 - @req.login.calledWith(@user).should.equal true - - it 'should send a json response with redirect', () -> - @AuthenticationController.passportLogin @req, @res, @next - @res.json.callCount.should.equal 1 - @res.json.calledWith({redir: 'some_redirect'}).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 + @AuthenticationController.finishLogin.callCount.should.equal 1 + @AuthenticationController.finishLogin.calledWith(@user).should.equal true describe 'when authenticate does not produce a user', -> @@ -151,9 +135,9 @@ describe "AuthenticationController", -> @info = {text: 'a', type: 'b'} @passport.authenticate.callsArgWith(1, null, false, @info) - it 'should not call req.login', () -> + it 'should not call finishLogin', () -> @AuthenticationController.passportLogin @req, @res, @next - @req.login.callCount.should.equal 0 + @AuthenticationController.finishLogin.callCount.should.equal 0 it 'should not send a json response with redirect', () -> @AuthenticationController.passportLogin @req, @res, @next @@ -255,7 +239,6 @@ describe "AuthenticationController", -> @LoginRateLimiter.processLoginRequest.callsArgWith(1, null, true) @AuthenticationManager.authenticate = sinon.stub().callsArgWith(2, null, @user) @req.sessionID = Math.random() - @AnalyticsManager.identifyUser = sinon.stub() @AuthenticationController.doPassportLogin(@req, @req.body.email, @req.body.password, @cb) it "should attempt to authorise the user", -> @@ -263,35 +246,37 @@ describe "AuthenticationController", -> .calledWith(email: @email.toLowerCase(), @password) .should.equal true - it "should call identifyUser", -> - @AnalyticsManager.identifyUser.calledWith(@user._id, @req.sessionID).should.equal true - - it "should setup the user data in the background", -> - @UserHandler.setupLoginData.calledWith(@user).should.equal true - it "should establish the user's session", -> @cb.calledWith(null, @user).should.equal true - it "should set res.session.justLoggedIn", -> - @req.session.justLoggedIn.should.equal true + # TODO: move these to a test for the async handlers, or for finishLogin - it "should record the successful login", -> - @AuthenticationController._recordSuccessfulLogin - .calledWith(@user._id) - .should.equal true + # it "should call identifyUser", -> + # @AnalyticsManager.identifyUser.calledWith(@user._id, @req.sessionID).should.equal true - it "should tell the rate limiter that there was a success for that email", -> - @LoginRateLimiter.recordSuccessfulLogin.calledWith(@email.toLowerCase()).should.equal true + # it "should setup the user data in the background", -> + # @UserHandler.setupLoginData.calledWith(@user).should.equal true - it "should log the successful login", -> - @logger.log - .calledWith(email: @email.toLowerCase(), user_id: @user._id.toString(), "successful log in") - .should.equal true + # it "should set res.session.justLoggedIn", -> + # @req.session.justLoggedIn.should.equal true - it "should track the login event", -> - @AnalyticsManager.recordEvent - .calledWith(@user._id, "user-logged-in") - .should.equal true + # it "should record the successful login", -> + # @AuthenticationController._recordSuccessfulLogin + # .calledWith(@user._id) + # .should.equal true + + # it "should tell the rate limiter that there was a success for that email", -> + # @LoginRateLimiter.recordSuccessfulLogin.calledWith(@email.toLowerCase()).should.equal true + + # it "should log the successful login", -> + # @logger.log + # .calledWith(email: @email.toLowerCase(), user_id: @user._id.toString(), "successful log in") + # .should.equal true + + # it "should track the login event", -> + # @AnalyticsManager.recordEvent + # .calledWith(@user._id, "user-logged-in") + # .should.equal true describe 'when the user is not authenticated', -> beforeEach -> From 943bfe98aad95c4df8a9ca3c3f07bf493385fc5f Mon Sep 17 00:00:00 2001 From: Shane Kilkelly Date: Wed, 18 Jul 2018 11:13:42 +0100 Subject: [PATCH 4/5] Unit test for `_loginAsyncHandlers` --- .../AuthenticationControllerTests.coffee | 49 +++++++++++-------- 1 file changed, 28 insertions(+), 21 deletions(-) diff --git a/services/web/test/unit/coffee/Authentication/AuthenticationControllerTests.coffee b/services/web/test/unit/coffee/Authentication/AuthenticationControllerTests.coffee index c48a3bc65c..2405e9fba7 100644 --- a/services/web/test/unit/coffee/Authentication/AuthenticationControllerTests.coffee +++ b/services/web/test/unit/coffee/Authentication/AuthenticationControllerTests.coffee @@ -249,34 +249,41 @@ describe "AuthenticationController", -> it "should establish the user's session", -> @cb.calledWith(null, @user).should.equal true - # TODO: move these to a test for the async handlers, or for finishLogin + describe '_loginAsyncHandlers', -> + beforeEach -> + @UserHandler.setupLoginData = sinon.stub() + @LoginRateLimiter.recordSuccessfulLogin = sinon.stub() + @AuthenticationController._recordSuccessfulLogin = sinon.stub() + @AnalyticsManager.recordEvent = sinon.stub() + @AnalyticsManager.identifyUser = sinon.stub() + @AuthenticationController._loginAsyncHandlers(@req, @user) - # it "should call identifyUser", -> - # @AnalyticsManager.identifyUser.calledWith(@user._id, @req.sessionID).should.equal true + it "should call identifyUser", -> + @AnalyticsManager.identifyUser.calledWith(@user._id, @req.sessionID).should.equal true - # it "should setup the user data in the background", -> - # @UserHandler.setupLoginData.calledWith(@user).should.equal true + it "should setup the user data in the background", -> + @UserHandler.setupLoginData.calledWith(@user).should.equal true - # it "should set res.session.justLoggedIn", -> - # @req.session.justLoggedIn.should.equal true + it "should set res.session.justLoggedIn", -> + @req.session.justLoggedIn.should.equal true - # it "should record the successful login", -> - # @AuthenticationController._recordSuccessfulLogin - # .calledWith(@user._id) - # .should.equal true + it "should record the successful login", -> + @AuthenticationController._recordSuccessfulLogin + .calledWith(@user._id) + .should.equal true - # it "should tell the rate limiter that there was a success for that email", -> - # @LoginRateLimiter.recordSuccessfulLogin.calledWith(@email.toLowerCase()).should.equal true + it "should tell the rate limiter that there was a success for that email", -> + @LoginRateLimiter.recordSuccessfulLogin.calledWith(@user.email).should.equal true - # it "should log the successful login", -> - # @logger.log - # .calledWith(email: @email.toLowerCase(), user_id: @user._id.toString(), "successful log in") - # .should.equal true + it "should log the successful login", -> + @logger.log + .calledWith(email: @user.email, user_id: @user._id.toString(), "successful log in") + .should.equal true - # it "should track the login event", -> - # @AnalyticsManager.recordEvent - # .calledWith(@user._id, "user-logged-in") - # .should.equal true + it "should track the login event", -> + @AnalyticsManager.recordEvent + .calledWith(@user._id, "user-logged-in") + .should.equal true describe 'when the user is not authenticated', -> beforeEach -> From c423672b552cc68594b8f5d29eadecaee41905e3 Mon Sep 17 00:00:00 2001 From: Shane Kilkelly Date: Wed, 18 Jul 2018 12:08:34 +0100 Subject: [PATCH 5/5] Unit test for `finishLogin` --- .../AuthenticationControllerTests.coffee | 53 +++++++++++++++++++ 1 file changed, 53 insertions(+) diff --git a/services/web/test/unit/coffee/Authentication/AuthenticationControllerTests.coffee b/services/web/test/unit/coffee/Authentication/AuthenticationControllerTests.coffee index 2405e9fba7..8482c28e04 100644 --- a/services/web/test/unit/coffee/Authentication/AuthenticationControllerTests.coffee +++ b/services/web/test/unit/coffee/Authentication/AuthenticationControllerTests.coffee @@ -579,3 +579,56 @@ describe "AuthenticationController", -> @AuthenticationController._clearRedirectFromSession(@req) expect(@req.session.postLoginRedirect).to.equal undefined + + describe 'finishLogin', -> + # - get redirect + # - async handlers + # - afterLoginSessionSetup + # - clear redirect + # - issue redir, two ways + beforeEach -> + @AuthenticationController._getRedirectFromSession = sinon.stub().returns '/some/page' + @AuthenticationController._loginAsyncHandlers = sinon.stub() + @AuthenticationController.afterLoginSessionSetup = sinon.stub().callsArgWith(2, null) + @AuthenticationController._clearRedirectFromSession = sinon.stub() + @req.headers = {accept: 'application/json, whatever'} + @res.json = sinon.stub() + @res.redirect = sinon.stub() + + it 'should extract the redirect from the session', () -> + @AuthenticationController.finishLogin(@user, @req, @res, @next) + expect(@AuthenticationController._getRedirectFromSession.callCount).to.equal 1 + expect(@AuthenticationController._getRedirectFromSession.calledWith(@req)).to.equal true + + it 'should call the async handlers', () -> + @AuthenticationController.finishLogin(@user, @req, @res, @next) + expect(@AuthenticationController._loginAsyncHandlers.callCount).to.equal 1 + expect(@AuthenticationController._loginAsyncHandlers.calledWith(@req, @user)).to.equal true + + it 'should call afterLoginSessionSetup', () -> + @AuthenticationController.finishLogin(@user, @req, @res, @next) + expect(@AuthenticationController.afterLoginSessionSetup.callCount).to.equal 1 + expect(@AuthenticationController.afterLoginSessionSetup.calledWith(@req, @user)).to.equal true + + it 'should clear redirect from session', () -> + @AuthenticationController.finishLogin(@user, @req, @res, @next) + expect(@AuthenticationController._clearRedirectFromSession.callCount).to.equal 1 + expect(@AuthenticationController._clearRedirectFromSession.calledWith(@req)).to.equal true + + it 'should issue a json response with a redirect', () -> + @AuthenticationController.finishLogin(@user, @req, @res, @next) + expect(@res.json.callCount).to.equal 1 + expect(@res.redirect.callCount).to.equal 0 + expect(@res.json.calledWith({ redir: '/some/page' })).to.equal true + + describe 'with a non-json request', -> + beforeEach -> + @req.headers = {} + @res.json = sinon.stub() + @res.redirect = sinon.stub() + + it 'should issue a plain redirect', () -> + @AuthenticationController.finishLogin(@user, @req, @res, @next) + expect(@res.json.callCount).to.equal 0 + expect(@res.redirect.callCount).to.equal 1 + expect(@res.redirect.calledWith('/some/page')).to.equal true