From 193c179110dfe208dd843123befbb3606b35a634 Mon Sep 17 00:00:00 2001 From: Naz Gargol Date: Thu, 21 Nov 2019 10:01:24 +0700 Subject: [PATCH] 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 49672a1e4d268683feb67a574429b910417100c1 --- core/server/services/members/index.js | 1 + core/server/services/members/middleware.js | 97 +++++++++++++++++++ core/server/web/site/app.js | 86 ++--------------- core/test/regression/site/members_spec.js | 106 +++++++++++++++++++++ 4 files changed, 212 insertions(+), 78 deletions(-) create mode 100644 core/server/services/members/middleware.js create mode 100644 core/test/regression/site/members_spec.js diff --git a/core/server/services/members/index.js b/core/server/services/members/index.js index 53daf7fb1b..082b2b1a76 100644 --- a/core/server/services/members/index.js +++ b/core/server/services/members/index.js @@ -49,3 +49,4 @@ const membersService = { }; module.exports = membersService; +module.exports.middleware = require('./middleware'); diff --git a/core/server/services/members/middleware.js b/core/server/services/members/middleware.js new file mode 100644 index 0000000000..c9d45c3fc0 --- /dev/null +++ b/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) + ] +}; diff --git a/core/server/web/site/app.js b/core/server/web/site/app.js index 261bf8de1c..cd2850660d 100644 --- a/core/server/web/site/app.js +++ b/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 diff --git a/core/test/regression/site/members_spec.js b/core/test/regression/site/members_spec.js new file mode 100644 index 0000000000..5c976e00bf --- /dev/null +++ b/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); + }); + }); + }); +});