From cd474f1562c5521f38c2022b6abbe53f66b81fad Mon Sep 17 00:00:00 2001 From: Val <68596159+valya@users.noreply.github.com> Date: Wed, 29 Nov 2023 16:32:27 +0000 Subject: [PATCH] feat: Allow owner to share workflows/credentials they don't own (no-changelog) (#7869) Github issue / Community forum post (link here to close automatically): --- .../credentials/credentials.controller.ee.ts | 30 +++++++- .../src/credentials/credentials.service.ee.ts | 3 +- .../src/workflows/workflows.controller.ee.ts | 30 +++++++- .../src/workflows/workflows.services.ee.ts | 3 +- .../test/integration/credentials.ee.test.ts | 61 +++++++++++++++- .../workflows.controller.ee.test.ts | 73 ++++++++++++++++++- 6 files changed, 186 insertions(+), 14 deletions(-) diff --git a/packages/cli/src/credentials/credentials.controller.ee.ts b/packages/cli/src/credentials/credentials.controller.ee.ts index 71a165581b..6de562b9dd 100644 --- a/packages/cli/src/credentials/credentials.controller.ee.ts +++ b/packages/cli/src/credentials/credentials.controller.ee.ts @@ -124,17 +124,39 @@ EECredentialsController.put( throw new BadRequestError('Bad request'); } - const { ownsCredential, credential } = await EECredentials.isOwned(req.user, credentialId); + const isOwnedRes = await EECredentials.isOwned(req.user, credentialId); + const { ownsCredential } = isOwnedRes; + let { credential } = isOwnedRes; if (!ownsCredential || !credential) { - throw new UnauthorizedError('Forbidden'); + credential = undefined; + // Allow owners/admins to share + if (await req.user.hasGlobalScope('credential:share')) { + const sharedRes = await EECredentials.getSharing(req.user, credentialId, { + allowGlobalScope: true, + globalScope: 'credential:share', + }); + credential = sharedRes?.credentials; + } + if (!credential) { + throw new UnauthorizedError('Forbidden'); + } } + const ownerIds = ( + await EECredentials.getSharings(Db.getConnection().createEntityManager(), credentialId, [ + 'shared', + 'shared.role', + ]) + ) + .filter((e) => e.role.name === 'owner') + .map((e) => e.userId); + let amountRemoved: number | null = null; let newShareeIds: string[] = []; await Db.transaction(async (trx) => { // remove all sharings that are not supposed to exist anymore const { affected } = await EECredentials.pruneSharings(trx, credentialId, [ - req.user.id, + ...ownerIds, ...shareWithIds, ]); if (affected) amountRemoved = affected; @@ -148,7 +170,7 @@ EECredentialsController.put( ); if (newShareeIds.length) { - await EECredentials.share(trx, credential, newShareeIds); + await EECredentials.share(trx, credential!, newShareeIds); } }); diff --git a/packages/cli/src/credentials/credentials.service.ee.ts b/packages/cli/src/credentials/credentials.service.ee.ts index 416df39bf0..418c674bb8 100644 --- a/packages/cli/src/credentials/credentials.service.ee.ts +++ b/packages/cli/src/credentials/credentials.service.ee.ts @@ -53,10 +53,11 @@ export class EECredentialsService extends CredentialsService { static async getSharings( transaction: EntityManager, credentialId: string, + relations = ['shared'], ): Promise { const credential = await transaction.findOne(CredentialsEntity, { where: { id: credentialId }, - relations: ['shared'], + relations, }); return credential?.shared ?? []; } diff --git a/packages/cli/src/workflows/workflows.controller.ee.ts b/packages/cli/src/workflows/workflows.controller.ee.ts index ffec9c12df..2f66a4ce6c 100644 --- a/packages/cli/src/workflows/workflows.controller.ee.ts +++ b/packages/cli/src/workflows/workflows.controller.ee.ts @@ -59,16 +59,38 @@ EEWorkflowController.put( throw new BadRequestError('Bad request'); } - const { ownsWorkflow, workflow } = await EEWorkflows.isOwned(req.user, workflowId); + const isOwnedRes = await EEWorkflows.isOwned(req.user, workflowId); + const { ownsWorkflow } = isOwnedRes; + let { workflow } = isOwnedRes; if (!ownsWorkflow || !workflow) { - throw new UnauthorizedError('Forbidden'); + workflow = undefined; + // Allow owners/admins to share + if (await req.user.hasGlobalScope('workflow:share')) { + const sharedRes = await EEWorkflows.getSharing(req.user, workflowId, { + allowGlobalScope: true, + globalScope: 'workflow:share', + }); + workflow = sharedRes?.workflow; + } + if (!workflow) { + throw new UnauthorizedError('Forbidden'); + } } + const ownerIds = ( + await EEWorkflows.getSharings(Db.getConnection().createEntityManager(), workflowId, [ + 'shared', + 'shared.role', + ]) + ) + .filter((e) => e.role.name === 'owner') + .map((e) => e.userId); + let newShareeIds: string[] = []; await Db.transaction(async (trx) => { // remove all sharings that are not supposed to exist anymore - await EEWorkflows.pruneSharings(trx, workflowId, [req.user.id, ...shareWithIds]); + await EEWorkflows.pruneSharings(trx, workflowId, [...ownerIds, ...shareWithIds]); const sharings = await EEWorkflows.getSharings(trx, workflowId); @@ -79,7 +101,7 @@ EEWorkflowController.put( ); if (newShareeIds.length) { - await EEWorkflows.share(trx, workflow, newShareeIds); + await EEWorkflows.share(trx, workflow!, newShareeIds); } }); diff --git a/packages/cli/src/workflows/workflows.services.ee.ts b/packages/cli/src/workflows/workflows.services.ee.ts index d6a9337b2d..ef348cb578 100644 --- a/packages/cli/src/workflows/workflows.services.ee.ts +++ b/packages/cli/src/workflows/workflows.services.ee.ts @@ -39,10 +39,11 @@ export class EEWorkflowsService extends WorkflowsService { static async getSharings( transaction: EntityManager, workflowId: string, + relations = ['shared'], ): Promise { const workflow = await transaction.findOne(WorkflowEntity, { where: { id: workflowId }, - relations: ['shared'], + relations, }); return workflow?.shared ?? []; } diff --git a/packages/cli/test/integration/credentials.ee.test.ts b/packages/cli/test/integration/credentials.ee.test.ts index d980338772..d77bf886c8 100644 --- a/packages/cli/test/integration/credentials.ee.test.ts +++ b/packages/cli/test/integration/credentials.ee.test.ts @@ -23,7 +23,9 @@ const testServer = utils.setupTestServer({ endpointGroups: ['credentials'] }); let globalMemberRole: Role; let owner: User; let member: User; +let anotherMember: User; let authOwnerAgent: SuperAgentTest; +let authAnotherMemberAgent: SuperAgentTest; let saveCredential: SaveCredentialFunction; beforeAll(async () => { @@ -33,8 +35,10 @@ beforeAll(async () => { owner = await createUser({ globalRole: globalOwnerRole }); member = await createUser({ globalRole: globalMemberRole }); + anotherMember = await createUser({ globalRole: globalMemberRole }); authOwnerAgent = testServer.authAgentFor(owner); + authAnotherMemberAgent = testServer.authAgentFor(anotherMember); saveCredential = affixRoleToSaveCredential(credentialOwnerRole); }); @@ -406,14 +410,65 @@ describe('PUT /credentials/:id/share', () => { expect(response.statusCode).toBe(403); }); - test('should respond 403 for non-owned credentials', async () => { + test('should respond 403 for non-owned credentials for shared members', async () => { + const savedCredential = await saveCredential(randomCredentialPayload(), { user: member }); + + await shareCredentialWithUsers(savedCredential, [anotherMember]); + + const response = await authAnotherMemberAgent + .put(`/credentials/${savedCredential.id}/share`) + .send({ shareWithIds: [owner.id] }); + + expect(response.statusCode).toBe(403); + const sharedCredentials = await Container.get(SharedCredentialsRepository).find({ + where: { credentialsId: savedCredential.id }, + }); + expect(sharedCredentials).toHaveLength(2); + }); + + test('should respond 403 for non-owned credentials for non-shared members sharing with self', async () => { + const savedCredential = await saveCredential(randomCredentialPayload(), { user: member }); + + const response = await authAnotherMemberAgent + .put(`/credentials/${savedCredential.id}/share`) + .send({ shareWithIds: [anotherMember.id] }); + + expect(response.statusCode).toBe(403); + + const sharedCredentials = await Container.get(SharedCredentialsRepository).find({ + where: { credentialsId: savedCredential.id }, + }); + expect(sharedCredentials).toHaveLength(1); + }); + + test('should respond 403 for non-owned credentials for non-shared members sharing', async () => { + const savedCredential = await saveCredential(randomCredentialPayload(), { user: member }); + const tempUser = await createUser({ globalRole: globalMemberRole }); + + const response = await authAnotherMemberAgent + .put(`/credentials/${savedCredential.id}/share`) + .send({ shareWithIds: [tempUser.id] }); + + expect(response.statusCode).toBe(403); + + const sharedCredentials = await Container.get(SharedCredentialsRepository).find({ + where: { credentialsId: savedCredential.id }, + }); + expect(sharedCredentials).toHaveLength(1); + }); + + test('should respond 200 for non-owned credentials for owners', async () => { const savedCredential = await saveCredential(randomCredentialPayload(), { user: member }); const response = await authOwnerAgent .put(`/credentials/${savedCredential.id}/share`) - .send({ shareWithIds: [member.id] }); + .send({ shareWithIds: [anotherMember.id] }); - expect(response.statusCode).toBe(403); + expect(response.statusCode).toBe(200); + const sharedCredentials = await Container.get(SharedCredentialsRepository).find({ + where: { credentialsId: savedCredential.id }, + }); + expect(sharedCredentials).toHaveLength(2); }); test('should ignore pending sharee', async () => { diff --git a/packages/cli/test/integration/workflows.controller.ee.test.ts b/packages/cli/test/integration/workflows.controller.ee.test.ts index 0687f4b865..c4f8d5a900 100644 --- a/packages/cli/test/integration/workflows.controller.ee.test.ts +++ b/packages/cli/test/integration/workflows.controller.ee.test.ts @@ -20,7 +20,9 @@ import { affixRoleToSaveCredential, shareCredentialWithUsers } from './shared/db import { getCredentialOwnerRole, getGlobalMemberRole, getGlobalOwnerRole } from './shared/db/roles'; import { createUser } from './shared/db/users'; import { createWorkflow, getWorkflowSharing, shareWorkflowWithUsers } from './shared/db/workflows'; +import type { Role } from '@/databases/entities/Role'; +let globalMemberRole: Role; let owner: User; let member: User; let anotherMember: User; @@ -43,7 +45,7 @@ const testServer = utils.setupTestServer({ beforeAll(async () => { const globalOwnerRole = await getGlobalOwnerRole(); - const globalMemberRole = await getGlobalMemberRole(); + globalMemberRole = await getGlobalMemberRole(); const credentialOwnerRole = await getCredentialOwnerRole(); owner = await createUser({ globalRole: globalOwnerRole }); @@ -152,6 +154,75 @@ describe('PUT /workflows/:id', () => { const secondSharedWorkflows = await getWorkflowSharing(workflow); expect(secondSharedWorkflows).toHaveLength(2); }); + + test('PUT /workflows/:id/share should allow sharing by the owner of the workflow', async () => { + const workflow = await createWorkflow({}, member); + + const response = await authMemberAgent + .put(`/workflows/${workflow.id}/share`) + .send({ shareWithIds: [anotherMember.id] }); + + expect(response.statusCode).toBe(200); + + const sharedWorkflows = await getWorkflowSharing(workflow); + expect(sharedWorkflows).toHaveLength(2); + }); + + test('PUT /workflows/:id/share should allow sharing by the instance owner', async () => { + const workflow = await createWorkflow({}, member); + + const response = await authOwnerAgent + .put(`/workflows/${workflow.id}/share`) + .send({ shareWithIds: [anotherMember.id] }); + + expect(response.statusCode).toBe(200); + + const sharedWorkflows = await getWorkflowSharing(workflow); + expect(sharedWorkflows).toHaveLength(2); + }); + + test('PUT /workflows/:id/share should not allow sharing by another shared member', async () => { + const workflow = await createWorkflow({}, member); + + await shareWorkflowWithUsers(workflow, [anotherMember]); + + const response = await authAnotherMemberAgent + .put(`/workflows/${workflow.id}/share`) + .send({ shareWithIds: [anotherMember.id, owner.id] }); + + expect(response.statusCode).toBe(403); + + const sharedWorkflows = await getWorkflowSharing(workflow); + expect(sharedWorkflows).toHaveLength(2); + }); + + test('PUT /workflows/:id/share should not allow sharing with self by another non-shared member', async () => { + const workflow = await createWorkflow({}, member); + + const response = await authAnotherMemberAgent + .put(`/workflows/${workflow.id}/share`) + .send({ shareWithIds: [anotherMember.id] }); + + expect(response.statusCode).toBe(403); + + const sharedWorkflows = await getWorkflowSharing(workflow); + expect(sharedWorkflows).toHaveLength(1); + }); + + test('PUT /workflows/:id/share should not allow sharing by another non-shared member', async () => { + const workflow = await createWorkflow({}, member); + + const tempUser = await createUser({ globalRole: globalMemberRole }); + + const response = await authAnotherMemberAgent + .put(`/workflows/${workflow.id}/share`) + .send({ shareWithIds: [tempUser.id] }); + + expect(response.statusCode).toBe(403); + + const sharedWorkflows = await getWorkflowSharing(workflow); + expect(sharedWorkflows).toHaveLength(1); + }); }); describe('GET /workflows/new', () => {