From 0919ae0d057e876a9963258696dbd328a65eed9b Mon Sep 17 00:00:00 2001 From: Fabien O'Carroll Date: Wed, 20 Nov 2024 12:12:23 +0700 Subject: [PATCH] Refactored webhook restriction to WebhookTrigger ref https://linear.app/ghost/issue/AP-598 This is not a change in functionality. The ActivityPub integration is an `internal` integration, because we want it to be available regardless of the plan a Ghost(Pro) site is on. However the webhooks service is not able to differentiate between webhooks for a custom integration and an internal one. Rather than disable webhooks entirely when the custom integrations limit active, we want to allow webhooks for internal integrations to work. The first step towards that is keeping the listener for the model events and have the limiting happen in the WebhookTrigger which allows us to be more specific as to which webhooks should be triggered or not. --- .../services/webhooks/WebhookTrigger.js | 19 ++++++++- .../core/server/services/webhooks/listen.js | 14 +------ ghost/core/test/e2e-webhooks/site.test.js | 39 +++++++++++++++++++ .../server/services/webhooks/trigger.test.js | 33 +++++++++++++++- .../test/utils/e2e-framework-mock-manager.js | 11 ++++++ 5 files changed, 99 insertions(+), 17 deletions(-) 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,