From 5fbd7971e04640be3f877b3aa22d4aee61c1d40a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Iv=C3=A1n=20Ovejero?= <ivov.src@gmail.com>
Date: Tue, 16 Jan 2024 09:17:41 +0100
Subject: [PATCH] fix(core): Account for immediate confirmation request during
 test webhook creation (#8329)

---
 packages/cli/src/TestWebhooks.ts            | 25 ++++++++++++++-------
 packages/cli/test/unit/TestWebhooks.test.ts | 11 +++++++--
 2 files changed, 26 insertions(+), 10 deletions(-)

diff --git a/packages/cli/src/TestWebhooks.ts b/packages/cli/src/TestWebhooks.ts
index 82d2d9e7efa33..e46be34dde82c 100644
--- a/packages/cli/src/TestWebhooks.ts
+++ b/packages/cli/src/TestWebhooks.ts
@@ -23,6 +23,7 @@ import { WorkflowMissingIdError } from '@/errors/workflow-missing-id.error';
 import { WebhookNotFoundError } from '@/errors/response-errors/webhook-not-found.error';
 import * as NodeExecuteFunctions from 'n8n-core';
 import { removeTrailingSlash } from './utils';
+import type { TestWebhookRegistration } from '@/services/test-webhook-registrations.service';
 import { TestWebhookRegistrationsService } from '@/services/test-webhook-registrations.service';
 import { MultiMainSetup } from './services/orchestration/main/MultiMainSetup.ee';
 import * as WorkflowExecuteAdditionalData from '@/WorkflowExecuteAdditionalData';
@@ -232,13 +233,13 @@ export class TestWebhooks implements IWebhookManager {
 
 		for (const webhook of webhooks) {
 			const key = this.registrations.toKey(webhook);
-			const registration = await this.registrations.get(key);
+			const isAlreadyRegistered = await this.registrations.get(key);
 
 			if (runData && webhook.node in runData) {
 				return false;
 			}
 
-			if (registration && !webhook.webhookId) {
+			if (isAlreadyRegistered && !webhook.webhookId) {
 				throw new WebhookPathTakenError(webhook.node);
 			}
 
@@ -253,17 +254,25 @@ export class TestWebhooks implements IWebhookManager {
 
 			cacheableWebhook.userId = userId;
 
+			const registration: TestWebhookRegistration = {
+				sessionId,
+				workflowEntity,
+				destinationNode,
+				webhook: cacheableWebhook as IWebhookData,
+			};
+
 			try {
+				/**
+				 * Register the test webhook _before_ creation at third-party service
+				 * in case service sends a confirmation request immediately on creation.
+				 */
+				await this.registrations.register(registration);
+
 				await workflow.createWebhookIfNotExists(webhook, NodeExecuteFunctions, 'manual', 'manual');
 
 				cacheableWebhook.staticData = workflow.staticData;
 
-				await this.registrations.register({
-					sessionId,
-					workflowEntity,
-					destinationNode,
-					webhook: cacheableWebhook as IWebhookData,
-				});
+				await this.registrations.register(registration);
 
 				this.timeouts[key] = timeout;
 			} catch (error) {
diff --git a/packages/cli/test/unit/TestWebhooks.test.ts b/packages/cli/test/unit/TestWebhooks.test.ts
index 6d2a561f106d7..3f53dc006b156 100644
--- a/packages/cli/test/unit/TestWebhooks.test.ts
+++ b/packages/cli/test/unit/TestWebhooks.test.ts
@@ -8,7 +8,7 @@ import * as WebhookHelpers from '@/WebhookHelpers';
 import type * as express from 'express';
 
 import type { IWorkflowDb, WebhookRequest } from '@/Interfaces';
-import type { IWebhookData, IWorkflowExecuteAdditionalData } from 'n8n-workflow';
+import type { IWebhookData, IWorkflowExecuteAdditionalData, Workflow } from 'n8n-workflow';
 import type {
 	TestWebhookRegistrationsService,
 	TestWebhookRegistration,
@@ -50,11 +50,18 @@ describe('TestWebhooks', () => {
 			mock<IWorkflowExecuteAdditionalData>(),
 		];
 
-		test('if webhook is needed, should return true and activate webhook', async () => {
+		test('if webhook is needed, should register then create webhook and return true', async () => {
+			const workflow = mock<Workflow>();
+
+			jest.spyOn(testWebhooks, 'toWorkflow').mockReturnValueOnce(workflow);
 			jest.spyOn(WebhookHelpers, 'getWorkflowWebhooks').mockReturnValue([webhook]);
 
 			const needsWebhook = await testWebhooks.needsWebhook(...args);
 
+			const [registerOrder] = registrations.register.mock.invocationCallOrder;
+			const [createOrder] = workflow.createWebhookIfNotExists.mock.invocationCallOrder;
+
+			expect(registerOrder).toBeLessThan(createOrder);
 			expect(needsWebhook).toBe(true);
 		});