use session for the post-login redirect, remove redir query string.

This commit is contained in:
Shane Kilkelly 2016-11-22 14:24:36 +00:00
parent da1be67aff
commit 8089bb55a4
6 changed files with 92 additions and 97 deletions

View file

@ -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

View file

@ -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)->

View file

@ -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(

View file

@ -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

View file

@ -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 ->

View file

@ -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("<title>Login - .*</title>")
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