fix(core): Do not return inviteAcceptUrl in response if email was sent (#7465)

packages/cli/src/Server.ts

@@ -310,7 +310,6 @@ export class Server extends AbstractServer {
 			new MeController(logger, externalHooks, internalHooks, userService),
 			new NodeTypesController(config, nodeTypes),
 			new PasswordResetController(
-				config,
 				logger,
 				externalHooks,
 				internalHooks,

packages/cli/src/UserManagement/UserManagementHelper.ts

@@ -12,13 +12,6 @@ import { License } from '@/License';
 import { getWebhookBaseUrl } from '@/WebhookHelpers';
 import { RoleService } from '@/services/role.service';
 
-export function isEmailSetUp(): boolean {
-	const smtp = config.getEnv('userManagement.emails.mode') === 'smtp';
-	const host = !!config.getEnv('userManagement.emails.smtp.host');
-
-	return smtp && host;
-}
-
 export function isSharingEnabled(): boolean {
 	return Container.get(License).isSharingEnabled();
 }

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

@@ -34,14 +34,17 @@ async function getTemplate(
 
 @Service()
 export class UserManagementMailer {
+	readonly isEmailSetUp: boolean;
+
 	private mailer: NodeMailer | undefined;
 
 	constructor() {
-		// Other implementations can be used in the future.
-		if (
+		this.isEmailSetUp =
 			config.getEnv('userManagement.emails.mode') === 'smtp' &&
-			config.getEnv('userManagement.emails.smtp.host') !== ''
-		) {
+			config.getEnv('userManagement.emails.smtp.host') !== '';
+
+		// Other implementations can be used in the future.
+		if (this.isEmailSetUp) {
 			this.mailer = new NodeMailer();
 		}
 	}

packages/cli/src/controllers/passwordReset.controller.ts

@@ -17,7 +17,6 @@ import { UserManagementMailer } from '@/UserManagement/email';
 
 import { Response } from 'express';
 import { ILogger } from 'n8n-workflow';
-import { Config } from '@/config';
 import { PasswordResetRequest } from '@/requests';
 import { IExternalHooksClass, IInternalHooksClass } from '@/Interfaces';
 import { issueCookie } from '@/auth/jwt';
@@ -35,7 +34,6 @@ import { MfaService } from '@/Mfa/mfa.service';
 @RestController()
 export class PasswordResetController {
 	constructor(
-		private readonly config: Config,
 		private readonly logger: ILogger,
 		private readonly externalHooks: IExternalHooksClass,
 		private readonly internalHooks: IInternalHooksClass,
@@ -50,7 +48,7 @@ export class PasswordResetController {
 	 */
 	@Post('/forgot-password')
 	async forgotPassword(req: PasswordResetRequest.Email) {
-		if (this.config.getEnv('userManagement.emails.mode') === '') {
+		if (!this.mailer.isEmailSetUp) {
 			this.logger.debug(
 				'Request to send password reset email failed because emailing was not set up',
 			);

packages/cli/src/controllers/users.controller.ts

@@ -10,7 +10,6 @@ import {
 	generateUserInviteUrl,
 	getInstanceBaseUrl,
 	hashPassword,
-	isEmailSetUp,
 	validatePassword,
 } from '@/UserManagement/UserManagementHelper';
 import { issueCookie } from '@/auth/jwt';
@@ -184,7 +183,7 @@ export class UsersController {
 				}
 				const inviteAcceptUrl = generateUserInviteUrl(req.user.id, id);
 				const resp: {
-					user: { id: string | null; email: string; inviteAcceptUrl: string; emailSent: boolean };
+					user: { id: string | null; email: string; inviteAcceptUrl?: string; emailSent: boolean };
 					error?: string;
 				} = {
 					user: {
@@ -202,6 +201,7 @@ export class UsersController {
 					});
 					if (result.emailSent) {
 						resp.user.emailSent = true;
+						delete resp.user.inviteAcceptUrl;
 						void this.internalHooks.onUserTransactionalEmail({
 							user_id: id,
 							message_type: 'New user invite',
@@ -619,7 +619,7 @@ export class UsersController {
 			throw new UnauthorizedError(RESPONSE_ERROR_MESSAGES.USERS_QUOTA_REACHED);
 		}
 
-		if (!isEmailSetUp()) {
+		if (!this.mailer.isEmailSetUp) {
 			this.logger.error('Request to reinvite a user failed because email sending was not set up');
 			throw new InternalServerError('Email sending must be set up in order to invite other users');
 		}

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

@@ -16,7 +16,7 @@ import { CredentialsOverwrites } from '@/CredentialsOverwrites';
 import { CredentialTypes } from '@/CredentialTypes';
 import { LoadNodesAndCredentials } from '@/LoadNodesAndCredentials';
 import { License } from '@/License';
-import { getInstanceBaseUrl, isEmailSetUp } from '@/UserManagement/UserManagementHelper';
+import { getInstanceBaseUrl } from '@/UserManagement/UserManagementHelper';
 import * as WebhookHelpers from '@/WebhookHelpers';
 import { LoggerProxy } from 'n8n-workflow';
 import config from '@/config';
@@ -28,6 +28,7 @@ import {
 	getWorkflowHistoryLicensePruneTime,
 	getWorkflowHistoryPruneTime,
 } from '@/workflows/workflowHistory/workflowHistoryHelper.ee';
+import { UserManagementMailer } from '@/UserManagement/email';
 
 @Service()
 export class FrontendService {
@@ -38,6 +39,7 @@ export class FrontendService {
 		private readonly credentialTypes: CredentialTypes,
 		private readonly credentialsOverwrites: CredentialsOverwrites,
 		private readonly license: License,
+		private readonly mailer: UserManagementMailer,
 	) {
 		this.initSettings();
 	}
@@ -103,7 +105,7 @@ export class FrontendService {
 			userManagement: {
 				quota: this.license.getUsersLimit(),
 				showSetupOnFirstLoad: !config.getEnv('userManagement.isInstanceOwnerSetUp'),
-				smtpSetup: isEmailSetUp(),
+				smtpSetup: this.mailer.isEmailSetUp,
 				authenticationMethod: getCurrentAuthenticationMethod(),
 			},
 			sso: {

packages/cli/test/integration/ldap/ldap.api.test.ts

@@ -1,7 +1,9 @@
 import type { SuperAgentTest } from 'supertest';
 import type { Entry as LdapUser } from 'ldapts';
 import { Not } from 'typeorm';
-import { jsonParse } from 'n8n-workflow';
+import { type ILogger, jsonParse, LoggerProxy } from 'n8n-workflow';
+import { mock } from 'jest-mock-extended';
+
 import config from '@/config';
 import * as Db from '@/Db';
 import type { Role } from '@db/entities/Role';
@@ -17,17 +19,13 @@ import { randomEmail, randomName, uniqueId } from './../shared/random';
 import * as testDb from './../shared/testDb';
 import * as utils from '../shared/utils/';
 
-import { LoggerProxy } from 'n8n-workflow';
-import { getLogger } from '@/Logger';
-
 jest.mock('@/telemetry');
-jest.mock('@/UserManagement/email/NodeMailer');
 
 let globalMemberRole: Role;
 let owner: User;
 let authOwnerAgent: SuperAgentTest;
 
-LoggerProxy.init(getLogger());
+LoggerProxy.init(mock<ILogger>());
 
 const defaultLdapConfig = {
 	...LDAP_DEFAULT_CONFIGURATION,
@@ -80,7 +78,6 @@ beforeEach(async () => {
 	jest.mock('@/telemetry');
 
 	config.set('userManagement.isInstanceOwnerSetUp', true);
-	config.set('userManagement.emails.mode', '');
 });
 
 const createLdapConfig = async (attributes: Partial<LdapConfig> = {}): Promise<LdapConfig> => {

packages/cli/test/integration/passwordReset.api.test.ts

@@ -1,11 +1,17 @@
 import { v4 as uuid } from 'uuid';
 import { compare } from 'bcryptjs';
+import { Container } from 'typedi';
 import { License } from '@/License';
 
 import * as Db from '@/Db';
 import config from '@/config';
 import type { Role } from '@db/entities/Role';
 import type { User } from '@db/entities/User';
+import { setCurrentAuthenticationMethod } from '@/sso/ssoHelpers';
+import { ExternalHooks } from '@/ExternalHooks';
+import { JwtService } from '@/services/jwt.service';
+import { UserManagementMailer } from '@/UserManagement/email';
+
 import * as utils from './shared/utils/';
 import {
 	randomEmail,
@@ -15,12 +21,7 @@ import {
 	randomValidPassword,
 } from './shared/random';
 import * as testDb from './shared/testDb';
-import { setCurrentAuthenticationMethod } from '@/sso/ssoHelpers';
-import { ExternalHooks } from '@/ExternalHooks';
-import { JwtService } from '@/services/jwt.service';
-import { Container } from 'typedi';
 
-jest.mock('@/UserManagement/email/NodeMailer');
 config.set('userManagement.jwtSecret', randomString(5, 10));
 
 let globalOwnerRole: Role;
@@ -29,6 +30,7 @@ let owner: User;
 let member: User;
 
 const externalHooks = utils.mockInstance(ExternalHooks);
+const mailer = utils.mockInstance(UserManagementMailer, { isEmailSetUp: true });
 const testServer = utils.setupTestServer({ endpointGroups: ['passwordReset'] });
 const jwtService = Container.get(JwtService);
 
@@ -43,6 +45,7 @@ beforeEach(async () => {
 	owner = await testDb.createUser({ globalRole: globalOwnerRole });
 	member = await testDb.createUser({ globalRole: globalMemberRole });
 	externalHooks.run.mockReset();
+	jest.replaceProperty(mailer, 'isEmailSetUp', true);
 });
 
 describe('POST /forgot-password', () => {
@@ -52,8 +55,6 @@ describe('POST /forgot-password', () => {
 			globalRole: globalMemberRole,
 		});
 
-		config.set('userManagement.emails.mode', 'smtp');
-
 		await Promise.all(
 			[{ email: owner.email }, { email: member.email.toUpperCase() }].map(async (payload) => {
 				const response = await testServer.authlessAgent.post('/forgot-password').send(payload);
@@ -65,7 +66,7 @@ describe('POST /forgot-password', () => {
 	});
 
 	test('should fail if emailing is not set up', async () => {
-		config.set('userManagement.emails.mode', '');
+		jest.replaceProperty(mailer, 'isEmailSetUp', false);
 
 		await testServer.authlessAgent
 			.post('/forgot-password')
@@ -75,7 +76,6 @@ describe('POST /forgot-password', () => {
 
 	test('should fail if SAML is authentication method', async () => {
 		await setCurrentAuthenticationMethod('saml');
-		config.set('userManagement.emails.mode', 'smtp');
 		const member = await testDb.createUser({
 			email: 'test@test.com',
 			globalRole: globalMemberRole,
@@ -91,7 +91,6 @@ describe('POST /forgot-password', () => {
 
 	test('should succeed if SAML is authentication method and requestor is owner', async () => {
 		await setCurrentAuthenticationMethod('saml');
-		config.set('userManagement.emails.mode', 'smtp');
 
 		const response = await testServer.authlessAgent
 			.post('/forgot-password')
@@ -104,8 +103,6 @@ describe('POST /forgot-password', () => {
 	});
 
 	test('should fail with invalid inputs', async () => {
-		config.set('userManagement.emails.mode', 'smtp');
-
 		const invalidPayloads = [
 			randomEmail(),
 			[randomEmail()],
@@ -121,8 +118,6 @@ describe('POST /forgot-password', () => {
 	});
 
 	test('should fail if user is not found', async () => {
-		config.set('userManagement.emails.mode', 'smtp');
-
 		const response = await testServer.authlessAgent
 			.post('/forgot-password')
 			.send({ email: randomEmail() });
@@ -132,10 +127,6 @@ describe('POST /forgot-password', () => {
 });
 
 describe('GET /resolve-password-token', () => {
-	beforeEach(() => {
-		config.set('userManagement.emails.mode', 'smtp');
-	});
-
 	test('should succeed with valid inputs', async () => {
 		const resetPasswordToken = jwtService.signData({ sub: owner.id });
 

packages/cli/test/integration/shared/utils/testServer.ts

@@ -256,7 +256,6 @@ export const setupTestServer = ({
 							app,
 							config,
 							new PasswordResetController(
-								config,
 								logger,
 								externalHooks,
 								internalHooks,

packages/cli/test/integration/users.api.test.ts

@@ -2,7 +2,6 @@ import validator from 'validator';
 import { Not } from 'typeorm';
 import type { SuperAgentTest } from 'supertest';
 
-import config from '@/config';
 import * as Db from '@/Db';
 import { CredentialsEntity } from '@db/entities/CredentialsEntity';
 import type { Role } from '@db/entities/Role';
@@ -10,7 +9,6 @@ import type { User } from '@db/entities/User';
 import { WorkflowEntity } from '@db/entities/WorkflowEntity';
 import { compareHash } from '@/UserManagement/UserManagementHelper';
 import { UserManagementMailer } from '@/UserManagement/email/UserManagementMailer';
-import { NodeMailer } from '@/UserManagement/email/NodeMailer';
 
 import { SUCCESS_RESPONSE_BODY } from './shared/constants';
 import {
@@ -23,14 +21,14 @@ import {
 import * as testDb from './shared/testDb';
 import * as utils from './shared/utils/';
 
-jest.mock('@/UserManagement/email/NodeMailer');
-
 let globalMemberRole: Role;
 let workflowOwnerRole: Role;
 let credentialOwnerRole: Role;
 let owner: User;
 let authOwnerAgent: SuperAgentTest;
 
+const mailer = utils.mockInstance(UserManagementMailer, { isEmailSetUp: true });
+
 const testServer = utils.setupTestServer({ endpointGroups: ['users'] });
 
 beforeAll(async () => {
@@ -54,10 +52,7 @@ beforeEach(async () => {
 	await testDb.truncate(['SharedCredentials', 'SharedWorkflow', 'Workflow', 'Credentials']);
 	await Db.collections.User.delete({ id: Not(owner.id) });
 
-	jest.mock('@/config');
-
-	config.set('userManagement.emails.mode', 'smtp');
-	config.set('userManagement.emails.smtp.host', '');
+	mailer.invite.mockResolvedValue({ emailSent: true });
 });
 
 describe('DELETE /users/:id', () => {
@@ -313,11 +308,8 @@ describe('POST /users/:id', () => {
 });
 
 describe('POST /users', () => {
-	beforeEach(() => {
-		config.set('userManagement.emails.mode', 'smtp');
-	});
-
 	test('should succeed if emailing is not set up', async () => {
+		mailer.invite.mockResolvedValueOnce({ emailSent: false });
 		const response = await authOwnerAgent.post('/users').send([{ email: randomEmail() }]);
 
 		expect(response.statusCode).toBe(200);
@@ -342,11 +334,12 @@ describe('POST /users', () => {
 		expect(response.statusCode).toBe(200);
 
 		for (const {
-			user: { id, email: receivedEmail },
+			user: { id, email: receivedEmail, inviteAcceptUrl },
 			error,
 		} of response.body.data) {
 			expect(validator.isUUID(id)).toBe(true);
 			expect(id).not.toBe(member.id);
+			expect(inviteAcceptUrl).toBeUndefined();
 
 			const lowerCasedEmail = receivedEmail.toLowerCase();
 			expect(receivedEmail).toBe(lowerCasedEmail);
@@ -401,14 +394,6 @@ describe('POST /users', () => {
 });
 
 describe('POST /users/:id/reinvite', () => {
-	beforeEach(() => {
-		config.set('userManagement.emails.mode', 'smtp');
-		// those configs are needed to make sure the reinvite email is sent,because of this check isEmailSetUp()
-		config.set('userManagement.emails.smtp.host', 'host');
-		config.set('userManagement.emails.smtp.auth.user', 'user');
-		config.set('userManagement.emails.smtp.auth.pass', 'pass');
-	});
-
 	test('should send reinvite, but fail if user already accepted invite', async () => {
 		const email = randomEmail();
 		const payload = [{ email }];
@@ -428,38 +413,3 @@ describe('POST /users/:id/reinvite', () => {
 		expect(reinviteMemberResponse.statusCode).toBe(400);
 	});
 });
-
-describe('UserManagementMailer expect NodeMailer.verifyConnection', () => {
-	let mockInit: jest.SpyInstance<Promise<void>, []>;
-	let mockVerifyConnection: jest.SpyInstance<Promise<void>, []>;
-
-	beforeAll(() => {
-		mockVerifyConnection = jest
-			.spyOn(NodeMailer.prototype, 'verifyConnection')
-			.mockImplementation(async () => {});
-		mockInit = jest.spyOn(NodeMailer.prototype, 'init').mockImplementation(async () => {});
-	});
-
-	afterAll(() => {
-		mockVerifyConnection.mockRestore();
-		mockInit.mockRestore();
-	});
-
-	test('not be called when SMTP not set up', async () => {
-		const userManagementMailer = new UserManagementMailer();
-		// NodeMailer.verifyConnection gets called only explicitly
-		await expect(async () => userManagementMailer.verifyConnection()).rejects.toThrow();
-
-		expect(NodeMailer.prototype.verifyConnection).toHaveBeenCalledTimes(0);
-	});
-
-	test('to be called when SMTP set up', async () => {
-		// host needs to be set, otherwise smtp is skipped
-		config.set('userManagement.emails.smtp.host', 'host');
-		config.set('userManagement.emails.mode', 'smtp');
-
-		const userManagementMailer = new UserManagementMailer();
-		// NodeMailer.verifyConnection gets called only explicitly
-		expect(async () => userManagementMailer.verifyConnection()).not.toThrow();
-	});
-});

packages/cli/test/unit/UserManagementMailer.test.ts

@@ -0,0 +1,40 @@
+import config from '@/config';
+import { NodeMailer } from '@/UserManagement/email/NodeMailer';
+import { UserManagementMailer } from '@/UserManagement/email/UserManagementMailer';
+
+describe('UserManagementMailer', () => {
+	describe('expect NodeMailer.verifyConnection', () => {
+		let mockInit: jest.SpyInstance<Promise<void>, []>;
+		let mockVerifyConnection: jest.SpyInstance<Promise<void>, []>;
+
+		beforeAll(() => {
+			mockVerifyConnection = jest
+				.spyOn(NodeMailer.prototype, 'verifyConnection')
+				.mockImplementation(async () => {});
+			mockInit = jest.spyOn(NodeMailer.prototype, 'init').mockImplementation(async () => {});
+		});
+
+		afterAll(() => {
+			mockVerifyConnection.mockRestore();
+			mockInit.mockRestore();
+		});
+
+		test('not be called when SMTP not set up', async () => {
+			const userManagementMailer = new UserManagementMailer();
+			// NodeMailer.verifyConnection gets called only explicitly
+			await expect(async () => userManagementMailer.verifyConnection()).rejects.toThrow();
+
+			expect(NodeMailer.prototype.verifyConnection).toHaveBeenCalledTimes(0);
+		});
+
+		test('to be called when SMTP set up', async () => {
+			// host needs to be set, otherwise smtp is skipped
+			config.set('userManagement.emails.smtp.host', 'host');
+			config.set('userManagement.emails.mode', 'smtp');
+
+			const userManagementMailer = new UserManagementMailer();
+			// NodeMailer.verifyConnection gets called only explicitly
+			expect(async () => userManagementMailer.verifyConnection()).not.toThrow();
+		});
+	});
+});