Infrastructure/ghost
0
Fork 0
mirror of https://github.com/TryGhost/Ghost.git synced 2025-01-20 22:42:53 -05:00
Code Issues Activity

Refactored webhook restriction to WebhookTrigger

Browse source 
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.
This commit is contained in:
Fabien O'Carroll 2024-11-20 12:12:23 +07:00
parent d41ccdde8d
commit 263c493e2b
5 changed files with 99 additions and 17 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

19
ghost/core/core/server/services/webhooks/WebhookTrigger.js
View file

@ -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}});

14
ghost/core/core/server/services/webhooks/listen.js
View file

@ -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

39
ghost/core/test/e2e-webhooks/site.test.js
View file

@ -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.');
}
});
});

33
ghost/core/test/unit/server/services/webhooks/trigger.test.js
View file

@ -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()

11
ghost/core/test/utils/e2e-framework-mock-manager.js
View file

@ -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,