From 9e939809575592622f6bdca112da1905ac9205ef Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Iv=C3=A1n=20Ovejero?= Date: Mon, 29 Jan 2024 16:15:30 +0100 Subject: [PATCH 1/6] fix(core): Prevent calling internal hook email event if emailing is disabled (#8462) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: कारतोफ्फेलस्क्रिप्ट™ --- .../email/UserManagementMailer.ts | 137 ++++-- .../credentials/credentials.controller.ee.ts | 38 +- .../cli/src/workflows/workflows.controller.ts | 35 +- .../controllers/invitation/assertions.ts | 29 ++ .../invitation.controller.integration.test.ts | 412 ++++++++++++++++ .../test/integration/credentials.ee.test.ts | 26 +- .../test/integration/invitations.api.test.ts | 443 ------------------ .../test/integration/shared/utils/users.ts | 26 +- .../workflows/workflows.controller.ee.test.ts | 17 +- .../test/unit/services/user.service.test.ts | 2 + 10 files changed, 597 insertions(+), 568 deletions(-) create mode 100644 packages/cli/test/integration/controllers/invitation/assertions.ts create mode 100644 packages/cli/test/integration/controllers/invitation/invitation.controller.integration.test.ts delete mode 100644 packages/cli/test/integration/invitations.api.test.ts diff --git a/packages/cli/src/UserManagement/email/UserManagementMailer.ts b/packages/cli/src/UserManagement/email/UserManagementMailer.ts index 95ad9614e0daa..b350da8e4c183 100644 --- a/packages/cli/src/UserManagement/email/UserManagementMailer.ts +++ b/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; 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}`); + } } } diff --git a/packages/cli/src/credentials/credentials.controller.ee.ts b/packages/cli/src/credentials/credentials.controller.ee.ts index 4a8211caf2fd6..09dc2a4e75052 100644 --- a/packages/cli/src/credentials/credentials.controller.ee.ts +++ b/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, }); }), ); diff --git a/packages/cli/src/workflows/workflows.controller.ts b/packages/cli/src/workflows/workflows.controller.ts index 912457760a663..799263ad5e009 100644 --- a/packages/cli/src/workflows/workflows.controller.ts +++ b/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, }); } } diff --git a/packages/cli/test/integration/controllers/invitation/assertions.ts b/packages/cli/test/integration/controllers/invitation/assertions.ts new file mode 100644 index 0000000000000..349eac4e8ee64 --- /dev/null +++ b/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); +}; diff --git a/packages/cli/test/integration/controllers/invitation/invitation.controller.integration.test.ts b/packages/cli/test/integration/controllers/invitation/invitation.controller.integration.test.ts new file mode 100644 index 0000000000000..971360bc05438 --- /dev/null +++ b/packages/cli/test/integration/controllers/invitation/invitation.controller.integration.test.ts @@ -0,0 +1,412 @@ +import { mocked } from 'jest-mock'; +import Container from 'typedi'; +import { Not } from 'typeorm'; + +import { InternalHooks } from '@/InternalHooks'; +import { ExternalHooks } from '@/ExternalHooks'; +import { UserManagementMailer } from '@/UserManagement/email'; +import { UserRepository } from '@/databases/repositories/user.repository'; +import { PasswordUtility } from '@/services/password.utility'; + +import { + randomEmail, + randomInvalidPassword, + randomName, + randomValidPassword, +} from '../../shared/random'; +import { createMember, createOwner, createUserShell } from '../../shared/db/users'; +import { mockInstance } from '../../../shared/mocking'; +import * as utils from '../../shared/utils'; + +import { + assertReturnedUserProps, + assertStoredUserProps, + assertUserInviteResult, +} from './assertions'; + +import type { User } from '@/databases/entities/User'; +import type { UserInvitationResult } from '../../shared/utils/users'; + +describe('InvitationController', () => { + const mailer = mockInstance(UserManagementMailer); + const externalHooks = mockInstance(ExternalHooks); + const internalHooks = mockInstance(InternalHooks); + + const testServer = utils.setupTestServer({ endpointGroups: ['invitations'] }); + + let instanceOwner: User; + let userRepository: UserRepository; + + beforeAll(async () => { + userRepository = Container.get(UserRepository); + instanceOwner = await createOwner(); + }); + + beforeEach(async () => { + jest.clearAllMocks(); + await userRepository.delete({ role: Not('global:owner') }); + }); + + afterEach(() => { + jest.restoreAllMocks(); + }); + + describe('POST /invitations/:id/accept', () => { + test('should fill out a member shell', async () => { + const memberShell = await createUserShell('global:member'); + + const memberProps = { + inviterId: instanceOwner.id, + firstName: randomName(), + lastName: randomName(), + password: randomValidPassword(), + }; + + const response = await testServer.authlessAgent + .post(`/invitations/${memberShell.id}/accept`) + .send(memberProps) + .expect(200); + + const { data: returnedMember } = response.body; + + assertReturnedUserProps(returnedMember); + + expect(returnedMember.firstName).toBe(memberProps.firstName); + expect(returnedMember.lastName).toBe(memberProps.lastName); + expect(returnedMember.role).toBe('global:member'); + expect(utils.getAuthToken(response)).toBeDefined(); + + const storedMember = await userRepository.findOneByOrFail({ id: returnedMember.id }); + + expect(storedMember.firstName).toBe(memberProps.firstName); + expect(storedMember.lastName).toBe(memberProps.lastName); + expect(storedMember.password).not.toBe(memberProps.password); + }); + + test('should fill out an admin shell', async () => { + const adminShell = await createUserShell('global:admin'); + + const memberProps = { + inviterId: instanceOwner.id, + firstName: randomName(), + lastName: randomName(), + password: randomValidPassword(), + }; + + const response = await testServer.authlessAgent + .post(`/invitations/${adminShell.id}/accept`) + .send(memberProps) + .expect(200); + + const { data: returnedAdmin } = response.body; + + assertReturnedUserProps(returnedAdmin); + + expect(returnedAdmin.firstName).toBe(memberProps.firstName); + expect(returnedAdmin.lastName).toBe(memberProps.lastName); + expect(returnedAdmin.role).toBe('global:admin'); + expect(utils.getAuthToken(response)).toBeDefined(); + + const storedAdmin = await userRepository.findOneByOrFail({ id: returnedAdmin.id }); + + expect(storedAdmin.firstName).toBe(memberProps.firstName); + expect(storedAdmin.lastName).toBe(memberProps.lastName); + expect(storedAdmin.password).not.toBe(memberProps.password); + }); + + test('should fail with invalid payloads', async () => { + const memberShell = await userRepository.save({ + email: randomEmail(), + role: 'global:member', + }); + + const invalidPaylods = [ + { + firstName: randomName(), + lastName: randomName(), + password: randomValidPassword(), + }, + { + inviterId: instanceOwner.id, + firstName: randomName(), + password: randomValidPassword(), + }, + { + inviterId: instanceOwner.id, + firstName: randomName(), + password: randomValidPassword(), + }, + { + inviterId: instanceOwner.id, + firstName: randomName(), + lastName: randomName(), + }, + { + inviterId: instanceOwner.id, + firstName: randomName(), + lastName: randomName(), + password: randomInvalidPassword(), + }, + ]; + + for (const payload of invalidPaylods) { + await testServer.authlessAgent + .post(`/invitations/${memberShell.id}/accept`) + .send(payload) + .expect(400); + + const storedMemberShell = await userRepository.findOneByOrFail({ + email: memberShell.email, + }); + + expect(storedMemberShell.firstName).toBeNull(); + expect(storedMemberShell.lastName).toBeNull(); + expect(storedMemberShell.password).toBeNull(); + } + }); + + test('should fail with already accepted invite', async () => { + const member = await createMember(); + + const memberProps = { + inviterId: instanceOwner.id, + firstName: randomName(), + lastName: randomName(), + password: randomValidPassword(), + }; + + await testServer.authlessAgent + .post(`/invitations/${member.id}/accept`) + .send(memberProps) + .expect(400); + + const storedMember = await userRepository.findOneByOrFail({ + email: member.email, + }); + + expect(storedMember.firstName).not.toBe(memberProps.firstName); + expect(storedMember.lastName).not.toBe(memberProps.lastName); + expect(storedMember.password).not.toBe(memberProps.password); + + const comparisonResult = await Container.get(PasswordUtility).compare( + member.password, + storedMember.password, + ); + + expect(comparisonResult).toBe(false); + }); + }); + + describe('POST /invitations', () => { + type InvitationResponse = { body: { data: UserInvitationResult[] } }; + + test('should fail with invalid payloads', async () => { + const invalidPayloads = [ + randomEmail(), + [randomEmail()], + {}, + [{ name: randomName() }], + [{ email: randomName() }], + ]; + + for (const invalidPayload of invalidPayloads) { + await testServer + .authAgentFor(instanceOwner) + .post('/invitations') + .send(invalidPayload) + .expect(400); + + await expect(userRepository.count()).resolves.toBe(2); // DB unaffected + } + }); + + test('should return 200 on empty payload', async () => { + const response = await testServer + .authAgentFor(instanceOwner) + .post('/invitations') + .send([]) + .expect(200); + + expect(response.body.data).toStrictEqual([]); + + await expect(userRepository.count()).resolves.toBe(2); // DB unaffected + }); + + test('should return 200 if emailing is not set up', async () => { + mailer.invite.mockResolvedValue({ emailSent: false }); + + const response = await testServer + .authAgentFor(instanceOwner) + .post('/invitations') + .send([{ email: randomEmail() }]); + + expect(response.body.data).toBeInstanceOf(Array); + expect(response.body.data.length).toBe(1); + + const { user } = response.body.data[0]; + + expect(user.inviteAcceptUrl).toBeDefined(); + + const inviteUrl = new URL(user.inviteAcceptUrl); + + expect(inviteUrl.searchParams.get('inviterId')).toBe(instanceOwner.id); + expect(inviteUrl.searchParams.get('inviteeId')).toBe(user.id); + }); + + test('should create member shell', async () => { + mailer.invite.mockResolvedValue({ emailSent: false }); + + const response: InvitationResponse = await testServer + .authAgentFor(instanceOwner) + .post('/invitations') + .send([{ email: randomEmail() }]) + .expect(200); + + const [result] = response.body.data; + + const storedUser = await userRepository.findOneByOrFail({ + id: result.user.id, + }); + + assertStoredUserProps(storedUser); + }); + + test('should create admin shell when advanced permissions is licensed', async () => { + testServer.license.enable('feat:advancedPermissions'); + + mailer.invite.mockResolvedValue({ emailSent: false }); + + const response: InvitationResponse = await testServer + .authAgentFor(instanceOwner) + .post('/invitations') + .send([{ email: randomEmail(), role: 'global:admin' }]) + .expect(200); + + const [result] = response.body.data; + + const storedUser = await userRepository.findOneByOrFail({ + id: result.user.id, + }); + + assertStoredUserProps(storedUser); + }); + + test('should reinvite member when sharing is licensed', async () => { + testServer.license.enable('feat:sharing'); + + mailer.invite.mockResolvedValue({ emailSent: false }); + + await testServer + .authAgentFor(instanceOwner) + .post('/invitations') + .send([{ email: randomEmail(), role: 'global:member' }]); + + await testServer + .authAgentFor(instanceOwner) + .post('/invitations') + .send([{ email: randomEmail(), role: 'global:member' }]) + .expect(200); + }); + + test('should reinvite admin when advanced permissions is licensed', async () => { + testServer.license.enable('feat:advancedPermissions'); + + mailer.invite.mockResolvedValue({ emailSent: false }); + + await testServer + .authAgentFor(instanceOwner) + .post('/invitations') + .send([{ email: randomEmail(), role: 'global:admin' }]); + + await testServer + .authAgentFor(instanceOwner) + .post('/invitations') + .send([{ email: randomEmail(), role: 'global:admin' }]) + .expect(200); + }); + + test('should return 403 on creating admin shell when advanced permissions is unlicensed', async () => { + testServer.license.disable('feat:advancedPermissions'); + + mailer.invite.mockResolvedValue({ emailSent: false }); + + await testServer + .authAgentFor(instanceOwner) + .post('/invitations') + .send([{ email: randomEmail(), role: 'global:admin' }]) + .expect(403); + }); + + test('should email invites and create user shells, without inviting existing users', async () => { + mailer.invite.mockResolvedValue({ emailSent: true }); + + const member = await createMember(); + const memberShell = await createUserShell('global:member'); + const newUserEmail = randomEmail(); + + const existingUserEmails = [member.email]; + const inviteeUserEmails = [memberShell.email, newUserEmail]; + const payload = inviteeUserEmails.concat(existingUserEmails).map((email) => ({ email })); + + const response: InvitationResponse = await testServer + .authAgentFor(instanceOwner) + .post('/invitations') + .send(payload) + .expect(200); + + // invite results + + const { data: results } = response.body; + + for (const result of results) { + assertUserInviteResult(result); + + const storedUser = await Container.get(UserRepository).findOneByOrFail({ + id: result.user.id, + }); + + assertStoredUserProps(storedUser); + } + + // external hooks + + expect(externalHooks.run).toHaveBeenCalledTimes(1); + + const [externalHookName, externalHookArg] = externalHooks.run.mock.calls[0]; + + expect(externalHookName).toBe('user.invited'); + expect(externalHookArg?.[0]).toStrictEqual([newUserEmail]); + + // internal hooks + + const calls = mocked(internalHooks).onUserTransactionalEmail.mock.calls; + + for (const [onUserTransactionalEmailArg] of calls) { + expect(onUserTransactionalEmailArg.user_id).toBeDefined(); + expect(onUserTransactionalEmailArg.message_type).toBe('New user invite'); + expect(onUserTransactionalEmailArg.public_api).toBe(false); + } + }); + + test('should return 200 and surface error when invite method throws error', async () => { + const errorMsg = 'Failed to send email'; + + mailer.invite.mockImplementation(async () => { + throw new Error(errorMsg); + }); + + const response: InvitationResponse = await testServer + .authAgentFor(instanceOwner) + .post('/invitations') + .send([{ email: randomEmail() }]) + .expect(200); + + expect(response.body.data).toBeInstanceOf(Array); + expect(response.body.data.length).toBe(1); + + const [result] = response.body.data; + + expect(result.error).toBe(errorMsg); + }); + }); +}); diff --git a/packages/cli/test/integration/credentials.ee.test.ts b/packages/cli/test/integration/credentials.ee.test.ts index 056d0f5329a10..be9fc74be4e9c 100644 --- a/packages/cli/test/integration/credentials.ee.test.ts +++ b/packages/cli/test/integration/credentials.ee.test.ts @@ -17,6 +17,7 @@ import { createManyUsers, createUser, createUserShell } from './shared/db/users' import { UserManagementMailer } from '@/UserManagement/email'; import { mockInstance } from '../shared/mocking'; +import config from '@/config'; const sharingSpy = jest.spyOn(License.prototype, 'isSharingEnabled').mockReturnValue(true); const testServer = utils.setupTestServer({ endpointGroups: ['credentials'] }); @@ -489,7 +490,6 @@ describe('PUT /credentials/:id/share', () => { expect(sharedCredentials).toHaveLength(1); expect(sharedCredentials[0].userId).toBe(owner.id); - expect(mailer.notifyCredentialsShared).toHaveBeenCalledTimes(0); }); test('should ignore non-existing sharee', async () => { @@ -507,7 +507,7 @@ describe('PUT /credentials/:id/share', () => { expect(sharedCredentials).toHaveLength(1); expect(sharedCredentials[0].userId).toBe(owner.id); - expect(mailer.notifyCredentialsShared).toHaveBeenCalledTimes(0); + expect(mailer.notifyCredentialsShared).toHaveBeenCalledTimes(1); }); test('should respond 400 if invalid payload is provided', async () => { @@ -542,7 +542,27 @@ describe('PUT /credentials/:id/share', () => { expect(sharedCredentials).toHaveLength(1); expect(sharedCredentials[0].userId).toBe(owner.id); - expect(mailer.notifyCredentialsShared).toHaveBeenCalledTimes(0); + expect(mailer.notifyCredentialsShared).toHaveBeenCalledTimes(1); + }); + + test('should not call internal hooks listener for email sent if emailing is disabled', async () => { + config.set('userManagement.emails.mode', ''); + + const savedCredential = await saveCredential(randomCredentialPayload(), { user: owner }); + + const [member1, member2] = await createManyUsers(2, { + role: 'global:member', + }); + + await shareCredentialWithUsers(savedCredential, [member1, member2]); + + const response = await authOwnerAgent + .put(`/credentials/${savedCredential.id}/share`) + .send({ shareWithIds: [] }); + + expect(response.statusCode).toBe(200); + + config.set('userManagement.emails.mode', 'smtp'); }); }); diff --git a/packages/cli/test/integration/invitations.api.test.ts b/packages/cli/test/integration/invitations.api.test.ts deleted file mode 100644 index 4ca48712ea829..0000000000000 --- a/packages/cli/test/integration/invitations.api.test.ts +++ /dev/null @@ -1,443 +0,0 @@ -import validator from 'validator'; -import type { SuperAgentTest } from 'supertest'; - -import type { User } from '@db/entities/User'; -import { PasswordUtility } from '@/services/password.utility'; -import { UserManagementMailer } from '@/UserManagement/email/UserManagementMailer'; - -import Container from 'typedi'; -import { UserRepository } from '@db/repositories/user.repository'; - -import { mockInstance } from '../shared/mocking'; -import { - randomEmail, - randomInvalidPassword, - randomName, - randomValidPassword, -} from './shared/random'; -import * as testDb from './shared/testDb'; -import * as utils from './shared/utils/'; -import { createMember, createOwner, createUser, createUserShell } from './shared/db/users'; -import { ExternalHooks } from '@/ExternalHooks'; -import { InternalHooks } from '@/InternalHooks'; -import type { UserInvitationResponse } from './shared/utils/users'; -import { - assertInviteUserSuccessResponse, - assertInvitedUsersOnDb, - assertInviteUserErrorResponse, -} from './shared/utils/users'; -import { mocked } from 'jest-mock'; -import { License } from '@/License'; - -mockInstance(InternalHooks); - -const license = mockInstance(License, { - isAdvancedPermissionsLicensed: jest.fn().mockReturnValue(true), - isWithinUsersLimit: jest.fn().mockReturnValue(true), -}); - -const externalHooks = mockInstance(ExternalHooks); -const mailer = mockInstance(UserManagementMailer, { isEmailSetUp: true }); - -const testServer = utils.setupTestServer({ endpointGroups: ['invitations'] }); - -describe('POST /invitations/:id/accept', () => { - let owner: User; - - let authlessAgent: SuperAgentTest; - - beforeAll(async () => { - await testDb.truncate(['User']); - - owner = await createOwner(); - - authlessAgent = testServer.authlessAgent; - }); - - test('should fill out a member shell', async () => { - const memberShell = await createUserShell('global:member'); - - const memberData = { - inviterId: owner.id, - firstName: randomName(), - lastName: randomName(), - password: randomValidPassword(), - }; - - const response = await authlessAgent - .post(`/invitations/${memberShell.id}/accept`) - .send(memberData) - .expect(200); - - const { - id, - email, - firstName, - lastName, - personalizationAnswers, - password, - role, - isPending, - apiKey, - globalScopes, - } = response.body.data; - - expect(validator.isUUID(id)).toBe(true); - expect(email).toBeDefined(); - expect(firstName).toBe(memberData.firstName); - expect(lastName).toBe(memberData.lastName); - expect(personalizationAnswers).toBeNull(); - expect(password).toBeUndefined(); - expect(isPending).toBe(false); - expect(role).toBe('global:member'); - expect(apiKey).not.toBeDefined(); - expect(globalScopes).toBeDefined(); - expect(globalScopes).not.toHaveLength(0); - - const authToken = utils.getAuthToken(response); - expect(authToken).toBeDefined(); - - const storedMember = await Container.get(UserRepository).findOneByOrFail({ - id: memberShell.id, - }); - - expect(storedMember.firstName).toBe(memberData.firstName); - expect(storedMember.lastName).toBe(memberData.lastName); - expect(storedMember.password).not.toBe(memberData.password); - }); - - test('should fill out an admin shell', async () => { - const adminShell = await createUserShell('global:admin'); - - const memberData = { - inviterId: owner.id, - firstName: randomName(), - lastName: randomName(), - password: randomValidPassword(), - }; - - const response = await authlessAgent - .post(`/invitations/${adminShell.id}/accept`) - .send(memberData) - .expect(200); - - const { - id, - email, - firstName, - lastName, - personalizationAnswers, - password, - role, - isPending, - apiKey, - globalScopes, - } = response.body.data; - - expect(validator.isUUID(id)).toBe(true); - expect(email).toBeDefined(); - expect(firstName).toBe(memberData.firstName); - expect(lastName).toBe(memberData.lastName); - expect(personalizationAnswers).toBeNull(); - expect(password).toBeUndefined(); - expect(isPending).toBe(false); - expect(role).toBe('global:admin'); - expect(apiKey).not.toBeDefined(); - expect(globalScopes).toBeDefined(); - expect(globalScopes).not.toHaveLength(0); - - const authToken = utils.getAuthToken(response); - expect(authToken).toBeDefined(); - - const storedAdmin = await Container.get(UserRepository).findOneByOrFail({ - id: adminShell.id, - }); - - expect(storedAdmin.firstName).toBe(memberData.firstName); - expect(storedAdmin.lastName).toBe(memberData.lastName); - expect(storedAdmin.password).not.toBe(memberData.password); - }); - - test('should fail with invalid payloads', async () => { - const memberShellEmail = randomEmail(); - - const memberShell = await Container.get(UserRepository).save({ - email: memberShellEmail, - role: 'global:member', - }); - - const invalidPayloads = [ - { - firstName: randomName(), - lastName: randomName(), - password: randomValidPassword(), - }, - { - inviterId: owner.id, - firstName: randomName(), - password: randomValidPassword(), - }, - { - inviterId: owner.id, - firstName: randomName(), - password: randomValidPassword(), - }, - { - inviterId: owner.id, - firstName: randomName(), - lastName: randomName(), - }, - { - inviterId: owner.id, - firstName: randomName(), - lastName: randomName(), - password: randomInvalidPassword(), - }, - ]; - - for (const invalidPayload of invalidPayloads) { - await authlessAgent - .post(`/invitations/${memberShell.id}/accept`) - .send(invalidPayload) - .expect(400); - - const storedMemberShell = await Container.get(UserRepository).findOneOrFail({ - where: { email: memberShellEmail }, - }); - - expect(storedMemberShell.firstName).toBeNull(); - expect(storedMemberShell.lastName).toBeNull(); - expect(storedMemberShell.password).toBeNull(); - } - }); - - test('should fail with already accepted invite', async () => { - const member = await createUser({ role: 'global:member' }); - - const memberData = { - inviterId: owner.id, - firstName: randomName(), - lastName: randomName(), - password: randomValidPassword(), - }; - - await authlessAgent.post(`/invitations/${member.id}/accept`).send(memberData).expect(400); - - const storedMember = await Container.get(UserRepository).findOneOrFail({ - where: { email: member.email }, - }); - - expect(storedMember.firstName).not.toBe(memberData.firstName); - expect(storedMember.lastName).not.toBe(memberData.lastName); - expect(storedMember.password).not.toBe(memberData.password); - - const comparisonResult = await Container.get(PasswordUtility).compare( - member.password, - storedMember.password, - ); - - expect(comparisonResult).toBe(false); - }); -}); - -describe('POST /invitations', () => { - let owner: User; - let member: User; - let ownerAgent: SuperAgentTest; - - beforeAll(async () => { - await testDb.truncate(['User']); - - owner = await createOwner(); - member = await createMember(); - ownerAgent = testServer.authAgentFor(owner); - }); - - test('should fail with invalid payloads', async () => { - const invalidPayloads = [ - randomEmail(), - [randomEmail()], - {}, - [{ name: randomName() }], - [{ email: randomName() }], - ]; - - await Promise.all( - invalidPayloads.map(async (invalidPayload) => { - await ownerAgent.post('/invitations').send(invalidPayload).expect(400); - - const usersCount = await Container.get(UserRepository).count(); - - expect(usersCount).toBe(2); // DB unaffected - }), - ); - }); - - test('should return 200 on empty payload', async () => { - const response = await ownerAgent.post('/invitations').send([]).expect(200); - - expect(response.body.data).toStrictEqual([]); - - const usersCount = await Container.get(UserRepository).count(); - - expect(usersCount).toBe(2); - }); - - test('should return 200 if emailing is not set up', async () => { - mailer.invite.mockResolvedValue({ emailSent: false }); - - const response = await ownerAgent - .post('/invitations') - .send([{ email: randomEmail() }]) - .expect(200); - - expect(response.body.data).toBeInstanceOf(Array); - expect(response.body.data.length).toBe(1); - - const { user } = response.body.data[0]; - - expect(user.inviteAcceptUrl).toBeDefined(); - - const inviteUrl = new URL(user.inviteAcceptUrl); - - expect(inviteUrl.searchParams.get('inviterId')).toBe(owner.id); - expect(inviteUrl.searchParams.get('inviteeId')).toBe(user.id); - }); - - test('should create member shell', async () => { - mailer.invite.mockResolvedValue({ emailSent: false }); - - const response = await ownerAgent - .post('/invitations') - .send([{ email: randomEmail() }]) - .expect(200); - - const [result] = response.body.data as UserInvitationResponse[]; - - const storedUser = await Container.get(UserRepository).findOneByOrFail({ - id: result.user.id, - }); - - assertInvitedUsersOnDb(storedUser); - }); - - test('should create admin shell if licensed', async () => { - mailer.invite.mockResolvedValue({ emailSent: false }); - - const response = await ownerAgent - .post('/invitations') - .send([{ email: randomEmail(), role: 'global:admin' }]) - .expect(200); - - const [result] = response.body.data as UserInvitationResponse[]; - - const storedUser = await Container.get(UserRepository).findOneByOrFail({ - id: result.user.id, - }); - - assertInvitedUsersOnDb(storedUser); - }); - - test('should reinvite member', async () => { - mailer.invite.mockResolvedValue({ emailSent: false }); - - await ownerAgent.post('/invitations').send([{ email: randomEmail(), role: 'global:member' }]); - - await ownerAgent - .post('/invitations') - .send([{ email: randomEmail(), role: 'global:member' }]) - .expect(200); - }); - - test('should reinvite admin if licensed', async () => { - license.isAdvancedPermissionsLicensed.mockReturnValue(true); - mailer.invite.mockResolvedValue({ emailSent: false }); - - await ownerAgent.post('/invitations').send([{ email: randomEmail(), role: 'global:admin' }]); - - await ownerAgent - .post('/invitations') - .send([{ email: randomEmail(), role: 'global:admin' }]) - .expect(200); - }); - - test('should fail to create admin shell if not licensed', async () => { - license.isAdvancedPermissionsLicensed.mockReturnValue(false); - mailer.invite.mockResolvedValue({ emailSent: false }); - - await ownerAgent - .post('/invitations') - .send([{ email: randomEmail(), role: 'global:admin' }]) - .expect(403); - }); - - test('should email invites and create user shells but ignore existing', async () => { - externalHooks.run.mockClear(); - - mailer.invite.mockResolvedValue({ emailSent: true }); - - const memberShell = await createUserShell('global:member'); - - const newUser = randomEmail(); - - const shellUsers = [memberShell.email]; - const usersToInvite = [newUser, ...shellUsers]; - const usersToCreate = [newUser]; - const existingUsers = [member.email]; - - const testEmails = [...usersToInvite, ...existingUsers]; - - const payload = testEmails.map((email) => ({ email })); - - const response = await ownerAgent.post('/invitations').send(payload).expect(200); - - const internalHooks = Container.get(InternalHooks); - - expect(internalHooks.onUserTransactionalEmail).toHaveBeenCalledTimes(usersToInvite.length); - - expect(externalHooks.run).toHaveBeenCalledTimes(1); - - const [hookName, hookData] = externalHooks.run.mock.calls[0]; - - expect(hookName).toBe('user.invited'); - expect(hookData?.[0]).toStrictEqual(usersToCreate); - - const result = response.body.data as UserInvitationResponse[]; - - for (const invitationResponse of result) { - assertInviteUserSuccessResponse(invitationResponse); - - const storedUser = await Container.get(UserRepository).findOneByOrFail({ - id: invitationResponse.user.id, - }); - - assertInvitedUsersOnDb(storedUser); - } - - const calls = mocked(internalHooks).onUserTransactionalEmail.mock.calls; - - for (const [onUserTransactionalEmailParameter] of calls) { - expect(onUserTransactionalEmailParameter.user_id).toBeDefined(); - expect(onUserTransactionalEmailParameter.message_type).toBe('New user invite'); - expect(onUserTransactionalEmailParameter.public_api).toBe(false); - } - }); - - test('should return 200 when invite method throws error', async () => { - mailer.invite.mockImplementation(async () => { - throw new Error('failed to send email'); - }); - - const response = await ownerAgent - .post('/invitations') - .send([{ email: randomEmail() }]) - .expect(200); - - expect(response.body.data).toBeInstanceOf(Array); - expect(response.body.data.length).toBe(1); - - const [invitationResponse] = response.body.data; - - assertInviteUserErrorResponse(invitationResponse); - }); -}); diff --git a/packages/cli/test/integration/shared/utils/users.ts b/packages/cli/test/integration/shared/utils/users.ts index c655f754b2b58..e812558674f7f 100644 --- a/packages/cli/test/integration/shared/utils/users.ts +++ b/packages/cli/test/integration/shared/utils/users.ts @@ -1,4 +1,3 @@ -import validator from 'validator'; import type { PublicUser } from '@/Interfaces'; import type { User } from '@/databases/entities/User'; @@ -16,30 +15,7 @@ export const validateUser = (user: PublicUser) => { expect(user.role).toBeDefined(); }; -export const assertInviteUserSuccessResponse = (data: UserInvitationResponse) => { - expect(validator.isUUID(data.user.id)).toBe(true); - expect(data.user.inviteAcceptUrl).toBeUndefined(); - expect(data.user.email).toBeDefined(); - expect(data.user.emailSent).toBe(true); -}; - -export const assertInviteUserErrorResponse = (data: UserInvitationResponse) => { - expect(validator.isUUID(data.user.id)).toBe(true); - expect(data.user.inviteAcceptUrl).toBeDefined(); - expect(data.user.email).toBeDefined(); - expect(data.user.emailSent).toBe(false); - expect(data.error).toBeDefined(); -}; - -export const assertInvitedUsersOnDb = (user: User) => { - expect(user.firstName).toBeNull(); - expect(user.lastName).toBeNull(); - expect(user.personalizationAnswers).toBeNull(); - expect(user.password).toBeNull(); - expect(user.isPending).toBe(true); -}; - -export type UserInvitationResponse = { +export type UserInvitationResult = { user: Pick & { inviteAcceptUrl: string; emailSent: boolean }; error?: string; }; diff --git a/packages/cli/test/integration/workflows/workflows.controller.ee.test.ts b/packages/cli/test/integration/workflows/workflows.controller.ee.test.ts index e24973e0134be..ed3e13d9f063d 100644 --- a/packages/cli/test/integration/workflows/workflows.controller.ee.test.ts +++ b/packages/cli/test/integration/workflows/workflows.controller.ee.test.ts @@ -19,6 +19,7 @@ import { createUser } from '../shared/db/users'; import { createWorkflow, getWorkflowSharing, shareWorkflowWithUsers } from '../shared/db/workflows'; import { License } from '@/License'; import { UserManagementMailer } from '@/UserManagement/email'; +import config from '@/config'; let owner: User; let member: User; @@ -149,7 +150,7 @@ describe('PUT /workflows/:id', () => { const secondSharedWorkflows = await getWorkflowSharing(workflow); expect(secondSharedWorkflows).toHaveLength(2); - expect(mailer.notifyWorkflowShared).toHaveBeenCalledTimes(1); + expect(mailer.notifyWorkflowShared).toHaveBeenCalledTimes(2); }); test('PUT /workflows/:id/share should allow sharing by the owner of the workflow', async () => { @@ -225,6 +226,20 @@ describe('PUT /workflows/:id', () => { expect(sharedWorkflows).toHaveLength(1); expect(mailer.notifyWorkflowShared).toHaveBeenCalledTimes(0); }); + + test('should not call internal hooks listener for email sent if emailing is disabled', async () => { + config.set('userManagement.emails.mode', ''); + + const workflow = await createWorkflow({}, owner); + + const response = await authOwnerAgent + .put(`/workflows/${workflow.id}/share`) + .send({ shareWithIds: [member.id] }); + + expect(response.statusCode).toBe(200); + + config.set('userManagement.emails.mode', 'smtp'); + }); }); describe('GET /workflows/new', () => { diff --git a/packages/cli/test/unit/services/user.service.test.ts b/packages/cli/test/unit/services/user.service.test.ts index dca81305af44c..6ab4f79c42911 100644 --- a/packages/cli/test/unit/services/user.service.test.ts +++ b/packages/cli/test/unit/services/user.service.test.ts @@ -7,11 +7,13 @@ import { UserRepository } from '@db/repositories/user.repository'; import { UserService } from '@/services/user.service'; import { mockInstance } from '../../shared/mocking'; import { v4 as uuid } from 'uuid'; +import { InternalHooks } from '@/InternalHooks'; describe('UserService', () => { config.set('userManagement.jwtSecret', 'random-secret'); mockInstance(Logger); + mockInstance(InternalHooks); const userRepository = mockInstance(UserRepository); const userService = Container.get(UserService); From eb27ed068ba21bbf4302686f0f0c0168e91c03f6 Mon Sep 17 00:00:00 2001 From: Csaba Tuncsik Date: Mon, 29 Jan 2024 16:34:10 +0100 Subject: [PATCH 2/6] fix(editor): Disable expression editor modal opening on readonly field (#8457) --- .../ExpressionEditorModalInput.vue | 15 ++++++- .../ExpressionEditorModalOutput.vue | 1 + .../components/ExpressionParameterInput.vue | 29 +++++++------- .../InlineExpressionEditorInput.vue | 18 ++++++++- .../ExpressionEditorModalInput.test.ts | 39 +++++++++++++++++++ .../ExpressionParameterInput.test.ts | 36 +++++++++++++++++ 6 files changed, 118 insertions(+), 20 deletions(-) create mode 100644 packages/editor-ui/src/components/__tests__/ExpressionEditorModalInput.test.ts create mode 100644 packages/editor-ui/src/components/__tests__/ExpressionParameterInput.test.ts diff --git a/packages/editor-ui/src/components/ExpressionEditorModal/ExpressionEditorModalInput.vue b/packages/editor-ui/src/components/ExpressionEditorModal/ExpressionEditorModalInput.vue index bccb83c72b632..52bac85329ceb 100644 --- a/packages/editor-ui/src/components/ExpressionEditorModal/ExpressionEditorModalInput.vue +++ b/packages/editor-ui/src/components/ExpressionEditorModal/ExpressionEditorModalInput.vue @@ -1,5 +1,5 @@ - + + diff --git a/packages/editor-ui/src/components/ExpressionEditorModal/ExpressionEditorModalOutput.vue b/packages/editor-ui/src/components/ExpressionEditorModal/ExpressionEditorModalOutput.vue index de1d60abecf43..0f79ef4ee6faf 100644 --- a/packages/editor-ui/src/components/ExpressionEditorModal/ExpressionEditorModalOutput.vue +++ b/packages/editor-ui/src/components/ExpressionEditorModal/ExpressionEditorModalOutput.vue @@ -75,6 +75,7 @@ export default defineComponent({ outputTheme(), EditorState.readOnly.of(true), EditorView.lineWrapping, + EditorView.editable.of(false), EditorView.domEventHandlers({ scroll: forceParse }), ]; diff --git a/packages/editor-ui/src/components/ExpressionParameterInput.vue b/packages/editor-ui/src/components/ExpressionParameterInput.vue index 091c04e26621a..be5a0f7ef81be 100644 --- a/packages/editor-ui/src/components/ExpressionParameterInput.vue +++ b/packages/editor-ui/src/components/ExpressionParameterInput.vue @@ -20,8 +20,11 @@ @blur="onBlur" @change="onChange" /> - -
+
- + + + diff --git a/packages/editor-ui/src/components/__tests__/ExpressionEditorModalInput.test.ts b/packages/editor-ui/src/components/__tests__/ExpressionEditorModalInput.test.ts new file mode 100644 index 0000000000000..3793d3d760483 --- /dev/null +++ b/packages/editor-ui/src/components/__tests__/ExpressionEditorModalInput.test.ts @@ -0,0 +1,39 @@ +import userEvent from '@testing-library/user-event'; +import { createComponentRenderer } from '@/__tests__/render'; +import ExpressionEditorModalInput from '@/components/ExpressionEditorModal/ExpressionEditorModalInput.vue'; + +const renderComponent = createComponentRenderer(ExpressionEditorModalInput); + +const originalRangeGetBoundingClientRect = Range.prototype.getBoundingClientRect; +const originalRangeGetClientRects = Range.prototype.getClientRects; + +describe('ExpressionParameterInput', () => { + beforeAll(() => { + Range.prototype.getBoundingClientRect = vi.fn(); + Range.prototype.getClientRects = () => ({ + item: vi.fn(), + length: 0, + [Symbol.iterator]: vi.fn(), + }); + }); + + afterAll(() => { + Range.prototype.getBoundingClientRect = originalRangeGetBoundingClientRect; + Range.prototype.getClientRects = originalRangeGetClientRects; + }); + test.each([ + ['not be editable', 'readonly', true, ''], + ['be editable', 'not readonly', false, 'test'], + ])('should %s when %s', async (_, __, isReadOnly, expected) => { + const { getByRole } = renderComponent({ + props: { + modelValue: '', + path: '', + isReadOnly, + }, + }); + + await userEvent.type(getByRole('textbox'), 'test'); + expect(getByRole('textbox')).toHaveTextContent(expected); + }); +}); diff --git a/packages/editor-ui/src/components/__tests__/ExpressionParameterInput.test.ts b/packages/editor-ui/src/components/__tests__/ExpressionParameterInput.test.ts new file mode 100644 index 0000000000000..43e5d73eb8b9b --- /dev/null +++ b/packages/editor-ui/src/components/__tests__/ExpressionParameterInput.test.ts @@ -0,0 +1,36 @@ +import { createPinia, setActivePinia } from 'pinia'; +import userEvent from '@testing-library/user-event'; +import { createComponentRenderer } from '@/__tests__/render'; +import { useWorkflowsStore } from '@/stores/workflows.store'; +import { useNDVStore } from '@/stores/ndv.store'; +import ExpressionParameterInput from '@/components/ExpressionParameterInput.vue'; + +const renderComponent = createComponentRenderer(ExpressionParameterInput); + +let pinia: ReturnType; +let workflowsStore: ReturnType; +let ndvStore: ReturnType; + +describe('ExpressionParameterInput', () => { + beforeEach(() => { + pinia = createPinia(); + setActivePinia(pinia); + workflowsStore = useWorkflowsStore(); + ndvStore = useNDVStore(); + }); + + test.each([ + ['not readonly', false, expect.anything()], + ['readonly', true, expect.anything()], + ])('should emit open expression editor modal when %s', async (_, isReadOnly, expected) => { + const { getByTestId, emitted } = renderComponent({ + props: { + modelValue: '', + isReadOnly, + }, + }); + + await userEvent.click(getByTestId('expander')); + expect(emitted().modalOpenerClick).toEqual(expected); + }); +}); From 981ea3930e96c3b45267fa7ddac48710846e49ac Mon Sep 17 00:00:00 2001 From: oleg Date: Mon, 29 Jan 2024 16:34:23 +0100 Subject: [PATCH 3/6] feat: Add model parameter to OpenAI embeddings (#8481) Signed-off-by: Oleg Ivaniv --- .../EmbeddingsOpenAI/EmbeddingsOpenAi.node.ts | 73 +++++++++++++++++++ 1 file changed, 73 insertions(+) diff --git a/packages/@n8n/nodes-langchain/nodes/embeddings/EmbeddingsOpenAI/EmbeddingsOpenAi.node.ts b/packages/@n8n/nodes-langchain/nodes/embeddings/EmbeddingsOpenAI/EmbeddingsOpenAi.node.ts index 8154559412a0f..edc4a48679557 100644 --- a/packages/@n8n/nodes-langchain/nodes/embeddings/EmbeddingsOpenAI/EmbeddingsOpenAi.node.ts +++ b/packages/@n8n/nodes-langchain/nodes/embeddings/EmbeddingsOpenAI/EmbeddingsOpenAi.node.ts @@ -5,6 +5,7 @@ import { type INodeType, type INodeTypeDescription, type SupplyData, + type INodeProperties, } from 'n8n-workflow'; import type { ClientOptions } from 'openai'; @@ -12,6 +13,60 @@ import { OpenAIEmbeddings } from 'langchain/embeddings/openai'; import { logWrapper } from '../../../utils/logWrapper'; import { getConnectionHintNoticeField } from '../../../utils/sharedFields'; +const modelParameter: INodeProperties = { + displayName: 'Model', + name: 'model', + type: 'options', + description: + 'The model which will generate the embeddings. Learn more.', + typeOptions: { + loadOptions: { + routing: { + request: { + method: 'GET', + url: '={{ $parameter.options?.baseURL?.split("/").slice(-1).pop() || "v1" }}/models', + }, + output: { + postReceive: [ + { + type: 'rootProperty', + properties: { + property: 'data', + }, + }, + { + type: 'filter', + properties: { + pass: "={{ $responseItem.id.includes('embed') }}", + }, + }, + { + type: 'setKeyValue', + properties: { + name: '={{$responseItem.id}}', + value: '={{$responseItem.id}}', + }, + }, + { + type: 'sort', + properties: { + key: 'name', + }, + }, + ], + }, + }, + }, + }, + routing: { + send: { + type: 'body', + property: 'model', + }, + }, + default: 'text-embedding-3-small', +}; + export class EmbeddingsOpenAi implements INodeType { description: INodeTypeDescription = { displayName: 'Embeddings OpenAI', @@ -50,6 +105,23 @@ export class EmbeddingsOpenAi implements INodeType { outputNames: ['Embeddings'], properties: [ getConnectionHintNoticeField([NodeConnectionType.AiVectorStore]), + { + ...modelParameter, + default: 'text-embedding-ada-002', + displayOptions: { + show: { + '@version': [1], + }, + }, + }, + { + ...modelParameter, + displayOptions: { + hide: { + '@version': [1], + }, + }, + }, { displayName: 'Options', name: 'options', @@ -115,6 +187,7 @@ export class EmbeddingsOpenAi implements INodeType { const embeddings = new OpenAIEmbeddings( { + modelName: this.getNodeParameter('model', itemIndex, 'text-embedding-3-small') as string, openAIApiKey: credentials.apiKey as string, ...options, }, From 238b54c77bba6f7abcc7fc2b3ac48a85206ce37e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Iv=C3=A1n=20Ovejero?= Date: Mon, 29 Jan 2024 16:34:58 +0100 Subject: [PATCH 4/6] fix(core): Fix stopping and retrying failed executions (#8480) --- .../src/databases/repositories/execution.repository.ts | 10 ++++++++++ .../cli/src/executions/active-execution.service.ts | 2 +- packages/cli/src/executions/execution.service.ts | 4 ++-- 3 files changed, 13 insertions(+), 3 deletions(-) diff --git a/packages/cli/src/databases/repositories/execution.repository.ts b/packages/cli/src/databases/repositories/execution.repository.ts index 3f17767d0f7d3..4a0f712be216b 100644 --- a/packages/cli/src/databases/repositories/execution.repository.ts +++ b/packages/cli/src/databases/repositories/execution.repository.ts @@ -649,6 +649,16 @@ export class ExecutionRepository extends Repository { }); } + async findWithUnflattenedData(executionId: string, accessibleWorkflowIds: string[]) { + return await this.findSingleExecution(executionId, { + where: { + workflowId: In(accessibleWorkflowIds), + }, + includeData: true, + unflattenData: true, + }); + } + async findIfShared(executionId: string, sharedWorkflowIds: string[]) { return await this.findSingleExecution(executionId, { where: { diff --git a/packages/cli/src/executions/active-execution.service.ts b/packages/cli/src/executions/active-execution.service.ts index 335c0a7faf661..4f5918378b66f 100644 --- a/packages/cli/src/executions/active-execution.service.ts +++ b/packages/cli/src/executions/active-execution.service.ts @@ -104,7 +104,7 @@ export class ActiveExecutionService { }; } - if (!this.isRegularMode) return await this.waitTracker.stopExecution(execution.id); + if (this.isRegularMode) return await this.waitTracker.stopExecution(execution.id); // queue mode diff --git a/packages/cli/src/executions/execution.service.ts b/packages/cli/src/executions/execution.service.ts index 174b7ff44647c..8d78c17d66d23 100644 --- a/packages/cli/src/executions/execution.service.ts +++ b/packages/cli/src/executions/execution.service.ts @@ -179,10 +179,10 @@ export class ExecutionService { async retry(req: ExecutionRequest.Retry, sharedWorkflowIds: string[]) { const { id: executionId } = req.params; - const execution = (await this.executionRepository.findIfShared( + const execution = await this.executionRepository.findWithUnflattenedData( executionId, sharedWorkflowIds, - )) as unknown as IExecutionResponse; + ); if (!execution) { this.logger.info( From 5cb55270b7838acb8a76f0174666dfdd89b33262 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: Tue, 30 Jan 2024 09:49:23 +0100 Subject: [PATCH 5/6] refactor(core): Replace promisify-d node calls with native promises (no-changelog) (#8464) --- .github/scripts/check-tests.mjs | 5 ++--- packages/cli/src/WebhookHelpers.ts | 5 +---- packages/cli/src/commands/start.ts | 4 +--- packages/node-dev/commands/new.ts | 9 ++------- packages/node-dev/src/Create.ts | 10 ++-------- packages/nodes-base/nodes/Crypto/Crypto.node.ts | 10 +++------- .../nodes-base/nodes/EditImage/EditImage.node.ts | 8 +++----- packages/nodes-base/nodes/Ftp/Ftp.node.ts | 14 +++++--------- 8 files changed, 19 insertions(+), 46 deletions(-) diff --git a/.github/scripts/check-tests.mjs b/.github/scripts/check-tests.mjs index 6c0f37b2aa640..1e2e38e05b064 100644 --- a/.github/scripts/check-tests.mjs +++ b/.github/scripts/check-tests.mjs @@ -1,11 +1,10 @@ -import fs from 'fs'; +import { readFile } from 'fs/promises'; import path from 'path'; import util from 'util'; import { exec } from 'child_process'; import { glob } from 'glob'; import ts from 'typescript'; -const readFileAsync = util.promisify(fs.readFile); const execAsync = util.promisify(exec); const filterAsync = async (asyncPredicate, arr) => { @@ -37,7 +36,7 @@ const isAbstractMethod = (node) => { // Function to check if a file has a function declaration, function expression, object method or class const hasFunctionOrClass = async (filePath) => { - const fileContent = await readFileAsync(filePath, 'utf-8'); + const fileContent = await readFile(filePath, 'utf-8'); const sourceFile = ts.createSourceFile(filePath, fileContent, ts.ScriptTarget.Latest, true); let hasFunctionOrClass = false; diff --git a/packages/cli/src/WebhookHelpers.ts b/packages/cli/src/WebhookHelpers.ts index 999740a19ba65..c429a06aa2f65 100644 --- a/packages/cli/src/WebhookHelpers.ts +++ b/packages/cli/src/WebhookHelpers.ts @@ -9,8 +9,7 @@ import type express from 'express'; import { Container } from 'typedi'; import get from 'lodash/get'; -import stream from 'stream'; -import { promisify } from 'util'; +import { pipeline } from 'stream/promises'; import formidable from 'formidable'; import { BinaryDataService, NodeExecuteFunctions } from 'n8n-core'; @@ -65,8 +64,6 @@ import { NotFoundError } from './errors/response-errors/not-found.error'; import { InternalServerError } from './errors/response-errors/internal-server.error'; import { UnprocessableRequestError } from './errors/response-errors/unprocessable.error'; -const pipeline = promisify(stream.pipeline); - export const WEBHOOK_METHODS: IHttpRequestMethods[] = [ 'DELETE', 'GET', diff --git a/packages/cli/src/commands/start.ts b/packages/cli/src/commands/start.ts index a1ddf03ec7442..82664ed90ef29 100644 --- a/packages/cli/src/commands/start.ts +++ b/packages/cli/src/commands/start.ts @@ -5,9 +5,8 @@ import { Flags, type Config } from '@oclif/core'; import path from 'path'; import { mkdir } from 'fs/promises'; import { createReadStream, createWriteStream, existsSync } from 'fs'; -import stream from 'stream'; +import { pipeline } from 'stream/promises'; import replaceStream from 'replacestream'; -import { promisify } from 'util'; import glob from 'fast-glob'; import { sleep, jsonParse } from 'n8n-workflow'; @@ -31,7 +30,6 @@ import { BaseCommand } from './BaseCommand'; // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment, @typescript-eslint/no-var-requires const open = require('open'); -const pipeline = promisify(stream.pipeline); export class Start extends BaseCommand { static description = 'Starts n8n. Makes Web-UI available and starts active workflows'; diff --git a/packages/node-dev/commands/new.ts b/packages/node-dev/commands/new.ts index 217ca21ba4148..0912578936c99 100644 --- a/packages/node-dev/commands/new.ts +++ b/packages/node-dev/commands/new.ts @@ -4,17 +4,12 @@ /* eslint-disable @typescript-eslint/no-unsafe-assignment */ import { Command } from '@oclif/core'; import * as changeCase from 'change-case'; -import * as fs from 'fs'; +import { access } from 'fs/promises'; import * as inquirer from 'inquirer'; import { join } from 'path'; import { createTemplate } from '../src'; -// eslint-disable-next-line @typescript-eslint/no-var-requires -const { promisify } = require('util'); - -const fsAccess = promisify(fs.access); - export class New extends Command { static description = 'Create new credentials/node'; @@ -113,7 +108,7 @@ export class New extends Command { // Check if node with the same name already exists in target folder // to not overwrite it by accident try { - await fsAccess(destinationFilePath); + await access(destinationFilePath); // File does already exist. So ask if it should be overwritten. const overwriteQuestion: inquirer.QuestionCollection = [ diff --git a/packages/node-dev/src/Create.ts b/packages/node-dev/src/Create.ts index b24fd7a5bda18..fffa2f66fc5ed 100644 --- a/packages/node-dev/src/Create.ts +++ b/packages/node-dev/src/Create.ts @@ -1,13 +1,7 @@ -import * as fs from 'fs'; +import { copyFile } from 'fs/promises'; import type { ReplaceInFileConfig } from 'replace-in-file'; import { replaceInFile } from 'replace-in-file'; -// eslint-disable-next-line @typescript-eslint/no-unsafe-assignment, @typescript-eslint/no-var-requires -const { promisify } = require('util'); - -// eslint-disable-next-line @typescript-eslint/no-unsafe-assignment, @typescript-eslint/no-unsafe-call -const fsCopyFile = promisify(fs.copyFile); - /** * Creates a new credentials or node * @@ -22,7 +16,7 @@ export async function createTemplate( ): Promise { // Copy the file to then replace the values in it // eslint-disable-next-line @typescript-eslint/no-unsafe-call - await fsCopyFile(sourceFilePath, destinationFilePath); + await copyFile(sourceFilePath, destinationFilePath); // Replace the variables in the template file const options: ReplaceInFileConfig = { diff --git a/packages/nodes-base/nodes/Crypto/Crypto.node.ts b/packages/nodes-base/nodes/Crypto/Crypto.node.ts index e95ff7ef65763..5ce4ba5dcde6f 100644 --- a/packages/nodes-base/nodes/Crypto/Crypto.node.ts +++ b/packages/nodes-base/nodes/Crypto/Crypto.node.ts @@ -1,7 +1,8 @@ import type { BinaryToTextEncoding } from 'crypto'; import { createHash, createHmac, createSign, getHashes, randomBytes } from 'crypto'; -import stream from 'stream'; -import { promisify } from 'util'; +import { pipeline } from 'stream/promises'; +import { v4 as uuid } from 'uuid'; +import set from 'lodash/set'; import type { IExecuteFunctions, INodeExecutionData, @@ -10,11 +11,6 @@ import type { JsonObject, } from 'n8n-workflow'; import { deepCopy, BINARY_ENCODING } from 'n8n-workflow'; -import set from 'lodash/set'; - -import { v4 as uuid } from 'uuid'; - -const pipeline = promisify(stream.pipeline); const unsupportedAlgorithms = [ 'RSA-MD4', diff --git a/packages/nodes-base/nodes/EditImage/EditImage.node.ts b/packages/nodes-base/nodes/EditImage/EditImage.node.ts index 7288b55776c7d..04b8e3dc9149a 100644 --- a/packages/nodes-base/nodes/EditImage/EditImage.node.ts +++ b/packages/nodes-base/nodes/EditImage/EditImage.node.ts @@ -1,6 +1,5 @@ import { parse as pathParse } from 'path'; -import { writeFile as fsWriteFile } from 'fs'; -import { promisify } from 'util'; +import { writeFile as fsWriteFile } from 'fs/promises'; import type { IDataObject, IExecuteFunctions, @@ -14,7 +13,6 @@ import type { import { deepCopy } from 'n8n-workflow'; import gm from 'gm'; import { file } from 'tmp-promise'; -const fsWriteFileAsync = promisify(fsWriteFile); import getSystemFonts from 'get-system-fonts'; const nodeOperations: INodePropertyOptions[] = [ @@ -1117,9 +1115,9 @@ export class EditImage implements INodeType { binaryPropertyName, ); - const { fd, path, cleanup } = await file(); + const { path, cleanup } = await file(); cleanupFunctions.push(cleanup); - await fsWriteFileAsync(fd, binaryDataBuffer); + await fsWriteFile(path, binaryDataBuffer); if (operations[0].operation === 'create') { // It seems like if the image gets created newly we have to create a new gm instance diff --git a/packages/nodes-base/nodes/Ftp/Ftp.node.ts b/packages/nodes-base/nodes/Ftp/Ftp.node.ts index 70f3ed726ae4d..5913605692e6c 100644 --- a/packages/nodes-base/nodes/Ftp/Ftp.node.ts +++ b/packages/nodes-base/nodes/Ftp/Ftp.node.ts @@ -1,8 +1,10 @@ import { createWriteStream } from 'fs'; import { basename, dirname } from 'path'; import type { Readable } from 'stream'; -import { pipeline } from 'stream'; -import { promisify } from 'util'; +import { pipeline } from 'stream/promises'; +import { file as tmpFile } from 'tmp-promise'; +import ftpClient from 'promise-ftp'; +import sftpClient from 'ssh2-sftp-client'; import { BINARY_ENCODING, NodeApiError } from 'n8n-workflow'; import type { ICredentialDataDecryptedObject, @@ -16,10 +18,6 @@ import type { INodeTypeDescription, JsonObject, } from 'n8n-workflow'; -import { file as tmpFile } from 'tmp-promise'; - -import ftpClient from 'promise-ftp'; -import sftpClient from 'ssh2-sftp-client'; import { formatPrivateKey } from '@utils/utilities'; interface ReturnFtpItem { @@ -40,8 +38,6 @@ interface ReturnFtpItem { path: string; } -const streamPipeline = promisify(pipeline); - async function callRecursiveList( path: string, client: sftpClient | ftpClient, @@ -722,7 +718,7 @@ export class Ftp implements INodeType { const binaryFile = await tmpFile({ prefix: 'n8n-sftp-' }); try { const stream = await ftp!.get(path); - await streamPipeline(stream, createWriteStream(binaryFile.path)); + await pipeline(stream, createWriteStream(binaryFile.path)); const dataPropertyNameDownload = this.getNodeParameter('binaryPropertyName', i); const remoteFilePath = this.getNodeParameter('path', i) as string; From 327cc8df7343b806bee87faaa86ed22d9d70127f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Milorad=20FIlipovi=C4=87?= Date: Tue, 30 Jan 2024 10:37:06 +0100 Subject: [PATCH 6/6] fix(editor): Send template id as a number in telemetry events (#8484) --- packages/editor-ui/src/mixins/workflowHelpers.ts | 2 +- .../views/SetupWorkflowFromTemplateView/setupTemplate.store.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/editor-ui/src/mixins/workflowHelpers.ts b/packages/editor-ui/src/mixins/workflowHelpers.ts index 6ea920f4efafb..30bf531665a77 100644 --- a/packages/editor-ui/src/mixins/workflowHelpers.ts +++ b/packages/editor-ui/src/mixins/workflowHelpers.ts @@ -1094,7 +1094,7 @@ export const workflowHelpers = defineComponent({ const templateId = this.$route.query.templateId; if (templateId) { this.$telemetry.track('User saved new workflow from template', { - template_id: templateId, + template_id: isNaN(+templateId) ? templateId : +templateId, workflow_id: workflowData.id, wf_template_repo_session_id: this.templatesStore.previousSessionId, }); diff --git a/packages/editor-ui/src/views/SetupWorkflowFromTemplateView/setupTemplate.store.ts b/packages/editor-ui/src/views/SetupWorkflowFromTemplateView/setupTemplate.store.ts index b0b1715792fdb..e75eb8e611211 100644 --- a/packages/editor-ui/src/views/SetupWorkflowFromTemplateView/setupTemplate.store.ts +++ b/packages/editor-ui/src/views/SetupWorkflowFromTemplateView/setupTemplate.store.ts @@ -207,7 +207,7 @@ export const useSetupTemplateStore = defineStore('setupTemplate', () => { ); telemetry.track('User saved new workflow from template', { - template_id: templateId.value, + template_id: isNaN(+templateId.value) ? templateId : +templateId.value, workflow_id: createdWorkflow.id, wf_template_repo_session_id: templatesStore.currentSessionId, });