From 8089bb55a48dbf7ad35085f0d0b2b8b01bc13b06 Mon Sep 17 00:00:00 2001 From: Shane Kilkelly Date: Tue, 22 Nov 2016 14:24:36 +0000 Subject: [PATCH 1/6] 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 From 8a4352fff2e1e47478756c6498b2aaca140ad205 Mon Sep 17 00:00:00 2001 From: Shane Kilkelly Date: Tue, 22 Nov 2016 16:54:03 +0000 Subject: [PATCH 2/6] Set redirect when redirecting from restricted --- .../Authentication/AuthenticationController.coffee | 7 ++++--- .../Features/Authorization/AuthorizationMiddlewear.coffee | 2 +- .../Authentication/AuthenticationControllerTests.coffee | 4 ++++ 3 files changed, 9 insertions(+), 4 deletions(-) diff --git a/services/web/app/coffee/Features/Authentication/AuthenticationController.coffee b/services/web/app/coffee/Features/Authentication/AuthenticationController.coffee index 765065823b..7a913604d9 100644 --- a/services/web/app/coffee/Features/Authentication/AuthenticationController.coffee +++ b/services/web/app/coffee/Features/Authentication/AuthenticationController.coffee @@ -189,10 +189,11 @@ module.exports = AuthenticationController = Metrics.inc "user.login.failed" callback() - _setRedirectInSession: (req) -> - target = if Object.keys(req.query) then "#{req.path}?#{querystring.stringify(req.query)}" else req.path + _setRedirectInSession: (req, value) -> + if !value? + value = if Object.keys(req.query) > 0 then "#{req.path}?#{querystring.stringify(req.query)}" else req.path if req.session? - req.session.postLoginRedirect = target + req.session.postLoginRedirect = value _getRedirectFromSession: (req) -> return req?.session?.postLoginRedirect || null diff --git a/services/web/app/coffee/Features/Authorization/AuthorizationMiddlewear.coffee b/services/web/app/coffee/Features/Authorization/AuthorizationMiddlewear.coffee index cb0f583674..be0a85107c 100644 --- a/services/web/app/coffee/Features/Authorization/AuthorizationMiddlewear.coffee +++ b/services/web/app/coffee/Features/Authorization/AuthorizationMiddlewear.coffee @@ -108,5 +108,5 @@ module.exports = AuthorizationMiddlewear = logger.log {from: from}, "redirecting to login" redirect_to = "/login" if from? - redirect_to += "?redir=#{encodeURIComponent(from)}" + AuthenticationController._setRedirectInSession(req, from) res.redirect redirect_to diff --git a/services/web/test/UnitTests/coffee/Authentication/AuthenticationControllerTests.coffee b/services/web/test/UnitTests/coffee/Authentication/AuthenticationControllerTests.coffee index 7dd2617e66..378c3bafd2 100644 --- a/services/web/test/UnitTests/coffee/Authentication/AuthenticationControllerTests.coffee +++ b/services/web/test/UnitTests/coffee/Authentication/AuthenticationControllerTests.coffee @@ -541,6 +541,10 @@ describe "AuthenticationController", -> @AuthenticationController._setRedirectInSession(@req) expect(@req.session.postLoginRedirect).to.equal "/somewhere?one=1" + it 'should set the supplied value', -> + @AuthenticationController._setRedirectInSession(@req, '/somewhere/specific') + expect(@req.session.postLoginRedirect).to.equal "/somewhere/specific" + describe '_getRedirectFromSession', -> beforeEach -> @req = {session: {postLoginRedirect: "/a?b=c"}} From cee3326ce3cda373114756200c3c1bbf785508ec Mon Sep 17 00:00:00 2001 From: Shane Kilkelly Date: Tue, 22 Nov 2016 17:06:05 +0000 Subject: [PATCH 3/6] fix omission of 'length' --- .../Features/Authentication/AuthenticationController.coffee | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/web/app/coffee/Features/Authentication/AuthenticationController.coffee b/services/web/app/coffee/Features/Authentication/AuthenticationController.coffee index 7a913604d9..59f55127f1 100644 --- a/services/web/app/coffee/Features/Authentication/AuthenticationController.coffee +++ b/services/web/app/coffee/Features/Authentication/AuthenticationController.coffee @@ -191,7 +191,7 @@ module.exports = AuthenticationController = _setRedirectInSession: (req, value) -> if !value? - value = if Object.keys(req.query) > 0 then "#{req.path}?#{querystring.stringify(req.query)}" else req.path + value = if Object.keys(req.query).length > 0 then "#{req.path}?#{querystring.stringify(req.query)}" else req.path if req.session? req.session.postLoginRedirect = value From 22101d030571f5886c0b1029d65b18966807c804 Mon Sep 17 00:00:00 2001 From: Shane Kilkelly Date: Thu, 24 Nov 2016 11:38:13 +0000 Subject: [PATCH 4/6] If user is sent to login page with explicit redirect, obey --- .../AuthenticationController.coffee | 4 +++- .../Subscription/SubscriptionController.coffee | 5 +---- .../Features/User/UserPagesController.coffee | 5 +++++ services/web/app/views/user/register.jade | 2 +- .../AuthenticationControllerTests.coffee | 6 ++---- .../coffee/User/UserPagesControllerTests.coffee | 16 ++++++++++++++++ .../acceptance/coffee/ProjectInviteTests.coffee | 8 -------- 7 files changed, 28 insertions(+), 18 deletions(-) diff --git a/services/web/app/coffee/Features/Authentication/AuthenticationController.coffee b/services/web/app/coffee/Features/Authentication/AuthenticationController.coffee index 59f55127f1..5a92cc0f75 100644 --- a/services/web/app/coffee/Features/Authentication/AuthenticationController.coffee +++ b/services/web/app/coffee/Features/Authentication/AuthenticationController.coffee @@ -157,7 +157,9 @@ module.exports = AuthenticationController = return isValid _redirectToLoginOrRegisterPage: (req, res, next)-> - if req.query.zipUrl? or req.query.project_name? + if (req.query.zipUrl? or + req.query.project_name? or + req.path == '/user/subscription/new') return AuthenticationController._redirectToRegisterPage(req, res, next) else AuthenticationController._redirectToLoginPage(req, res, next) diff --git a/services/web/app/coffee/Features/Subscription/SubscriptionController.coffee b/services/web/app/coffee/Features/Subscription/SubscriptionController.coffee index 51b3db8f56..d864cac1d1 100644 --- a/services/web/app/coffee/Features/Subscription/SubscriptionController.coffee +++ b/services/web/app/coffee/Features/Subscription/SubscriptionController.coffee @@ -14,10 +14,7 @@ module.exports = SubscriptionController = plansPage: (req, res, next) -> plans = SubscriptionViewModelBuilder.buildViewModel() - if AuthenticationController.isUserLoggedIn(req) - baseUrl = "" - else - baseUrl = "/register?redir=" + baseUrl = "" viewName = "subscriptions/plans" if req.query.v? viewName = "#{viewName}_#{req.query.v}" diff --git a/services/web/app/coffee/Features/User/UserPagesController.coffee b/services/web/app/coffee/Features/User/UserPagesController.coffee index 00494d71e5..54cfd54892 100644 --- a/services/web/app/coffee/Features/User/UserPagesController.coffee +++ b/services/web/app/coffee/Features/User/UserPagesController.coffee @@ -48,6 +48,11 @@ module.exports = token: req.query.token loginPage : (req, res)-> + # if user is being sent to /login with explicit redirect (redir=/foo), + # such as being sent from the editor to /login, then set the redirect explicitly + if req.query.redir? and !AuthenticationController._getRedirectFromSession(req)? + logger.log {redir: req.query.redir}, "setting explicit redirect from login page" + AuthenticationController._setRedirectInSession(req, req.query.redir) res.render 'user/login', title: 'login', email: req.query.email diff --git a/services/web/app/views/user/register.jade b/services/web/app/views/user/register.jade index 5a1196b6a6..2a5ab707a6 100644 --- a/services/web/app/views/user/register.jade +++ b/services/web/app/views/user/register.jade @@ -12,7 +12,7 @@ block content | #{translate("join_sl_to_view_project")}. div | #{translate("if_you_are_registered")}, - a(href="/login?redir=#{getReqQueryParam('redir')}") #{translate("login_here")} + a(href="/login") #{translate("login_here")} else if newTemplateData.templateName !== undefined h1 #{translate("register_to_edit_template", {templateName:newTemplateData.templateName})} diff --git a/services/web/test/UnitTests/coffee/Authentication/AuthenticationControllerTests.coffee b/services/web/test/UnitTests/coffee/Authentication/AuthenticationControllerTests.coffee index 378c3bafd2..72265eac11 100644 --- a/services/web/test/UnitTests/coffee/Authentication/AuthenticationControllerTests.coffee +++ b/services/web/test/UnitTests/coffee/Authentication/AuthenticationControllerTests.coffee @@ -155,10 +155,11 @@ describe "AuthenticationController", -> @AuthenticationController.passportLogin @req, @res, @next @req.login.callCount.should.equal 0 - it 'should send a json response with redirect', () -> + it 'should not send a json response with redirect', () -> @AuthenticationController.passportLogin @req, @res, @next @res.json.callCount.should.equal 1 @res.json.calledWith({message: @info}).should.equal true + expect(@res.json.lastCall.args[0].redir?).to.equal false describe 'afterLoginSessionSetup', -> @@ -269,9 +270,6 @@ describe "AuthenticationController", -> it "should set res.session.justLoggedIn", -> @req.session.justLoggedIn.should.equal true - it "should redirect the user to the specified location", -> - expect(@req._redir).to.deep.equal @redir - it "should record the successful login", -> @AuthenticationController._recordSuccessfulLogin .calledWith(@user._id) diff --git a/services/web/test/UnitTests/coffee/User/UserPagesControllerTests.coffee b/services/web/test/UnitTests/coffee/User/UserPagesControllerTests.coffee index 24ca3eeeda..f0fa84176f 100644 --- a/services/web/test/UnitTests/coffee/User/UserPagesControllerTests.coffee +++ b/services/web/test/UnitTests/coffee/User/UserPagesControllerTests.coffee @@ -30,6 +30,8 @@ describe "UserPagesController", -> @AuthenticationController = getLoggedInUserId: sinon.stub().returns(@user._id) getSessionUser: sinon.stub().returns(@user) + _getRedirectFromSession: sinon.stub() + _setRedirectInSession: sinon.stub() @UserPagesController = SandboxedModule.require modulePath, requires: "settings-sharelatex":@settings "logger-sharelatex": @@ -90,6 +92,20 @@ describe "UserPagesController", -> done() @UserPagesController.loginPage @req, @res + describe 'when an explicit redirect is set via query string', -> + + beforeEach -> + @AuthenticationController._getRedirectFromSession = sinon.stub().returns(null) + @AuthenticationController._setRedirectInSession = sinon.stub() + @req.query.redir = '/somewhere/in/particular' + + it 'should set a redirect', (done) -> + @res.render = (page) => + @AuthenticationController._setRedirectInSession.callCount.should.equal 1 + expect(@AuthenticationController._setRedirectInSession.lastCall.args[1]).to.equal @req.query.redir + 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 3068892a68..b599578738 100644 --- a/services/web/test/acceptance/coffee/ProjectInviteTests.coffee +++ b/services/web/test/acceptance/coffee/ProjectInviteTests.coffee @@ -144,10 +144,6 @@ expectInviteRedirectToRegister = (user, link, callback=(err,result)->) -> 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) expectLoginPage = (user, callback=(err, result)->) -> @@ -161,14 +157,12 @@ 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}\?.*$") callback(null, null) 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}\?.*$") callback(null, null) expectInviteRedirectToProject = (user, link, invite, callback=(err,result)->) -> @@ -421,7 +415,6 @@ 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) -> @@ -438,7 +431,6 @@ describe "ProjectInviteTests", -> (cb) => expectProjectAccess @user, @invite.projectId, cb ], done - # # # # describe 'registration prompt workflow with non-valid token', -> before (done)-> From 167f01857a87583b16c0e7b9f649c0ec61a7817e Mon Sep 17 00:00:00 2001 From: Shane Kilkelly Date: Thu, 24 Nov 2016 14:15:01 +0000 Subject: [PATCH 5/6] Remove stray `next` params. --- .../Authentication/AuthenticationController.coffee | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/services/web/app/coffee/Features/Authentication/AuthenticationController.coffee b/services/web/app/coffee/Features/Authentication/AuthenticationController.coffee index 5a92cc0f75..df92fa1d7d 100644 --- a/services/web/app/coffee/Features/Authentication/AuthenticationController.coffee +++ b/services/web/app/coffee/Features/Authentication/AuthenticationController.coffee @@ -127,7 +127,7 @@ module.exports = AuthenticationController = requireLogin: () -> doRequest = (req, res, next = (error) ->) -> if !AuthenticationController.isUserLoggedIn(req) - AuthenticationController._redirectToLoginOrRegisterPage(req, res, next) + AuthenticationController._redirectToLoginOrRegisterPage(req, res) 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, next)-> + _redirectToLoginOrRegisterPage: (req, res)-> if (req.query.zipUrl? or req.query.project_name? or req.path == '/user/subscription/new') - return AuthenticationController._redirectToRegisterPage(req, res, next) + return AuthenticationController._redirectToRegisterPage(req, res) else - AuthenticationController._redirectToLoginPage(req, res, next) + AuthenticationController._redirectToLoginPage(req, res) - _redirectToLoginPage: (req, res, next) -> + _redirectToLoginPage: (req, res) -> logger.log url: req.url, "user not logged in so redirecting to login page" AuthenticationController._setRedirectInSession(req) url = "/login?#{querystring.stringify(req.query)}" res.redirect url Metrics.inc "security.login-redirect" - _redirectToRegisterPage: (req, res, next) -> + _redirectToRegisterPage: (req, res) -> logger.log url: req.url, "user not logged in so redirecting to register page" AuthenticationController._setRedirectInSession(req) url = "/register?#{querystring.stringify(req.query)}" From acce8853bac5023d02741c1dd3b88d78cdba2bb7 Mon Sep 17 00:00:00 2001 From: Shane Kilkelly Date: Fri, 25 Nov 2016 15:24:50 +0000 Subject: [PATCH 6/6] Remove redundant `baseUrl` from plans page. --- .../Features/Subscription/SubscriptionController.coffee | 2 -- services/web/app/views/subscriptions/plans.jade | 8 ++++---- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/services/web/app/coffee/Features/Subscription/SubscriptionController.coffee b/services/web/app/coffee/Features/Subscription/SubscriptionController.coffee index d864cac1d1..84de417bb6 100644 --- a/services/web/app/coffee/Features/Subscription/SubscriptionController.coffee +++ b/services/web/app/coffee/Features/Subscription/SubscriptionController.coffee @@ -14,7 +14,6 @@ module.exports = SubscriptionController = plansPage: (req, res, next) -> plans = SubscriptionViewModelBuilder.buildViewModel() - baseUrl = "" viewName = "subscriptions/plans" if req.query.v? viewName = "#{viewName}_#{req.query.v}" @@ -26,7 +25,6 @@ module.exports = SubscriptionController = res.render viewName, title: "plans_and_pricing" plans: plans - baseUrl: baseUrl gaExperiments: Settings.gaExperiments.plansPage recomendedCurrency:recomendedCurrency shouldABTestPlans: currentUser == null or (currentUser?.signUpDate? and currentUser.signUpDate >= (new Date('2016-10-27'))) diff --git a/services/web/app/views/subscriptions/plans.jade b/services/web/app/views/subscriptions/plans.jade index 16376c7e72..88584dba76 100644 --- a/services/web/app/views/subscriptions/plans.jade +++ b/services/web/app/views/subscriptions/plans.jade @@ -100,7 +100,7 @@ block content li br a.btn.btn-info( - ng-href="#{baseUrl}/user/subscription/new?planCode={{ getCollaboratorPlanCode() }}¤cy={{currencyCode}}", ng-click="signUpNowClicked('collaborator')" + ng-href="/user/subscription/new?planCode={{ getCollaboratorPlanCode() }}¤cy={{currencyCode}}", ng-click="signUpNowClicked('collaborator')" ) span(ng-show="ui.view != 'annual'") #{translate("start_free_trial")} span(ng-show="ui.view == 'annual'") #{translate("buy_now")} @@ -124,7 +124,7 @@ block content li br a.btn.btn-info( - ng-href="#{baseUrl}/user/subscription/new?planCode=professional{{ ui.view == 'annual' && '-annual' || planQueryString}}¤cy={{currencyCode}}", ng-click="signUpNowClicked('professional')" + ng-href="/user/subscription/new?planCode=professional{{ ui.view == 'annual' && '-annual' || planQueryString}}¤cy={{currencyCode}}", ng-click="signUpNowClicked('professional')" ) span(ng-show="ui.view != 'annual'") #{translate("start_free_trial")} span(ng-show="ui.view == 'annual'") #{translate("buy_now")} @@ -166,7 +166,7 @@ block content li br a.btn.btn-info( - ng-href="#{baseUrl}/user/subscription/new?planCode=student{{ plansVariant == 'default' ? planQueryString : '_'+plansVariant }}¤cy={{currencyCode}}", + ng-href="/user/subscription/new?planCode=student{{ plansVariant == 'default' ? planQueryString : '_'+plansVariant }}¤cy={{currencyCode}}", ng-click="signUpNowClicked('student')" ) #{translate("start_free_trial")} @@ -189,7 +189,7 @@ block content li br a.btn.btn-info( - ng-href="#{baseUrl}/user/subscription/new?planCode=student-annual{{ plansVariant == 'default' ? '' : '_'+plansVariant }}¤cy={{currencyCode}}", + ng-href="/user/subscription/new?planCode=student-annual{{ plansVariant == 'default' ? '' : '_'+plansVariant }}¤cy={{currencyCode}}", ng-click="signUpNowClicked('student')" ) #{translate("buy_now")}