From 3265e257b7575595df50835045108ca3c6f9554d Mon Sep 17 00:00:00 2001 From: Fabien O'Carroll Date: Wed, 20 Nov 2024 17:54:30 +0700 Subject: [PATCH] Fixed webhooks not firing for internal integrations ref https://linear.app/ghost/issue/AP-598 We want to make sure that webhooks for internal integrations are fired even when the custom integrations limit has been hit. In order to test this we've had to update the fixtureManager to allow passing the integration type when inserting fixture webhooks into the db. --- .../services/webhooks/WebhookTrigger.js | 13 +++++-- .../__snapshots__/site.test.js.snap | 12 +++++++ ghost/core/test/e2e-webhooks/site.test.js | 36 +++++++++++++++++++ .../server/services/webhooks/trigger.test.js | 10 ++++-- ghost/core/test/utils/e2e-framework.js | 4 ++- ghost/core/test/utils/fixture-utils.js | 10 ++++-- 6 files changed, 77 insertions(+), 8 deletions(-) diff --git a/ghost/core/core/server/services/webhooks/WebhookTrigger.js b/ghost/core/core/server/services/webhooks/WebhookTrigger.js index e88545f541f4..a12d98e98a01 100644 --- a/ghost/core/core/server/services/webhooks/WebhookTrigger.js +++ b/ghost/core/core/server/services/webhooks/WebhookTrigger.js @@ -27,9 +27,18 @@ class WebhookTrigger { const overLimit = await this.limitService.checkWouldGoOverLimit('customIntegrations'); if (overLimit) { - logging.info(`Skipping all webhooks for event ${event}. The "customIntegrations" plan limit is enabled.`); + logging.info(`Skipping all non-internal webhooks for event ${event}. The "customIntegrations" plan limit is enabled.`); + const result = await this.models + .Webhook + .findAllByEvent(event, { + context: {internal: true}, + withRelated: ['integration'] + }); + return { - models: [] + models: result?.models?.filter((model) => { + return model.related('integration')?.get('type') === 'internal'; + }) || [] }; } } diff --git a/ghost/core/test/e2e-webhooks/__snapshots__/site.test.js.snap b/ghost/core/test/e2e-webhooks/__snapshots__/site.test.js.snap index 7ececc9f6509..67dbdaceb844 100644 --- a/ghost/core/test/e2e-webhooks/__snapshots__/site.test.js.snap +++ b/ghost/core/test/e2e-webhooks/__snapshots__/site.test.js.snap @@ -11,3 +11,15 @@ Object { `; exports[`site.* events site.changed event is triggered 2: [body] 1`] = `Object {}`; + +exports[`site.* events site.changed event is triggered, custom integrations are limited but we have an internal webhook 1: [headers] 1`] = ` +Object { + "accept-encoding": "gzip, deflate, br", + "content-length": Any, + "content-type": "application/json", + "content-version": StringMatching /v\\\\d\\+\\\\\\.\\\\d\\+/, + "user-agent": StringMatching /Ghost\\\\/\\\\d\\+\\\\\\.\\\\d\\+\\\\\\.\\\\d\\+\\\\s\\\\\\(https:\\\\/\\\\/github\\.com\\\\/TryGhost\\\\/Ghost\\\\\\)/, +} +`; + +exports[`site.* events site.changed event is triggered, custom integrations are limited but we have an internal webhook 2: [body] 1`] = `Object {}`; diff --git a/ghost/core/test/e2e-webhooks/site.test.js b/ghost/core/test/e2e-webhooks/site.test.js index 0d1158352d07..0472ba9e00a7 100644 --- a/ghost/core/test/e2e-webhooks/site.test.js +++ b/ghost/core/test/e2e-webhooks/site.test.js @@ -87,4 +87,40 @@ describe('site.* events', function () { throw new Error('The webhook should not have been sent.'); } }); + + it('site.changed event is triggered, custom integrations are limited but we have an internal webhook', async function () { + const webhookURL = 'https://test-webhook-receiver.com/site-changed'; + await webhookMockReceiver.mock(webhookURL); + await fixtureManager.insertWebhook({ + event: 'site.changed', + url: webhookURL, + integrationType: 'internal' + }); + + 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); + + await webhookMockReceiver.receivedRequest(); + + webhookMockReceiver + .matchHeaderSnapshot({ + 'content-version': anyContentVersion, + 'content-length': anyNumber, + 'user-agent': anyGhostAgent + }) + .matchBodySnapshot(); + }); }); 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 f6146b368d84..776b2969f931 100644 --- a/ghost/core/test/unit/server/services/webhooks/trigger.test.js +++ b/ghost/core/test/unit/server/services/webhooks/trigger.test.js @@ -61,15 +61,19 @@ describe('Webhook Service', function () { 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() + get: sinon.stub(), + related: sinon.stub() }; webhookModel.get .withArgs('event').returns(WEBHOOK_EVENT) .withArgs('target_url').returns(WEBHOOK_TARGET_URL); + webhookModel.related + .withArgs('integration').returns(null); + models.Webhook.findAllByEvent - .withArgs(WEBHOOK_EVENT, {context: {internal: true}}) + .withArgs(WEBHOOK_EVENT, {context: {internal: true}, withRelated: ['integration']}) .resolves({models: [webhookModel]}); limitService.isLimited.withArgs('customIntegrations').returns(true); @@ -77,7 +81,7 @@ describe('Webhook Service', function () { await webhookTrigger.trigger(WEBHOOK_EVENT); - assert.equal(models.Webhook.findAllByEvent.called, false); + assert.equal(models.Webhook.findAllByEvent.called, true); assert.equal(payload.called, false); assert.equal(request.called, false); }); diff --git a/ghost/core/test/utils/e2e-framework.js b/ghost/core/test/utils/e2e-framework.js index bcf727ae4845..6b2efae1d309 100644 --- a/ghost/core/test/utils/e2e-framework.js +++ b/ghost/core/test/utils/e2e-framework.js @@ -409,10 +409,12 @@ const getAgentsWithFrontend = async () => { }; }; -const insertWebhook = ({event, url}) => { +const insertWebhook = ({event, url, integrationType = undefined}) => { return fixtureUtils.fixtures.insertWebhook({ event: event, target_url: url + }, { + integrationType }); }; diff --git a/ghost/core/test/utils/fixture-utils.js b/ghost/core/test/utils/fixture-utils.js index 9f4e3f5b5607..b154b3558c07 100644 --- a/ghost/core/test/utils/fixture-utils.js +++ b/ghost/core/test/utils/fixture-utils.js @@ -426,10 +426,16 @@ const fixtures = { })); }, - insertWebhook: function (attributes) { + insertWebhook: function (attributes, options = {}) { + let integration = DataGenerator.forKnex.integrations[0]; + if (options.integrationType) { + integration = DataGenerator.forKnex.integrations.find( + props => props.type === options.integrationType + ); + } const webhook = DataGenerator.forKnex.createWebhook( Object.assign(attributes, { - integration_id: DataGenerator.forKnex.integrations[0].id + integration_id: integration.id }) );