fix(core): Prevent calling internal hook email event if emailing is disabled

packages/cli/src/UserManagement/email/UserManagementMailer.ts

@@ -1,12 +1,22 @@
+import { Container, Service } from 'typedi';
 import { existsSync } from 'fs';
 import { readFile } from 'fs/promises';
 import Handlebars from 'handlebars';
 import { join as pathJoin } from 'path';
-import { Container, Service } from 'typedi';
+import { ApplicationError } from 'n8n-workflow';
+
 import config from '@/config';
+import type { User } from '@db/entities/User';
+import type { WorkflowEntity } from '@db/entities/WorkflowEntity';
+import { UserRepository } from '@db/repositories/user.repository';
+import { InternalHooks } from '@/InternalHooks';
+import { Logger } from '@/Logger';
+import { UrlService } from '@/services/url.service';
+import { InternalServerError } from '@/errors/response-errors/internal-server.error';
+import { toError } from '@/utils';
+
 import type { InviteEmailData, PasswordResetData, SendEmailResult } from './Interfaces';
 import { NodeMailer } from './NodeMailer';
-import { ApplicationError } from 'n8n-workflow';
 
 type Template = HandlebarsTemplateDelegate<unknown>;
 type TemplateName = 'invite' | 'passwordReset' | 'workflowShared' | 'credentialsShared';
@@ -39,7 +49,11 @@ export class UserManagementMailer {
 
 	private mailer: NodeMailer | undefined;
 
-	constructor() {
+	constructor(
+		private readonly userRepository: UserRepository,
+		private readonly logger: Logger,
+		private readonly urlService: UrlService,
+	) {
 		this.isEmailSetUp =
 			config.getEnv('userManagement.emails.mode') === 'smtp' &&
 			config.getEnv('userManagement.emails.smtp.host') !== '';
@@ -83,48 +97,109 @@ export class UserManagementMailer {
 	}
 
 	async notifyWorkflowShared({
-		recipientEmails,
-		workflowName,
-		baseUrl,
-		workflowId,
-		sharerFirstName,
+		sharer,
+		newShareeIds,
+		workflow,
 	}: {
-		recipientEmails: string[];
-		workflowName: string;
-		baseUrl: string;
-		workflowId: string;
-		sharerFirstName: string;
+		sharer: User;
+		newShareeIds: string[];
+		workflow: WorkflowEntity;
 	}) {
+		if (!this.mailer) return;
+
+		const recipients = await this.userRepository.getEmailsByIds(newShareeIds);
+
+		if (recipients.length === 0) return;
+
+		const emailRecipients = recipients.map(({ email }) => email);
+
 		const populateTemplate = await getTemplate('workflowShared', 'workflowShared.html');
 
-		const result = await this.mailer?.sendMail({
-			emailRecipients: recipientEmails,
-			subject: `${sharerFirstName} has shared an n8n workflow with you`,
-			body: populateTemplate({ workflowName, workflowUrl: `${baseUrl}/workflow/${workflowId}` }),
-		});
+		const baseUrl = this.urlService.getInstanceBaseUrl();
 
-		return result ?? { emailSent: false };
+		try {
+			const result = await this.mailer.sendMail({
+				emailRecipients,
+				subject: `${sharer.firstName} has shared an n8n workflow with you`,
+				body: populateTemplate({
+					workflowName: workflow.name,
+					workflowUrl: `${baseUrl}/workflow/${workflow.id}`,
+				}),
+			});
+
+			if (!result) return { emailSent: false };
+
+			this.logger.info('Sent workflow shared email successfully', { sharerId: sharer.id });
+
+			void Container.get(InternalHooks).onUserTransactionalEmail({
+				user_id: sharer.id,
+				message_type: 'Workflow shared',
+				public_api: false,
+			});
+
+			return result;
+		} catch (e) {
+			void Container.get(InternalHooks).onEmailFailed({
+				user: sharer,
+				message_type: 'Workflow shared',
+				public_api: false,
+			});
+
+			const error = toError(e);
+
+			throw new InternalServerError(`Please contact your administrator: ${error.message}`);
+		}
 	}
 
 	async notifyCredentialsShared({
-		sharerFirstName,
+		sharer,
+		newShareeIds,
 		credentialsName,
-		recipientEmails,
-		baseUrl,
 	}: {
-		sharerFirstName: string;
+		sharer: User;
+		newShareeIds: string[];
 		credentialsName: string;
-		recipientEmails: string[];
-		baseUrl: string;
 	}) {
+		if (!this.mailer) return;
+
+		const recipients = await this.userRepository.getEmailsByIds(newShareeIds);
+
+		if (recipients.length === 0) return;
+
+		const emailRecipients = recipients.map(({ email }) => email);
+
 		const populateTemplate = await getTemplate('credentialsShared', 'credentialsShared.html');
 
-		const result = await this.mailer?.sendMail({
-			emailRecipients: recipientEmails,
-			subject: `${sharerFirstName} has shared an n8n credential with you`,
-			body: populateTemplate({ credentialsName, credentialsListUrl: `${baseUrl}/credentials` }),
-		});
+		const baseUrl = this.urlService.getInstanceBaseUrl();
 
-		return result ?? { emailSent: false };
+		try {
+			const result = await this.mailer.sendMail({
+				emailRecipients,
+				subject: `${sharer.firstName} has shared an n8n credential with you`,
+				body: populateTemplate({ credentialsName, credentialsListUrl: `${baseUrl}/credentials` }),
+			});
+
+			if (!result) return { emailSent: false };
+
+			this.logger.info('Sent credentials shared email successfully', { sharerId: sharer.id });
+
+			void Container.get(InternalHooks).onUserTransactionalEmail({
+				user_id: sharer.id,
+				message_type: 'Credentials shared',
+				public_api: false,
+			});
+
+			return result;
+		} catch (e) {
+			void Container.get(InternalHooks).onEmailFailed({
+				user: sharer,
+				message_type: 'Credentials shared',
+				public_api: false,
+			});
+
+			const error = toError(e);
+
+			throw new InternalServerError(`Please contact your administrator: ${error.message}`);
+		}
 	}
 }

packages/cli/src/credentials/credentials.controller.ee.ts

@@ -15,11 +15,7 @@ import { NotFoundError } from '@/errors/response-errors/not-found.error';
 import { UnauthorizedError } from '@/errors/response-errors/unauthorized.error';
 import { CredentialsRepository } from '@/databases/repositories/credentials.repository';
 import * as utils from '@/utils';
-import { UserRepository } from '@/databases/repositories/user.repository';
 import { UserManagementMailer } from '@/UserManagement/email';
-import { UrlService } from '@/services/url.service';
-import { Logger } from '@/Logger';
-import { InternalServerError } from '@/errors/response-errors/internal-server.error';
 
 export const EECredentialsController = express.Router();
 
@@ -190,36 +186,10 @@ EECredentialsController.put(
 			sharees_removed: amountRemoved,
 		});
 
-		const recipients = await Container.get(UserRepository).getEmailsByIds(newShareeIds);
-
-		if (recipients.length === 0) return;
-
-		try {
-			await Container.get(UserManagementMailer).notifyCredentialsShared({
-				sharerFirstName: req.user.firstName,
-				credentialsName: credential.name,
-				recipientEmails: recipients.map(({ email }) => email),
-				baseUrl: Container.get(UrlService).getInstanceBaseUrl(),
-			});
-		} catch (error) {
-			void Container.get(InternalHooks).onEmailFailed({
-				user: req.user,
-				message_type: 'Credentials shared',
-				public_api: false,
-			});
-			if (error instanceof Error) {
-				throw new InternalServerError(`Please contact your administrator: ${error.message}`);
-			}
-		}
-
-		Container.get(Logger).info('Sent credentials shared email successfully', {
-			sharerId: req.user.id,
-		});
-
-		void Container.get(InternalHooks).onUserTransactionalEmail({
-			user_id: req.user.id,
-			message_type: 'Credentials shared',
-			public_api: false,
+		await Container.get(UserManagementMailer).notifyCredentialsShared({
+			sharer: req.user,
+			newShareeIds,
+			credentialsName: credential.name,
 		});
 	}),
 );

packages/cli/src/workflows/workflows.controller.ts

@@ -39,7 +39,6 @@ import { EnterpriseWorkflowService } from './workflow.service.ee';
 import { WorkflowExecutionService } from './workflowExecution.service';
 import { WorkflowSharingService } from './workflowSharing.service';
 import { UserManagementMailer } from '@/UserManagement/email';
-import { UrlService } from '@/services/url.service';
 
 @Service()
 @Authorized()
@@ -63,7 +62,6 @@ export class WorkflowsController {
 		private readonly userRepository: UserRepository,
 		private readonly license: License,
 		private readonly mailer: UserManagementMailer,
-		private readonly urlService: UrlService,
 	) {}
 
 	@Post('/')
@@ -403,35 +401,10 @@ export class WorkflowsController {
 
 		void this.internalHooks.onWorkflowSharingUpdate(workflowId, req.user.id, shareWithIds);
 
-		const recipients = await this.userRepository.getEmailsByIds(newShareeIds);
-
-		if (recipients.length === 0) return;
-
-		try {
-			await this.mailer.notifyWorkflowShared({
-				recipientEmails: recipients.map(({ email }) => email),
-				workflowName: workflow.name,
-				workflowId,
-				sharerFirstName: req.user.firstName,
-				baseUrl: this.urlService.getInstanceBaseUrl(),
-			});
-		} catch (error) {
-			void this.internalHooks.onEmailFailed({
-				user: req.user,
-				message_type: 'Workflow shared',
-				public_api: false,
-			});
-			if (error instanceof Error) {
-				throw new InternalServerError(`Please contact your administrator: ${error.message}`);
-			}
-		}
-
-		this.logger.info('Sent workflow shared email successfully', { sharerId: req.user.id });
-
-		void this.internalHooks.onUserTransactionalEmail({
-			user_id: req.user.id,
-			message_type: 'Workflow shared',
-			public_api: false,
+		await this.mailer.notifyWorkflowShared({
+			sharer: req.user,
+			newShareeIds,
+			workflow,
 		});
 	}
 }

packages/cli/test/integration/controllers/invitation/assertions.ts

@@ -0,0 +1,29 @@
+import validator from 'validator';
+import type { User } from '@/databases/entities/User';
+import type { UserInvitationResult } from '../../shared/utils/users';
+
+export function assertReturnedUserProps(user: User) {
+	expect(validator.isUUID(user.id)).toBe(true);
+	expect(user.email).toBeDefined();
+	expect(user.personalizationAnswers).toBeNull();
+	expect(user.password).toBeUndefined();
+	expect(user.isPending).toBe(false);
+	expect(user.apiKey).not.toBeDefined();
+	expect(user.globalScopes).toBeDefined();
+	expect(user.globalScopes).not.toHaveLength(0);
+}
+
+export const assertStoredUserProps = (user: User) => {
+	expect(user.firstName).toBeNull();
+	expect(user.lastName).toBeNull();
+	expect(user.personalizationAnswers).toBeNull();
+	expect(user.password).toBeNull();
+	expect(user.isPending).toBe(true);
+};
+
+export const assertUserInviteResult = (data: UserInvitationResult) => {
+	expect(validator.isUUID(data.user.id)).toBe(true);
+	expect(data.user.inviteAcceptUrl).toBeUndefined();
+	expect(data.user.email).toBeDefined();
+	expect(data.user.emailSent).toBe(true);
+};