feat(core): Add includeData parameter to GET /credentials (#12220)

Co-authored-by: r00gm <raul00gm@gmail.com>

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

@@ -1,5 +1,6 @@
 import { mock } from 'jest-mock-extended';
 import { nanoId, date } from 'minifaker';
+import { Credentials } from 'n8n-core';
 import { CREDENTIAL_EMPTY_VALUE, type ICredentialType } from 'n8n-workflow';
 
 import { CREDENTIAL_BLANKING_VALUE } from '@/constants';
@@ -30,6 +31,7 @@ describe('CredentialsService', () => {
 			},
 		],
 	});
+
 	const credentialTypes = mock<CredentialTypes>();
 	const service = new CredentialsService(
 		mock(),
@@ -61,7 +63,7 @@ describe('CredentialsService', () => {
 				csrfSecret: 'super-secret',
 			};
 
-			credentialTypes.getByName.calledWith(credential.type).mockReturnValue(credType);
+			credentialTypes.getByName.calledWith(credential.type).mockReturnValueOnce(credType);
 
 			const redactedData = service.redact(decryptedData, credential);
 
@@ -137,4 +139,60 @@ describe('CredentialsService', () => {
 			});
 		});
 	});
+
+	describe('decrypt', () => {
+		it('should redact sensitive values by default', () => {
+			// ARRANGE
+			const data = {
+				clientId: 'abc123',
+				clientSecret: 'sensitiveSecret',
+				accessToken: '',
+				oauthTokenData: 'super-secret',
+				csrfSecret: 'super-secret',
+			};
+			const credential = mock<CredentialsEntity>({
+				id: '123',
+				name: 'Test Credential',
+				type: 'oauth2',
+			});
+			jest.spyOn(Credentials.prototype, 'getData').mockReturnValueOnce(data);
+			credentialTypes.getByName.calledWith(credential.type).mockReturnValueOnce(credType);
+
+			// ACT
+			const redactedData = service.decrypt(credential);
+
+			// ASSERT
+			expect(redactedData).toEqual({
+				clientId: 'abc123',
+				clientSecret: CREDENTIAL_BLANKING_VALUE,
+				accessToken: CREDENTIAL_EMPTY_VALUE,
+				oauthTokenData: CREDENTIAL_BLANKING_VALUE,
+				csrfSecret: CREDENTIAL_BLANKING_VALUE,
+			});
+		});
+
+		it('should return sensitive values if `includeRawData` is true', () => {
+			// ARRANGE
+			const data = {
+				clientId: 'abc123',
+				clientSecret: 'sensitiveSecret',
+				accessToken: '',
+				oauthTokenData: 'super-secret',
+				csrfSecret: 'super-secret',
+			};
+			const credential = mock<CredentialsEntity>({
+				id: '123',
+				name: 'Test Credential',
+				type: 'oauth2',
+			});
+			jest.spyOn(Credentials.prototype, 'getData').mockReturnValueOnce(data);
+			credentialTypes.getByName.calledWith(credential.type).mockReturnValueOnce(credType);
+
+			// ACT
+			const redactedData = service.decrypt(credential, true);
+
+			// ASSERT
+			expect(redactedData).toEqual(data);
+		});
+	});
 });

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

@@ -1,3 +1,4 @@
+import { CredentialsGetManyRequestQuery, CredentialsGetOneRequestQuery } from '@n8n/api-types';
 import { GlobalConfig } from '@n8n/config';
 // eslint-disable-next-line n8n-local-rules/misplaced-n8n-typeorm-import
 import { In } from '@n8n/typeorm';
@@ -19,6 +20,7 @@ import {
 	RestController,
 	ProjectScope,
 } from '@/decorators';
+import { Param, Query } from '@/decorators/args';
 import { BadRequestError } from '@/errors/response-errors/bad-request.error';
 import { ForbiddenError } from '@/errors/response-errors/forbidden.error';
 import { NotFoundError } from '@/errors/response-errors/not-found.error';
@@ -49,10 +51,15 @@ export class CredentialsController {
 	) {}
 
 	@Get('/', { middlewares: listQueryMiddleware })
-	async getMany(req: CredentialRequest.GetMany) {
+	async getMany(
+		req: CredentialRequest.GetMany,
+		_res: unknown,
+		@Query query: CredentialsGetManyRequestQuery,
+	) {
 		const credentials = await this.credentialsService.getMany(req.user, {
 			listQueryOptions: req.listQueryOptions,
-			includeScopes: req.query.includeScopes,
+			includeScopes: query.includeScopes,
+			includeData: query.includeData,
 		});
 		credentials.forEach((c) => {
 			// @ts-expect-error: This is to emulate the old behavior of removing the shared
@@ -82,21 +89,22 @@ export class CredentialsController {
 
 	@Get('/:credentialId')
 	@ProjectScope('credential:read')
-	async getOne(req: CredentialRequest.Get) {
+	async getOne(
+		req: CredentialRequest.Get,
+		_res: unknown,
+		@Param('credentialId') credentialId: string,
+		@Query query: CredentialsGetOneRequestQuery,
+	) {
 		const { shared, ...credential } = this.license.isSharingEnabled()
 			? await this.enterpriseCredentialsService.getOne(
 					req.user,
-					req.params.credentialId,
+					credentialId,
 					// TODO: editor-ui is always sending this, maybe we can just rely on the
 					// the scopes and always decrypt the data if the user has the permissions
 					// to do so.
-					req.query.includeData === 'true',
+					query.includeData,
 				)
-			: await this.credentialsService.getOne(
-					req.user,
-					req.params.credentialId,
-					req.query.includeData === 'true',
-				);
+			: await this.credentialsService.getOne(req.user, credentialId, query.includeData);
 
 		const scopes = await this.credentialsService.getCredentialScopes(
 			req.user,

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

@@ -87,10 +87,7 @@ export class EnterpriseCredentialsService {
 		if (credential) {
 			// Decrypt the data if we found the credential with the `credential:update`
 			// scope.
-			decryptedData = this.credentialsService.redact(
-				this.credentialsService.decrypt(credential),
-				credential,
-			);
+			decryptedData = this.credentialsService.decrypt(credential);
 		} else {
 			// Otherwise try to find them with only the `credential:read` scope. In
 			// that case we return them without the decrypted data.

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

@@ -38,6 +38,7 @@ import type { CredentialRequest, ListQuery } from '@/requests';
 import { CredentialsTester } from '@/services/credentials-tester.service';
 import { OwnershipService } from '@/services/ownership.service';
 import { ProjectService } from '@/services/project.service.ee';
+import type { ScopesField } from '@/services/role.service';
 import { RoleService } from '@/services/role.service';
 
 export type CredentialsGetSharedOptions =
@@ -62,33 +63,47 @@ export class CredentialsService {
 
 	async getMany(
 		user: User,
-		options: {
-			listQueryOptions?: ListQuery.Options;
-			includeScopes?: string;
+		{
+			listQueryOptions = {},
+			includeScopes = false,
+			includeData = false,
+		}: {
+			listQueryOptions?: ListQuery.Options & { includeData?: boolean };
+			includeScopes?: boolean;
+			includeData?: boolean;
 		} = {},
 	) {
 		const returnAll = user.hasGlobalScope('credential:list');
-		const isDefaultSelect = !options.listQueryOptions?.select;
+		const isDefaultSelect = !listQueryOptions.select;
+
+		if (includeData) {
+			// We need the scopes to check if we're allowed to include the decrypted
+			// data.
+			// Only if the user has the `credential:update` scope the user is allowed
+			// to get the data.
+			includeScopes = true;
+			listQueryOptions.includeData = true;
+		}
 
 		let projectRelations: ProjectRelation[] | undefined = undefined;
-		if (options.includeScopes) {
+		if (includeScopes) {
 			projectRelations = await this.projectService.getProjectRelationsForUser(user);
-			if (options.listQueryOptions?.filter?.projectId && user.hasGlobalScope('credential:list')) {
+			if (listQueryOptions.filter?.projectId && user.hasGlobalScope('credential:list')) {
 				// Only instance owners and admins have the credential:list scope
 				// Those users should be able to use _all_ credentials within their workflows.
 				// TODO: Change this so we filter by `workflowId` in this case. Require a slight FE change
 				const projectRelation = projectRelations.find(
-					(relation) => relation.projectId === options.listQueryOptions?.filter?.projectId,
+					(relation) => relation.projectId === listQueryOptions.filter?.projectId,
 				);
 				if (projectRelation?.role === 'project:personalOwner') {
 					// Will not affect team projects as these have admins, not owners.
-					delete options.listQueryOptions?.filter?.projectId;
+					delete listQueryOptions.filter?.projectId;
 				}
 			}
 		}
 
 		if (returnAll) {
-			let credentials = await this.credentialsRepository.findMany(options.listQueryOptions);
+			let credentials = await this.credentialsRepository.findMany(listQueryOptions);
 
 			if (isDefaultSelect) {
 				// Since we're filtering using project ID as part of the relation,
@@ -96,7 +111,7 @@ export class CredentialsService {
 				// it's shared to a project, it won't be able to find the home project.
 				// To solve this, we have to get all the relation now, even though
 				// we're deleting them later.
-				if ((options.listQueryOptions?.filter?.shared as { projectId?: string })?.projectId) {
+				if ((listQueryOptions.filter?.shared as { projectId?: string })?.projectId) {
 					const relations = await this.sharedCredentialsRepository.getAllRelationsForCredentials(
 						credentials.map((c) => c.id),
 					);
@@ -107,23 +122,32 @@ export class CredentialsService {
 				credentials = credentials.map((c) => this.ownershipService.addOwnedByAndSharedWith(c));
 			}
 
-			if (options.includeScopes) {
+			if (includeScopes) {
 				credentials = credentials.map((c) =>
 					this.roleService.addScopes(c, user, projectRelations!),
 				);
 			}
 
+			if (includeData) {
+				credentials = credentials.map((c: CredentialsEntity & ScopesField) => {
+					return {
+						...c,
+						data: c.scopes.includes('credential:update') ? this.decrypt(c) : undefined,
+					} as unknown as CredentialsEntity;
+				});
+			}
+
 			return credentials;
 		}
 
-		// If the workflow is part of a personal project we want to show the credentials the user making the request has access to, not the credentials the user owning the workflow has access to.
-		if (typeof options.listQueryOptions?.filter?.projectId === 'string') {
-			const project = await this.projectService.getProject(
-				options.listQueryOptions.filter.projectId,
-			);
+		// If the workflow is part of a personal project we want to show the
+		// credentials the user making the request has access to, not the
+		// credentials the user owning the workflow has access to.
+		if (typeof listQueryOptions.filter?.projectId === 'string') {
+			const project = await this.projectService.getProject(listQueryOptions.filter.projectId);
 			if (project?.type === 'personal') {
 				const currentUsersPersonalProject = await this.projectService.getPersonalProject(user);
-				options.listQueryOptions.filter.projectId = currentUsersPersonalProject?.id;
+				listQueryOptions.filter.projectId = currentUsersPersonalProject?.id;
 			}
 		}
 
@@ -132,7 +156,7 @@ export class CredentialsService {
 		});
 
 		let credentials = await this.credentialsRepository.findMany(
-			options.listQueryOptions,
+			listQueryOptions,
 			ids, // only accessible credentials
 		);
 
@@ -142,7 +166,7 @@ export class CredentialsService {
 			// it's shared to a project, it won't be able to find the home project.
 			// To solve this, we have to get all the relation now, even though
 			// we're deleting them later.
-			if ((options.listQueryOptions?.filter?.shared as { projectId?: string })?.projectId) {
+			if ((listQueryOptions.filter?.shared as { projectId?: string })?.projectId) {
 				const relations = await this.sharedCredentialsRepository.getAllRelationsForCredentials(
 					credentials.map((c) => c.id),
 				);
@@ -154,10 +178,19 @@ export class CredentialsService {
 			credentials = credentials.map((c) => this.ownershipService.addOwnedByAndSharedWith(c));
 		}
 
-		if (options.includeScopes) {
+		if (includeScopes) {
 			credentials = credentials.map((c) => this.roleService.addScopes(c, user, projectRelations!));
 		}
 
+		if (includeData) {
+			credentials = credentials.map((c: CredentialsEntity & ScopesField) => {
+				return {
+					...c,
+					data: c.scopes.includes('credential:update') ? this.decrypt(c) : undefined,
+				} as unknown as CredentialsEntity;
+			});
+		}
+
 		return credentials;
 	}
 
@@ -308,9 +341,18 @@ export class CredentialsService {
 		return newCredentialData;
 	}
 
-	decrypt(credential: CredentialsEntity) {
+	/**
+	 * Decrypts the credentials data and redacts the content by default.
+	 *
+	 * If `includeRawData` is set to true it will not redact the data.
+	 */
+	decrypt(credential: CredentialsEntity, includeRawData = false) {
 		const coreCredential = createCredentialsFromCredentialsEntity(credential);
-		return coreCredential.getData();
+		const data = coreCredential.getData();
+		if (includeRawData) {
+			return data;
+		}
+		return this.redact(data, credential);
 	}
 
 	async update(credentialId: string, newCredentialData: ICredentialsDb) {
@@ -500,7 +542,7 @@ export class CredentialsService {
 		if (sharing) {
 			// Decrypt the data if we found the credential with the `credential:update`
 			// scope.
-			decryptedData = this.redact(this.decrypt(sharing.credentials), sharing.credentials);
+			decryptedData = this.decrypt(sharing.credentials);
 		} else {
 			// Otherwise try to find them with only the `credential:read` scope. In
 			// that case we return them without the decrypted data.

packages/cli/src/databases/repositories/__tests__/credentials.repository.test.ts

@@ -0,0 +1,50 @@
+import { mock } from 'jest-mock-extended';
+import { Container } from 'typedi';
+
+import { CredentialsEntity } from '@/databases/entities/credentials-entity';
+import { mockEntityManager } from '@test/mocking';
+
+import { CredentialsRepository } from '../credentials.repository';
+
+const entityManager = mockEntityManager(CredentialsEntity);
+const repository = Container.get(CredentialsRepository);
+
+describe('findMany', () => {
+	const credentialsId = 'cred_123';
+	const credential = mock<CredentialsEntity>({ id: credentialsId });
+
+	beforeEach(() => {
+		jest.resetAllMocks();
+	});
+
+	test('return `data` property if `includeData:true` and select is using the record syntax', async () => {
+		// ARRANGE
+		entityManager.find.mockResolvedValueOnce([credential]);
+
+		// ACT
+		const credentials = await repository.findMany({ includeData: true, select: { id: true } });
+
+		// ASSERT
+		expect(credentials).toHaveLength(1);
+		expect(credentials[0]).toHaveProperty('data');
+	});
+
+	test('return `data` property if `includeData:true` and select is using the record syntax', async () => {
+		// ARRANGE
+		entityManager.find.mockResolvedValueOnce([credential]);
+
+		// ACT
+		const credentials = await repository.findMany({
+			includeData: true,
+			//TODO: fix this
+			// The function's type does not support this but this is what it
+			// actually gets from the service because the middlewares are typed
+			// loosely.
+			select: ['id'] as never,
+		});
+
+		// ASSERT
+		expect(credentials).toHaveLength(1);
+		expect(credentials[0]).toHaveProperty('data');
+	});
+});

packages/cli/src/databases/repositories/credentials.repository.ts

@@ -25,7 +25,10 @@ export class CredentialsRepository extends Repository<CredentialsEntity> {
 		});
 	}
 
-	async findMany(listQueryOptions?: ListQuery.Options, credentialIds?: string[]) {
+	async findMany(
+		listQueryOptions?: ListQuery.Options & { includeData?: boolean },
+		credentialIds?: string[],
+	) {
 		const findManyOptions = this.toFindManyOptions(listQueryOptions);
 
 		if (credentialIds) {
@@ -35,7 +38,7 @@ export class CredentialsRepository extends Repository<CredentialsEntity> {
 		return await this.find(findManyOptions);
 	}
 
-	private toFindManyOptions(listQueryOptions?: ListQuery.Options) {
+	private toFindManyOptions(listQueryOptions?: ListQuery.Options & { includeData?: boolean }) {
 		const findManyOptions: FindManyOptions<CredentialsEntity> = {};
 
 		type Select = Array<keyof CredentialsEntity>;
@@ -74,6 +77,14 @@ export class CredentialsRepository extends Repository<CredentialsEntity> {
 			findManyOptions.relations = defaultRelations;
 		}
 
+		if (listQueryOptions.includeData) {
+			if (Array.isArray(findManyOptions.select)) {
+				findManyOptions.select.push('data');
+			} else {
+				findManyOptions.select.data = true;
+			}
+		}
+
 		return findManyOptions;
 	}
 

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

@@ -226,6 +226,161 @@ describe('GET /credentials', () => {
 		}
 	});
 
+	test('should return data when ?includeData=true', async () => {
+		// ARRANGE
+		const [actor, otherMember] = await createManyUsers(2, {
+			role: 'global:member',
+		});
+
+		const teamProjectViewer = await createTeamProject(undefined);
+		await linkUserToProject(actor, teamProjectViewer, 'project:viewer');
+		const teamProjectEditor = await createTeamProject(undefined);
+		await linkUserToProject(actor, teamProjectEditor, 'project:editor');
+
+		const [
+			// should have data
+			ownedCredential,
+			// should not have
+			sharedCredential,
+			// should not have data
+			teamCredentialAsViewer,
+			// should have data
+			teamCredentialAsEditor,
+		] = await Promise.all([
+			saveCredential(randomCredentialPayload(), { user: actor, role: 'credential:owner' }),
+			saveCredential(randomCredentialPayload(), { user: otherMember, role: 'credential:owner' }),
+			saveCredential(randomCredentialPayload(), {
+				project: teamProjectViewer,
+				role: 'credential:owner',
+			}),
+			saveCredential(randomCredentialPayload(), {
+				project: teamProjectEditor,
+				role: 'credential:owner',
+			}),
+		]);
+		await shareCredentialWithUsers(sharedCredential, [actor]);
+
+		// ACT
+		const response = await testServer
+			.authAgentFor(actor)
+			.get('/credentials')
+			.query({ includeData: true });
+
+		// ASSERT
+		expect(response.statusCode).toBe(200);
+		expect(response.body.data.length).toBe(4);
+
+		const creds = response.body.data as Array<Credentials & { scopes: Scope[] }>;
+		const ownedCred = creds.find((c) => c.id === ownedCredential.id)!;
+		const sharedCred = creds.find((c) => c.id === sharedCredential.id)!;
+		const teamCredAsViewer = creds.find((c) => c.id === teamCredentialAsViewer.id)!;
+		const teamCredAsEditor = creds.find((c) => c.id === teamCredentialAsEditor.id)!;
+
+		expect(ownedCred.id).toBe(ownedCredential.id);
+		expect(ownedCred.data).toBeDefined();
+		expect(ownedCred.scopes).toEqual(
+			[
+				'credential:move',
+				'credential:read',
+				'credential:update',
+				'credential:share',
+				'credential:delete',
+			].sort(),
+		);
+
+		expect(sharedCred.id).toBe(sharedCredential.id);
+		expect(sharedCred.data).not.toBeDefined();
+		expect(sharedCred.scopes).toEqual(['credential:read'].sort());
+
+		expect(teamCredAsViewer.id).toBe(teamCredentialAsViewer.id);
+		expect(teamCredAsViewer.data).not.toBeDefined();
+		expect(teamCredAsViewer.scopes).toEqual(['credential:read'].sort());
+
+		expect(teamCredAsEditor.id).toBe(teamCredentialAsEditor.id);
+		expect(teamCredAsEditor.data).toBeDefined();
+		expect(teamCredAsEditor.scopes).toEqual(
+			['credential:read', 'credential:update', 'credential:delete'].sort(),
+		);
+	});
+
+	test('should return data when ?includeData=true for owners', async () => {
+		// ARRANGE
+		const teamProjectViewer = await createTeamProject(undefined);
+
+		const [
+			// should have data
+			ownedCredential,
+			// should have data
+			sharedCredential,
+			// should have data
+			teamCredentialAsViewer,
+		] = await Promise.all([
+			saveCredential(randomCredentialPayload(), { user: owner, role: 'credential:owner' }),
+			saveCredential(randomCredentialPayload(), { user: member, role: 'credential:owner' }),
+			saveCredential(randomCredentialPayload(), {
+				project: teamProjectViewer,
+				role: 'credential:owner',
+			}),
+		]);
+
+		// ACT
+		const response = await testServer
+			.authAgentFor(owner)
+			.get('/credentials')
+			.query({ includeData: true });
+
+		// ASSERT
+		expect(response.statusCode).toBe(200);
+		expect(response.body.data.length).toBe(3);
+
+		const creds = response.body.data as Array<Credentials & { scopes: Scope[] }>;
+		const ownedCred = creds.find((c) => c.id === ownedCredential.id)!;
+		const sharedCred = creds.find((c) => c.id === sharedCredential.id)!;
+		const teamCredAsViewer = creds.find((c) => c.id === teamCredentialAsViewer.id)!;
+
+		expect(ownedCred.id).toBe(ownedCredential.id);
+		expect(ownedCred.data).toBeDefined();
+		expect(ownedCred.scopes).toEqual(
+			[
+				'credential:move',
+				'credential:read',
+				'credential:update',
+				'credential:share',
+				'credential:delete',
+				'credential:create',
+				'credential:list',
+			].sort(),
+		);
+
+		expect(sharedCred.id).toBe(sharedCredential.id);
+		expect(sharedCred.data).toBeDefined();
+		expect(sharedCred.scopes).toEqual(
+			[
+				'credential:move',
+				'credential:read',
+				'credential:update',
+				'credential:share',
+				'credential:delete',
+				'credential:create',
+				'credential:list',
+			].sort(),
+		);
+
+		expect(teamCredAsViewer.id).toBe(teamCredentialAsViewer.id);
+		expect(teamCredAsViewer.data).toBeDefined();
+		expect(teamCredAsViewer.scopes).toEqual(
+			[
+				'credential:move',
+				'credential:read',
+				'credential:update',
+				'credential:share',
+				'credential:delete',
+				'credential:create',
+				'credential:list',
+			].sort(),
+		);
+	});
+
 	describe('should return', () => {
 		test('all credentials for owner', async () => {
 			const { id: id1 } = await saveCredential(payload(), {