Added support for token session to /ghost (#11709)

no-issue * Added default for getting origin of request This function is used to attach the origin of the request to the session, and later check that requests using the session are coming from the same origin. This protects us against CSRF attacks as requests in the browser MUST originate from the same origin on which the user logged in. Previously, when we could not determine the origin we would return null, as a "safety" net. This updates the function to use a secure and sensible default - which is the origin of the Ghost-Admin application, and if that's not set - the origin of the Ghost application. This will make dealing with magic links simpler as you can not always guaruntee the existence of these headers when visiting via a hyperlink * Removed init fns and getters from session service This simplifies the code here, making it easier to read and maintain * Moved express-session initialisation to own file This is complex enough that it deserves its own module * Added createSessionFromToken to session service * Wired up the createSessionFromToken middleware
TryGhost · Apr 6, 2020 · a701ee7 · a701ee7
1 parent 022a433
commit a701ee7
Show file tree

Hide file tree

Showing 9 changed files with 78 additions and 125 deletions.
diff --git a/core/server/services/auth/session/express-session.js b/core/server/services/auth/session/express-session.js
@@ -0,0 +1,37 @@
+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 SessionStore = require('./store');
+
+const 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'))
+    }
+});
+
+exports.getSession = 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);
+        });
+    });
+};
diff --git a/core/server/services/auth/session/index.js b/core/server/services/auth/session/index.js
@@ -1,18 +1,17 @@
-const session = require('express-session');
-const constants = require('../../../lib/constants');
-const config = require('../../../config');
-const settingsCache = require('../../settings/cache');
+const adapterManager = require('../../adapter-manager');
+const createSessionService = require('@tryghost/session-service');
+const sessionFromToken = require('@tryghost/mw-session-from-token');
+const createSessionMiddleware = require('./middleware');
+
+const expressSession = require('./express-session');
+
 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');
+    const referrer = req.get('referrer') || urlUtils.getAdminUrl() || urlUtils.getSiteUrl();
 
     if (!origin && !referrer) {
         return null;
@@ -29,95 +28,22 @@ function getOriginOfRequest(req) {
     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 = {
-    get createSession() {
-        return this.middleware.createSession;
-    },
-
-    get destroySession() {
-        return this.middleware.destroySession;
-    },
-
-    get authenticate() {
-        return this.middleware.authenticate;
-    },
-
-    get service() {
-        if (!sessionService) {
-            initSessionService();
-        }
-        return sessionService;
-    },
-
-    get middleware() {
-        if (!sessionMiddleware) {
-            initSessionMiddleware();
-        }
-        return sessionMiddleware;
-    }
-};
+const sessionService = createSessionService({
+    getOriginOfRequest,
+    getSession: expressSession.getSession,
+    findUserById({id}) {
+        return models.User.findOne({id});
+    }
+});
+
+module.exports = createSessionMiddleware({sessionService});
+
+const ssoAdapter = adapterManager.getAdapter('sso');
+// Looks funky but this is a "custom" piece of middleware
+module.exports.createSessionFromToken = sessionFromToken({
+    callNextWithError: false,
+    createSession: sessionService.createSessionForUser,
+    findUserByLookup: ssoAdapter.getUserForIdentity,
+    getLookupFromToken: ssoAdapter.getIdentityFromCredentials,
+    getTokenFromRequest: ssoAdapter.getRequestCredentials
+});
diff --git a/core/server/web/parent-app.js b/core/server/web/parent-app.js
@@ -59,7 +59,7 @@ module.exports = function setupParentApp(options = {}) {
     adminApp.enable('trust proxy'); // required to respect x-forwarded-proto in admin requests
     adminApp.use('/ghost/api', require('./api')());
     adminApp.use('/ghost/.well-known', require('./well-known')());
-    adminApp.use('/ghost', require('./admin')());
+    adminApp.use('/ghost', require('../services/auth/session').createSessionFromToken, require('./admin')());
 
     // TODO: remove {admin url}/content/* once we're sure the API is not returning relative asset URLs anywhere
     // only register this route if the admin is separate so we're not overriding the {site}/content/* route

diff --git a/package.json b/package.json
@@ -50,6 +50,7 @@
     "@tryghost/kg-markdown-html-renderer": "1.0.2",
     "@tryghost/members-api": "0.18.0",
     "@tryghost/members-ssr": "0.7.4",
+    "@tryghost/mw-session-from-token": "^0.1.0",
     "@tryghost/session-service": "^0.1.0",
     "@tryghost/social-urls": "0.1.8",
     "@tryghost/string": "0.1.8",

diff --git 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');
 
 describe('Session controller', function () {
     before(function () {

diff --git 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');
 
 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
@@ -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');
 
 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
@@ -2,11 +2,6 @@ const sessionMiddleware = require('../../../../../core/server/services/auth').se
 const models = require('../../../../../core/server/models');
 const sinon = require('sinon');
 const should = require('should');
-const {
-    BadRequestError,
-    UnauthorizedError,
-    InternalServerError
-} = require('../../../../../core/server/lib/common/errors');
 
 describe('Session Service', function () {
     before(function () {
@@ -22,6 +17,7 @@ describe('Session Service', function () {
             session: {
                 destroy() {}
             },
+            user: null,
             body: {},
             get() {}
         };
@@ -34,18 +30,6 @@ describe('Session Service', function () {
     };
 
     describe('createSession', function () {
-        it('calls next with a BadRequestError if there is no Origin or Refferer', function (done) {
-            const req = fakeReq();
-            sinon.stub(req, 'get')
-                .withArgs('origin').returns('')
-                .withArgs('referrer').returns('');
-
-            sessionMiddleware.createSession(req, fakeRes(), function next(err) {
-                should.equal(err instanceof BadRequestError, true);
-                done();
-            });
-        });
-
         it('sets req.session.origin from the Referer header', function (done) {
             const req = fakeReq();
             const res = fakeRes();
@@ -59,7 +43,7 @@ describe('Session Service', function () {
             req.user = models.User.forge({id: 23});
 
             sinon.stub(res, 'sendStatus')
-                .callsFake(function (statusCode) {
+                .callsFake(function () {
                     should.equal(req.session.origin, 'http://ghost.org');
                     done();
                 });
@@ -102,7 +86,7 @@ describe('Session Service', function () {
                 });
 
             sinon.stub(res, 'sendStatus')
-                .callsFake(function (statusCode) {
+                .callsFake(function () {
                     should.equal(destroyStub.callCount, 1);
                     done();
                 });
@@ -119,7 +103,7 @@ describe('Session Service', function () {
                 });
 
             sessionMiddleware.destroySession(req, res, function next(err) {
-                should.equal(err instanceof InternalServerError, true);
+                should.equal(err.errorType, 'InternalServerError');
                 done();
             });
         });

diff --git a/yarn.lock b/yarn.lock
@@ -435,6 +435,11 @@
     mobiledoc-dom-renderer "0.7.0"
     mobiledoc-text-renderer "0.4.0"
 
+"@tryghost/mw-session-from-token@^0.1.0":
+  version "0.1.0"
+  resolved "https://registry.yarnpkg.com/@tryghost/mw-session-from-token/-/mw-session-from-token-0.1.0.tgz#ad73da936d3cc3abdfc7753a5098e76ae5dc271e"
+  integrity sha512-t6a9YMMHaQ2Uypl+WaGLc4mFKjwhfyfA+l7V9H5DwFeAFIYWcRssy/D3e+gTT3rbLC39W0vqkmZFNE2JQZuuEQ==
+
 "@tryghost/pretty-cli@1.2.3":
   version "1.2.3"
   resolved "https://registry.yarnpkg.com/@tryghost/pretty-cli/-/pretty-cli-1.2.3.tgz#06fc84e4659ffde7166ca2a9ebe17c3951438a6c"