feat: Allow owner to share workflows/credentials they don't own (no-changelog) (#7869)

Github issue / Community forum post (link here to close automatically):

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);
 			}
 		});
 

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<SharedCredentials[]> {
 		const credential = await transaction.findOne(CredentialsEntity, {
 			where: { id: credentialId },
-			relations: ['shared'],
+			relations,
 		});
 		return credential?.shared ?? [];
 	}

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);
 			}
 		});
 

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<SharedWorkflow[]> {
 		const workflow = await transaction.findOne(WorkflowEntity, {
 			where: { id: workflowId },
-			relations: ['shared'],
+			relations,
 		});
 		return workflow?.shared ?? [];
 	}

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 () => {

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', () => {