From 810c3077e85f4e0f5b5ef1e2d343c19df16c4785 Mon Sep 17 00:00:00 2001 From: Fabien 'egg' O'Carroll Date: Wed, 27 Apr 2022 17:44:16 +0100 Subject: [PATCH] Wired up LimitService to NewsletterService (#14602) refs https://github.com/TryGhost/Team/issues/1549 This allows us to restrict certain sites to a single newsletter --- core/server/services/newsletters/index.js | 4 ++- core/server/services/newsletters/service.js | 12 ++++++- package.json | 2 +- .../__snapshots__/newsletters.test.js.snap | 19 +++++++++++ test/e2e-api/admin/newsletters.test.js | 34 +++++++++++++++++++ .../services/newsletters/service.test.js | 25 +++++++++++++- yarn.lock | 8 ++--- 7 files changed, 96 insertions(+), 8 deletions(-) diff --git a/core/server/services/newsletters/index.js b/core/server/services/newsletters/index.js index 762556cbde..9f09b22576 100644 --- a/core/server/services/newsletters/index.js +++ b/core/server/services/newsletters/index.js @@ -3,6 +3,7 @@ const SingleUseTokenProvider = require('../members/SingleUseTokenProvider'); const mail = require('../mail'); const models = require('../../models'); const urlUtils = require('../../../shared/url-utils'); +const limitService = require('../limits'); const MAGIC_LINK_TOKEN_VALIDITY = 24 * 60 * 60 * 1000; @@ -11,5 +12,6 @@ module.exports = new NewslettersService({ MemberModel: models.Member, mail, singleUseTokenProvider: new SingleUseTokenProvider(models.SingleUseToken, MAGIC_LINK_TOKEN_VALIDITY), - urlUtils + urlUtils, + limitService }); diff --git a/core/server/services/newsletters/service.js b/core/server/services/newsletters/service.js index 4208690afe..176c8a4ed7 100644 --- a/core/server/services/newsletters/service.js +++ b/core/server/services/newsletters/service.js @@ -13,11 +13,14 @@ class NewslettersService { * @param {Object} options.mail * @param {Object} options.singleUseTokenProvider * @param {Object} options.urlUtils + * @param {ILimitService} options.limitService */ - constructor({NewsletterModel, MemberModel, mail, singleUseTokenProvider, urlUtils}) { + constructor({NewsletterModel, MemberModel, mail, singleUseTokenProvider, urlUtils, limitService}) { this.NewsletterModel = NewsletterModel; this.MemberModel = MemberModel; this.urlUtils = urlUtils; + /** @private */ + this.limitService = limitService; /* email verification setup */ @@ -96,6 +99,8 @@ class NewslettersService { }); } + await this.limitService.errorIfWouldGoOverLimit('newsletters'); + // remove any email properties that are not allowed to be set without verification const {cleanedAttrs, emailsToVerify} = await this.prepAttrsForEmailVerification(attrs); @@ -242,4 +247,9 @@ class NewslettersService { } } +/** + * @typedef {object} ILimitService + * @prop {(name: string) => Promise} errorIfWouldGoOverLimit + **/ + module.exports = NewslettersService; diff --git a/package.json b/package.json index 5603609702..c2eca5182b 100644 --- a/package.json +++ b/package.json @@ -80,7 +80,7 @@ "@tryghost/kg-default-cards": "5.16.2", "@tryghost/kg-markdown-html-renderer": "5.1.5", "@tryghost/kg-mobiledoc-html-renderer": "5.3.5", - "@tryghost/limit-service": "1.0.11", + "@tryghost/limit-service": "1.1.0", "@tryghost/logging": "2.1.8", "@tryghost/magic-link": "1.0.21", "@tryghost/member-events": "0.4.1", diff --git a/test/e2e-api/admin/__snapshots__/newsletters.test.js.snap b/test/e2e-api/admin/__snapshots__/newsletters.test.js.snap index 8b16317479..0b4b6cc503 100644 --- a/test/e2e-api/admin/__snapshots__/newsletters.test.js.snap +++ b/test/e2e-api/admin/__snapshots__/newsletters.test.js.snap @@ -654,3 +654,22 @@ Object { ], } `; + +exports[`Newsletters API Host Settings: newsletter limits Request fails when newsletter limit is in place 1: [body] 1`] = ` +Object { + "errors": Array [ + Object { + "code": null, + "context": "Nuh uh", + "details": Object { + "name": "newsletters", + }, + "help": "https://ghost.org/help/", + "id": StringMatching /\\[a-f0-9\\]\\{8\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{12\\}/, + "message": "Host Limit error, cannot save newsletter.", + "property": null, + "type": "HostLimitError", + }, + ], +} +`; diff --git a/test/e2e-api/admin/newsletters.test.js b/test/e2e-api/admin/newsletters.test.js index 544d6505a5..87f4c0c869 100644 --- a/test/e2e-api/admin/newsletters.test.js +++ b/test/e2e-api/admin/newsletters.test.js @@ -1,5 +1,6 @@ const {agentProvider, mockManager, fixtureManager, matchers} = require('../../utils/e2e-framework'); const {anyEtag, anyObjectId, anyUuid, anyISODateTime, anyLocationFor} = matchers; +const configUtils = require('../../utils/configUtils'); const uuid = require('uuid'); const assert = require('assert'); @@ -261,4 +262,37 @@ describe('Newsletters API', function () { newsletters: [newsletterSnapshot] }); }); + + describe('Host Settings: newsletter limits', function () { + afterEach(function () { + configUtils.set('hostSettings:limits', undefined); + }); + + it('Request fails when newsletter limit is in place', async function () { + configUtils.set('hostSettings:limits', { + newsletters: { + disabled: true, + error: 'Nuh uh' + } + }); + + agent = await agentProvider.getAdminAPIAgent(); + await fixtureManager.init('newsletters', 'members:newsletters'); + await agent.loginAsOwner(); + + const newsletter = { + name: 'Naughty newsletter' + }; + + await agent + .post(`newsletters/?opt_in_existing=true`) + .body({newsletters: [newsletter]}) + .expectStatus(403) + .matchBodySnapshot({ + errors: [{ + id: anyUuid + }] + }); + }); + }); }); diff --git a/test/unit/server/services/newsletters/service.test.js b/test/unit/server/services/newsletters/service.test.js index 3f01d507e7..0b0e683abb 100644 --- a/test/unit/server/services/newsletters/service.test.js +++ b/test/unit/server/services/newsletters/service.test.js @@ -23,18 +23,25 @@ class TestTokenProvider { describe('NewslettersService', function () { let newsletterService, getStub, tokenProvider; + /** @type {NewslettersService.ILimitService} */ + let limitService; before(function () { models.init(); tokenProvider = new TestTokenProvider(); + limitService = { + async errorIfWouldGoOverLimit() {} + }; + newsletterService = new NewslettersService({ NewsletterModel: models.Newsletter, MemberModel: models.Member, mail, singleUseTokenProvider: tokenProvider, - urlUtils: urlUtils.stubUrlUtilsFromConfig() + urlUtils: urlUtils.stubUrlUtilsFromConfig(), + limitService }); }); @@ -74,6 +81,22 @@ describe('NewslettersService', function () { getNextAvailableSortOrderStub = sinon.stub(models.Newsletter, 'getNextAvailableSortOrder').returns(1); }); + it('rejects if the limit services determines it would be over the limit', async function () { + const error = new Error('No way, Jose!'); + sinon.stub(limitService, 'errorIfWouldGoOverLimit').rejects(error); + + let thrownError; + try { + await newsletterService.add({ + name: 'Newsletter Name' + }); + } catch (err) { + thrownError = err; + } + assert(thrownError, 'It should have thrown an error'); + assert(thrownError === error, 'It should have rethrown the error from limit service'); + }); + it('rejects if called with no data', async function () { assert.rejects(await newsletterService.add, {name: 'TypeError'}); sinon.assert.notCalled(addStub); diff --git a/yarn.lock b/yarn.lock index 3ab83c8eaf..acc43f45ec 100644 --- a/yarn.lock +++ b/yarn.lock @@ -2091,10 +2091,10 @@ dependencies: semver "^7.3.5" -"@tryghost/limit-service@1.0.11": - version "1.0.11" - resolved "https://registry.yarnpkg.com/@tryghost/limit-service/-/limit-service-1.0.11.tgz#93424eac034dacd6bcf8ab661f121eae55bf14ea" - integrity sha512-Xuh7rUlmy/nLwDuLAJQOBPTnLHWKZoyHsEnFT7SgA4ra2ZqSDcWECpFW6QyARdB1AXpqCXkT8QGhXkTyytAtUg== +"@tryghost/limit-service@1.1.0": + version "1.1.0" + resolved "https://registry.yarnpkg.com/@tryghost/limit-service/-/limit-service-1.1.0.tgz#12af2d0b8242212871be8cf893e2563466c25ec2" + integrity sha512-CC0/Ly1KIVIiYMKwqWfgKPkFoGssyyPjwcdS4XzJ3KyqdwEckzPX1e2jZMzptZlMqDuAa4uZ0xSf5VH4YZ71+Q== dependencies: "@tryghost/errors" "^1.2.1" lodash "^4.17.21"