From 0631f69d98e5420faebba1a54d9ad47a2664d110 Mon Sep 17 00:00:00 2001 From: Michael Auerswald Date: Fri, 26 May 2023 18:02:55 +0200 Subject: [PATCH] fix(core): Optimize getSharedWorkflowIds query (#6314) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: कारतोफ्फेलस्क्रिप्ट™ --- jest.config.js | 1 + packages/cli/src/WorkflowHelpers.ts | 23 ++++++++++----- packages/cli/src/config/index.ts | 12 ++++++-- .../cli/src/workflows/workflows.services.ts | 3 +- .../workflows.controller.ee.test.ts | 28 ++++++++++++++++++- 5 files changed, 55 insertions(+), 12 deletions(-) diff --git a/jest.config.js b/jest.config.js index 729d2d3a59..e6eb3e16c8 100644 --- a/jest.config.js +++ b/jest.config.js @@ -26,6 +26,7 @@ const config = { collectCoverage: true, coverageReporters: [process.env.COVERAGE_REPORT === 'true' ? 'text' : 'text-summary'], collectCoverageFrom: ['src/**/*.ts'], + testTimeout: 10_000, }; if (process.env.CI === 'true') { diff --git a/packages/cli/src/WorkflowHelpers.ts b/packages/cli/src/WorkflowHelpers.ts index 4f8b121bbb..c6a40455fe 100644 --- a/packages/cli/src/WorkflowHelpers.ts +++ b/packages/cli/src/WorkflowHelpers.ts @@ -1,3 +1,4 @@ +import type { FindOptionsWhere } from 'typeorm'; import { In } from 'typeorm'; import { Container } from 'typedi'; import type { @@ -26,16 +27,16 @@ import type { } from '@/Interfaces'; import { NodeTypes } from '@/NodeTypes'; import { WorkflowRunner } from '@/WorkflowRunner'; - import config from '@/config'; import type { WorkflowEntity } from '@db/entities/WorkflowEntity'; import type { User } from '@db/entities/User'; import { RoleRepository } from '@db/repositories'; -import { whereClause } from '@/UserManagement/UserManagementHelper'; import omit from 'lodash.omit'; import { PermissionChecker } from './UserManagement/PermissionChecker'; import { isWorkflowIdValid } from './utils'; import { UserService } from './user/user.service'; +import type { SharedWorkflow } from './databases/entities/SharedWorkflow'; +import type { RoleNames } from './databases/entities/Role'; const ERROR_TRIGGER_TYPE = config.getEnv('nodes.errorTriggerType'); @@ -372,16 +373,24 @@ export async function replaceInvalidCredentials(workflow: WorkflowEntity): Promi * Get the IDs of the workflows that have been shared with the user. * Returns all IDs if user is global owner (see `whereClause`) */ -export async function getSharedWorkflowIds(user: User, roles?: string[]): Promise { +export async function getSharedWorkflowIds(user: User, roles?: RoleNames[]): Promise { + const where: FindOptionsWhere = {}; + if (user.globalRole?.name !== 'owner') { + where.userId = user.id; + } + if (roles?.length) { + const roleIds = await Db.collections.Role.find({ + select: ['id'], + where: { name: In(roles), scope: 'workflow' }, + }).then((data) => data.map(({ id }) => id)); + where.roleId = In(roleIds); + } const sharedWorkflows = await Db.collections.SharedWorkflow.find({ - relations: ['workflow', 'role'], - where: whereClause({ user, entityType: 'workflow', roles }), + where, select: ['workflowId'], }); - return sharedWorkflows.map(({ workflowId }) => workflowId); } - /** * Check if user owns more than 15 workflows or more than 2 workflows with at least 2 nodes. * If user does, set flag in its settings. diff --git a/packages/cli/src/config/index.ts b/packages/cli/src/config/index.ts index 6f20ba3fb9..e75db78b7a 100644 --- a/packages/cli/src/config/index.ts +++ b/packages/cli/src/config/index.ts @@ -1,16 +1,18 @@ import convict from 'convict'; import dotenv from 'dotenv'; import { tmpdir } from 'os'; -import { mkdtempSync, readFileSync } from 'fs'; +import { mkdirSync, mkdtempSync, readFileSync } from 'fs'; import { join } from 'path'; import { schema } from './schema'; import { inTest, inE2ETests } from '@/constants'; if (inE2ETests) { + const testsDir = join(tmpdir(), 'n8n-e2e/'); + mkdirSync(testsDir, { recursive: true }); // Skip loading config from env variables in end-to-end tests process.env = { E2E_TESTS: 'true', - N8N_USER_FOLDER: mkdtempSync(join(tmpdir(), 'n8n-e2e-')), + N8N_USER_FOLDER: mkdtempSync(testsDir), EXECUTIONS_PROCESS: 'main', N8N_DIAGNOSTICS_ENABLED: 'false', N8N_PUBLIC_API_DISABLED: 'true', @@ -19,8 +21,12 @@ if (inE2ETests) { NODE_FUNCTION_ALLOW_EXTERNAL: 'node-fetch', }; } else if (inTest) { + const testsDir = join(tmpdir(), 'n8n-tests/'); + mkdirSync(testsDir, { recursive: true }); + process.env.N8N_LOG_LEVEL = 'silent'; + process.env.N8N_ENCRYPTION_KEY = 'test-encryption-key'; process.env.N8N_PUBLIC_API_DISABLED = 'true'; - process.env.N8N_PUBLIC_API_SWAGGERUI_DISABLED = 'true'; + process.env.N8N_USER_FOLDER = mkdtempSync(testsDir); } else { dotenv.config(); } diff --git a/packages/cli/src/workflows/workflows.services.ts b/packages/cli/src/workflows/workflows.services.ts index e942620e07..747cede32c 100644 --- a/packages/cli/src/workflows/workflows.services.ts +++ b/packages/cli/src/workflows/workflows.services.ts @@ -27,6 +27,7 @@ import { getSharedWorkflowIds } from '@/WorkflowHelpers'; import { isSharingEnabled, whereClause } from '@/UserManagement/UserManagementHelper'; import type { WorkflowForList } from '@/workflows/workflows.types'; import { InternalHooks } from '@/InternalHooks'; +import type { RoleNames } from '../databases/entities/Role'; export type IGetWorkflowsQueryFilter = Pick< FindOptionsWhere, @@ -111,7 +112,7 @@ export class WorkflowsService { } // Warning: this function is overridden by EE to disregard role list. - static async getWorkflowIdsForUser(user: User, roles?: string[]): Promise { + static async getWorkflowIdsForUser(user: User, roles?: RoleNames[]): Promise { return getSharedWorkflowIds(user, roles); } diff --git a/packages/cli/test/integration/workflows.controller.ee.test.ts b/packages/cli/test/integration/workflows.controller.ee.test.ts index bf8ff9948f..223f0bcc52 100644 --- a/packages/cli/test/integration/workflows.controller.ee.test.ts +++ b/packages/cli/test/integration/workflows.controller.ee.test.ts @@ -8,11 +8,12 @@ import type { User } from '@db/entities/User'; import * as utils from './shared/utils'; import * as testDb from './shared/testDb'; -import { createWorkflow } from './shared/testDb'; +import { createWorkflow, getGlobalMemberRole, getGlobalOwnerRole } from './shared/testDb'; import type { SaveCredentialFunction } from './shared/types'; import { makeWorkflow } from './shared/utils'; import { randomCredentialPayload } from './shared/random'; import { License } from '@/License'; +import { getSharedWorkflowIds } from '../../src/WorkflowHelpers'; let owner: User; let member: User; @@ -850,3 +851,28 @@ describe('PATCH /workflows/:id - validate interim updates', () => { expect(updateAttemptResponse.body.code).toBe(100); }); }); + +describe('getSharedWorkflowIds', () => { + it('should show all workflows to owners', async () => { + owner.globalRole = await getGlobalOwnerRole(); + const workflow1 = await createWorkflow({}, member); + const workflow2 = await createWorkflow({}, anotherMember); + const sharedWorkflowIds = await getSharedWorkflowIds(owner); + expect(sharedWorkflowIds).toHaveLength(2); + expect(sharedWorkflowIds).toContain(workflow1.id); + expect(sharedWorkflowIds).toContain(workflow2.id); + }); + + it('should show shared workflows to users', async () => { + member.globalRole = await getGlobalMemberRole(); + const workflow1 = await createWorkflow({}, anotherMember); + const workflow2 = await createWorkflow({}, anotherMember); + const workflow3 = await createWorkflow({}, anotherMember); + await testDb.shareWorkflowWithUsers(workflow1, [member]); + await testDb.shareWorkflowWithUsers(workflow3, [member]); + const sharedWorkflowIds = await getSharedWorkflowIds(member); + expect(sharedWorkflowIds).toHaveLength(2); + expect(sharedWorkflowIds).toContain(workflow1.id); + expect(sharedWorkflowIds).toContain(workflow3.id); + }); +});