From 38b88b946bab67dc1a964bb3c980a627d4a32595 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Iv=C3=A1n=20Ovejero?= Date: Tue, 5 Dec 2023 10:11:18 +0100 Subject: [PATCH] fix(core): Consolidate ownership and sharing data on workflows and credentials (#7920) ## Summary Ensure `ownedBy` and `sharedWith` are present and uniform for credentials and workflows. Details in story: https://linear.app/n8n/issue/PAY-987 --- .../src/credentials/credentials.service.ts | 12 +++--- packages/cli/src/requests.ts | 20 +++++---- .../cli/src/services/ownership.service.ts | 42 ++++++++----------- .../cli/src/workflows/workflows.services.ts | 21 ++++------ .../integration/workflows.controller.test.ts | 40 +++++++++++++++--- .../unit/services/ownership.service.test.ts | 33 ++++++++++++++- 6 files changed, 113 insertions(+), 55 deletions(-) diff --git a/packages/cli/src/credentials/credentials.service.ts b/packages/cli/src/credentials/credentials.service.ts index 83e28f6d0d6eb..e684423922592 100644 --- a/packages/cli/src/credentials/credentials.service.ts +++ b/packages/cli/src/credentials/credentials.service.ts @@ -82,10 +82,6 @@ export class CredentialsService { return findManyOptions; } - private static addOwnedByAndSharedWith(credentials: CredentialsEntity[]) { - return credentials.map((c) => Container.get(OwnershipService).addOwnedByAndSharedWith(c)); - } - static async getMany( user: User, options: { listQueryOptions?: ListQuery.Options; onlyOwn?: boolean } = {}, @@ -98,7 +94,9 @@ export class CredentialsService { if (returnAll) { const credentials = await Container.get(CredentialsRepository).find(findManyOptions); - return isDefaultSelect ? this.addOwnedByAndSharedWith(credentials) : credentials; + return isDefaultSelect + ? credentials.map((c) => Container.get(OwnershipService).addOwnedByAndSharedWith(c)) + : credentials; } const ids = await this.getAccessibleCredentials(user.id); @@ -108,7 +106,9 @@ export class CredentialsService { where: { ...findManyOptions.where, id: In(ids) }, // only accessible credentials }); - return isDefaultSelect ? this.addOwnedByAndSharedWith(credentials) : credentials; + return isDefaultSelect + ? credentials.map((c) => Container.get(OwnershipService).addOwnedByAndSharedWith(c)) + : credentials; } /** diff --git a/packages/cli/src/requests.ts b/packages/cli/src/requests.ts index 073046502758e..585b900356830 100644 --- a/packages/cli/src/requests.ts +++ b/packages/cli/src/requests.ts @@ -158,26 +158,32 @@ export namespace ListQuery { type SharedField = Partial>; - type OwnedByField = { ownedBy: Pick | null }; + type OwnedByField = { ownedBy: SlimUser | null }; export type Plain = BaseFields; export type WithSharing = BaseFields & SharedField; export type WithOwnership = BaseFields & OwnedByField; + + type SharedWithField = { sharedWith: SlimUser[] }; + + export type WithOwnedByAndSharedWith = BaseFields & OwnedByField & SharedWithField; } -} -export namespace Credentials { - type SlimUser = Pick; + export namespace Credentials { + type OwnedByField = { ownedBy: SlimUser | null }; - type OwnedByField = { ownedBy: SlimUser | null }; + type SharedWithField = { sharedWith: SlimUser[] }; - type SharedWithField = { sharedWith: SlimUser[] }; + export type WithSharing = CredentialsEntity & Partial>; - export type WithOwnedByAndSharedWith = CredentialsEntity & OwnedByField & SharedWithField; + export type WithOwnedByAndSharedWith = CredentialsEntity & OwnedByField & SharedWithField; + } } +type SlimUser = Pick; + export function hasSharing( workflows: ListQuery.Workflow.Plain[] | ListQuery.Workflow.WithSharing[], ): workflows is ListQuery.Workflow.WithSharing[] { diff --git a/packages/cli/src/services/ownership.service.ts b/packages/cli/src/services/ownership.service.ts index 08a3699f200eb..3bda4d04f0fe4 100644 --- a/packages/cli/src/services/ownership.service.ts +++ b/packages/cli/src/services/ownership.service.ts @@ -4,9 +4,7 @@ import { SharedWorkflowRepository } from '@db/repositories/sharedWorkflow.reposi import type { User } from '@db/entities/User'; import { RoleService } from './role.service'; import { UserService } from './user.service'; -import type { Credentials, ListQuery } from '@/requests'; -import type { Role } from '@db/entities/Role'; -import type { CredentialsEntity } from '@db/entities/CredentialsEntity'; +import type { ListQuery } from '@/requests'; import { ApplicationError } from 'n8n-workflow'; @Service() @@ -40,37 +38,33 @@ export class OwnershipService { return sharedWorkflow.user; } - addOwnedBy( - workflow: ListQuery.Workflow.WithSharing, - workflowOwnerRole: Role, - ): ListQuery.Workflow.WithOwnership { - const { shared, ...rest } = workflow; + addOwnedByAndSharedWith( + rawWorkflow: ListQuery.Workflow.WithSharing, + ): ListQuery.Workflow.WithOwnedByAndSharedWith; + addOwnedByAndSharedWith( + rawCredential: ListQuery.Credentials.WithSharing, + ): ListQuery.Credentials.WithOwnedByAndSharedWith; + addOwnedByAndSharedWith( + rawEntity: ListQuery.Workflow.WithSharing | ListQuery.Credentials.WithSharing, + ): ListQuery.Workflow.WithOwnedByAndSharedWith | ListQuery.Credentials.WithOwnedByAndSharedWith { + const { shared, ...rest } = rawEntity; - const ownerId = shared?.find((s) => s.roleId.toString() === workflowOwnerRole.id)?.userId; + const entity = rest as + | ListQuery.Workflow.WithOwnedByAndSharedWith + | ListQuery.Credentials.WithOwnedByAndSharedWith; - return Object.assign(rest, { - ownedBy: ownerId ? { id: ownerId } : null, - }); - } - - addOwnedByAndSharedWith(_credential: CredentialsEntity): Credentials.WithOwnedByAndSharedWith { - const { shared, ...rest } = _credential; - - const credential = rest as Credentials.WithOwnedByAndSharedWith; - - credential.ownedBy = null; - credential.sharedWith = []; + Object.assign(entity, { ownedBy: null, sharedWith: [] }); shared?.forEach(({ user, role }) => { const { id, email, firstName, lastName } = user; if (role.name === 'owner') { - credential.ownedBy = { id, email, firstName, lastName }; + entity.ownedBy = { id, email, firstName, lastName }; } else { - credential.sharedWith.push({ id, email, firstName, lastName }); + entity.sharedWith.push({ id, email, firstName, lastName }); } }); - return credential; + return entity; } } diff --git a/packages/cli/src/workflows/workflows.services.ts b/packages/cli/src/workflows/workflows.services.ts index d0485f9c467c6..ea6d6704a22c0 100644 --- a/packages/cli/src/workflows/workflows.services.ts +++ b/packages/cli/src/workflows/workflows.services.ts @@ -23,7 +23,6 @@ import { TestWebhooks } from '@/TestWebhooks'; import { whereClause } from '@/UserManagement/UserManagementHelper'; import { InternalHooks } from '@/InternalHooks'; import { WorkflowRepository } from '@db/repositories/workflow.repository'; -import { RoleService } from '@/services/role.service'; import { OwnershipService } from '@/services/ownership.service'; import { isStringArray, isWorkflowIdValid } from '@/utils'; import { WorkflowHistoryService } from './workflowHistory/workflowHistory.service.ee'; @@ -150,7 +149,7 @@ export class WorkflowsService { select.tags = { id: true, name: true }; } - if (isOwnedByIncluded) relations.push('shared'); + if (isOwnedByIncluded) relations.push('shared', 'shared.role', 'shared.user'); if (typeof where.name === 'string' && where.name !== '') { where.name = Like(`%${where.name}%`); @@ -178,16 +177,14 @@ export class WorkflowsService { findManyOptions, )) as [ListQuery.Workflow.Plain[] | ListQuery.Workflow.WithSharing[], number]; - if (!hasSharing(workflows)) return { workflows, count }; - - const workflowOwnerRole = await Container.get(RoleService).findWorkflowOwnerRole(); - - return { - workflows: workflows.map((w) => - Container.get(OwnershipService).addOwnedBy(w, workflowOwnerRole), - ), - count, - }; + return hasSharing(workflows) + ? { + workflows: workflows.map((w) => + Container.get(OwnershipService).addOwnedByAndSharedWith(w), + ), + count, + } + : { workflows, count }; } static async update( diff --git a/packages/cli/test/integration/workflows.controller.test.ts b/packages/cli/test/integration/workflows.controller.test.ts index cf435108028fc..01c12b3bde110 100644 --- a/packages/cli/test/integration/workflows.controller.test.ts +++ b/packages/cli/test/integration/workflows.controller.test.ts @@ -211,7 +211,13 @@ describe('GET /workflows', () => { updatedAt: any(String), tags: [{ id: any(String), name: 'A' }], versionId: any(String), - ownedBy: { id: owner.id }, + ownedBy: { + id: owner.id, + email: any(String), + firstName: any(String), + lastName: any(String), + }, + sharedWith: [], }), objectContaining({ id: any(String), @@ -221,7 +227,13 @@ describe('GET /workflows', () => { updatedAt: any(String), tags: [], versionId: any(String), - ownedBy: { id: owner.id }, + ownedBy: { + id: owner.id, + email: any(String), + firstName: any(String), + lastName: any(String), + }, + sharedWith: [], }), ]), }); @@ -231,7 +243,7 @@ describe('GET /workflows', () => { ); expect(found.nodes).toBeUndefined(); - expect(found.sharedWith).toBeUndefined(); + expect(found.sharedWith).toHaveLength(0); expect(found.usedCredentials).toBeUndefined(); }); @@ -412,8 +424,26 @@ describe('GET /workflows', () => { expect(response.body).toEqual({ count: 2, data: arrayContaining([ - { id: any(String), ownedBy: { id: owner.id } }, - { id: any(String), ownedBy: { id: owner.id } }, + { + id: any(String), + ownedBy: { + id: owner.id, + email: any(String), + firstName: any(String), + lastName: any(String), + }, + sharedWith: [], + }, + { + id: any(String), + ownedBy: { + id: owner.id, + email: any(String), + firstName: any(String), + lastName: any(String), + }, + sharedWith: [], + }, ]), }); }); diff --git a/packages/cli/test/unit/services/ownership.service.test.ts b/packages/cli/test/unit/services/ownership.service.test.ts index f197c1ee15379..601dccc66a17c 100644 --- a/packages/cli/test/unit/services/ownership.service.test.ts +++ b/packages/cli/test/unit/services/ownership.service.test.ts @@ -15,6 +15,7 @@ import { randomInteger, randomName, } from '../../integration/shared/random'; +import { WorkflowEntity } from '@/databases/entities/WorkflowEntity'; const wfOwnerRole = () => Object.assign(new Role(), { @@ -94,7 +95,7 @@ describe('OwnershipService', () => { }); describe('addOwnedByAndSharedWith()', () => { - test('should add ownedBy and sharedWith to credential', async () => { + test('should add `ownedBy` and `sharedWith` to credential', async () => { const owner = mockUser(); const editor = mockUser(); @@ -124,6 +125,36 @@ describe('OwnershipService', () => { ]); }); + test('should add `ownedBy` and `sharedWith` to workflow', async () => { + const owner = mockUser(); + const editor = mockUser(); + + const workflow = new WorkflowEntity(); + + workflow.shared = [ + { role: mockCredRole('owner'), user: owner }, + { role: mockCredRole('editor'), user: editor }, + ] as SharedWorkflow[]; + + const { ownedBy, sharedWith } = ownershipService.addOwnedByAndSharedWith(workflow); + + expect(ownedBy).toStrictEqual({ + id: owner.id, + email: owner.email, + firstName: owner.firstName, + lastName: owner.lastName, + }); + + expect(sharedWith).toStrictEqual([ + { + id: editor.id, + email: editor.email, + firstName: editor.firstName, + lastName: editor.lastName, + }, + ]); + }); + test('should produce an empty sharedWith if no sharee', async () => { const owner = mockUser();