fix(core): Do not return inviteAcceptUrl in response if email was s…

…ent (n8n-io#7465)
netroy · Oct 19, 2023 · 55c6a1b · 55c6a1b
1 parent ab6a9bb
commit 55c6a1b
Show file tree

Hide file tree

Showing 11 changed files with 74 additions and 102 deletions.
diff --git a/packages/cli/src/Server.ts b/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,

diff --git a/packages/cli/src/UserManagement/UserManagementHelper.ts b/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();
 }

diff --git a/packages/cli/src/UserManagement/email/UserManagementMailer.ts b/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();
 		}
 	}

diff --git a/packages/cli/src/controllers/passwordReset.controller.ts b/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',
 			);

diff --git a/packages/cli/src/controllers/users.controller.ts b/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');
 		}

diff --git a/packages/cli/src/services/frontend.service.ts b/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: {

diff --git a/packages/cli/test/integration/ldap/ldap.api.test.ts b/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> => {

diff --git a/packages/cli/test/integration/passwordReset.api.test.ts b/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 });
 

diff --git a/packages/cli/test/integration/shared/utils/testServer.ts b/packages/cli/test/integration/shared/utils/testServer.ts
@@ -256,7 +256,6 @@ export const setupTestServer = ({
 							app,
 							config,
 							new PasswordResetController(
-								config,
 								logger,
 								externalHooks,
 								internalHooks,