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; return findManyOptions;
} }
private static addOwnedByAndSharedWith(credentials: CredentialsEntity[]) {
return credentials.map((c) => Container.get(OwnershipService).addOwnedByAndSharedWith(c));
}
static async getMany( static async getMany(
user: User, user: User,
options: { listQueryOptions?: ListQuery.Options; onlyOwn?: boolean } = {}, options: { listQueryOptions?: ListQuery.Options; onlyOwn?: boolean } = {},
@ -98,7 +94,9 @@ export class CredentialsService {
if (returnAll) { if (returnAll) {
const credentials = await Container.get(CredentialsRepository).find(findManyOptions); 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); const ids = await this.getAccessibleCredentials(user.id);
@ -108,7 +106,9 @@ export class CredentialsService {
where: { ...findManyOptions.where, id: In(ids) }, // only accessible credentials 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 SharedField = Partial<Pick<WorkflowEntity, 'shared'>>;
type OwnedByField = { ownedBy: Pick<IUser, 'id'> | null }; type OwnedByField = { ownedBy: SlimUser | null };
export type Plain = BaseFields; export type Plain = BaseFields;
export type WithSharing = BaseFields & SharedField; export type WithSharing = BaseFields & SharedField;
export type WithOwnership = BaseFields & OwnedByField; 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 SlimUser = Pick<IUser, 'id' | 'email' | 'firstName' | 'lastName'>;
type OwnedByField = { ownedBy: SlimUser | null };
type SharedWithField = { sharedWith: SlimUser[] };
export type WithOwnedByAndSharedWith = CredentialsEntity & OwnedByField & SharedWithField;
}
export function hasSharing( export function hasSharing(
workflows: ListQuery.Workflow.Plain[] | ListQuery.Workflow.WithSharing[], 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 type { User } from '@db/entities/User';
import { RoleService } from './role.service'; import { RoleService } from './role.service';
import { UserService } from './user.service'; import { UserService } from './user.service';
import type { Credentials, ListQuery } from '@/requests'; import type { ListQuery } from '@/requests';
import type { Role } from '@db/entities/Role';
import type { CredentialsEntity } from '@db/entities/CredentialsEntity';
import { ApplicationError } from 'n8n-workflow'; import { ApplicationError } from 'n8n-workflow';
@Service() @Service()
@ -40,37 +38,33 @@ export class OwnershipService {
return sharedWorkflow.user; return sharedWorkflow.user;
} }
addOwnedBy( addOwnedByAndSharedWith(
workflow: ListQuery.Workflow.WithSharing, rawWorkflow: ListQuery.Workflow.WithSharing,
workflowOwnerRole: Role, ): ListQuery.Workflow.WithOwnedByAndSharedWith;
): ListQuery.Workflow.WithOwnership { addOwnedByAndSharedWith(
const { shared, ...rest } = workflow; 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, { Object.assign(entity, { ownedBy: null, sharedWith: [] });
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 = [];
shared?.forEach(({ user, role }) => { shared?.forEach(({ user, role }) => {
const { id, email, firstName, lastName } = user; const { id, email, firstName, lastName } = user;
if (role.name === 'owner') { if (role.name === 'owner') {
credential.ownedBy = { id, email, firstName, lastName }; entity.ownedBy = { id, email, firstName, lastName };
} else { } 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 { whereClause } from '@/UserManagement/UserManagementHelper';
import { InternalHooks } from '@/InternalHooks'; import { InternalHooks } from '@/InternalHooks';
import { WorkflowRepository } from '@db/repositories/workflow.repository'; import { WorkflowRepository } from '@db/repositories/workflow.repository';
import { RoleService } from '@/services/role.service';
import { OwnershipService } from '@/services/ownership.service'; import { OwnershipService } from '@/services/ownership.service';
import { isStringArray, isWorkflowIdValid } from '@/utils'; import { isStringArray, isWorkflowIdValid } from '@/utils';
import { WorkflowHistoryService } from './workflowHistory/workflowHistory.service.ee'; import { WorkflowHistoryService } from './workflowHistory/workflowHistory.service.ee';
@ -150,7 +149,7 @@ export class WorkflowsService {
select.tags = { id: true, name: true }; 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 !== '') { if (typeof where.name === 'string' && where.name !== '') {
where.name = Like(`%${where.name}%`); where.name = Like(`%${where.name}%`);
@ -178,16 +177,14 @@ export class WorkflowsService {
findManyOptions, findManyOptions,
)) as [ListQuery.Workflow.Plain[] | ListQuery.Workflow.WithSharing[], number]; )) as [ListQuery.Workflow.Plain[] | ListQuery.Workflow.WithSharing[], number];
if (!hasSharing(workflows)) return { workflows, count }; return hasSharing(workflows)
? {
const workflowOwnerRole = await Container.get(RoleService).findWorkflowOwnerRole(); workflows: workflows.map((w) =>
Container.get(OwnershipService).addOwnedByAndSharedWith(w),
return { ),
workflows: workflows.map((w) => count,
Container.get(OwnershipService).addOwnedBy(w, workflowOwnerRole), }
), : { workflows, count };
count,
};
} }
static async update( static async update(

View file

@ -211,7 +211,13 @@ describe('GET /workflows', () => {
updatedAt: any(String), updatedAt: any(String),
tags: [{ id: any(String), name: 'A' }], tags: [{ id: any(String), name: 'A' }],
versionId: any(String), versionId: any(String),
ownedBy: { id: owner.id }, ownedBy: {
id: owner.id,
email: any(String),
firstName: any(String),
lastName: any(String),
},
sharedWith: [],
}), }),
objectContaining({ objectContaining({
id: any(String), id: any(String),
@ -221,7 +227,13 @@ describe('GET /workflows', () => {
updatedAt: any(String), updatedAt: any(String),
tags: [], tags: [],
versionId: any(String), 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.nodes).toBeUndefined();
expect(found.sharedWith).toBeUndefined(); expect(found.sharedWith).toHaveLength(0);
expect(found.usedCredentials).toBeUndefined(); expect(found.usedCredentials).toBeUndefined();
}); });
@ -412,8 +424,26 @@ describe('GET /workflows', () => {
expect(response.body).toEqual({ expect(response.body).toEqual({
count: 2, count: 2,
data: arrayContaining([ 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, randomInteger,
randomName, randomName,
} from '../../integration/shared/random'; } from '../../integration/shared/random';
import { WorkflowEntity } from '@/databases/entities/WorkflowEntity';
const wfOwnerRole = () => const wfOwnerRole = () =>
Object.assign(new Role(), { Object.assign(new Role(), {
@ -94,7 +95,7 @@ describe('OwnershipService', () => {
}); });
describe('addOwnedByAndSharedWith()', () => { describe('addOwnedByAndSharedWith()', () => {
test('should add ownedBy and sharedWith to credential', async () => { test('should add `ownedBy` and `sharedWith` to credential', async () => {
const owner = mockUser(); const owner = mockUser();
const editor = 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 () => { test('should produce an empty sharedWith if no sharee', async () => {
const owner = mockUser(); const owner = mockUser();