From b76ab318f8919138391850738de99e592aa020a7 Mon Sep 17 00:00:00 2001 From: Omar Ajoue Date: Fri, 14 Apr 2023 11:05:42 +0200 Subject: [PATCH] fix(core): Fix broken API permissions in public API (#5978) --- .../handlers/executions/executions.handler.ts | 4 +- .../handlers/workflows/workflows.service.ts | 5 +- .../integration/publicApi/executions.test.ts | 139 +++++++++++++++++- 3 files changed, 142 insertions(+), 6 deletions(-) diff --git a/packages/cli/src/PublicApi/v1/handlers/executions/executions.handler.ts b/packages/cli/src/PublicApi/v1/handlers/executions/executions.handler.ts index f3cbab6a3d09f..0ffb8202e2859 100644 --- a/packages/cli/src/PublicApi/v1/handlers/executions/executions.handler.ts +++ b/packages/cli/src/PublicApi/v1/handlers/executions/executions.handler.ts @@ -91,7 +91,7 @@ export = { // user does not have workflows hence no executions // or the execution he is trying to access belongs to a workflow he does not own - if (!sharedWorkflowsIds.length) { + if (!sharedWorkflowsIds.length || (workflowId && !sharedWorkflowsIds.includes(workflowId))) { return res.status(200).json({ data: [], nextCursor: null }); } @@ -105,7 +105,7 @@ export = { limit, lastId, includeData, - ...(workflowId && { workflowIds: [workflowId] }), + workflowIds: workflowId ? [workflowId] : sharedWorkflowsIds, excludedExecutionsIds: runningExecutionsIds, }; diff --git a/packages/cli/src/PublicApi/v1/handlers/workflows/workflows.service.ts b/packages/cli/src/PublicApi/v1/handlers/workflows/workflows.service.ts index d727fd88055ee..adb37dce57dd1 100644 --- a/packages/cli/src/PublicApi/v1/handlers/workflows/workflows.service.ts +++ b/packages/cli/src/PublicApi/v1/handlers/workflows/workflows.service.ts @@ -18,9 +18,12 @@ function insertIf(condition: boolean, elements: string[]): string[] { } export async function getSharedWorkflowIds(user: User): Promise { + const where = user.globalRole.name === 'owner' ? {} : { userId: user.id }; const sharedWorkflows = await Db.collections.SharedWorkflow.find({ - where: { userId: user.id }, + where, + select: ['workflowId'], }); + return sharedWorkflows.map(({ workflowId }) => workflowId); return sharedWorkflows.map(({ workflowId }) => workflowId); } diff --git a/packages/cli/test/integration/publicApi/executions.test.ts b/packages/cli/test/integration/publicApi/executions.test.ts index 4c70977dcef75..1d5debdbfb1e3 100644 --- a/packages/cli/test/integration/publicApi/executions.test.ts +++ b/packages/cli/test/integration/publicApi/executions.test.ts @@ -10,7 +10,11 @@ import * as testDb from '../shared/testDb'; let app: Application; let owner: User; +let user1: User; +let user2: User; let authOwnerAgent: SuperAgentTest; +let authUser1Agent: SuperAgentTest; +let authUser2Agent: SuperAgentTest; let workflowRunner: ActiveWorkflowRunner; beforeAll(async () => { @@ -21,7 +25,10 @@ beforeAll(async () => { }); const globalOwnerRole = await testDb.getGlobalOwnerRole(); + const globalUserRole = await testDb.getGlobalMemberRole(); owner = await testDb.createUser({ globalRole: globalOwnerRole, apiKey: randomApiKey() }); + user1 = await testDb.createUser({ globalRole: globalUserRole, apiKey: randomApiKey() }); + user2 = await testDb.createUser({ globalRole: globalUserRole, apiKey: randomApiKey() }); await utils.initBinaryManager(); await utils.initNodeTypes(); @@ -46,6 +53,20 @@ beforeEach(async () => { version: 1, }); + authUser1Agent = utils.createAgent(app, { + apiPath: 'public', + auth: true, + user: user1, + version: 1, + }); + + authUser2Agent = utils.createAgent(app, { + apiPath: 'public', + auth: true, + user: user2, + version: 1, + }); + config.set('userManagement.disabled', false); config.set('userManagement.isInstanceOwnerSetUp', true); }); @@ -70,7 +91,7 @@ describe('GET /executions/:id', () => { test('should fail due to invalid API Key', testWithAPIKey('get', '/executions/1', 'abcXYZ')); - test('should get an execution', async () => { + test('owner should be able to get an execution owned by him', async () => { const workflow = await testDb.createWorkflow({}, owner); const execution = await testDb.createSuccessfulExecution(workflow); @@ -101,6 +122,46 @@ describe('GET /executions/:id', () => { expect(workflowId).toBe(execution.workflowId); expect(waitTill).toBeNull(); }); + + test('owner should be able to read executions of other users', async () => { + const workflow = await testDb.createWorkflow({}, user1); + const execution = await testDb.createSuccessfulExecution(workflow); + + const response = await authOwnerAgent.get(`/executions/${execution.id}`); + + expect(response.statusCode).toBe(200); + }); + + test('member should be able to fetch his own executions', async () => { + const workflow = await testDb.createWorkflow({}, user1); + const execution = await testDb.createSuccessfulExecution(workflow); + + const response = await authUser1Agent.get(`/executions/${execution.id}`); + + expect(response.statusCode).toBe(200); + }); + + test('member should not get an execution of another user without the workflow being shared', async () => { + const workflow = await testDb.createWorkflow({}, owner); + + const execution = await testDb.createSuccessfulExecution(workflow); + + const response = await authUser1Agent.get(`/executions/${execution.id}`); + + expect(response.statusCode).toBe(404); + }); + + test('member should be able to fetch executions of workflows shared with him', async () => { + const workflow = await testDb.createWorkflow({}, user1); + + const execution = await testDb.createSuccessfulExecution(workflow); + + await testDb.shareWorkflowWithUsers(workflow, [user2]); + + const response = await authUser2Agent.get(`/executions/${execution.id}`); + + expect(response.statusCode).toBe(200); + }); }); describe('DELETE /executions/:id', () => { @@ -324,10 +385,8 @@ describe('GET /executions', () => { const savedExecutions = await testDb.createManyExecutions( 2, workflow, - // @ts-ignore testDb.createSuccessfulExecution, ); - // @ts-ignore await testDb.createManyExecutions(2, workflow2, testDb.createSuccessfulExecution); const response = await authOwnerAgent.get(`/executions`).query({ @@ -362,4 +421,78 @@ describe('GET /executions', () => { expect(waitTill).toBeNull(); } }); + + test('owner should retrieve all executions regardless of ownership', async () => { + const [firstWorkflowForUser1, secondWorkflowForUser1] = await testDb.createManyWorkflows( + 2, + {}, + user1, + ); + await testDb.createManyExecutions(2, firstWorkflowForUser1, testDb.createSuccessfulExecution); + await testDb.createManyExecutions(2, secondWorkflowForUser1, testDb.createSuccessfulExecution); + + const [firstWorkflowForUser2, secondWorkflowForUser2] = await testDb.createManyWorkflows( + 2, + {}, + user2, + ); + await testDb.createManyExecutions(2, firstWorkflowForUser2, testDb.createSuccessfulExecution); + await testDb.createManyExecutions(2, secondWorkflowForUser2, testDb.createSuccessfulExecution); + + const response = await authOwnerAgent.get(`/executions`); + + expect(response.statusCode).toBe(200); + expect(response.body.data.length).toBe(8); + expect(response.body.nextCursor).toBe(null); + }); + + test('member should not see executions of workflows not shared with him', async () => { + const [firstWorkflowForUser1, secondWorkflowForUser1] = await testDb.createManyWorkflows( + 2, + {}, + user1, + ); + await testDb.createManyExecutions(2, firstWorkflowForUser1, testDb.createSuccessfulExecution); + await testDb.createManyExecutions(2, secondWorkflowForUser1, testDb.createSuccessfulExecution); + + const [firstWorkflowForUser2, secondWorkflowForUser2] = await testDb.createManyWorkflows( + 2, + {}, + user2, + ); + await testDb.createManyExecutions(2, firstWorkflowForUser2, testDb.createSuccessfulExecution); + await testDb.createManyExecutions(2, secondWorkflowForUser2, testDb.createSuccessfulExecution); + + const response = await authUser1Agent.get(`/executions`); + + expect(response.statusCode).toBe(200); + expect(response.body.data.length).toBe(4); + expect(response.body.nextCursor).toBe(null); + }); + + test('member should also see executions of workflows shared with him', async () => { + const [firstWorkflowForUser1, secondWorkflowForUser1] = await testDb.createManyWorkflows( + 2, + {}, + user1, + ); + await testDb.createManyExecutions(2, firstWorkflowForUser1, testDb.createSuccessfulExecution); + await testDb.createManyExecutions(2, secondWorkflowForUser1, testDb.createSuccessfulExecution); + + const [firstWorkflowForUser2, secondWorkflowForUser2] = await testDb.createManyWorkflows( + 2, + {}, + user2, + ); + await testDb.createManyExecutions(2, firstWorkflowForUser2, testDb.createSuccessfulExecution); + await testDb.createManyExecutions(2, secondWorkflowForUser2, testDb.createSuccessfulExecution); + + await testDb.shareWorkflowWithUsers(firstWorkflowForUser2, [user1]); + + const response = await authUser1Agent.get(`/executions`); + + expect(response.statusCode).toBe(200); + expect(response.body.data.length).toBe(6); + expect(response.body.nextCursor).toBe(null); + }); });