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

Co-authored-by: r00gm <[email protected]>

packages/@n8n/api-types/package.json

@@ -27,6 +27,6 @@
   "dependencies": {
     "xss": "catalog:",
     "zod": "catalog:",
-    "zod-class": "0.0.15"
+    "zod-class": "0.0.16"
   }
 }

packages/@n8n/api-types/src/dto/credentials/__tests__/credentials-get-many-request.dto.test.ts

@@ -0,0 +1,55 @@
+import { CredentialsGetManyRequestQuery } from '../credentials-get-many-request.dto';
+
+describe('CredentialsGetManyRequestQuery', () => {
+	describe('should pass validation', () => {
+		it('with empty object', () => {
+			const data = {};
+
+			const result = CredentialsGetManyRequestQuery.safeParse(data);
+
+			expect(result.success).toBe(true);
+		});
+
+		test.each([
+			{ field: 'includeScopes', value: 'true' },
+			{ field: 'includeScopes', value: 'false' },
+			{ field: 'includeData', value: 'true' },
+			{ field: 'includeData', value: 'false' },
+		])('with $field set to $value', ({ field, value }) => {
+			const data = { [field]: value };
+
+			const result = CredentialsGetManyRequestQuery.safeParse(data);
+
+			expect(result.success).toBe(true);
+		});
+
+		it('with both parameters set', () => {
+			const data = {
+				includeScopes: 'true',
+				includeData: 'true',
+			};
+
+			const result = CredentialsGetManyRequestQuery.safeParse(data);
+
+			expect(result.success).toBe(true);
+		});
+	});
+
+	describe('should fail validation', () => {
+		test.each([
+			{ field: 'includeScopes', value: true },
+			{ field: 'includeScopes', value: false },
+			{ field: 'includeScopes', value: 'invalid' },
+			{ field: 'includeData', value: true },
+			{ field: 'includeData', value: false },
+			{ field: 'includeData', value: 'invalid' },
+		])('with invalid value $value for $field', ({ field, value }) => {
+			const data = { [field]: value };
+
+			const result = CredentialsGetManyRequestQuery.safeParse(data);
+
+			expect(result.success).toBe(false);
+			expect(result.error?.issues[0].path[0]).toBe(field);
+		});
+	});
+});

packages/@n8n/api-types/src/dto/credentials/__tests__/credentials-get-one-request.dto.test.ts

@@ -0,0 +1,52 @@
+import { CredentialsGetOneRequestQuery } from '../credentials-get-one-request.dto';
+
+describe('CredentialsGetManyRequestQuery', () => {
+	describe('should pass validation', () => {
+		it('with empty object', () => {
+			const data = {};
+
+			const result = CredentialsGetOneRequestQuery.safeParse(data);
+
+			expect(result.success).toBe(true);
+			// defaults to false
+			expect(result.data?.includeData).toBe(false);
+		});
+
+		test.each([
+			{ field: 'includeData', value: 'true' },
+			{ field: 'includeData', value: 'false' },
+		])('with $field set to $value', ({ field, value }) => {
+			const data = { [field]: value };
+
+			const result = CredentialsGetOneRequestQuery.safeParse(data);
+
+			expect(result.success).toBe(true);
+		});
+
+		it('with both parameters set', () => {
+			const data = {
+				includeScopes: 'true',
+				includeData: 'true',
+			};
+
+			const result = CredentialsGetOneRequestQuery.safeParse(data);
+
+			expect(result.success).toBe(true);
+		});
+	});
+
+	describe('should fail validation', () => {
+		test.each([
+			{ field: 'includeData', value: true },
+			{ field: 'includeData', value: false },
+			{ field: 'includeData', value: 'invalid' },
+		])('with invalid value $value for $field', ({ field, value }) => {
+			const data = { [field]: value };
+
+			const result = CredentialsGetOneRequestQuery.safeParse(data);
+
+			expect(result.success).toBe(false);
+			expect(result.error?.issues[0].path[0]).toBe(field);
+		});
+	});
+});

packages/@n8n/api-types/src/dto/credentials/credentials-get-many-request.dto.ts

@@ -0,0 +1,22 @@
+import { Z } from 'zod-class';
+
+import { booleanFromString } from '../../schemas/booleanFromString';
+
+export class CredentialsGetManyRequestQuery extends Z.class({
+	/**
+	 * Adds the `scopes` field to each credential which includes all scopes the
+	 * requesting user has in relation to the credential, e.g.
+	 *     ['credential:read', 'credential:update']
+	 */
+	includeScopes: booleanFromString.optional(),
+
+	/**
+	 * Adds the decrypted `data` field to each credential.
+	 *
+	 * It only does this for credentials for which the user has the
+	 * `credential:update` scope.
+	 *
+	 * This switches `includeScopes` to true to be able to check for the scopes
+	 */
+	includeData: booleanFromString.optional(),
+}) {}

packages/@n8n/api-types/src/dto/credentials/credentials-get-one-request.dto.ts

@@ -0,0 +1,13 @@
+import { Z } from 'zod-class';
+
+import { booleanFromString } from '../../schemas/booleanFromString';
+
+export class CredentialsGetOneRequestQuery extends Z.class({
+	/**
+	 * Adds the decrypted `data` field to each credential.
+	 *
+	 * It only does this for credentials for which the user has the
+	 * `credential:update` scope.
+	 */
+	includeData: booleanFromString.optional().default('false'),
+}) {}

packages/@n8n/api-types/src/dto/index.ts

@@ -29,3 +29,5 @@ export { UserUpdateRequestDto } from './user/user-update-request.dto';
 export { CommunityRegisteredRequestDto } from './license/community-registered-request.dto';
 
 export { VariableListRequestDto } from './variables/variables-list-request.dto';
+export { CredentialsGetOneRequestQuery } from './credentials/credentials-get-one-request.dto';
+export { CredentialsGetManyRequestQuery } from './credentials/credentials-get-many-request.dto';

packages/@n8n/api-types/src/schemas/booleanFromString.ts

@@ -0,0 +1,3 @@
+import { z } from 'zod';
+
+export const booleanFromString = z.enum(['true', 'false']).transform((value) => value === 'true');

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.