From f1653d01b725413582bea72487ef72fa54500181 Mon Sep 17 00:00:00 2001 From: Shane Kilkelly Date: Fri, 1 Jul 2016 15:33:59 +0100 Subject: [PATCH] Refactor method names in `UserSessionsManager` --- .../AuthenticationController.coffee | 10 ++-- .../Features/User/UserController.coffee | 4 +- .../Features/User/UserSessionsManager.coffee | 6 +-- .../AuthenticationControllerTests.coffee | 53 ++++++++++--------- .../coffee/User/UserControllerTests.coffee | 28 +++++----- 5 files changed, 51 insertions(+), 50 deletions(-) diff --git a/services/web/app/coffee/Features/Authentication/AuthenticationController.coffee b/services/web/app/coffee/Features/Authentication/AuthenticationController.coffee index 8851363b5f..58ff246794 100644 --- a/services/web/app/coffee/Features/Authentication/AuthenticationController.coffee +++ b/services/web/app/coffee/Features/Authentication/AuthenticationController.coffee @@ -14,7 +14,7 @@ UserSessionsManager = require("../User/UserSessionsManager") module.exports = AuthenticationController = login: (req, res, next = (error) ->) -> AuthenticationController.doLogin req.body, req, res, next - + doLogin: (options, req, res, next) -> email = options.email?.toLowerCase() password = options.password @@ -62,7 +62,7 @@ module.exports = AuthenticationController = requireLogin: () -> doRequest = (req, res, next = (error) ->) -> if !req.session.user? - AuthenticationController._redirectToLoginOrRegisterPage(req, res) + AuthenticationController._redirectToLoginOrRegisterPage(req, res) else req.user = req.session.user return next() @@ -93,9 +93,9 @@ module.exports = AuthenticationController = _redirectToLoginOrRegisterPage: (req, res)-> if req.query.zipUrl? or req.query.project_name? - return AuthenticationController._redirectToRegisterPage(req, res) + return AuthenticationController._redirectToRegisterPage(req, res) else - AuthenticationController._redirectToLoginPage(req, res) + AuthenticationController._redirectToLoginPage(req, res) _redirectToLoginPage: (req, res) -> @@ -143,5 +143,5 @@ module.exports = AuthenticationController = req.session.user = lightUser - UserSessionsManager.onLogin(user, req.sessionID, () ->) + UserSessionsManager.trackSession(user, req.sessionID, () ->) callback() diff --git a/services/web/app/coffee/Features/User/UserController.coffee b/services/web/app/coffee/Features/User/UserController.coffee index f40bf59037..c85ed98e1e 100644 --- a/services/web/app/coffee/Features/User/UserController.coffee +++ b/services/web/app/coffee/Features/User/UserController.coffee @@ -87,7 +87,7 @@ module.exports = UserController = req.session.destroy (err)-> if err logger.err err: err, 'error destorying session' - UserSessionsManager.onLogout(user, sessionId) + UserSessionsManager.untrackSession(user, sessionId) res.redirect '/login' register : (req, res, next = (error) ->)-> @@ -121,7 +121,7 @@ module.exports = UserController = logger.log user: user, "password changed" AuthenticationManager.setUserPassword user._id, newPassword1, (error) -> return next(error) if error? - UserSessionsManager.revokeAllSessions user, (err) -> + UserSessionsManager.revokeAllUserSessions user, (err) -> return next(err) if err res.send message: diff --git a/services/web/app/coffee/Features/User/UserSessionsManager.coffee b/services/web/app/coffee/Features/User/UserSessionsManager.coffee index bad1a1cb37..5d9b19d68f 100644 --- a/services/web/app/coffee/Features/User/UserSessionsManager.coffee +++ b/services/web/app/coffee/Features/User/UserSessionsManager.coffee @@ -12,7 +12,7 @@ module.exports = UserSessionsManager = _sessionKey: (sessionId) -> return "sess:#{sessionId}" - onLogin: (user, sessionId, callback=(err)-> ) -> + trackSession: (user, sessionId, callback=(err)-> ) -> logger.log {user_id: user._id, sessionId}, "onLogin handler" sessionSetKey = UserSessionsManager._sessionSetKey(user) value = UserSessionsManager._sessionKey sessionId @@ -25,7 +25,7 @@ module.exports = UserSessionsManager = return callback(err) callback() - onLogout: (user, sessionId, callback=(err)-> ) -> + untrackSession: (user, sessionId, callback=(err)-> ) -> logger.log {user_id: user._id, sessionId}, "onLogout handler" if !user logger.log {sessionId}, "no user, for some reason" @@ -41,7 +41,7 @@ module.exports = UserSessionsManager = return callback(err) callback() - revokeAllSessions: (user, callback=(err)->) -> + revokeAllUserSessions: (user, callback=(err)->) -> logger.log {user_id: user._id}, "revoking all existing sessions for user" sessionSetKey = UserSessionsManager._sessionSetKey(user) rclient.smembers sessionSetKey, (err, sessionKeys) -> diff --git a/services/web/test/UnitTests/coffee/Authentication/AuthenticationControllerTests.coffee b/services/web/test/UnitTests/coffee/Authentication/AuthenticationControllerTests.coffee index f83b38617b..d981824246 100644 --- a/services/web/test/UnitTests/coffee/Authentication/AuthenticationControllerTests.coffee +++ b/services/web/test/UnitTests/coffee/Authentication/AuthenticationControllerTests.coffee @@ -21,6 +21,10 @@ describe "AuthenticationController", -> "../User/UserHandler": @UserHandler = {setupLoginData:sinon.stub()} "logger-sharelatex": @logger = { log: sinon.stub(), error: sinon.stub() } "settings-sharelatex": {} + "../User/UserSessionsManager": @UserSessionsManager = + trackSession: sinon.stub() + untrackSession: sinon.stub() + revokeAllUserSessions: sinon.stub().callsArgWith(1, null) @user = _id: ObjectId() email: @email = "USER@example.com" @@ -57,7 +61,7 @@ describe "AuthenticationController", -> @res.statusCode.should.equal 429 done() @AuthenticationController.login(@req, @res) - + describe 'when the user is authenticated', -> beforeEach -> @LoginRateLimiter.processLoginRequest.callsArgWith(1, null, true) @@ -76,7 +80,7 @@ describe "AuthenticationController", -> @AuthenticationController.establishUserSession .calledWith(@req, @user) .should.equal true - + it "should set res.session.justLoggedIn", -> @req.session.justLoggedIn.should.equal true @@ -95,7 +99,7 @@ describe "AuthenticationController", -> @logger.log .calledWith(email: @email.toLowerCase(), user_id: @user._id.toString(), "successful log in") .should.equal true - + describe 'when the user is not authenticated', -> beforeEach -> @@ -112,7 +116,7 @@ describe "AuthenticationController", -> 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 @@ -137,12 +141,12 @@ describe "AuthenticationController", -> describe "getLoggedInUserId", -> beforeEach -> - @req = + @req = session :{} it "should return the user id from the session", (done)-> @user_id = "2134" - @req.session.user = + @req.session.user = _id:@user_id @AuthenticationController.getLoggedInUserId @req, (err, user_id)=> expect(user_id).to.equal @user_id @@ -168,7 +172,7 @@ describe "AuthenticationController", -> describe "getLoggedInUser", -> beforeEach -> @UserGetter.getUser = sinon.stub().callsArgWith(1, null, @user) - + describe "with an established session", -> beforeEach -> @req.session = @@ -189,7 +193,7 @@ describe "AuthenticationController", -> _id: "user-id-123" email: "user@sharelatex.com" @middleware = @AuthenticationController.requireLogin() - + describe "when the user is logged in", -> beforeEach -> @req.session = @@ -219,13 +223,13 @@ describe "AuthenticationController", -> beforeEach -> @req.headers = {} @AuthenticationController.httpAuth = sinon.stub() - + describe "with white listed url", -> beforeEach -> @AuthenticationController.addEndpointToLoginWhitelist "/login" @req._parsedUrl.pathname = "/login" @AuthenticationController.requireGlobalLogin @req, @res, @next - + it "should call next() directly", -> @next.called.should.equal true @@ -235,9 +239,9 @@ describe "AuthenticationController", -> @req._parsedUrl.pathname = "/login" @req.url = "/login?query=something" @AuthenticationController.requireGlobalLogin @req, @res, @next - + it "should call next() directly", -> - @next.called.should.equal true + @next.called.should.equal true describe "with http auth", -> beforeEach -> @@ -248,20 +252,20 @@ describe "AuthenticationController", -> @AuthenticationController.httpAuth .calledWith(@req, @res, @next) .should.equal true - + describe "with a user session", -> beforeEach -> @req.session = user: {"mock": "user"} @AuthenticationController.requireGlobalLogin @req, @res, @next - + it "should call next() directly", -> @next.called.should.equal true - + describe "with no login credentials", -> beforeEach -> @AuthenticationController.requireGlobalLogin @req, @res, @next - + it "should redirect to the /login page", -> @res.redirectedTo.should.equal "/login" @@ -274,7 +278,7 @@ describe "AuthenticationController", -> @req.query = {} describe "they have come directly to the url", -> - beforeEach -> + beforeEach -> @req.query = {} @middleware(@req, @res, @next) @@ -284,7 +288,7 @@ describe "AuthenticationController", -> describe "they have come via a templates link", -> - beforeEach -> + beforeEach -> @req.query.zipUrl = "something" @middleware(@req, @res, @next) @@ -293,8 +297,8 @@ describe "AuthenticationController", -> @AuthenticationController._redirectToLoginPage.calledWith(@req, @res).should.equal false describe "they have been invited to a project", -> - - beforeEach -> + + beforeEach -> @req.query.project_name = "something" @middleware(@req, @res, @next) @@ -333,7 +337,7 @@ describe "AuthenticationController", -> beforeEach -> @UserUpdater.updateUser = sinon.stub().callsArg(2) @AuthenticationController._recordSuccessfulLogin(@user._id, @callback) - + it "should increment the user.login.success metric", -> @Metrics.inc .calledWith("user.login.success") @@ -349,7 +353,7 @@ describe "AuthenticationController", -> describe "_recordFailedLogin", -> beforeEach -> @AuthenticationController._recordFailedLogin(@callback) - + it "should increment the user.login.failed metric", -> @Metrics.inc .calledWith("user.login.failed") @@ -374,13 +378,12 @@ 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 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 - diff --git a/services/web/test/UnitTests/coffee/User/UserControllerTests.coffee b/services/web/test/UnitTests/coffee/User/UserControllerTests.coffee index 098b60fe5d..bb16d872cd 100644 --- a/services/web/test/UnitTests/coffee/User/UserControllerTests.coffee +++ b/services/web/test/UnitTests/coffee/User/UserControllerTests.coffee @@ -19,9 +19,9 @@ describe "UserController", -> save:sinon.stub().callsArgWith(0) ace:{} - @UserDeleter = + @UserDeleter = deleteUser: sinon.stub().callsArgWith(1) - @UserLocator = + @UserLocator = findById: sinon.stub().callsArgWith(1, null, @user) @User = findById: sinon.stub().callsArgWith(1, null, @user) @@ -36,18 +36,18 @@ describe "UserController", -> setUserPassword: sinon.stub() @ReferalAllocator = allocate:sinon.stub() - @SubscriptionDomainHandler = + @SubscriptionDomainHandler = autoAllocate:sinon.stub() @UserUpdater = changeEmailAddress:sinon.stub() @settings = siteUrl: "sharelatex.example.com" - @UserHandler = + @UserHandler = populateGroupLicenceInvite:sinon.stub().callsArgWith(1) @UserSessionsManager = - onLogin: sinon.stub() - onLogout: sinon.stub() - revokeAllSessions: sinon.stub().callsArgWith(1, null) + trackSession: sinon.stub() + untrackSession: sinon.stub() + revokeAllUserSessions: sinon.stub().callsArgWith(1, null) @UserController = SandboxedModule.require modulePath, requires: "./UserLocator": @UserLocator "./UserDeleter": @UserDeleter @@ -62,13 +62,13 @@ describe "UserController", -> "./UserHandler":@UserHandler "./UserSessionsManager": @UserSessionsManager "settings-sharelatex": @settings - "logger-sharelatex": + "logger-sharelatex": log:-> err:-> "../../infrastructure/Metrics": inc:-> - @req = - session: + @req = + session: destroy:-> user : _id : @user_id @@ -203,12 +203,12 @@ describe "UserController", -> @UserRegistrationHandler.registerNewUserAndSendActivationEmail = sinon.stub().callsArgWith(1, null, @user, @url = "mock/url") @req.body.email = @user.email = @email = "email@example.com" @UserController.register @req, @res - + it "should register the user and send them an email", -> @UserRegistrationHandler.registerNewUserAndSendActivationEmail .calledWith(@email) .should.equal true - + it "should return the user and activation url", -> @res.json .calledWith({ @@ -238,7 +238,7 @@ describe "UserController", -> @res.send = => @AuthenticationManager.setUserPassword.called.should.equal false done() - @UserController.changePassword @req, @res + @UserController.changePassword @req, @res it "should set the new password if they do match", (done)-> @AuthenticationManager.authenticate.callsArgWith(2, null, @user) @@ -250,5 +250,3 @@ describe "UserController", -> @AuthenticationManager.setUserPassword.calledWith(@user._id, "newpass").should.equal true done() @UserController.changePassword @req, @res - -