From 8089bb55a48dbf7ad35085f0d0b2b8b01bc13b06 Mon Sep 17 00:00:00 2001 From: Shane Kilkelly Date: Tue, 22 Nov 2016 14:24:36 +0000 Subject: [PATCH] use session for the post-login redirect, remove `redir` query string. --- .../AuthenticationController.coffee | 34 +++++--- .../Features/User/UserPagesController.coffee | 2 - services/web/app/views/user/login.jade | 1 - .../AuthenticationControllerTests.coffee | 59 +++++++++----- .../User/UserPagesControllerTests.coffee | 16 ---- .../coffee/ProjectInviteTests.coffee | 77 +++++++------------ 6 files changed, 92 insertions(+), 97 deletions(-) diff --git a/services/web/app/coffee/Features/Authentication/AuthenticationController.coffee b/services/web/app/coffee/Features/Authentication/AuthenticationController.coffee index 3c19f0de25..765065823b 100644 --- a/services/web/app/coffee/Features/Authentication/AuthenticationController.coffee +++ b/services/web/app/coffee/Features/Authentication/AuthenticationController.coffee @@ -62,17 +62,18 @@ 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) - res.json {redir: req._redir} + AuthenticationController._clearRedirectFromSession(req) + res.json {redir: redir} else res.json message: info )(req, res, next) doPassportLogin: (req, username, password, done) -> email = username.toLowerCase() - redir = Url.parse(req?.body?.redir or "/project").path LoginRateLimiter.processLoginRequest email, (err, isAllowed)-> return done(err) if err? if !isAllowed @@ -90,7 +91,6 @@ module.exports = AuthenticationController = req.session.justLoggedIn = true # capture the request ip for use when creating the session user._login_req_ip = req.ip - req._redir = redir return done(null, user) else AuthenticationController._recordFailedLogin() @@ -127,7 +127,7 @@ module.exports = AuthenticationController = requireLogin: () -> doRequest = (req, res, next = (error) ->) -> if !AuthenticationController.isUserLoggedIn(req) - AuthenticationController._redirectToLoginOrRegisterPage(req, res) + AuthenticationController._redirectToLoginOrRegisterPage(req, res, next) else req.user = AuthenticationController.getSessionUser(req) next() @@ -156,22 +156,22 @@ module.exports = AuthenticationController = logger.err user:user, pass:pass, "invalid login details" return isValid - _redirectToLoginOrRegisterPage: (req, res)-> + _redirectToLoginOrRegisterPage: (req, res, next)-> if req.query.zipUrl? or req.query.project_name? - return AuthenticationController._redirectToRegisterPage(req, res) + return AuthenticationController._redirectToRegisterPage(req, res, next) else - AuthenticationController._redirectToLoginPage(req, res) + AuthenticationController._redirectToLoginPage(req, res, next) - _redirectToLoginPage: (req, res) -> + _redirectToLoginPage: (req, res, next) -> logger.log url: req.url, "user not logged in so redirecting to login page" - req.query.redir = req.path + AuthenticationController._setRedirectInSession(req) url = "/login?#{querystring.stringify(req.query)}" res.redirect url Metrics.inc "security.login-redirect" - _redirectToRegisterPage: (req, res) -> + _redirectToRegisterPage: (req, res, next) -> logger.log url: req.url, "user not logged in so redirecting to register page" - req.query.redir = req.path + AuthenticationController._setRedirectInSession(req) url = "/register?#{querystring.stringify(req.query)}" res.redirect url Metrics.inc "security.login-redirect" @@ -188,3 +188,15 @@ module.exports = AuthenticationController = _recordFailedLogin: (callback = (error) ->) -> Metrics.inc "user.login.failed" callback() + + _setRedirectInSession: (req) -> + target = if Object.keys(req.query) then "#{req.path}?#{querystring.stringify(req.query)}" else req.path + if req.session? + req.session.postLoginRedirect = target + + _getRedirectFromSession: (req) -> + return req?.session?.postLoginRedirect || null + + _clearRedirectFromSession: (req) -> + if req.session? + delete req.session.postLoginRedirect diff --git a/services/web/app/coffee/Features/User/UserPagesController.coffee b/services/web/app/coffee/Features/User/UserPagesController.coffee index 76d88803a7..00494d71e5 100644 --- a/services/web/app/coffee/Features/User/UserPagesController.coffee +++ b/services/web/app/coffee/Features/User/UserPagesController.coffee @@ -20,7 +20,6 @@ module.exports = res.render 'user/register', title: 'register' - redir: req.query.redir sharedProjectData: sharedProjectData newTemplateData: newTemplateData new_email:req.query.new_email || "" @@ -51,7 +50,6 @@ module.exports = loginPage : (req, res)-> res.render 'user/login', title: 'login', - redir: req.query.redir, email: req.query.email settingsPage : (req, res, next)-> diff --git a/services/web/app/views/user/login.jade b/services/web/app/views/user/login.jade index a6587782bf..8339f27189 100644 --- a/services/web/app/views/user/login.jade +++ b/services/web/app/views/user/login.jade @@ -10,7 +10,6 @@ block content h1 #{translate("log_in")} form(async-form="login", name="loginForm", action='/login', method="POST", ng-cloak) input(name='_csrf', type='hidden', value=csrfToken) - input(name='redir', type='hidden', value=redir) form-messages(for="loginForm") .form-group input.form-control( diff --git a/services/web/test/UnitTests/coffee/Authentication/AuthenticationControllerTests.coffee b/services/web/test/UnitTests/coffee/Authentication/AuthenticationControllerTests.coffee index e5a4486d4a..7dd2617e66 100644 --- a/services/web/test/UnitTests/coffee/Authentication/AuthenticationControllerTests.coffee +++ b/services/web/test/UnitTests/coffee/Authentication/AuthenticationControllerTests.coffee @@ -91,7 +91,10 @@ describe "AuthenticationController", -> @info = null @req.login = sinon.stub().callsArgWith(1, null) @res.json = sinon.stub() - @req.session = @session = {passport: {user: @user}} + @req.session = @session = { + passport: {user: @user}, + postLoginRedirect: "/path/to/redir/to" + } @req.session.destroy = sinon.stub().callsArgWith(0, null) @req.session.save = sinon.stub().callsArgWith(0, null) @req.sessionStore = {generate: sinon.stub()} @@ -114,11 +117,11 @@ describe "AuthenticationController", -> describe 'when authenticate produces a user', -> beforeEach -> - @req._redir = 'some_redirect' + @req.session.postLoginRedirect = 'some_redirect' @passport.authenticate.callsArgWith(1, null, @user, @info) afterEach -> - delete @req._redir + delete @req.session.postLoginRedirect it 'should call req.login', () -> @AuthenticationController.passportLogin @req, @res, @next @@ -128,7 +131,7 @@ describe "AuthenticationController", -> it 'should send a json response with redirect', () -> @AuthenticationController.passportLogin @req, @res, @next @res.json.callCount.should.equal 1 - @res.json.calledWith({redir: @req._redir}).should.equal true + @res.json.calledWith({redir: 'some_redirect'}).should.equal true describe 'when session.save produces an error', () -> beforeEach -> @@ -230,7 +233,8 @@ describe "AuthenticationController", -> @req.body = email: @email password: @password - redir: @redir = "/path/to/redir/to" + session: + postLoginRedirect: "/path/to/redir/to" @cb = sinon.stub() describe "when the users rate limit", -> @@ -313,17 +317,6 @@ describe "AuthenticationController", -> .calledWith(email: @email.toLowerCase(), "failed log in") .should.equal true - describe "with a URL to a different domain", -> - beforeEach -> - @LoginRateLimiter.processLoginRequest.callsArgWith(1, null, true) - @req.body.redir = "http://www.facebook.com/test" - @AuthenticationManager.authenticate = sinon.stub().callsArgWith(2, null, @user) - @cb = sinon.stub() - @AuthenticationController.doPassportLogin(@req, @req.body.email, @req.body.password, @cb) - - it "should only redirect to the local path", -> - expect(@req._redir).to.equal "/test" - describe "getLoggedInUserId", -> beforeEach -> @@ -488,8 +481,8 @@ describe "AuthenticationController", -> @AuthenticationController._redirectToRegisterPage(@req, @res) it "should redirect to the register page with a query string attached", -> - @res.redirectedTo - .should.equal "/register?extra_query=foo&redir=%2Ftarget%2Furl" + @req.session.postLoginRedirect.should.equal '/target/url?extra_query=foo' + @res.redirectedTo.should.equal "/register?extra_query=foo" it "should log out a message", -> @logger.log @@ -504,7 +497,8 @@ describe "AuthenticationController", -> @AuthenticationController._redirectToLoginPage(@req, @res) it "should redirect to the register page with a query string attached", -> - @res.redirectedTo.should.equal "/login?extra_query=foo&redir=%2Ftarget%2Furl" + @req.session.postLoginRedirect.should.equal '/target/url?extra_query=foo' + @res.redirectedTo.should.equal "/login?extra_query=foo" describe "_recordSuccessfulLogin", -> @@ -535,3 +529,30 @@ describe "AuthenticationController", -> it "should call the callback", -> @callback.called.should.equal true + + + describe '_setRedirectInSession', -> + beforeEach -> + @req = {session: {}} + @req.path = "/somewhere" + @req.query = {one: "1"} + + it 'should set redirect property on session', -> + @AuthenticationController._setRedirectInSession(@req) + expect(@req.session.postLoginRedirect).to.equal "/somewhere?one=1" + + describe '_getRedirectFromSession', -> + beforeEach -> + @req = {session: {postLoginRedirect: "/a?b=c"}} + + it 'should get redirect property from session', -> + expect(@AuthenticationController._getRedirectFromSession(@req)).to.equal "/a?b=c" + + describe '_clearRedirectFromSession', -> + beforeEach -> + @req = {session: {postLoginRedirect: "/a?b=c"}} + + it 'should remove the redirect property from session', -> + @AuthenticationController._clearRedirectFromSession(@req) + expect(@req.session.postLoginRedirect).to.equal undefined + diff --git a/services/web/test/UnitTests/coffee/User/UserPagesControllerTests.coffee b/services/web/test/UnitTests/coffee/User/UserPagesControllerTests.coffee index bb9fa22a15..24ca3eeeda 100644 --- a/services/web/test/UnitTests/coffee/User/UserPagesControllerTests.coffee +++ b/services/web/test/UnitTests/coffee/User/UserPagesControllerTests.coffee @@ -56,14 +56,6 @@ describe "UserPagesController", -> done() @UserPagesController.registerPage @req, @res - it "should set the redirect", (done)-> - redirect = "/go/here/please" - @req.query.redir = redirect - @res.render = (page, opts)=> - opts.redir.should.equal redirect - done() - @UserPagesController.registerPage @req, @res - it "should set sharedProjectData", (done)-> @req.query.project_name = "myProject" @req.query.user_first_name = "user_first_name_here" @@ -98,14 +90,6 @@ describe "UserPagesController", -> done() @UserPagesController.loginPage @req, @res - it "should set the redirect", (done)-> - redirect = "/go/here/please" - @req.query.redir = redirect - @res.render = (page, opts)=> - opts.redir.should.equal redirect - done() - @UserPagesController.loginPage @req, @res - describe 'sessionsPage', -> beforeEach -> diff --git a/services/web/test/acceptance/coffee/ProjectInviteTests.coffee b/services/web/test/acceptance/coffee/ProjectInviteTests.coffee index c322317b2f..3068892a68 100644 --- a/services/web/test/acceptance/coffee/ProjectInviteTests.coffee +++ b/services/web/test/acceptance/coffee/ProjectInviteTests.coffee @@ -60,7 +60,7 @@ tryAcceptInvite = (user, invite, callback=(err, response, body)->) -> token: invite.token }, callback -tryRegisterUser = (user, email, redir, callback=(err, response, body)->) -> +tryRegisterUser = (user, email, callback=(err, response, body)->) -> user.getCsrfToken (error) => return callback(error) if error? user.request.post { @@ -68,7 +68,6 @@ tryRegisterUser = (user, email, redir, callback=(err, response, body)->) -> json: email: email password: "some_weird_password" - redir: redir }, callback tryFollowLoginLink = (user, loginLink, callback=(err, response, body)->) -> @@ -76,7 +75,7 @@ tryFollowLoginLink = (user, loginLink, callback=(err, response, body)->) -> return callback(error) if error? user.request.get loginLink, callback -tryLoginUser = (user, redir, callback=(err, response, body)->) -> +tryLoginUser = (user, callback=(err, response, body)->) -> user.getCsrfToken (error) => return callback(error) if error? user.request.post { @@ -84,7 +83,6 @@ tryLoginUser = (user, redir, callback=(err, response, body)->) -> json: email: user.email password: user.password - redir: redir }, callback tryGetInviteList = (user, projectId, callback=(err, response, body)->) -> @@ -143,35 +141,34 @@ expectInviteRedirectToRegister = (user, link, callback=(err,result)->) -> tryFollowInviteLink user, link, (err, response, body) -> expect(err).to.be.oneOf [null, undefined] expect(response.statusCode).to.equal 302 - expect(response.headers.location).to.match new RegExp("^/register\?.*redir=.*$") + expect(response.headers.location).to.match new RegExp("^/register.*$") # follow redirect to register page and extract the redirectUrl from form user.request.get response.headers.location, (err, response, body) -> - redirectUrl = body.match(/input name="redir" type="hidden" value="([^"]*)"/m)?[1] - loginUrl = body.match(/href="([^"]*)">\s*Login here/m)?[1] - expect(redirectUrl).to.not.be.oneOf [null, undefined] - expect(loginUrl).to.not.be.oneOf [null, undefined] - callback(null, redirectUrl, loginUrl) + # redirectUrl = body.match(/input name="redir" type="hidden" value="([^"]*)"/m)?[1] + # loginUrl = body.match(/href="([^"]*)">\s*Login here/m)?[1] + # expect(redirectUrl).to.not.be.oneOf [null, undefined] + # expect(loginUrl).to.not.be.oneOf [null, undefined] + callback(null) -expectLoginPage = (user, loginLink, callback=(err, result)->) -> - tryFollowLoginLink user, loginLink, (err, response, body) -> +expectLoginPage = (user, callback=(err, result)->) -> + tryFollowLoginLink user, "/login", (err, response, body) -> expect(err).to.be.oneOf [null, undefined] expect(response.statusCode).to.equal 200 expect(body).to.match new RegExp("Login - .*") - redirectUrl = body.match(/input name="redir" type="hidden" value="([^"]*)"/m)?[1] - callback(null, redirectUrl) + callback(null) -expectLoginRedirectToInvite = (user, redir, link, callback=(err, result)->) -> - tryLoginUser user, redir, (err, response, body) -> +expectLoginRedirectToInvite = (user, link, callback=(err, result)->) -> + tryLoginUser user, (err, response, body) -> expect(err).to.be.oneOf [null, undefined] expect(response.statusCode).to.equal 200 - expect(link).to.match new RegExp("^.*#{body.redir}\?.*$") + # expect(link).to.match new RegExp("^.*#{body.redir}\?.*$") callback(null, null) -expectRegistrationRedirectToInvite = (user, email, redir, link, callback=(err, result)->) -> - tryRegisterUser user, email, redir, (err, response, body) -> +expectRegistrationRedirectToInvite = (user, email, link, callback=(err, result)->) -> + tryRegisterUser user, email, (err, response, body) -> expect(err).to.be.oneOf [null, undefined] expect(response.statusCode).to.equal 200 - expect(link).to.match new RegExp("^.*#{body.redir}\?.*$") + # expect(link).to.match new RegExp("^.*#{body.redir}\?.*$") callback(null, null) expectInviteRedirectToProject = (user, link, invite, callback=(err,result)->) -> @@ -424,6 +421,7 @@ describe "ProjectInviteTests", -> (cb) => revokeInvite(@sendingUser, @projectId, @invite._id, cb) ], done + # # # # describe 'registration prompt workflow with valid token', -> it 'should redirect to the register page', (done) -> @@ -433,16 +431,14 @@ describe "ProjectInviteTests", -> it 'should allow user to accept the invite if the user registers a new account', (done) -> Async.series [ - (cb) => - expectInviteRedirectToRegister @user, @link, (err, redirectUrl) => - @_redir = redirectUrl - cb() - (cb) => expectRegistrationRedirectToInvite @user, "some_email@example.com", @_redir, @link, cb + (cb) => expectInviteRedirectToRegister @user, @link, cb + (cb) => expectRegistrationRedirectToInvite @user, "some_email@example.com", @link, cb (cb) => expectInvitePage @user, @link, cb (cb) => expectAcceptInviteAndRedirect @user, @invite, cb (cb) => expectProjectAccess @user, @invite.projectId, cb ], done + # # # # describe 'registration prompt workflow with non-valid token', -> before (done)-> @@ -457,11 +453,8 @@ describe "ProjectInviteTests", -> it 'should display invalid-invite if the user registers a new account', (done) -> badLink = @link.replace(@invite.token, 'not_a_real_token') Async.series [ - (cb) => - expectInviteRedirectToRegister @user, badLink, (err, redirectUrl) => - @_redir = redirectUrl - cb() - (cb) => expectRegistrationRedirectToInvite @user, "some_email@example.com", @_redir, badLink, cb + (cb) => expectInviteRedirectToRegister @user, badLink, cb + (cb) => expectRegistrationRedirectToInvite @user, "some_email@example.com", badLink, cb (cb) => expectInvalidInvitePage @user, badLink, cb (cb) => expectNoProjectAccess @user, @invite.projectId, cb ], done @@ -479,16 +472,9 @@ describe "ProjectInviteTests", -> it 'should allow the user to login to view the invite', (done) -> Async.series [ - (cb) => - expectInviteRedirectToRegister @user, @link, (err, redirectUrl, loginUrl) => - @_redir = redirectUrl - @_loginLink = loginUrl - cb() - (cb) => - expectLoginPage @user, @_loginLink, (err, redirectUrl) => - expect(@_redir).to.equal redirectUrl - cb() - (cb) => expectLoginRedirectToInvite @user, @_redir, @link, cb + (cb) => expectInviteRedirectToRegister @user, @link, cb + (cb) => expectLoginPage @user, cb + (cb) => expectLoginRedirectToInvite @user, @link, cb (cb) => expectInvitePage @user, @link, cb (cb) => expectNoProjectAccess @user, @invite.projectId, cb ], done @@ -515,15 +501,10 @@ describe "ProjectInviteTests", -> badLink = @link.replace(@invite.token, 'not_a_real_token') Async.series [ (cb) => - expectInviteRedirectToRegister @user, badLink, (err, redirectUrl, loginUrl) => - @_redir = redirectUrl - @_loginLink = loginUrl - cb() + expectInviteRedirectToRegister @user, badLink, cb (cb) => - expectLoginPage @user, @_loginLink, (err, redirectUrl) => - expect(@_redir).to.equal redirectUrl - cb() - (cb) => expectLoginRedirectToInvite @user, @_redir, badLink, cb + expectLoginPage @user, cb + (cb) => expectLoginRedirectToInvite @user, badLink, cb (cb) => expectInvalidInvitePage @user, badLink, cb (cb) => expectNoProjectAccess @user, @invite.projectId, cb ], done