Skip to content

Commit

Permalink
fix(core): Consolidate ownership and sharing data on workflows and cr…
Browse files Browse the repository at this point in the history
…edentials (#7920)

## Summary

Ensure `ownedBy` and `sharedWith` are present and uniform for
credentials and workflows.

Details in story: https://linear.app/n8n/issue/PAY-987
  • Loading branch information
ivov committed Dec 5, 2023
1 parent 0a745d1 commit 38b88b9
Show file tree
Hide file tree
Showing 6 changed files with 113 additions and 55 deletions.
12 changes: 6 additions & 6 deletions packages/cli/src/credentials/credentials.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 } = {},
Expand All @@ -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);
Expand All @@ -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;
}

/**
Expand Down
20 changes: 13 additions & 7 deletions packages/cli/src/requests.ts
Original file line number Diff line number Diff line change
Expand Up @@ -158,26 +158,32 @@ export namespace ListQuery {

type SharedField = Partial<Pick<WorkflowEntity, 'shared'>>;

type OwnedByField = { ownedBy: Pick<IUser, 'id'> | 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<IUser, 'id' | 'email' | 'firstName' | 'lastName'>;
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<Pick<CredentialsEntity, 'shared'>>;

export type WithOwnedByAndSharedWith = CredentialsEntity & OwnedByField & SharedWithField;
export type WithOwnedByAndSharedWith = CredentialsEntity & OwnedByField & SharedWithField;
}
}

type SlimUser = Pick<IUser, 'id' | 'email' | 'firstName' | 'lastName'>;

export function hasSharing(
workflows: ListQuery.Workflow.Plain[] | ListQuery.Workflow.WithSharing[],
): workflows is ListQuery.Workflow.WithSharing[] {
Expand Down
42 changes: 18 additions & 24 deletions packages/cli/src/services/ownership.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -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;
}
}
21 changes: 9 additions & 12 deletions packages/cli/src/workflows/workflows.services.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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}%`);
Expand Down Expand Up @@ -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(
Expand Down
40 changes: 35 additions & 5 deletions packages/cli/test/integration/workflows.controller.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand All @@ -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: [],
}),
]),
});
Expand All @@ -231,7 +243,7 @@ describe('GET /workflows', () => {
);

expect(found.nodes).toBeUndefined();
expect(found.sharedWith).toBeUndefined();
expect(found.sharedWith).toHaveLength(0);
expect(found.usedCredentials).toBeUndefined();
});

Expand Down Expand Up @@ -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: [],
},
]),
});
});
Expand Down
33 changes: 32 additions & 1 deletion packages/cli/test/unit/services/ownership.service.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import {
randomInteger,
randomName,
} from '../../integration/shared/random';
import { WorkflowEntity } from '@/databases/entities/WorkflowEntity';

const wfOwnerRole = () =>
Object.assign(new Role(), {
Expand Down Expand Up @@ -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();

Expand Down Expand Up @@ -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();

Expand Down

0 comments on commit 38b88b9

Please sign in to comment.