From 6093c5a95e51f20576af3d667a0ccb52680dd721 Mon Sep 17 00:00:00 2001 From: Danny Martini Date: Fri, 13 Dec 2024 20:18:52 +0100 Subject: [PATCH 01/19] add DTOs for `GET /credentials` and `GET /credentials/:credentialId` --- .../credentials-get-many-request.dto.ts | 7 +++++ .../credentials-get-one-request.dto.ts | 6 +++++ packages/@n8n/api-types/src/dto/index.ts | 2 ++ .../src/credentials/credentials.controller.ts | 27 ++++++++++++------- .../src/credentials/credentials.service.ts | 2 +- 5 files changed, 33 insertions(+), 11 deletions(-) create mode 100644 packages/@n8n/api-types/src/dto/credentials/credentials-get-many-request.dto.ts create mode 100644 packages/@n8n/api-types/src/dto/credentials/credentials-get-one-request.dto.ts diff --git a/packages/@n8n/api-types/src/dto/credentials/credentials-get-many-request.dto.ts b/packages/@n8n/api-types/src/dto/credentials/credentials-get-many-request.dto.ts new file mode 100644 index 0000000000000..c9d98953352cf --- /dev/null +++ b/packages/@n8n/api-types/src/dto/credentials/credentials-get-many-request.dto.ts @@ -0,0 +1,7 @@ +import { z } from 'zod'; +import { Z } from 'zod-class'; + +// TODO: document what these flags do +export class CredentialsGetManyRequest extends Z.class({ + includeScopes: z.union([z.literal('true'), z.literal('false')]).optional(), +}) {} diff --git a/packages/@n8n/api-types/src/dto/credentials/credentials-get-one-request.dto.ts b/packages/@n8n/api-types/src/dto/credentials/credentials-get-one-request.dto.ts new file mode 100644 index 0000000000000..a049f2997e04c --- /dev/null +++ b/packages/@n8n/api-types/src/dto/credentials/credentials-get-one-request.dto.ts @@ -0,0 +1,6 @@ +import { z } from 'zod'; +import { Z } from 'zod-class'; + +export class CredentialsGetOneRequest extends Z.class({ + includeData: z.union([z.literal('true'), z.literal('false')]).optional(), +}) {} diff --git a/packages/@n8n/api-types/src/dto/index.ts b/packages/@n8n/api-types/src/dto/index.ts index e2642746c7660..026c084c56980 100644 --- a/packages/@n8n/api-types/src/dto/index.ts +++ b/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 { CredentialsGetOneRequest } from './credentials/credentials-get-one-request.dto'; +export { CredentialsGetManyRequest } from './credentials/credentials-get-many-request.dto'; diff --git a/packages/cli/src/credentials/credentials.controller.ts b/packages/cli/src/credentials/credentials.controller.ts index 934e91f64dee7..a15d2ae97a44b 100644 --- a/packages/cli/src/credentials/credentials.controller.ts +++ b/packages/cli/src/credentials/credentials.controller.ts @@ -1,3 +1,4 @@ +import { CredentialsGetManyRequest, CredentialsGetOneRequest } 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,14 @@ export class CredentialsController { ) {} @Get('/', { middlewares: listQueryMiddleware }) - async getMany(req: CredentialRequest.GetMany) { + async getMany( + req: CredentialRequest.GetMany, + _res: unknown, + @Query query: CredentialsGetManyRequest, + ) { const credentials = await this.credentialsService.getMany(req.user, { listQueryOptions: req.listQueryOptions, - includeScopes: req.query.includeScopes, + includeScopes: query.includeScopes === 'true', }); credentials.forEach((c) => { // @ts-expect-error: This is to emulate the old behavior of removing the shared @@ -82,21 +88,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: CredentialsGetOneRequest, + ) { 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 === 'true', ) - : await this.credentialsService.getOne( - req.user, - req.params.credentialId, - req.query.includeData === 'true', - ); + : await this.credentialsService.getOne(req.user, credentialId, query.includeData === 'true'); const scopes = await this.credentialsService.getCredentialScopes( req.user, diff --git a/packages/cli/src/credentials/credentials.service.ts b/packages/cli/src/credentials/credentials.service.ts index 84398b5475776..90214c57f9572 100644 --- a/packages/cli/src/credentials/credentials.service.ts +++ b/packages/cli/src/credentials/credentials.service.ts @@ -64,7 +64,7 @@ export class CredentialsService { user: User, options: { listQueryOptions?: ListQuery.Options; - includeScopes?: string; + includeScopes?: boolean; } = {}, ) { const returnAll = user.hasGlobalScope('credential:list'); From fc99b5543d31f503f3a9afe90eff3963210bde5c Mon Sep 17 00:00:00 2001 From: Danny Martini Date: Fri, 13 Dec 2024 20:21:06 +0100 Subject: [PATCH 02/19] destructure options and assign defaults --- .../src/credentials/credentials.service.ts | 45 ++++++++++--------- 1 file changed, 24 insertions(+), 21 deletions(-) diff --git a/packages/cli/src/credentials/credentials.service.ts b/packages/cli/src/credentials/credentials.service.ts index 90214c57f9572..ace7886e0db3d 100644 --- a/packages/cli/src/credentials/credentials.service.ts +++ b/packages/cli/src/credentials/credentials.service.ts @@ -62,33 +62,36 @@ export class CredentialsService { async getMany( user: User, - options: { - listQueryOptions?: ListQuery.Options; - includeScopes?: boolean; - } = {}, + { + listQueryOptions = {}, + includeScopes = false, + }: { + listQueryOptions: ListQuery.Options; + includeScopes: boolean; + }, ) { const returnAll = user.hasGlobalScope('credential:list'); - const isDefaultSelect = !options.listQueryOptions?.select; + const isDefaultSelect = !listQueryOptions.select; 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 +99,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,7 +110,7 @@ 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!), ); @@ -116,14 +119,14 @@ export class CredentialsService { 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 +135,7 @@ export class CredentialsService { }); let credentials = await this.credentialsRepository.findMany( - options.listQueryOptions, + listQueryOptions, ids, // only accessible credentials ); @@ -142,7 +145,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,7 +157,7 @@ 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!)); } From bbc3f95615173373fbdbb1d0cd11f3d9b853d75c Mon Sep 17 00:00:00 2001 From: Danny Martini Date: Fri, 13 Dec 2024 20:35:04 +0100 Subject: [PATCH 03/19] feat(core): Add `includeData` parameter to `GET /credentials` This returns the decrypted data property of credentials if the user has the `credential:update` scope for it. --- .../credentials-get-many-request.dto.ts | 15 ++ .../src/credentials/credentials.controller.ts | 1 + .../src/credentials/credentials.service.ts | 44 ++++- .../repositories/credentials.repository.ts | 16 +- packages/cli/src/license.ts | 2 + .../credentials/credentials.api.test.ts | 156 ++++++++++++++++++ 6 files changed, 232 insertions(+), 2 deletions(-) diff --git a/packages/@n8n/api-types/src/dto/credentials/credentials-get-many-request.dto.ts b/packages/@n8n/api-types/src/dto/credentials/credentials-get-many-request.dto.ts index c9d98953352cf..b998fc50aa41d 100644 --- a/packages/@n8n/api-types/src/dto/credentials/credentials-get-many-request.dto.ts +++ b/packages/@n8n/api-types/src/dto/credentials/credentials-get-many-request.dto.ts @@ -3,5 +3,20 @@ import { Z } from 'zod-class'; // TODO: document what these flags do export class CredentialsGetManyRequest 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: z.union([z.literal('true'), z.literal('false')]).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: z.union([z.literal('true'), z.literal('false')]).optional(), }) {} diff --git a/packages/cli/src/credentials/credentials.controller.ts b/packages/cli/src/credentials/credentials.controller.ts index a15d2ae97a44b..662ced8f82a53 100644 --- a/packages/cli/src/credentials/credentials.controller.ts +++ b/packages/cli/src/credentials/credentials.controller.ts @@ -59,6 +59,7 @@ export class CredentialsController { const credentials = await this.credentialsService.getMany(req.user, { listQueryOptions: req.listQueryOptions, includeScopes: query.includeScopes === 'true', + includeData: query.includeData === 'true', }); credentials.forEach((c) => { // @ts-expect-error: This is to emulate the old behavior of removing the shared diff --git a/packages/cli/src/credentials/credentials.service.ts b/packages/cli/src/credentials/credentials.service.ts index ace7886e0db3d..07a9bb737e2d3 100644 --- a/packages/cli/src/credentials/credentials.service.ts +++ b/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 = @@ -65,14 +66,25 @@ export class CredentialsService { { listQueryOptions = {}, includeScopes = false, + includeData = false, }: { - listQueryOptions: ListQuery.Options; + listQueryOptions: ListQuery.Options & { includeData?: boolean }; includeScopes: boolean; + includeData: boolean; }, ) { const returnAll = user.hasGlobalScope('credential:list'); 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 (includeScopes) { projectRelations = await this.projectService.getProjectRelationsForUser(user); @@ -116,6 +128,21 @@ export class CredentialsService { ); } + if (includeData) { + credentials = credentials.map((c: CredentialsEntity & ScopesField) => { + if (c.scopes.includes('credential:update')) { + return { + ...c, + data: this.decrypt(c), + } as unknown as CredentialsEntity; + } + return { + ...c, + data: undefined, + } as unknown as CredentialsEntity; + }); + } + return credentials; } @@ -161,6 +188,21 @@ export class CredentialsService { credentials = credentials.map((c) => this.roleService.addScopes(c, user, projectRelations!)); } + if (includeData) { + credentials = credentials.map((c: CredentialsEntity & ScopesField) => { + if (c.scopes.includes('credential:update')) { + return { + ...c, + data: this.decrypt(c), + } as unknown as CredentialsEntity; + } + return { + ...c, + data: undefined, + } as unknown as CredentialsEntity; + }); + } + return credentials; } diff --git a/packages/cli/src/databases/repositories/credentials.repository.ts b/packages/cli/src/databases/repositories/credentials.repository.ts index e5aecf1069f0e..6e71576a67fb8 100644 --- a/packages/cli/src/databases/repositories/credentials.repository.ts +++ b/packages/cli/src/databases/repositories/credentials.repository.ts @@ -25,9 +25,23 @@ export class CredentialsRepository extends Repository { }); } - async findMany(listQueryOptions?: ListQuery.Options, credentialIds?: string[]) { + async findMany( + listQueryOptions?: ListQuery.Options & { includeData?: boolean }, + credentialIds?: string[], + ) { const findManyOptions = this.toFindManyOptions(listQueryOptions); + if (listQueryOptions?.includeData) { + if (Array.isArray(findManyOptions.select)) { + findManyOptions.select.push('data'); + } else if (typeof findManyOptions.select === 'object') { + findManyOptions.select.data = true; + } else { + findManyOptions.select ??= []; + findManyOptions.select.push('data'); + } + } + if (credentialIds) { findManyOptions.where = { ...findManyOptions.where, id: In(credentialIds) }; } diff --git a/packages/cli/src/license.ts b/packages/cli/src/license.ts index ded98d3f3c01c..8a8fe5e91926a 100644 --- a/packages/cli/src/license.ts +++ b/packages/cli/src/license.ts @@ -228,6 +228,7 @@ export class License { } isFeatureEnabled(feature: BooleanLicenseFeature) { + return true; return this.manager?.hasFeatureEnabled(feature) ?? false; } @@ -380,6 +381,7 @@ export class License { } getTeamProjectLimit() { + return UNLIMITED_LICENSE_QUOTA; return this.getFeatureValue(LICENSE_QUOTAS.TEAM_PROJECT_LIMIT) ?? 0; } diff --git a/packages/cli/test/integration/credentials/credentials.api.test.ts b/packages/cli/test/integration/credentials/credentials.api.test.ts index eb9712fefd00d..c50e75c5c756a 100644 --- a/packages/cli/test/integration/credentials/credentials.api.test.ts +++ b/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; + 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; + 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(), { @@ -537,6 +692,7 @@ describe('GET /credentials', () => { .authAgentFor(owner) .get('/credentials') .query('select=["type"]') + .query(JSON.stringify({ select: '' })) .expect(200); expect(response.body).toEqual({ From fe46872bf600a20d21a2f29c1ea35dc380ad1ad8 Mon Sep 17 00:00:00 2001 From: r00gm Date: Wed, 18 Dec 2024 09:16:33 +0100 Subject: [PATCH 04/19] fix: default value type --- packages/cli/src/credentials/credentials.service.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/cli/src/credentials/credentials.service.ts b/packages/cli/src/credentials/credentials.service.ts index 07a9bb737e2d3..7ddddd28beaa2 100644 --- a/packages/cli/src/credentials/credentials.service.ts +++ b/packages/cli/src/credentials/credentials.service.ts @@ -68,10 +68,10 @@ export class CredentialsService { includeScopes = false, includeData = false, }: { - listQueryOptions: ListQuery.Options & { includeData?: boolean }; - includeScopes: boolean; - includeData: boolean; - }, + listQueryOptions?: ListQuery.Options & { includeData?: boolean }; + includeScopes?: boolean; + includeData?: boolean; + } = {}, ) { const returnAll = user.hasGlobalScope('credential:list'); const isDefaultSelect = !listQueryOptions.select; From e57c92c34856bcf599b11c05a61764fa812ee2c1 Mon Sep 17 00:00:00 2001 From: r00gm Date: Wed, 18 Dec 2024 09:20:05 +0100 Subject: [PATCH 05/19] chore: remove mocks --- packages/cli/src/license.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/packages/cli/src/license.ts b/packages/cli/src/license.ts index 8a8fe5e91926a..ded98d3f3c01c 100644 --- a/packages/cli/src/license.ts +++ b/packages/cli/src/license.ts @@ -228,7 +228,6 @@ export class License { } isFeatureEnabled(feature: BooleanLicenseFeature) { - return true; return this.manager?.hasFeatureEnabled(feature) ?? false; } @@ -381,7 +380,6 @@ export class License { } getTeamProjectLimit() { - return UNLIMITED_LICENSE_QUOTA; return this.getFeatureValue(LICENSE_QUOTAS.TEAM_PROJECT_LIMIT) ?? 0; } From 37834b3cfb2cd67f5ab3c782c40417d2ccd6c3ab Mon Sep 17 00:00:00 2001 From: Danny Martini Date: Fri, 27 Dec 2024 10:26:11 +0100 Subject: [PATCH 06/19] apply feedback --- packages/@n8n/api-types/src/dto/common.ts | 3 +++ .../credentials-get-many-request.dto.ts | 10 ++++---- .../credentials-get-one-request.dto.ts | 7 +++--- packages/@n8n/api-types/src/dto/index.ts | 4 ++-- .../src/credentials/credentials.controller.ts | 6 ++--- .../src/credentials/credentials.service.ts | 16 ++----------- .../repositories/credentials.repository.ts | 24 +++++++++---------- 7 files changed, 31 insertions(+), 39 deletions(-) create mode 100644 packages/@n8n/api-types/src/dto/common.ts diff --git a/packages/@n8n/api-types/src/dto/common.ts b/packages/@n8n/api-types/src/dto/common.ts new file mode 100644 index 0000000000000..51679aa0110a0 --- /dev/null +++ b/packages/@n8n/api-types/src/dto/common.ts @@ -0,0 +1,3 @@ +import { z } from 'zod'; + +export const booleanLiteral = z.union([z.literal('true'), z.literal('false')]); diff --git a/packages/@n8n/api-types/src/dto/credentials/credentials-get-many-request.dto.ts b/packages/@n8n/api-types/src/dto/credentials/credentials-get-many-request.dto.ts index b998fc50aa41d..a798554f55878 100644 --- a/packages/@n8n/api-types/src/dto/credentials/credentials-get-many-request.dto.ts +++ b/packages/@n8n/api-types/src/dto/credentials/credentials-get-many-request.dto.ts @@ -1,14 +1,14 @@ -import { z } from 'zod'; import { Z } from 'zod-class'; -// TODO: document what these flags do -export class CredentialsGetManyRequest extends Z.class({ +import { booleanLiteral } from 'dto/common'; + +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: z.union([z.literal('true'), z.literal('false')]).optional(), + includeScopes: booleanLiteral.optional(), /** * Adds the decrypted `data` field to each credential. @@ -18,5 +18,5 @@ export class CredentialsGetManyRequest extends Z.class({ * * This switches `includeScopes` to true to be able to check for the scopes */ - includeData: z.union([z.literal('true'), z.literal('false')]).optional(), + includeData: booleanLiteral.optional(), }) {} diff --git a/packages/@n8n/api-types/src/dto/credentials/credentials-get-one-request.dto.ts b/packages/@n8n/api-types/src/dto/credentials/credentials-get-one-request.dto.ts index a049f2997e04c..6343115af8100 100644 --- a/packages/@n8n/api-types/src/dto/credentials/credentials-get-one-request.dto.ts +++ b/packages/@n8n/api-types/src/dto/credentials/credentials-get-one-request.dto.ts @@ -1,6 +1,7 @@ -import { z } from 'zod'; import { Z } from 'zod-class'; -export class CredentialsGetOneRequest extends Z.class({ - includeData: z.union([z.literal('true'), z.literal('false')]).optional(), +import { booleanLiteral } from 'dto/common'; + +export class CredentialsGetOneRequestQuery extends Z.class({ + includeData: booleanLiteral.optional(), }) {} diff --git a/packages/@n8n/api-types/src/dto/index.ts b/packages/@n8n/api-types/src/dto/index.ts index 026c084c56980..ea725e2ec5bbc 100644 --- a/packages/@n8n/api-types/src/dto/index.ts +++ b/packages/@n8n/api-types/src/dto/index.ts @@ -29,5 +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 { CredentialsGetOneRequest } from './credentials/credentials-get-one-request.dto'; -export { CredentialsGetManyRequest } from './credentials/credentials-get-many-request.dto'; +export { CredentialsGetOneRequestQuery } from './credentials/credentials-get-one-request.dto'; +export { CredentialsGetManyRequestQuery } from './credentials/credentials-get-many-request.dto'; diff --git a/packages/cli/src/credentials/credentials.controller.ts b/packages/cli/src/credentials/credentials.controller.ts index 662ced8f82a53..ce4968104a8de 100644 --- a/packages/cli/src/credentials/credentials.controller.ts +++ b/packages/cli/src/credentials/credentials.controller.ts @@ -1,4 +1,4 @@ -import { CredentialsGetManyRequest, CredentialsGetOneRequest } from '@n8n/api-types'; +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'; @@ -54,7 +54,7 @@ export class CredentialsController { async getMany( req: CredentialRequest.GetMany, _res: unknown, - @Query query: CredentialsGetManyRequest, + @Query query: CredentialsGetManyRequestQuery, ) { const credentials = await this.credentialsService.getMany(req.user, { listQueryOptions: req.listQueryOptions, @@ -93,7 +93,7 @@ export class CredentialsController { req: CredentialRequest.Get, _res: unknown, @Param('credentialId') credentialId: string, - @Query query: CredentialsGetOneRequest, + @Query query: CredentialsGetOneRequestQuery, ) { const { shared, ...credential } = this.license.isSharingEnabled() ? await this.enterpriseCredentialsService.getOne( diff --git a/packages/cli/src/credentials/credentials.service.ts b/packages/cli/src/credentials/credentials.service.ts index 7ddddd28beaa2..670d9fed132b7 100644 --- a/packages/cli/src/credentials/credentials.service.ts +++ b/packages/cli/src/credentials/credentials.service.ts @@ -130,15 +130,9 @@ export class CredentialsService { if (includeData) { credentials = credentials.map((c: CredentialsEntity & ScopesField) => { - if (c.scopes.includes('credential:update')) { - return { - ...c, - data: this.decrypt(c), - } as unknown as CredentialsEntity; - } return { ...c, - data: undefined, + data: c.scopes.includes('credential:update') ? this.decrypt(c) : undefined, } as unknown as CredentialsEntity; }); } @@ -190,15 +184,9 @@ export class CredentialsService { if (includeData) { credentials = credentials.map((c: CredentialsEntity & ScopesField) => { - if (c.scopes.includes('credential:update')) { - return { - ...c, - data: this.decrypt(c), - } as unknown as CredentialsEntity; - } return { ...c, - data: undefined, + data: c.scopes.includes('credential:update') ? this.decrypt(c) : undefined, } as unknown as CredentialsEntity; }); } diff --git a/packages/cli/src/databases/repositories/credentials.repository.ts b/packages/cli/src/databases/repositories/credentials.repository.ts index 6e71576a67fb8..dda47aed75566 100644 --- a/packages/cli/src/databases/repositories/credentials.repository.ts +++ b/packages/cli/src/databases/repositories/credentials.repository.ts @@ -31,17 +31,6 @@ export class CredentialsRepository extends Repository { ) { const findManyOptions = this.toFindManyOptions(listQueryOptions); - if (listQueryOptions?.includeData) { - if (Array.isArray(findManyOptions.select)) { - findManyOptions.select.push('data'); - } else if (typeof findManyOptions.select === 'object') { - findManyOptions.select.data = true; - } else { - findManyOptions.select ??= []; - findManyOptions.select.push('data'); - } - } - if (credentialIds) { findManyOptions.where = { ...findManyOptions.where, id: In(credentialIds) }; } @@ -49,7 +38,7 @@ export class CredentialsRepository extends Repository { return await this.find(findManyOptions); } - private toFindManyOptions(listQueryOptions?: ListQuery.Options) { + private toFindManyOptions(listQueryOptions?: ListQuery.Options & { includeData?: boolean }) { const findManyOptions: FindManyOptions = {}; type Select = Array; @@ -88,6 +77,17 @@ export class CredentialsRepository extends Repository { findManyOptions.relations = defaultRelations; } + if (listQueryOptions.includeData) { + if (Array.isArray(findManyOptions.select)) { + findManyOptions.select.push('data'); + } else if (typeof findManyOptions.select === 'object') { + findManyOptions.select.data = true; + } else { + findManyOptions.select ??= []; + findManyOptions.select.push('data'); + } + } + return findManyOptions; } From e9220ce34848e370371c433c479a6a8441c6f91f Mon Sep 17 00:00:00 2001 From: Danny Martini Date: Fri, 27 Dec 2024 10:26:44 +0100 Subject: [PATCH 07/19] redact by default when decrypting credentials --- .../cli/src/credentials/credentials.service.ee.ts | 5 +---- .../cli/src/credentials/credentials.service.ts | 15 ++++++++++++--- 2 files changed, 13 insertions(+), 7 deletions(-) diff --git a/packages/cli/src/credentials/credentials.service.ee.ts b/packages/cli/src/credentials/credentials.service.ee.ts index 274feff81b6c4..949748600db5e 100644 --- a/packages/cli/src/credentials/credentials.service.ee.ts +++ b/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. diff --git a/packages/cli/src/credentials/credentials.service.ts b/packages/cli/src/credentials/credentials.service.ts index 670d9fed132b7..e7e0447417e4f 100644 --- a/packages/cli/src/credentials/credentials.service.ts +++ b/packages/cli/src/credentials/credentials.service.ts @@ -341,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) { @@ -533,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. From 733d7edf3bf98c98e6043c8d0b3880bd8b47c087 Mon Sep 17 00:00:00 2001 From: Danny Martini Date: Fri, 27 Dec 2024 10:46:51 +0100 Subject: [PATCH 08/19] add docstring --- .../src/dto/credentials/credentials-get-one-request.dto.ts | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/packages/@n8n/api-types/src/dto/credentials/credentials-get-one-request.dto.ts b/packages/@n8n/api-types/src/dto/credentials/credentials-get-one-request.dto.ts index 6343115af8100..83cb3e05ee824 100644 --- a/packages/@n8n/api-types/src/dto/credentials/credentials-get-one-request.dto.ts +++ b/packages/@n8n/api-types/src/dto/credentials/credentials-get-one-request.dto.ts @@ -3,5 +3,11 @@ import { Z } from 'zod-class'; import { booleanLiteral } from 'dto/common'; 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: booleanLiteral.optional(), }) {} From f6b2b2fc5644f19affbd782933a4c648db1633c3 Mon Sep 17 00:00:00 2001 From: Danny Martini Date: Fri, 27 Dec 2024 10:51:29 +0100 Subject: [PATCH 09/19] fix imports --- .../src/dto/credentials/credentials-get-many-request.dto.ts | 2 +- .../src/dto/credentials/credentials-get-one-request.dto.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/@n8n/api-types/src/dto/credentials/credentials-get-many-request.dto.ts b/packages/@n8n/api-types/src/dto/credentials/credentials-get-many-request.dto.ts index a798554f55878..36cbddf0ba26a 100644 --- a/packages/@n8n/api-types/src/dto/credentials/credentials-get-many-request.dto.ts +++ b/packages/@n8n/api-types/src/dto/credentials/credentials-get-many-request.dto.ts @@ -1,6 +1,6 @@ import { Z } from 'zod-class'; -import { booleanLiteral } from 'dto/common'; +import { booleanLiteral } from '../common'; export class CredentialsGetManyRequestQuery extends Z.class({ /** diff --git a/packages/@n8n/api-types/src/dto/credentials/credentials-get-one-request.dto.ts b/packages/@n8n/api-types/src/dto/credentials/credentials-get-one-request.dto.ts index 83cb3e05ee824..d6994297d4cd5 100644 --- a/packages/@n8n/api-types/src/dto/credentials/credentials-get-one-request.dto.ts +++ b/packages/@n8n/api-types/src/dto/credentials/credentials-get-one-request.dto.ts @@ -1,6 +1,6 @@ import { Z } from 'zod-class'; -import { booleanLiteral } from 'dto/common'; +import { booleanLiteral } from '../common'; export class CredentialsGetOneRequestQuery extends Z.class({ /** From d38e918c21a3c75e05c3902f9260d70b1dbdbcf4 Mon Sep 17 00:00:00 2001 From: Danny Martini Date: Fri, 27 Dec 2024 10:51:36 +0100 Subject: [PATCH 10/19] remove unintended change --- .../cli/test/integration/credentials/credentials.api.test.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/cli/test/integration/credentials/credentials.api.test.ts b/packages/cli/test/integration/credentials/credentials.api.test.ts index c50e75c5c756a..2b3ee73b582fa 100644 --- a/packages/cli/test/integration/credentials/credentials.api.test.ts +++ b/packages/cli/test/integration/credentials/credentials.api.test.ts @@ -692,7 +692,6 @@ describe('GET /credentials', () => { .authAgentFor(owner) .get('/credentials') .query('select=["type"]') - .query(JSON.stringify({ select: '' })) .expect(200); expect(response.body).toEqual({ From e8143d2e64b16d12d1b823e1093d2dccd8bfc72f Mon Sep 17 00:00:00 2001 From: Danny Martini Date: Mon, 30 Dec 2024 10:50:07 +0100 Subject: [PATCH 11/19] update zod-class --- packages/@n8n/api-types/package.json | 2 +- pnpm-lock.yaml | 42 +++++++++++----------------- 2 files changed, 18 insertions(+), 26 deletions(-) diff --git a/packages/@n8n/api-types/package.json b/packages/@n8n/api-types/package.json index c14e18992236e..299986a5c9cc7 100644 --- a/packages/@n8n/api-types/package.json +++ b/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" } } diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index dcc3dfdf4317b..14b86804780a2 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -272,8 +272,8 @@ importers: specifier: 'catalog:' version: 3.23.8 zod-class: - specifier: 0.0.15 - version: 0.0.15(zod@3.23.8) + specifier: 0.0.16 + version: 0.0.16(zod@3.23.8) devDependencies: '@n8n/config': specifier: workspace:* @@ -434,7 +434,7 @@ importers: version: 3.666.0(@aws-sdk/client-sts@3.666.0) '@getzep/zep-cloud': specifier: 1.0.12 - version: 1.0.12(@langchain/core@0.3.19(openai@4.73.1(encoding@0.1.13)(zod@3.23.8)))(encoding@0.1.13)(langchain@0.3.6(e4rnrwhosnp2xiru36mqgdy2bu)) + version: 1.0.12(@langchain/core@0.3.19(openai@4.73.1(encoding@0.1.13)(zod@3.23.8)))(encoding@0.1.13)(langchain@0.3.6(4axcxpjbcq5bce7ff6ajxrpp4i)) '@getzep/zep-js': specifier: 0.9.0 version: 0.9.0 @@ -461,7 +461,7 @@ importers: version: 0.3.1(@aws-sdk/client-sso-oidc@3.666.0(@aws-sdk/client-sts@3.666.0))(@langchain/core@0.3.19(openai@4.73.1(encoding@0.1.13)(zod@3.23.8)))(encoding@0.1.13) '@langchain/community': specifier: 0.3.15 - version: 0.3.15(vc5hvyy27o4cmm4jplsptc2fqm) + version: 0.3.15(v4qhcw25bevfr6xzz4fnsvjiqe) '@langchain/core': specifier: 'catalog:' version: 0.3.19(openai@4.73.1(encoding@0.1.13)(zod@3.23.8)) @@ -548,7 +548,7 @@ importers: version: 23.0.1 langchain: specifier: 0.3.6 - version: 0.3.6(e4rnrwhosnp2xiru36mqgdy2bu) + version: 0.3.6(4axcxpjbcq5bce7ff6ajxrpp4i) lodash: specifier: 'catalog:' version: 4.17.21 @@ -13393,8 +13393,8 @@ packages: resolution: {integrity: sha512-Z2Fe1bn+eLstG8DRR6FTavGD+MeAwyfmouhHsIUgaADz8jvFKbO/fXc2trJKZg+5EBjh4gGm3iU/t3onKlXHIg==} engines: {node: '>=10'} - zod-class@0.0.15: - resolution: {integrity: sha512-CD5B4e9unKPj1hiy7JOSwRV01WqbEBkFOlhws0C9s9wB0FSpECOnlKXOAkjo9tKYX2enQsXWyyOIBNPPNUHWRA==} + zod-class@0.0.16: + resolution: {integrity: sha512-3A1l81VEUOxvSTGoNPsU4fTUY9CKin/HSySnXT3bIc+TJTDGCPbzSPE8W1VvwXqyzHEIWK608eFZja2uew9Ivw==} peerDependencies: zod: ^3 @@ -15690,7 +15690,7 @@ snapshots: '@gar/promisify@1.1.3': optional: true - '@getzep/zep-cloud@1.0.12(@langchain/core@0.3.19(openai@4.73.1(encoding@0.1.13)(zod@3.23.8)))(encoding@0.1.13)(langchain@0.3.6(e4rnrwhosnp2xiru36mqgdy2bu))': + '@getzep/zep-cloud@1.0.12(@langchain/core@0.3.19(openai@4.73.1(encoding@0.1.13)(zod@3.23.8)))(encoding@0.1.13)(langchain@0.3.6(4axcxpjbcq5bce7ff6ajxrpp4i))': dependencies: form-data: 4.0.0 node-fetch: 2.7.0(encoding@0.1.13) @@ -15699,7 +15699,7 @@ snapshots: zod: 3.23.8 optionalDependencies: '@langchain/core': 0.3.19(openai@4.73.1(encoding@0.1.13)(zod@3.23.8)) - langchain: 0.3.6(e4rnrwhosnp2xiru36mqgdy2bu) + langchain: 0.3.6(4axcxpjbcq5bce7ff6ajxrpp4i) transitivePeerDependencies: - encoding @@ -16163,7 +16163,7 @@ snapshots: - aws-crt - encoding - '@langchain/community@0.3.15(vc5hvyy27o4cmm4jplsptc2fqm)': + '@langchain/community@0.3.15(v4qhcw25bevfr6xzz4fnsvjiqe)': dependencies: '@ibm-cloud/watsonx-ai': 1.1.2 '@langchain/core': 0.3.19(openai@4.73.1(encoding@0.1.13)(zod@3.23.8)) @@ -16173,7 +16173,7 @@ snapshots: flat: 5.0.2 ibm-cloud-sdk-core: 5.1.0 js-yaml: 4.1.0 - langchain: 0.3.6(e4rnrwhosnp2xiru36mqgdy2bu) + langchain: 0.3.6(4axcxpjbcq5bce7ff6ajxrpp4i) langsmith: 0.2.3(openai@4.73.1(encoding@0.1.13)(zod@3.23.8)) uuid: 10.0.0 zod: 3.23.8 @@ -16186,7 +16186,7 @@ snapshots: '@aws-sdk/client-s3': 3.666.0 '@aws-sdk/credential-provider-node': 3.666.0(@aws-sdk/client-sso-oidc@3.666.0(@aws-sdk/client-sts@3.666.0))(@aws-sdk/client-sts@3.666.0) '@azure/storage-blob': 12.18.0(encoding@0.1.13) - '@getzep/zep-cloud': 1.0.12(@langchain/core@0.3.19(openai@4.73.1(encoding@0.1.13)(zod@3.23.8)))(encoding@0.1.13)(langchain@0.3.6(e4rnrwhosnp2xiru36mqgdy2bu)) + '@getzep/zep-cloud': 1.0.12(@langchain/core@0.3.19(openai@4.73.1(encoding@0.1.13)(zod@3.23.8)))(encoding@0.1.13)(langchain@0.3.6(4axcxpjbcq5bce7ff6ajxrpp4i)) '@getzep/zep-js': 0.9.0 '@google-ai/generativelanguage': 2.6.0(encoding@0.1.13) '@google-cloud/storage': 7.12.1(encoding@0.1.13) @@ -19470,14 +19470,6 @@ snapshots: transitivePeerDependencies: - debug - axios@1.7.4(debug@4.3.7): - dependencies: - follow-redirects: 1.15.6(debug@4.3.7) - form-data: 4.0.0 - proxy-from-env: 1.1.0 - transitivePeerDependencies: - - debug - axios@1.7.7: dependencies: follow-redirects: 1.15.6(debug@4.3.6) @@ -22329,7 +22321,7 @@ snapshots: '@types/debug': 4.1.12 '@types/node': 18.16.16 '@types/tough-cookie': 4.0.2 - axios: 1.7.4(debug@4.3.7) + axios: 1.7.4 camelcase: 6.3.0 debug: 4.3.7 dotenv: 16.4.5 @@ -22339,7 +22331,7 @@ snapshots: isstream: 0.1.2 jsonwebtoken: 9.0.2 mime-types: 2.1.35 - retry-axios: 2.6.0(axios@1.7.4) + retry-axios: 2.6.0(axios@1.7.4(debug@4.3.7)) tough-cookie: 4.1.3 transitivePeerDependencies: - supports-color @@ -23340,7 +23332,7 @@ snapshots: kuler@2.0.0: {} - langchain@0.3.6(e4rnrwhosnp2xiru36mqgdy2bu): + langchain@0.3.6(4axcxpjbcq5bce7ff6ajxrpp4i): dependencies: '@langchain/core': 0.3.19(openai@4.73.1(encoding@0.1.13)(zod@3.23.8)) '@langchain/openai': 0.3.14(@langchain/core@0.3.19(openai@4.73.1(encoding@0.1.13)(zod@3.23.8)))(encoding@0.1.13) @@ -25710,7 +25702,7 @@ snapshots: ret@0.1.15: {} - retry-axios@2.6.0(axios@1.7.4): + retry-axios@2.6.0(axios@1.7.4(debug@4.3.7)): dependencies: axios: 1.7.4 @@ -27783,7 +27775,7 @@ snapshots: property-expr: 2.0.5 toposort: 2.0.2 - zod-class@0.0.15(zod@3.23.8): + zod-class@0.0.16(zod@3.23.8): dependencies: type-fest: 4.26.1 zod: 3.23.8 From c0aba3b123f101227a7b7f4dc9cdaae9043c117e Mon Sep 17 00:00:00 2001 From: Danny Martini Date: Mon, 30 Dec 2024 10:50:44 +0100 Subject: [PATCH 12/19] move booleanLiteral to right folder --- .../src/dto/credentials/credentials-get-many-request.dto.ts | 2 +- .../src/dto/credentials/credentials-get-one-request.dto.ts | 2 +- .../api-types/src/{dto/common.ts => schemas/booleanLiteral.ts} | 0 3 files changed, 2 insertions(+), 2 deletions(-) rename packages/@n8n/api-types/src/{dto/common.ts => schemas/booleanLiteral.ts} (100%) diff --git a/packages/@n8n/api-types/src/dto/credentials/credentials-get-many-request.dto.ts b/packages/@n8n/api-types/src/dto/credentials/credentials-get-many-request.dto.ts index 36cbddf0ba26a..f31bde5f35572 100644 --- a/packages/@n8n/api-types/src/dto/credentials/credentials-get-many-request.dto.ts +++ b/packages/@n8n/api-types/src/dto/credentials/credentials-get-many-request.dto.ts @@ -1,6 +1,6 @@ import { Z } from 'zod-class'; -import { booleanLiteral } from '../common'; +import { booleanLiteral } from '../../schemas/booleanLiteral'; export class CredentialsGetManyRequestQuery extends Z.class({ /** diff --git a/packages/@n8n/api-types/src/dto/credentials/credentials-get-one-request.dto.ts b/packages/@n8n/api-types/src/dto/credentials/credentials-get-one-request.dto.ts index d6994297d4cd5..ff4ed7fee4551 100644 --- a/packages/@n8n/api-types/src/dto/credentials/credentials-get-one-request.dto.ts +++ b/packages/@n8n/api-types/src/dto/credentials/credentials-get-one-request.dto.ts @@ -1,6 +1,6 @@ import { Z } from 'zod-class'; -import { booleanLiteral } from '../common'; +import { booleanLiteral } from '../../schemas/booleanLiteral'; export class CredentialsGetOneRequestQuery extends Z.class({ /** diff --git a/packages/@n8n/api-types/src/dto/common.ts b/packages/@n8n/api-types/src/schemas/booleanLiteral.ts similarity index 100% rename from packages/@n8n/api-types/src/dto/common.ts rename to packages/@n8n/api-types/src/schemas/booleanLiteral.ts From 21496024a57c157ccf980394e1bbb5df86f42317 Mon Sep 17 00:00:00 2001 From: Danny Martini Date: Mon, 30 Dec 2024 10:52:17 +0100 Subject: [PATCH 13/19] convert boolean literals to booleans --- packages/@n8n/api-types/src/schemas/booleanLiteral.ts | 2 +- packages/cli/src/credentials/credentials.controller.ts | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/@n8n/api-types/src/schemas/booleanLiteral.ts b/packages/@n8n/api-types/src/schemas/booleanLiteral.ts index 51679aa0110a0..dae910e46c0be 100644 --- a/packages/@n8n/api-types/src/schemas/booleanLiteral.ts +++ b/packages/@n8n/api-types/src/schemas/booleanLiteral.ts @@ -1,3 +1,3 @@ import { z } from 'zod'; -export const booleanLiteral = z.union([z.literal('true'), z.literal('false')]); +export const booleanLiteral = z.enum(['true', 'false']).transform((value) => value === 'true'); diff --git a/packages/cli/src/credentials/credentials.controller.ts b/packages/cli/src/credentials/credentials.controller.ts index ce4968104a8de..26ac330eafd43 100644 --- a/packages/cli/src/credentials/credentials.controller.ts +++ b/packages/cli/src/credentials/credentials.controller.ts @@ -58,8 +58,8 @@ export class CredentialsController { ) { const credentials = await this.credentialsService.getMany(req.user, { listQueryOptions: req.listQueryOptions, - includeScopes: query.includeScopes === 'true', - includeData: query.includeData === 'true', + includeScopes: query.includeScopes, + includeData: query.includeData, }); credentials.forEach((c) => { // @ts-expect-error: This is to emulate the old behavior of removing the shared @@ -102,9 +102,9 @@ export class CredentialsController { // 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. - query.includeData === 'true', + query.includeData ?? false, ) - : await this.credentialsService.getOne(req.user, credentialId, query.includeData === 'true'); + : await this.credentialsService.getOne(req.user, credentialId, query.includeData ?? false); const scopes = await this.credentialsService.getCredentialScopes( req.user, From ba14efba726e0a129e22199009760d41d800ad52 Mon Sep 17 00:00:00 2001 From: Danny Martini Date: Mon, 30 Dec 2024 10:52:23 +0100 Subject: [PATCH 14/19] add tests for DTOs --- .../credentials-get-many-request.dto.test.ts | 55 +++++++++++++++++++ .../credentials-get-one-request.dto.test.ts | 50 +++++++++++++++++ 2 files changed, 105 insertions(+) create mode 100644 packages/@n8n/api-types/src/dto/credentials/__tests__/credentials-get-many-request.dto.test.ts create mode 100644 packages/@n8n/api-types/src/dto/credentials/__tests__/credentials-get-one-request.dto.test.ts diff --git a/packages/@n8n/api-types/src/dto/credentials/__tests__/credentials-get-many-request.dto.test.ts b/packages/@n8n/api-types/src/dto/credentials/__tests__/credentials-get-many-request.dto.test.ts new file mode 100644 index 0000000000000..0fa074b97d9a8 --- /dev/null +++ b/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); + }); + }); +}); diff --git a/packages/@n8n/api-types/src/dto/credentials/__tests__/credentials-get-one-request.dto.test.ts b/packages/@n8n/api-types/src/dto/credentials/__tests__/credentials-get-one-request.dto.test.ts new file mode 100644 index 0000000000000..2e3060147495c --- /dev/null +++ b/packages/@n8n/api-types/src/dto/credentials/__tests__/credentials-get-one-request.dto.test.ts @@ -0,0 +1,50 @@ +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); + }); + + 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); + }); + }); +}); From 5b88f748e30ba4990389c986589164bc148e8eeb Mon Sep 17 00:00:00 2001 From: Danny Martini Date: Mon, 30 Dec 2024 13:12:39 +0100 Subject: [PATCH 15/19] rename schema --- .../src/dto/credentials/credentials-get-many-request.dto.ts | 6 +++--- .../src/dto/credentials/credentials-get-one-request.dto.ts | 4 ++-- packages/@n8n/api-types/src/schemas/booleanFromString.ts | 3 +++ packages/@n8n/api-types/src/schemas/booleanLiteral.ts | 3 --- 4 files changed, 8 insertions(+), 8 deletions(-) create mode 100644 packages/@n8n/api-types/src/schemas/booleanFromString.ts delete mode 100644 packages/@n8n/api-types/src/schemas/booleanLiteral.ts diff --git a/packages/@n8n/api-types/src/dto/credentials/credentials-get-many-request.dto.ts b/packages/@n8n/api-types/src/dto/credentials/credentials-get-many-request.dto.ts index f31bde5f35572..47332ca7f9c6a 100644 --- a/packages/@n8n/api-types/src/dto/credentials/credentials-get-many-request.dto.ts +++ b/packages/@n8n/api-types/src/dto/credentials/credentials-get-many-request.dto.ts @@ -1,6 +1,6 @@ import { Z } from 'zod-class'; -import { booleanLiteral } from '../../schemas/booleanLiteral'; +import { booleanFromString } from '../../schemas/booleanFromString'; export class CredentialsGetManyRequestQuery extends Z.class({ /** @@ -8,7 +8,7 @@ export class CredentialsGetManyRequestQuery extends Z.class({ * requesting user has in relation to the credential, e.g. * ['credential:read', 'credential:update'] */ - includeScopes: booleanLiteral.optional(), + includeScopes: booleanFromString.optional(), /** * Adds the decrypted `data` field to each credential. @@ -18,5 +18,5 @@ export class CredentialsGetManyRequestQuery extends Z.class({ * * This switches `includeScopes` to true to be able to check for the scopes */ - includeData: booleanLiteral.optional(), + includeData: booleanFromString.optional(), }) {} diff --git a/packages/@n8n/api-types/src/dto/credentials/credentials-get-one-request.dto.ts b/packages/@n8n/api-types/src/dto/credentials/credentials-get-one-request.dto.ts index ff4ed7fee4551..cc860a450eadb 100644 --- a/packages/@n8n/api-types/src/dto/credentials/credentials-get-one-request.dto.ts +++ b/packages/@n8n/api-types/src/dto/credentials/credentials-get-one-request.dto.ts @@ -1,6 +1,6 @@ import { Z } from 'zod-class'; -import { booleanLiteral } from '../../schemas/booleanLiteral'; +import { booleanFromString } from '../../schemas/booleanFromString'; export class CredentialsGetOneRequestQuery extends Z.class({ /** @@ -9,5 +9,5 @@ export class CredentialsGetOneRequestQuery extends Z.class({ * It only does this for credentials for which the user has the * `credential:update` scope. */ - includeData: booleanLiteral.optional(), + includeData: booleanFromString.optional(), }) {} diff --git a/packages/@n8n/api-types/src/schemas/booleanFromString.ts b/packages/@n8n/api-types/src/schemas/booleanFromString.ts new file mode 100644 index 0000000000000..bcc9e8133c853 --- /dev/null +++ b/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'); diff --git a/packages/@n8n/api-types/src/schemas/booleanLiteral.ts b/packages/@n8n/api-types/src/schemas/booleanLiteral.ts deleted file mode 100644 index dae910e46c0be..0000000000000 --- a/packages/@n8n/api-types/src/schemas/booleanLiteral.ts +++ /dev/null @@ -1,3 +0,0 @@ -import { z } from 'zod'; - -export const booleanLiteral = z.enum(['true', 'false']).transform((value) => value === 'true'); From 5fad0ca1897d800a6c5c765c4af2aa5e5dc13b26 Mon Sep 17 00:00:00 2001 From: Danny Martini Date: Mon, 30 Dec 2024 13:12:51 +0100 Subject: [PATCH 16/19] default to false --- .../__tests__/credentials-get-one-request.dto.test.ts | 2 ++ .../src/dto/credentials/credentials-get-one-request.dto.ts | 2 +- packages/cli/src/credentials/credentials.controller.ts | 4 ++-- 3 files changed, 5 insertions(+), 3 deletions(-) diff --git a/packages/@n8n/api-types/src/dto/credentials/__tests__/credentials-get-one-request.dto.test.ts b/packages/@n8n/api-types/src/dto/credentials/__tests__/credentials-get-one-request.dto.test.ts index 2e3060147495c..274b00b759e19 100644 --- a/packages/@n8n/api-types/src/dto/credentials/__tests__/credentials-get-one-request.dto.test.ts +++ b/packages/@n8n/api-types/src/dto/credentials/__tests__/credentials-get-one-request.dto.test.ts @@ -8,6 +8,8 @@ describe('CredentialsGetManyRequestQuery', () => { const result = CredentialsGetOneRequestQuery.safeParse(data); expect(result.success).toBe(true); + // defaults to false + expect(result.data?.includeData).toBe(false); }); test.each([ diff --git a/packages/@n8n/api-types/src/dto/credentials/credentials-get-one-request.dto.ts b/packages/@n8n/api-types/src/dto/credentials/credentials-get-one-request.dto.ts index cc860a450eadb..ad790014e8efe 100644 --- a/packages/@n8n/api-types/src/dto/credentials/credentials-get-one-request.dto.ts +++ b/packages/@n8n/api-types/src/dto/credentials/credentials-get-one-request.dto.ts @@ -9,5 +9,5 @@ export class CredentialsGetOneRequestQuery extends Z.class({ * It only does this for credentials for which the user has the * `credential:update` scope. */ - includeData: booleanFromString.optional(), + includeData: booleanFromString.optional().default('false'), }) {} diff --git a/packages/cli/src/credentials/credentials.controller.ts b/packages/cli/src/credentials/credentials.controller.ts index 26ac330eafd43..4cc0b500f2e0b 100644 --- a/packages/cli/src/credentials/credentials.controller.ts +++ b/packages/cli/src/credentials/credentials.controller.ts @@ -102,9 +102,9 @@ export class CredentialsController { // 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. - query.includeData ?? false, + query.includeData, ) - : await this.credentialsService.getOne(req.user, credentialId, query.includeData ?? false); + : await this.credentialsService.getOne(req.user, credentialId, query.includeData); const scopes = await this.credentialsService.getCredentialScopes( req.user, From dccbd33de0b3920b573e331a84994a50c2a0b43f Mon Sep 17 00:00:00 2001 From: Danny Martini Date: Mon, 30 Dec 2024 17:24:13 +0100 Subject: [PATCH 17/19] remove unnecessary case select is always defined, see condition above --- .../cli/src/databases/repositories/credentials.repository.ts | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/packages/cli/src/databases/repositories/credentials.repository.ts b/packages/cli/src/databases/repositories/credentials.repository.ts index dda47aed75566..5b88a4ed87e23 100644 --- a/packages/cli/src/databases/repositories/credentials.repository.ts +++ b/packages/cli/src/databases/repositories/credentials.repository.ts @@ -80,11 +80,8 @@ export class CredentialsRepository extends Repository { if (listQueryOptions.includeData) { if (Array.isArray(findManyOptions.select)) { findManyOptions.select.push('data'); - } else if (typeof findManyOptions.select === 'object') { - findManyOptions.select.data = true; } else { - findManyOptions.select ??= []; - findManyOptions.select.push('data'); + findManyOptions.select.data = true; } } From 8a1b5235a64b369580ea77031f962d0012ec14d6 Mon Sep 17 00:00:00 2001 From: Danny Martini Date: Mon, 30 Dec 2024 17:24:20 +0100 Subject: [PATCH 18/19] add tests to increase coverage --- .../__tests__/credentials.service.test.ts | 62 ++++++++++++++++++- .../__tests__/credentials.repository.test.ts | 50 +++++++++++++++ 2 files changed, 111 insertions(+), 1 deletion(-) create mode 100644 packages/cli/src/databases/repositories/__tests__/credentials.repository.test.ts diff --git a/packages/cli/src/credentials/__tests__/credentials.service.test.ts b/packages/cli/src/credentials/__tests__/credentials.service.test.ts index 8df06059830c3..5b9a1f021bfee 100644 --- a/packages/cli/src/credentials/__tests__/credentials.service.test.ts +++ b/packages/cli/src/credentials/__tests__/credentials.service.test.ts @@ -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'; @@ -30,6 +32,8 @@ describe('CredentialsService', () => { }, ], }); + + const cipher = Container.get(Cipher); const credentialTypes = mock(); const service = new CredentialsService( mock(), @@ -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); @@ -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({ + id: '123', + name: 'Test Credential', + type: 'oauth2', + data: cipher.encrypt(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({ + 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); + }); + }); }); diff --git a/packages/cli/src/databases/repositories/__tests__/credentials.repository.test.ts b/packages/cli/src/databases/repositories/__tests__/credentials.repository.test.ts new file mode 100644 index 0000000000000..439b806a9afd3 --- /dev/null +++ b/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({ 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'); + }); +}); From 79563021ae8700cdee38ebe6eab95448a81e0fd3 Mon Sep 17 00:00:00 2001 From: Danny Martini Date: Tue, 31 Dec 2024 11:23:03 +0100 Subject: [PATCH 19/19] mock more --- .../src/credentials/__tests__/credentials.service.test.ts | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/packages/cli/src/credentials/__tests__/credentials.service.test.ts b/packages/cli/src/credentials/__tests__/credentials.service.test.ts index 5b9a1f021bfee..526acfc17d777 100644 --- a/packages/cli/src/credentials/__tests__/credentials.service.test.ts +++ b/packages/cli/src/credentials/__tests__/credentials.service.test.ts @@ -1,8 +1,7 @@ import { mock } from 'jest-mock-extended'; import { nanoId, date } from 'minifaker'; -import { Cipher } from 'n8n-core'; +import { Credentials } 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'; @@ -33,7 +32,6 @@ describe('CredentialsService', () => { ], }); - const cipher = Container.get(Cipher); const credentialTypes = mock(); const service = new CredentialsService( mock(), @@ -156,8 +154,8 @@ describe('CredentialsService', () => { id: '123', name: 'Test Credential', type: 'oauth2', - data: cipher.encrypt(data), }); + jest.spyOn(Credentials.prototype, 'getData').mockReturnValueOnce(data); credentialTypes.getByName.calledWith(credential.type).mockReturnValueOnce(credType); // ACT @@ -186,8 +184,8 @@ describe('CredentialsService', () => { id: '123', name: 'Test Credential', type: 'oauth2', - data: cipher.encrypt(data), }); + jest.spyOn(Credentials.prototype, 'getData').mockReturnValueOnce(data); credentialTypes.getByName.calledWith(credential.type).mockReturnValueOnce(credType); // ACT