From b01dd2c5d026903407358578e72daf88dbcea402 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Iv=C3=A1n=20Ovejero?= Date: Tue, 30 Jan 2024 09:31:42 +0100 Subject: [PATCH 1/2] refactor(core): Modernize credentials controllers and services (no-changelog) --- packages/cli/src/Server.ts | 5 +- .../credentials/credentials.controller.ee.ts | 195 --------- .../src/credentials/credentials.controller.ts | 384 ++++++++++++------ .../src/credentials/credentials.service.ee.ts | 42 +- .../src/credentials/credentials.service.ts | 101 +++-- .../cli/src/workflows/workflow.service.ee.ts | 5 +- .../cli/src/workflows/workflows.controller.ts | 5 +- .../test/integration/credentials.ee.test.ts | 34 +- .../integration/shared/utils/testServer.ts | 4 +- 9 files changed, 332 insertions(+), 443 deletions(-) delete mode 100644 packages/cli/src/credentials/credentials.controller.ee.ts diff --git a/packages/cli/src/Server.ts b/packages/cli/src/Server.ts index 7f1b06b6b8614..82ed161622320 100644 --- a/packages/cli/src/Server.ts +++ b/packages/cli/src/Server.ts @@ -34,7 +34,7 @@ import { N8N_VERSION, TEMPLATES_DIR, } from '@/constants'; -import { credentialsController } from '@/credentials/credentials.controller'; +import { CredentialsController } from '@/credentials/credentials.controller'; import type { CurlHelper } from '@/requests'; import { registerController } from '@/decorators'; import { AuthController } from '@/controllers/auth.controller'; @@ -230,6 +230,7 @@ export class Server extends AbstractServer { ActiveWorkflowsController, WorkflowsController, ExecutionsController, + CredentialsController, ]; if ( @@ -348,8 +349,6 @@ export class Server extends AbstractServer { await this.registerControllers(ignoredEndpoints); - this.app.use(`/${this.restEndpoint}/credentials`, credentialsController); - // ---------------------------------------- // SAML // ---------------------------------------- diff --git a/packages/cli/src/credentials/credentials.controller.ee.ts b/packages/cli/src/credentials/credentials.controller.ee.ts deleted file mode 100644 index 09dc2a4e75052..0000000000000 --- a/packages/cli/src/credentials/credentials.controller.ee.ts +++ /dev/null @@ -1,195 +0,0 @@ -import express from 'express'; -import type { INodeCredentialTestResult } from 'n8n-workflow'; -import { deepCopy } from 'n8n-workflow'; -import * as Db from '@/Db'; -import * as ResponseHelper from '@/ResponseHelper'; - -import type { CredentialRequest } from '@/requests'; -import { License } from '@/License'; -import { EECredentialsService as EECredentials } from './credentials.service.ee'; -import { OwnershipService } from '@/services/ownership.service'; -import { Container } from 'typedi'; -import { InternalHooks } from '@/InternalHooks'; -import { BadRequestError } from '@/errors/response-errors/bad-request.error'; -import { NotFoundError } from '@/errors/response-errors/not-found.error'; -import { UnauthorizedError } from '@/errors/response-errors/unauthorized.error'; -import { CredentialsRepository } from '@/databases/repositories/credentials.repository'; -import * as utils from '@/utils'; -import { UserManagementMailer } from '@/UserManagement/email'; - -export const EECredentialsController = express.Router(); - -EECredentialsController.use((req, res, next) => { - if (!Container.get(License).isSharingEnabled()) { - // skip ee router and use free one - next('router'); - return; - } - // use ee router - next(); -}); - -/** - * GET /credentials/:id - */ -EECredentialsController.get( - '/:id(\\w+)', - (req, res, next) => (req.params.id === 'new' ? next('router') : next()), // skip ee router and use free one for naming - ResponseHelper.send(async (req: CredentialRequest.Get) => { - const { id: credentialId } = req.params; - const includeDecryptedData = req.query.includeData === 'true'; - - let credential = await Container.get(CredentialsRepository).findOne({ - where: { id: credentialId }, - relations: ['shared', 'shared.user'], - }); - - if (!credential) { - throw new NotFoundError( - 'Could not load the credential. If you think this is an error, ask the owner to share it with you again', - ); - } - - const userSharing = credential.shared?.find((shared) => shared.user.id === req.user.id); - - if (!userSharing && !req.user.hasGlobalScope('credential:read')) { - throw new UnauthorizedError('Forbidden.'); - } - - credential = Container.get(OwnershipService).addOwnedByAndSharedWith(credential); - - if (!includeDecryptedData || !userSharing || userSharing.role !== 'credential:owner') { - const { data: _, ...rest } = credential; - return { ...rest }; - } - - const { data: _, ...rest } = credential; - - const decryptedData = EECredentials.redact(EECredentials.decrypt(credential), credential); - - return { data: decryptedData, ...rest }; - }), -); - -/** - * POST /credentials/test - * - * Test if a credential is valid. - */ -EECredentialsController.post( - '/test', - ResponseHelper.send(async (req: CredentialRequest.Test): Promise => { - const { credentials } = req.body; - - const credentialId = credentials.id; - const { ownsCredential } = await EECredentials.isOwned(req.user, credentialId); - - const sharing = await EECredentials.getSharing(req.user, credentialId, { - allowGlobalScope: true, - globalScope: 'credential:read', - }); - if (!ownsCredential) { - if (!sharing) { - throw new UnauthorizedError('Forbidden'); - } - - const decryptedData = EECredentials.decrypt(sharing.credentials); - Object.assign(credentials, { data: decryptedData }); - } - - const mergedCredentials = deepCopy(credentials); - if (mergedCredentials.data && sharing?.credentials) { - const decryptedData = EECredentials.decrypt(sharing.credentials); - mergedCredentials.data = EECredentials.unredact(mergedCredentials.data, decryptedData); - } - - return await EECredentials.test(req.user, mergedCredentials); - }), -); - -/** - * (EE) PUT /credentials/:id/share - * - * Grant or remove users' access to a credential. - */ - -EECredentialsController.put( - '/:credentialId/share', - ResponseHelper.send(async (req: CredentialRequest.Share) => { - const { credentialId } = req.params; - const { shareWithIds } = req.body; - - if ( - !Array.isArray(shareWithIds) || - !shareWithIds.every((userId) => typeof userId === 'string') - ) { - throw new BadRequestError('Bad request'); - } - - const isOwnedRes = await EECredentials.isOwned(req.user, credentialId); - const { ownsCredential } = isOwnedRes; - let { credential } = isOwnedRes; - if (!ownsCredential || !credential) { - credential = undefined; - // Allow owners/admins to share - if (req.user.hasGlobalScope('credential:share')) { - const sharedRes = await EECredentials.getSharing(req.user, credentialId, { - allowGlobalScope: true, - globalScope: 'credential:share', - }); - credential = sharedRes?.credentials; - } - if (!credential) { - throw new UnauthorizedError('Forbidden'); - } - } - - const ownerIds = ( - await EECredentials.getSharings(Db.getConnection().createEntityManager(), credentialId, [ - 'shared', - ]) - ) - .filter((e) => e.role === 'credential:owner') - .map((e) => e.userId); - - let amountRemoved: number | null = null; - let newShareeIds: string[] = []; - await Db.transaction(async (trx) => { - // remove all sharings that are not supposed to exist anymore - const { affected } = await Container.get(CredentialsRepository).pruneSharings( - trx, - credentialId, - [...ownerIds, ...shareWithIds], - ); - if (affected) amountRemoved = affected; - - const sharings = await EECredentials.getSharings(trx, credentialId); - - // extract the new sharings that need to be added - newShareeIds = utils.rightDiff( - [sharings, (sharing) => sharing.userId], - [shareWithIds, (shareeId) => shareeId], - ); - - if (newShareeIds.length) { - await EECredentials.share(trx, credential!, newShareeIds); - } - }); - - void Container.get(InternalHooks).onUserSharedCredentials({ - user: req.user, - credential_name: credential.name, - credential_type: credential.type, - credential_id: credential.id, - user_id_sharer: req.user.id, - user_ids_sharees_added: newShareeIds, - sharees_removed: amountRemoved, - }); - - await Container.get(UserManagementMailer).notifyCredentialsShared({ - sharer: req.user, - newShareeIds, - credentialsName: credential.name, - }); - }), -); diff --git a/packages/cli/src/credentials/credentials.controller.ts b/packages/cli/src/credentials/credentials.controller.ts index d7406d496f265..4e0b9e0729b5f 100644 --- a/packages/cli/src/credentials/credentials.controller.ts +++ b/packages/cli/src/credentials/credentials.controller.ts @@ -1,62 +1,100 @@ -import express from 'express'; -import type { INodeCredentialTestResult } from 'n8n-workflow'; import { deepCopy } from 'n8n-workflow'; - -import * as ResponseHelper from '@/ResponseHelper'; import config from '@/config'; -import { EECredentialsController } from './credentials.controller.ee'; import { CredentialsService } from './credentials.service'; - -import type { ICredentialsDb } from '@/Interfaces'; -import type { CredentialRequest, ListQuery } from '@/requests'; -import { Container } from 'typedi'; +import { CredentialRequest, ListQuery } from '@/requests'; import { InternalHooks } from '@/InternalHooks'; -import { listQueryMiddleware } from '@/middlewares'; import { Logger } from '@/Logger'; import { NotFoundError } from '@/errors/response-errors/not-found.error'; import { UnauthorizedError } from '@/errors/response-errors/unauthorized.error'; import { NamingService } from '@/services/naming.service'; +import { License } from '@/License'; +import { CredentialsRepository } from '@/databases/repositories/credentials.repository'; +import { OwnershipService } from '@/services/ownership.service'; +import { EnterpriseCredentialsService } from './credentials.service.ee'; +import { Authorized, Delete, Get, Licensed, Patch, Post, Put, RestController } from '@/decorators'; +import { BadRequestError } from '@/errors/response-errors/bad-request.error'; +import { UserManagementMailer } from '@/UserManagement/email'; +import * as Db from '@/Db'; +import * as utils from '@/utils'; +import { listQueryMiddleware } from '@/middlewares'; -export const credentialsController = express.Router(); -credentialsController.use('/', EECredentialsController); - -/** - * GET /credentials - */ -credentialsController.get( - '/', - listQueryMiddleware, - ResponseHelper.send(async (req: ListQuery.Request) => { - return await CredentialsService.getMany(req.user, { listQueryOptions: req.listQueryOptions }); - }), -); - -/** - * GET /credentials/new - * - * Generate a unique credential name. - */ -credentialsController.get( - '/new', - ResponseHelper.send(async (req: CredentialRequest.NewName) => { +@Authorized() +@RestController('/credentials') +export class CredentialsController { + constructor( + private readonly credentialsService: CredentialsService, + private readonly enterpriseCredentialsService: EnterpriseCredentialsService, + private readonly credentialsRepository: CredentialsRepository, + private readonly namingService: NamingService, + private readonly license: License, + private readonly logger: Logger, + private readonly ownershipService: OwnershipService, + private readonly internalHooks: InternalHooks, + private readonly userManagementMailer: UserManagementMailer, + ) {} + + @Get('/', { middlewares: listQueryMiddleware }) + async getMany(req: ListQuery.Request) { + return await this.credentialsService.getMany(req.user, { + listQueryOptions: req.listQueryOptions, + }); + } + + @Get('/new') + async generateUniqueName(req: CredentialRequest.NewName) { const requestedName = req.query.name ?? config.getEnv('credentials.defaultName'); return { - name: await Container.get(NamingService).getUniqueCredentialName(requestedName), + name: await this.namingService.getUniqueCredentialName(requestedName), }; - }), -); - -/** - * GET /credentials/:id - */ -credentialsController.get( - '/:id(\\w+)', - ResponseHelper.send(async (req: CredentialRequest.Get) => { + } + + @Get('/:id') + async getOne(req: CredentialRequest.Get) { + if (this.license.isSharingEnabled()) { + const { id: credentialId } = req.params; + const includeDecryptedData = req.query.includeData === 'true'; + + let credential = await this.credentialsRepository.findOne({ + where: { id: credentialId }, + relations: ['shared', 'shared.user'], + }); + + if (!credential) { + throw new NotFoundError( + 'Could not load the credential. If you think this is an error, ask the owner to share it with you again', + ); + } + + const userSharing = credential.shared?.find((shared) => shared.user.id === req.user.id); + + if (!userSharing && !req.user.hasGlobalScope('credential:read')) { + throw new UnauthorizedError('Forbidden.'); + } + + credential = this.ownershipService.addOwnedByAndSharedWith(credential); + + if (!includeDecryptedData || !userSharing || userSharing.role !== 'credential:owner') { + const { data: _, ...rest } = credential; + return { ...rest }; + } + + const { data: _, ...rest } = credential; + + const decryptedData = this.credentialsService.redact( + this.credentialsService.decrypt(credential), + credential, + ); + + return { data: decryptedData, ...rest }; + } + + // non-enterprise + const { id: credentialId } = req.params; const includeDecryptedData = req.query.includeData === 'true'; - const sharing = await CredentialsService.getSharing( + const sharing = await this.credentialsService.getSharing( req.user, credentialId, { allowGlobalScope: true, globalScope: 'credential:read' }, @@ -75,52 +113,79 @@ credentialsController.get( return { ...rest }; } - const decryptedData = CredentialsService.redact( - CredentialsService.decrypt(credential), + const decryptedData = this.credentialsService.redact( + this.credentialsService.decrypt(credential), credential, ); return { data: decryptedData, ...rest }; - }), -); - -/** - * POST /credentials/test - * - * Test if a credential is valid. - */ -credentialsController.post( - '/test', - ResponseHelper.send(async (req: CredentialRequest.Test): Promise => { + } + + @Post('/test') + async testCredentials(req: CredentialRequest.Test) { + if (this.license.isSharingEnabled()) { + const { credentials } = req.body; + + const credentialId = credentials.id; + const { ownsCredential } = await this.enterpriseCredentialsService.isOwned( + req.user, + credentialId, + ); + + const sharing = await this.enterpriseCredentialsService.getSharing(req.user, credentialId, { + allowGlobalScope: true, + globalScope: 'credential:read', + }); + if (!ownsCredential) { + if (!sharing) { + throw new UnauthorizedError('Forbidden'); + } + + const decryptedData = this.credentialsService.decrypt(sharing.credentials); + Object.assign(credentials, { data: decryptedData }); + } + + const mergedCredentials = deepCopy(credentials); + if (mergedCredentials.data && sharing?.credentials) { + const decryptedData = this.credentialsService.decrypt(sharing.credentials); + mergedCredentials.data = this.credentialsService.unredact( + mergedCredentials.data, + decryptedData, + ); + } + + return await this.credentialsService.test(req.user, mergedCredentials); + } + + // non-enterprise + const { credentials } = req.body; - const sharing = await CredentialsService.getSharing(req.user, credentials.id, { + const sharing = await this.credentialsService.getSharing(req.user, credentials.id, { allowGlobalScope: true, globalScope: 'credential:read', }); const mergedCredentials = deepCopy(credentials); if (mergedCredentials.data && sharing?.credentials) { - const decryptedData = CredentialsService.decrypt(sharing.credentials); - mergedCredentials.data = CredentialsService.unredact(mergedCredentials.data, decryptedData); + const decryptedData = this.credentialsService.decrypt(sharing.credentials); + mergedCredentials.data = this.credentialsService.unredact( + mergedCredentials.data, + decryptedData, + ); } - return await CredentialsService.test(req.user, mergedCredentials); - }), -); + return await this.credentialsService.test(req.user, mergedCredentials); + } -/** - * POST /credentials - */ -credentialsController.post( - '/', - ResponseHelper.send(async (req: CredentialRequest.Create) => { - const newCredential = await CredentialsService.prepareCreateData(req.body); + @Post('/') + async createCredentials(req: CredentialRequest.Create) { + const newCredential = await this.credentialsService.prepareCreateData(req.body); - const encryptedData = CredentialsService.createEncryptedData(null, newCredential); - const credential = await CredentialsService.save(newCredential, encryptedData, req.user); + const encryptedData = this.credentialsService.createEncryptedData(null, newCredential); + const credential = await this.credentialsService.save(newCredential, encryptedData, req.user); - void Container.get(InternalHooks).onUserCreatedCredentials({ + void this.internalHooks.onUserCreatedCredentials({ user: req.user, credential_name: newCredential.name, credential_type: credential.type, @@ -129,18 +194,13 @@ credentialsController.post( }); return credential; - }), -); - -/** - * PATCH /credentials/:id - */ -credentialsController.patch( - '/:id(\\w+)', - ResponseHelper.send(async (req: CredentialRequest.Update): Promise => { + } + + @Patch('/:id') + async updateCredentials(req: CredentialRequest.Update) { const { id: credentialId } = req.params; - const sharing = await CredentialsService.getSharing( + const sharing = await this.credentialsService.getSharing( req.user, credentialId, { @@ -151,42 +211,36 @@ credentialsController.patch( ); if (!sharing) { - Container.get(Logger).info( - 'Attempt to update credential blocked due to lack of permissions', - { - credentialId, - userId: req.user.id, - }, - ); + this.logger.info('Attempt to update credential blocked due to lack of permissions', { + credentialId, + userId: req.user.id, + }); throw new NotFoundError( 'Credential to be updated not found. You can only update credentials owned by you', ); } if (sharing.role !== 'credential:owner' && !req.user.hasGlobalScope('credential:update')) { - Container.get(Logger).info( - 'Attempt to update credential blocked due to lack of permissions', - { - credentialId, - userId: req.user.id, - }, - ); + this.logger.info('Attempt to update credential blocked due to lack of permissions', { + credentialId, + userId: req.user.id, + }); throw new UnauthorizedError('You can only update credentials owned by you'); } const { credentials: credential } = sharing; - const decryptedData = CredentialsService.decrypt(credential); - const preparedCredentialData = await CredentialsService.prepareUpdateData( + const decryptedData = this.credentialsService.decrypt(credential); + const preparedCredentialData = await this.credentialsService.prepareUpdateData( req.body, decryptedData, ); - const newCredentialData = CredentialsService.createEncryptedData( + const newCredentialData = this.credentialsService.createEncryptedData( credentialId, preparedCredentialData, ); - const responseData = await CredentialsService.update(credentialId, newCredentialData); + const responseData = await this.credentialsService.update(credentialId, newCredentialData); if (responseData === null) { throw new NotFoundError(`Credential ID "${credentialId}" could not be found to be updated.`); @@ -195,21 +249,16 @@ credentialsController.patch( // Remove the encrypted data as it is not needed in the frontend const { data: _, ...rest } = responseData; - Container.get(Logger).verbose('Credential updated', { credentialId }); + this.logger.verbose('Credential updated', { credentialId }); return { ...rest }; - }), -); - -/** - * DELETE /credentials/:id - */ -credentialsController.delete( - '/:id(\\w+)', - ResponseHelper.send(async (req: CredentialRequest.Delete) => { + } + + @Delete('/:id') + async deleteCredentials(req: CredentialRequest.Delete) { const { id: credentialId } = req.params; - const sharing = await CredentialsService.getSharing( + const sharing = await this.credentialsService.getSharing( req.user, credentialId, { @@ -220,33 +269,112 @@ credentialsController.delete( ); if (!sharing) { - Container.get(Logger).info( - 'Attempt to delete credential blocked due to lack of permissions', - { - credentialId, - userId: req.user.id, - }, - ); + this.logger.info('Attempt to delete credential blocked due to lack of permissions', { + credentialId, + userId: req.user.id, + }); throw new NotFoundError( 'Credential to be deleted not found. You can only removed credentials owned by you', ); } if (sharing.role !== 'credential:owner' && !req.user.hasGlobalScope('credential:delete')) { - Container.get(Logger).info( - 'Attempt to delete credential blocked due to lack of permissions', - { - credentialId, - userId: req.user.id, - }, - ); + this.logger.info('Attempt to delete credential blocked due to lack of permissions', { + credentialId, + userId: req.user.id, + }); throw new UnauthorizedError('You can only remove credentials owned by you'); } const { credentials: credential } = sharing; - await CredentialsService.delete(credential); + await this.credentialsService.delete(credential); return true; - }), -); + } + + @Licensed('feat:sharing') + @Put('/:id/share') + async shareCredentials(req: CredentialRequest.Share) { + const { credentialId } = req.params; + const { shareWithIds } = req.body; + + if ( + !Array.isArray(shareWithIds) || + !shareWithIds.every((userId) => typeof userId === 'string') + ) { + throw new BadRequestError('Bad request'); + } + + const isOwnedRes = await this.enterpriseCredentialsService.isOwned(req.user, credentialId); + const { ownsCredential } = isOwnedRes; + let { credential } = isOwnedRes; + if (!ownsCredential || !credential) { + credential = undefined; + // Allow owners/admins to share + if (req.user.hasGlobalScope('credential:share')) { + const sharedRes = await this.enterpriseCredentialsService.getSharing( + req.user, + credentialId, + { + allowGlobalScope: true, + globalScope: 'credential:share', + }, + ); + credential = sharedRes?.credentials; + } + if (!credential) { + throw new UnauthorizedError('Forbidden'); + } + } + + const ownerIds = ( + await this.enterpriseCredentialsService.getSharings( + Db.getConnection().createEntityManager(), + credentialId, + ['shared'], + ) + ) + .filter((e) => e.role === 'credential:owner') + .map((e) => e.userId); + + let amountRemoved: number | null = null; + let newShareeIds: string[] = []; + await Db.transaction(async (trx) => { + // remove all sharings that are not supposed to exist anymore + const { affected } = await this.credentialsRepository.pruneSharings(trx, credentialId, [ + ...ownerIds, + ...shareWithIds, + ]); + if (affected) amountRemoved = affected; + + const sharings = await this.enterpriseCredentialsService.getSharings(trx, credentialId); + + // extract the new sharings that need to be added + newShareeIds = utils.rightDiff( + [sharings, (sharing) => sharing.userId], + [shareWithIds, (shareeId) => shareeId], + ); + + if (newShareeIds.length) { + await this.enterpriseCredentialsService.share(trx, credential!, newShareeIds); + } + }); + + void this.internalHooks.onUserSharedCredentials({ + user: req.user, + credential_name: credential.name, + credential_type: credential.type, + credential_id: credential.id, + user_id_sharer: req.user.id, + user_ids_sharees_added: newShareeIds, + sharees_removed: amountRemoved, + }); + + await this.userManagementMailer.notifyCredentialsShared({ + sharer: req.user, + newShareeIds, + credentialsName: credential.name, + }); + } +} diff --git a/packages/cli/src/credentials/credentials.service.ee.ts b/packages/cli/src/credentials/credentials.service.ee.ts index 9b31d3c4ebf8b..1d56bb65c4695 100644 --- a/packages/cli/src/credentials/credentials.service.ee.ts +++ b/packages/cli/src/credentials/credentials.service.ee.ts @@ -1,17 +1,20 @@ -import { Container } from 'typedi'; import type { EntityManager, FindOptionsWhere } from 'typeorm'; -import { CredentialsEntity } from '@db/entities/CredentialsEntity'; import type { SharedCredentials } from '@db/entities/SharedCredentials'; import type { User } from '@db/entities/User'; -import { CredentialsService, type CredentialsGetSharedOptions } from './credentials.service'; +import { type CredentialsGetSharedOptions } from './credentials.service'; import { SharedCredentialsRepository } from '@db/repositories/sharedCredentials.repository'; import { UserRepository } from '@/databases/repositories/user.repository'; +import { CredentialsEntity } from '@/databases/entities/CredentialsEntity'; +import { Service } from 'typedi'; -export class EECredentialsService extends CredentialsService { - static async isOwned( - user: User, - credentialId: string, - ): Promise<{ ownsCredential: boolean; credential?: CredentialsEntity }> { +@Service() +export class EnterpriseCredentialsService { + constructor( + private readonly userRepository: UserRepository, + private readonly sharedCredentialsRepository: SharedCredentialsRepository, + ) {} + + async isOwned(user: User, credentialId: string) { const sharing = await this.getSharing(user, credentialId, { allowGlobalScope: false }, [ 'credentials', ]); @@ -26,12 +29,12 @@ export class EECredentialsService extends CredentialsService { /** * Retrieve the sharing that matches a user and a credential. */ - static async getSharing( + async getSharing( user: User, credentialId: string, options: CredentialsGetSharedOptions, relations: string[] = ['credentials'], - ): Promise { + ) { const where: FindOptionsWhere = { credentialsId: credentialId }; // Omit user from where if the requesting user has relevant @@ -41,35 +44,28 @@ export class EECredentialsService extends CredentialsService { where.userId = user.id; } - return await Container.get(SharedCredentialsRepository).findOne({ + return await this.sharedCredentialsRepository.findOne({ where, relations, }); } - static async getSharings( - transaction: EntityManager, - credentialId: string, - relations = ['shared'], - ): Promise { + async getSharings(transaction: EntityManager, credentialId: string, relations = ['shared']) { const credential = await transaction.findOne(CredentialsEntity, { where: { id: credentialId }, relations, }); + return credential?.shared ?? []; } - static async share( - transaction: EntityManager, - credential: CredentialsEntity, - shareWithIds: string[], - ): Promise { - const users = await Container.get(UserRepository).getByIds(transaction, shareWithIds); + async share(transaction: EntityManager, credential: CredentialsEntity, shareWithIds: string[]) { + const users = await this.userRepository.getByIds(transaction, shareWithIds); const newSharedCredentials = users .filter((user) => !user.isPending) .map((user) => - Container.get(SharedCredentialsRepository).create({ + this.sharedCredentialsRepository.create({ credentialsId: credential.id, userId: user.id, role: 'credential:user', diff --git a/packages/cli/src/credentials/credentials.service.ts b/packages/cli/src/credentials/credentials.service.ts index 9533b5a1ad6f7..9141ce6e29b11 100644 --- a/packages/cli/src/credentials/credentials.service.ts +++ b/packages/cli/src/credentials/credentials.service.ts @@ -3,15 +3,11 @@ import type { ICredentialDataDecryptedObject, ICredentialsDecrypted, ICredentialType, - INodeCredentialTestResult, INodeProperties, } from 'n8n-workflow'; import { CREDENTIAL_EMPTY_VALUE, deepCopy, NodeHelpers } from 'n8n-workflow'; -import { Container } from 'typedi'; import type { FindOptionsWhere } from 'typeorm'; - import type { Scope } from '@n8n/permissions'; - import * as Db from '@/Db'; import type { ICredentialsDb } from '@/Interfaces'; import { CredentialsHelper, createCredentialsFromCredentialsEntity } from '@/CredentialsHelper'; @@ -27,20 +23,32 @@ import { OwnershipService } from '@/services/ownership.service'; import { Logger } from '@/Logger'; import { CredentialsRepository } from '@db/repositories/credentials.repository'; import { SharedCredentialsRepository } from '@db/repositories/sharedCredentials.repository'; +import { Service } from 'typedi'; export type CredentialsGetSharedOptions = | { allowGlobalScope: true; globalScope: Scope } | { allowGlobalScope: false }; +@Service() export class CredentialsService { - static async get(where: FindOptionsWhere, options?: { relations: string[] }) { - return await Container.get(CredentialsRepository).findOne({ + constructor( + private readonly credentialsRepository: CredentialsRepository, + private readonly sharedCredentialsRepository: SharedCredentialsRepository, + private readonly ownershipService: OwnershipService, + private readonly logger: Logger, + private readonly credenntialsHelper: CredentialsHelper, + private readonly externalHooks: ExternalHooks, + private readonly credentialTypes: CredentialTypes, + ) {} + + async get(where: FindOptionsWhere, options?: { relations: string[] }) { + return await this.credentialsRepository.findOne({ relations: options?.relations, where, }); } - static async getMany( + async getMany( user: User, options: { listQueryOptions?: ListQuery.Options; onlyOwn?: boolean } = {}, ) { @@ -48,31 +56,29 @@ export class CredentialsService { const isDefaultSelect = !options.listQueryOptions?.select; if (returnAll) { - const credentials = await Container.get(CredentialsRepository).findMany( - options.listQueryOptions, - ); + const credentials = await this.credentialsRepository.findMany(options.listQueryOptions); return isDefaultSelect - ? credentials.map((c) => Container.get(OwnershipService).addOwnedByAndSharedWith(c)) + ? credentials.map((c) => this.ownershipService.addOwnedByAndSharedWith(c)) : credentials; } - const ids = await Container.get(SharedCredentialsRepository).getAccessibleCredentials(user.id); + const ids = await this.sharedCredentialsRepository.getAccessibleCredentials(user.id); - const credentials = await Container.get(CredentialsRepository).findMany( + const credentials = await this.credentialsRepository.findMany( options.listQueryOptions, ids, // only accessible credentials ); return isDefaultSelect - ? credentials.map((c) => Container.get(OwnershipService).addOwnedByAndSharedWith(c)) + ? credentials.map((c) => this.ownershipService.addOwnedByAndSharedWith(c)) : credentials; } /** * Retrieve the sharing that matches a user and a credential. */ - static async getSharing( + async getSharing( user: User, credentialId: string, options: CredentialsGetSharedOptions, @@ -88,17 +94,17 @@ export class CredentialsService { where.role = 'credential:owner'; } - return await Container.get(SharedCredentialsRepository).findOne({ where, relations }); + return await this.sharedCredentialsRepository.findOne({ where, relations }); } - static async prepareCreateData( + async prepareCreateData( data: CredentialRequest.CredentialProperties, ): Promise { const { id, ...rest } = data; // This saves us a merge but requires some type casting. These // types are compatible for this case. - const newCredentials = Container.get(CredentialsRepository).create(rest as ICredentialsDb); + const newCredentials = this.credentialsRepository.create(rest as ICredentialsDb); await validateEntity(newCredentials); @@ -110,7 +116,7 @@ export class CredentialsService { return newCredentials; } - static async prepareUpdateData( + async prepareUpdateData( data: CredentialRequest.CredentialProperties, decryptedData: ICredentialDataDecryptedObject, ): Promise { @@ -121,7 +127,7 @@ export class CredentialsService { // This saves us a merge but requires some type casting. These // types are compatible for this case. - const updateData = Container.get(CredentialsRepository).create(mergedData as ICredentialsDb); + const updateData = this.credentialsRepository.create(mergedData as ICredentialsDb); await validateEntity(updateData); @@ -141,7 +147,7 @@ export class CredentialsService { return updateData; } - static createEncryptedData(credentialId: string | null, data: CredentialsEntity): ICredentialsDb { + createEncryptedData(credentialId: string | null, data: CredentialsEntity): ICredentialsDb { const credentials = new Credentials( { id: credentialId, name: data.name }, data.type, @@ -158,35 +164,28 @@ export class CredentialsService { return newCredentialData; } - static decrypt(credential: CredentialsEntity): ICredentialDataDecryptedObject { + decrypt(credential: CredentialsEntity) { const coreCredential = createCredentialsFromCredentialsEntity(credential); return coreCredential.getData(); } - static async update( - credentialId: string, - newCredentialData: ICredentialsDb, - ): Promise { - await Container.get(ExternalHooks).run('credentials.update', [newCredentialData]); + async update(credentialId: string, newCredentialData: ICredentialsDb) { + await this.externalHooks.run('credentials.update', [newCredentialData]); // Update the credentials in DB - await Container.get(CredentialsRepository).update(credentialId, newCredentialData); + await this.credentialsRepository.update(credentialId, newCredentialData); // We sadly get nothing back from "update". Neither if it updated a record // nor the new value. So query now the updated entry. - return await Container.get(CredentialsRepository).findOneBy({ id: credentialId }); + return await this.credentialsRepository.findOneBy({ id: credentialId }); } - static async save( - credential: CredentialsEntity, - encryptedData: ICredentialsDb, - user: User, - ): Promise { + async save(credential: CredentialsEntity, encryptedData: ICredentialsDb, user: User) { // To avoid side effects const newCredential = new CredentialsEntity(); Object.assign(newCredential, credential, encryptedData); - await Container.get(ExternalHooks).run('credentials.create', [encryptedData]); + await this.externalHooks.run('credentials.create', [encryptedData]); const result = await Db.transaction(async (transactionManager) => { const savedCredential = await transactionManager.save(newCredential); @@ -205,39 +204,31 @@ export class CredentialsService { return savedCredential; }); - Container.get(Logger).verbose('New credential created', { + this.logger.verbose('New credential created', { credentialId: newCredential.id, ownerId: user.id, }); return result; } - static async delete(credentials: CredentialsEntity): Promise { - await Container.get(ExternalHooks).run('credentials.delete', [credentials.id]); + async delete(credentials: CredentialsEntity) { + await this.externalHooks.run('credentials.delete', [credentials.id]); - await Container.get(CredentialsRepository).remove(credentials); + await this.credentialsRepository.remove(credentials); } - static async test( - user: User, - credentials: ICredentialsDecrypted, - ): Promise { - const helper = Container.get(CredentialsHelper); - return await helper.testCredentials(user, credentials.type, credentials); + async test(user: User, credentials: ICredentialsDecrypted) { + return await this.credenntialsHelper.testCredentials(user, credentials.type, credentials); } // Take data and replace all sensitive values with a sentinel value. // This will replace password fields and oauth data. - static redact( - data: ICredentialDataDecryptedObject, - credential: CredentialsEntity, - ): ICredentialDataDecryptedObject { + redact(data: ICredentialDataDecryptedObject, credential: CredentialsEntity) { const copiedData = deepCopy(data); - const credTypes = Container.get(CredentialTypes); let credType: ICredentialType; try { - credType = credTypes.getByName(credential.type); + credType = this.credentialTypes.getByName(credential.type); } catch { // This _should_ only happen when testing. If it does happen in // production it means it's either a mangled credential or a @@ -249,7 +240,7 @@ export class CredentialsService { const getExtendedProps = (type: ICredentialType) => { const props: INodeProperties[] = []; for (const e of type.extends ?? []) { - const extendsType = credTypes.getByName(e); + const extendsType = this.credentialTypes.getByName(e); const extendedProps = getExtendedProps(extendsType); NodeHelpers.mergeNodeProperties(props, extendedProps); } @@ -287,7 +278,7 @@ export class CredentialsService { return copiedData; } - private static unredactRestoreValues(unmerged: any, replacement: any) { + private unredactRestoreValues(unmerged: any, replacement: any) { // eslint-disable-next-line @typescript-eslint/no-unsafe-argument for (const [key, value] of Object.entries(unmerged)) { if (value === CREDENTIAL_BLANKING_VALUE || value === CREDENTIAL_EMPTY_VALUE) { @@ -310,10 +301,10 @@ export class CredentialsService { // Take unredacted data (probably from the DB) and merge it with // redacted data to create an unredacted version. - static unredact( + unredact( redactedData: ICredentialDataDecryptedObject, savedData: ICredentialDataDecryptedObject, - ): ICredentialDataDecryptedObject { + ) { // Replace any blank sentinel values with their saved version const mergedData = deepCopy(redactedData); this.unredactRestoreValues(mergedData, savedData); diff --git a/packages/cli/src/workflows/workflow.service.ee.ts b/packages/cli/src/workflows/workflow.service.ee.ts index 5c96808451fed..a95536d80f86b 100644 --- a/packages/cli/src/workflows/workflow.service.ee.ts +++ b/packages/cli/src/workflows/workflow.service.ee.ts @@ -24,6 +24,7 @@ export class EnterpriseWorkflowService { private readonly sharedWorkflowRepository: SharedWorkflowRepository, private readonly workflowRepository: WorkflowRepository, private readonly credentialsRepository: CredentialsRepository, + private readonly credentialsService: CredentialsService, ) {} async isOwned( @@ -70,7 +71,7 @@ export class EnterpriseWorkflowService { currentUser: User, ): Promise { workflow.usedCredentials = []; - const userCredentials = await CredentialsService.getMany(currentUser, { onlyOwn: true }); + const userCredentials = await this.credentialsService.getMany(currentUser, { onlyOwn: true }); const credentialIdsUsedByWorkflow = new Set(); workflow.nodes.forEach((node) => { if (!node.credentials) { @@ -139,7 +140,7 @@ export class EnterpriseWorkflowService { throw new NotFoundError('Workflow not found'); } - const allCredentials = await CredentialsService.getMany(user); + const allCredentials = await this.credentialsService.getMany(user); try { return this.validateWorkflowCredentialUsage(workflow, previousVersion, allCredentials); diff --git a/packages/cli/src/workflows/workflows.controller.ts b/packages/cli/src/workflows/workflows.controller.ts index 799263ad5e009..807b892cc3a27 100644 --- a/packages/cli/src/workflows/workflows.controller.ts +++ b/packages/cli/src/workflows/workflows.controller.ts @@ -1,4 +1,3 @@ -import { Service } from 'typedi'; import express from 'express'; import { v4 as uuid } from 'uuid'; import axios from 'axios'; @@ -40,7 +39,6 @@ import { WorkflowExecutionService } from './workflowExecution.service'; import { WorkflowSharingService } from './workflowSharing.service'; import { UserManagementMailer } from '@/UserManagement/email'; -@Service() @Authorized() @RestController('/workflows') export class WorkflowsController { @@ -62,6 +60,7 @@ export class WorkflowsController { private readonly userRepository: UserRepository, private readonly license: License, private readonly mailer: UserManagementMailer, + private readonly credentialsService: CredentialsService, ) {} @Post('/') @@ -92,7 +91,7 @@ export class WorkflowsController { // This is a new workflow, so we simply check if the user has access to // all used workflows - const allCredentials = await CredentialsService.getMany(req.user); + const allCredentials = await this.credentialsService.getMany(req.user); try { this.enterpriseWorkflowService.validateCredentialPermissionsToUser( diff --git a/packages/cli/test/integration/credentials.ee.test.ts b/packages/cli/test/integration/credentials.ee.test.ts index be9fc74be4e9c..4f68792d192f8 100644 --- a/packages/cli/test/integration/credentials.ee.test.ts +++ b/packages/cli/test/integration/credentials.ee.test.ts @@ -49,38 +49,6 @@ afterEach(() => { jest.clearAllMocks(); }); -// ---------------------------------------- -// dynamic router switching -// ---------------------------------------- -describe('router should switch based on flag', () => { - let savedCredentialId: string; - - beforeEach(async () => { - const savedCredential = await saveCredential(randomCredentialPayload(), { user: owner }); - savedCredentialId = savedCredential.id; - }); - - test('when sharing is disabled', async () => { - sharingSpy.mockReturnValueOnce(false); - - await authOwnerAgent - .put(`/credentials/${savedCredentialId}/share`) - .send({ shareWithIds: [member.id] }) - .expect(404); - - await authOwnerAgent.get(`/credentials/${savedCredentialId}`).send().expect(200); - }); - - test('when sharing is enabled', async () => { - await authOwnerAgent - .put(`/credentials/${savedCredentialId}/share`) - .send({ shareWithIds: [member.id] }) - .expect(200); - - await authOwnerAgent.get(`/credentials/${savedCredentialId}`).send().expect(200); - }); -}); - // ---------------------------------------- // GET /credentials - fetch all credentials // ---------------------------------------- @@ -330,6 +298,8 @@ describe('GET /credentials/:id', () => { // idempotent share/unshare // ---------------------------------------- describe('PUT /credentials/:id/share', () => { + testServer.license.setDefaults({ features: ['feat:sharing'] }); + test('should share the credential with the provided userIds and unshare it for missing ones', async () => { const savedCredential = await saveCredential(randomCredentialPayload(), { user: owner }); diff --git a/packages/cli/test/integration/shared/utils/testServer.ts b/packages/cli/test/integration/shared/utils/testServer.ts index dfa22f173a30e..d791d7dc895e7 100644 --- a/packages/cli/test/integration/shared/utils/testServer.ts +++ b/packages/cli/test/integration/shared/utils/testServer.ts @@ -123,8 +123,8 @@ export const setupTestServer = ({ for (const group of endpointGroups) { switch (group) { case 'credentials': - const { credentialsController } = await import('@/credentials/credentials.controller'); - app.use(`/${REST_PATH_SEGMENT}/credentials`, credentialsController); + const { CredentialsController } = await import('@/credentials/credentials.controller'); + registerController(app, CredentialsController); break; case 'workflows': From aa5f1b6d622490b2ebc6118b433c398bc7842d9a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E0=A4=95=E0=A4=BE=E0=A4=B0=E0=A4=A4=E0=A5=8B=E0=A4=AB?= =?UTF-8?q?=E0=A5=8D=E0=A4=AB=E0=A5=87=E0=A4=B2=E0=A4=B8=E0=A5=8D=E0=A4=95?= =?UTF-8?q?=E0=A5=8D=E0=A4=B0=E0=A4=BF=E0=A4=AA=E0=A5=8D=E0=A4=9F=E2=84=A2?= Date: Tue, 30 Jan 2024 18:40:27 +0100 Subject: [PATCH 2/2] fix the sharing endpoint --- packages/cli/src/credentials/credentials.controller.ts | 2 +- packages/cli/src/requests.ts | 2 +- packages/cli/test/integration/credentials.ee.test.ts | 10 +++++----- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/packages/cli/src/credentials/credentials.controller.ts b/packages/cli/src/credentials/credentials.controller.ts index 4e0b9e0729b5f..9d26c37f08d05 100644 --- a/packages/cli/src/credentials/credentials.controller.ts +++ b/packages/cli/src/credentials/credentials.controller.ts @@ -296,7 +296,7 @@ export class CredentialsController { @Licensed('feat:sharing') @Put('/:id/share') async shareCredentials(req: CredentialRequest.Share) { - const { credentialId } = req.params; + const { id: credentialId } = req.params; const { shareWithIds } = req.body; if ( diff --git a/packages/cli/src/requests.ts b/packages/cli/src/requests.ts index 27c2ebb1109c6..cdcd1bffe1c59 100644 --- a/packages/cli/src/requests.ts +++ b/packages/cli/src/requests.ts @@ -161,7 +161,7 @@ export declare namespace CredentialRequest { type Test = AuthenticatedRequest<{}, {}, INodeCredentialTestRequest>; - type Share = AuthenticatedRequest<{ credentialId: string }, {}, { shareWithIds: string[] }>; + type Share = AuthenticatedRequest<{ id: string }, {}, { shareWithIds: string[] }>; } // ---------------------------------- diff --git a/packages/cli/test/integration/credentials.ee.test.ts b/packages/cli/test/integration/credentials.ee.test.ts index 4f68792d192f8..2e09bc905c1b2 100644 --- a/packages/cli/test/integration/credentials.ee.test.ts +++ b/packages/cli/test/integration/credentials.ee.test.ts @@ -6,7 +6,6 @@ import type { IUser } from 'n8n-workflow'; import type { ListQuery } from '@/requests'; import type { User } from '@db/entities/User'; import { SharedCredentialsRepository } from '@db/repositories/sharedCredentials.repository'; -import { License } from '@/License'; import { randomCredentialPayload } from './shared/random'; import * as testDb from './shared/testDb'; @@ -19,8 +18,10 @@ import { UserManagementMailer } from '@/UserManagement/email'; import { mockInstance } from '../shared/mocking'; import config from '@/config'; -const sharingSpy = jest.spyOn(License.prototype, 'isSharingEnabled').mockReturnValue(true); -const testServer = utils.setupTestServer({ endpointGroups: ['credentials'] }); +const testServer = utils.setupTestServer({ + endpointGroups: ['credentials'], + enabledFeatures: ['feat:sharing'], +}); let owner: User; let member: User; @@ -298,8 +299,6 @@ describe('GET /credentials/:id', () => { // idempotent share/unshare // ---------------------------------------- describe('PUT /credentials/:id/share', () => { - testServer.license.setDefaults({ features: ['feat:sharing'] }); - test('should share the credential with the provided userIds and unshare it for missing ones', async () => { const savedCredential = await saveCredential(randomCredentialPayload(), { user: owner }); @@ -491,6 +490,7 @@ describe('PUT /credentials/:id/share', () => { responses.forEach((response) => expect(response.statusCode).toBe(400)); expect(mailer.notifyCredentialsShared).toHaveBeenCalledTimes(0); }); + test('should unshare the credential', async () => { const savedCredential = await saveCredential(randomCredentialPayload(), { user: owner });