From 1881775379d9d1a358d1dc9762524723cc005673 Mon Sep 17 00:00:00 2001 From: foobarable Date: Wed, 27 Nov 2019 10:50:14 +0100 Subject: [PATCH 1/6] Fixing redirection after SAML login Saving referer into session in SAML auth so passport can redirect correctly after SAML login. Signed-off-by: Ralph Krimmel --- lib/web/auth/saml/index.js | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/lib/web/auth/saml/index.js b/lib/web/auth/saml/index.js index 40a6f8b34..1c80c934c 100644 --- a/lib/web/auth/saml/index.js +++ b/lib/web/auth/saml/index.js @@ -7,6 +7,7 @@ const config = require('../../../config') const models = require('../../../models') const logger = require('../../../logger') const { urlencodedParser } = require('../../utils') +const { setReturnToFromReferer } = require('../utils') const fs = require('fs') const intersection = function (array1, array2) { return array1.filter((n) => array2.includes(n)) } @@ -76,11 +77,13 @@ passport.use(new SamlStrategy({ }) })) -samlAuth.get('/auth/saml', +samlAuth.get('/auth/saml',function(req,res,next) { + setReturnToFromReferer(req) passport.authenticate('saml', { successReturnToOrRedirect: config.serverURL + '/', failureRedirect: config.serverURL + '/' - }) + })(req,res,next) + } ) samlAuth.post('/auth/saml/callback', urlencodedParser, From 3e8cf5778f6aaa149a45aec25e2ec6de5d6048e3 Mon Sep 17 00:00:00 2001 From: Ralph Krimmel Date: Wed, 27 Nov 2019 15:17:00 +0100 Subject: [PATCH 2/6] Fixing linting problems Signed-off-by: Ralph Krimmel --- lib/web/auth/saml/index.js | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/lib/web/auth/saml/index.js b/lib/web/auth/saml/index.js index 1c80c934c..dd2748143 100644 --- a/lib/web/auth/saml/index.js +++ b/lib/web/auth/saml/index.js @@ -77,14 +77,13 @@ passport.use(new SamlStrategy({ }) })) -samlAuth.get('/auth/saml',function(req,res,next) { +samlAuth.get('/auth/saml', function (req, res, next) { setReturnToFromReferer(req) passport.authenticate('saml', { successReturnToOrRedirect: config.serverURL + '/', failureRedirect: config.serverURL + '/' - })(req,res,next) - } -) + })(req, res, next) +}) samlAuth.post('/auth/saml/callback', urlencodedParser, passport.authenticate('saml', { From e0a88727423dfdb24e09f4e7e69cae718a7de127 Mon Sep 17 00:00:00 2001 From: Ralph Krimmel Date: Thu, 28 Nov 2019 10:59:59 +0100 Subject: [PATCH 3/6] Moving the storage of referrer information to main authorization check instead of doing it in the authentication source Signed-off-by: Ralph Krimmel --- lib/errors.js | 2 ++ lib/web/auth/saml/index.js | 8 +++----- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/lib/errors.js b/lib/errors.js index 64f938597..56fb70276 100644 --- a/lib/errors.js +++ b/lib/errors.js @@ -6,6 +6,8 @@ module.exports = { if (req.user) { responseError(res, '403', 'Forbidden', 'oh no.') } else { + if (!req.session) req.session = {} + req.session.returnTo = req.originalUrl || config.serverUrl + '/'; req.flash('error', 'You are not allowed to access this page. Maybe try logging in?') res.redirect(config.serverURL + '/') } diff --git a/lib/web/auth/saml/index.js b/lib/web/auth/saml/index.js index dd2748143..40a6f8b34 100644 --- a/lib/web/auth/saml/index.js +++ b/lib/web/auth/saml/index.js @@ -7,7 +7,6 @@ const config = require('../../../config') const models = require('../../../models') const logger = require('../../../logger') const { urlencodedParser } = require('../../utils') -const { setReturnToFromReferer } = require('../utils') const fs = require('fs') const intersection = function (array1, array2) { return array1.filter((n) => array2.includes(n)) } @@ -77,13 +76,12 @@ passport.use(new SamlStrategy({ }) })) -samlAuth.get('/auth/saml', function (req, res, next) { - setReturnToFromReferer(req) +samlAuth.get('/auth/saml', passport.authenticate('saml', { successReturnToOrRedirect: config.serverURL + '/', failureRedirect: config.serverURL + '/' - })(req, res, next) -}) + }) +) samlAuth.post('/auth/saml/callback', urlencodedParser, passport.authenticate('saml', { From 3fb3ca54e9c038ad091d234b19f5bd64003f8321 Mon Sep 17 00:00:00 2001 From: Ralph Krimmel Date: Thu, 28 Nov 2019 12:25:59 +0100 Subject: [PATCH 4/6] Removing returnTo setting from referer in all other authentication sources Signed-off-by: Ralph Krimmel --- lib/web/auth/dropbox/index.js | 3 +-- lib/web/auth/email/index.js | 2 -- lib/web/auth/facebook/index.js | 3 +-- lib/web/auth/github/index.js | 3 +-- lib/web/auth/gitlab/index.js | 3 +-- lib/web/auth/google/index.js | 3 +-- lib/web/auth/ldap/index.js | 2 -- lib/web/auth/mattermost/index.js | 3 +-- lib/web/auth/oauth2/index.js | 3 +-- lib/web/auth/openid/index.js | 2 -- lib/web/auth/twitter/index.js | 3 +-- lib/web/auth/utils.js | 6 ------ 12 files changed, 8 insertions(+), 28 deletions(-) diff --git a/lib/web/auth/dropbox/index.js b/lib/web/auth/dropbox/index.js index 1cfabd296..aef011cb5 100644 --- a/lib/web/auth/dropbox/index.js +++ b/lib/web/auth/dropbox/index.js @@ -4,7 +4,7 @@ const Router = require('express').Router const passport = require('passport') const DropboxStrategy = require('passport-dropbox-oauth2').Strategy const config = require('../../../config') -const { setReturnToFromReferer, passportGeneralCallback } = require('../utils') +const { passportGeneralCallback } = require('../utils') let dropboxAuth = module.exports = Router() @@ -16,7 +16,6 @@ passport.use(new DropboxStrategy({ }, passportGeneralCallback)) dropboxAuth.get('/auth/dropbox', function (req, res, next) { - setReturnToFromReferer(req) passport.authenticate('dropbox-oauth2')(req, res, next) }) diff --git a/lib/web/auth/email/index.js b/lib/web/auth/email/index.js index 06560545d..78ca933b9 100644 --- a/lib/web/auth/email/index.js +++ b/lib/web/auth/email/index.js @@ -7,7 +7,6 @@ const LocalStrategy = require('passport-local').Strategy const config = require('../../../config') const models = require('../../../models') const logger = require('../../../logger') -const { setReturnToFromReferer } = require('../utils') const { urlencodedParser } = require('../../utils') const errors = require('../../../errors') @@ -71,7 +70,6 @@ if (config.allowEmailRegister) { emailAuth.post('/login', urlencodedParser, function (req, res, next) { if (!req.body.email || !req.body.password) return errors.errorBadRequest(res) if (!validator.isEmail(req.body.email)) return errors.errorBadRequest(res) - setReturnToFromReferer(req) passport.authenticate('local', { successReturnToOrRedirect: config.serverURL + '/', failureRedirect: config.serverURL + '/', diff --git a/lib/web/auth/facebook/index.js b/lib/web/auth/facebook/index.js index 418ddeeee..0ba948bb6 100644 --- a/lib/web/auth/facebook/index.js +++ b/lib/web/auth/facebook/index.js @@ -5,7 +5,7 @@ const passport = require('passport') const FacebookStrategy = require('passport-facebook').Strategy const config = require('../../../config') -const { setReturnToFromReferer, passportGeneralCallback } = require('../utils') +const { passportGeneralCallback } = require('../utils') let facebookAuth = module.exports = Router() @@ -16,7 +16,6 @@ passport.use(new FacebookStrategy({ }, passportGeneralCallback)) facebookAuth.get('/auth/facebook', function (req, res, next) { - setReturnToFromReferer(req) passport.authenticate('facebook')(req, res, next) }) diff --git a/lib/web/auth/github/index.js b/lib/web/auth/github/index.js index afa5fa314..3a3a84c6e 100644 --- a/lib/web/auth/github/index.js +++ b/lib/web/auth/github/index.js @@ -5,7 +5,7 @@ const passport = require('passport') const GithubStrategy = require('passport-github').Strategy const config = require('../../../config') const response = require('../../../response') -const { setReturnToFromReferer, passportGeneralCallback } = require('../utils') +const { passportGeneralCallback } = require('../utils') let githubAuth = module.exports = Router() @@ -16,7 +16,6 @@ passport.use(new GithubStrategy({ }, passportGeneralCallback)) githubAuth.get('/auth/github', function (req, res, next) { - setReturnToFromReferer(req) passport.authenticate('github')(req, res, next) }) diff --git a/lib/web/auth/gitlab/index.js b/lib/web/auth/gitlab/index.js index 4cebbc10c..1b628e815 100644 --- a/lib/web/auth/gitlab/index.js +++ b/lib/web/auth/gitlab/index.js @@ -5,7 +5,7 @@ const passport = require('passport') const GitlabStrategy = require('passport-gitlab2').Strategy const config = require('../../../config') const response = require('../../../response') -const { setReturnToFromReferer, passportGeneralCallback } = require('../utils') +const { passportGeneralCallback } = require('../utils') let gitlabAuth = module.exports = Router() @@ -18,7 +18,6 @@ passport.use(new GitlabStrategy({ }, passportGeneralCallback)) gitlabAuth.get('/auth/gitlab', function (req, res, next) { - setReturnToFromReferer(req) passport.authenticate('gitlab')(req, res, next) }) diff --git a/lib/web/auth/google/index.js b/lib/web/auth/google/index.js index ad9bcd7ab..feb830257 100644 --- a/lib/web/auth/google/index.js +++ b/lib/web/auth/google/index.js @@ -4,7 +4,7 @@ const Router = require('express').Router const passport = require('passport') var GoogleStrategy = require('passport-google-oauth20').Strategy const config = require('../../../config') -const { setReturnToFromReferer, passportGeneralCallback } = require('../utils') +const { passportGeneralCallback } = require('../utils') let googleAuth = module.exports = Router() @@ -16,7 +16,6 @@ passport.use(new GoogleStrategy({ }, passportGeneralCallback)) googleAuth.get('/auth/google', function (req, res, next) { - setReturnToFromReferer(req) passport.authenticate('google', { scope: ['profile'] })(req, res, next) }) // google auth callback diff --git a/lib/web/auth/ldap/index.js b/lib/web/auth/ldap/index.js index 28f3e471b..b501106dc 100644 --- a/lib/web/auth/ldap/index.js +++ b/lib/web/auth/ldap/index.js @@ -6,7 +6,6 @@ const LDAPStrategy = require('passport-ldapauth') const config = require('../../../config') const models = require('../../../models') const logger = require('../../../logger') -const { setReturnToFromReferer } = require('../utils') const { urlencodedParser } = require('../../utils') const errors = require('../../../errors') @@ -82,7 +81,6 @@ passport.use(new LDAPStrategy({ ldapAuth.post('/auth/ldap', urlencodedParser, function (req, res, next) { if (!req.body.username || !req.body.password) return errors.errorBadRequest(res) - setReturnToFromReferer(req) passport.authenticate('ldapauth', { successReturnToOrRedirect: config.serverURL + '/', failureRedirect: config.serverURL + '/', diff --git a/lib/web/auth/mattermost/index.js b/lib/web/auth/mattermost/index.js index 48d6d297b..78eca2af4 100644 --- a/lib/web/auth/mattermost/index.js +++ b/lib/web/auth/mattermost/index.js @@ -5,7 +5,7 @@ const passport = require('passport') const Mattermost = require('mattermost') const OAuthStrategy = require('passport-oauth2').Strategy const config = require('../../../config') -const { setReturnToFromReferer, passportGeneralCallback } = require('../utils') +const { passportGeneralCallback } = require('../utils') const mattermost = new Mattermost.Client() @@ -36,7 +36,6 @@ mattermostStrategy.userProfile = (accessToken, done) => { passport.use(mattermostStrategy) mattermostAuth.get('/auth/mattermost', function (req, res, next) { - setReturnToFromReferer(req) passport.authenticate('oauth2')(req, res, next) }) diff --git a/lib/web/auth/oauth2/index.js b/lib/web/auth/oauth2/index.js index 784342711..2bd731961 100644 --- a/lib/web/auth/oauth2/index.js +++ b/lib/web/auth/oauth2/index.js @@ -4,7 +4,7 @@ const Router = require('express').Router const passport = require('passport') const { Strategy, InternalOAuthError } = require('passport-oauth2') const config = require('../../../config') -const { setReturnToFromReferer, passportGeneralCallback } = require('../utils') +const { passportGeneralCallback } = require('../utils') let oauth2Auth = module.exports = Router() @@ -93,7 +93,6 @@ passport.use(new OAuth2CustomStrategy({ }, passportGeneralCallback)) oauth2Auth.get('/auth/oauth2', function (req, res, next) { - setReturnToFromReferer(req) passport.authenticate('oauth2')(req, res, next) }) diff --git a/lib/web/auth/openid/index.js b/lib/web/auth/openid/index.js index b0a28bec9..28e164f55 100644 --- a/lib/web/auth/openid/index.js +++ b/lib/web/auth/openid/index.js @@ -7,7 +7,6 @@ const config = require('../../../config') const models = require('../../../models') const logger = require('../../../logger') const { urlencodedParser } = require('../../utils') -const { setReturnToFromReferer } = require('../utils') let openIDAuth = module.exports = Router() @@ -48,7 +47,6 @@ passport.use(new OpenIDStrategy({ })) openIDAuth.post('/auth/openid', urlencodedParser, function (req, res, next) { - setReturnToFromReferer(req) passport.authenticate('openid')(req, res, next) }) diff --git a/lib/web/auth/twitter/index.js b/lib/web/auth/twitter/index.js index 5aba20ff3..56389f842 100644 --- a/lib/web/auth/twitter/index.js +++ b/lib/web/auth/twitter/index.js @@ -5,7 +5,7 @@ const passport = require('passport') const TwitterStrategy = require('passport-twitter').Strategy const config = require('../../../config') -const { setReturnToFromReferer, passportGeneralCallback } = require('../utils') +const { passportGeneralCallback } = require('../utils') let twitterAuth = module.exports = Router() @@ -16,7 +16,6 @@ passport.use(new TwitterStrategy({ }, passportGeneralCallback)) twitterAuth.get('/auth/twitter', function (req, res, next) { - setReturnToFromReferer(req) passport.authenticate('twitter')(req, res, next) }) diff --git a/lib/web/auth/utils.js b/lib/web/auth/utils.js index 141a0d6f1..fb69f08cf 100644 --- a/lib/web/auth/utils.js +++ b/lib/web/auth/utils.js @@ -3,12 +3,6 @@ const models = require('../../models') const logger = require('../../logger') -exports.setReturnToFromReferer = function setReturnToFromReferer (req) { - var referer = req.get('referer') - if (!req.session) req.session = {} - req.session.returnTo = referer -} - exports.passportGeneralCallback = function callback (accessToken, refreshToken, profile, done) { var stringifiedProfile = JSON.stringify(profile) models.User.findOrCreate({ From 32113e874d090bdb315b912e2a926dbbd31ccd38 Mon Sep 17 00:00:00 2001 From: Ralph Krimmel Date: Thu, 28 Nov 2019 12:26:50 +0100 Subject: [PATCH 5/6] Making the linter happy by removing superfluous ; --- lib/errors.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/errors.js b/lib/errors.js index 56fb70276..f86e8aa37 100644 --- a/lib/errors.js +++ b/lib/errors.js @@ -7,7 +7,7 @@ module.exports = { responseError(res, '403', 'Forbidden', 'oh no.') } else { if (!req.session) req.session = {} - req.session.returnTo = req.originalUrl || config.serverUrl + '/'; + req.session.returnTo = req.originalUrl || config.serverUrl + '/' req.flash('error', 'You are not allowed to access this page. Maybe try logging in?') res.redirect(config.serverURL + '/') } From 9534cdafbfa42b3d80901c8e06807c62513784fd Mon Sep 17 00:00:00 2001 From: Ralph Krimmel Date: Thu, 28 Nov 2019 12:26:50 +0100 Subject: [PATCH 6/6] Making the linter happy by removing superfluous ; Signed-off-by: Ralph Krimmel --- lib/errors.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/errors.js b/lib/errors.js index 56fb70276..f86e8aa37 100644 --- a/lib/errors.js +++ b/lib/errors.js @@ -7,7 +7,7 @@ module.exports = { responseError(res, '403', 'Forbidden', 'oh no.') } else { if (!req.session) req.session = {} - req.session.returnTo = req.originalUrl || config.serverUrl + '/'; + req.session.returnTo = req.originalUrl || config.serverUrl + '/' req.flash('error', 'You are not allowed to access this page. Maybe try logging in?') res.redirect(config.serverURL + '/') }