refactor(core): Make external hooks type-safe, and add tests (#12893)

packages/@n8n/config/src/configs/external-hooks.config.ts

@@ -0,0 +1,17 @@
+import { Config, Env } from '../decorators';
+
+class ColonSeperatedStringArray<T extends string = string> extends Array<T> {
+	constructor(str: string) {
+		super();
+		const parsed = str.split(':') as this;
+		const filtered = parsed.filter((i) => typeof i === 'string' && i.length);
+		return filtered.length ? filtered : [];
+	}
+}
+
+@Config
+export class ExternalHooksConfig {
+	/** Files containing external hooks. Multiple files can be separated by colon (":") */
+	@Env('EXTERNAL_HOOK_FILES')
+	files: ColonSeperatedStringArray = [];
+}

packages/@n8n/config/src/index.ts

@@ -6,6 +6,7 @@ import { DiagnosticsConfig } from './configs/diagnostics.config';
 import { EndpointsConfig } from './configs/endpoints.config';
 import { EventBusConfig } from './configs/event-bus.config';
 import { ExecutionsConfig } from './configs/executions.config';
+import { ExternalHooksConfig } from './configs/external-hooks.config';
 import { ExternalSecretsConfig } from './configs/external-secrets.config';
 import { ExternalStorageConfig } from './configs/external-storage.config';
 import { GenericConfig } from './configs/generic.config';
@@ -51,6 +52,9 @@ export class GlobalConfig {
 	@Nested
 	publicApi: PublicApiConfig;
 
+	@Nested
+	externalHooks: ExternalHooksConfig;
+
 	@Nested
 	externalSecrets: ExternalSecretsConfig;
 

packages/@n8n/config/test/config.test.ts

@@ -107,6 +107,9 @@ describe('GlobalConfig', () => {
 				maxFileSizeInKB: 10240,
 			},
 		},
+		externalHooks: {
+			files: [],
+		},
 		externalSecrets: {
 			preferGet: false,
 			updateInterval: 300,

packages/cli/src/__tests__/external-hooks.test.ts

@@ -0,0 +1,125 @@
+import type { GlobalConfig } from '@n8n/config';
+import { mock } from 'jest-mock-extended';
+import type { ErrorReporter, Logger } from 'n8n-core';
+import type { IWorkflowBase } from 'n8n-workflow';
+import { ApplicationError } from 'n8n-workflow';
+
+import type { CredentialsRepository } from '@/databases/repositories/credentials.repository';
+import type { SettingsRepository } from '@/databases/repositories/settings.repository';
+import type { UserRepository } from '@/databases/repositories/user.repository';
+import type { WorkflowRepository } from '@/databases/repositories/workflow.repository';
+import { ExternalHooks } from '@/external-hooks';
+
+describe('ExternalHooks', () => {
+	const logger = mock<Logger>();
+	const errorReporter = mock<ErrorReporter>();
+	const globalConfig = mock<GlobalConfig>();
+	const userRepository = mock<UserRepository>();
+	const settingsRepository = mock<SettingsRepository>();
+	const credentialsRepository = mock<CredentialsRepository>();
+	const workflowRepository = mock<WorkflowRepository>();
+
+	const workflowData = mock<IWorkflowBase>({ id: '123', name: 'Test Workflow' });
+	const hookFn = jest.fn();
+
+	let externalHooks: ExternalHooks;
+
+	beforeEach(() => {
+		jest.resetAllMocks();
+		globalConfig.externalHooks.files = [];
+		externalHooks = new ExternalHooks(
+			logger,
+			errorReporter,
+			globalConfig,
+			userRepository,
+			settingsRepository,
+			credentialsRepository,
+			workflowRepository,
+		);
+	});
+
+	describe('init()', () => {
+		it('should not load hooks if no external hook files are configured', async () => {
+			// @ts-expect-error private method
+			const loadHooksSpy = jest.spyOn(externalHooks, 'loadHooks');
+			await externalHooks.init();
+			expect(loadHooksSpy).not.toHaveBeenCalled();
+		});
+
+		it('should throw an error if hook file cannot be loaded', async () => {
+			globalConfig.externalHooks.files = ['/path/to/non-existent-hook.js'];
+
+			jest.mock(
+				'/path/to/non-existent-hook.js',
+				() => {
+					throw new Error('File not found');
+				},
+				{ virtual: true },
+			);
+
+			await expect(externalHooks.init()).rejects.toThrow(ApplicationError);
+		});
+
+		it('should successfully load hooks from valid hook file', async () => {
+			const mockHookFile = {
+				workflow: {
+					create: [hookFn],
+				},
+			};
+
+			globalConfig.externalHooks.files = ['/path/to/valid-hook.js'];
+			jest.mock('/path/to/valid-hook.js', () => mockHookFile, { virtual: true });
+
+			await externalHooks.init();
+
+			// eslint-disable-next-line @typescript-eslint/dot-notation
+			expect(externalHooks['registered']['workflow.create']).toHaveLength(1);
+
+			await externalHooks.run('workflow.create', [workflowData]);
+
+			expect(hookFn).toHaveBeenCalledTimes(1);
+			expect(hookFn).toHaveBeenCalledWith(workflowData);
+		});
+	});
+
+	describe('run()', () => {
+		it('should not throw if no hooks are registered', async () => {
+			await externalHooks.run('n8n.stop');
+		});
+
+		it('should execute registered hooks', async () => {
+			// eslint-disable-next-line @typescript-eslint/dot-notation
+			externalHooks['registered']['workflow.create'] = [hookFn];
+
+			await externalHooks.run('workflow.create', [workflowData]);
+
+			expect(hookFn).toHaveBeenCalledTimes(1);
+
+			const hookInvocationContext = hookFn.mock.instances[0];
+			expect(hookInvocationContext).toHaveProperty('dbCollections');
+			expect(hookInvocationContext.dbCollections).toEqual({
+				User: userRepository,
+				Settings: settingsRepository,
+				Credentials: credentialsRepository,
+				Workflow: workflowRepository,
+			});
+		});
+
+		it('should report error if hook execution fails', async () => {
+			hookFn.mockRejectedValueOnce(new Error('Hook failed'));
+			// eslint-disable-next-line @typescript-eslint/dot-notation
+			externalHooks['registered']['workflow.create'] = [hookFn];
+
+			await expect(externalHooks.run('workflow.create', [workflowData])).rejects.toThrow(
+				ApplicationError,
+			);
+
+			expect(errorReporter.error).toHaveBeenCalledWith(expect.any(ApplicationError), {
+				level: 'fatal',
+			});
+			expect(logger.error).toHaveBeenCalledWith(
+				'There was a problem running hook "workflow.create"',
+			);
+		});
+	});
+});

packages/cli/src/active-workflow-manager.ts

@@ -89,7 +89,7 @@ export class ActiveWorkflowManager {
 
 		await this.addActiveWorkflows('init');
 
-		await this.externalHooks.run('activeWorkflows.initialized', []);
+		await this.externalHooks.run('activeWorkflows.initialized');
 		await this.webhookService.populateCache();
 	}
 

packages/cli/src/commands/start.ts

@@ -94,7 +94,7 @@ export class Start extends BaseCommand {
 
 			Container.get(WaitTracker).stopTracking();
 
-			await this.externalHooks?.run('n8n.stop', []);
+			await this.externalHooks?.run('n8n.stop');
 
 			await this.activeWorkflowManager.removeAllTriggerAndPollerBasedWorkflows();
 

packages/cli/src/commands/webhook.ts

@@ -33,7 +33,7 @@ export class Webhook extends BaseCommand {
 		this.logger.info('\nStopping n8n...');
 
 		try {
-			await this.externalHooks?.run('n8n.stop', []);
+			await this.externalHooks?.run('n8n.stop');
 
 			await Container.get(ActiveExecutions).shutdown();
 		} catch (error) {

packages/cli/src/commands/worker.ts

@@ -49,7 +49,7 @@ export class Worker extends BaseCommand {
 		this.logger.info('Stopping worker...');
 
 		try {
-			await this.externalHooks?.run('n8n.stop', []);
+			await this.externalHooks?.run('n8n.stop');
 		} catch (error) {
 			await this.exitWithCrash('Error shutting down worker', error);
 		}

packages/cli/src/config/index.ts

@@ -133,3 +133,5 @@ setGlobalState({
 
 // eslint-disable-next-line import/no-default-export
 export default config;
+
+export type Config = typeof config;

packages/cli/src/config/schema.ts

@@ -189,13 +189,6 @@ export const schema = {
 		env: 'EXTERNAL_FRONTEND_HOOKS_URLS',
 	},
 
-	externalHookFiles: {
-		doc: 'Files containing external hooks. Multiple files can be separated by colon (":")',
-		format: String,
-		default: '',
-		env: 'EXTERNAL_HOOK_FILES',
-	},
-
 	push: {
 		backend: {
 			format: ['sse', 'websocket'] as const,

packages/cli/src/execution-lifecycle/execution-lifecycle-hooks.ts

@@ -436,22 +436,13 @@ function hookFunctionsSaveWorker(): IWorkflowExecuteHooks {
 			},
 			async function (this: WorkflowHooks, fullRunData: IRun) {
 				const externalHooks = Container.get(ExternalHooks);
-				if (externalHooks.exists('workflow.postExecute')) {
+				try {
-					try {
+					await externalHooks.run('workflow.postExecute', [
-						await externalHooks.run('workflow.postExecute', [
+						fullRunData,
-							fullRunData,
+						this.workflowData,
-							this.workflowData,
+						this.executionId,
-							this.executionId,
+					]);
-						]);
+				} catch {}
-					} catch (error) {
-						Container.get(ErrorReporter).error(error);
-						Container.get(Logger).error(
-							'There was a problem running hook "workflow.postExecute"',
-							// eslint-disable-next-line @typescript-eslint/no-unsafe-argument
-							error,
-						);
-					}
-				}
 			},
 		],
 		nodeFetchedData: [

packages/cli/src/external-hooks.ts

@@ -1,27 +1,104 @@
-/* eslint-disable @typescript-eslint/no-var-requires */
+import type { FrontendSettings, UserUpdateRequestDto } from '@n8n/api-types';
+import type { ClientOAuth2Options } from '@n8n/client-oauth2';
+import { GlobalConfig } from '@n8n/config';
 import { Service } from '@n8n/di';
-import { ErrorReporter } from 'n8n-core';
+import { ErrorReporter, Logger } from 'n8n-core';
+import type { IRun, IWorkflowBase, Workflow, WorkflowExecuteMode } from 'n8n-workflow';
 import { ApplicationError } from 'n8n-workflow';
+import type clientOAuth1 from 'oauth-1.0a';
 
-import config from '@/config';
+import type { AbstractServer } from '@/abstract-server';
+import type { Config } from '@/config';
+import type { TagEntity } from '@/databases/entities/tag-entity';
+import type { User } from '@/databases/entities/user';
 import { CredentialsRepository } from '@/databases/repositories/credentials.repository';
 import { SettingsRepository } from '@/databases/repositories/settings.repository';
 import { UserRepository } from '@/databases/repositories/user.repository';
 import { WorkflowRepository } from '@/databases/repositories/workflow.repository';
-import type { IExternalHooksFileData, IExternalHooksFunctions } from '@/interfaces';
+import type { ICredentialsDb, PublicUser } from '@/interfaces';
+
+type Repositories = {
+	User: UserRepository;
+	Settings: SettingsRepository;
+	Credentials: CredentialsRepository;
+	Workflow: WorkflowRepository;
+};
+
+type Hooks = {
+	'n8n.ready': [server: AbstractServer, config: Config];
+	'n8n.stop': [];
+	'worker.ready': [];
+
+	'activeWorkflows.initialized': [];
+
+	'credentials.create': [encryptedData: ICredentialsDb];
+	'credentials.update': [newCredentialData: ICredentialsDb];
+	'credentials.delete': [credentialId: string];
+
+	'frontend.settings': [frontendSettings: FrontendSettings];
+
+	'mfa.beforeSetup': [user: User];
+
+	'oauth1.authenticate': [
+		oAuthOptions: clientOAuth1.Options,
+		oauthRequestData: { oauth_callback: string },
+	];
+	'oauth2.authenticate': [oAuthOptions: ClientOAuth2Options];
+	'oauth2.callback': [oAuthOptions: ClientOAuth2Options];
+
+	'tag.beforeCreate': [tag: TagEntity];
+	'tag.afterCreate': [tag: TagEntity];
+	'tag.beforeUpdate': [tag: TagEntity];
+	'tag.afterUpdate': [tag: TagEntity];
+	'tag.beforeDelete': [tagId: string];
+	'tag.afterDelete': [tagId: string];
+
+	'user.deleted': [user: PublicUser];
+	'user.profile.beforeUpdate': [
+		userId: string,
+		currentEmail: string,
+		payload: UserUpdateRequestDto,
+	];
+	'user.profile.update': [currentEmail: string, publicUser: PublicUser];
+	'user.password.update': [updatedEmail: string, updatedPassword: string];
+	'user.invited': [emails: string[]];
+
+	'workflow.create': [createdWorkflow: IWorkflowBase];
+	'workflow.afterCreate': [createdWorkflow: IWorkflowBase];
+	'workflow.activate': [updatedWorkflow: IWorkflowBase];
+	'workflow.update': [updatedWorkflow: IWorkflowBase];
+	'workflow.afterUpdate': [updatedWorkflow: IWorkflowBase];
+	'workflow.delete': [workflowId: string];
+	'workflow.afterDelete': [workflowId: string];
+
+	'workflow.preExecute': [workflow: Workflow, mode: WorkflowExecuteMode];
+	'workflow.postExecute': [
+		fullRunData: IRun | undefined,
+		workflowData: IWorkflowBase,
+		executionId: string,
+	];
+};
+type HookNames = keyof Hooks;
+
+// TODO: Derive this type from Hooks
+interface IExternalHooksFileData {
+	[Resource: string]: {
+		[Operation: string]: Array<(...args: unknown[]) => Promise<void>>;
+	};
+}
 
 @Service()
 export class ExternalHooks {
-	externalHooks: {
+	private readonly registered: {
-		[key: string]: Array<() => {}>;
+		[hookName in HookNames]?: Array<(...args: Hooks[hookName]) => Promise<void>>;
 	} = {};
 
-	private initDidRun = false;
+	private readonly dbCollections: Repositories;
-
-	private dbCollections: IExternalHooksFunctions['dbCollections'];
 
 	constructor(
+		private readonly logger: Logger,
 		private readonly errorReporter: ErrorReporter,
+		private readonly globalConfig: GlobalConfig,
 		userRepository: UserRepository,
 		settingsRepository: SettingsRepository,
 		credentialsRepository: CredentialsRepository,
@@ -35,70 +112,53 @@ export class ExternalHooks {
 		};
 	}
 
-	async init(): Promise<void> {
+	async init() {
-		if (this.initDidRun) {
+		const externalHookFiles = this.globalConfig.externalHooks.files;
-			return;
-		}
-
-		await this.loadHooksFiles();
-
-		this.initDidRun = true;
-	}
-
-	private async loadHooksFiles() {
-		const externalHookFiles = config.getEnv('externalHookFiles').split(':');
 
 		// Load all the provided hook-files
 		for (let hookFilePath of externalHookFiles) {
 			hookFilePath = hookFilePath.trim();
-			if (hookFilePath !== '') {
+			try {
-				try {
+				// eslint-disable-next-line @typescript-eslint/no-var-requires
-					const hookFile = require(hookFilePath) as IExternalHooksFileData;
+				const hookFile = require(hookFilePath) as IExternalHooksFileData;
-					this.loadHooks(hookFile);
+				this.loadHooks(hookFile);
-				} catch (e) {
+			} catch (e) {
-					const error = e instanceof Error ? e : new Error(`${e}`);
+				const error = e instanceof Error ? e : new Error(`${e}`);
 
-					throw new ApplicationError('Problem loading external hook file', {
+				throw new ApplicationError('Problem loading external hook file', {
-						extra: { errorMessage: error.message, hookFilePath },
+					extra: { errorMessage: error.message, hookFilePath },
-						cause: error,
+					cause: error,
-					});
+				});
-				}
 			}
 		}
 	}
 
 	private loadHooks(hookFileData: IExternalHooksFileData) {
-		for (const resource of Object.keys(hookFileData)) {
+		const { registered } = this;
-			for (const operation of Object.keys(hookFileData[resource])) {
+		for (const [resource, operations] of Object.entries(hookFileData)) {
-				// Save all the hook functions directly under their string
+			for (const operation of Object.keys(operations)) {
-				// format in an array
+				const hookName = `${resource}.${operation}` as HookNames;
-				const hookString = `${resource}.${operation}`;
+				registered[hookName] ??= [];
-				if (this.externalHooks[hookString] === undefined) {
+				registered[hookName].push(...operations[operation]);
-					this.externalHooks[hookString] = [];
-				}
-
-				// eslint-disable-next-line prefer-spread
-				this.externalHooks[hookString].push.apply(
-					this.externalHooks[hookString],
-					hookFileData[resource][operation],
-				);
 			}
 		}
 	}
 
-	async run(hookName: string, hookParameters?: any[]): Promise<void> {
+	async run<HookName extends HookNames>(
-		if (this.externalHooks[hookName] === undefined) {
+		hookName: HookName,
-			return;
+		hookParameters?: Hooks[HookName],
-		}
+	): Promise<void> {
+		const { registered, dbCollections } = this;
+		const hookFunctions = registered[hookName];
+		if (!hookFunctions?.length) return;
 
-		const externalHookFunctions: IExternalHooksFunctions = {
+		const context = { dbCollections };
-			dbCollections: this.dbCollections,
-		};
 
-		for (const externalHookFunction of this.externalHooks[hookName]) {
+		for (const hookFunction of hookFunctions) {
 			try {
-				await externalHookFunction.apply(externalHookFunctions, hookParameters);
+				await hookFunction.apply(context, hookParameters);
 			} catch (cause) {
+				this.logger.error(`There was a problem running hook "${hookName}"`);
 				// eslint-disable-next-line @typescript-eslint/no-unsafe-assignment
 				const error = new ApplicationError(`External hook "${hookName}" failed`, { cause });
 				this.errorReporter.error(error, { level: 'fatal' });
@@ -106,8 +166,4 @@ export class ExternalHooks {
 			}
 		}
 	}
-
-	exists(hookName: string): boolean {
-		return !!this.externalHooks[hookName];
-	}
 }

packages/cli/src/interfaces.ts

@@ -31,10 +31,6 @@ import type { AuthProviderType } from '@/databases/entities/auth-identity';
 import type { SharedCredentials } from '@/databases/entities/shared-credentials';
 import type { TagEntity } from '@/databases/entities/tag-entity';
 import type { AssignableRole, GlobalRole, User } from '@/databases/entities/user';
-import type { CredentialsRepository } from '@/databases/repositories/credentials.repository';
-import type { SettingsRepository } from '@/databases/repositories/settings.repository';
-import type { UserRepository } from '@/databases/repositories/user.repository';
-import type { WorkflowRepository } from '@/databases/repositories/workflow.repository';
 
 import type { LICENSE_FEATURES, LICENSE_QUOTAS } from './constants';
 import type { ExternalHooks } from './external-hooks';
@@ -220,46 +216,6 @@ export interface IExecutingWorkflowData {
 	status: ExecutionStatus;
 }
 
-export interface IExternalHooks {
-	credentials?: {
-		create?: Array<{
-			(this: IExternalHooksFunctions, credentialsData: ICredentialsEncrypted): Promise<void>;
-		}>;
-		delete?: Array<{ (this: IExternalHooksFunctions, credentialId: string): Promise<void> }>;
-		update?: Array<{
-			(this: IExternalHooksFunctions, credentialsData: ICredentialsDb): Promise<void>;
-		}>;
-	};
-	workflow?: {
-		activate?: Array<{ (this: IExternalHooksFunctions, workflowData: IWorkflowDb): Promise<void> }>;
-		create?: Array<{ (this: IExternalHooksFunctions, workflowData: IWorkflowBase): Promise<void> }>;
-		delete?: Array<{ (this: IExternalHooksFunctions, workflowId: string): Promise<void> }>;
-		execute?: Array<{
-			(
-				this: IExternalHooksFunctions,
-				workflowData: IWorkflowDb,
-				mode: WorkflowExecuteMode,
-			): Promise<void>;
-		}>;
-		update?: Array<{ (this: IExternalHooksFunctions, workflowData: IWorkflowDb): Promise<void> }>;
-	};
-}
-
-export interface IExternalHooksFileData {
-	[key: string]: {
-		[key: string]: Array<(...args: any[]) => Promise<void>>;
-	};
-}
-
-export interface IExternalHooksFunctions {
-	dbCollections: {
-		User: UserRepository;
-		Settings: SettingsRepository;
-		Credentials: CredentialsRepository;
-		Workflow: WorkflowRepository;
-	};
-}
-
 export interface IPersonalizationSurveyAnswers {
 	email: string | null;
 	codingSkill: string | null;

packages/cli/src/services/tag.service.ts

@@ -8,6 +8,8 @@ import type { ITagWithCountDb } from '@/interfaces';
 
 type GetAllResult<T> = T extends { withUsageCount: true } ? ITagWithCountDb[] : TagEntity[];
 
+type Action = 'Create' | 'Update';
+
 @Service()
 export class TagService {
 	constructor(
@@ -24,7 +26,7 @@ export class TagService {
 	async save(tag: TagEntity, actionKind: 'create' | 'update') {
 		await validateEntity(tag);
 
-		const action = actionKind[0].toUpperCase() + actionKind.slice(1);
+		const action = (actionKind[0].toUpperCase() + actionKind.slice(1)) as Action;
 
 		await this.externalHooks.run(`tag.before${action}`, [tag]);
 

packages/cli/src/workflow-execute-additional-data.ts

@@ -182,9 +182,6 @@ async function startExecution(
 	runData: IWorkflowExecutionDataProcess,
 	workflowData: IWorkflowBase,
 ): Promise<ExecuteWorkflowData> {
-	const externalHooks = Container.get(ExternalHooks);
-	await externalHooks.init();
-
 	const nodeTypes = Container.get(NodeTypes);
 	const activeExecutions = Container.get(ActiveExecutions);
 	const executionRepository = Container.get(ExecutionRepository);
@@ -306,6 +303,7 @@ async function startExecution(
 		);
 	}
 
+	const externalHooks = Container.get(ExternalHooks);
 	await externalHooks.run('workflow.postExecute', [data, workflowData, executionId]);
 
 	// subworkflow either finished, or is in status waiting due to a wait node, both cases are considered successes here

packages/cli/src/workflow-runner.ts

@@ -179,18 +179,13 @@ export class WorkflowRunner {
 			const postExecutePromise = this.activeExecutions.getPostExecutePromise(executionId);
 			postExecutePromise
 				.then(async (executionData) => {
-					if (this.externalHooks.exists('workflow.postExecute')) {
+					try {
-						try {
+						await this.externalHooks.run('workflow.postExecute', [
-							await this.externalHooks.run('workflow.postExecute', [
+							executionData,
-								executionData,
+							data.workflowData,
-								data.workflowData,
+							executionId,
-								executionId,
+						]);
-							]);
+					} catch {}
-						} catch (error) {
-							this.errorReporter.error(error);
-							this.logger.error('There was a problem running hook "workflow.postExecute"', error);
-						}
-					}
 				})
 				.catch((error) => {
 					if (error instanceof ExecutionCancelledError) return;

packages/cli/test/integration/active-workflow-manager.test.ts

@@ -76,10 +76,7 @@ describe('init()', () => {
 	it('should call external hook', async () => {
 		await activeWorkflowManager.init();
 
-		const [hook, arg] = externalHooks.run.mock.calls[0];
+		expect(externalHooks.run).toHaveBeenCalledWith('activeWorkflows.initialized');
-
-		expect(hook).toBe('activeWorkflows.initialized');
-		expect(arg).toBeEmptyArray();
 	});
 
 	it('should check that workflow can be activated', async () => {