refactor(core): Consolidate CredentialsService.getMany() (no-changelog) (#7028)

Consolidate `CredentialsService.getMany()` in preparation for adding
list query middleware to `GET /credentials`.

packages/cli/src/WorkflowHelpers.ts

@@ -23,12 +23,7 @@ import {
 } from 'n8n-workflow';
 import { v4 as uuid } from 'uuid';
 import * as Db from '@/Db';
-import type {
-	ICredentialsDb,
-	IExecutionDb,
-	IWorkflowErrorData,
-	IWorkflowExecutionDataProcess,
-} from '@/Interfaces';
+import type { IExecutionDb, IWorkflowErrorData, IWorkflowExecutionDataProcess } from '@/Interfaces';
 import { NodeTypes } from '@/NodeTypes';
 // eslint-disable-next-line import/no-cycle
 import { WorkflowRunner } from '@/WorkflowRunner';
@@ -45,6 +40,7 @@ import type { RoleNames } from '@db/entities/Role';
 import { RoleService } from './services/role.service';
 import { ExecutionRepository, RoleRepository } from './databases/repositories';
 import { VariablesService } from './environments/variables/variables.service';
+import type { Credentials } from './requests';
 
 const ERROR_TRIGGER_TYPE = config.getEnv('nodes.errorTriggerType');
 
@@ -543,7 +539,7 @@ export function getNodesWithInaccessibleCreds(workflow: WorkflowEntity, userCred
 export function validateWorkflowCredentialUsage(
 	newWorkflowVersion: WorkflowEntity,
 	previousWorkflowVersion: WorkflowEntity,
-	credentialsUserHasAccessTo: ICredentialsDb[],
+	credentialsUserHasAccessTo: Credentials.WithOwnedByAndSharedWith[],
 ) {
 	/**
 	 * We only need to check nodes that use credentials the current user cannot access,

packages/cli/src/credentials/credentials.controller.ee.ts

@@ -1,16 +1,16 @@
 import express from 'express';
 import type { INodeCredentialTestResult } from 'n8n-workflow';
-import { deepCopy, LoggerProxy } from 'n8n-workflow';
+import { deepCopy } from 'n8n-workflow';
 import * as Db from '@/Db';
 import * as ResponseHelper from '@/ResponseHelper';
-import type { CredentialsEntity } from '@db/entities/CredentialsEntity';
 
 import type { CredentialRequest } from '@/requests';
 import { isSharingEnabled, rightDiff } from '@/UserManagement/UserManagementHelper';
 import { EECredentialsService as EECredentials } from './credentials.service.ee';
-import type { CredentialWithSharings } from './credentials.types';
+import { OwnershipService } from '@/services/ownership.service';
 import { Container } from 'typedi';
 import { InternalHooks } from '@/InternalHooks';
+import type { CredentialsEntity } from '@/databases/entities/CredentialsEntity';
 
 // eslint-disable-next-line @typescript-eslint/naming-convention
 export const EECredentialsController = express.Router();
@@ -25,27 +25,6 @@ EECredentialsController.use((req, res, next) => {
 	next();
 });
 
-/**
- * GET /credentials
- */
-EECredentialsController.get(
-	'/',
-	ResponseHelper.send(async (req: CredentialRequest.GetAll): Promise<CredentialWithSharings[]> => {
-		try {
-			const allCredentials = await EECredentials.getAll(req.user, {
-				relations: ['shared', 'shared.role', 'shared.user'],
-			});
-
-			return allCredentials.map((credential: CredentialsEntity & CredentialWithSharings) =>
-				EECredentials.addOwnerAndSharings(credential),
-			);
-		} catch (error) {
-			LoggerProxy.error('Request to list credentials failed', error as Error);
-			throw error;
-		}
-	}),
-);
-
 /**
  * GET /credentials/:id
  */
@@ -59,7 +38,7 @@ EECredentialsController.get(
 		let credential = (await EECredentials.get(
 			{ id: credentialId },
 			{ relations: ['shared', 'shared.role', 'shared.user'] },
-		)) as CredentialsEntity & CredentialWithSharings;
+		)) as CredentialsEntity;
 
 		if (!credential) {
 			throw new ResponseHelper.NotFoundError(
@@ -73,7 +52,7 @@ EECredentialsController.get(
 			throw new ResponseHelper.UnauthorizedError('Forbidden.');
 		}
 
-		credential = EECredentials.addOwnerAndSharings(credential);
+		credential = Container.get(OwnershipService).addOwnedByAndSharedWith(credential);
 
 		if (!includeDecryptedData || !userSharing || userSharing.role.name !== 'owner') {
 			const { data: _, ...rest } = credential;

packages/cli/src/credentials/credentials.controller.ts

@@ -35,8 +35,8 @@ credentialsController.use('/', EECredentialsController);
  */
 credentialsController.get(
 	'/',
-	ResponseHelper.send(async (req: CredentialRequest.GetAll): Promise<ICredentialsDb[]> => {
-		return CredentialsService.getAll(req.user, { roles: ['owner'] });
+	ResponseHelper.send(async (req: CredentialRequest.GetAll) => {
+		return CredentialsService.getMany(req.user);
 	}),
 );
 

packages/cli/src/credentials/credentials.service.ee.ts

@@ -6,7 +6,6 @@ import { SharedCredentials } from '@db/entities/SharedCredentials';
 import type { User } from '@db/entities/User';
 import { UserService } from '@/services/user.service';
 import { CredentialsService } from './credentials.service';
-import type { CredentialWithSharings } from './credentials.types';
 import { RoleService } from '@/services/role.service';
 import Container from 'typedi';
 
@@ -93,27 +92,4 @@ export class EECredentialsService extends CredentialsService {
 
 		return transaction.save(newSharedCredentials);
 	}
-
-	static addOwnerAndSharings(
-		credential: CredentialsEntity & CredentialWithSharings,
-	): CredentialsEntity & CredentialWithSharings {
-		credential.ownedBy = null;
-		credential.sharedWith = [];
-
-		credential.shared?.forEach(({ user, role }) => {
-			const { id, email, firstName, lastName } = user;
-
-			if (role.name === 'owner') {
-				credential.ownedBy = { id, email, firstName, lastName };
-				return;
-			}
-
-			credential.sharedWith?.push({ id, email, firstName, lastName });
-		});
-
-		// @ts-ignore
-		delete credential.shared;
-
-		return credential;
-	}
 }

packages/cli/src/credentials/credentials.service.ts

@@ -24,6 +24,7 @@ import type { User } from '@db/entities/User';
 import type { CredentialRequest } from '@/requests';
 import { CredentialTypes } from '@/CredentialTypes';
 import { RoleService } from '@/services/role.service';
+import { OwnershipService } from '@/services/ownership.service';
 
 export class CredentialsService {
 	static async get(
@@ -36,48 +37,58 @@ export class CredentialsService {
 		});
 	}
 
-	static async getAll(
-		user: User,
-		options?: { relations?: string[]; roles?: string[]; disableGlobalRole?: boolean },
-	): Promise<ICredentialsDb[]> {
-		const SELECT_FIELDS: Array<keyof ICredentialsDb> = [
-			'id',
-			'name',
-			'type',
-			'nodesAccess',
-			'createdAt',
-			'updatedAt',
-		];
+	static async getMany(user: User, options?: { disableGlobalRole: boolean }) {
+		type Select = Array<keyof ICredentialsDb>;
 
-		// if instance owner, return all credentials
+		const select: Select = ['id', 'name', 'type', 'nodesAccess', 'createdAt', 'updatedAt'];
 
-		if (user.globalRole.name === 'owner' && options?.disableGlobalRole !== true) {
-			return Db.collections.Credentials.find({
-				select: SELECT_FIELDS,
-				relations: options?.relations,
-			});
+		const relations = ['shared', 'shared.role', 'shared.user'];
+
+		const returnAll = user.globalRole.name === 'owner' && options?.disableGlobalRole !== true;
+
+		const addOwnedByAndSharedWith = (c: CredentialsEntity) =>
+			Container.get(OwnershipService).addOwnedByAndSharedWith(c);
+
+		if (returnAll) {
+			const credentials = await Db.collections.Credentials.find({ select, relations });
+
+			return credentials.map(addOwnedByAndSharedWith);
 		}
 
-		// if member, return credentials owned by or shared with member
-		const userSharings = await Db.collections.SharedCredentials.find({
-			where: {
-				userId: user.id,
-				...(options?.roles?.length ? { role: { name: In(options.roles) } } : {}),
-			},
-			relations: options?.roles?.length ? ['role'] : [],
+		const ids = await CredentialsService.getAccessibleCredentials(user.id);
+
+		const credentials = await Db.collections.Credentials.find({
+			select,
+			relations,
+			where: { id: In(ids) },
 		});
 
-		return Db.collections.Credentials.find({
-			select: SELECT_FIELDS,
-			relations: options?.relations,
-			where: {
-				id: In(userSharings.map((x) => x.credentialsId)),
-			},
-		});
+		return credentials.map(addOwnedByAndSharedWith);
 	}
 
-	static async getMany(filter: FindManyOptions<ICredentialsDb>): Promise<ICredentialsDb[]> {
-		return Db.collections.Credentials.find(filter);
+	/**
+	 * Get the IDs of all credentials owned by or shared with a user.
+	 */
+	private static async getAccessibleCredentials(userId: string) {
+		const sharings = await Db.collections.SharedCredentials.find({
+			relations: ['role'],
+			where: {
+				userId,
+				role: { name: In(['owner', 'user']), scope: 'credential' },
+			},
+		});
+
+		return sharings.map((s) => s.credentialsId);
+	}
+
+	static async getManyByIds(ids: string[], { withSharings } = { withSharings: false }) {
+		const options: FindManyOptions<CredentialsEntity> = { where: { id: In(ids) } };
+
+		if (withSharings) {
+			options.relations = ['shared', 'shared.user', 'shared.role'];
+		}
+
+		return Db.collections.Credentials.find(options);
 	}
 
 	/**

packages/cli/src/credentials/credentials.types.ts

@@ -1,7 +0,0 @@
-import type { IUser } from 'n8n-workflow';
-import type { ICredentialsDb } from '@/Interfaces';
-
-export interface CredentialWithSharings extends ICredentialsDb {
-	ownedBy?: IUser | null;
-	sharedWith?: IUser[];
-}

packages/cli/src/requests.ts

@@ -27,6 +27,7 @@ import type { User } from '@db/entities/User';
 import type { UserManagementMailer } from '@/UserManagement/email';
 import type { Variables } from '@db/entities/Variables';
 import type { WorkflowEntity } from './databases/entities/WorkflowEntity';
+import type { CredentialsEntity } from './databases/entities/CredentialsEntity';
 
 export class UserUpdatePayload implements Pick<User, 'email' | 'firstName' | 'lastName'> {
 	@IsEmail()
@@ -162,6 +163,16 @@ export namespace ListQuery {
 	}
 }
 
+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;
+}
+
 export function hasSharing(
 	workflows: ListQuery.Workflow.Plain[] | ListQuery.Workflow.WithSharing[],
 ): workflows is ListQuery.Workflow.WithSharing[] {

packages/cli/src/services/ownership.service.ts

@@ -4,8 +4,9 @@ import { SharedWorkflowRepository } from '@/databases/repositories';
 import type { User } from '@/databases/entities/User';
 import { RoleService } from './role.service';
 import { UserService } from './user.service';
-import type { ListQuery } from '@/requests';
+import type { Credentials, ListQuery } from '@/requests';
 import type { Role } from '@/databases/entities/Role';
+import type { CredentialsEntity } from '@/databases/entities/CredentialsEntity';
 
 @Service()
 export class OwnershipService {
@@ -50,4 +51,25 @@ export class OwnershipService {
 			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 }) => {
+			const { id, email, firstName, lastName } = user;
+
+			if (role.name === 'owner') {
+				credential.ownedBy = { id, email, firstName, lastName };
+			} else {
+				credential.sharedWith.push({ id, email, firstName, lastName });
+			}
+		});
+
+		return credential;
+	}
 }

packages/cli/src/workflows/workflows.controller.ee.ts

@@ -12,7 +12,7 @@ import { EEWorkflowsService as EEWorkflows } from './workflows.services.ee';
 import { ExternalHooks } from '@/ExternalHooks';
 import { SharedWorkflow } from '@db/entities/SharedWorkflow';
 import { LoggerProxy } from 'n8n-workflow';
-import { EECredentialsService as EECredentials } from '../credentials/credentials.service.ee';
+import { CredentialsService } from '../credentials/credentials.service';
 import type { IExecutionPushResponse } from '@/Interfaces';
 import * as GenericHelpers from '@/GenericHelpers';
 import { In } from 'typeorm';
@@ -151,7 +151,7 @@ EEWorkflowController.post(
 		// This is a new workflow, so we simply check if the user has access to
 		// all used workflows
 
-		const allCredentials = await EECredentials.getAll(req.user);
+		const allCredentials = await CredentialsService.getMany(req.user);
 
 		try {
 			EEWorkflows.validateCredentialPermissionsToUser(newWorkflow, allCredentials);

packages/cli/src/workflows/workflows.services.ee.ts

@@ -3,7 +3,6 @@ import { In, Not } from 'typeorm';
 import * as Db from '@/Db';
 import * as ResponseHelper from '@/ResponseHelper';
 import * as WorkflowHelpers from '@/WorkflowHelpers';
-import type { ICredentialsDb } from '@/Interfaces';
 import { SharedWorkflow } from '@db/entities/SharedWorkflow';
 import type { User } from '@db/entities/User';
 import { WorkflowEntity } from '@db/entities/WorkflowEntity';
@@ -13,10 +12,11 @@ import type {
 	CredentialUsedByWorkflow,
 	WorkflowWithSharingsAndCredentials,
 } from './workflows.types';
-import { EECredentialsService as EECredentials } from '@/credentials/credentials.service.ee';
+import { CredentialsService } from '@/credentials/credentials.service';
 import { NodeOperationError } from 'n8n-workflow';
 import { RoleService } from '@/services/role.service';
 import Container from 'typedi';
+import type { Credentials } from '@/requests';
 
 export class EEWorkflowsService extends WorkflowsService {
 	static async isOwned(
@@ -106,7 +106,9 @@ export class EEWorkflowsService extends WorkflowsService {
 		currentUser: User,
 	): Promise<void> {
 		workflow.usedCredentials = [];
-		const userCredentials = await EECredentials.getAll(currentUser, { disableGlobalRole: true });
+		const userCredentials = await CredentialsService.getMany(currentUser, {
+			disableGlobalRole: true,
+		});
 		const credentialIdsUsedByWorkflow = new Set<string>();
 		workflow.nodes.forEach((node) => {
 			if (!node.credentials) {
@@ -120,12 +122,10 @@ export class EEWorkflowsService extends WorkflowsService {
 				credentialIdsUsedByWorkflow.add(credential.id);
 			});
 		});
-		const workflowCredentials = await EECredentials.getMany({
-			where: {
-				id: In(Array.from(credentialIdsUsedByWorkflow)),
-			},
-			relations: ['shared', 'shared.user', 'shared.role'],
-		});
+		const workflowCredentials = await CredentialsService.getManyByIds(
+			Array.from(credentialIdsUsedByWorkflow),
+			{ withSharings: true },
+		);
 		const userCredentialIds = userCredentials.map((credential) => credential.id);
 		workflowCredentials.forEach((credential) => {
 			const credentialId = credential.id;
@@ -151,7 +151,7 @@ export class EEWorkflowsService extends WorkflowsService {
 
 	static validateCredentialPermissionsToUser(
 		workflow: WorkflowEntity,
-		allowedCredentials: ICredentialsDb[],
+		allowedCredentials: Credentials.WithOwnedByAndSharedWith[],
 	) {
 		workflow.nodes.forEach((node) => {
 			if (!node.credentials) {
@@ -175,7 +175,7 @@ export class EEWorkflowsService extends WorkflowsService {
 			throw new ResponseHelper.NotFoundError('Workflow not found');
 		}
 
-		const allCredentials = await EECredentials.getAll(user);
+		const allCredentials = await CredentialsService.getMany(user);
 
 		try {
 			return WorkflowHelpers.validateWorkflowCredentialUsage(

packages/cli/test/integration/credentials.ee.test.ts

@@ -5,7 +5,7 @@ import type { IUser } from 'n8n-workflow';
 
 import * as Db from '@/Db';
 import { RESPONSE_ERROR_MESSAGES } from '@/constants';
-import type { CredentialWithSharings } from '@/credentials/credentials.types';
+import type { Credentials } from '@/requests';
 import * as UserManagementHelpers from '@/UserManagement/UserManagementHelper';
 import type { Role } from '@db/entities/Role';
 import type { User } from '@db/entities/User';
@@ -92,10 +92,10 @@ describe('GET /credentials', () => {
 		expect(response.statusCode).toBe(200);
 		expect(response.body.data).toHaveLength(2); // owner retrieved owner cred and member cred
 		const ownerCredential = response.body.data.find(
-			(e: CredentialWithSharings) => e.ownedBy?.id === owner.id,
+			(e: Credentials.WithOwnedByAndSharedWith) => e.ownedBy?.id === owner.id,
 		);
 		const memberCredential = response.body.data.find(
-			(e: CredentialWithSharings) => e.ownedBy?.id === member1.id,
+			(e: Credentials.WithOwnedByAndSharedWith) => e.ownedBy?.id === member1.id,
 		);
 
 		validateMainCredentialData(ownerCredential);
@@ -497,7 +497,7 @@ describe('PUT /credentials/:id/share', () => {
 	});
 });
 
-function validateMainCredentialData(credential: CredentialWithSharings) {
+function validateMainCredentialData(credential: Credentials.WithOwnedByAndSharedWith) {
 	expect(typeof credential.name).toBe('string');
 	expect(typeof credential.type).toBe('string');
 	expect(typeof credential.nodesAccess[0].nodeType).toBe('string');

packages/cli/test/integration/credentials.test.ts

@@ -5,7 +5,7 @@ import * as Db from '@/Db';
 import config from '@/config';
 import { RESPONSE_ERROR_MESSAGES } from '@/constants';
 import * as UserManagementHelpers from '@/UserManagement/UserManagementHelper';
-import type { CredentialsEntity } from '@db/entities/CredentialsEntity';
+import type { Credentials } from '@/requests';
 import type { Role } from '@db/entities/Role';
 import type { User } from '@db/entities/User';
 import { randomCredentialPayload, randomName, randomString } from './shared/random';
@@ -59,9 +59,9 @@ describe('GET /credentials', () => {
 		expect(response.body.data.length).toBe(2); // owner retrieved owner cred and member cred
 
 		const savedCredentialsIds = [savedOwnerCredentialId, savedMemberCredentialId];
-		response.body.data.forEach((credential: CredentialsEntity) => {
+		response.body.data.forEach((credential: Credentials.WithOwnedByAndSharedWith) => {
 			validateMainCredentialData(credential);
-			expect(credential.data).toBeUndefined();
+			expect('data' in credential).toBe(false);
 			expect(savedCredentialsIds).toContain(credential.id);
 		});
 	});
@@ -532,14 +532,25 @@ describe('GET /credentials/:id', () => {
 	});
 });
 
-function validateMainCredentialData(credential: CredentialsEntity) {
-	expect(typeof credential.name).toBe('string');
-	expect(typeof credential.type).toBe('string');
-	expect(typeof credential.nodesAccess[0].nodeType).toBe('string');
-	// @ts-ignore
-	expect(credential.ownedBy).toBeUndefined();
-	// @ts-ignore
-	expect(credential.sharedWith).toBeUndefined();
+function validateMainCredentialData(credential: Credentials.WithOwnedByAndSharedWith) {
+	const { name, type, nodesAccess, sharedWith, ownedBy } = credential;
+
+	expect(typeof name).toBe('string');
+	expect(typeof type).toBe('string');
+	expect(typeof nodesAccess?.[0].nodeType).toBe('string');
+
+	if (sharedWith) {
+		expect(Array.isArray(sharedWith)).toBe(true);
+	}
+
+	if (ownedBy) {
+		const { id, email, firstName, lastName } = ownedBy;
+
+		expect(typeof id).toBe('string');
+		expect(typeof email).toBe('string');
+		expect(typeof firstName).toBe('string');
+		expect(typeof lastName).toBe('string');
+	}
 }
 
 const INVALID_PAYLOADS = [

packages/cli/test/unit/services/ownership.service.test.ts

@@ -2,12 +2,19 @@ import { OwnershipService } from '@/services/ownership.service';
 import { SharedWorkflowRepository } from '@/databases/repositories';
 import { mockInstance } from '../../integration/shared/utils';
 import { Role } from '@/databases/entities/Role';
-import { randomInteger } from '../../integration/shared/random';
+import {
+	randomCredentialPayload,
+	randomEmail,
+	randomInteger,
+	randomName,
+} from '../../integration/shared/random';
 import { SharedWorkflow } from '@/databases/entities/SharedWorkflow';
 import { CacheService } from '@/services/cache.service';
 import { User } from '@/databases/entities/User';
 import { RoleService } from '@/services/role.service';
 import { UserService } from '@/services/user.service';
+import { CredentialsEntity } from '@/databases/entities/CredentialsEntity';
+import type { SharedCredentials } from '@/databases/entities/SharedCredentials';
 
 const wfOwnerRole = () =>
 	Object.assign(new Role(), {
@@ -16,6 +23,24 @@ const wfOwnerRole = () =>
 		id: randomInteger(),
 	});
 
+const mockCredRole = (name: 'owner' | 'editor'): Role =>
+	Object.assign(new Role(), {
+		scope: 'credentials',
+		name,
+		id: randomInteger(),
+	});
+
+const mockCredential = (): CredentialsEntity =>
+	Object.assign(new CredentialsEntity(), randomCredentialPayload());
+
+const mockUser = (): User =>
+	Object.assign(new User(), {
+		id: randomInteger(),
+		email: randomEmail(),
+		firstName: randomName(),
+		lastName: randomName(),
+	});
+
 describe('OwnershipService', () => {
 	const cacheService = mockInstance(CacheService);
 	const roleService = mockInstance(RoleService);
@@ -67,4 +92,55 @@ describe('OwnershipService', () => {
 			await expect(ownershipService.getWorkflowOwnerCached('some-workflow-id')).rejects.toThrow();
 		});
 	});
+
+	describe('addOwnedByAndSharedWith()', () => {
+		test('should add ownedBy and sharedWith to credential', async () => {
+			const owner = mockUser();
+			const editor = mockUser();
+
+			const credential = mockCredential();
+
+			credential.shared = [
+				{ role: mockCredRole('owner'), user: owner },
+				{ role: mockCredRole('editor'), user: editor },
+			] as SharedCredentials[];
+
+			const { ownedBy, sharedWith } = ownershipService.addOwnedByAndSharedWith(credential);
+
+			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();
+
+			const credential = mockCredential();
+
+			credential.shared = [{ role: mockCredRole('owner'), user: owner }] as SharedCredentials[];
+
+			const { ownedBy, sharedWith } = ownershipService.addOwnedByAndSharedWith(credential);
+
+			expect(ownedBy).toStrictEqual({
+				id: owner.id,
+				email: owner.email,
+				firstName: owner.firstName,
+				lastName: owner.lastName,
+			});
+
+			expect(sharedWith).toHaveLength(0);
+		});
+	});
 });