From 55c6a1b0d394265fa4018a7023971589d8e61b4a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E0=A4=95=E0=A4=BE=E0=A4=B0=E0=A4=A4=E0=A5=8B=E0=A4=AB?= =?UTF-8?q?=E0=A5=8D=E0=A4=AB=E0=A5=87=E0=A4=B2=E0=A4=B8=E0=A5=8D=E0=A4=95?= =?UTF-8?q?=E0=A5=8D=E0=A4=B0=E0=A4=BF=E0=A4=AA=E0=A5=8D=E0=A4=9F=E2=84=A2?= Date: Thu, 19 Oct 2023 13:58:06 +0200 Subject: [PATCH] fix(core): Do not return `inviteAcceptUrl` in response if email was sent (#7465) --- packages/cli/src/Server.ts | 1 - .../UserManagement/UserManagementHelper.ts | 7 --- .../email/UserManagementMailer.ts | 11 ++-- .../controllers/passwordReset.controller.ts | 4 +- .../cli/src/controllers/users.controller.ts | 6 +- packages/cli/src/services/frontend.service.ts | 6 +- .../test/integration/ldap/ldap.api.test.ts | 11 ++-- .../integration/passwordReset.api.test.ts | 27 +++----- .../integration/shared/utils/testServer.ts | 1 - .../cli/test/integration/users.api.test.ts | 62 ++----------------- .../test/unit/UserManagementMailer.test.ts | 40 ++++++++++++ 11 files changed, 74 insertions(+), 102 deletions(-) create mode 100644 packages/cli/test/unit/UserManagementMailer.test.ts diff --git a/packages/cli/src/Server.ts b/packages/cli/src/Server.ts index bcb1d688991c0..c6dd61317ddbb 100644 --- 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 index 6ec733e675863..724943f5731c3 100644 --- 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 index 4a140184cfe69..549d3b8f66d7f 100644 --- 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 index b3d8de193feae..c11eacd5e2e7c 100644 --- 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 index e77774c9ecfdf..95322a805fc12 100644 --- 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 index d4224fee8d07f..fd776cc56cae1 100644 --- 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 index 1a3e23cd8ccf8..c225bbffbbbd4 100644 --- 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()); 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 = {}): Promise => { diff --git a/packages/cli/test/integration/passwordReset.api.test.ts b/packages/cli/test/integration/passwordReset.api.test.ts index 70a094fbdf0c9..9a17e401ce954 100644 --- 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 index 33fb220a0423d..233cf8a76c3a6 100644 --- 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, diff --git a/packages/cli/test/integration/users.api.test.ts b/packages/cli/test/integration/users.api.test.ts index 3f3457d5cb194..1373e3a0c532e 100644 --- a/packages/cli/test/integration/users.api.test.ts +++ b/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, []>; - let mockVerifyConnection: jest.SpyInstance, []>; - - 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(); - }); -}); diff --git a/packages/cli/test/unit/UserManagementMailer.test.ts b/packages/cli/test/unit/UserManagementMailer.test.ts new file mode 100644 index 0000000000000..06ed1ab12de21 --- /dev/null +++ b/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, []>; + let mockVerifyConnection: jest.SpyInstance, []>; + + 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(); + }); + }); +});