Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
Merged
Show file tree
Hide file tree
Changes from 18 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/@n8n/api-types/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,6 @@
"dependencies": {
"xss": "catalog:",
"zod": "catalog:",
"zod-class": "0.0.15"
"zod-class": "0.0.16"
}
}
Original file line number Diff line number Diff line change
@@ -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);
});
});
});
Original file line number Diff line number Diff line change
@@ -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);
});
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
import { Z } from 'zod-class';

import { booleanFromString } from '../../schemas/booleanFromString';

export class CredentialsGetManyRequestQuery extends Z.class({
despairblue marked this conversation as resolved.
Show resolved Hide resolved
/**
* 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(),
}) {}
Original file line number Diff line number Diff line change
@@ -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'),
}) {}
2 changes: 2 additions & 0 deletions packages/@n8n/api-types/src/dto/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
3 changes: 3 additions & 0 deletions packages/@n8n/api-types/src/schemas/booleanFromString.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
import { z } from 'zod';

export const booleanFromString = z.enum(['true', 'false']).transform((value) => value === 'true');
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
import { mock } from 'jest-mock-extended';
import { nanoId, date } from 'minifaker';
import { Cipher } from 'n8n-core';
import { CREDENTIAL_EMPTY_VALUE, type ICredentialType } from 'n8n-workflow';
import Container from 'typedi';

import { CREDENTIAL_BLANKING_VALUE } from '@/constants';
import type { CredentialTypes } from '@/credential-types';
Expand Down Expand Up @@ -30,6 +32,8 @@ describe('CredentialsService', () => {
},
],
});

const cipher = Container.get(Cipher);
const credentialTypes = mock<CredentialTypes>();
const service = new CredentialsService(
mock(),
Expand Down Expand Up @@ -61,7 +65,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);

Expand Down Expand Up @@ -137,4 +141,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',
data: cipher.encrypt(data),
});
credentialTypes.getByName.calledWith(credential.type).mockReturnValueOnce(credType);

// ACT
const redactedData = service.decrypt(credential);
despairblue marked this conversation as resolved.
Show resolved Hide resolved

// 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',
data: cipher.encrypt(data),
});
credentialTypes.getByName.calledWith(credential.type).mockReturnValueOnce(credType);

// ACT
const redactedData = service.decrypt(credential, true);

// ASSERT
expect(redactedData).toEqual(data);
});
});
});
28 changes: 18 additions & 10 deletions packages/cli/src/credentials/credentials.controller.ts
Original file line number Diff line number Diff line change
@@ -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';
Expand All @@ -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';
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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,
Expand Down
5 changes: 1 addition & 4 deletions packages/cli/src/credentials/credentials.service.ee.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
Loading
Loading