Extracted members-specific middleware from site app module (#11405)

no issue

- In order to keep site/app.js module tidy and less coupled with members module we need to extract some of the functionality where it belongs conceptually
- Added "members enabled check" middleware to stripe webhook endpoint
- Reshuffled members middleware so that siteApp is in control of mounting points. This is meant to be a more explicit way to see which endpoints are being handled by members middleware
- Extracted member-specific public file middleware
- Unified use of `labs.member` alias method. Done for code style consistency
- Added basic members' test suite. This is a base we could work from when more modifications are needed
- Removed route handler for unexisting members file "members-theme-bindings.js". Calling this route otherwise causes a 500. Looks like a leftover from 49672a1e4d

core/server/services/members/index.js

@@ -49,3 +49,4 @@ const membersService = {
 };
 
 module.exports = membersService;
+module.exports.middleware = require('./middleware');

core/server/services/members/middleware.js

@@ -0,0 +1,97 @@
+const common = require('../../lib/common');
+const constants = require('../../lib/constants');
+const shared = require('../../web/shared');
+const labsService = require('../labs');
+const membersService = require('./index');
+
+const login = async function (req, res) {
+    try {
+        const token = await membersService.ssr.getIdentityTokenForMemberFromSession(req, res);
+        res.writeHead(200);
+        res.end(token);
+    } catch (err) {
+        common.logging.warn(err.message);
+        res.writeHead(err.statusCode);
+        res.end(err.message);
+    }
+};
+
+const logout = async function (req, res) {
+    try {
+        await membersService.ssr.deleteSession(req, res);
+        res.writeHead(204);
+        res.end();
+    } catch (err) {
+        common.logging.warn(err.message);
+        res.writeHead(err.statusCode);
+        res.end(err.message);
+    }
+};
+
+const getMemberDataFromSession = async function (req, res, next) {
+    if (!labsService.isSet('members')) {
+        req.member = null;
+        return next();
+    }
+    try {
+        const member = await membersService.ssr.getMemberDataFromSession(req, res);
+        Object.assign(req, {member});
+        next();
+    } catch (err) {
+        common.logging.warn(err.message);
+        Object.assign(req, {member: null});
+        next();
+    }
+};
+
+const exchangeTokenForSession = async function (req, res, next) {
+    if (!labsService.isSet('members')) {
+        return next();
+    }
+    if (!req.url.includes('token=')) {
+        return next();
+    }
+    try {
+        const member = await membersService.ssr.exchangeTokenForSession(req, res);
+        Object.assign(req, {member});
+        next();
+    } catch (err) {
+        common.logging.warn(err.message);
+        return next();
+    }
+};
+
+const decorateResponse = function (req, res, next) {
+    res.locals.member = req.member;
+    next();
+};
+
+// @TODO only loads this stuff if members is enabled
+// Set req.member & res.locals.member if a cookie is set
+module.exports = {
+    public: [
+        shared.middlewares.labs.members,
+        shared.middlewares.servePublicFile.createPublicFileMiddleware(
+            'public/members.js',
+            'application/javascript',
+            constants.ONE_HOUR_S
+        )
+    ],
+    authentication: [
+        getMemberDataFromSession,
+        exchangeTokenForSession,
+        decorateResponse
+    ],
+    login: [
+        shared.middlewares.labs.members,
+        login
+    ],
+    logout: [
+        shared.middlewares.labs.members,
+        logout
+    ],
+    stripeWebhooks: [
+        shared.middlewares.labs.members,
+        (req, res, next) => membersService.api.middleware.handleStripeWebhook(req, res, next)
+    ]
+};

core/server/web/site/app.js

@@ -6,17 +6,16 @@ const {URL} = require('url');
 
 // App requires
 const config = require('../../config');
-const common = require('../../lib/common');
 const apps = require('../../services/apps');
 const constants = require('../../lib/constants');
 const storage = require('../../adapters/storage');
 const urlService = require('../../../frontend/services/url');
-const labsService = require('../../services/labs');
 const urlUtils = require('../../lib/url-utils');
 const sitemapHandler = require('../../../frontend/services/sitemap/handler');
 const themeService = require('../../../frontend/services/themes');
 const themeMiddleware = themeService.middleware;
 const membersService = require('../../services/members');
+const membersMiddleware = membersService.middleware;
 const siteRoutes = require('./routes');
 const shared = require('../shared');
 
@@ -95,22 +94,7 @@ module.exports = function setupSiteApp(options = {}) {
     siteApp.use(shared.middlewares.serveFavicon());
 
     // /public/members.js
-    siteApp.get('/public/members-theme-bindings.js',
-        shared.middlewares.labs('members'),
-        shared.middlewares.servePublicFile.createPublicFileMiddleware(
-            'public/members-theme-bindings.js',
-            'application/javascript',
-            constants.ONE_HOUR_S
-        )
-    );
-    siteApp.get('/public/members.js',
-        shared.middlewares.labs('members'),
-        shared.middlewares.servePublicFile.createPublicFileMiddleware(
-            'public/members.js',
-            'application/javascript',
-            constants.ONE_HOUR_S
-        )
-    );
+    siteApp.get('/public/members.js', membersMiddleware.public);
 
     // Serve sitemap.xsl file
     siteApp.use(shared.middlewares.servePublicFile('sitemap.xsl', 'text/xsl', constants.ONE_DAY_S));
@@ -133,67 +117,13 @@ module.exports = function setupSiteApp(options = {}) {
     require('../../../frontend/helpers').loadCoreHelpers();
     debug('Helpers done');
 
-    // @TODO only loads this stuff if members is enabled
-    // Set req.member & res.locals.member if a cookie is set
-    siteApp.get('/members/ssr', shared.middlewares.labs.members, async function (req, res) {
-        try {
-            const token = await membersService.ssr.getIdentityTokenForMemberFromSession(req, res);
-            res.writeHead(200);
-            res.end(token);
-        } catch (err) {
-            common.logging.warn(err.message);
-            res.writeHead(err.statusCode);
-            res.end(err.message);
-        }
-    });
+    // Members middleware
+    // Initializes members specific routes as well as assigns members specific data to the req/res objects
+    siteApp.get('/members/ssr', membersMiddleware.login);
+    siteApp.delete('/members/ssr', membersMiddleware.logout);
+    siteApp.post('/members/webhooks/stripe', membersMiddleware.stripeWebhooks);
 
-    siteApp.delete('/members/ssr', shared.middlewares.labs.members, async function (req, res) {
-        try {
-            await membersService.ssr.deleteSession(req, res);
-            res.writeHead(204);
-            res.end();
-        } catch (err) {
-            common.logging.warn(err.message);
-            res.writeHead(err.statusCode);
-            res.end(err.message);
-        }
-    });
-    siteApp.post('/members/webhooks/stripe', (req, res, next) => membersService.api.middleware.handleStripeWebhook(req, res, next));
-    siteApp.use(async function (req, res, next) {
-        if (!labsService.isSet('members')) {
-            req.member = null;
-            return next();
-        }
-        try {
-            const member = await membersService.ssr.getMemberDataFromSession(req, res);
-            Object.assign(req, {member});
-            next();
-        } catch (err) {
-            common.logging.warn(err.message);
-            Object.assign(req, {member: null});
-            next();
-        }
-    });
-    siteApp.use(async function (req, res, next) {
-        if (!labsService.isSet('members')) {
-            return next();
-        }
-        if (!req.url.includes('token=')) {
-            return next();
-        }
-        try {
-            const member = await membersService.ssr.exchangeTokenForSession(req, res);
-            Object.assign(req, {member});
-            next();
-        } catch (err) {
-            common.logging.warn(err.message);
-            return next();
-        }
-    });
-    siteApp.use(function (req, res, next) {
-        res.locals.member = req.member;
-        next();
-    });
+    siteApp.use(membersMiddleware.authentication);
 
     // Theme middleware
     // This should happen AFTER any shared assets are served, as it only changes things to do with templates

core/test/regression/site/members_spec.js

@@ -0,0 +1,106 @@
+const should = require('should');
+const sinon = require('sinon');
+const supertest = require('supertest');
+const testUtils = require('../../utils');
+const configUtils = require('../../utils/configUtils');
+const settingsCache = require('../../../server/services/settings/cache');
+
+const ghost = testUtils.startGhost;
+
+// NOTE: if only this suite is run some of the tests will fail due to
+//       wrong template loading issues which would need to be investigated
+//       As a workaround run it with some of other tests e.g. "frontend_spec"
+describe('Integration - Web - Members', function () {
+    let request;
+
+    before(function () {
+        return ghost()
+            .then(function () {
+                request = supertest.agent(configUtils.config.get('url'));
+            });
+    });
+
+    describe('Members enabled', function () {
+        before(function () {
+            const originalSettingsCacheGetFn = settingsCache.get;
+
+            sinon.stub(settingsCache, 'get').callsFake(function (key, options) {
+                if (key === 'labs') {
+                    return {members: true};
+                }
+
+                return originalSettingsCacheGetFn(key, options);
+            });
+        });
+
+        after(function () {
+            sinon.restore();
+        });
+
+        describe('Static files', function () {
+            it('should serve members.js file', function () {
+                return request.get('/public/members.js')
+                    .expect(200);
+            });
+        });
+
+        describe('Routes', function () {
+            it('should error when invalid member token is passed in to ssr', function () {
+                return request.get('/members/ssr')
+                    .expect(400);
+            });
+
+            it('should return no content when removing member sessions', function () {
+                return request.del('/members/ssr')
+                    .expect(204);
+            });
+
+            it('should error serving webhook endpoint without any parameters', function () {
+                return request.post('/members/webhooks/stripe')
+                    .expect(400);
+            });
+        });
+    });
+
+    describe('Members disabled', function () {
+        before(function () {
+            const originalSettingsCacheGetFn = settingsCache.get;
+
+            sinon.stub(settingsCache, 'get').callsFake(function (key, options) {
+                if (key === 'labs') {
+                    return {members: false};
+                }
+
+                return originalSettingsCacheGetFn(key, options);
+            });
+        });
+
+        after(function () {
+            sinon.restore();
+        });
+
+        describe('Static files', function () {
+            it('should not serve members js file', function () {
+                return request.get('/public/members.js')
+                    .expect(404);
+            });
+        });
+
+        describe('Routes', function () {
+            it('should not serve ssr endpoint', function () {
+                return request.get('/members/ssr')
+                    .expect(404);
+            });
+
+            it('should not serve ssr removal endpoint', function () {
+                return request.del('/members/ssr')
+                    .expect(404);
+            });
+
+            it('should not serve webhook endpoint', function () {
+                return request.post('/members/webhooks/stripe')
+                    .expect(404);
+            });
+        });
+    });
+});