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
2023-12-13 21:00:40 +01:00 · 2020-04-02 16:27:31 +02:00 · 2020-04-02 16:27:31 +02:00 · 23154f0739
commit 23154f0739
parent 5a748ee5a9
9 changed files with 169 additions and 184 deletions
--- 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;
    }
 };
--- 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;
--- 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
--- 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",
--- 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 () {
--- 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 () {
--- 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 () {
--- 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();
-            }
-        });
-    });
 });
--- 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"