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
This commit is contained in:
Iván Ovejero 2023-12-05 10:11:18 +01:00 committed by GitHub
parent 0a745d16e4
commit 38b88b946b
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
6 changed files with 116 additions and 58 deletions

View file

@ -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;
}
/**

View file

@ -158,25 +158,31 @@ 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 OwnedByField = { ownedBy: SlimUser | null };
type SharedWithField = { sharedWith: SlimUser[] };
export type WithSharing = CredentialsEntity & Partial<Pick<CredentialsEntity, 'shared'>>;
export type WithOwnedByAndSharedWith = CredentialsEntity & OwnedByField & SharedWithField;
}
}
export namespace Credentials {
type SlimUser = Pick<IUser, 'id' | 'email' | 'firstName' | 'lastName'>;
type OwnedByField = { ownedBy: SlimUser | null };
type SharedWithField = { sharedWith: SlimUser[] };
export type WithOwnedByAndSharedWith = CredentialsEntity & OwnedByField & SharedWithField;
}
type SlimUser = Pick<IUser, 'id' | 'email' | 'firstName' | 'lastName'>;
export function hasSharing(
workflows: ListQuery.Workflow.Plain[] | ListQuery.Workflow.WithSharing[],

View file

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

View file

@ -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(

View file

@ -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: [],
},
]),
});
});

View file

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