From 23154f0739cc8266d50670e21420329eb5de7aee Mon Sep 17 00:00:00 2001 From: Fabien O'Carroll Date: Thu, 2 Apr 2020 16:27:31 +0200 Subject: [PATCH] Refactored session service (#11701) * Refactored SessionStore to use @tryghost/errors no-issue * Updated tests to test exposed API no-issue This will make refactoring easier, as we only have the "public" contract to maintain * Refactored session functionality to SessionService no-issue This splits the session logic away from the HTTP responding logic, which will allows us to decouple session creation/modification from the API. Eventually this can be used to create sessions based on magiclink style tokens. * Instantiated and exported the new SessionService no-issue * Refactored session middleware to take session service no-issue This removes duplication of code and makes the middleware more explicit that it's just a wrapper around the session service. * Updated to use external @tryghost/session-service no-issue --- core/server/services/auth/session/index.js | 117 ++++++++++++- .../services/auth/session/middleware.js | 163 ++++-------------- core/server/services/auth/session/store.js | 6 +- package.json | 1 + test/unit/api/canary/session_spec.js | 2 +- test/unit/api/v2/session_spec.js | 2 +- test/unit/api/v3/session_spec.js | 2 +- .../services/auth/session/middleware_spec.js | 53 ++---- yarn.lock | 7 + 9 files changed, 169 insertions(+), 184 deletions(-) diff --git a/core/server/services/auth/session/index.js b/core/server/services/auth/session/index.js index 9f2b16d2a0..31e466cc8c 100644 --- a/core/server/services/auth/session/index.js +++ b/core/server/services/auth/session/index.js @@ -1,14 +1,123 @@ +const session = require('express-session'); +const constants = require('../../../lib/constants'); +const config = require('../../../config'); +const settingsCache = require('../../settings/cache'); +const models = require('../../../models'); +const urlUtils = require('../../../lib/url-utils'); +const url = require('url'); + +const SessionService = require('@tryghost/session-service'); +const SessionMiddleware = require('./middleware'); +const SessionStore = require('./store'); + +function getOriginOfRequest(req) { + const origin = req.get('origin'); + const referrer = req.get('referrer'); + + if (!origin && !referrer) { + return null; + } + + if (origin) { + return origin; + } + + const {protocol, host} = url.parse(referrer); + if (protocol && host) { + return `${protocol}//${host}`; + } + return null; +} + +async function getSession(req, res) { + if (req.session) { + return req.session; + } + return new Promise((resolve, reject) => { + expressSessionMiddleware(req, res, function (err) { + if (err) { + return reject(err); + } + resolve(req.session); + }); + }); +} + +function findUserById({id}) { + return models.User.findOne({id}); +} + +let expressSessionMiddleware; +function initExpressSessionMiddleware() { + if (!expressSessionMiddleware) { + expressSessionMiddleware = session({ + store: new SessionStore(models.Session), + secret: settingsCache.get('session_secret'), + resave: false, + saveUninitialized: false, + name: 'ghost-admin-api-session', + cookie: { + maxAge: constants.SIX_MONTH_MS, + httpOnly: true, + path: urlUtils.getSubdir() + '/ghost', + sameSite: 'lax', + secure: urlUtils.isSSL(config.get('url')) + } + }); + } +} + +let sessionService; +function initSessionService() { + if (!sessionService) { + if (!expressSessionMiddleware) { + initExpressSessionMiddleware(); + } + + sessionService = SessionService({ + getOriginOfRequest, + getSession, + findUserById + }); + } +} + +let sessionMiddleware; +function initSessionMiddleware() { + if (!sessionMiddleware) { + if (!sessionService) { + initSessionService(); + } + sessionMiddleware = SessionMiddleware({ + sessionService + }); + } +} + module.exports = { - // @TODO: expose files/units and not functions of units get createSession() { - return require('./middleware').createSession; + return this.middleware.createSession; }, get destroySession() { - return require('./middleware').destroySession; + return this.middleware.destroySession; }, get authenticate() { - return require('./middleware').authenticate; + return this.middleware.authenticate; + }, + + get service() { + if (!sessionService) { + initSessionService(); + } + return sessionService; + }, + + get middleware() { + if (!sessionMiddleware) { + initSessionMiddleware(); + } + return sessionMiddleware; } }; diff --git a/core/server/services/auth/session/middleware.js b/core/server/services/auth/session/middleware.js index 2ff99bb9a0..45de4ab93f 100644 --- a/core/server/services/auth/session/middleware.js +++ b/core/server/services/auth/session/middleware.js @@ -1,140 +1,37 @@ -const url = require('url'); -const session = require('express-session'); -const common = require('../../../lib/common'); -const constants = require('../../../lib/constants'); -const config = require('../../../config'); -const settingsCache = require('../../settings/cache'); -const models = require('../../../models'); -const SessionStore = require('./store'); -const urlUtils = require('../../../lib/url-utils'); - -const getOrigin = (req) => { - const origin = req.get('origin'); - const referrer = req.get('referrer'); - - if (!origin && !referrer) { - return null; - } - - if (origin) { - return origin; - } - - const {protocol, host} = url.parse(referrer); - if (protocol && host) { - return `${protocol}//${host}`; - } - return null; -}; - -let UNO_SESSIONIONA; -const getSession = (req, res, next) => { - if (!UNO_SESSIONIONA) { - UNO_SESSIONIONA = session({ - store: new SessionStore(models.Session), - secret: settingsCache.get('session_secret'), - resave: false, - saveUninitialized: false, - name: 'ghost-admin-api-session', - cookie: { - maxAge: constants.SIX_MONTH_MS, - httpOnly: true, - path: urlUtils.getSubdir() + '/ghost', - sameSite: 'lax', - secure: urlUtils.isSSL(config.get('url')) - } - }); - } - return UNO_SESSIONIONA(req, res, next); -}; - -const createSession = (req, res, next) => { - getSession(req, res, function (err) { - if (err) { - return next(err); - } - const origin = getOrigin(req); - if (!origin) { - return next(new common.errors.BadRequestError({ - message: common.i18n.t('errors.middleware.auth.unknownOrigin') - })); - } - req.session.user_id = req.user.id; - req.session.origin = origin; - req.session.user_agent = req.get('user-agent'); - req.session.ip = req.ip; - res.sendStatus(201); - }); -}; - -const destroySession = (req, res, next) => { - req.session.destroy((err) => { - if (err) { - return next(new common.errors.InternalServerError({err})); - } - return res.sendStatus(204); - }); -}; - -const cookieCsrfProtection = (req) => { - // If there is no origin on the session object it means this is a *new* - // session, that hasn't been initialised yet. So we don't need CSRF protection - if (!req.session.origin) { - return; - } - - const origin = getOrigin(req); - - if (req.session.origin !== origin) { - throw new common.errors.BadRequestError({ - message: common.i18n.t('errors.middleware.auth.mismatchedOrigin', { - expected: req.session.origin, - actual: origin - }) - }); - } -}; - -const authenticate = (req, res, next) => { - // CASE: we don't have a cookie header so allow fallthrough to other - // auth middleware or final "ensure authenticated" check - if (!req.headers || !req.headers.cookie) { - req.user = null; - return next(); - } - - getSession(req, res, function (err) { - if (err) { - return next(err); - } - +function SessionMiddleware({sessionService}) { + async function createSession(req, res, next) { try { - cookieCsrfProtection(req); + await sessionService.createSessionForUser(req, res, req.user); + res.sendStatus(201); } catch (err) { - return next(err); + next(err); } + } - if (!req.session || !req.session.user_id) { - req.user = null; - return next(); + async function destroySession(req, res, next) { + try { + await sessionService.destroyCurrentSession(req); + res.sendStatus(204); + } catch (err) { + next(err); } + } - models.User.findOne({id: req.session.user_id}) - .then((user) => { - req.user = user; - next(); - }) - .catch(() => { - req.user = null; - next(); - }); - }); -}; + async function authenticate(req, res, next) { + try { + const user = await sessionService.getUserForSession(req, res); + req.user = user; + next(); + } catch (err) { + next(err); + } + } -// @TODO: this interface exposes private functions -module.exports = exports = { - createSession, - destroySession, - cookieCsrfProtection, - authenticate -}; + return { + createSession: createSession, + destroySession: destroySession, + authenticate: authenticate + }; +} + +module.exports = SessionMiddleware; diff --git a/core/server/services/auth/session/store.js b/core/server/services/auth/session/store.js index 22eba1b3d8..cd9f5b12b8 100644 --- a/core/server/services/auth/session/store.js +++ b/core/server/services/auth/session/store.js @@ -1,5 +1,5 @@ const {Store} = require('express-session'); -const common = require('../../../lib/common'); +const {InternalServerError} = require('@tryghost/errors'); module.exports = class SessionStore extends Store { constructor(SessionModel) { @@ -30,8 +30,8 @@ module.exports = class SessionStore extends Store { set(sid, sessionData, callback) { if (!sessionData.user_id) { - return callback(new common.errors.InternalServerError({ - message: common.i18n.t('errors.middleware.auth.missingUserID') + return callback(new InternalServerError({ + message: 'Cannot create a session with no user_id' })); } this.SessionModel diff --git a/package.json b/package.json index 743021cf09..dbefff3b2f 100644 --- a/package.json +++ b/package.json @@ -49,6 +49,7 @@ "@tryghost/kg-markdown-html-renderer": "1.0.2", "@tryghost/members-api": "0.18.0", "@tryghost/members-ssr": "0.7.4", + "@tryghost/session-service": "^0.1.0", "@tryghost/social-urls": "0.1.7", "@tryghost/string": "0.1.7", "@tryghost/url-utils": "0.6.16", diff --git a/test/unit/api/canary/session_spec.js b/test/unit/api/canary/session_spec.js index cb63444b5a..a48b4b6ad6 100644 --- a/test/unit/api/canary/session_spec.js +++ b/test/unit/api/canary/session_spec.js @@ -5,7 +5,7 @@ const models = require('../../../../core/server/models'); const {UnauthorizedError} = require('../../../../core/server/lib/common/errors'); const sessionController = require('../../../../core/server/api/canary/session'); -const sessionServiceMiddleware = require('../../../../core/server/services/auth/session/middleware'); +const sessionServiceMiddleware = require('../../../../core/server/services/auth/session').middleware; describe('Session controller', function () { before(function () { diff --git a/test/unit/api/v2/session_spec.js b/test/unit/api/v2/session_spec.js index 0bec542c55..8ff08d2cef 100644 --- a/test/unit/api/v2/session_spec.js +++ b/test/unit/api/v2/session_spec.js @@ -5,7 +5,7 @@ const models = require('../../../../core/server/models'); const {UnauthorizedError} = require('../../../../core/server/lib/common/errors'); const sessionController = require('../../../../core/server/api/v2/session'); -const sessionServiceMiddleware = require('../../../../core/server/services/auth/session/middleware'); +const sessionServiceMiddleware = require('../../../../core/server/services/auth/session').middleware; describe('v2 Session controller', function () { before(function () { diff --git a/test/unit/api/v3/session_spec.js b/test/unit/api/v3/session_spec.js index a38e247489..6b4906d718 100644 --- a/test/unit/api/v3/session_spec.js +++ b/test/unit/api/v3/session_spec.js @@ -5,7 +5,7 @@ const models = require('../../../../core/server/models'); const {UnauthorizedError} = require('../../../../core/server/lib/common/errors'); const sessionController = require('../../../../core/server/api/canary/session'); -const sessionServiceMiddleware = require('../../../../core/server/services/auth/session/middleware'); +const sessionServiceMiddleware = require('../../../../core/server/services/auth/session').middleware; describe('v3 Session controller', function () { before(function () { diff --git a/test/unit/services/auth/session/middleware_spec.js b/test/unit/services/auth/session/middleware_spec.js index edf5edcf46..1f0be4fb5a 100644 --- a/test/unit/services/auth/session/middleware_spec.js +++ b/test/unit/services/auth/session/middleware_spec.js @@ -1,4 +1,4 @@ -const sessionMiddleware = require('../../../../../core/server/services/auth/session/middleware'); +const sessionMiddleware = require('../../../../../core/server/services/auth').session; const models = require('../../../../../core/server/models'); const sinon = require('sinon'); const should = require('should'); @@ -93,14 +93,21 @@ describe('Session Service', function () { }); describe('destroySession', function () { - it('calls req.session.destroy', function () { + it('calls req.session.destroy', function (done) { const req = fakeReq(); const res = fakeRes(); - const destroyStub = sinon.stub(req.session, 'destroy'); + const destroyStub = sinon.stub(req.session, 'destroy') + .callsFake(function (fn) { + fn(); + }); + + sinon.stub(res, 'sendStatus') + .callsFake(function (statusCode) { + should.equal(destroyStub.callCount, 1); + done(); + }); sessionMiddleware.destroySession(req, res); - - should.equal(destroyStub.callCount, 1); }); it('calls next with InternalServerError if destroy errors', function (done) { @@ -133,40 +140,4 @@ describe('Session Service', function () { sessionMiddleware.destroySession(req, res); }); }); - - describe('CSRF protection', function () { - it('calls next if the session is uninitialized', function (done) { - const req = fakeReq(); - const res = fakeRes(); - - sessionMiddleware.cookieCsrfProtection(req); - done(); - }); - - it('calls next if req origin matches the session origin', function (done) { - const req = fakeReq(); - const res = fakeRes(); - sinon.stub(req, 'get') - .withArgs('origin').returns('http://host.tld'); - req.session.origin = 'http://host.tld'; - - sessionMiddleware.cookieCsrfProtection(req); - done(); - }); - - it('calls next with BadRequestError if the origin of req does not match the session', function (done) { - const req = fakeReq(); - const res = fakeRes(); - sinon.stub(req, 'get') - .withArgs('origin').returns('http://host.tld'); - req.session.origin = 'http://different-host.tld'; - - try { - sessionMiddleware.cookieCsrfProtection(req); - } catch (err) { - should.equal(err instanceof BadRequestError, true); - done(); - } - }); - }); }); diff --git a/yarn.lock b/yarn.lock index d7aea86d93..205985d7aa 100644 --- a/yarn.lock +++ b/yarn.lock @@ -448,6 +448,13 @@ request "^2.88.0" request-promise "^4.2.4" +"@tryghost/session-service@^0.1.0": + version "0.1.0" + resolved "https://registry.yarnpkg.com/@tryghost/session-service/-/session-service-0.1.0.tgz#3e37f1047b6404cbac59de0953ef1246657e742d" + integrity sha512-yAnJ0Bnl5FReA8mrL+J7nc7S9QWobegaOqK0lTHQpbCHdlkXaDdkNYFniztE8BClWmHrRrtRfHLTN+eovEpenA== + dependencies: + "@tryghost/errors" "^0.1.1" + "@tryghost/social-urls@0.1.7": version "0.1.7" resolved "https://registry.yarnpkg.com/@tryghost/social-urls/-/social-urls-0.1.7.tgz#a62b008c16e2e1f6d7519a9f36f3b2966be2bad8"