diff --git a/ghost/core/core/server/services/webhooks/WebhookTrigger.js b/ghost/core/core/server/services/webhooks/WebhookTrigger.js index 68b9239523..e88545f541 100644 --- a/ghost/core/core/server/services/webhooks/WebhookTrigger.js +++ b/ghost/core/core/server/services/webhooks/WebhookTrigger.js @@ -9,16 +9,31 @@ class WebhookTrigger { * @param {Object} options * @param {Object} options.models - Ghost models * @param {Function} options.payload - Function to generate payload + * @param {import('../../services/limits')} options.limitService - Function to generate payload * @param {Object} [options.request] - HTTP request handling library */ - constructor({models, payload, request}){ + constructor({models, payload, request, limitService}){ this.models = models; this.payload = payload; this.request = request ?? require('@tryghost/request'); + this.limitService = limitService; } - getAll(event) { + async getAll(event) { + if (this.limitService.isLimited('customIntegrations')) { + // NOTE: using "checkWouldGoOverLimit" instead of "checkIsOverLimit" here because flag limits don't have + // a concept of measuring if the limit has been surpassed + const overLimit = await this.limitService.checkWouldGoOverLimit('customIntegrations'); + + if (overLimit) { + logging.info(`Skipping all webhooks for event ${event}. The "customIntegrations" plan limit is enabled.`); + return { + models: [] + }; + } + } + return this.models .Webhook .findAllByEvent(event, {context: {internal: true}}); diff --git a/ghost/core/core/server/services/webhooks/listen.js b/ghost/core/core/server/services/webhooks/listen.js index d76e6fcd62..4ce8817a8a 100644 --- a/ghost/core/core/server/services/webhooks/listen.js +++ b/ghost/core/core/server/services/webhooks/listen.js @@ -1,6 +1,5 @@ const _ = require('lodash'); const limitService = require('../../services/limits'); -const logging = require('@tryghost/logging'); const WebhookTrigger = require('./WebhookTrigger'); const models = require('../../models'); const payload = require('./payload'); @@ -46,18 +45,7 @@ const WEBHOOKS = [ ]; const listen = async () => { - if (limitService.isLimited('customIntegrations')) { - // NOTE: using "checkWouldGoOverLimit" instead of "checkIsOverLimit" here because flag limits don't have - // a concept of measuring if the limit has been surpassed - const overLimit = await limitService.checkWouldGoOverLimit('customIntegrations'); - - if (overLimit) { - logging.info(`Skipped subscribing webhooks to events. The "customIntegrations" plan limit is enabled.`); - return; - } - } - - const webhookTrigger = new WebhookTrigger({models, payload}); + const webhookTrigger = new WebhookTrigger({models, payload, limitService}); _.each(WEBHOOKS, (event) => { // @NOTE: The early exit makes sure the listeners are only registered once. // During testing the "events" instance is kept around with all the diff --git a/ghost/core/test/e2e-webhooks/site.test.js b/ghost/core/test/e2e-webhooks/site.test.js index 86278fa469..0d1158352d 100644 --- a/ghost/core/test/e2e-webhooks/site.test.js +++ b/ghost/core/test/e2e-webhooks/site.test.js @@ -48,4 +48,43 @@ describe('site.* events', function () { }) .matchBodySnapshot(); }); + + it('site.changed event is triggered but the custom integrations are limited', async function () { + const webhookURL = 'https://test-webhook-receiver.com/site-changed'; + await webhookMockReceiver.mock(webhookURL); + await fixtureManager.insertWebhook({ + event: 'site.changed', + url: webhookURL + }); + + mockManager.mockLimitService('customIntegrations', { + isLimited: true, + wouldGoOverLimit: true + }); + + await adminAPIAgent + .post('posts/') + .body({ + posts: [{ + title: 'webhookz', + status: 'published', + mobiledoc: fixtureManager.get('posts', 1).mobiledoc + }] + }) + .expectStatus(201); + + const receivedRequest = webhookMockReceiver.receivedRequest().then(() => true); + const wait = new Promise((resolve) => { + setTimeout(resolve, 2000, false); + }); + + const requestWasRecieved = await Promise.race([ + receivedRequest, + wait + ]); + + if (requestWasRecieved) { + throw new Error('The webhook should not have been sent.'); + } + }); }); diff --git a/ghost/core/test/unit/server/services/webhooks/trigger.test.js b/ghost/core/test/unit/server/services/webhooks/trigger.test.js index b4791ab7d0..f6146b368d 100644 --- a/ghost/core/test/unit/server/services/webhooks/trigger.test.js +++ b/ghost/core/test/unit/server/services/webhooks/trigger.test.js @@ -1,6 +1,7 @@ const assert = require('assert/strict'); const crypto = require('crypto'); const sinon = require('sinon'); +const LimitService = require('@tryghost/limit-service'); const WebhookTrigger = require('../../../../../core/server/services/webhooks/WebhookTrigger'); @@ -12,7 +13,7 @@ describe('Webhook Service', function () { const WEBHOOK_TARGET_URL = 'http://example.com'; const WEBHOOK_SECRET = 'abc123dontstealme'; - let models, payload, request, webhookTrigger; + let models, payload, request, webhookTrigger, limitService; beforeEach(function () { models = { @@ -34,7 +35,12 @@ describe('Webhook Service', function () { payload = sinon.stub(); request = sinon.stub().resolves({}); - webhookTrigger = new WebhookTrigger({models, payload, request}); + const realLimitService = new LimitService(); + limitService = sinon.stub(realLimitService); + limitService.isLimited.withArgs('customIntegrations').returns(false); + limitService.checkWouldGoOverLimit.withArgs('customIntegrations').resolves(false); + + webhookTrigger = new WebhookTrigger({models, payload, request, limitService}); sinon.stub(webhookTrigger, 'onSuccess').callsFake(() => Promise.resolve()); sinon.stub(webhookTrigger, 'onError').callsFake(() => Promise.resolve()); @@ -53,6 +59,29 @@ describe('Webhook Service', function () { assert.equal(request.called, false); }); + it('does not trigger payload handler when there are hooks registered for an event, but the custom integrations limit is active', async function () { + const webhookModel = { + get: sinon.stub() + }; + + webhookModel.get + .withArgs('event').returns(WEBHOOK_EVENT) + .withArgs('target_url').returns(WEBHOOK_TARGET_URL); + + models.Webhook.findAllByEvent + .withArgs(WEBHOOK_EVENT, {context: {internal: true}}) + .resolves({models: [webhookModel]}); + + limitService.isLimited.withArgs('customIntegrations').returns(true); + limitService.checkWouldGoOverLimit.withArgs('customIntegrations').resolves(true); + + await webhookTrigger.trigger(WEBHOOK_EVENT); + + assert.equal(models.Webhook.findAllByEvent.called, false); + assert.equal(payload.called, false); + assert.equal(request.called, false); + }); + it('triggers payload handler when there are hooks registered for an event', async function () { const webhookModel = { get: sinon.stub() diff --git a/ghost/core/test/utils/e2e-framework-mock-manager.js b/ghost/core/test/utils/e2e-framework-mock-manager.js index 3d5eec4021..8bbba5d301 100644 --- a/ghost/core/test/utils/e2e-framework-mock-manager.js +++ b/ghost/core/test/utils/e2e-framework-mock-manager.js @@ -19,6 +19,7 @@ const originalMailServiceSendMail = mailService.GhostMailer.prototype.sendMail; const labs = require('../../core/shared/labs'); const events = require('../../core/server/lib/common/events'); const settingsCache = require('../../core/shared/settings-cache'); +const limitService = require('../../core/server/services/limits'); const dns = require('dns'); const dnsPromises = dns.promises; const StripeMocker = require('./stripe-mocker'); @@ -283,6 +284,15 @@ const mockLabsDisabled = (flag, alpha = true) => { fakedLabsFlags[flag] = false; }; +const mockLimitService = (limit, options) => { + if (!mocks.limitService) { + mocks.limitService = sinon.stub(limitService); + } + + mocks.limitService.isLimited.withArgs(limit).returns(options.isLimited); + mocks.limitService.checkWouldGoOverLimit.withArgs(limit).resolves(options.wouldGoOverLimit); +}; + const restore = () => { // eslint-disable-next-line no-console configUtils.restore().catch(console.error); @@ -319,6 +329,7 @@ module.exports = { mockLabsDisabled, mockWebhookRequests, mockSetting, + mockLimitService, disableNetwork, restore, stripeMocker,