From fd78021b68a261291d76811a2a01d7336577bca7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Iv=C3=A1n=20Ovejero?= Date: Mon, 4 Sep 2023 15:00:25 +0200 Subject: [PATCH] feat(core): Add list query middleware to credentials (#7041) --- packages/cli/src/WorkflowHelpers.ts | 4 +- .../src/credentials/credentials.controller.ts | 8 +- .../src/credentials/credentials.service.ts | 69 +++- .../listQuery/dtos/credentials.filter.dto.ts | 19 ++ .../listQuery/dtos/credentials.select.dto.ts | 15 + .../cli/src/middlewares/listQuery/filter.ts | 3 + .../cli/src/middlewares/listQuery/select.ts | 3 + .../src/workflows/workflows.services.ee.ts | 7 +- .../credentials.controller.test.ts | 297 ++++++++++++++++++ 9 files changed, 400 insertions(+), 25 deletions(-) create mode 100644 packages/cli/src/middlewares/listQuery/dtos/credentials.filter.dto.ts create mode 100644 packages/cli/src/middlewares/listQuery/dtos/credentials.select.dto.ts create mode 100644 packages/cli/test/integration/credentials.controller.test.ts diff --git a/packages/cli/src/WorkflowHelpers.ts b/packages/cli/src/WorkflowHelpers.ts index b1bf820363df4..c5f06c0be8868 100644 --- a/packages/cli/src/WorkflowHelpers.ts +++ b/packages/cli/src/WorkflowHelpers.ts @@ -40,7 +40,7 @@ import type { RoleNames } from '@db/entities/Role'; import { RoleService } from './services/role.service'; import { ExecutionRepository, RoleRepository } from './databases/repositories'; import { VariablesService } from './environments/variables/variables.service'; -import type { Credentials } from './requests'; +import type { CredentialsEntity } from './databases/entities/CredentialsEntity'; const ERROR_TRIGGER_TYPE = config.getEnv('nodes.errorTriggerType'); @@ -539,7 +539,7 @@ export function getNodesWithInaccessibleCreds(workflow: WorkflowEntity, userCred export function validateWorkflowCredentialUsage( newWorkflowVersion: WorkflowEntity, previousWorkflowVersion: WorkflowEntity, - credentialsUserHasAccessTo: Credentials.WithOwnedByAndSharedWith[], + credentialsUserHasAccessTo: CredentialsEntity[], ) { /** * We only need to check nodes that use credentials the current user cannot access, diff --git a/packages/cli/src/credentials/credentials.controller.ts b/packages/cli/src/credentials/credentials.controller.ts index 006f103d84409..098872cbc0f98 100644 --- a/packages/cli/src/credentials/credentials.controller.ts +++ b/packages/cli/src/credentials/credentials.controller.ts @@ -10,9 +10,10 @@ import { EECredentialsController } from './credentials.controller.ee'; import { CredentialsService } from './credentials.service'; import type { ICredentialsDb } from '@/Interfaces'; -import type { CredentialRequest } from '@/requests'; +import type { CredentialRequest, ListQuery } from '@/requests'; import { Container } from 'typedi'; import { InternalHooks } from '@/InternalHooks'; +import { listQueryMiddleware } from '@/middlewares'; export const credentialsController = express.Router(); @@ -35,8 +36,9 @@ credentialsController.use('/', EECredentialsController); */ credentialsController.get( '/', - ResponseHelper.send(async (req: CredentialRequest.GetAll) => { - return CredentialsService.getMany(req.user); + listQueryMiddleware, + ResponseHelper.send(async (req: ListQuery.Request) => { + return CredentialsService.getMany(req.user, { listQueryOptions: req.listQueryOptions }); }), ); diff --git a/packages/cli/src/credentials/credentials.service.ts b/packages/cli/src/credentials/credentials.service.ts index d5cfc0c6ff8fa..c141952a68598 100644 --- a/packages/cli/src/credentials/credentials.service.ts +++ b/packages/cli/src/credentials/credentials.service.ts @@ -9,7 +9,7 @@ import type { import { CREDENTIAL_EMPTY_VALUE, deepCopy, LoggerProxy, NodeHelpers } from 'n8n-workflow'; import { Container } from 'typedi'; import type { FindManyOptions, FindOptionsWhere } from 'typeorm'; -import { In } from 'typeorm'; +import { In, Like } from 'typeorm'; import * as Db from '@/Db'; import * as ResponseHelper from '@/ResponseHelper'; @@ -21,7 +21,7 @@ import { SharedCredentials } from '@db/entities/SharedCredentials'; import { validateEntity } from '@/GenericHelpers'; import { ExternalHooks } from '@/ExternalHooks'; import type { User } from '@db/entities/User'; -import type { CredentialRequest } from '@/requests'; +import type { CredentialRequest, ListQuery } from '@/requests'; import { CredentialTypes } from '@/CredentialTypes'; import { RoleService } from '@/services/role.service'; import { OwnershipService } from '@/services/ownership.service'; @@ -37,33 +37,70 @@ export class CredentialsService { }); } - static async getMany(user: User, options?: { disableGlobalRole: boolean }) { - type Select = Array; + private static toFindManyOptions(listQueryOptions?: ListQuery.Options) { + const findManyOptions: FindManyOptions = {}; - const select: Select = ['id', 'name', 'type', 'nodesAccess', 'createdAt', 'updatedAt']; + type Select = Array; - const relations = ['shared', 'shared.role', 'shared.user']; + const defaultRelations = ['shared', 'shared.role', 'shared.user']; + const defaultSelect: Select = ['id', 'name', 'type', 'nodesAccess', 'createdAt', 'updatedAt']; - const returnAll = user.globalRole.name === 'owner' && options?.disableGlobalRole !== true; + if (!listQueryOptions) return { select: defaultSelect, relations: defaultRelations }; - const addOwnedByAndSharedWith = (c: CredentialsEntity) => - Container.get(OwnershipService).addOwnedByAndSharedWith(c); + const { filter, select, take, skip } = listQueryOptions; + + if (typeof filter?.name === 'string' && filter?.name !== '') { + filter.name = Like(`%${filter.name}%`); + } + + if (typeof filter?.type === 'string' && filter?.type !== '') { + filter.type = Like(`%${filter.type}%`); + } + + if (filter) findManyOptions.where = filter; + if (select) findManyOptions.select = select; + if (take) findManyOptions.take = take; + if (skip) findManyOptions.skip = skip; + + if (take && select && !select?.id) { + findManyOptions.select = { ...findManyOptions.select, id: true }; // pagination requires id + } + + if (!findManyOptions.select) { + findManyOptions.select = defaultSelect; + findManyOptions.relations = defaultRelations; + } + + return findManyOptions; + } + + private static addOwnedByAndSharedWith(credentials: CredentialsEntity[]) { + return credentials.map((c) => Container.get(OwnershipService).addOwnedByAndSharedWith(c)); + } + + static async getMany( + user: User, + options: { listQueryOptions?: ListQuery.Options; onlyOwn?: boolean } = {}, + ) { + const findManyOptions = this.toFindManyOptions(options.listQueryOptions); + + const returnAll = user.globalRole.name === 'owner' && !options.onlyOwn; + const isDefaultSelect = !options.listQueryOptions?.select; if (returnAll) { - const credentials = await Db.collections.Credentials.find({ select, relations }); + const credentials = await Db.collections.Credentials.find(findManyOptions); - return credentials.map(addOwnedByAndSharedWith); + return isDefaultSelect ? this.addOwnedByAndSharedWith(credentials) : credentials; } - const ids = await CredentialsService.getAccessibleCredentials(user.id); + const ids = await this.getAccessibleCredentials(user.id); const credentials = await Db.collections.Credentials.find({ - select, - relations, - where: { id: In(ids) }, + ...findManyOptions, + where: { ...findManyOptions.where, id: In(ids) }, // only accessible credentials }); - return credentials.map(addOwnedByAndSharedWith); + return isDefaultSelect ? this.addOwnedByAndSharedWith(credentials) : credentials; } /** diff --git a/packages/cli/src/middlewares/listQuery/dtos/credentials.filter.dto.ts b/packages/cli/src/middlewares/listQuery/dtos/credentials.filter.dto.ts new file mode 100644 index 0000000000000..de7d5cf23050c --- /dev/null +++ b/packages/cli/src/middlewares/listQuery/dtos/credentials.filter.dto.ts @@ -0,0 +1,19 @@ +import { IsOptional, IsString } from 'class-validator'; +import { Expose } from 'class-transformer'; +import { BaseFilter } from './base.filter.dto'; + +export class CredentialsFilter extends BaseFilter { + @IsString() + @IsOptional() + @Expose() + name?: string; + + @IsString() + @IsOptional() + @Expose() + type?: string; + + static async fromString(rawFilter: string) { + return this.toFilter(rawFilter, CredentialsFilter); + } +} diff --git a/packages/cli/src/middlewares/listQuery/dtos/credentials.select.dto.ts b/packages/cli/src/middlewares/listQuery/dtos/credentials.select.dto.ts new file mode 100644 index 0000000000000..0bcce83cf3e8a --- /dev/null +++ b/packages/cli/src/middlewares/listQuery/dtos/credentials.select.dto.ts @@ -0,0 +1,15 @@ +import { BaseSelect } from './base.select.dto'; + +export class CredentialsSelect extends BaseSelect { + static get selectableFields() { + return new Set([ + 'id', // always included downstream + 'name', + 'type', + ]); + } + + static fromString(rawFilter: string) { + return this.toSelect(rawFilter, CredentialsSelect); + } +} diff --git a/packages/cli/src/middlewares/listQuery/filter.ts b/packages/cli/src/middlewares/listQuery/filter.ts index cb189a5042cc5..bfdb35d76d2c2 100644 --- a/packages/cli/src/middlewares/listQuery/filter.ts +++ b/packages/cli/src/middlewares/listQuery/filter.ts @@ -2,6 +2,7 @@ import * as ResponseHelper from '@/ResponseHelper'; import { WorkflowFilter } from './dtos/workflow.filter.dto'; +import { CredentialsFilter } from './dtos/credentials.filter.dto'; import { UserFilter } from './dtos/user.filter.dto'; import { toError } from '@/utils'; @@ -21,6 +22,8 @@ export const filterListQueryMiddleware = async ( if (req.baseUrl.endsWith('workflows')) { Filter = WorkflowFilter; + } else if (req.baseUrl.endsWith('credentials')) { + Filter = CredentialsFilter; } else if (req.baseUrl.endsWith('users')) { Filter = UserFilter; } else { diff --git a/packages/cli/src/middlewares/listQuery/select.ts b/packages/cli/src/middlewares/listQuery/select.ts index 4ff4ac525af66..a96a3cd31c34f 100644 --- a/packages/cli/src/middlewares/listQuery/select.ts +++ b/packages/cli/src/middlewares/listQuery/select.ts @@ -2,6 +2,7 @@ import { WorkflowSelect } from './dtos/workflow.select.dto'; import { UserSelect } from './dtos/user.select.dto'; +import { CredentialsSelect } from './dtos/credentials.select.dto'; import * as ResponseHelper from '@/ResponseHelper'; import { toError } from '@/utils'; @@ -17,6 +18,8 @@ export const selectListQueryMiddleware: RequestHandler = (req: ListQuery.Request if (req.baseUrl.endsWith('workflows')) { Select = WorkflowSelect; + } else if (req.baseUrl.endsWith('credentials')) { + Select = CredentialsSelect; } else if (req.baseUrl.endsWith('users')) { Select = UserSelect; } else { diff --git a/packages/cli/src/workflows/workflows.services.ee.ts b/packages/cli/src/workflows/workflows.services.ee.ts index dd8c672938614..0fad82e56fa9c 100644 --- a/packages/cli/src/workflows/workflows.services.ee.ts +++ b/packages/cli/src/workflows/workflows.services.ee.ts @@ -16,6 +16,7 @@ import { CredentialsService } from '@/credentials/credentials.service'; import { NodeOperationError } from 'n8n-workflow'; import { RoleService } from '@/services/role.service'; import Container from 'typedi'; +import type { CredentialsEntity } from '@/databases/entities/CredentialsEntity'; import type { Credentials } from '@/requests'; export class EEWorkflowsService extends WorkflowsService { @@ -106,9 +107,7 @@ export class EEWorkflowsService extends WorkflowsService { currentUser: User, ): Promise { workflow.usedCredentials = []; - const userCredentials = await CredentialsService.getMany(currentUser, { - disableGlobalRole: true, - }); + const userCredentials = await CredentialsService.getMany(currentUser, { onlyOwn: true }); const credentialIdsUsedByWorkflow = new Set(); workflow.nodes.forEach((node) => { if (!node.credentials) { @@ -151,7 +150,7 @@ export class EEWorkflowsService extends WorkflowsService { static validateCredentialPermissionsToUser( workflow: WorkflowEntity, - allowedCredentials: Credentials.WithOwnedByAndSharedWith[], + allowedCredentials: CredentialsEntity[], ) { workflow.nodes.forEach((node) => { if (!node.credentials) { diff --git a/packages/cli/test/integration/credentials.controller.test.ts b/packages/cli/test/integration/credentials.controller.test.ts new file mode 100644 index 0000000000000..1b55c71d46c74 --- /dev/null +++ b/packages/cli/test/integration/credentials.controller.test.ts @@ -0,0 +1,297 @@ +import * as testDb from './shared/testDb'; +import * as utils from './shared/utils/'; +import { randomCredentialPayload as payload } from './shared/random'; + +import type { Credentials } from '@/requests'; +import type { User } from '@db/entities/User'; + +const { any } = expect; + +const testServer = utils.setupTestServer({ endpointGroups: ['credentials'] }); + +let owner: User; +let member: User; + +beforeEach(async () => { + await testDb.truncate(['SharedCredentials', 'Credentials']); + + owner = await testDb.createOwner(); + member = await testDb.createMember(); +}); + +type GetAllResponse = { body: { data: Credentials.WithOwnedByAndSharedWith[] } }; + +describe('GET /credentials', () => { + describe('should return', () => { + test('all credentials for owner', async () => { + const role = await testDb.getCredentialOwnerRole(); + + const { id: id1 } = await testDb.saveCredential(payload(), { user: owner, role }); + const { id: id2 } = await testDb.saveCredential(payload(), { user: member, role }); + + const response: GetAllResponse = await testServer + .authAgentFor(owner) + .get('/credentials') + .expect(200); + + expect(response.body.data).toHaveLength(2); + + response.body.data.forEach(validateCredential); + + const savedIds = [id1, id2]; + const returnedIds = response.body.data.map((c) => c.id); + + expect(savedIds).toEqual(returnedIds); + }); + + test('only own credentials for member', async () => { + const role = await testDb.getCredentialOwnerRole(); + + const firstMember = member; + const secondMember = await testDb.createMember(); + + const c1 = await testDb.saveCredential(payload(), { user: firstMember, role }); + const c2 = await testDb.saveCredential(payload(), { user: secondMember, role }); + + const response: GetAllResponse = await testServer + .authAgentFor(firstMember) + .get('/credentials') + .expect(200); + + expect(response.body.data).toHaveLength(1); + + const [firstMemberCred] = response.body.data; + + validateCredential(firstMemberCred); + expect(firstMemberCred.id).toBe(c1.id); + expect(firstMemberCred.id).not.toBe(c2.id); + }); + }); + + describe('filter', () => { + test('should filter credentials by field: name - full match', async () => { + const role = await testDb.getCredentialOwnerRole(); + const savedCred = await testDb.saveCredential(payload(), { user: owner, role }); + + const response: GetAllResponse = await testServer + .authAgentFor(owner) + .get('/credentials') + .query(`filter={ "name": "${savedCred.name}" }`) + .expect(200); + + expect(response.body.data).toHaveLength(1); + + const [returnedCred] = response.body.data; + + expect(returnedCred.name).toBe(savedCred.name); + + const _response = await testServer + .authAgentFor(owner) + .get('/credentials') + .query('filter={ "name": "Non-Existing Credential" }') + .expect(200); + + expect(_response.body.data).toHaveLength(0); + }); + + test('should filter credentials by field: name - partial match', async () => { + const role = await testDb.getCredentialOwnerRole(); + const savedCred = await testDb.saveCredential(payload(), { user: owner, role }); + + const partialName = savedCred.name.slice(3); + + const response: GetAllResponse = await testServer + .authAgentFor(owner) + .get('/credentials') + .query(`filter={ "name": "${partialName}" }`) + .expect(200); + + expect(response.body.data).toHaveLength(1); + + const [returnedCred] = response.body.data; + + expect(returnedCred.name).toBe(savedCred.name); + + const _response = await testServer + .authAgentFor(owner) + .get('/credentials') + .query('filter={ "name": "Non-Existing Credential" }') + .expect(200); + + expect(_response.body.data).toHaveLength(0); + }); + + test('should filter credentials by field: type - full match', async () => { + const role = await testDb.getCredentialOwnerRole(); + + const savedCred = await testDb.saveCredential(payload(), { user: owner, role }); + + const response: GetAllResponse = await testServer + .authAgentFor(owner) + .get('/credentials') + .query(`filter={ "type": "${savedCred.type}" }`) + .expect(200); + + expect(response.body.data).toHaveLength(1); + + const [returnedCred] = response.body.data; + + expect(returnedCred.type).toBe(savedCred.type); + + const _response = await testServer + .authAgentFor(owner) + .get('/credentials') + .query('filter={ "type": "Non-Existing Credential" }') + .expect(200); + + expect(_response.body.data).toHaveLength(0); + }); + + test('should filter credentials by field: type - partial match', async () => { + const role = await testDb.getCredentialOwnerRole(); + + const savedCred = await testDb.saveCredential(payload(), { user: owner, role }); + + const partialType = savedCred.type.slice(3); + + const response: GetAllResponse = await testServer + .authAgentFor(owner) + .get('/credentials') + .query(`filter={ "type": "${partialType}" }`) + .expect(200); + + expect(response.body.data).toHaveLength(1); + + const [returnedCred] = response.body.data; + + expect(returnedCred.type).toBe(savedCred.type); + + const _response = await testServer + .authAgentFor(owner) + .get('/credentials') + .query('filter={ "type": "Non-Existing Credential" }') + .expect(200); + + expect(_response.body.data).toHaveLength(0); + }); + }); + + describe('select', () => { + test('should select credential field: id', async () => { + const role = await testDb.getCredentialOwnerRole(); + + await testDb.saveCredential(payload(), { user: owner, role }); + await testDb.saveCredential(payload(), { user: owner, role }); + + const response: GetAllResponse = await testServer + .authAgentFor(owner) + .get('/credentials') + .query('select=["id"]') + .expect(200); + + expect(response.body).toEqual({ + data: [{ id: any(String) }, { id: any(String) }], + }); + }); + + test('should select credential field: name', async () => { + const role = await testDb.getCredentialOwnerRole(); + + await testDb.saveCredential(payload(), { user: owner, role }); + await testDb.saveCredential(payload(), { user: owner, role }); + + const response: GetAllResponse = await testServer + .authAgentFor(owner) + .get('/credentials') + .query('select=["name"]') + .expect(200); + + expect(response.body).toEqual({ + data: [{ name: any(String) }, { name: any(String) }], + }); + }); + + test('should select credential field: type', async () => { + const role = await testDb.getCredentialOwnerRole(); + + await testDb.saveCredential(payload(), { user: owner, role }); + await testDb.saveCredential(payload(), { user: owner, role }); + + const response: GetAllResponse = await testServer + .authAgentFor(owner) + .get('/credentials') + .query('select=["type"]') + .expect(200); + + expect(response.body).toEqual({ + data: [{ type: any(String) }, { type: any(String) }], + }); + }); + }); + + describe('take', () => { + test('should return n credentials or less, without skip', async () => { + const role = await testDb.getCredentialOwnerRole(); + + await testDb.saveCredential(payload(), { user: owner, role }); + await testDb.saveCredential(payload(), { user: owner, role }); + + const response = await testServer + .authAgentFor(owner) + .get('/credentials') + .query('take=2') + .expect(200); + + expect(response.body.data).toHaveLength(2); + + response.body.data.forEach(validateCredential); + + const _response = await testServer + .authAgentFor(owner) + .get('/credentials') + .query('take=1') + .expect(200); + + expect(_response.body.data).toHaveLength(1); + + _response.body.data.forEach(validateCredential); + }); + + test('should return n credentials or less, with skip', async () => { + const role = await testDb.getCredentialOwnerRole(); + + await testDb.saveCredential(payload(), { user: owner, role }); + await testDb.saveCredential(payload(), { user: owner, role }); + + const response = await testServer + .authAgentFor(owner) + .get('/credentials') + .query('take=1&skip=1') + .expect(200); + + expect(response.body.data).toHaveLength(1); + + response.body.data.forEach(validateCredential); + }); + }); +}); + +function validateCredential(credential: Credentials.WithOwnedByAndSharedWith) { + const { name, type, nodesAccess, sharedWith, ownedBy } = credential; + + expect(typeof name).toBe('string'); + expect(typeof type).toBe('string'); + expect(typeof nodesAccess[0].nodeType).toBe('string'); + expect('data' in credential).toBe(false); + + if (sharedWith) expect(Array.isArray(sharedWith)).toBe(true); + + if (ownedBy) { + const { id, email, firstName, lastName } = ownedBy; + + expect(typeof id).toBe('string'); + expect(typeof email).toBe('string'); + expect(typeof firstName).toBe('string'); + expect(typeof lastName).toBe('string'); + } +}