From 261389307bf0f1eef2155c4c00a6d745cbefd8db Mon Sep 17 00:00:00 2001 From: Alessandro Magionami Date: Tue, 10 Sep 2024 14:55:52 +0200 Subject: [PATCH] chore(webhooks): refactor delete webhook multiregion --- .../modules/webhooks/domain/operations.ts | 2 + .../webhooks/graph/resolvers/webhooks-new.ts | 20 ++++- .../webhooks/graph/resolvers/webhooks.js | 23 ------ .../modules/webhooks/repositories/webhooks.ts | 7 ++ .../modules/webhooks/services/webhooks-new.ts | 23 ++++++ .../modules/webhooks/services/webhooks.js | 4 - .../modules/webhooks/tests/webhooks.spec.js | 81 ++++++++++++++++--- 7 files changed, 120 insertions(+), 40 deletions(-) diff --git a/packages/server/modules/webhooks/domain/operations.ts b/packages/server/modules/webhooks/domain/operations.ts index 210af25b2..a1f10be64 100644 --- a/packages/server/modules/webhooks/domain/operations.ts +++ b/packages/server/modules/webhooks/domain/operations.ts @@ -26,3 +26,5 @@ export type UpdateWebhook = ({ > > }) => Promise + +export type DeleteWebhook = ({ id }: Pick) => Promise diff --git a/packages/server/modules/webhooks/graph/resolvers/webhooks-new.ts b/packages/server/modules/webhooks/graph/resolvers/webhooks-new.ts index a3b7f7b97..271167942 100644 --- a/packages/server/modules/webhooks/graph/resolvers/webhooks-new.ts +++ b/packages/server/modules/webhooks/graph/resolvers/webhooks-new.ts @@ -1,10 +1,15 @@ import { Resolvers } from '@/modules/core/graph/generated/graphql' import { authorizeResolver } from '@/modules/shared' -import { createWebhook, updateWebhook } from '@/modules/webhooks/services/webhooks-new' +import { + createWebhook, + deleteWebhook, + updateWebhook +} from '@/modules/webhooks/services/webhooks-new' import { Roles } from '@speckle/shared' import { countWebhooksByStreamIdFactory, createWebhookFactory, + deleteWebhookFactory, getWebhookByIdFactory, updateWebhookFactory } from '@/modules/webhooks/repositories/webhooks' @@ -61,6 +66,19 @@ export = { }) return updated + }, + webhookDelete: async (_parent, args, context) => { + await authorizeResolver( + context.userId, + args.webhook.streamId, + Roles.Stream.Owner, + context.resourceAccessRules + ) + + return await deleteWebhook({ + deleteWebhookConfig: deleteWebhookFactory({ db }), + getWebhookById: getWebhookByIdFactory({ db }) + })(args.webhook) } } } as Resolvers diff --git a/packages/server/modules/webhooks/graph/resolvers/webhooks.js b/packages/server/modules/webhooks/graph/resolvers/webhooks.js index 3bbe8869f..fba6d6417 100644 --- a/packages/server/modules/webhooks/graph/resolvers/webhooks.js +++ b/packages/server/modules/webhooks/graph/resolvers/webhooks.js @@ -1,6 +1,5 @@ const { authorizeResolver } = require('@/modules/shared') const { - deleteWebhook, getStreamWebhooks, getLastWebhookEvents, getWebhookEventsCount @@ -8,7 +7,6 @@ const { const { Roles } = require('@speckle/shared') const { getWebhookByIdFactory } = require('../../repositories/webhooks') const { db } = require('@/db/knex') -const { ForbiddenError } = require('@/modules/shared/errors') const streamWebhooksResolver = async (parent, args, context) => { await authorizeResolver( @@ -49,26 +47,5 @@ module.exports = { return { items, totalCount } } - }, - - Mutation: { - async webhookDelete(parent, args, context) { - await authorizeResolver( - context.userId, - args.webhook.streamId, - Roles.Stream.Owner, - context.resourceAccessRules - ) - - const wh = await getWebhookByIdFactory({ db })({ id: args.webhook.id }) - if (args.webhook.streamId !== wh.streamId) - throw new ForbiddenError( - 'The webhook id and stream id do not match. Please check your inputs.' - ) - - const deleted = await deleteWebhook({ id: args.webhook.id }) - - return !!deleted - } } } diff --git a/packages/server/modules/webhooks/repositories/webhooks.ts b/packages/server/modules/webhooks/repositories/webhooks.ts index 436d1ee97..f9fbaff75 100644 --- a/packages/server/modules/webhooks/repositories/webhooks.ts +++ b/packages/server/modules/webhooks/repositories/webhooks.ts @@ -3,6 +3,7 @@ import { Webhook } from '@/modules/webhooks/domain/types' import { CountWebhooksByStreamId, CreateWebhook, + DeleteWebhook, GetWebhookById, UpdateWebhook } from '@/modules/webhooks/domain/operations' @@ -72,3 +73,9 @@ export const updateWebhookFactory = return webhookId } + +export const deleteWebhookFactory = + ({ db }: { db: Knex }): DeleteWebhook => + async ({ id }) => { + return await tables(db).webhooksConfigs.where({ id }).del() + } diff --git a/packages/server/modules/webhooks/services/webhooks-new.ts b/packages/server/modules/webhooks/services/webhooks-new.ts index 8e13bca5b..7dc19d694 100644 --- a/packages/server/modules/webhooks/services/webhooks-new.ts +++ b/packages/server/modules/webhooks/services/webhooks-new.ts @@ -1,6 +1,9 @@ +import { ForbiddenError } from '@/modules/shared/errors' import { CountWebhooksByStreamId, CreateWebhook, + DeleteWebhook, + GetWebhookById, UpdateWebhook } from '@/modules/webhooks/domain/operations' import { Webhook } from '@/modules/webhooks/domain/types' @@ -64,3 +67,23 @@ export const updateWebhook = } }) } + +export const deleteWebhook = + ({ + deleteWebhookConfig, + getWebhookById + }: { + deleteWebhookConfig: DeleteWebhook + getWebhookById: GetWebhookById + }) => + async ({ id, streamId }: { id: string; streamId: string }) => { + const wh = await getWebhookById({ id }) + if (streamId !== wh?.streamId) + throw new ForbiddenError( + 'The webhook id and stream id do not match. Please check your inputs.' + ) + + await deleteWebhookConfig({ id }) + + return id + } diff --git a/packages/server/modules/webhooks/services/webhooks.js b/packages/server/modules/webhooks/services/webhooks.js index 7393e0d87..a1719bb7b 100644 --- a/packages/server/modules/webhooks/services/webhooks.js +++ b/packages/server/modules/webhooks/services/webhooks.js @@ -11,10 +11,6 @@ const Users = () => knex('users') const { getServerInfo } = require('../../core/services/generic') module.exports = { - async deleteWebhook({ id }) { - return await WebhooksConfig().where({ id }).del() - }, - async getStreamWebhooks({ streamId }) { const webhooks = await WebhooksConfig() .select('*') diff --git a/packages/server/modules/webhooks/tests/webhooks.spec.js b/packages/server/modules/webhooks/tests/webhooks.spec.js index 0e44ec48b..5c7044d6a 100644 --- a/packages/server/modules/webhooks/tests/webhooks.spec.js +++ b/packages/server/modules/webhooks/tests/webhooks.spec.js @@ -12,7 +12,6 @@ const { createPersonalAccessToken } = require('../../core/services/tokens') const { getStreamWebhooks, getLastWebhookEvents, - deleteWebhook, dispatchStreamEvent } = require('../services/webhooks') const { createUser } = require('../../core/services/users') @@ -22,12 +21,14 @@ const { createWebhookFactory, countWebhooksByStreamIdFactory, getWebhookByIdFactory, - updateWebhookFactory + updateWebhookFactory, + deleteWebhookFactory } = require('@/modules/webhooks/repositories/webhooks') const { db } = require('@/db/knex') const { createWebhook, - updateWebhook: updateWebhookService + updateWebhook: updateWebhookService, + deleteWebhook } = require('@/modules/webhooks/services/webhooks-new') const { Users, Streams } = require('@/modules/core/dbSchema') @@ -113,9 +114,31 @@ describe('Webhooks @webhooks', () => { }) it('Should delete a webhook', async () => { - await deleteWebhook({ id: webhookOne.id }) - const webhook = await getWebhook({ id: webhookOne.id }) - expect(webhook).to.be.null + const stream = { + name: 'test stream', + description: 'stream', + isPublic: true, + ownerId: userOne.id + } + const streamId = await createStream(stream) + const webhook = { + streamId, + url: 'http://localhost:42/non-existent', + description: 'test wh', + secret: 'secret', + enabled: true, + triggers: ['commit_create', 'commit_update'] + } + webhook.id = await createWebhook({ + createWebhookConfig: createWebhookFactory({ db }), + countWebhooksByStreamId: countWebhooksByStreamIdFactory({ db }) + })(webhook) + await deleteWebhook({ + deleteWebhookConfig: deleteWebhookFactory({ db }), + getWebhookById: getWebhookByIdFactory({ db }) + })(webhook) + const webhookDeleted = await getWebhookByIdFactory({ db })({ id: webhook.id }) + expect(webhookDeleted).to.be.null }) it('Should get webhooks for stream', async () => { @@ -298,11 +321,30 @@ describe('Webhooks @webhooks', () => { }) it('Should delete a webhook', async () => { - const res = await sendRequest(userTwo.token, { - query: `mutation { webhookDelete(webhook: { id: "${webhookTwo.id}", streamId: "${streamTwo.id}" } ) }` + const stream = { + name: 'test stream', + description: 'stream', + isPublic: true, + ownerId: userOne.id + } + const streamId = await createStream(stream) + const webhook = { + streamId, + url: 'http://localhost:42/non-existent', + description: 'test wh', + secret: 'secret', + enabled: true, + triggers: ['commit_create', 'commit_update'] + } + webhook.id = await createWebhook({ + createWebhookConfig: createWebhookFactory({ db }), + countWebhooksByStreamId: countWebhooksByStreamIdFactory({ db }) + })(webhook) + const res = await sendRequest(userOne.token, { + query: `mutation { webhookDelete(webhook: { id: "${webhook.id}", streamId: "${streamId}" } ) }` }) expect(noErrors(res)) - expect(res.body.data.webhookDelete).to.equal('true') + expect(res.body.data.webhookDelete).to.equal(webhook.id) }) it('Should *not* create a webhook if user is not a stream owner', async () => { @@ -328,18 +370,33 @@ describe('Webhooks @webhooks', () => { it('Should have a webhook limit for streams', async () => { const limit = 100 - for (let i = 0; i < limit - 1; i++) { + const stream = { + name: 'test stream', + description: 'stream', + isPublic: true, + ownerId: userOne.id + } + const streamId = await createStream(stream) + const webhook = { + streamId, + url: 'http://localhost:42/non-existent', + description: 'test wh', + secret: 'secret', + enabled: true, + triggers: ['commit_create', 'commit_update'] + } + for (let i = 0; i < limit; i++) { await createWebhook({ createWebhookConfig: createWebhookFactory({ db }), countWebhooksByStreamId: countWebhooksByStreamIdFactory({ db }) - })(webhookOne) + })(webhook) } try { await createWebhook({ createWebhookConfig: createWebhookFactory({ db }), countWebhooksByStreamId: countWebhooksByStreamIdFactory({ db }) - })(webhookOne) + })(webhook) } catch (err) { if (err.toString().indexOf('Maximum') > -1) return }