From 7d8426f05536aaa7232d685decd7d4dc6cf73077 Mon Sep 17 00:00:00 2001 From: Albert Alises Date: Sat, 25 Jul 2026 16:22:22 +0200 Subject: [PATCH] feat: Add admin-managed instance credentials (#34364) --- .../dto/credentials/create-credential.dto.ts | 1 + .../src/schemas/instance-ai.schema.ts | 2 + .../__tests__/db-connection.test.ts | 2 +- .../db/src/entities/credentials-entity.ts | 5 + packages/@n8n/db/src/entities/index.ts | 6 +- .../instance-credential-assignment.ts | 26 + packages/@n8n/db/src/entities/types-db.ts | 3 +- .../__tests__/credentials.repository.test.ts | 106 ++- ...e-credential-assignment.repository.test.ts | 118 ++++ .../__tests__/settings.repository.test.ts | 30 + .../repositories/credentials.repository.ts | 128 +++- packages/@n8n/db/src/repositories/index.ts | 1 + ...stance-credential-assignment.repository.ts | 118 ++++ .../src/repositories/settings.repository.ts | 20 +- .../__tests__/db-lock.service.test.ts | 25 +- .../@n8n/db/src/services/db-lock.service.ts | 20 +- .../decorators/src/pubsub/pubsub-metadata.ts | 1 + .../scope-information.test.ts.snap | 1 + packages/@n8n/permissions/src/constants.ee.ts | 1 + .../src/roles/scopes/global-scopes.ee.ts | 1 + .../@n8n/permissions/src/scope-information.ts | 5 + .../src/__tests__/credentials-helper.test.ts | 24 + .../cli/src/commands/import/credentials.ts | 179 ++++- .../cli/src/commands/user-management/reset.ts | 8 +- packages/cli/src/credentials-helper.ts | 6 + .../__tests__/credentials.controller.test.ts | 88 +++ .../__tests__/credentials.service.ee.test.ts | 20 + .../__tests__/credentials.service.test.ts | 628 +++++++++++++++++- .../instance-credential-broker.test.ts | 156 +++++ .../credentials/credentials-finder.service.ts | 42 +- .../src/credentials/credentials.controller.ts | 34 +- .../src/credentials/credentials.service.ee.ts | 33 +- .../src/credentials/credentials.service.ts | 395 ++++++++++- .../credentials/instance-credential-broker.ts | 108 +++ .../instance-credential-use.registry.ts | 46 ++ .../src/credentials/instance-credentials.md | 63 ++ .../__tests__/credentials.repository.test.ts | 18 + .../credentials-permission-checker.test.ts | 58 +- .../credentials-permission-checker.ts | 33 +- .../instance-ai-settings.service.test.ts | 180 ++++- .../__tests__/instance-ai.controller.test.ts | 44 +- .../instance-ai-settings.service.ts | 232 +++++-- .../instance-ai/instance-ai.controller.ts | 67 +- .../modules/instance-ai/instance-ai.module.ts | 11 +- .../instance-ai/instance-ai.service.ts | 2 +- .../mcp/__tests__/webhook-utils.test.ts | 1 + .../source-control-import.service.ee.test.ts | 176 ++++- .../source-control-scoped.service.test.ts | 15 + .../source-control-export.service.ee.ts | 9 +- .../source-control-import.service.ee.ts | 214 +++--- .../source-control-scoped.service.ts | 3 +- .../types/exportable-credential.ts | 2 + .../workflow-dependency-query.service.ts | 2 +- .../workflow-review-request.service.test.ts | 4 +- ...flow-review-request.update.service.test.ts | 4 +- .../src/oauth/__tests__/oauth.service.test.ts | 17 + packages/cli/src/oauth/oauth.service.ts | 19 +- .../__tests__/check-access.test.ts | 41 ++ .../cli/src/permissions.ee/check-access.ts | 26 + packages/cli/src/public-api/types.ts | 8 +- .../__tests__/credentials.service.test.ts | 63 +- .../credentials/credentials.handler.ts | 1 + .../credentials/credentials.service.ts | 22 +- .../src/scaling/pubsub/pubsub.event-map.ts | 1 + .../cli/src/scaling/pubsub/pubsub.types.ts | 2 + .../credentials-risk-reporter.ts | 5 +- .../credentials-finder.service.test.ts | 136 +++- .../services/__tests__/import.service.test.ts | 13 + packages/cli/src/services/import.service.ts | 4 +- .../workflows/workflow-validation.service.ts | 2 +- .../cli/src/workflows/workflow.service.ee.ts | 1 + .../cli/test/integration/ai/ai.api.test.ts | 1 + .../commands/credentials.cmd.test.ts | 164 ++++- .../credentials/credentials.api.test.ts | 6 + .../instance-credentials.api.test.ts | 194 ++++++ .../database/services/db-lock.service.test.ts | 85 ++- .../frontend/@n8n/i18n/src/locales/en.json | 43 +- packages/frontend/editor-ui/src/Interface.ts | 1 + .../editor-ui/src/app/stores/ui.store.ts | 2 + .../__tests__/SettingsInstanceAiView.test.ts | 25 +- .../src/features/ai/instanceAi/constants.ts | 1 + .../ai/instanceAi/instanceAi.settings.api.ts | 6 + .../ai/instanceAi/instanceAiSettings.store.ts | 22 +- .../ai/instanceAi/module.descriptor.ts | 21 + .../SettingsInstanceAiCredentialsView.vue | 214 ++++++ .../views/SettingsInstanceAiView.vue | 562 ++++++++-------- .../CredentialEdit/CredentialEdit.vue | 31 +- .../components/CredentialsSelectModal.vue | 7 + .../useNodeCredentialOptions.test.ts | 22 +- .../composables/useNodeCredentialOptions.ts | 2 + .../features/credentials/credentials.store.ts | 2 + .../features/credentials/credentials.types.ts | 1 + .../credentials/views/CredentialsView.test.ts | 5 +- .../credentials/views/CredentialsView.vue | 3 +- packages/workflow/src/interfaces.ts | 1 + 95 files changed, 4640 insertions(+), 667 deletions(-) create mode 100644 packages/@n8n/db/src/entities/instance-credential-assignment.ts create mode 100644 packages/@n8n/db/src/repositories/__tests__/instance-credential-assignment.repository.test.ts create mode 100644 packages/@n8n/db/src/repositories/__tests__/settings.repository.test.ts create mode 100644 packages/@n8n/db/src/repositories/instance-credential-assignment.repository.ts create mode 100644 packages/cli/src/credentials/__tests__/instance-credential-broker.test.ts create mode 100644 packages/cli/src/credentials/instance-credential-broker.ts create mode 100644 packages/cli/src/credentials/instance-credential-use.registry.ts create mode 100644 packages/cli/src/credentials/instance-credentials.md create mode 100644 packages/cli/test/integration/credentials/instance-credentials.api.test.ts create mode 100644 packages/frontend/editor-ui/src/features/ai/instanceAi/views/SettingsInstanceAiCredentialsView.vue diff --git a/packages/@n8n/api-types/src/dto/credentials/create-credential.dto.ts b/packages/@n8n/api-types/src/dto/credentials/create-credential.dto.ts index 8c35452c058..5e7c9e25ddb 100644 --- a/packages/@n8n/api-types/src/dto/credentials/create-credential.dto.ts +++ b/packages/@n8n/api-types/src/dto/credentials/create-credential.dto.ts @@ -10,4 +10,5 @@ export class CreateCredentialDto extends Z.class({ uiContext: z.string().optional(), isGlobal: z.boolean().optional(), isResolvable: z.boolean().optional(), + usageScope: z.enum(['project', 'instance']).optional(), }) {} diff --git a/packages/@n8n/api-types/src/schemas/instance-ai.schema.ts b/packages/@n8n/api-types/src/schemas/instance-ai.schema.ts index 37a2c63019d..26c19bb4553 100644 --- a/packages/@n8n/api-types/src/schemas/instance-ai.schema.ts +++ b/packages/@n8n/api-types/src/schemas/instance-ai.schema.ts @@ -1423,6 +1423,7 @@ export interface InstanceAiAdminSettingsResponse { daytonaCredentialId: string | null; n8nSandboxCredentialId: string | null; searchCredentialId: string | null; + modelCredentialId: string | null; localGatewayDisabled: boolean; browserUseEnabled: boolean; } @@ -1439,6 +1440,7 @@ export class InstanceAiAdminSettingsUpdateRequest extends Z.class({ daytonaCredentialId: z.string().nullable().optional(), n8nSandboxCredentialId: z.string().nullable().optional(), searchCredentialId: z.string().nullable().optional(), + modelCredentialId: z.string().nullable().optional(), localGatewayDisabled: z.boolean().optional(), browserUseEnabled: z.boolean().optional(), }) {} diff --git a/packages/@n8n/db/src/connection/__tests__/db-connection.test.ts b/packages/@n8n/db/src/connection/__tests__/db-connection.test.ts index bdea6066ed4..59731fcf7f0 100644 --- a/packages/@n8n/db/src/connection/__tests__/db-connection.test.ts +++ b/packages/@n8n/db/src/connection/__tests__/db-connection.test.ts @@ -171,7 +171,7 @@ describe('DbConnection', () => { beforeEach(() => { Container.set(DbLockService, dbLockService); - dbLockService.withLock.mockImplementation(async (_lockId, fn) => await fn(tx)); + dbLockService.withLock.mockImplementation(async (_lockId, fn) => await fn(tx, {})); tx.query.mockResolvedValue([]); vi.mocked(MigrationExecutor).mockImplementation(function () { return migrationExecutor; diff --git a/packages/@n8n/db/src/entities/credentials-entity.ts b/packages/@n8n/db/src/entities/credentials-entity.ts index ccf3575cfea..aef06ec688f 100644 --- a/packages/@n8n/db/src/entities/credentials-entity.ts +++ b/packages/@n8n/db/src/entities/credentials-entity.ts @@ -5,6 +5,8 @@ import { WithTimestampsAndStringId } from './abstract-entity'; import type { SharedCredentials } from './shared-credentials'; import type { ICredentialsDb } from './types-db'; +export type CredentialUsageScope = 'project' | 'instance'; + @Entity() export class CredentialsEntity extends WithTimestampsAndStringId implements ICredentialsDb { @Column({ length: 128 }) @@ -61,6 +63,9 @@ export class CredentialsEntity extends WithTimestampsAndStringId implements ICre @Column({ type: 'varchar', nullable: true }) resolverId: string | null; + @Column({ type: 'varchar', length: 16, default: 'project' }) + usageScope: CredentialUsageScope; + toJSON() { const { shared, ...rest } = this; return rest; diff --git a/packages/@n8n/db/src/entities/index.ts b/packages/@n8n/db/src/entities/index.ts index eda21f1914e..2e6cc5b16d1 100644 --- a/packages/@n8n/db/src/entities/index.ts +++ b/packages/@n8n/db/src/entities/index.ts @@ -13,7 +13,7 @@ import { CredentialDependency, type CredentialDependencyType, } from './credential-dependency-entity'; -import { CredentialsEntity } from './credentials-entity'; +import { CredentialsEntity, type CredentialUsageScope } from './credentials-entity'; import { DeploymentKey } from './deployment-key'; import { EvaluationCollection } from './evaluation-collection.ee'; import { EvaluationConfig } from './evaluation-config.ee'; @@ -24,6 +24,7 @@ import type { ExecutionDataStorageLocation } from './execution-entity'; import { ExecutionMetadata } from './execution-metadata'; import { Folder } from './folder'; import { FolderTagMapping } from './folder-tag-mapping'; +import { InstanceCredentialAssignment } from './instance-credential-assignment'; import { InvalidAuthToken } from './invalid-auth-token'; import { ProcessedData } from './processed-data'; import { Project } from './project'; @@ -89,6 +90,7 @@ export { AgentEvalRating, type AgentEvalVote, InvalidAuthToken, + InstanceCredentialAssignment, AiBuilderTemporaryWorkflow, ProcessedData, Settings, @@ -101,6 +103,7 @@ export { WebhookEntity, AuthIdentity, CredentialsEntity, + type CredentialUsageScope, CredentialDependency, type CredentialDependencyType, DeploymentKey, @@ -166,6 +169,7 @@ export const entities = { AgentEvalResult, AgentEvalRating, InvalidAuthToken, + InstanceCredentialAssignment, AiBuilderTemporaryWorkflow, ProcessedData, Settings, diff --git a/packages/@n8n/db/src/entities/instance-credential-assignment.ts b/packages/@n8n/db/src/entities/instance-credential-assignment.ts new file mode 100644 index 00000000000..1e93b48a515 --- /dev/null +++ b/packages/@n8n/db/src/entities/instance-credential-assignment.ts @@ -0,0 +1,26 @@ +import { + Column, + Entity, + Index, + JoinColumn, + ManyToOne, + PrimaryColumn, + Relation, +} from '@n8n/typeorm'; + +import { WithTimestamps } from './abstract-entity'; +import type { CredentialsEntity } from './credentials-entity'; + +@Entity({ name: 'instance_credential_assignment' }) +export class InstanceCredentialAssignment extends WithTimestamps { + @PrimaryColumn({ type: 'varchar', length: 128 }) + credentialUseId: string; + + @Column({ type: 'varchar', length: 36 }) + @Index() + credentialId: string; + + @ManyToOne('CredentialsEntity', { onDelete: 'RESTRICT' }) + @JoinColumn({ name: 'credentialId' }) + credential: Relation; +} diff --git a/packages/@n8n/db/src/entities/types-db.ts b/packages/@n8n/db/src/entities/types-db.ts index b12c5797157..2908d3cf91f 100644 --- a/packages/@n8n/db/src/entities/types-db.ts +++ b/packages/@n8n/db/src/entities/types-db.ts @@ -18,7 +18,7 @@ import type { } from 'n8n-workflow'; import { z } from 'zod'; -import type { CredentialsEntity } from './credentials-entity'; +import type { CredentialUsageScope, CredentialsEntity } from './credentials-entity'; import type { ExecutionDataStorageLocation } from './execution-entity'; import type { Folder } from './folder'; import type { Project } from './project'; @@ -103,6 +103,7 @@ export interface ICredentialsDb extends ICredentialsBase, ICredentialsEncrypted isGlobal?: boolean; isResolvable?: boolean; isManaged?: boolean; + usageScope?: CredentialUsageScope; } export interface IExecutionResponse extends IExecutionBase { diff --git a/packages/@n8n/db/src/repositories/__tests__/credentials.repository.test.ts b/packages/@n8n/db/src/repositories/__tests__/credentials.repository.test.ts index 223dd53f1b5..83ebd2cde91 100644 --- a/packages/@n8n/db/src/repositories/__tests__/credentials.repository.test.ts +++ b/packages/@n8n/db/src/repositories/__tests__/credentials.repository.test.ts @@ -1,20 +1,122 @@ import { Container } from '@n8n/di'; -import type { SelectQueryBuilder } from '@n8n/typeorm'; -import { In } from '@n8n/typeorm'; +import type { EntityManager, SelectQueryBuilder } from '@n8n/typeorm'; +import { In, Not, QueryFailedError } from '@n8n/typeorm'; import { mock } from 'vitest-mock-extended'; import { CredentialsEntity } from '../../entities'; +import { TypeOrmTransaction } from '../../services/typeorm-transaction'; import { mockEntityManager } from '../../utils/test-utils/mock-entity-manager'; import { CredentialsRepository } from '../credentials.repository'; +import { InstanceCredentialAssignmentRepository } from '../instance-credential-assignment.repository'; describe('CredentialsRepository', () => { const entityManager = mockEntityManager(CredentialsEntity); const credentialsRepository = Container.get(CredentialsRepository); + const assignmentRepository = Container.get(InstanceCredentialAssignmentRepository); beforeEach(() => { vi.resetAllMocks(); }); + it('finds non-project credentials by ID', async () => { + entityManager.find.mockResolvedValueOnce([]); + + await credentialsRepository.findNonProjectCredentialsByIds(['credential-id']); + + expect(entityManager.find).toHaveBeenCalledWith(CredentialsEntity, { + where: { id: In(['credential-id']), usageScope: Not('project') }, + select: ['id'], + }); + }); + + it('finds only dangling project credentials', async () => { + const queryBuilder = mock>(); + queryBuilder.leftJoinAndSelect.mockReturnValue(queryBuilder); + queryBuilder.where.mockReturnValue(queryBuilder); + queryBuilder.andWhere.mockReturnValue(queryBuilder); + queryBuilder.getMany.mockResolvedValue([]); + vi.spyOn(credentialsRepository, 'createQueryBuilder').mockReturnValue(queryBuilder); + + await credentialsRepository.findDanglingProjectCredentials(); + + expect(queryBuilder.andWhere).toHaveBeenCalledWith('credentials.usageScope = :usageScope', { + usageScope: 'project', + }); + }); + + it('uses the operation transaction for instance credential writes', async () => { + const transactionManager = mock(); + const ctx = { trx: new TypeOrmTransaction(transactionManager) }; + const credential = mock({ + id: 'credential-id', + usageScope: 'instance', + }); + transactionManager.save.mockResolvedValue(credential); + transactionManager.findOneBy.mockResolvedValue(credential); + transactionManager.find.mockResolvedValue([]); + transactionManager.delete.mockRejectedValue( + new QueryFailedError('DELETE', [], new Error('foreign key constraint')), + ); + + await credentialsRepository.saveInstanceCredential(credential, ctx); + await credentialsRepository.updateInstanceCredential( + credential.id, + { ...credential, name: 'Updated', type: 'openAiApi', data: 'encrypted' }, + ctx, + ); + await expect( + credentialsRepository.deleteInstanceCredentialIfUnassigned(credential.id, ctx), + ).rejects.toThrow('foreign key constraint'); + + expect(transactionManager.save).toHaveBeenCalledWith(CredentialsEntity, credential); + expect(transactionManager.update).toHaveBeenCalledWith( + CredentialsEntity, + { id: credential.id, usageScope: 'instance' }, + expect.objectContaining({ name: 'Updated' }), + ); + expect(transactionManager.delete).toHaveBeenCalledWith(CredentialsEntity, { + id: credential.id, + usageScope: 'instance', + }); + expect(transactionManager.find).toHaveBeenCalledTimes(1); + expect(entityManager.save).not.toHaveBeenCalled(); + }); + + it('keeps instance credentials that are assigned', async () => { + const credential = mock({ id: 'credential-id', usageScope: 'instance' }); + entityManager.findOneBy.mockResolvedValue(credential); + vi.spyOn(assignmentRepository, 'findCredentialUseIds').mockResolvedValue([ + 'example:primary', + 'example:secondary', + ]); + + await expect( + credentialsRepository.deleteInstanceCredentialIfUnassigned(credential.id), + ).resolves.toEqual({ + status: 'assigned', + credentialUseIds: ['example:primary', 'example:secondary'], + }); + expect(entityManager.delete).not.toHaveBeenCalled(); + }); + + it('reports an assignment created concurrently with deletion', async () => { + const credential = mock({ id: 'credential-id', usageScope: 'instance' }); + entityManager.findOneBy.mockResolvedValue(credential); + vi.spyOn(assignmentRepository, 'findCredentialUseIds') + .mockResolvedValueOnce([]) + .mockResolvedValueOnce(['example:primary']); + entityManager.delete.mockRejectedValue( + new QueryFailedError('DELETE', [], new Error('foreign key constraint')), + ); + + await expect( + credentialsRepository.deleteInstanceCredentialIfUnassigned(credential.id), + ).resolves.toEqual({ + status: 'assigned', + credentialUseIds: ['example:primary'], + }); + }); + describe('findManyAndCount', () => { it('should call findAndCount with options from toFindManyOptions and return [entities, count]', async () => { const mockCredentials = [ diff --git a/packages/@n8n/db/src/repositories/__tests__/instance-credential-assignment.repository.test.ts b/packages/@n8n/db/src/repositories/__tests__/instance-credential-assignment.repository.test.ts new file mode 100644 index 00000000000..a73fe6e33d0 --- /dev/null +++ b/packages/@n8n/db/src/repositories/__tests__/instance-credential-assignment.repository.test.ts @@ -0,0 +1,118 @@ +import { Container } from '@n8n/di'; +import { In, QueryFailedError, type EntityManager } from '@n8n/typeorm'; +import { mock } from 'vitest-mock-extended'; + +import { CredentialsEntity, InstanceCredentialAssignment } from '../../entities'; +import { TypeOrmTransaction } from '../../services/typeorm-transaction'; +import { mockEntityManager } from '../../utils/test-utils/mock-entity-manager'; +import { InstanceCredentialAssignmentRepository } from '../instance-credential-assignment.repository'; + +describe('InstanceCredentialAssignmentRepository', () => { + const entityManager = mockEntityManager(InstanceCredentialAssignment); + const repository = Container.get(InstanceCredentialAssignmentRepository); + const credential = mock({ + id: 'credential-id', + name: 'Primary model', + type: 'openAiApi', + usageScope: 'instance', + }); + + beforeEach(() => { + vi.resetAllMocks(); + }); + + it('finds available credentials by type', async () => { + entityManager.find.mockResolvedValue([credential]); + + await expect(repository.findAvailableCredentials(['openAiApi'])).resolves.toEqual([credential]); + expect(entityManager.find).toHaveBeenCalledWith(CredentialsEntity, { + select: ['id', 'name', 'type'], + where: { usageScope: 'instance', type: In(['openAiApi']) }, + order: { name: 'ASC' }, + }); + }); + + it('assigns an available credential', async () => { + const transactionManager = mock({ + connection: { options: { type: 'postgres' } } as EntityManager['connection'], + }); + transactionManager.findOne.mockResolvedValue(credential); + const ctx = { trx: new TypeOrmTransaction(transactionManager) }; + + await expect( + repository.assignCredential('example:primary', credential.id, ['openAiApi'], ctx), + ).resolves.toBe(credential); + expect(transactionManager.findOne).toHaveBeenCalledWith(CredentialsEntity, { + where: { + id: credential.id, + usageScope: 'instance', + type: In(['openAiApi']), + }, + lock: { mode: 'pessimistic_write' }, + }); + expect(transactionManager.upsert).toHaveBeenCalledWith( + InstanceCredentialAssignment, + { + credentialUseId: 'example:primary', + credentialId: credential.id, + updatedAt: expect.any(Date) as Date, + }, + ['credentialUseId'], + ); + }); + + it('returns null when the credential is deleted before assignment', async () => { + entityManager.findOne.mockResolvedValueOnce(credential).mockResolvedValueOnce(null); + entityManager.upsert.mockRejectedValue( + new QueryFailedError('INSERT', [], new Error('FOREIGN KEY constraint failed')), + ); + + await expect( + repository.assignCredential('example:primary', credential.id, ['openAiApi']), + ).resolves.toBeNull(); + }); + + it('rethrows transient assignment failures', async () => { + entityManager.findOne.mockResolvedValue(credential); + entityManager.upsert.mockRejectedValue( + new QueryFailedError('INSERT', [], new Error('deadlock detected')), + ); + + await expect( + repository.assignCredential('example:primary', credential.id, ['openAiApi']), + ).rejects.toThrow('deadlock detected'); + }); + + it('resolves the assigned credential', async () => { + entityManager.findOneBy.mockResolvedValue( + mock({ + credentialUseId: 'example:primary', + credentialId: credential.id, + }), + ); + entityManager.findOne.mockResolvedValue(credential); + + await expect( + repository.findAssignedCredential('example:primary', ['openAiApi']), + ).resolves.toEqual({ credentialId: credential.id, credential }); + }); + + it('finds a credential use through the operation transaction', async () => { + const transactionManager = mock(); + const ctx = { trx: new TypeOrmTransaction(transactionManager) }; + transactionManager.find.mockResolvedValue([ + mock({ credentialUseId: 'example:primary' }), + mock({ credentialUseId: 'example:secondary' }), + ]); + + await expect(repository.findCredentialUseIds(credential.id, ctx)).resolves.toEqual([ + 'example:primary', + 'example:secondary', + ]); + expect(transactionManager.find).toHaveBeenCalledWith(InstanceCredentialAssignment, { + select: ['credentialUseId'], + where: { credentialId: credential.id }, + order: { credentialUseId: 'ASC' }, + }); + }); +}); diff --git a/packages/@n8n/db/src/repositories/__tests__/settings.repository.test.ts b/packages/@n8n/db/src/repositories/__tests__/settings.repository.test.ts new file mode 100644 index 00000000000..9f655b65edd --- /dev/null +++ b/packages/@n8n/db/src/repositories/__tests__/settings.repository.test.ts @@ -0,0 +1,30 @@ +import { Container } from '@n8n/di'; +import type { EntityManager } from '@n8n/typeorm'; +import { mock } from 'vitest-mock-extended'; + +import { Settings } from '../../entities'; +import { TypeOrmTransaction } from '../../services/typeorm-transaction'; +import { mockEntityManager } from '../../utils/test-utils/mock-entity-manager'; +import { SettingsRepository } from '../settings.repository'; + +describe('SettingsRepository', () => { + mockEntityManager(Settings); + const repository = Container.get(SettingsRepository); + + it('upserts a setting with the provided operation context', async () => { + const transactionManager = mock(); + const ctx = { trx: new TypeOrmTransaction(transactionManager) }; + + await repository.upsertByKey('instanceAi.settings', '{"enabled":true}', true, ctx); + + expect(transactionManager.upsert).toHaveBeenCalledWith( + Settings, + { + key: 'instanceAi.settings', + value: '{"enabled":true}', + loadOnStartup: true, + }, + ['key'], + ); + }); +}); diff --git a/packages/@n8n/db/src/repositories/credentials.repository.ts b/packages/@n8n/db/src/repositories/credentials.repository.ts index e8d594f6b48..0563fa7e13c 100644 --- a/packages/@n8n/db/src/repositories/credentials.repository.ts +++ b/packages/@n8n/db/src/repositories/credentials.repository.ts @@ -1,29 +1,119 @@ import { Container, Service } from '@n8n/di'; import type { Scope } from '@n8n/permissions'; import type { FindManyOptions, SelectQueryBuilder } from '@n8n/typeorm'; -import { DataSource, In, Like, Repository } from '@n8n/typeorm'; +import { DataSource, In, Like, Not, QueryFailedError } from '@n8n/typeorm'; import { CredentialsEntity, type User } from '../entities'; +import { BaseRepository } from './base-repository'; import { addCredentialDependencyExistsFilter, type CredentialDependencyFilter, } from './credential-dependency.repository'; +import { InstanceCredentialAssignmentRepository } from './instance-credential-assignment.repository'; import { SharedCredentialsRepository } from './shared-credentials.repository'; -import type { ListQuery } from '../entities/types-db'; +import type { ICredentialsDb, ListQuery } from '../entities/types-db'; +import type { OperationContext } from '../services/transaction'; @Service() -export class CredentialsRepository extends Repository { - constructor(dataSource: DataSource) { +export class CredentialsRepository extends BaseRepository { + constructor( + dataSource: DataSource, + private readonly instanceCredentialAssignmentRepository: InstanceCredentialAssignmentRepository, + ) { super(CredentialsEntity, dataSource.manager); } async findStartingWith(credentialName: string) { return await this.find({ select: ['name'], - where: { name: Like(`${credentialName}%`) }, + where: { name: Like(`${credentialName}%`), usageScope: 'project' }, }); } + async findNonProjectCredentialsByIds(ids: string[]): Promise { + return await this.find({ + where: { id: In(ids), usageScope: Not('project') }, + select: ['id'], + }); + } + + async findDanglingProjectCredentials(): Promise { + return await this.createQueryBuilder('credentials') + .leftJoinAndSelect('credentials.shared', 'shared') + .where('shared.credentialsId is null') + .andWhere('credentials.usageScope = :usageScope', { usageScope: 'project' }) + .getMany(); + } + + async findInstanceCredentialById( + credentialId: string, + ctx: OperationContext, + ): Promise { + return await this.managerFor(ctx).findOneBy(CredentialsEntity, { + id: credentialId, + usageScope: 'instance', + }); + } + + async saveInstanceCredential( + credential: CredentialsEntity, + ctx: OperationContext, + ): Promise { + return await this.managerFor(ctx).save(CredentialsEntity, credential); + } + + async updateInstanceCredential( + credentialId: string, + data: Pick, + ctx: OperationContext, + ): Promise { + const manager = this.managerFor(ctx); + await manager.update(CredentialsEntity, { id: credentialId, usageScope: 'instance' }, data); + return await manager.findOneBy(CredentialsEntity, { + id: credentialId, + usageScope: 'instance', + }); + } + + async deleteInstanceCredentialIfUnassigned( + credentialId: string, + ctx: OperationContext = {}, + ): Promise< + | { status: 'deleted' } + | { status: 'notFound' } + | { status: 'assigned'; credentialUseIds: string[] } + > { + const manager = this.managerFor(ctx); + const credential = await manager.findOneBy(CredentialsEntity, { + id: credentialId, + usageScope: 'instance', + }); + if (!credential) return { status: 'notFound' }; + + const credentialUseIds = await this.instanceCredentialAssignmentRepository.findCredentialUseIds( + credentialId, + ctx, + ); + if (credentialUseIds.length > 0) return { status: 'assigned', credentialUseIds }; + + try { + const result = await manager.delete(CredentialsEntity, { + id: credentialId, + usageScope: 'instance', + }); + return result.affected === 0 ? { status: 'notFound' } : { status: 'deleted' }; + } catch (error) { + if (error instanceof QueryFailedError && !ctx.trx) { + const concurrentCredentialUseIds = + await this.instanceCredentialAssignmentRepository.findCredentialUseIds(credentialId, ctx); + if (concurrentCredentialUseIds.length > 0) { + return { status: 'assigned', credentialUseIds: concurrentCredentialUseIds }; + } + } + throw error; + } + } + async findMany( listQueryOptions?: ListQuery.Options & { includeData?: boolean; @@ -39,7 +129,7 @@ export class CredentialsRepository extends Repository { findManyOptions.where = { ...findManyOptions.where, id: In(credentialIds) }; } - return await this.find(findManyOptions); + return await this.find(this.onlyProjectCredentials(findManyOptions)); } async findManyAndCount( @@ -57,7 +147,14 @@ export class CredentialsRepository extends Repository { findManyOptions.where = { ...findManyOptions.where, id: In(credentialIds) }; } - return await this.findAndCount(findManyOptions); + return await this.findAndCount(this.onlyProjectCredentials(findManyOptions)); + } + + private onlyProjectCredentials( + findManyOptions: FindManyOptions, + ): FindManyOptions { + findManyOptions.where = { ...findManyOptions.where, usageScope: 'project' }; + return findManyOptions; } private toFindManyOptions( @@ -172,7 +269,9 @@ export class CredentialsRepository extends Repository { } async getManyByIds(ids: string[], { withSharings } = { withSharings: false }) { - const findManyOptions: FindManyOptions = { where: { id: In(ids) } }; + const findManyOptions: FindManyOptions = { + where: { id: In(ids), usageScope: 'project' }, + }; if (withSharings) { findManyOptions.relations = { @@ -212,6 +311,7 @@ export class CredentialsRepository extends Repository { findManyOptions.where = { ...findManyOptions.where, isGlobal: true, + usageScope: 'project', ...(type ? { type: Like(`%${type}%`) } : {}), }; return await this.find(findManyOptions); @@ -226,6 +326,7 @@ export class CredentialsRepository extends Repository { const qb = this.createQueryBuilder('credential'); qb.where('credential.isGlobal = :isGlobal', { isGlobal: true }); + qb.andWhere('credential.usageScope = :usageScope', { usageScope: 'project' }); if (type) { qb.andWhere('credential.type LIKE :type', { type: `%${type}%` }); } @@ -259,7 +360,10 @@ export class CredentialsRepository extends Repository { * Find all credentials that are owned by a personal project. */ async findAllPersonalCredentials(): Promise { - return await this.findBy({ shared: { project: { type: 'personal' } } }); + return await this.findBy({ + usageScope: 'project', + shared: { project: { type: 'personal' } }, + }); } /** @@ -271,6 +375,7 @@ export class CredentialsRepository extends Repository { */ async findAllCredentialsForWorkflow(workflowId: string): Promise { return await this.findBy({ + usageScope: 'project', shared: { project: { sharedWorkflows: { workflowId } } }, }); } @@ -282,7 +387,7 @@ export class CredentialsRepository extends Repository { * are part of this project. */ async findAllCredentialsForProject(projectId: string): Promise { - return await this.findBy({ shared: { projectId } }); + return await this.findBy({ usageScope: 'project', shared: { projectId } }); } /** @@ -294,7 +399,7 @@ export class CredentialsRepository extends Repository { type: string, projectId: string, ): Promise { - return await this.findBy({ name, type, shared: { projectId } }); + return await this.findBy({ name, type, usageScope: 'project', shared: { projectId } }); } /** @@ -360,6 +465,7 @@ export class CredentialsRepository extends Repository { } = {}, ): SelectQueryBuilder { const qb = this.createQueryBuilder('credential'); + qb.andWhere('credential.usageScope = :usageScope', { usageScope: 'project' }); if (options.filters?.dependency) { addCredentialDependencyExistsFilter(qb, options.filters.dependency); diff --git a/packages/@n8n/db/src/repositories/index.ts b/packages/@n8n/db/src/repositories/index.ts index 2790dcf9d66..44342f673b4 100644 --- a/packages/@n8n/db/src/repositories/index.ts +++ b/packages/@n8n/db/src/repositories/index.ts @@ -35,6 +35,7 @@ export { FolderRepository } from './folder.repository'; export { FolderTagMappingRepository } from './folder-tag-mapping.repository'; export { ScopeRepository } from './scope.repository'; export { InvalidAuthTokenRepository } from './invalid-auth-token.repository'; +export { InstanceCredentialAssignmentRepository } from './instance-credential-assignment.repository'; export { LicenseMetricsRepository } from './license-metrics.repository'; export { ProjectRelationRepository } from './project-relation.repository'; export { ProjectRepository, type ProjectListOptions } from './project.repository'; diff --git a/packages/@n8n/db/src/repositories/instance-credential-assignment.repository.ts b/packages/@n8n/db/src/repositories/instance-credential-assignment.repository.ts new file mode 100644 index 00000000000..282ea79ca9a --- /dev/null +++ b/packages/@n8n/db/src/repositories/instance-credential-assignment.repository.ts @@ -0,0 +1,118 @@ +import { Service } from '@n8n/di'; +import { DataSource, In, QueryFailedError, type EntityManager } from '@n8n/typeorm'; + +import { CredentialsEntity, InstanceCredentialAssignment } from '../entities'; +import { BaseRepository } from './base-repository'; +import type { OperationContext } from '../services/transaction'; + +@Service() +export class InstanceCredentialAssignmentRepository extends BaseRepository { + constructor(dataSource: DataSource) { + super(InstanceCredentialAssignment, dataSource.manager); + } + + async findAvailableCredentials( + credentialTypes: readonly string[], + ctx: OperationContext = {}, + ): Promise { + return await this.managerFor(ctx).find(CredentialsEntity, { + select: ['id', 'name', 'type'], + where: { + usageScope: 'instance', + type: In([...credentialTypes]), + }, + order: { name: 'ASC' }, + }); + } + + async assignCredential( + credentialUseId: string, + credentialId: string, + credentialTypes: readonly string[], + ctx: OperationContext = {}, + ): Promise { + const manager = this.managerFor(ctx); + const credential = await this.findCredential( + credentialId, + credentialTypes, + manager, + ctx.trx !== undefined, + ); + if (!credential) return null; + + try { + await manager.upsert( + InstanceCredentialAssignment, + { credentialUseId, credentialId, updatedAt: new Date() }, + ['credentialUseId'], + ); + } catch (error) { + // Distinguish a concurrent deletion from a transient database failure. + if (error instanceof QueryFailedError) { + const stillExists = await this.findCredential(credentialId, credentialTypes, manager) + .then((found) => Boolean(found)) + .catch(() => true); + if (!stillExists) return null; + } + throw error; + } + + return credential; + } + + async clearCredential(credentialUseId: string, ctx: OperationContext = {}): Promise { + const manager = this.managerFor(ctx); + await manager.delete(InstanceCredentialAssignment, { credentialUseId }); + } + + async findAssignedCredentialId( + credentialUseId: string, + ctx: OperationContext = {}, + ): Promise { + const manager = this.managerFor(ctx); + const assignment = await manager.findOneBy(InstanceCredentialAssignment, { credentialUseId }); + return assignment?.credentialId ?? null; + } + + async findCredentialUseIds(credentialId: string, ctx: OperationContext = {}): Promise { + const assignments = await this.managerFor(ctx).find(InstanceCredentialAssignment, { + select: ['credentialUseId'], + where: { credentialId }, + order: { credentialUseId: 'ASC' }, + }); + return assignments.map(({ credentialUseId }) => credentialUseId); + } + + async findAssignedCredential( + credentialUseId: string, + credentialTypes: readonly string[], + ctx: OperationContext = {}, + ): Promise<{ credentialId: string; credential: CredentialsEntity | null } | null> { + const manager = this.managerFor(ctx); + const assignment = await manager.findOneBy(InstanceCredentialAssignment, { credentialUseId }); + if (!assignment) return null; + + return { + credentialId: assignment.credentialId, + credential: await this.findCredential(assignment.credentialId, credentialTypes, manager), + }; + } + + private async findCredential( + credentialId: string, + credentialTypes: readonly string[], + manager: EntityManager, + lock = false, + ): Promise { + return await manager.findOne(CredentialsEntity, { + where: { + id: credentialId, + usageScope: 'instance', + type: In([...credentialTypes]), + }, + ...(lock && manager.connection.options.type === 'postgres' + ? { lock: { mode: 'pessimistic_write' as const } } + : {}), + }); + } +} diff --git a/packages/@n8n/db/src/repositories/settings.repository.ts b/packages/@n8n/db/src/repositories/settings.repository.ts index 727ae03ee9a..79e10eefd61 100644 --- a/packages/@n8n/db/src/repositories/settings.repository.ts +++ b/packages/@n8n/db/src/repositories/settings.repository.ts @@ -1,11 +1,13 @@ import { Service } from '@n8n/di'; import type { EntityManager } from '@n8n/typeorm'; -import { DataSource, In, Like, Repository } from '@n8n/typeorm'; +import { DataSource, In, Like } from '@n8n/typeorm'; import { Settings } from '../entities'; +import { BaseRepository } from './base-repository'; +import type { OperationContext } from '../services/transaction'; @Service() -export class SettingsRepository extends Repository { +export class SettingsRepository extends BaseRepository { constructor(dataSource: DataSource) { super(Settings, dataSource.manager); } @@ -14,6 +16,20 @@ export class SettingsRepository extends Repository { const manager = em ?? this.manager; return await manager.findOneBy(Settings, { key }); } + + async findByKeyInContext(key: string, ctx: OperationContext): Promise { + return await this.managerFor(ctx).findOneBy(Settings, { key }); + } + + async upsertByKey( + key: string, + value: string, + loadOnStartup: boolean, + ctx: OperationContext, + ): Promise { + await this.managerFor(ctx).upsert(Settings, { key, value, loadOnStartup }, ['key']); + } + async findByKeys(keys: string[]): Promise { return await this.findBy({ key: In(keys) }); } diff --git a/packages/@n8n/db/src/services/__tests__/db-lock.service.test.ts b/packages/@n8n/db/src/services/__tests__/db-lock.service.test.ts index 1fdd0178fd5..dda14890aca 100644 --- a/packages/@n8n/db/src/services/__tests__/db-lock.service.test.ts +++ b/packages/@n8n/db/src/services/__tests__/db-lock.service.test.ts @@ -5,6 +5,7 @@ import { OperationalError } from 'n8n-workflow'; import { mock } from 'vitest-mock-extended'; import { DbLockService } from '../db-lock.service'; +import { TypeOrmTransaction } from '../typeorm-transaction'; describe('DbLockService', () => { const mockTx = mock(); @@ -33,7 +34,9 @@ describe('DbLockService', () => { expect(result).toBe('result'); expect(mockTx.query).toHaveBeenCalledWith('SELECT pg_advisory_xact_lock($1)', [1001]); - expect(fn).toHaveBeenCalledWith(mockTx); + expect(fn).toHaveBeenCalledWith(mockTx, { + trx: expect.any(TypeOrmTransaction) as TypeOrmTransaction, + }); }); it('should skip advisory lock on SQLite and still execute fn', async () => { @@ -44,7 +47,9 @@ describe('DbLockService', () => { expect(result).toBe('result'); expect(mockTx.query).not.toHaveBeenCalled(); - expect(fn).toHaveBeenCalledWith(mockTx); + expect(fn).toHaveBeenCalledWith(mockTx, { + trx: expect.any(TypeOrmTransaction) as TypeOrmTransaction, + }); }); it('should set lock_timeout when timeoutMs is provided', async () => { @@ -112,7 +117,9 @@ describe('DbLockService', () => { 'SELECT pg_advisory_xact_lock($1, $2)', [1005, -12345], ); - expect(fn).toHaveBeenCalledWith(mockTx); + expect(fn).toHaveBeenCalledWith(mockTx, { + trx: expect.any(TypeOrmTransaction) as TypeOrmTransaction, + }); }); it('should include both keys in the timeout error message when subKey is provided', async () => { @@ -177,6 +184,18 @@ describe('DbLockService', () => { }); }); + describe('withLockContext', () => { + it('provides an opaque context for the locked transaction', async () => { + databaseConfig.type = 'sqlite'; + const fn = vi.fn().mockResolvedValue('result'); + + await expect(service.withLockContext(1001, fn)).resolves.toBe('result'); + expect(fn).toHaveBeenCalledWith({ + trx: expect.any(TypeOrmTransaction) as TypeOrmTransaction, + }); + }); + }); + describe('tryWithLock', () => { it('should execute fn when lock is acquired', async () => { databaseConfig.type = 'postgresdb'; diff --git a/packages/@n8n/db/src/services/db-lock.service.ts b/packages/@n8n/db/src/services/db-lock.service.ts index ef695dab66a..da398e6cc96 100644 --- a/packages/@n8n/db/src/services/db-lock.service.ts +++ b/packages/@n8n/db/src/services/db-lock.service.ts @@ -5,6 +5,9 @@ import { DataSource, QueryFailedError } from '@n8n/typeorm'; import type { EntityManager } from '@n8n/typeorm'; import { OperationalError } from 'n8n-workflow'; +import type { OperationContext } from './transaction'; +import { TypeOrmTransaction } from './typeorm-transaction'; + /** * Centrally managed advisory lock IDs. Every Postgres advisory lock * used in the application MUST be registered here to prevent collisions. @@ -16,6 +19,7 @@ export const enum DbLock { WORKFLOW_REVIEW_REQUEST_CREATE = 1004, MIGRATIONS = 1005, EVAL_COLLECTION_RERUN = 1006, + INSTANCE_AI_SETTINGS = 1007, /** Reserved for integration tests — never use in production code */ TEST = 9999, } @@ -123,13 +127,15 @@ export class DbLockService { */ async withLock( lockId: DbLock, - fn: (tx: EntityManager) => Promise, + fn: (tx: EntityManager, ctx: OperationContext) => Promise, options?: WithLockOptions, ): Promise { if (this.databaseConfig.type !== 'postgresdb') { const release = await this.acquireLock(lockId, options?.timeoutMs, options?.subKey); try { - return await this.dataSource.manager.transaction(async (tx) => await fn(tx)); + return await this.dataSource.manager.transaction( + async (tx) => await fn(tx, { trx: new TypeOrmTransaction(tx) }), + ); } finally { release(); } @@ -165,10 +171,18 @@ export class DbLockService { } throw error; } - return await fn(tx); + return await fn(tx, { trx: new TypeOrmTransaction(tx) }); }); } + async withLockContext( + lockId: DbLock, + fn: (ctx: OperationContext) => Promise, + options?: { timeoutMs?: number }, + ): Promise { + return await this.withLock(lockId, async (_manager, ctx) => await fn(ctx), options); + } + /** * Execute `fn` inside a database transaction, but only if the advisory * lock (Postgres) or in-process mutex (SQLite) can be acquired immediately. diff --git a/packages/@n8n/decorators/src/pubsub/pubsub-metadata.ts b/packages/@n8n/decorators/src/pubsub/pubsub-metadata.ts index 1c5318d4123..db7dfbcb5c8 100644 --- a/packages/@n8n/decorators/src/pubsub/pubsub-metadata.ts +++ b/packages/@n8n/decorators/src/pubsub/pubsub-metadata.ts @@ -34,6 +34,7 @@ export type PubSubEventName = | 'reload-source-control-config' | 'reload-mcp-registry' | 'reload-otel-config' + | 'reload-instance-ai-settings' | 'cancel-test-run' | 'cancel-collection' | 'agent-chat-integration-changed' diff --git a/packages/@n8n/permissions/src/__tests__/__snapshots__/scope-information.test.ts.snap b/packages/@n8n/permissions/src/__tests__/__snapshots__/scope-information.test.ts.snap index ff5dfab5be9..e0f71e7a598 100644 --- a/packages/@n8n/permissions/src/__tests__/__snapshots__/scope-information.test.ts.snap +++ b/packages/@n8n/permissions/src/__tests__/__snapshots__/scope-information.test.ts.snap @@ -31,6 +31,7 @@ exports[`Scope Information > ensure scopes are defined correctly 1`] = ` "credential:move", "credential:connect", "credential:createEndUser", + "credential:manageInstance", "credential:create", "credential:read", "credential:update", diff --git a/packages/@n8n/permissions/src/constants.ee.ts b/packages/@n8n/permissions/src/constants.ee.ts index c4b9871d9f5..1af89ba4422 100644 --- a/packages/@n8n/permissions/src/constants.ee.ts +++ b/packages/@n8n/permissions/src/constants.ee.ts @@ -15,6 +15,7 @@ export const RESOURCES = { 'move', 'connect', 'createEndUser', + 'manageInstance', ...DEFAULT_OPERATIONS, ] as const, externalSecretsProvider: ['sync', ...DEFAULT_OPERATIONS] as const, diff --git a/packages/@n8n/permissions/src/roles/scopes/global-scopes.ee.ts b/packages/@n8n/permissions/src/roles/scopes/global-scopes.ee.ts index f7105c0be2d..0bed16809a5 100644 --- a/packages/@n8n/permissions/src/roles/scopes/global-scopes.ee.ts +++ b/packages/@n8n/permissions/src/roles/scopes/global-scopes.ee.ts @@ -29,6 +29,7 @@ export const GLOBAL_OWNER_SCOPES: Scope[] = [ 'credential:move', 'credential:connect', 'credential:createEndUser', + 'credential:manageInstance', 'community:register', 'communityPackage:install', 'communityPackage:uninstall', diff --git a/packages/@n8n/permissions/src/scope-information.ts b/packages/@n8n/permissions/src/scope-information.ts index a5f97a7de07..90127ad8318 100644 --- a/packages/@n8n/permissions/src/scope-information.ts +++ b/packages/@n8n/permissions/src/scope-information.ts @@ -103,6 +103,11 @@ export const scopeInformation: Partial> = { description: "Allows creating, deleting, and changing the type of end-user credentials, which resolve to each user's own connection.", }, + 'credential:manageInstance': { + displayName: 'Manage provider connections', + description: + 'Allows creating, updating, and deleting provider connections used by instance-level features. These connections are not available in workflows.', + }, 'insights:read': { displayName: 'Read Insights', description: 'Allows reading insights data.', diff --git a/packages/cli/src/__tests__/credentials-helper.test.ts b/packages/cli/src/__tests__/credentials-helper.test.ts index 3723d1036c2..3c5282e4796 100644 --- a/packages/cli/src/__tests__/credentials-helper.test.ts +++ b/packages/cli/src/__tests__/credentials-helper.test.ts @@ -101,6 +101,21 @@ describe('CredentialsHelper', () => { credentialsHelper.getCredentials({ id: '1', name: 'foo' }, 'bar'), ).rejects.toThrow(errorMessage); }); + + test('rejects credentials that are not available to workflows', async () => { + credentialsRepository.findOneByOrFail.mockResolvedValueOnce( + mock({ + id: '1', + name: 'Instance credential', + type: 'bar', + usageScope: 'instance', + }), + ); + + await expect( + credentialsHelper.getCredentials({ id: '1', name: 'foo' }, 'bar'), + ).rejects.toThrow('This credential cannot be used in workflows'); + }); }); describe('applyDefaultsAndOverwrites', () => { @@ -555,6 +570,7 @@ describe('CredentialsHelper', () => { name: 'Test OAuth2 Credential', type: 'oAuth2Api', data: cipher.encrypt(existingCredentialData), + usageScope: 'project', }; credentialsRepository.findOneByOrFail.mockResolvedValue( @@ -659,6 +675,7 @@ describe('CredentialsHelper', () => { data: cipher.encrypt(existingCredentialData), isResolvable: true, resolverId: 'resolver-123', + usageScope: 'project', } as CredentialsEntity; credentialsRepository.findOneByOrFail.mockResolvedValue(mockCredentialEntity); @@ -708,6 +725,7 @@ describe('CredentialsHelper', () => { data: cipher.encrypt(existingCredentialData), isResolvable: true, resolverId: null, + usageScope: 'project', } as unknown as CredentialsEntity; credentialsRepository.findOneByOrFail.mockResolvedValue(mockCredentialEntity); @@ -762,6 +780,7 @@ describe('CredentialsHelper', () => { data: cipher.encrypt(existingCredentialData), isResolvable: true, resolverId: 'resolver-123', + usageScope: 'project', } as CredentialsEntity; credentialsRepository.findOneByOrFail.mockResolvedValue(mockCredentialEntity); @@ -813,6 +832,7 @@ describe('CredentialsHelper', () => { data: cipher.encrypt(existingCredentialData), isResolvable: true, resolverId: 'resolver-123', + usageScope: 'project', } as CredentialsEntity; credentialsRepository.findOneByOrFail.mockResolvedValue(mockCredentialEntity); @@ -1012,6 +1032,7 @@ describe('CredentialsHelper', () => { type: 'testApi', data: cipher.encrypt({ apiKey: 'test' }), isResolvable: false, + usageScope: 'project', } as CredentialsEntity; beforeEach(() => { @@ -1097,6 +1118,7 @@ describe('CredentialsHelper', () => { type: credentialType, data: cipher.encrypt({ apiKey: 'static-key' }), isResolvable: false, + usageScope: 'project', } as CredentialsEntity; beforeEach(() => { @@ -1555,6 +1577,7 @@ describe('CredentialsHelper', () => { data: cipher.encrypt(credentialDataA), isResolvable: false, resolverId: null, + usageScope: 'project', } as CredentialsEntity; const credEntityB = { @@ -1564,6 +1587,7 @@ describe('CredentialsHelper', () => { data: cipher.encrypt(credentialDataB), isResolvable: false, resolverId: null, + usageScope: 'project', } as CredentialsEntity; const additionalData = { diff --git a/packages/cli/src/commands/import/credentials.ts b/packages/cli/src/commands/import/credentials.ts index b3aa504cf1e..c302c4d8160 100644 --- a/packages/cli/src/commands/import/credentials.ts +++ b/packages/cli/src/commands/import/credentials.ts @@ -1,10 +1,13 @@ import { CredentialsEntity, + DbLock, + DbLockService, Project, User, SharedCredentials, ProjectRepository, GLOBAL_OWNER_ROLE, + type OperationContext, } from '@n8n/db'; import { Command } from '@n8n/decorators'; import { Container } from '@n8n/di'; @@ -15,10 +18,11 @@ import fs from 'fs'; import omit from 'lodash/omit'; import pick from 'lodash/pick'; import { Cipher } from 'n8n-core'; -import { jsonParse, UserError } from 'n8n-workflow'; +import { jsonParse, UserError, type ICredentialDataDecryptedObject } from 'n8n-workflow'; import { z } from 'zod'; import { UM_FIX_INSTRUCTION } from '@/constants'; +import { CredentialsService } from '@/credentials/credentials.service'; import { BaseCommand } from '../base-command'; @@ -62,6 +66,9 @@ type ImportableCredentialProperty = Exclude< 'shared' | 'toJSON' | 'generateId' | 'setUpdateDate' >; +const isCredentialData = (data: unknown): data is ICredentialDataDecryptedObject => + typeof data === 'object' && data !== null && !Array.isArray(data); + @Command({ name: 'import:credentials', description: 'Import credentials', @@ -77,8 +84,6 @@ type ImportableCredentialProperty = Exclude< flagsSchema, }) export class ImportCredentialsCommand extends BaseCommand> { - private transactionManager: EntityManager; - async run(): Promise { const { flags } = this; @@ -118,22 +123,27 @@ export class ImportCredentialsCommand extends BaseCommand { - this.transactionManager = transactionManager; + await Container.get(DbLockService).withLock( + DbLock.INSTANCE_AI_SETTINGS, + async (transactionManager, ctx) => { + const project = await this.getProject(transactionManager, flags.userId, flags.projectId); - const project = await this.getProject(flags.userId, flags.projectId); + const result = await this.checkRelations( + transactionManager, + credentials, + flags.projectId, + flags.userId, + ); - const result = await this.checkRelations(credentials, flags.projectId, flags.userId); + if (!result.success) { + throw new UserError(result.message); + } - if (!result.success) { - throw new UserError(result.message); - } - - for (const credential of credentials) { - await this.storeCredential(credential, project); - } - }); + for (const credential of credentials) { + await this.storeCredential(transactionManager, credential, project, ctx); + } + }, + ); this.reportSuccess(credentials.length); } @@ -151,19 +161,75 @@ export class ImportCredentialsCommand extends BaseCommand, project: Project) { - const result = await this.transactionManager.upsert(CredentialsEntity, credential, ['id']); + private async storeCredential( + transactionManager: EntityManager, + credential: Partial, + project: Project, + ctx: OperationContext, + ) { + // UsageScope is instance-local state; imports never change it for existing credentials. + let existing: Pick | null = null; + if (credential.id) { + existing = await transactionManager.findOne(CredentialsEntity, { + where: { id: credential.id }, + select: ['id', 'usageScope', 'type'], + }); + if (existing) { + credential.usageScope = existing.usageScope; + if ( + existing.usageScope === 'instance' && + credential.type !== undefined && + credential.type !== existing.type + ) { + throw new UserError( + 'Provider connection type cannot be changed. Create a new connection instead.', + ); + } + } + } + credential.usageScope ??= 'project'; - const sharingExists = await this.transactionManager.existsBy(SharedCredentials, { - credentialsId: credential.id, + if (credential.usageScope === 'instance') { + if ( + credential.isGlobal || + credential.isResolvable || + credential.isManaged || + credential.resolvableAllowFallback || + credential.resolverId + ) { + throw new UserError( + 'Provider connections cannot be global, managed, or dynamically resolved', + ); + } + Object.assign(credential, { + isGlobal: false, + isResolvable: false, + isManaged: false, + resolvableAllowFallback: false, + resolverId: null, + }); + await this.validateInstanceCredentialData(transactionManager, credential, existing, ctx); + } + + const result = await transactionManager.upsert(CredentialsEntity, credential, ['id']); + const credentialsId = credential.id ?? (result.identifiers[0].id as string); + + if (credential.usageScope === 'instance') { + // Instance credentials are instance-owned and must not retain project sharing rows. + await transactionManager.delete(SharedCredentials, { credentialsId }); + return; + } + + const sharingExists = await transactionManager.existsBy(SharedCredentials, { + credentialsId, role: 'credential:owner', }); if (!sharingExists) { - await this.transactionManager.upsert( + await transactionManager.upsert( SharedCredentials, { - credentialsId: result.identifiers[0].id as string, + credentialsId, role: 'credential:owner', projectId: project.id, }, @@ -172,7 +238,48 @@ export class ImportCredentialsCommand extends BaseCommand, + existing: Pick | null, + ctx: OperationContext, + ) { + let data: unknown = credential.data; + if (data === undefined && credential.id) { + data = ( + await transactionManager.findOne(CredentialsEntity, { + where: { id: credential.id }, + select: { data: true }, + }) + )?.data; + } + if (data === undefined) return; + if (data === null || data === '') { + throw new UserError('Provider connection data cannot be empty'); + } + + const decrypted = + typeof data === 'string' + ? jsonParse(await Container.get(Cipher).decryptV2(data)) + : data; + if (!isCredentialData(decrypted)) { + throw new UserError('Provider connection data must be a JSON object'); + } + const credentialsService = Container.get(CredentialsService); + if (existing?.usageScope === 'instance') { + await credentialsService.validateInstanceCredentialUpdate( + existing, + decrypted, + undefined, + ctx, + ); + } else { + credentialsService.validateInstanceCredentialData(decrypted); + } + } + private async checkRelations( + transactionManager: EntityManager, credentials: Array, 'id'>>, projectId?: string, userId?: string, @@ -190,11 +297,14 @@ export class ImportCredentialsCommand extends BaseCommand { const filteredCredential = this.filterCredentialProperties(credential, include, exclude); - if (typeof filteredCredential.data === 'object') { + if (isCredentialData(filteredCredential.data)) { // plain data / decrypted input. Should be encrypted first. filteredCredential.data = await cipher.encryptV2(filteredCredential.data); } @@ -350,19 +460,20 @@ export class ImportCredentialsCommand extends BaseCommand; return property in importableProperties; } - private async getCredentialOwner(credentialsId: string) { - const sharedCredential = await this.transactionManager.findOne(SharedCredentials, { + private async getCredentialOwner(transactionManager: EntityManager, credentialsId: string) { + const sharedCredential = await transactionManager.findOne(SharedCredentials, { where: { credentialsId, role: 'credential:owner' }, relations: { project: true }, }); if (sharedCredential && sharedCredential.project.type === 'personal') { - const user = await this.transactionManager.findOneByOrFail(User, { + const user = await transactionManager.findOneByOrFail(User, { projectRelations: { role: { slug: PROJECT_OWNER_ROLE_SLUG }, projectId: sharedCredential.projectId, @@ -375,17 +486,17 @@ export class ImportCredentialsCommand extends BaseCommand Container.get(SharedCredentialsRepository).create({ credentials, diff --git a/packages/cli/src/credentials-helper.ts b/packages/cli/src/credentials-helper.ts index fde22da0c9f..4c904ce2b8d 100644 --- a/packages/cli/src/credentials-helper.ts +++ b/packages/cli/src/credentials-helper.ts @@ -33,6 +33,7 @@ import { NodeHelpers, Workflow, UnexpectedError, + UserError, isExpression, } from 'n8n-workflow'; @@ -308,6 +309,11 @@ export class CredentialsHelper extends ICredentialsHelper { throw error; } + // Keep non-project credentials blocked even if an earlier access check is bypassed. + if (credential.usageScope !== 'project') { + throw new UserError('This credential cannot be used in workflows'); + } + return credential; } diff --git a/packages/cli/src/credentials/__tests__/credentials.controller.test.ts b/packages/cli/src/credentials/__tests__/credentials.controller.test.ts index 1bf0896d85a..2a2c67cbd11 100644 --- a/packages/cli/src/credentials/__tests__/credentials.controller.test.ts +++ b/packages/cli/src/credentials/__tests__/credentials.controller.test.ts @@ -62,6 +62,9 @@ describe('CredentialsController', () => { mock(), // externalSecretsConfig mock(), // externalSecretsProviderAccessCheckService mock(), // connectionStatusProxy + mock(), // instanceCredentialAssignmentRepository + mock(), // instanceCredentialUseRegistry + mock(), // dbLockService ); // Spy on methods that need to be mocked in tests @@ -69,6 +72,7 @@ describe('CredentialsController', () => { // for isChangingExternalSecretExpression and validateExternalSecretsPermissions let decryptSpy: MockInstance; let createEncryptedDataSpy: MockInstance; + let prepareUpdateDataSpy: MockInstance; let getCredentialScopesSpy: MockInstance; let updateSpy: MockInstance; let createUnmanagedCredentialSpy: MockInstance; @@ -102,6 +106,7 @@ describe('CredentialsController', () => { vi.resetAllMocks(); decryptSpy = vi.spyOn(credentialsService, 'decrypt'); createEncryptedDataSpy = vi.spyOn(credentialsService, 'createEncryptedData'); + prepareUpdateDataSpy = vi.spyOn(credentialsService, 'prepareUpdateData'); getCredentialScopesSpy = vi.spyOn(credentialsService, 'getCredentialScopes'); updateSpy = vi.spyOn(credentialsService, 'update'); createUnmanagedCredentialSpy = vi.spyOn(credentialsService, 'createUnmanagedCredential'); @@ -355,6 +360,89 @@ describe('CredentialsController', () => { }); }); + it('should accept unchanged false flags when editing an instance credential', async () => { + const instanceCredential = mock({ + ...existingCredential, + usageScope: 'instance', + shared: [], + }); + const ownerReq = { + user: { id: 'owner-id', role: GLOBAL_OWNER_ROLE }, + params: { credentialId }, + body: { + name: 'Updated Credential', + type: 'apiKey', + data: { apiKey: 'updated-key' }, + isGlobal: false, + isResolvable: false, + }, + } as unknown as CredentialRequest.Update; + credentialsFinderService.findCredentialForUser.mockResolvedValue(instanceCredential); + prepareUpdateDataSpy.mockResolvedValue(ownerReq.body); + updateSpy.mockResolvedValue(instanceCredential); + + await expect(credentialsController.updateCredentials(ownerReq)).resolves.toBeDefined(); + expect(credentialsFinderService.findCredentialForUser).toHaveBeenCalledWith( + credentialId, + ownerReq.user, + ['credential:update'], + { includeInstanceCredentials: true }, + ); + }); + + it.each([{ isGlobal: true }, { isResolvable: true }])( + 'should reject converting an instance credential with %o', + async (flag) => { + const instanceCredential = mock({ + ...existingCredential, + usageScope: 'instance', + shared: [], + }); + const ownerReq = { + user: { id: 'owner-id', role: GLOBAL_OWNER_ROLE }, + params: { credentialId }, + body: { + name: 'Updated Credential', + type: 'apiKey', + data: { apiKey: 'updated-key' }, + isGlobal: false, + isResolvable: false, + ...flag, + }, + } as unknown as CredentialRequest.Update; + credentialsFinderService.findCredentialForUser.mockResolvedValue(instanceCredential); + + await expect(credentialsController.updateCredentials(ownerReq)).rejects.toThrow( + BadRequestError, + ); + expect(updateSpy).not.toHaveBeenCalled(); + }, + ); + + it('should reject changing the type of an instance credential', async () => { + const instanceCredential = mock({ + ...existingCredential, + type: 'apiKey', + usageScope: 'instance', + shared: [], + }); + const ownerReq = { + user: { id: 'owner-id', role: GLOBAL_OWNER_ROLE }, + params: { credentialId }, + body: { + name: 'Updated Credential', + type: 'httpHeaderAuth', + data: { name: 'x-api-key', value: 'secret' }, + }, + } as unknown as CredentialRequest.Update; + credentialsFinderService.findCredentialForUser.mockResolvedValue(instanceCredential); + + await expect(credentialsController.updateCredentials(ownerReq)).rejects.toThrow( + 'Provider connection type cannot be changed', + ); + expect(prepareUpdateDataSpy).not.toHaveBeenCalled(); + }); + it('should emit "credentials-updated" with jweEnabled true when JWE is enabled in payload', async () => { // ARRANGE const ownerReq = { diff --git a/packages/cli/src/credentials/__tests__/credentials.service.ee.test.ts b/packages/cli/src/credentials/__tests__/credentials.service.ee.test.ts index ea2762d8057..7dd2e75d3ac 100644 --- a/packages/cli/src/credentials/__tests__/credentials.service.ee.test.ts +++ b/packages/cli/src/credentials/__tests__/credentials.service.ee.test.ts @@ -47,6 +47,24 @@ describe('EnterpriseCredentialsService', () => { vi.resetAllMocks(); }); + describe('shareWithProjects', () => { + it('rejects credentials that are not available to workflows', async () => { + const manager = { + exists: vi.fn().mockResolvedValue(false), + find: vi.fn(), + save: vi.fn(), + }; + + // @ts-expect-error - Mocking manager for testing + sharedCredentialsRepository.manager = manager; + + await expect( + service.shareWithProjects(mock(), 'credential-id', ['project-id']), + ).rejects.toThrow('Credential not found'); + expect(manager.find).not.toHaveBeenCalled(); + }); + }); + /** * Helper function to mock the transaction manager for credential transfer tests */ @@ -265,6 +283,7 @@ describe('EnterpriseCredentialsService', () => { credentialId, user, ['credential:connect'], + { includeInstanceCredentials: true }, ); expect(result).toHaveProperty('data', redacted); }); @@ -297,6 +316,7 @@ describe('EnterpriseCredentialsService', () => { credentialId, user, ['credential:connect'], + { includeInstanceCredentials: true }, ); expect(result).not.toHaveProperty('data'); }); diff --git a/packages/cli/src/credentials/__tests__/credentials.service.test.ts b/packages/cli/src/credentials/__tests__/credentials.service.test.ts index 6adeb78e0ec..0f32205417b 100644 --- a/packages/cli/src/credentials/__tests__/credentials.service.test.ts +++ b/packages/cli/src/credentials/__tests__/credentials.service.test.ts @@ -1,16 +1,18 @@ import type { Logger } from '@n8n/backend-common'; import type { - CredentialsEntity, CredentialsRepository, ICredentialsDb, + InstanceCredentialAssignmentRepository, SharedCredentialsRepository, ProjectRepository, UserRepository, User, SharedCredentials, ListQueryDb, + DbLockService, } from '@n8n/db'; -import { GLOBAL_OWNER_ROLE, GLOBAL_MEMBER_ROLE } from '@n8n/db'; +import { CredentialsEntity, DbLock, GLOBAL_OWNER_ROLE, GLOBAL_MEMBER_ROLE } from '@n8n/db'; +import type { EntityManager } from '@n8n/typeorm'; import { CREDENTIAL_ERRORS, CredentialDataError, Credentials, type ErrorReporter } from 'n8n-core'; import { CREDENTIAL_BLANKING_VALUE, @@ -27,6 +29,7 @@ import type { CredentialConnectionStatusProxy } from '@/credentials/credential-c import type { CredentialDependencyService } from '@/credentials/credential-dependency.service'; import type { CredentialsFinderService } from '@/credentials/credentials-finder.service'; import { CredentialsService } from '@/credentials/credentials.service'; +import type { InstanceCredentialUseRegistry } from '@/credentials/instance-credential-use.registry'; import * as validation from '@/credentials/validation'; import type { CredentialsHelper } from '@/credentials-helper'; import { CredentialNotFoundError } from '@/errors/credential-not-found.error'; @@ -81,6 +84,9 @@ describe('CredentialsService', () => { const externalSecretsConfig = mock(); const externalSecretsProviderAccessCheckService = mock(); const connectionStatusProxy = mock(); + const instanceCredentialAssignmentRepository = mock(); + const instanceCredentialUseRegistry = mock(); + const dbLockService = mock(); const service = new CredentialsService( credentialsRepository, @@ -101,6 +107,9 @@ describe('CredentialsService', () => { externalSecretsConfig, externalSecretsProviderAccessCheckService, connectionStatusProxy, + instanceCredentialAssignmentRepository, + instanceCredentialUseRegistry, + dbLockService, ); beforeEach(() => { @@ -111,6 +120,9 @@ describe('CredentialsService', () => { credentialDependencyService.syncExternalSecretProviderDependenciesForCredential.mockResolvedValue( undefined, ); + credentialDependencyService.upsertExternalSecretProviderDependenciesForCredential.mockResolvedValue( + undefined, + ); ownershipService.addOwnedByAndSharedWith.mockImplementation((credential: any) => credential); // Mock the subquery method used by member users and admin users with onlySharedWithMe credentialsRepository.getManyAndCountWithSharingSubquery.mockResolvedValue({ @@ -119,6 +131,7 @@ describe('CredentialsService', () => { }); // Mock roleService for the new subquery implementation roleService.rolesWithScope.mockResolvedValue(['project:viewer', 'project:editor']); + instanceCredentialAssignmentRepository.findCredentialUseIds.mockResolvedValue([]); }); /** @@ -878,11 +891,26 @@ describe('CredentialsService', () => { expect(credentialsTester.testCredentials).not.toHaveBeenCalled(); }); + it('does not expose instance credentials through public API testing', async () => { + credentialsFinderService.findCredentialById.mockResolvedValue( + mock({ + id: 'instance-credential', + usageScope: 'instance', + }), + ); + + await expect(service.testById(ownerUser.id, 'instance-credential')).rejects.toThrow( + CredentialNotFoundError, + ); + expect(credentialsTester.testCredentials).not.toHaveBeenCalled(); + }); + it('decrypts stored credential and calls credentials tester', async () => { const storedCredential = mock({ id: 'credential-id', name: 'Test Credential', type: 'githubApi', + usageScope: 'project', }); const decryptedData = { accessToken: 'secret-token' } as ICredentialDataDecryptedObject; const testResult = { status: 'OK', message: 'Credential tested successfully' } as const; @@ -909,7 +937,440 @@ describe('CredentialsService', () => { }); }); + describe('getOne', () => { + const instanceCredential = mock({ + id: 'instance-credential', + usageScope: 'instance', + data: 'encrypted-data', + }); + + it('does not return instance credentials without an explicit opt-in', async () => { + credentialsRepository.findOneBy.mockResolvedValue(instanceCredential); + sharedCredentialsRepository.findOne.mockResolvedValue(null); + + await expect(service.getOne(ownerUser, instanceCredential.id, false)).rejects.toThrow( + `Credential with ID "${instanceCredential.id}" could not be found.`, + ); + expect(credentialsRepository.findOneBy).not.toHaveBeenCalled(); + expect(sharedCredentialsRepository.findOne).toHaveBeenCalledWith({ + where: { + credentialsId: instanceCredential.id, + credentials: { usageScope: 'project' }, + }, + relations: { credentials: true }, + }); + }); + + it('returns instance credentials to managers when explicitly requested', async () => { + credentialsRepository.findOneBy.mockResolvedValue(instanceCredential); + + await expect( + service.getOne(ownerUser, instanceCredential.id, false, { + includeInstanceCredentials: true, + }), + ).resolves.toMatchObject({ id: instanceCredential.id, usageScope: 'instance' }); + expect(credentialsRepository.findOneBy).toHaveBeenCalledWith({ + id: instanceCredential.id, + usageScope: 'instance', + }); + }); + }); + + describe('delete', () => { + it('does not opt generic callers into instance credential access', async () => { + credentialsFinderService.findCredentialForUser.mockResolvedValue(null); + + await service.delete(ownerUser, 'credential-id'); + + expect(credentialsFinderService.findCredentialForUser).toHaveBeenCalledWith( + 'credential-id', + ownerUser, + ['credential:delete'], + {}, + ); + expect(credentialsRepository.remove).not.toHaveBeenCalled(); + }); + + it('deletes instance credentials when management access is explicitly requested', async () => { + const credential = mock({ + id: 'instance-credential', + usageScope: 'instance', + isResolvable: false, + }); + credentialsFinderService.findCredentialForUser.mockResolvedValue(credential); + credentialsRepository.deleteInstanceCredentialIfUnassigned.mockResolvedValue({ + status: 'deleted', + }); + + await service.delete(ownerUser, credential.id, { includeInstanceCredentials: true }); + + expect(credentialsFinderService.findCredentialForUser).toHaveBeenCalledWith( + credential.id, + ownerUser, + ['credential:delete'], + { includeInstanceCredentials: true }, + ); + expect(credentialsRepository.deleteInstanceCredentialIfUnassigned).toHaveBeenCalledWith( + credential.id, + ); + expect(externalHooks.run).toHaveBeenCalledWith('credentials.delete', [credential.id]); + }); + + it('does not delete an instance credential bound to a feature', async () => { + const credential = mock({ + id: 'instance-credential', + usageScope: 'instance', + isResolvable: false, + }); + credentialsFinderService.findCredentialForUser.mockResolvedValue(credential); + credentialsRepository.deleteInstanceCredentialIfUnassigned.mockResolvedValue({ + status: 'assigned', + credentialUseIds: ['instance-ai:model'], + }); + + await expect( + service.delete(ownerUser, credential.id, { includeInstanceCredentials: true }), + ).rejects.toThrow('instance-ai:model'); + expect(externalHooks.run).toHaveBeenCalledWith('credentials.delete', [credential.id]); + }); + }); + + describe('update', () => { + const setRepositoryTransaction = (transactionManager: EntityManager) => { + const transaction = vi.fn( + async (run: (manager: EntityManager) => Promise) => await run(transactionManager), + ); + Object.defineProperty(credentialsRepository, 'manager', { + configurable: true, + value: { transaction }, + writable: true, + }); + return transaction; + }; + + it('serializes instance credential validation and persistence with settings updates', async () => { + const credential = mock({ + id: 'instance-credential', + name: 'Provider connection', + type: 'apiKey', + }); + const encrypted = await service.createEncryptedData({ + id: credential.id, + name: credential.name, + type: credential.type, + data: { apiKey: 'secret' }, + }); + const lockTransactionManager = mock(); + lockTransactionManager.findOneBy.mockResolvedValue(credential); + const repositoryTransaction = setRepositoryTransaction(mock()); + let lockHeld = false; + let releaseLock = () => {}; + const lockAvailable = new Promise((resolve) => { + releaseLock = resolve; + }); + dbLockService.withLock.mockImplementation(async (lockId, run) => { + expect(lockId).toBe(DbLock.INSTANCE_AI_SETTINGS); + expect(externalHooks.run).toHaveBeenCalledWith('credentials.update', [encrypted]); + await lockAvailable; + lockHeld = true; + try { + return await run(lockTransactionManager, {}); + } finally { + lockHeld = false; + } + }); + instanceCredentialAssignmentRepository.findCredentialUseIds.mockImplementation(async () => { + expect(lockHeld).toBe(true); + return []; + }); + + const update = service.update(credential.id, encrypted, undefined, { + instanceCredential: credential, + }); + await vi.waitFor(() => expect(dbLockService.withLock).toHaveBeenCalledOnce()); + expect(lockTransactionManager.update).not.toHaveBeenCalled(); + releaseLock(); + await expect(update).resolves.toBe(credential); + + expect(repositoryTransaction).not.toHaveBeenCalled(); + expect(lockTransactionManager.update).toHaveBeenCalledWith( + CredentialsEntity, + credential.id, + encrypted, + ); + }); + + it('keeps project credential updates on their existing transaction path', async () => { + const credential = mock({ id: 'project-credential' }); + const transactionManager = mock(); + transactionManager.findOneBy.mockResolvedValue(credential); + const repositoryTransaction = setRepositoryTransaction(transactionManager); + const encrypted = mock({ + id: credential.id, + name: 'Project credential', + type: 'apiKey', + data: 'encrypted', + }); + + await expect(service.update(credential.id, encrypted)).resolves.toBe(credential); + + expect(repositoryTransaction).toHaveBeenCalledOnce(); + expect(dbLockService.withLock).not.toHaveBeenCalled(); + expect(transactionManager.update).toHaveBeenCalledWith( + CredentialsEntity, + credential.id, + encrypted, + ); + }); + }); + + describe('updateInstanceCredential', () => { + const payload = { name: 'AI Assistant model', type: 'openAiApi', data: { apiKey: 'new-key' } }; + const preparedCredential = { + name: payload.name, + type: payload.type, + data: payload.data, + } as unknown as CredentialsEntity; + const ctx = {}; + + beforeEach(() => { + credentialsRepository.findInstanceCredentialById.mockResolvedValue( + mock({ + id: 'instance-credential', + type: 'openAiApi', + usageScope: 'instance', + }), + ); + instanceCredentialAssignmentRepository.findCredentialUseIds.mockResolvedValue([]); + }); + + it('rejects users without the instance credential management scope', async () => { + await expect( + service.updateInstanceCredential(memberUser, 'instance-credential', payload, ctx), + ).rejects.toThrow('You do not have permission to update provider connections'); + expect(credentialsRepository.updateInstanceCredential).not.toHaveBeenCalled(); + }); + + it('rejects a credential type change', async () => { + vi.spyOn(service, 'prepareUpdateData').mockResolvedValue( + mock({ name: payload.name, type: 'anthropicApi' }), + ); + + await expect( + service.updateInstanceCredential( + ownerUser, + 'instance-credential', + { ...payload, type: 'anthropicApi' }, + ctx, + ), + ).rejects.toThrow('Provider connection type cannot be changed'); + expect(credentialsRepository.updateInstanceCredential).not.toHaveBeenCalled(); + }); + + it('runs the update hook unless the caller already ran it', async () => { + const encrypted = await service.createEncryptedData({ + id: 'instance-credential', + ...payload, + }); + vi.spyOn(service, 'prepareUpdateData').mockResolvedValue(preparedCredential); + vi.spyOn(service, 'createEncryptedData').mockResolvedValue(encrypted as never); + credentialsRepository.updateInstanceCredential.mockResolvedValue( + mock({ id: 'instance-credential' }), + ); + + await service.updateInstanceCredential(ownerUser, 'instance-credential', payload, ctx); + expect(externalHooks.run).toHaveBeenCalledWith('credentials.update', [encrypted]); + + externalHooks.run.mockClear(); + await service.updateInstanceCredential(ownerUser, 'instance-credential', payload, ctx, { + skipExternalHooks: true, + }); + expect(externalHooks.run).not.toHaveBeenCalled(); + expect(credentialsRepository.updateInstanceCredential).toHaveBeenCalledWith( + 'instance-credential', + encrypted, + ctx, + ); + + const hooked = { ...encrypted, name: 'Updated by hook' }; + await service.updateInstanceCredential(ownerUser, 'instance-credential', payload, ctx, { + skipExternalHooks: true, + encryptedData: hooked as never, + }); + expect(credentialsRepository.updateInstanceCredential).toHaveBeenLastCalledWith( + 'instance-credential', + hooked, + ctx, + ); + }); + + it('revalidates a pre-hooked payload before updating', async () => { + vi.spyOn(service, 'prepareUpdateData').mockResolvedValue(preparedCredential); + const encrypted = await service.createEncryptedData({ + id: 'instance-credential', + name: payload.name, + type: payload.type, + data: { apiKey: 'rejected-by-use' }, + }); + instanceCredentialAssignmentRepository.findCredentialUseIds.mockResolvedValue([ + 'instance-ai:model', + ]); + instanceCredentialUseRegistry.get.mockReturnValue({ + id: 'instance-ai:model', + credentialTypes: [payload.type], + validate: ({ data }) => { + if (data.apiKey === 'rejected-by-use') throw new Error('Invalid hooked API key'); + }, + }); + + await expect( + service.updateInstanceCredential(ownerUser, 'instance-credential', payload, ctx, { + skipExternalHooks: true, + encryptedData: encrypted, + }), + ).rejects.toThrow('Invalid hooked API key'); + expect(credentialsRepository.updateInstanceCredential).not.toHaveBeenCalled(); + }); + + it('rejects invalid ciphertext from a pre-run hook', async () => { + vi.spyOn(service, 'prepareUpdateData').mockResolvedValue(preparedCredential); + const encrypted = await service.createEncryptedData({ + id: 'instance-credential', + ...payload, + }); + encrypted.data = 'invalid-ciphertext'; + + await expect( + service.updateInstanceCredential(ownerUser, 'instance-credential', payload, ctx, { + skipExternalHooks: true, + encryptedData: encrypted, + }), + ).rejects.toThrow('Provider connection hooks returned invalid credential data'); + expect(credentialsRepository.updateInstanceCredential).not.toHaveBeenCalled(); + }); + + it.each([ + ['ID', { id: 'other-credential' }], + ['type', { type: 'anthropicApi' }], + ['scope', { isGlobal: true }], + ['scope', { isManaged: true }], + ['scope', { isResolvable: true }], + ['scope', { resolvableAllowFallback: true }], + ['scope', { resolverId: 'resolver-id' }], + ['scope', { usageScope: 'project' }], + ])('rejects a hook that changes provider connection %s', async (invariant, mutation) => { + vi.spyOn(service, 'prepareUpdateData').mockResolvedValue(preparedCredential); + const encrypted = await service.createEncryptedData({ + id: 'instance-credential', + ...payload, + }); + Object.assign(encrypted, mutation); + + await expect( + service.updateInstanceCredential(ownerUser, 'instance-credential', payload, ctx, { + skipExternalHooks: true, + encryptedData: encrypted, + }), + ).rejects.toThrow(`Provider connection hooks cannot change the credential ${invariant}`); + expect(credentialsRepository.updateInstanceCredential).not.toHaveBeenCalled(); + }); + }); + + describe('createInstanceCredential hook validation', () => { + it('revalidates the data after the create hook', async () => { + credentialsHelper.getCredentialsProperties.mockReturnValue([]); + const invalid = await service.createEncryptedData({ + id: null, + name: 'Instance Credential', + type: 'apiKey', + data: { apiKey: '={{ $secrets.provider.key }}' }, + }); + externalHooks.run.mockImplementation(async (_event, hookParameters) => { + const hookData = (hookParameters as [ICredentialsDb] | undefined)?.[0]; + if (!hookData) throw new Error('Expected credential hook data'); + hookData.data = invalid.data; + }); + + await expect( + service.createUnmanagedCredential( + { + name: 'Instance Credential', + type: 'apiKey', + data: { apiKey: 'valid' }, + usageScope: 'instance', + }, + ownerUser, + ), + ).rejects.toThrow('Provider connections cannot reference project-scoped external secrets'); + expect(credentialsRepository.saveInstanceCredential).not.toHaveBeenCalled(); + }); + }); + + describe('runInstanceCredentialHooks', () => { + it('returns the payload after the matching hook runs', async () => { + const encrypted = { name: 'AI Assistant model', type: 'openAiApi', data: 'encrypted' }; + const createEncryptedDataSpy = vi + .spyOn(service, 'createEncryptedData') + .mockResolvedValue(encrypted as never); + externalHooks.run.mockImplementation(async (_event, hookParameters) => { + const hookData = (hookParameters as [ICredentialsDb] | undefined)?.[0]; + if (!hookData) throw new Error('Expected credential hook data'); + hookData.name = 'Updated by hook'; + }); + + const result = await service.runInstanceCredentialHooks('create', { + id: null, + name: 'AI Assistant model', + type: 'openAiApi', + data: { apiKey: 'k' }, + }); + + expect(createEncryptedDataSpy).toHaveBeenCalledWith({ + id: null, + name: 'AI Assistant model', + type: 'openAiApi', + data: { apiKey: 'k' }, + }); + expect(externalHooks.run).toHaveBeenCalledWith('credentials.create', [encrypted]); + expect(result).toBe(encrypted); + expect(result.name).toBe('Updated by hook'); + }); + }); + describe('testWithCredentials', () => { + it('tests an unsaved provider connection for an instance credential manager', async () => { + const testResult = { status: 'OK', message: 'Credential tested successfully' } as const; + credentialsFinderService.findCredentialForUser.mockResolvedValue(null); + credentialsTester.testCredentials.mockResolvedValue(testResult); + const payload = { + id: '', + name: 'AI Assistant model', + type: 'openAiApi', + data: { apiKey: 'key' }, + }; + + await expect(service.testWithCredentials(ownerUser, payload)).resolves.toEqual(testResult); + expect(credentialsTester.testCredentials).toHaveBeenCalledWith( + ownerUser.id, + payload.type, + payload, + ); + }); + + it('does not test an unsaved provider connection for other users', async () => { + credentialsFinderService.findCredentialForUser.mockResolvedValue(null); + + await expect( + service.testWithCredentials(memberUser, { + id: '', + name: 'AI Assistant model', + type: 'openAiApi', + data: { apiKey: 'key' }, + }), + ).rejects.toThrow(CredentialNotFoundError); + expect(credentialsTester.testCredentials).not.toHaveBeenCalled(); + }); + it('throws CredentialNotFoundError when user cannot access credential', async () => { credentialsFinderService.findCredentialForUser.mockResolvedValue(null); @@ -954,6 +1415,7 @@ describe('CredentialsService', () => { payload.id, ownerUser, ['credential:read'], + { includeInstanceCredentials: true }, ); expect(service.decrypt).toHaveBeenCalledWith(storedCredential, true); expect(service.unredact).toHaveBeenCalledWith(payload.data, decryptedData, []); @@ -2393,6 +2855,52 @@ describe('CredentialsService', () => { externalSecretsProviderAccessCheckService.isProviderAvailableInProject, ).toHaveBeenCalledWith('validProvider', 'WHwt9vP3keCUvmB5'); }); + + it('should reject project-scoped external secrets in instance credentials', async () => { + credentialsHelper.getCredentialsProperties.mockReturnValue([]); + const payload = { + name: 'Test Credential', + type: 'apiKey', + data: { + apiKey: '={{ $secrets.validProvider.bar }}', + }, + usageScope: 'instance' as const, + }; + + await expect(service.createUnmanagedCredential(payload, ownerUser)).rejects.toThrow( + 'Provider connections cannot reference project-scoped external secrets', + ); + expect(projectRepository.getPersonalProjectForUserOrFail).not.toHaveBeenCalled(); + }); + + it('should reject instance credential creation without the global scope', async () => { + await expect( + service.createUnmanagedCredential( + { + name: 'Instance Credential', + type: 'apiKey', + data: {}, + usageScope: 'instance', + }, + memberUser, + ), + ).rejects.toThrow('You do not have permission to create provider connections'); + }); + + it('should reject contradictory instance credential flags', async () => { + await expect( + service.createUnmanagedCredential( + { + name: 'Instance Credential', + type: 'apiKey', + data: {}, + usageScope: 'instance', + isGlobal: true, + }, + ownerUser, + ), + ).rejects.toThrow('Provider connections cannot be globally shared'); + }); }); }); @@ -2662,6 +3170,122 @@ describe('CredentialsService', () => { await service.prepareUpdateData(ownerUser, payload, existingCredential); }); + + it('should reject project-scoped external secrets when updating an instance credential', async () => { + credentialsHelper.getCredentialsProperties.mockReturnValue([]); + const payload = { + name: 'Test Credential', + type: 'apiKey', + data: { + apiKey: '={{ $secrets.validProvider.bar }}', + }, + }; + const existingCredential = mockExistingCredential({ + id: 'instance-credential-id', + name: 'Test Credential', + type: 'apiKey', + data: { apiKey: 'old-key' }, + usageScope: 'instance', + shared: [], + }); + + credentialsRepository.create.mockImplementation((data) => ({ ...data }) as any); + + await expect( + service.prepareUpdateData(ownerUser, payload, existingCredential), + ).rejects.toThrow('Provider connections cannot reference project-scoped external secrets'); + expect(projectRepository.getPersonalProjectForUserOrFail).not.toHaveBeenCalled(); + }); + }); + + describe('assigned instance credentials', () => { + const existingCredential = mockExistingCredential({ + id: 'instance-credential-id', + name: 'AI Assistant sandbox', + type: 'httpHeaderAuth', + data: {}, + usageScope: 'instance', + shared: [], + }); + const payload = { + name: 'AI Assistant sandbox', + type: 'httpHeaderAuth', + data: { name: 'Authorization', value: 'secret' }, + }; + + beforeEach(() => { + vi.spyOn(service, 'decrypt').mockResolvedValue({}); + credentialsHelper.getCredentialsProperties.mockReturnValue([]); + credentialsRepository.create.mockImplementation((data) => ({ ...data }) as never); + }); + + it('runs the registered per-use validation with the updated data', async () => { + const validatePrimary = vi.fn(); + const validateSecondary = vi.fn(); + const operationContext = {}; + instanceCredentialAssignmentRepository.findCredentialUseIds.mockResolvedValue([ + 'instance-ai:sandbox:n8n', + 'other-feature:sandbox', + ]); + instanceCredentialUseRegistry.get.mockImplementation((credentialUseId) => ({ + id: credentialUseId, + credentialTypes: ['httpHeaderAuth'], + validate: + credentialUseId === 'instance-ai:sandbox:n8n' ? validatePrimary : validateSecondary, + })); + + await service.prepareUpdateData(ownerUser, payload, existingCredential, { + operationContext, + }); + + const expectedCredential = { + type: 'httpHeaderAuth', + data: { name: 'Authorization', value: 'secret' }, + }; + expect(validatePrimary).toHaveBeenCalledWith(expectedCredential); + expect(validateSecondary).toHaveBeenCalledWith(expectedCredential); + expect(instanceCredentialAssignmentRepository.findCredentialUseIds).toHaveBeenCalledWith( + existingCredential.id, + operationContext, + ); + }); + + it('propagates per-use validation failures', async () => { + instanceCredentialAssignmentRepository.findCredentialUseIds.mockResolvedValue([ + 'instance-ai:sandbox:n8n', + ]); + instanceCredentialUseRegistry.get.mockReturnValue({ + id: 'instance-ai:sandbox:n8n', + credentialTypes: ['httpHeaderAuth'], + validate: () => { + throw new Error('The credential\'s header name must be "x-api-key"'); + }, + }); + + await expect( + service.prepareUpdateData(ownerUser, payload, existingCredential), + ).rejects.toThrow(/x-api-key/); + }); + + it('skips per-use validation for unassigned instance credentials', async () => { + await expect( + service.prepareUpdateData(ownerUser, payload, existingCredential), + ).resolves.toBeDefined(); + expect(instanceCredentialUseRegistry.get).not.toHaveBeenCalled(); + }); + + it('fails closed when an assigned use is no longer registered', async () => { + instanceCredentialAssignmentRepository.findCredentialUseIds.mockResolvedValue([ + 'instance-ai:sandbox:n8n', + ]); + instanceCredentialUseRegistry.get.mockImplementation(() => { + throw new Error('Unknown instance credential use "instance-ai:sandbox:n8n"'); + }); + + await expect( + service.prepareUpdateData(ownerUser, payload, existingCredential), + ).rejects.toThrow('Unknown instance credential use'); + }); }); }); diff --git a/packages/cli/src/credentials/__tests__/instance-credential-broker.test.ts b/packages/cli/src/credentials/__tests__/instance-credential-broker.test.ts new file mode 100644 index 00000000000..1cdf4e7e8cb --- /dev/null +++ b/packages/cli/src/credentials/__tests__/instance-credential-broker.test.ts @@ -0,0 +1,156 @@ +import type { Logger } from '@n8n/backend-common'; +import type { CredentialsEntity, InstanceCredentialAssignmentRepository } from '@n8n/db'; +import { mock } from 'vitest-mock-extended'; + +import type { CredentialsService } from '../credentials.service'; +import { InstanceCredentialBroker } from '../instance-credential-broker'; +import { InstanceCredentialUseRegistry } from '../instance-credential-use.registry'; + +describe('InstanceCredentialBroker', () => { + const credentialUse = { + id: 'example:primary', + credentialTypes: ['openAiApi'], + } as const; + const credentialUseId = credentialUse.id; + const logger = mock(); + const assignmentRepository = mock(); + const credentialsService = mock(); + const useRegistry = new InstanceCredentialUseRegistry(); + useRegistry.register(credentialUse); + const broker = new InstanceCredentialBroker( + logger, + useRegistry, + assignmentRepository, + credentialsService, + ); + + beforeEach(() => { + vi.resetAllMocks(); + }); + + it('rejects credential use IDs with surrounding whitespace', () => { + const registry = new InstanceCredentialUseRegistry(); + + expect(() => + registry.register({ id: ' example:primary', credentialTypes: ['openAiApi'] }), + ).toThrow('cannot contain surrounding whitespace'); + }); + + it('rejects unregistered credential uses', async () => { + const unknownUse = { id: 'unknown:use', credentialTypes: ['openAiApi'] } as const; + + await expect(broker.listForUse(unknownUse)).rejects.toThrow( + 'Unknown instance credential use "unknown:use"', + ); + expect(assignmentRepository.findAvailableCredentials).not.toHaveBeenCalled(); + }); + + it('enforces the registered credential types', async () => { + const spoofedUse = { id: credentialUseId, credentialTypes: ['httpHeaderAuth'] } as const; + + await broker.listForUse(spoofedUse); + + expect(assignmentRepository.findAvailableCredentials).toHaveBeenCalledWith(['openAiApi']); + }); + + it('lists only credentials allowed for the credential use', async () => { + const credential = mock({ + id: 'credential-id', + name: 'Primary model', + type: 'openAiApi', + usageScope: 'instance', + }); + assignmentRepository.findAvailableCredentials.mockResolvedValue([credential]); + + await expect(broker.listForUse(credentialUse)).resolves.toEqual([credential]); + expect(assignmentRepository.findAvailableCredentials).toHaveBeenCalledWith(['openAiApi']); + }); + + it('rejects assignments outside the credential use policy', async () => { + assignmentRepository.assignCredential.mockResolvedValue(null); + + await expect(broker.assignForUse(credentialUse, 'workflow-credential')).rejects.toThrow( + 'not valid for instance credential use', + ); + expect(credentialsService.decrypt).not.toHaveBeenCalled(); + }); + + it('assigns and clears a credential for a credential use', async () => { + const credential = mock({ + id: 'credential-id', + name: 'Primary model', + type: 'openAiApi', + usageScope: 'instance', + }); + assignmentRepository.assignCredential.mockResolvedValue(credential); + + await expect(broker.assignForUse(credentialUse, credential.id)).resolves.toEqual({ + id: credential.id, + name: credential.name, + type: credential.type, + }); + expect(assignmentRepository.assignCredential).toHaveBeenCalledWith( + credentialUseId, + credential.id, + ['openAiApi'], + undefined, + ); + + await broker.clearForUse(credentialUse); + expect(assignmentRepository.clearCredential).toHaveBeenCalledWith(credentialUseId, undefined); + }); + + it('rejects the assignment when the credential is deleted concurrently', async () => { + assignmentRepository.assignCredential.mockResolvedValue(null); + + await expect(broker.assignForUse(credentialUse, 'credential-id')).rejects.toThrow( + 'not valid for instance credential use', + ); + }); + + it('rethrows transient DB failures instead of blaming the credential', async () => { + assignmentRepository.assignCredential.mockRejectedValue(new Error('deadlock detected')); + + await expect(broker.assignForUse(credentialUse, 'credential-id')).rejects.toThrow( + 'deadlock detected', + ); + }); + + it('returns no credential when the credential use has no assignment', async () => { + assignmentRepository.findAssignedCredential.mockResolvedValue(null); + + await expect(broker.resolveForUse(credentialUse)).resolves.toBeNull(); + expect(credentialsService.decrypt).not.toHaveBeenCalled(); + }); + + it('resolves a credential through the shared decrypt path', async () => { + const credential = mock({ + id: 'credential-id', + name: 'Primary model', + type: 'openAiApi', + usageScope: 'instance', + }); + assignmentRepository.findAssignedCredential.mockResolvedValue({ + credentialId: credential.id, + credential, + }); + credentialsService.decrypt.mockResolvedValue({ apiKey: 'secret' }); + + await expect(broker.resolveForUse(credentialUse)).resolves.toEqual({ + id: credential.id, + name: credential.name, + type: credential.type, + data: { apiKey: 'secret' }, + }); + expect(credentialsService.decrypt).toHaveBeenCalledWith(credential, true); + expect(logger.debug).toHaveBeenCalledWith('Resolved instance credential', { + credentialUseId, + credentialId: credential.id, + }); + expect(assignmentRepository.findAssignedCredential).toHaveBeenCalledWith( + credentialUseId, + ['openAiApi'], + undefined, + ); + }); +}); diff --git a/packages/cli/src/credentials/credentials-finder.service.ts b/packages/cli/src/credentials/credentials-finder.service.ts index c37d1450172..d55b47bca10 100644 --- a/packages/cli/src/credentials/credentials-finder.service.ts +++ b/packages/cli/src/credentials/credentials-finder.service.ts @@ -22,7 +22,7 @@ export class CredentialsFinderService { private async fetchGlobalCredentials(trx?: EntityManager): Promise { const em = trx ?? this.credentialsRepository.manager; return await em.find(CredentialsEntity, { - where: { isGlobal: true }, + where: { isGlobal: true, usageScope: 'project' }, relations: { shared: true }, }); } @@ -46,13 +46,22 @@ export class CredentialsFinderService { where: { id: credentialId, isGlobal: true, + usageScope: 'project', }, relations, }); } - async findCredentialById(credentialId: string): Promise { - return await this.credentialsRepository.findOne({ where: { id: credentialId } }); + async findCredentialById( + credentialId: string, + options: { includeInstanceCredentials?: boolean } = {}, + ): Promise { + return await this.credentialsRepository.findOne({ + where: { + id: credentialId, + usageScope: options.includeInstanceCredentials ? In(['project', 'instance']) : 'project', + }, + }); } /** @@ -81,7 +90,10 @@ export class CredentialsFinderService { * all scopes the user has for the credential using `RoleService.addScopes`. **/ async findCredentialsForUser(user: User, scopes: Scope[]) { - let where: FindOptionsWhere = { isGlobal: false }; + let where: FindOptionsWhere = { + isGlobal: false, + usageScope: 'project', + }; if (!hasGlobalScope(user, scopes, { mode: 'allOf' })) { const [projectRoles, credentialRoles] = await Promise.all([ @@ -121,7 +133,16 @@ export class CredentialsFinderService { credentialsId: string, user: User, scopes: Scope[], + options: { includeInstanceCredentials?: boolean } = {}, ): Promise { + if (options.includeInstanceCredentials && hasGlobalScope(user, 'credential:manageInstance')) { + const instanceCredential = await this.credentialsRepository.findOneBy({ + id: credentialsId, + usageScope: 'instance', + }); + if (instanceCredential) return instanceCredential; + } + let where: FindOptionsWhere = { credentialsId }; if (!hasGlobalScope(user, scopes, { mode: 'allOf' })) { @@ -152,6 +173,7 @@ export class CredentialsFinderService { }); if (sharedCredential) { + if (sharedCredential.credentials.usageScope !== 'project') return null; return sharedCredential.credentials; } @@ -172,7 +194,9 @@ export class CredentialsFinderService { trx?: EntityManager, options?: { includeGlobalCredentials?: boolean }, ) { - let where: FindOptionsWhere = {}; + let where: FindOptionsWhere = { + credentials: { usageScope: 'project' }, + }; if (!hasGlobalScope(user, scopes, { mode: 'allOf' })) { const [projectRoles, credentialRoles] = await Promise.all([ @@ -180,6 +204,7 @@ export class CredentialsFinderService { this.roleService.rolesWithScope('credential', scopes), ]); where = { + ...where, role: In(credentialRoles), project: { projectRelations: { @@ -232,7 +257,10 @@ export class CredentialsFinderService { ): Promise> { if (credentialIds.length === 0) return new Set(); - let where: FindOptionsWhere = { credentialsId: In(credentialIds) }; + let where: FindOptionsWhere = { + credentialsId: In(credentialIds), + credentials: { usageScope: 'project' }, + }; if (!hasGlobalScope(user, scopes, { mode: 'allOf' })) { const [projectRoles, credentialRoles] = await Promise.all([ @@ -261,7 +289,7 @@ export class CredentialsFinderService { // Also include global credentials if scopes allow read-only access if (this.hasGlobalReadOnlyAccess(scopes)) { const globalCreds = await this.credentialsRepository.find({ - where: { id: In(credentialIds), isGlobal: true }, + where: { id: In(credentialIds), isGlobal: true, usageScope: 'project' }, select: ['id'], }); for (const gc of globalCreds) result.add(gc.id); diff --git a/packages/cli/src/credentials/credentials.controller.ts b/packages/cli/src/credentials/credentials.controller.ts index df88955064f..4b642c15e2c 100644 --- a/packages/cli/src/credentials/credentials.controller.ts +++ b/packages/cli/src/credentials/credentials.controller.ts @@ -130,7 +130,9 @@ export class CredentialsController { // to do so. query.includeData, ) - : await this.credentialsService.getOne(req.user, credentialId, query.includeData); + : await this.credentialsService.getOne(req.user, credentialId, query.includeData, { + includeInstanceCredentials: true, + }); const scopes = await this.credentialsService.getCredentialScopes( req.user, @@ -210,6 +212,7 @@ export class CredentialsController { credentialId, user, ['credential:update'], + { includeInstanceCredentials: true }, ); if (!credential) { @@ -226,6 +229,25 @@ export class CredentialsController { throw new BadRequestError('Managed credentials cannot be updated'); } + if ( + credential.usageScope === 'instance' && + body.type !== undefined && + body.type !== credential.type + ) { + throw new BadRequestError( + 'Provider connection type cannot be changed. Create a new connection instead.', + ); + } + + if ( + credential.usageScope === 'instance' && + (body.isGlobal === true || body.isResolvable === true) + ) { + throw new BadRequestError( + 'Provider connections cannot be globally shared or converted to end-user credentials', + ); + } + // We never want to allow users to change the oauthTokenData delete body.data?.oauthTokenData; @@ -290,7 +312,10 @@ export class CredentialsController { body.data ? (preparedCredentialData.data as unknown as ICredentialDataDecryptedObject) : undefined, - { deleteUserEntries: isTogglingToStatic || sharedFieldsChanged }, + { + deleteUserEntries: isTogglingToStatic || sharedFieldsChanged, + instanceCredential: credential.usageScope === 'instance' ? credential : undefined, + }, ); if (responseData === null) { @@ -384,6 +409,7 @@ export class CredentialsController { credentialId, req.user, ['credential:delete'], + { includeInstanceCredentials: true }, ); if (!credential) { @@ -396,7 +422,9 @@ export class CredentialsController { ); } - await this.credentialsService.delete(req.user, credential.id); + await this.credentialsService.delete(req.user, credential.id, { + includeInstanceCredentials: true, + }); this.eventService.emit('credentials-deleted', { user: req.user, diff --git a/packages/cli/src/credentials/credentials.service.ee.ts b/packages/cli/src/credentials/credentials.service.ee.ts index e148330dd0c..846c7817a5f 100644 --- a/packages/cli/src/credentials/credentials.service.ee.ts +++ b/packages/cli/src/credentials/credentials.service.ee.ts @@ -1,6 +1,11 @@ import { LicenseState } from '@n8n/backend-common'; -import type { CredentialsEntity, User } from '@n8n/db'; -import { Project, SharedCredentials, SharedCredentialsRepository } from '@n8n/db'; +import type { User } from '@n8n/db'; +import { + CredentialsEntity, + Project, + SharedCredentials, + SharedCredentialsRepository, +} from '@n8n/db'; import { Service } from '@n8n/di'; import { hasGlobalScope } from '@n8n/permissions'; import { In, type EntityManager } from '@n8n/typeorm'; @@ -41,6 +46,11 @@ export class EnterpriseCredentialsService { entityManager?: EntityManager, ) { const em = entityManager ?? this.sharedCredentialsRepository.manager; + const canShare = await em.exists(CredentialsEntity, { + where: { id: credentialId, usageScope: 'project' }, + }); + if (!canShare) throw new NotFoundError('Credential not found'); + const roles = await this.roleService.rolesWithScope('project', ['project:list']); let projects = await em.find(Project, { @@ -102,6 +112,7 @@ export class EnterpriseCredentialsService { // TODO: replace credential:update with credential:decrypt once it lands // see: https://n8nio.slack.com/archives/C062YRE7EG4/p1708531433206069?thread_ts=1708525972.054149&cid=C062YRE7EG4 ['credential:read', 'credential:update'], + { includeInstanceCredentials: true }, ) : null; @@ -112,9 +123,12 @@ export class EnterpriseCredentialsService { } else { // Otherwise try to find them with only the `credential:read` scope. In // that case we return them without the decrypted data. - credential = await this.credentialsFinderService.findCredentialForUser(credentialId, user, [ - 'credential:read', - ]); + credential = await this.credentialsFinderService.findCredentialForUser( + credentialId, + user, + ['credential:read'], + { includeInstanceCredentials: true }, + ); // Connect-capable users of a private credential need the redacted blueprint // (secrets stay masked) so the UI can detect the OAuth type and render the @@ -122,9 +136,12 @@ export class EnterpriseCredentialsService { if ( includeDecryptedData && credential?.isResolvable && - (await this.credentialsFinderService.findCredentialForUser(credentialId, user, [ - 'credential:connect', - ])) + (await this.credentialsFinderService.findCredentialForUser( + credentialId, + user, + ['credential:connect'], + { includeInstanceCredentials: true }, + )) ) { decryptedData = await this.credentialsService.decrypt(credential); } diff --git a/packages/cli/src/credentials/credentials.service.ts b/packages/cli/src/credentials/credentials.service.ts index 55d15578317..388bace398e 100644 --- a/packages/cli/src/credentials/credentials.service.ts +++ b/packages/cli/src/credentials/credentials.service.ts @@ -3,13 +3,23 @@ import { Logger } from '@n8n/backend-common'; import { Project, CredentialsEntity, + DbLock, + DbLockService, SharedCredentials, CredentialsRepository, + InstanceCredentialAssignmentRepository, ProjectRepository, SharedCredentialsRepository, UserRepository, } from '@n8n/db'; -import type { ListQueryDb, SlimProject, User, ICredentialsDb, ScopesField } from '@n8n/db'; +import type { + ListQueryDb, + SlimProject, + User, + ICredentialsDb, + ScopesField, + OperationContext, +} from '@n8n/db'; import { Service } from '@n8n/di'; import { hasGlobalScope, PROJECT_OWNER_ROLE_SLUG, type Scope } from '@n8n/permissions'; import { @@ -64,6 +74,8 @@ import { type CredentialDependencyFilter, } from './credential-dependency.service'; import { CredentialsFinderService } from './credentials-finder.service'; +import { getExternalSecretExpressionPaths } from './external-secrets.utils'; +import { InstanceCredentialUseRegistry } from './instance-credential-use.registry'; import { validateAccessToReferencedSecretProviders, validateExternalSecretsPermissions, @@ -82,17 +94,40 @@ type PrepareUpdateDataOptions = { * updated credential blob. Used on Static→Private toggle. */ clearOauthTokenData?: boolean; + operationContext?: OperationContext; }; type UpdateOptions = { /** When true, delete all per-user entries for this credential (Private→Static toggle). */ deleteUserEntries?: boolean; + /** Existing instance credential whose hook-mutated payload must be revalidated. */ + instanceCredential?: Pick; }; type CreateCredentialOptions = CreateCredentialDto & { isManaged: boolean; }; +type InstanceCredentialWriteOptions = { + /** Set when the caller already ran the external hooks before its transaction. */ + skipExternalHooks?: boolean; + /** Carries hook mutations into the transaction. */ + encryptedData?: ICredentialsDb; +}; + +type InstanceCredentialHookPayload = ICredentialsDb & + Partial< + Pick< + CredentialsEntity, + | 'isGlobal' + | 'isManaged' + | 'isResolvable' + | 'resolvableAllowFallback' + | 'resolverId' + | 'usageScope' + > + >; + type GetManyCredentialsOptions = { listQueryOptions: ListQuery.Options; includeGlobal: boolean; @@ -140,6 +175,9 @@ export class CredentialsService { private readonly externalSecretsConfig: ExternalSecretsConfig, private readonly externalSecretsProviderAccessCheckService: SecretsProviderAccessCheckService, private readonly connectionStatusProxy: CredentialConnectionStatusProxy, + private readonly instanceCredentialAssignmentRepository: InstanceCredentialAssignmentRepository, + private readonly instanceCredentialUseRegistry: InstanceCredentialUseRegistry, + private readonly dbLockService: DbLockService, ) {} /** @@ -677,7 +715,10 @@ export class CredentialsService { globalScopes: Scope[], relations: FindOptionsRelations = { credentials: true }, ): Promise { - let where: FindOptionsWhere = { credentialsId: credentialId }; + let where: FindOptionsWhere = { + credentialsId: credentialId, + credentials: { usageScope: 'project' }, + }; if (!hasGlobalScope(user, globalScopes, { mode: 'allOf' })) { where = { @@ -706,27 +747,6 @@ export class CredentialsService { ): Promise { const decryptedData = await this.decrypt(existingCredential, true); - // eslint-disable-next-line @typescript-eslint/no-non-null-asserted-optional-chain -- credential will always have an owner - const projectOwningCredential = existingCredential.shared?.find( - (shared) => shared.role === 'credential:owner', - )!; - - await validateExternalSecretsPermissions({ - user, - projectId: projectOwningCredential.projectId, - dataToSave: data.data, - decryptedExistingData: decryptedData, - }); - - if (this.externalSecretsConfig.externalSecretsForProjects && data.data) { - await validateAccessToReferencedSecretProviders( - projectOwningCredential.projectId, - data.data, - this.externalSecretsProviderAccessCheckService, - 'update', - ); - } - const mergedData = deepCopy(data); if (mergedData.data) { mergedData.data = this.unredact( @@ -735,6 +755,37 @@ export class CredentialsService { this.getCredentialTypeProperties(existingCredential.type), ); } + if (existingCredential.usageScope === 'instance') { + await this.validateInstanceCredentialUpdate( + existingCredential, + mergedData.data ?? decryptedData, + undefined, + options?.operationContext, + ); + } else { + const projectOwningCredential = existingCredential.shared?.find( + (shared) => shared.role === 'credential:owner', + ); + if (!projectOwningCredential) { + throw new NotFoundError(`Could not find owner for credential "${existingCredential.id}"`); + } + + await validateExternalSecretsPermissions({ + user, + projectId: projectOwningCredential.projectId, + dataToSave: data.data, + decryptedExistingData: decryptedData, + }); + + if (this.externalSecretsConfig.externalSecretsForProjects && data.data) { + await validateAccessToReferencedSecretProviders( + projectOwningCredential.projectId, + data.data, + this.externalSecretsProviderAccessCheckService, + 'update', + ); + } + } // This saves us a merge but requires some type casting. These // types are compatible for this case. @@ -836,7 +887,7 @@ export class CredentialsService { ) { await this.externalHooks.run('credentials.update', [newCredentialData]); - return await this.credentialsRepository.manager.transaction(async (transactionManager) => { + const persist = async (transactionManager: EntityManager) => { // Update the credentials in DB await transactionManager.update(CredentialsEntity, credentialId, newCredentialData); @@ -855,7 +906,100 @@ export class CredentialsService { // 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 transactionManager.findOneBy(CredentialsEntity, { id: credentialId }); + }; + + if (!options?.instanceCredential) { + return await this.credentialsRepository.manager.transaction(persist); + } + + const instanceCredential = options.instanceCredential; + const hookedData = await this.getValidatedInstanceCredentialHookData( + newCredentialData, + instanceCredential.id, + instanceCredential.type, + ); + return await this.dbLockService.withLock( + DbLock.INSTANCE_AI_SETTINGS, + async (transactionManager, ctx) => { + await this.validateInstanceCredentialUpdate(instanceCredential, hookedData, undefined, ctx); + return await persist(transactionManager); + }, + ); + } + + /** + * Runs external credential hooks with an encrypted snapshot of the payload. + * Transactional callers suppress hooks on the write and invoke this only + * after the transaction commits. + */ + async runInstanceCredentialHooks( + event: 'create' | 'update', + credential: { + id: string | null; + name: string; + type: string; + data: ICredentialDataDecryptedObject; + }, + ): Promise { + const encrypted = await this.createEncryptedData(credential); + await this.externalHooks.run(`credentials.${event}`, [encrypted]); + return encrypted; + } + + async updateInstanceCredential( + user: User, + credentialId: string, + data: CredentialRequest.CredentialProperties, + ctx: OperationContext, + options: InstanceCredentialWriteOptions = {}, + ): Promise { + if (!hasGlobalScope(user, 'credential:manageInstance')) { + throw new ForbiddenError('You do not have permission to update provider connections'); + } + + const credential = await this.credentialsRepository.findInstanceCredentialById( + credentialId, + ctx, + ); + if (!credential) { + throw new NotFoundError(`Credential with ID "${credentialId}" could not be found.`); + } + + const prepared = await this.prepareUpdateData(user, data, credential, { + operationContext: ctx, }); + if (prepared.type !== credential.type) { + throw new BadRequestError( + 'Provider connection type cannot be changed. Create a new connection instead.', + ); + } + const decryptedData = prepared.data as unknown as ICredentialDataDecryptedObject; + const encrypted = + options.encryptedData ?? + (await this.createEncryptedData({ + id: credential.id, + name: prepared.name, + type: prepared.type, + data: decryptedData, + })); + if (!options.skipExternalHooks) { + await this.externalHooks.run('credentials.update', [encrypted]); + } + const hookedData = await this.getValidatedInstanceCredentialHookData( + encrypted, + credential.id, + credential.type, + ); + await this.validateInstanceCredentialUpdate(credential, hookedData, undefined, ctx); + const updated = await this.credentialsRepository.updateInstanceCredential( + credential.id, + encrypted, + ctx, + ); + if (!updated) { + throw new NotFoundError(`Credential with ID "${credentialId}" could not be found.`); + } + return updated; } private async resolveOwningProjectIdForNewCredential( @@ -943,13 +1087,16 @@ export class CredentialsService { * If the user does not have permission to delete the credential this does * nothing and returns void. */ - async delete(user: User, credentialId: string) { - await this.externalHooks.run('credentials.delete', [credentialId]); - + async delete( + user: User, + credentialId: string, + options: { includeInstanceCredentials?: boolean } = {}, + ) { const credential = await this.credentialsFinderService.findCredentialForUser( credentialId, user, ['credential:delete'], + options, ); if (!credential) { @@ -961,6 +1108,19 @@ export class CredentialsService { await this.sharedCredentialsRepository.findCredentialOwningProject(credentialId); await this.ensureCanManageEndUserCredential(user, owningProject?.id); } + await this.externalHooks.run('credentials.delete', [credentialId]); + + if (credential.usageScope === 'instance') { + const result = await this.credentialsRepository.deleteInstanceCredentialIfUnassigned( + credential.id, + ); + if (result.status === 'assigned') { + throw new BadRequestError( + `This credential is assigned to credential use "${result.credentialUseIds.join(', ')}" and cannot be deleted`, + ); + } + return; + } await this.credentialsRepository.remove(credential); } @@ -972,7 +1132,8 @@ export class CredentialsService { async testById(userId: User['id'], credentialId: string) { const storedCredential = await this.credentialsFinderService.findCredentialById(credentialId); - if (!storedCredential) { + // Dynamic-credential flows only; admins test instance credentials via testWithCredentials + if (!storedCredential || storedCredential.usageScope !== 'project') { throw new CredentialNotFoundError(credentialId); } @@ -985,9 +1146,14 @@ export class CredentialsService { credentials.id, user, ['credential:read'], + { includeInstanceCredentials: true }, ); if (!storedCredential) { + if (credentials.id === '' && hasGlobalScope(user, 'credential:manageInstance')) { + this.validateInstanceCredentialData(credentials.data ?? {}); + return await this.test(user.id, credentials); + } throw new CredentialNotFoundError(credentials.id); } @@ -1217,7 +1383,32 @@ export class CredentialsService { return mergedData; } - async getOne(user: User, credentialId: string, includeDecryptedData: boolean) { + async getOne( + user: User, + credentialId: string, + includeDecryptedData: boolean, + options: { includeInstanceCredentials?: boolean } = {}, + ) { + if (options.includeInstanceCredentials && hasGlobalScope(user, 'credential:manageInstance')) { + const instanceCredential = await this.credentialsRepository.findOneBy({ + id: credentialId, + usageScope: 'instance', + }); + if (instanceCredential) { + const { data: _, ...rest } = instanceCredential; + if (includeDecryptedData) { + const decryptedData = await this.decrypt(instanceCredential); + // We never want to expose the oauthTokenData to the frontend, but it + // expects it to check if the credential is already connected. + if (decryptedData.oauthTokenData) { + decryptedData.oauthTokenData = true; + } + return { data: decryptedData, ...rest }; + } + return { ...rest }; + } + } + let sharing: SharedCredentials | null = null; let decryptedData: ICredentialDataDecryptedObject | null = null; @@ -1373,6 +1564,18 @@ export class CredentialsService { return await this.createCredential({ ...dto, isManaged: false }, user); } + async createInstanceCredential( + dto: CreateCredentialDto, + user: User, + ctx: OperationContext, + options: InstanceCredentialWriteOptions = {}, + ): Promise { + if (dto.usageScope !== 'instance') { + throw new BadRequestError('Provider connections must use instance usage scope'); + } + return await this.persistInstanceCredential({ ...dto, isManaged: false }, user, ctx, options); + } + /** * Creates an empty credential placeholder for package import. Skips field * validation so every known type can be stubbed; {@link save} still enforces @@ -1409,6 +1612,11 @@ export class CredentialsService { user: User, projectId: string, ): Promise { + this.validateCredentialData(type, data); + await validateExternalSecretsPermissions({ user, projectId, dataToSave: data }); + } + + private validateCredentialData(type: string, data: ICredentialDataDecryptedObject): void { // check mandatory fields are present const credentialProperties = this.credentialsHelper.getCredentialsProperties(type); for (const property of credentialProperties) { @@ -1433,7 +1641,6 @@ export class CredentialsService { } } } - await validateExternalSecretsPermissions({ user, projectId, dataToSave: data }); this.validateOAuthCredentialUrls(type, data); } @@ -1507,6 +1714,13 @@ export class CredentialsService { } private async createCredential(opts: CreateCredentialOptions, user: User) { + if (opts.usageScope === 'instance') { + const savedCredential = await this.persistInstanceCredential(opts, user, {}); + const scopes = await this.getCredentialScopes(user, savedCredential.id); + const { shared, ...credential } = savedCredential; + return { ...credential, scopes }; + } + const targetProjectId = await this.resolveOwningProjectIdForNewCredential(user, opts.projectId); if (opts.isResolvable === true) { @@ -1565,6 +1779,127 @@ export class CredentialsService { return { ...credential, scopes }; } + private async persistInstanceCredential( + opts: CreateCredentialOptions, + user: User, + ctx: OperationContext, + options: InstanceCredentialWriteOptions = {}, + ): Promise { + if (!hasGlobalScope(user, 'credential:manageInstance')) { + throw new ForbiddenError('You do not have permission to create provider connections'); + } + if (opts.isGlobal === true) { + throw new BadRequestError('Provider connections cannot be globally shared'); + } + if (opts.isResolvable === true || opts.isManaged) { + throw new BadRequestError('Provider connections cannot be end-user or managed credentials'); + } + const data = opts.data as ICredentialDataDecryptedObject; + this.validateInstanceCredentialData(data); + + this.validateCredentialData(opts.type, data); + + const encryptedCredential = + options.encryptedData ?? + (await this.createEncryptedData({ + id: null, + name: opts.name, + type: opts.type, + data, + })); + if (!options.skipExternalHooks) { + await this.externalHooks.run('credentials.create', [encryptedCredential]); + } + const hookedData = await this.getValidatedInstanceCredentialHookData( + encryptedCredential, + null, + opts.type, + ); + this.validateInstanceCredentialData(hookedData); + this.validateCredentialData(opts.type, hookedData); + const credentialEntity = this.credentialsRepository.create({ + ...encryptedCredential, + isManaged: false, + isResolvable: false, + usageScope: 'instance' as const, + }); + + const savedCredential = await this.credentialsRepository.saveInstanceCredential( + credentialEntity, + ctx, + ); + + this.logger.debug('New instance credential created', { + credentialId: savedCredential.id, + ownerId: user.id, + }); + + return savedCredential; + } + + private async getValidatedInstanceCredentialHookData( + credential: InstanceCredentialHookPayload, + expectedId: string | null, + expectedType: string, + ): Promise { + if ((credential.id ?? null) !== expectedId) { + throw new BadRequestError('Provider connection hooks cannot change the credential ID'); + } + if (credential.type !== expectedType) { + throw new BadRequestError('Provider connection hooks cannot change the credential type'); + } + if ( + credential.isGlobal || + credential.isManaged || + credential.isResolvable || + credential.resolvableAllowFallback || + (credential.resolverId !== undefined && credential.resolverId !== null) || + (credential.usageScope !== undefined && credential.usageScope !== 'instance') + ) { + throw new BadRequestError('Provider connection hooks cannot change the credential scope'); + } + + try { + return await createCredentialsFromCredentialsEntity(credential).getData(); + } catch (error) { + if (error instanceof CredentialDataError) { + throw new BadRequestError('Provider connection hooks returned invalid credential data'); + } + throw error; + } + } + + validateInstanceCredentialData(data: ICredentialDataDecryptedObject): void { + if (getExternalSecretExpressionPaths(data).length > 0) { + throw new BadRequestError( + 'Provider connections cannot reference project-scoped external secrets', + ); + } + } + + /** An assigned instance credential must keep satisfying every use's policy. */ + async validateInstanceCredentialUpdate( + credential: Pick, + data: ICredentialDataDecryptedObject, + credentialUseIds?: string[], + ctx: OperationContext = {}, + ): Promise { + this.validateInstanceCredentialData(data); + const assignedUseIds = + credentialUseIds ?? + (await this.instanceCredentialAssignmentRepository.findCredentialUseIds(credential.id, ctx)); + + for (const credentialUseId of assignedUseIds) { + const credentialUse = this.instanceCredentialUseRegistry.get(credentialUseId); + if (!credentialUse.credentialTypes.includes(credential.type)) { + throw new BadRequestError( + `Credential type "${credential.type}" is not valid for instance credential use "${credentialUseId}"`, + ); + } + credentialUse.validate?.({ type: credential.type, data }); + } + } + /** * Build credentials payload ready to pass to credential testing. * diff --git a/packages/cli/src/credentials/instance-credential-broker.ts b/packages/cli/src/credentials/instance-credential-broker.ts new file mode 100644 index 00000000000..cb4c03a6105 --- /dev/null +++ b/packages/cli/src/credentials/instance-credential-broker.ts @@ -0,0 +1,108 @@ +import { Logger } from '@n8n/backend-common'; +import { + CredentialsEntity, + InstanceCredentialAssignmentRepository, + type OperationContext, +} from '@n8n/db'; +import { Service } from '@n8n/di'; +import type { ICredentialDataDecryptedObject } from 'n8n-workflow'; + +import { UnprocessableRequestError } from '@/errors/response-errors/unprocessable.error'; + +import { CredentialsService } from './credentials.service'; +import { + InstanceCredentialUseRegistry, + type InstanceCredentialUse, +} from './instance-credential-use.registry'; + +export type { InstanceCredentialUse } from './instance-credential-use.registry'; + +type InstanceCredentialSummary = Pick; + +export interface ResolvedInstanceCredential extends InstanceCredentialSummary { + data: ICredentialDataDecryptedObject; +} + +@Service() +export class InstanceCredentialBroker { + constructor( + private readonly logger: Logger, + private readonly useRegistry: InstanceCredentialUseRegistry, + private readonly assignmentRepository: InstanceCredentialAssignmentRepository, + private readonly credentialsService: CredentialsService, + ) {} + + registerUse(credentialUse: InstanceCredentialUse): void { + this.useRegistry.register(credentialUse); + } + + async listForUse(credentialUse: InstanceCredentialUse): Promise { + const registeredUse = this.useRegistry.get(credentialUse.id); + return await this.assignmentRepository.findAvailableCredentials(registeredUse.credentialTypes); + } + + async assignForUse( + credentialUse: InstanceCredentialUse, + credentialId: string, + ctx?: OperationContext, + ): Promise { + const registeredUse = this.useRegistry.get(credentialUse.id); + const credentialUseId = registeredUse.id; + const credential = await this.assignmentRepository.assignCredential( + credentialUseId, + credentialId, + registeredUse.credentialTypes, + ctx, + ); + if (!credential) throw this.invalidCredentialError(credentialUseId, credentialId); + this.logger.debug('Assigned instance credential', { credentialUseId, credentialId }); + return { id: credential.id, name: credential.name, type: credential.type }; + } + + async clearForUse(credentialUse: InstanceCredentialUse, ctx?: OperationContext): Promise { + this.useRegistry.get(credentialUse.id); + await this.assignmentRepository.clearCredential(credentialUse.id, ctx); + } + + async getAssignedCredentialId( + credentialUse: InstanceCredentialUse, + ctx?: OperationContext, + ): Promise { + this.useRegistry.get(credentialUse.id); + return await this.assignmentRepository.findAssignedCredentialId(credentialUse.id, ctx); + } + + async resolveForUse( + credentialUse: InstanceCredentialUse, + ctx?: OperationContext, + ): Promise { + const registeredUse = this.useRegistry.get(credentialUse.id); + const credentialUseId = registeredUse.id; + const assignment = await this.assignmentRepository.findAssignedCredential( + credentialUseId, + registeredUse.credentialTypes, + ctx, + ); + if (!assignment) return null; + const { credentialId, credential } = assignment; + if (!credential) throw this.invalidCredentialError(credentialUseId, credentialId); + + const resolved = { + id: credential.id, + name: credential.name, + type: credential.type, + data: await this.credentialsService.decrypt(credential, true), + }; + this.logger.debug('Resolved instance credential', { + credentialUseId, + credentialId: credential.id, + }); + return resolved; + } + + private invalidCredentialError(credentialUseId: string, credentialId: string) { + return new UnprocessableRequestError( + `Credential "${credentialId}" is not valid for instance credential use "${credentialUseId}"`, + ); + } +} diff --git a/packages/cli/src/credentials/instance-credential-use.registry.ts b/packages/cli/src/credentials/instance-credential-use.registry.ts new file mode 100644 index 00000000000..95299f4afdb --- /dev/null +++ b/packages/cli/src/credentials/instance-credential-use.registry.ts @@ -0,0 +1,46 @@ +import { Service } from '@n8n/di'; +import { UnexpectedError, type ICredentialDataDecryptedObject } from 'n8n-workflow'; + +export interface InstanceCredentialUse { + readonly id: string; + readonly credentialTypes: readonly string[]; + readonly validate?: (credential: { + type: string; + data: ICredentialDataDecryptedObject; + }) => void; +} + +@Service() +export class InstanceCredentialUseRegistry { + // Use policies are code-defined; mutable assignments are persisted in the database. + private readonly uses = new Map(); + + register(credentialUse: InstanceCredentialUse) { + if (credentialUse.id.trim().length === 0) { + throw new UnexpectedError('Instance credential use ID cannot be empty'); + } + if (credentialUse.id !== credentialUse.id.trim()) { + throw new UnexpectedError('Instance credential use ID cannot contain surrounding whitespace'); + } + if (this.uses.has(credentialUse.id)) { + throw new UnexpectedError( + `Instance credential use "${credentialUse.id}" is already registered`, + ); + } + if (credentialUse.credentialTypes.length === 0) { + throw new UnexpectedError( + `Instance credential use "${credentialUse.id}" must allow at least one credential type`, + ); + } + + this.uses.set(credentialUse.id, credentialUse); + } + + get(credentialUseId: string): InstanceCredentialUse { + const credentialUse = this.uses.get(credentialUseId); + if (!credentialUse) { + throw new UnexpectedError(`Unknown instance credential use "${credentialUseId}"`); + } + return credentialUse; + } +} diff --git a/packages/cli/src/credentials/instance-credentials.md b/packages/cli/src/credentials/instance-credentials.md new file mode 100644 index 00000000000..69b291545cb --- /dev/null +++ b/packages/cli/src/credentials/instance-credentials.md @@ -0,0 +1,63 @@ +# Instance credentials + +Instance credentials are credential rows with `usageScope: 'instance'`. Owners and +admins manage them through the global `credential:manageInstance` scope. They have no +`SharedCredentials` owner row and workflows cannot use them. + +`InstanceCredentialBroker` is the only feature-facing API for using them. A +**credential use** is a stable feature-purpose policy (for example `instance-ai:model`) +that identifies trusted backend code, not a user. Project credentials and disallowed +credential types fail closed. + +## Integrating a feature + +1. **Define each credential use** as an exported const the feature passes to every + broker call, so the feature uses one typed policy object consistently: + + ```typescript + export const MY_FEATURE_CREDENTIAL_USE: InstanceCredentialUse = { + id: 'my-feature:purpose', + credentialTypes: ['someApi'], + }; + ``` + +2. **Register every use during module initialization**, before loading settings or + making broker calls: + + ```typescript + broker.registerUse(MY_FEATURE_CREDENTIAL_USE); + ``` + + Unregistered use IDs fail closed. The broker always enforces the registered + credential types, even if a caller passes a different policy object with the same ID. + +3. **List matching credentials.** Only `id`, `name`, and `type` are returned: + + ```typescript + await broker.listForUse(MY_FEATURE_CREDENTIAL_USE); + ``` + +4. **Assign or clear the credential through the broker.** The broker validates the + credential usage scope and type before it writes the + `instance_credential_assignment` row: + + ```typescript + await broker.assignForUse(MY_FEATURE_CREDENTIAL_USE, credentialId); + await broker.clearForUse(MY_FEATURE_CREDENTIAL_USE); + ``` + +5. **Resolve at runtime** with the credential use. The broker reads the assignment, + repeats the policy checks, and decrypts the credential on the server: + + ```typescript + await broker.resolveForUse(MY_FEATURE_CREDENTIAL_USE); + ``` + +Apply your feature's deployment and license gates before broker calls; the broker +enforces credential policy, not feature availability. The assignment foreign key +prevents credential deletion while it has any assignment. Feature settings should +store only feature data. If that data depends on the credential, update it in the +same transaction as the assignment. + +For a complete example, see the `instance-ai:model`, sandbox, and search credential uses +in `packages/cli/src/modules/instance-ai/`. diff --git a/packages/cli/src/databases/repositories/__tests__/credentials.repository.test.ts b/packages/cli/src/databases/repositories/__tests__/credentials.repository.test.ts index 3482cae4b27..8e5eaa57867 100644 --- a/packages/cli/src/databases/repositories/__tests__/credentials.repository.test.ts +++ b/packages/cli/src/databases/repositories/__tests__/credentials.repository.test.ts @@ -64,6 +64,21 @@ describe('CredentialsRepository', () => { }); }); + describe('findStartingWith', () => { + it('only searches project credential names', async () => { + entityManager.find.mockResolvedValueOnce([]); + + await repository.findStartingWith('API key'); + + expect(entityManager.find).toHaveBeenCalledWith( + CredentialsEntity, + expect.objectContaining({ + where: expect.objectContaining({ usageScope: 'project' }), + }), + ); + }); + }); + describe('findAllGlobalCredentials', () => { test('should find all global credentials without data by default', async () => { // ARRANGE @@ -240,6 +255,7 @@ describe('CredentialsRepository', () => { // ASSERT expect(entityManager.findBy).toHaveBeenCalledWith(CredentialsEntity, { + usageScope: 'project', shared: { project: { type: 'personal' } }, }); expect(credentials).toHaveLength(2); @@ -271,6 +287,7 @@ describe('CredentialsRepository', () => { // ASSERT expect(entityManager.findBy).toHaveBeenCalledWith(CredentialsEntity, { + usageScope: 'project', shared: { project: { sharedWorkflows: { workflowId } } }, }); expect(credentials).toHaveLength(2); @@ -303,6 +320,7 @@ describe('CredentialsRepository', () => { // ASSERT expect(entityManager.findBy).toHaveBeenCalledWith(CredentialsEntity, { + usageScope: 'project', shared: { projectId }, }); expect(credentials).toHaveLength(2); diff --git a/packages/cli/src/executions/pre-execution-checks/__tests__/credentials-permission-checker.test.ts b/packages/cli/src/executions/pre-execution-checks/__tests__/credentials-permission-checker.test.ts index 1f046fe69df..e6d65beffda 100644 --- a/packages/cli/src/executions/pre-execution-checks/__tests__/credentials-permission-checker.test.ts +++ b/packages/cli/src/executions/pre-execution-checks/__tests__/credentials-permission-checker.test.ts @@ -61,6 +61,8 @@ describe('CredentialsPermissionChecker', () => { node.credentials!.someCredential.id = credentialId; ownershipService.getWorkflowProjectCached.mockResolvedValueOnce(personalProject); projectService.findProjectsWorkflowIsIn.mockResolvedValueOnce([personalProject.id]); + credentialsRepository.find.mockResolvedValue([]); + credentialsRepository.findNonProjectCredentialsByIds.mockResolvedValue([]); }); it('should throw if a node has a credential without an id', async () => { @@ -70,7 +72,6 @@ describe('CredentialsPermissionChecker', () => { 'Node "Test Node" uses invalid credential', ); - expect(projectService.findProjectsWorkflowIsIn).toHaveBeenCalledWith(workflowId); expect(sharedCredentialsRepository.getFilteredAccessibleCredentials).not.toHaveBeenCalled(); }); @@ -109,7 +110,6 @@ describe('CredentialsPermissionChecker', () => { it('should not throw an error if the workflow has no credentials', async () => { await expect(permissionChecker.check(workflowId, [])).resolves.not.toThrow(); - expect(projectService.findProjectsWorkflowIsIn).toHaveBeenCalledWith(workflowId); expect(sharedCredentialsRepository.getFilteredAccessibleCredentials).not.toHaveBeenCalled(); }); @@ -139,6 +139,42 @@ describe('CredentialsPermissionChecker', () => { expect(sharedCredentialsRepository.getFilteredAccessibleCredentials).not.toHaveBeenCalled(); }); + it('should reject instance credentials even when the home project owner has global scope', async () => { + const projectOwner = mock({ role: GLOBAL_OWNER_ROLE }); + ownershipService.getPersonalProjectOwnerCached.mockResolvedValue(projectOwner); + const instanceCredential = mock({ + id: credentialId, + usageScope: 'instance', + }); + credentialsRepository.findNonProjectCredentialsByIds.mockResolvedValueOnce([ + instanceCredential, + ]); + + await expect(permissionChecker.check(workflowId, [node])).rejects.toThrow( + 'Node "Test Node" does not have access to the credential', + ); + + expect(credentialsRepository.findNonProjectCredentialsByIds).toHaveBeenCalledWith([ + credentialId, + ]); + expect(sharedCredentialsRepository.getFilteredAccessibleCredentials).not.toHaveBeenCalled(); + }); + + it('should reject instance credentials for member workflows', async () => { + ownershipService.getPersonalProjectOwnerCached.mockResolvedValueOnce(null); + const instanceCredential = mock({ + id: credentialId, + usageScope: 'instance', + }); + credentialsRepository.findNonProjectCredentialsByIds.mockResolvedValueOnce([ + instanceCredential, + ]); + + await expect(permissionChecker.check(workflowId, [node])).rejects.toThrow( + 'Node "Test Node" does not have access to the credential', + ); + }); + it('should allow global credentials for any project', async () => { ownershipService.getPersonalProjectOwnerCached.mockResolvedValueOnce(null); sharedCredentialsRepository.getFilteredAccessibleCredentials.mockResolvedValueOnce([]); @@ -159,6 +195,7 @@ describe('CredentialsPermissionChecker', () => { select: ['id'], where: { isGlobal: true, + usageScope: 'project', }, }); }); @@ -175,11 +212,12 @@ describe('CredentialsPermissionChecker', () => { projectService.findProjectsWorkflowIsIn.mockResolvedValue([teamProject.id]); ownershipService.getPersonalProjectOwnerCached.mockResolvedValue(null); sharedCredentialsRepository.getFilteredAccessibleCredentials.mockResolvedValue([]); + credentialsRepository.findNonProjectCredentialsByIds.mockResolvedValue([]); const globalCredential = mock({ id: credentialId, isGlobal: true, }); - credentialsRepository.find.mockResolvedValue([globalCredential]); + credentialsRepository.find.mockResolvedValueOnce([globalCredential]); await expect(permissionChecker.check(workflowId, [node])).resolves.not.toThrow(); @@ -192,6 +230,7 @@ describe('CredentialsPermissionChecker', () => { select: ['id'], where: { isGlobal: true, + usageScope: 'project', }, }); }); @@ -233,6 +272,7 @@ describe('CredentialsPermissionChecker', () => { ownershipService.getWorkflowProjectCached.mockResolvedValue(teamProject); ownershipService.getPersonalProjectOwnerCached.mockResolvedValue(null); projectService.findProjectsWorkflowIsIn.mockResolvedValue([teamProject.id]); + credentialsRepository.findNonProjectCredentialsByIds.mockResolvedValue([]); }); it('should only check the active credential type for nodes with nodeCredentialType', async () => { @@ -456,5 +496,17 @@ describe('CredentialsPermissionChecker', () => { await expect(permissionChecker.checkForUser(userId, [node])).resolves.not.toThrow(); expect(credentialsFinderService.findCredentialsForUser).not.toHaveBeenCalled(); }); + + it('should reject instance credentials before the global-scope shortcut', async () => { + userRepository.findOne.mockResolvedValueOnce(mock({ role: GLOBAL_OWNER_ROLE })); + credentialsRepository.findNonProjectCredentialsByIds.mockResolvedValueOnce([ + mock({ id: credentialId, usageScope: 'instance' }), + ]); + + await expect(permissionChecker.checkForUser(userId, [node])).rejects.toThrow( + 'Node "Test Node" uses a credential you do not have access to', + ); + expect(credentialsFinderService.findCredentialsForUser).not.toHaveBeenCalled(); + }); }); }); diff --git a/packages/cli/src/executions/pre-execution-checks/credentials-permission-checker.ts b/packages/cli/src/executions/pre-execution-checks/credentials-permission-checker.ts index 152352887fd..72aa2f88c31 100644 --- a/packages/cli/src/executions/pre-execution-checks/credentials-permission-checker.ts +++ b/packages/cli/src/executions/pre-execution-checks/credentials-permission-checker.ts @@ -75,6 +75,11 @@ export class CredentialsPermissionChecker { // Cannot resolve the triggering user - fail closed. throw new InaccessibleCredentialForUserError(credIdsToNodes[workflowCredIds[0]][0]); } + const unavailableCredentials = + await this.credentialsRepository.findNonProjectCredentialsByIds(workflowCredIds); + if (unavailableCredentials.length > 0) { + throw new InaccessibleCredentialForUserError(credIdsToNodes[unavailableCredentials[0].id][0]); + } // A user with instance-wide credential listing can use any credential. if (hasGlobalScope(user, 'credential:list')) return; @@ -96,6 +101,14 @@ export class CredentialsPermissionChecker { */ async check(workflowId: string, nodes: INode[]) { const homeProject = await this.ownershipService.getWorkflowProjectCached(workflowId); + const credIdsToNodes = this.mapCredIdsToNodes(nodes); + + const workflowCredIds = Object.keys(credIdsToNodes); + + if (workflowCredIds.length === 0) return; + + await this.ensureOnlyWorkflowCredentials(workflowCredIds, credIdsToNodes, homeProject); + const homeProjectOwner = await this.ownershipService.getPersonalProjectOwnerCached( homeProject.id, ); @@ -109,11 +122,6 @@ export class CredentialsPermissionChecker { return; } const projectIds = await this.projectService.findProjectsWorkflowIsIn(workflowId); - const credIdsToNodes = this.mapCredIdsToNodes(nodes); - - const workflowCredIds = Object.keys(credIdsToNodes); - - if (workflowCredIds.length === 0) return; const accessible = await this.sharedCredentialsRepository.getFilteredAccessibleCredentials( projectIds, @@ -130,6 +138,19 @@ export class CredentialsPermissionChecker { } } + private async ensureOnlyWorkflowCredentials( + workflowCredIds: string[], + credIdsToNodes: { [credentialId: string]: INode[] }, + homeProject: Project, + ) { + const unavailableCredentials = + await this.credentialsRepository.findNonProjectCredentialsByIds(workflowCredIds); + if (unavailableCredentials.length > 0) { + const nodeToFlag = credIdsToNodes[unavailableCredentials[0].id][0]; + throw new InaccessibleCredentialError(nodeToFlag, homeProject); + } + } + /** * Adds global credentials (isGlobal: true) to the set of accessible credentials. */ @@ -138,7 +159,7 @@ export class CredentialsPermissionChecker { ): Promise> { const accessibleSet = new Set(accessibleCredentialIds); const globalCredentials = await this.credentialsRepository.find({ - where: { isGlobal: true }, + where: { isGlobal: true, usageScope: 'project' }, select: ['id'], }); for (const globalCred of globalCredentials) { diff --git a/packages/cli/src/modules/instance-ai/__tests__/instance-ai-settings.service.test.ts b/packages/cli/src/modules/instance-ai/__tests__/instance-ai-settings.service.test.ts index 0f6826ed36b..38354aa8f3a 100644 --- a/packages/cli/src/modules/instance-ai/__tests__/instance-ai-settings.service.test.ts +++ b/packages/cli/src/modules/instance-ai/__tests__/instance-ai-settings.service.test.ts @@ -1,6 +1,12 @@ import { Logger } from '@n8n/backend-common'; import type { InstanceAiConfig } from '@n8n/config'; -import type { SettingsRepository, User, UserRepository } from '@n8n/db'; +import type { + CredentialsEntity, + DbLockService, + SettingsRepository, + User, + UserRepository, +} from '@n8n/db'; import { Container } from '@n8n/di'; import { mock } from 'vitest-mock-extended'; @@ -10,9 +16,14 @@ import type { AiService } from '@/services/ai.service'; import type { UserService } from '@/services/user.service'; import type { CredentialsFinderService } from '@/credentials/credentials-finder.service'; import type { CredentialsService } from '@/credentials/credentials.service'; +import type { InstanceCredentialBroker } from '@/credentials/instance-credential-broker'; import { InstanceAiSettingsService } from '../instance-ai-settings.service'; +type CredentialOperationContext = NonNullable< + Parameters[1] +>; + describe('InstanceAiSettingsService', () => { const globalConfig = mock<{ instanceAi: InstanceAiConfig; @@ -35,18 +46,21 @@ describe('InstanceAiSettingsService', () => { } as unknown as InstanceAiConfig, deployment: { type: 'default' }, }); + const operationContext = mock(); + const dbLockService = mock(); const settingsRepository = mock(); const userRepository = mock(); const userService = mock(); const aiService = mock(); const credentialsService = mock(); const credentialsFinderService = mock(); + const instanceCredentialBroker = mock(); const eventService = mock(); let service: InstanceAiSettingsService; beforeEach(() => { - vi.clearAllMocks(); + vi.resetAllMocks(); Object.assign(globalConfig.instanceAi, { sandboxEnabled: false, sandboxProvider: 'n8n-sandbox', @@ -56,14 +70,27 @@ describe('InstanceAiSettingsService', () => { browserMcp: false, }); globalConfig.deployment.type = 'default'; + instanceCredentialBroker.listForUse.mockResolvedValue([]); + instanceCredentialBroker.getAssignedCredentialId.mockResolvedValue(null); + settingsRepository.findByKeyInContext.mockImplementation( + async (key) => await settingsRepository.findByKey(key), + ); + settingsRepository.upsertByKey.mockImplementation(async (key, value, loadOnStartup) => { + await settingsRepository.upsert({ key, value, loadOnStartup }, ['key']); + }); + dbLockService.withLockContext.mockImplementation(async (_lockId, fn) => { + return await fn(operationContext); + }); service = new InstanceAiSettingsService( globalConfig as never, + dbLockService, settingsRepository, userRepository, userService, aiService, credentialsService, credentialsFinderService, + instanceCredentialBroker, eventService, ); }); @@ -157,8 +184,8 @@ describe('InstanceAiSettingsService', () => { settingsRepository.upsert.mockResolvedValue(undefined as never); }); - it('defaults to true', () => { - expect(service.getAdminSettings().mcpAccessEnabled).toBe(true); + it('defaults to true', async () => { + expect((await service.getAdminSettings()).mcpAccessEnabled).toBe(true); expect(service.isMcpAccessEnabled()).toBe(true); }); @@ -187,7 +214,7 @@ describe('InstanceAiSettingsService', () => { const result = await service.updateAdminSettings({ mcpAccessEnabled: false }); expect(result.mcpAccessEnabled).toBe(false); - expect(service.getAdminSettings().mcpAccessEnabled).toBe(false); + expect((await service.getAdminSettings()).mcpAccessEnabled).toBe(false); expect(settingsRepository.upsert).toHaveBeenCalledWith( expect.objectContaining({ value: expect.stringContaining('"mcpAccessEnabled":false'), @@ -195,6 +222,135 @@ describe('InstanceAiSettingsService', () => { ['key'], ); }); + + it('merges an update with the latest persisted settings', async () => { + settingsRepository.findByKey.mockResolvedValue({ + key: 'instanceAi.settings', + value: JSON.stringify({ mcpAccessEnabled: false }), + loadOnStartup: true, + } as never); + + await service.updateAdminSettings({ permissions: { createWorkflow: 'always_allow' } }); + + expect(settingsRepository.upsert).toHaveBeenCalledWith( + expect.objectContaining({ + value: expect.stringContaining('"mcpAccessEnabled":false'), + }), + ['key'], + ); + expect(settingsRepository.upsert).toHaveBeenCalledWith( + expect.objectContaining({ + value: expect.stringContaining('"createWorkflow":"always_allow"'), + }), + ['key'], + ); + }); + + it('does not apply an update when persistence fails', async () => { + settingsRepository.upsert.mockRejectedValue(new Error('write failed')); + + await expect(service.updateAdminSettings({ mcpAccessEnabled: false })).rejects.toThrow( + 'write failed', + ); + + expect((await service.getAdminSettings()).mcpAccessEnabled).toBe(true); + }); + }); + + describe('instance model credential', () => { + beforeEach(() => { + aiService.isProxyEnabled.mockReturnValue(false); + settingsRepository.upsert.mockResolvedValue(undefined as never); + }); + + it('delegates model credential validation to the broker', async () => { + instanceCredentialBroker.assignForUse.mockRejectedValue( + new UnprocessableRequestError('Invalid instance credential'), + ); + + await expect(service.updateAdminSettings({ modelCredentialId: 'cred-1' })).rejects.toThrow( + 'Invalid instance credential', + ); + expect(instanceCredentialBroker.assignForUse).toHaveBeenCalledWith( + expect.objectContaining({ id: 'instance-ai:model' }), + 'cred-1', + operationContext, + ); + }); + + it('uses the admin credential before per-user credentials', async () => { + const credential = mock({ + id: 'cred-1', + name: 'Admin model', + type: 'openAiApi', + usageScope: 'instance', + }); + instanceCredentialBroker.resolveForUse.mockResolvedValue({ + id: credential.id, + name: credential.name, + type: credential.type, + data: { apiKey: 'admin-key' }, + }); + instanceCredentialBroker.assignForUse.mockResolvedValue({ + id: credential.id, + name: credential.name, + type: credential.type, + }); + + await service.updateAdminSettings({ modelCredentialId: credential.id }); + const result = await service.resolveModelConfig( + mock({ + settings: { + instanceAi: { credentialId: 'user-credential', modelName: 'gpt-4.1' }, + }, + }), + ); + + expect(result).toEqual({ id: 'openai/gpt-4.1', url: '', apiKey: 'admin-key' }); + expect(instanceCredentialBroker.resolveForUse).toHaveBeenCalledWith( + expect.objectContaining({ id: 'instance-ai:model' }), + ); + expect(credentialsFinderService.findCredentialForUser).not.toHaveBeenCalled(); + expect(settingsRepository.upsert).toHaveBeenCalledWith( + expect.objectContaining({ value: expect.not.stringContaining('modelCredentialId') }), + ['key'], + ); + }); + + it('reads the configured model credential from the broker', async () => { + instanceCredentialBroker.getAssignedCredentialId.mockResolvedValue('cred-1'); + + await expect(service.getAdminSettings()).resolves.toMatchObject({ + modelCredentialId: 'cred-1', + }); + }); + + it('ignores a configured admin credential on cloud', async () => { + const credential = mock({ + id: 'cred-1', + name: 'Admin model', + type: 'openAiApi', + usageScope: 'instance', + }); + instanceCredentialBroker.resolveForUse.mockResolvedValue({ + id: credential.id, + name: credential.name, + type: credential.type, + data: { apiKey: 'admin-key' }, + }); + instanceCredentialBroker.assignForUse.mockResolvedValue({ + id: credential.id, + name: credential.name, + type: credential.type, + }); + await service.updateAdminSettings({ modelCredentialId: credential.id }); + vi.clearAllMocks(); + globalConfig.deployment.type = 'cloud'; + + await expect(service.resolveModelConfig(mock())).resolves.toBe('openai/gpt-4'); + expect(instanceCredentialBroker.resolveForUse).not.toHaveBeenCalled(); + await expect(service.listInstanceModelCredentials()).resolves.toEqual([]); + }); }); describe('executeMcpTool permission', () => { @@ -203,8 +359,10 @@ describe('InstanceAiSettingsService', () => { settingsRepository.upsert.mockResolvedValue(undefined as never); }); - it('defaults to require_approval', () => { - expect(service.getAdminSettings().permissions.executeMcpTool).toBe('require_approval'); + it('defaults to require_approval', async () => { + expect((await service.getAdminSettings()).permissions.executeMcpTool).toBe( + 'require_approval', + ); }); it('persists and reflects an update', async () => { @@ -213,7 +371,7 @@ describe('InstanceAiSettingsService', () => { }); expect(result.permissions.executeMcpTool).toBe('always_allow'); - expect(service.getAdminSettings().permissions.executeMcpTool).toBe('always_allow'); + expect((await service.getAdminSettings()).permissions.executeMcpTool).toBe('always_allow'); expect(settingsRepository.upsert).toHaveBeenCalledWith( expect.objectContaining({ value: expect.stringContaining('"executeMcpTool":"always_allow"'), @@ -325,6 +483,12 @@ describe('InstanceAiSettingsService', () => { }); describe('updateAdminSettings', () => { + it('should reject model credentials on cloud', async () => { + await expect(service.updateAdminSettings({ modelCredentialId: 'cred-1' })).rejects.toThrow( + UnprocessableRequestError, + ); + }); + it('should reject advanced fields on cloud', async () => { await expect(service.updateAdminSettings({ mcpServers: '[]' })).rejects.toThrow( UnprocessableRequestError, diff --git a/packages/cli/src/modules/instance-ai/__tests__/instance-ai.controller.test.ts b/packages/cli/src/modules/instance-ai/__tests__/instance-ai.controller.test.ts index 50c7b20e7c8..89dd6f25256 100644 --- a/packages/cli/src/modules/instance-ai/__tests__/instance-ai.controller.test.ts +++ b/packages/cli/src/modules/instance-ai/__tests__/instance-ai.controller.test.ts @@ -59,6 +59,7 @@ import { ForbiddenError } from '@/errors/response-errors/forbidden.error'; import { NotFoundError } from '@/errors/response-errors/not-found.error'; import type { CredentialsService } from '@/credentials/credentials.service'; import type { Push } from '@/push'; +import type { Publisher } from '@/scaling/pubsub/publisher.service'; import type { ProjectService } from '@/services/project.service.ee'; import type { UrlService } from '@/services/url.service'; @@ -102,6 +103,7 @@ describe('InstanceAiController', () => { const durableLogMetrics = mock(); const moduleRegistry = mock(); const push = mock(); + const publisher = mock(); const urlService = mock(); const globalConfig = mock({ instanceAi: { gatewayApiKey: 'static-key', durableLog: false }, @@ -137,6 +139,7 @@ describe('InstanceAiController', () => { credentialsService, projectService, instanceAiErrorReporter, + publisher, globalConfig, ); @@ -923,7 +926,7 @@ describe('InstanceAiController', () => { }); it('should disconnect all gateways when enabled is set to false', async () => { - settingsService.updateAdminSettings.mockResolvedValue({} as never); + settingsService.updateAdminSettings.mockResolvedValue({ enabled: false } as never); gatewayService.disconnectAllGateways.mockReturnValue(['user-a', 'user-b']); const payload = { enabled: false } as InstanceAiAdminSettingsUpdateRequest; @@ -940,7 +943,10 @@ describe('InstanceAiController', () => { }); it('should disconnect all gateways when localGatewayDisabled is set to true', async () => { - settingsService.updateAdminSettings.mockResolvedValue({} as never); + settingsService.updateAdminSettings.mockResolvedValue({ + enabled: true, + localGatewayDisabled: true, + } as never); gatewayService.disconnectAllGateways.mockReturnValue(['user-c']); const payload = { localGatewayDisabled: true } as InstanceAiAdminSettingsUpdateRequest; @@ -956,7 +962,10 @@ describe('InstanceAiController', () => { }); it('should not disconnect gateways when enabling features', async () => { - settingsService.updateAdminSettings.mockResolvedValue({} as never); + settingsService.updateAdminSettings.mockResolvedValue({ + enabled: true, + localGatewayDisabled: false, + } as never); const payload = { enabled: true, localGatewayDisabled: false, @@ -966,6 +975,25 @@ describe('InstanceAiController', () => { expect(gatewayService.disconnectAllGateways).not.toHaveBeenCalled(); }); + + it('should publish settings reloads to other mains', async () => { + settingsService.updateAdminSettings.mockResolvedValue({ enabled: true } as never); + + await controller.updateAdminSettings(req, res, { enabled: true }); + + expect(publisher.publishCommand).toHaveBeenCalledWith({ + command: 'reload-instance-ai-settings', + }); + }); + + it('should wait for the settings reload to be published', async () => { + settingsService.updateAdminSettings.mockResolvedValue({ enabled: true } as never); + publisher.publishCommand.mockRejectedValue(new Error('publish failed')); + + await expect(controller.updateAdminSettings(req, res, { enabled: true })).rejects.toThrow( + 'publish failed', + ); + }); }); describe('getUserPreferences', () => { @@ -1030,6 +1058,15 @@ describe('InstanceAiController', () => { }); }); + describe('listInstanceModelCredentials', () => { + it('should require instanceAi:manage scope', () => { + expect(scopeOf('listInstanceModelCredentials')).toEqual({ + scope: 'instanceAi:manage', + globalOnly: true, + }); + }); + }); + describe('listThreads', () => { it('should require instanceAi:message scope', () => { expect(scopeOf('listThreads')).toEqual({ scope: 'instanceAi:message', globalOnly: true }); @@ -1697,6 +1734,7 @@ describe('InstanceAiController — durable-log SSE replay (flag on)', () => { mock(), mock(), mock(), + mock(), globalConfig, ); diff --git a/packages/cli/src/modules/instance-ai/instance-ai-settings.service.ts b/packages/cli/src/modules/instance-ai/instance-ai-settings.service.ts index e2c88663bce..c3411296bd3 100644 --- a/packages/cli/src/modules/instance-ai/instance-ai-settings.service.ts +++ b/packages/cli/src/modules/instance-ai/instance-ai-settings.service.ts @@ -11,15 +11,16 @@ import type { import { Logger } from '@n8n/backend-common'; import { GlobalConfig } from '@n8n/config'; import type { InstanceAiConfig, DeploymentConfig } from '@n8n/config'; -import { SettingsRepository, UserRepository } from '@n8n/db'; -import type { User } from '@n8n/db'; +import { DbLock, DbLockService, SettingsRepository, UserRepository } from '@n8n/db'; +import type { CredentialsEntity, User } from '@n8n/db'; import { Container, Service } from '@n8n/di'; import type { ModelConfig } from '@n8n/instance-ai'; -import type { IUserSettings } from 'n8n-workflow'; +import type { ICredentialDataDecryptedObject, IUserSettings } from 'n8n-workflow'; import { jsonParse } from 'n8n-workflow'; import { CredentialsFinderService } from '@/credentials/credentials-finder.service'; import { CredentialsService } from '@/credentials/credentials.service'; +import { InstanceCredentialBroker } from '@/credentials/instance-credential-broker'; import { UnprocessableRequestError } from '@/errors/response-errors/unprocessable.error'; import { EventService } from '@/events/event.service'; import { AiService } from '@/services/ai.service'; @@ -55,7 +56,10 @@ const CREDENTIAL_TO_MODEL_PROVIDER: Record = { cohereApi: 'cohere', }; -const SUPPORTED_CREDENTIAL_TYPES = Object.keys(CREDENTIAL_TO_MODEL_PROVIDER); +export const INSTANCE_AI_MODEL_CREDENTIAL_POLICY = { + id: 'instance-ai:model', + credentialTypes: Object.keys(CREDENTIAL_TO_MODEL_PROVIDER), +}; /** Fields that contain the base URL per credential type. */ const URL_FIELD_MAP: Record = { @@ -115,12 +119,14 @@ export class InstanceAiSettingsService { constructor( globalConfig: GlobalConfig, + private readonly dbLockService: DbLockService, private readonly settingsRepository: SettingsRepository, private readonly userRepository: UserRepository, private readonly userService: UserService, private readonly aiService: AiService, private readonly credentialsService: CredentialsService, private readonly credentialsFinderService: CredentialsFinderService, + private readonly instanceCredentialBroker: InstanceCredentialBroker, private readonly eventService: EventService, ) { this.config = globalConfig.instanceAi; @@ -145,14 +151,7 @@ export class InstanceAiSettingsService { sandboxProvider: this.config.sandboxProvider, }; - const row = await this.settingsRepository.findByKey(ADMIN_SETTINGS_KEY); - if (row) { - const persisted = jsonParse(row.value, { - fallbackValue: {}, - }); - this.applyAdminSettings(persisted); - } - + await this.reloadFromDb(); // Surface the effective sandbox config so operators (and CI) can tell whether env vars // or a persisted DB setting are in effect — these can silently disagree. const c = this.config; @@ -174,8 +173,14 @@ export class InstanceAiSettingsService { // ── Admin settings ──────────────────────────────────────────────────── - getAdminSettings(): InstanceAiAdminSettingsResponse { + async getAdminSettings(): Promise { const c = this.config; + const modelCredentialId = + this.isCloud || this.aiService.isProxyEnabled() + ? null + : await this.instanceCredentialBroker.getAssignedCredentialId( + INSTANCE_AI_MODEL_CREDENTIAL_POLICY, + ); return { enabled: this.enabled, permissions: { ...this.permissions }, @@ -188,6 +193,7 @@ export class InstanceAiSettingsService { daytonaCredentialId: this.adminDaytonaCredentialId, n8nSandboxCredentialId: this.adminN8nSandboxCredentialId, searchCredentialId: this.adminSearchCredentialId, + modelCredentialId, localGatewayDisabled: this.isLocalGatewayDisabled(), browserUseEnabled: this.isBrowserUseEnabled(), }; @@ -201,37 +207,52 @@ export class InstanceAiSettingsService { InstanceAiSettingsService.MANAGED_ADMIN_FIELDS, this.deploymentLabel(), ); - this.validateAdminSettingsUpdate(update); - const c = this.config; - const previousMcpServers = c.mcpServers; - const previousMcpAccessEnabled = this.mcpAccessEnabled; - if (update.enabled !== undefined) this.enabled = update.enabled; - if (update.permissions) { - this.permissions = { ...this.permissions, ...update.permissions }; + if (this.isCloud || this.aiService.isProxyEnabled()) { + this.rejectManagedFields(update, ['modelCredentialId'], this.deploymentLabel()); } - if (update.mcpServers !== undefined) c.mcpServers = update.mcpServers; - if (update.mcpAccessEnabled !== undefined) this.mcpAccessEnabled = update.mcpAccessEnabled; - if (update.sandboxEnabled !== undefined) c.sandboxEnabled = update.sandboxEnabled; - if (update.sandboxProvider !== undefined) c.sandboxProvider = update.sandboxProvider; - if (update.sandboxImage !== undefined) c.sandboxImage = update.sandboxImage; - if (update.sandboxTimeout !== undefined) c.sandboxTimeout = update.sandboxTimeout; - if (update.daytonaCredentialId !== undefined) - this.adminDaytonaCredentialId = update.daytonaCredentialId; - if (update.n8nSandboxCredentialId !== undefined) - this.adminN8nSandboxCredentialId = update.n8nSandboxCredentialId; - if (update.searchCredentialId !== undefined) - this.adminSearchCredentialId = update.searchCredentialId; - if (update.localGatewayDisabled !== undefined) - c.localGatewayDisabled = update.localGatewayDisabled; - if (update.browserUseEnabled !== undefined) c.browserUseEnabled = update.browserUseEnabled; - await this.persistAdminSettings(); + const { modelCredentialId, ...settingsUpdate } = update; + const { previous, next } = await this.dbLockService.withLockContext( + DbLock.INSTANCE_AI_SETTINGS, + async (ctx) => { + const persisted = await this.settingsRepository.findByKeyInContext(ADMIN_SETTINGS_KEY, ctx); + const current = this.mergeAdminSettings( + this.snapshotAdminSettings(), + this.parsePersistedAdminSettings(persisted?.value), + ); + this.validateAdminSettingsUpdate(settingsUpdate, current); + if (modelCredentialId === null) { + await this.instanceCredentialBroker.clearForUse(INSTANCE_AI_MODEL_CREDENTIAL_POLICY, ctx); + } else if (modelCredentialId !== undefined) { + const credential = await this.instanceCredentialBroker.assignForUse( + INSTANCE_AI_MODEL_CREDENTIAL_POLICY, + modelCredentialId, + ctx, + ); + this.ensureCredentialMatchesConfiguredModel(credential.type); + } - this.eventService.emit('instance-ai-settings-updated', { - mcpSettingsChanged: - c.mcpServers !== previousMcpServers || this.mcpAccessEnabled !== previousMcpAccessEnabled, - }); + const previous = this.snapshotAdminSettings(); + const next = this.mergeAdminSettings(current, settingsUpdate); + await this.settingsRepository.upsertByKey( + ADMIN_SETTINGS_KEY, + JSON.stringify(next), + true, + ctx, + ); + return { previous, next }; + }, + ); + this.applyAdminSettings(next); + this.emitSettingsUpdated(previous, next); - return this.getAdminSettings(); + return await this.getAdminSettings(); + } + + async reloadFromDb(): Promise { + const previous = this.snapshotAdminSettings(); + const persisted = await this.readPersistedAdminSettings(); + this.applyAdminSettings(persisted); + this.emitSettingsUpdated(previous, this.snapshotAdminSettings()); } // ── User preferences ────────────────────────────────────────────────── @@ -290,7 +311,7 @@ export class InstanceAiSettingsService { 'credential:read', ]); return allCredentials - .filter((c) => SUPPORTED_CREDENTIAL_TYPES.includes(c.type)) + .filter((c) => INSTANCE_AI_MODEL_CREDENTIAL_POLICY.credentialTypes.includes(c.type)) .map((c) => ({ id: c.id, name: c.name, @@ -299,6 +320,19 @@ export class InstanceAiSettingsService { })); } + async listInstanceModelCredentials(): Promise { + if (this.isCloud || this.aiService.isProxyEnabled()) return []; + const instanceCredentials = await this.instanceCredentialBroker.listForUse( + INSTANCE_AI_MODEL_CREDENTIAL_POLICY, + ); + return instanceCredentials.map((c) => ({ + id: c.id, + name: c.name, + type: c.type, + provider: CREDENTIAL_TO_MODEL_PROVIDER[c.type] ?? 'custom', + })); + } + /** List credentials the user can access that are usable as sandbox/search services. */ async listServiceCredentials(user: User): Promise { if (this.aiService.isProxyEnabled()) return []; @@ -461,9 +495,15 @@ export class InstanceAiSettingsService { return prefs.modelName ?? this.extractModelName(this.config.model); } - /** Resolve the current model configuration for an agent run. */ async resolveModelConfig(user: User): Promise { const prefs = this.readUserPreferences(user); + const modelName = prefs.modelName ?? this.extractModelName(this.config.model); + + const adminModelConfig = await this.resolveAdminModelConfig(modelName); + if (adminModelConfig) { + return adminModelConfig; + } + const credentialId = prefs.credentialId ?? null; if (!credentialId) { @@ -480,17 +520,57 @@ export class InstanceAiSettingsService { return this.envVarModelConfig(); } - const provider = CREDENTIAL_TO_MODEL_PROVIDER[credential.type]; + return ( + (await this.buildModelConfigFromCredential(credential, modelName)) ?? this.envVarModelConfig() + ); + } + + private async resolveAdminModelConfig(modelName: string): Promise { + if (this.isCloud || this.aiService.isProxyEnabled()) return null; + const resolved = await this.resolveModelCredential(); + if (!resolved) return null; + + return this.buildModelConfig(resolved.type, resolved.data, modelName); + } + + private async resolveModelCredential() { + return await this.instanceCredentialBroker.resolveForUse(INSTANCE_AI_MODEL_CREDENTIAL_POLICY); + } + + private ensureCredentialMatchesConfiguredModel(credentialType: string): void { + const slash = this.config.model.indexOf('/'); + if (slash < 0) return; + const configuredProvider = this.config.model.slice(0, slash); + const credentialProvider = CREDENTIAL_TO_MODEL_PROVIDER[credentialType]; + if (credentialProvider !== configuredProvider) { + throw new UnprocessableRequestError( + `This credential is for "${credentialProvider}" but the configured model "${this.config.model}" requires a "${configuredProvider}" credential. Select a matching credential or set N8N_INSTANCE_AI_MODEL to a "${credentialProvider}" model.`, + ); + } + } + + private async buildModelConfigFromCredential( + credential: CredentialsEntity, + modelName: string, + ): Promise { + const data = await this.credentialsService.decrypt(credential, true); + return this.buildModelConfig(credential.type, data, modelName); + } + + private buildModelConfig( + credentialType: string, + data: ICredentialDataDecryptedObject, + modelName: string, + ): ModelConfig | null { + const provider = CREDENTIAL_TO_MODEL_PROVIDER[credentialType]; if (!provider) { - return this.envVarModelConfig(); + return null; } - const data = await this.credentialsService.decrypt(credential, true); const apiKey = typeof data.apiKey === 'string' ? data.apiKey : ''; - const urlField = URL_FIELD_MAP[credential.type]; + const urlField = URL_FIELD_MAP[credentialType]; const rawUrl = urlField ? data[urlField] : undefined; const baseUrl = typeof rawUrl === 'string' ? rawUrl : ''; - const modelName = prefs.modelName ?? this.extractModelName(this.config.model); const id: `${string}/${string}` = `${provider}/${modelName}`; if (baseUrl) { @@ -551,8 +631,10 @@ export class InstanceAiSettingsService { } } - private validateAdminSettingsUpdate(update: InstanceAiAdminSettingsUpdateRequest): void { - const c = this.config; + private validateAdminSettingsUpdate( + update: InstanceAiAdminSettingsUpdateRequest, + current: PersistedAdminSettings, + ): void { const touchesSandboxSettings = update.sandboxEnabled !== undefined || update.sandboxProvider !== undefined || @@ -567,8 +649,10 @@ export class InstanceAiSettingsService { // `update.sandboxProvider` is already enum-validated by the request DTO; we only // need the resolved provider here to enforce the cross-field service-URL rule, // which spans the request body and env-backed config and can't live in the schema. - const sandboxProvider = update.sandboxProvider ?? normalizeSandboxProvider(c.sandboxProvider); - const sandboxEnabled = update.sandboxEnabled ?? c.sandboxEnabled; + const sandboxProvider = normalizeSandboxProvider( + update.sandboxProvider ?? current.sandboxProvider, + ); + const sandboxEnabled = update.sandboxEnabled ?? current.sandboxEnabled ?? false; const unavailableReason = this.getSandboxUnavailableReason(sandboxEnabled, sandboxProvider); if (unavailableReason) { throw new UnprocessableRequestError(unavailableReason); @@ -649,9 +733,9 @@ export class InstanceAiSettingsService { return user.settings?.instanceAi ?? {}; } - private async persistAdminSettings(): Promise { + private snapshotAdminSettings(): PersistedAdminSettings { const c = this.config; - const value: PersistedAdminSettings = { + return { enabled: this.enabled, permissions: this.permissions, mcpServers: c.mcpServers, @@ -666,10 +750,44 @@ export class InstanceAiSettingsService { localGatewayDisabled: c.localGatewayDisabled, browserUseEnabled: c.browserUseEnabled, }; + } - await this.settingsRepository.upsert( - { key: ADMIN_SETTINGS_KEY, value: JSON.stringify(value), loadOnStartup: true }, - ['key'], + private mergeAdminSettings( + base: PersistedAdminSettings, + update: PersistedAdminSettings, + ): PersistedAdminSettings { + return { + ...base, + ...update, + permissions: update.permissions + ? { ...(base.permissions ?? DEFAULT_INSTANCE_AI_PERMISSIONS), ...update.permissions } + : base.permissions, + }; + } + + private async readPersistedAdminSettings(): Promise { + const row = await this.settingsRepository.findByKey(ADMIN_SETTINGS_KEY); + return this.parsePersistedAdminSettings(row?.value); + } + + private parsePersistedAdminSettings(value: string | undefined): PersistedAdminSettings { + if (value === undefined) return {}; + const settings = jsonParse( + value, + { fallbackValue: {} }, ); + delete settings.modelCredentialId; + return settings; + } + + private emitSettingsUpdated( + previous: PersistedAdminSettings, + current: PersistedAdminSettings, + ): void { + this.eventService.emit('instance-ai-settings-updated', { + mcpSettingsChanged: + current.mcpServers !== previous.mcpServers || + current.mcpAccessEnabled !== previous.mcpAccessEnabled, + }); } } diff --git a/packages/cli/src/modules/instance-ai/instance-ai.controller.ts b/packages/cli/src/modules/instance-ai/instance-ai.controller.ts index cc8c845e79f..fd776bc6994 100644 --- a/packages/cli/src/modules/instance-ai/instance-ai.controller.ts +++ b/packages/cli/src/modules/instance-ai/instance-ai.controller.ts @@ -19,7 +19,11 @@ import { InstanceAiEvalRestoreThreadRequest, InstanceAiEvalSeedDataTableRowsRequest, } from '@n8n/api-types'; -import type { InstanceAiAgentNode, InstanceAiEvent } from '@n8n/api-types'; +import type { + InstanceAiAdminSettingsResponse, + InstanceAiAgentNode, + InstanceAiEvent, +} from '@n8n/api-types'; import { ModuleRegistry } from '@n8n/backend-common'; import { GlobalConfig } from '@n8n/config'; import { AuthenticatedRequest, User, UserRepository } from '@n8n/db'; @@ -32,6 +36,7 @@ import { Put, Patch, Delete, + OnPubSubEvent, Param, Body, Query, @@ -61,6 +66,7 @@ import { ConflictError } from '@/errors/response-errors/conflict.error'; import { ForbiddenError } from '@/errors/response-errors/forbidden.error'; import { NotFoundError } from '@/errors/response-errors/not-found.error'; import { Push } from '@/push'; +import { Publisher } from '@/scaling/pubsub/publisher.service'; import { ProjectService } from '@/services/project.service.ee'; import { UrlService } from '@/services/url.service'; @@ -126,6 +132,7 @@ export class InstanceAiController { private readonly credentialsService: CredentialsService, private readonly projectService: ProjectService, private readonly instanceAiErrorReporter: InstanceAiErrorReporterService, + private readonly publisher: Publisher, globalConfig: GlobalConfig, ) { this.gatewayApiKey = globalConfig.instanceAi.gatewayApiKey; @@ -661,7 +668,7 @@ export class InstanceAiController { @Get('/settings') @GlobalScope('instanceAi:manage') async getAdminSettings(_req: AuthenticatedRequest) { - return this.settingsService.getAdminSettings(); + return await this.settingsService.getAdminSettings(); } @Put('/settings') @@ -672,31 +679,41 @@ export class InstanceAiController { @Body payload: InstanceAiAdminSettingsUpdateRequest, ) { const result = await this.settingsService.updateAdminSettings(payload); + await this.applyAdminSettingsSideEffects(result); + await this.publisher.publishCommand({ command: 'reload-instance-ai-settings' }); + + return result; + } + + @OnPubSubEvent('reload-instance-ai-settings', { instanceType: 'main' }) + async reloadAdminSettings() { + await this.settingsService.reloadFromDb(); + await this.applyAdminSettingsSideEffects(await this.settingsService.getAdminSettings()); + } + + private async applyAdminSettingsSideEffects(settings: InstanceAiAdminSettingsResponse) { await this.moduleRegistry.refreshModuleSettings('instance-ai'); - if (payload.enabled === false || payload.browserUseEnabled === false) { + if (!settings.enabled || !settings.browserUseEnabled) { await this.browserSessionService.shutdown(); } - if (payload.enabled === false || payload.localGatewayDisabled === true) { - const disconnectedUserIds = this.gatewayService.disconnectAllGateways(); - if (disconnectedUserIds.length > 0) { - this.push.sendToUsers( - { - type: 'instanceAiGatewayStateChanged', - data: { - connected: false, - directory: null, - hostIdentifier: null, - toolCategories: [], - }, - }, - disconnectedUserIds, - ); - } - } + if (settings.enabled && !settings.localGatewayDisabled) return; - return result; + const disconnectedUserIds = this.gatewayService.disconnectAllGateways(); + if (disconnectedUserIds.length === 0) return; + this.push.sendToUsers( + { + type: 'instanceAiGatewayStateChanged', + data: { + connected: false, + directory: null, + hostIdentifier: null, + toolCategories: [], + }, + }, + disconnectedUserIds, + ); } // ── User preferences (per-user, self-service) ────────────────────────── @@ -733,6 +750,12 @@ export class InstanceAiController { return await this.settingsService.listServiceCredentials(req.user); } + @Get('/settings/model-credentials') + @GlobalScope('instanceAi:manage') + async listInstanceModelCredentials(_req: AuthenticatedRequest) { + return await this.settingsService.listInstanceModelCredentials(); + } + @Get('/threads') @GlobalScope('instanceAi:message') async listThreads(req: AuthenticatedRequest) { @@ -1265,7 +1288,7 @@ export class InstanceAiController { // ── Helpers ────────────────────────────────────────────────────────────── private assertBrowserChannelEnabled(): void { - if (!this.settingsService.getAdminSettings().browserUseEnabled) { + if (!this.settingsService.isBrowserUseEnabled()) { throw new ForbiddenError('Browser Use is disabled'); } } diff --git a/packages/cli/src/modules/instance-ai/instance-ai.module.ts b/packages/cli/src/modules/instance-ai/instance-ai.module.ts index c28bbcd8c04..6af80931228 100644 --- a/packages/cli/src/modules/instance-ai/instance-ai.module.ts +++ b/packages/cli/src/modules/instance-ai/instance-ai.module.ts @@ -16,8 +16,15 @@ export class InstanceAiModule implements ModuleInterface { const logger = Container.get(Logger).scoped('instance-ai'); logger.warn(`${YELLOW}${WARNING_MESSAGE}${CLEAR}`); - const { InstanceAiSettingsService } = await import('./instance-ai-settings.service.js'); - await Container.get(InstanceAiSettingsService).loadFromDb(); + const { InstanceCredentialBroker } = await import( + '@/credentials/instance-credential-broker.js' + ); + const { InstanceAiSettingsService, INSTANCE_AI_MODEL_CREDENTIAL_POLICY } = await import( + './instance-ai-settings.service.js' + ); + const settingsService = Container.get(InstanceAiSettingsService); + Container.get(InstanceCredentialBroker).registerUse(INSTANCE_AI_MODEL_CREDENTIAL_POLICY); + await settingsService.loadFromDb(); await import('./instance-ai.controller.js'); await import('./mcp/instance-ai-mcp-connection.controller.js'); diff --git a/packages/cli/src/modules/instance-ai/instance-ai.service.ts b/packages/cli/src/modules/instance-ai/instance-ai.service.ts index 882a2ba88ee..9ba62065d61 100644 --- a/packages/cli/src/modules/instance-ai/instance-ai.service.ts +++ b/packages/cli/src/modules/instance-ai/instance-ai.service.ts @@ -2078,7 +2078,7 @@ export class InstanceAiService { ); } - const adminSettings = this.settingsService.getAdminSettings(); + const adminSettings = await this.settingsService.getAdminSettings(); const localGatewayDisabledGlobally = adminSettings.localGatewayDisabled; const browserUseEnabledGlobally = adminSettings.browserUseEnabled; const localGatewayDisabledForUser = await this.settingsService.isLocalGatewayDisabledForUser( diff --git a/packages/cli/src/modules/mcp/__tests__/webhook-utils.test.ts b/packages/cli/src/modules/mcp/__tests__/webhook-utils.test.ts index a1bd1a4eafb..8ae39f42cbd 100644 --- a/packages/cli/src/modules/mcp/__tests__/webhook-utils.test.ts +++ b/packages/cli/src/modules/mcp/__tests__/webhook-utils.test.ts @@ -26,6 +26,7 @@ const mockCredentialsService = ( isManaged: false, isGlobal: false, isResolvable: false, + usageScope: 'project', resolverId: null, resolvableAllowFallback: false, id, diff --git a/packages/cli/src/modules/source-control.ee/__tests__/source-control-import.service.ee.test.ts b/packages/cli/src/modules/source-control.ee/__tests__/source-control-import.service.ee.test.ts index 58ee61360a7..05136131c10 100644 --- a/packages/cli/src/modules/source-control.ee/__tests__/source-control-import.service.ee.test.ts +++ b/packages/cli/src/modules/source-control.ee/__tests__/source-control-import.service.ee.test.ts @@ -22,6 +22,7 @@ import { type WorkflowRepository, } from '@n8n/db'; import { In } from '@n8n/typeorm'; +import type { EntityManager } from '@n8n/typeorm'; import * as fastGlob from 'fast-glob'; import { type InstanceSettings } from 'n8n-core'; import fsp from 'node:fs/promises'; @@ -31,6 +32,7 @@ import type { Mock } from 'vitest'; import { mock } from 'vitest-mock-extended'; import type { ActiveWorkflowManager } from '@/active-workflow-manager'; +import type { CredentialsService } from '@/credentials/credentials.service'; import type { VariablesService } from '@/environments.ee/variables/variables.service.ee'; import type { ExecutionPersistence } from '@/executions/execution-persistence'; import type { DataTableColumnRepository } from '@/modules/data-table/data-table-column.repository'; @@ -59,7 +61,10 @@ describe('SourceControlImportService', () => { const tagRepository = mock(); const workflowTagMappingRepository = mock(); const userRepository = mock(); - const credentialsRepository = mock(); + const credentialsRepositoryManager = mock(); + const credentialsRepository = mock({ + manager: credentialsRepositoryManager, + }); const sharedCredentialsRepository = mock(); const mockLogger = mock(); const sourceControlContextFactory = mock(); @@ -75,6 +80,8 @@ describe('SourceControlImportService', () => { const dataTableSizeValidator = mock(); const activeWorkflowManager = mock(); const executionPersistence = mock(); + const credentialsService = mock(); + const transactionManager = mock(); const globalAdminContext = new SourceControlContext( Object.assign(new User(), { role: GLOBAL_ADMIN_ROLE }), @@ -102,7 +109,7 @@ describe('SourceControlImportService', () => { workflowRepository, workflowTagMappingRepository, workflowService, - mock(), + credentialsService, mock(), folderRepository, mock({ n8nFolder: '/mock/n8n' }), @@ -123,6 +130,19 @@ describe('SourceControlImportService', () => { beforeEach(() => { vi.clearAllMocks(); + credentialsRepository.find.mockResolvedValue([]); + transactionManager.upsert.mockImplementation( + async (_entity, value, conflictPaths) => + await credentialsRepository.upsert(value as never, conflictPaths as never), + ); + transactionManager.delete.mockImplementation( + async (_entity, criteria) => await sharedCredentialsRepository.delete(criteria as never), + ); + credentialsRepositoryManager.transaction.mockImplementation(async (...args) => { + const callback = args.find((arg) => typeof arg === 'function'); + if (!callback) throw new Error('Transaction callback is required'); + return await callback(transactionManager); + }); sourceControlScopedService.getDataTablesInAdminProjectsFromContextFilter.mockReturnValue({}); }); @@ -1376,6 +1396,42 @@ describe('SourceControlImportService', () => { ); }); + it('should skip credentials that are instance credentials locally, for any caller', async () => { + globMock.mockResolvedValue(['/mock/credential1.json']); + fsReadFile.mockResolvedValue( + JSON.stringify({ + id: 'cred1', + name: 'Disguised Credential', + type: 'oauth2Api', + data: {}, + ownedBy: null, + }), + ); + credentialsRepository.find.mockResolvedValue([ + { id: 'cred1', usageScope: 'instance' } as any, + ]); + + await expect(service.getRemoteCredentialsFromFiles(globalMemberContext)).resolves.toEqual([]); + await expect(service.getRemoteCredentialsFromFiles(globalAdminContext)).resolves.toEqual([]); + }); + + it('should skip remote credential files flagged as instance credentials', async () => { + globMock.mockResolvedValue(['/mock/credential1.json']); + fsReadFile.mockResolvedValue( + JSON.stringify({ + id: 'cred1', + name: 'Provider Connection', + type: 'anthropicApi', + data: {}, + ownedBy: null, + usageScope: 'instance', + }), + ); + credentialsRepository.find.mockResolvedValue([]); + + await expect(service.getRemoteCredentialsFromFiles(globalAdminContext)).resolves.toEqual([]); + }); + it('should filter out files without valid credential data', async () => { globMock.mockResolvedValue(['/mock/invalid.json']); fsReadFile.mockResolvedValue('{}'); @@ -1620,6 +1676,122 @@ describe('SourceControlImportService', () => { ); }); + it('should skip credential files flagged as instance credentials', async () => { + const candidates: SourceControlledFile[] = [ + { + file: '/mock/credential_stubs/cred1.json', + id: 'cred1', + name: 'Instance Credential', + type: 'credential', + status: 'modified', + location: 'local', + conflict: false, + updatedAt: '', + }, + ]; + + fsReadFile.mockResolvedValue( + JSON.stringify({ + id: 'cred1', + name: 'Instance Credential', + type: 'oauth2Api', + data: {}, + ownedBy: null, + usageScope: 'instance', + }), + ); + credentialsRepository.find.mockResolvedValue([]); + sharedCredentialsRepository.find.mockResolvedValue([]); + + const result = await service.importCredentialsFromWorkFolder(candidates, mockUserId); + + expect(result).toEqual([]); + expect(credentialsRepository.upsert).not.toHaveBeenCalled(); + expect(credentialsRepositoryManager.transaction).not.toHaveBeenCalled(); + }); + + it('should not touch an existing instance credential even when the remote file omits usageScope', async () => { + const candidates: SourceControlledFile[] = [ + { + file: '/mock/credential_stubs/cred1.json', + id: 'cred1', + name: 'Instance Credential', + type: 'credential', + status: 'modified', + location: 'local', + conflict: false, + updatedAt: '', + }, + ]; + + fsReadFile.mockResolvedValue( + JSON.stringify({ + id: 'cred1', + name: 'Instance Credential', + type: 'oauth2Api', + data: {}, + ownedBy: null, + }), + ); + credentialsRepository.find.mockResolvedValue([ + { + id: 'cred1', + name: 'Instance Credential', + type: 'oauth2Api', + data: undefined, + usageScope: 'instance', + } as any, + ]); + sharedCredentialsRepository.find.mockResolvedValue([]); + + const result = await service.importCredentialsFromWorkFolder(candidates, mockUserId); + + expect(result).toEqual([]); + expect(credentialsRepository.upsert).not.toHaveBeenCalled(); + expect(sharedCredentialsRepository.delete).not.toHaveBeenCalled(); + }); + + it('should not convert an existing project credential when a remote file declares instance', async () => { + const candidates: SourceControlledFile[] = [ + { + file: '/mock/credential_stubs/cred1.json', + id: 'cred1', + name: 'Workflow Credential', + type: 'credential', + status: 'modified', + location: 'local', + conflict: false, + updatedAt: '', + }, + ]; + + fsReadFile.mockResolvedValue( + JSON.stringify({ + id: 'cred1', + name: 'Workflow Credential', + type: 'oauth2Api', + data: {}, + ownedBy: null, + usageScope: 'instance', + }), + ); + credentialsRepository.find.mockResolvedValue([ + { + id: 'cred1', + name: 'Workflow Credential', + type: 'oauth2Api', + data: undefined, + usageScope: 'project', + } as any, + ]); + sharedCredentialsRepository.find.mockResolvedValue([]); + + const result = await service.importCredentialsFromWorkFolder(candidates, mockUserId); + + expect(result).toEqual([]); + expect(credentialsRepository.upsert).not.toHaveBeenCalled(); + }); + it('should default resolver fields to false when absent from the stub', async () => { // Arrange - a stub written before resolver fields were tracked omits them const candidates: SourceControlledFile[] = [ diff --git a/packages/cli/src/modules/source-control.ee/__tests__/source-control-scoped.service.test.ts b/packages/cli/src/modules/source-control.ee/__tests__/source-control-scoped.service.test.ts index acd233cc575..567dc42f7b8 100644 --- a/packages/cli/src/modules/source-control.ee/__tests__/source-control-scoped.service.test.ts +++ b/packages/cli/src/modules/source-control.ee/__tests__/source-control-scoped.service.test.ts @@ -54,4 +54,19 @@ describe('SourceControlScopedService', () => { await expect(service.ensureIsAllowedToGetStatus(req)).rejects.toThrow(ForbiddenError); }); }); + + describe('getCredentialsInAdminProjectsFromContextFilter', () => { + const context = mock({ + user: req.user, + hasAccessToAllProjects: () => true, + }); + + it('excludes instance credentials regardless of the instance credential scope', () => { + vi.mocked(hasGlobalScope).mockReturnValue(true); + + expect(service.getCredentialsInAdminProjectsFromContextFilter(context)).toEqual({ + usageScope: 'project', + }); + }); + }); }); diff --git a/packages/cli/src/modules/source-control.ee/source-control-export.service.ee.ts b/packages/cli/src/modules/source-control.ee/source-control-export.service.ee.ts index de6b6609888..7bd806b48f4 100644 --- a/packages/cli/src/modules/source-control.ee/source-control-export.service.ee.ts +++ b/packages/cli/src/modules/source-control.ee/source-control-export.service.ee.ts @@ -520,12 +520,11 @@ export class SourceControlExportService { credentialIds, 'credential:owner', ); + let missingIds: string[] = []; - if (credentialsToBeExported.length !== credentialIds.length) { - const foundCredentialIds = credentialsToBeExported.map((e) => e.credentialsId); - missingIds = credentialIds.filter( - (remote) => foundCredentialIds.findIndex((local) => local === remote) === -1, - ); + const foundCredentialIds = new Set(credentialsToBeExported.map((e) => e.credentialsId)); + if (foundCredentialIds.size !== credentialIds.length) { + missingIds = credentialIds.filter((remote) => !foundCredentialIds.has(remote)); } await Promise.all( credentialsToBeExported.map(async (sharing) => { diff --git a/packages/cli/src/modules/source-control.ee/source-control-import.service.ee.ts b/packages/cli/src/modules/source-control.ee/source-control-import.service.ee.ts index 707ae77b6f7..d6cfff712f8 100644 --- a/packages/cli/src/modules/source-control.ee/source-control-import.service.ee.ts +++ b/packages/cli/src/modules/source-control.ee/source-control-import.service.ee.ts @@ -10,6 +10,7 @@ import type { WorkflowEntity, } from '@n8n/db'; import { + CredentialsEntity, CredentialsRepository, FolderRepository, ProjectRelationRepository, @@ -25,7 +26,7 @@ import { } from '@n8n/db'; import { Service } from '@n8n/di'; import { PROJECT_ADMIN_ROLE_SLUG, PROJECT_OWNER_ROLE_SLUG } from '@n8n/permissions'; -import { In, type DataSourceOptions } from '@n8n/typeorm'; +import { In, type DataSourceOptions, type EntityManager } from '@n8n/typeorm'; import { QueryDeepPartialEntity } from '@n8n/typeorm/query-builder/QueryPartialEntity'; import glob from 'fast-glob'; import isEqual from 'lodash/isEqual'; @@ -323,11 +324,29 @@ export class SourceControlImportService { }, ); + const remoteIds = remoteCredentialFilesRead.flatMap((remote) => + remote?.id ? [remote.id] : [], + ); + const localUsageScope = new Map( + remoteIds.length === 0 + ? [] + : ( + await this.credentialsRepository.find({ + where: { id: In(remoteIds) }, + select: ['id', 'usageScope'], + }) + ).map((local) => [local.id, local.usageScope]), + ); + const remoteCredentialFilesParsed = remoteCredentialFilesRead .filter((remote) => { if (!remote?.id) { return false; } + // Instance credentials (provider connections) are instance-local and never synced + if (remote.usageScope === 'instance' || localUsageScope.get(remote.id) === 'instance') { + return false; + } const owner = remote.ownedBy; return ( !owner || context.hasAccessToAllProjects() || context.findAuthorizedProjectByOwner(owner) @@ -969,8 +988,11 @@ export class SourceControlImportService { where: { id: In(candidateIds), }, - select: ['id', 'name', 'type', 'data'], + select: ['id', 'name', 'type', 'data', 'usageScope'], }); + const existingCredentialsById = new Map( + existingCredentials.map((credential) => [credential.id, credential]), + ); const existingSharedCredentials = await this.sharedCredentialsRepository.find({ select: ['credentialsId', 'projectId', 'role'], where: { @@ -979,75 +1001,99 @@ export class SourceControlImportService { }, }); - let importCredentialsResult: Array<{ id: string; name: string; type: string }> = []; - importCredentialsResult = await Promise.all( - candidates.map(async (candidate) => { - this.logger.debug(`Importing credentials file ${candidate.file}`); - const credential = jsonParse( - await fsReadFile(candidate.file, { encoding: 'utf8' }), - ); - const existingCredential = existingCredentials.find( - (e) => e.id === credential.id && e.type === credential.type, - ); - - // Carry the "private"/resolvable nature across environments. resolverId is - // instance-local and handled separately (see IAM-906). - const { - name, - type, - data, - id, - isGlobal = false, - isResolvable = false, - resolvableAllowFallback = false, - } = credential; - const newCredentialObject = new Credentials({ id, name }, type); - - if (existingCredential?.data) { - // Credential exists - merge expressions from remote while preserving local plain values - const existingDecrypted = new Credentials( - { id: existingCredential.id, name: existingCredential.name }, - existingCredential.type, - existingCredential.data, + const importCredentialsResult: Array<{ id: string; name: string; type: string } | undefined> = + await Promise.all( + candidates.map(async (candidate) => { + this.logger.debug(`Importing credentials file ${candidate.file}`); + const credential = jsonParse( + await fsReadFile(candidate.file, { encoding: 'utf8' }), ); - const localData = await existingDecrypted.getData(); - const mergedData = mergeRemoteCrendetialDataIntoLocalCredentialData({ - local: localData, - remote: data, + const existingCredentialById = existingCredentialsById.get(credential.id); + + // Instance credentials (provider connections) are instance-local and never synced + if ( + credential.usageScope === 'instance' || + existingCredentialById?.usageScope === 'instance' + ) { + this.logger.debug(`Skipping provider connection file ${candidate.file}`); + return undefined; + } + + const existingCredential = + existingCredentialById?.type === credential.type ? existingCredentialById : undefined; + + // Carry the "private"/resolvable nature across environments. resolverId is + // instance-local and handled separately (see IAM-906). + const { + name, + type, + data, + id, + isGlobal = false, + isResolvable = false, + resolvableAllowFallback = false, + } = credential; + const newCredentialObject = new Credentials({ id, name }, type); + + if (existingCredential?.data) { + // Credential exists - merge expressions from remote while preserving local plain values + const existingDecrypted = new Credentials( + { id: existingCredential.id, name: existingCredential.name }, + existingCredential.type, + existingCredential.data, + ); + const localData = await existingDecrypted.getData(); + const mergedData = mergeRemoteCrendetialDataIntoLocalCredentialData({ + local: localData, + remote: data, + }); + await newCredentialObject.setData(mergedData); + } else { + // This is a safe guard, in principle remote data should already be sanitized + // This prevents importing invalid data that should have not been synched in the first place + const sanitizedData = sanitizeCredentialData(data); + await newCredentialObject.setData(sanitizedData); + } + const targetOwnerProject = await this.resolveTargetOwnerProject( + credential.ownedBy, + personalProject, + ); + + this.logger.debug(`Updating credential id ${newCredentialObject.id as string}`); + await this.credentialsRepository.manager.transaction(async (transactionManager) => { + await transactionManager.upsert( + CredentialsEntity, + { + ...newCredentialObject, + isGlobal, + isResolvable, + resolvableAllowFallback, + }, + ['id'], + ); + + const localOwner = existingSharedCredentials.find( + (c) => c.credentialsId === credential.id && c.role === 'credential:owner', + ); + + await this.syncResourceOwnership({ + resourceId: credential.id, + remoteOwner: credential.ownedBy, + localOwner, + fallbackProject: personalProject, + repository: this.sharedCredentialsRepository, + transactionManager, + targetOwnerProject, + }); }); - await newCredentialObject.setData(mergedData); - } else { - // This is a safe guard, in principle remote data should already be sanitized - // This prevents importing invalid data that should have not been synched in the first place - const sanitizedData = sanitizeCredentialData(data); - await newCredentialObject.setData(sanitizedData); - } - this.logger.debug(`Updating credential id ${newCredentialObject.id as string}`); - await this.credentialsRepository.upsert( - { ...newCredentialObject, isGlobal, isResolvable, resolvableAllowFallback }, - ['id'], - ); - - const localOwner = existingSharedCredentials.find( - (c) => c.credentialsId === credential.id && c.role === 'credential:owner', - ); - - await this.syncResourceOwnership({ - resourceId: credential.id, - remoteOwner: credential.ownedBy, - localOwner, - fallbackProject: personalProject, - repository: this.sharedCredentialsRepository, - }); - - return { - id: newCredentialObject.id as string, - name: newCredentialObject.name, - type: newCredentialObject.type, - }; - }), - ); + return { + id: newCredentialObject.id as string, + name: newCredentialObject.name, + type: newCredentialObject.type, + }; + }), + ); return importCredentialsResult.filter((e) => e !== undefined); } @@ -1681,7 +1727,9 @@ export class SourceControlImportService { async deleteCredentialsNotInWorkfolder(user: User, candidates: SourceControlledFile[]) { for (const candidate of candidates) { - await this.credentialsService.delete(user, candidate.id); + await this.credentialsService.delete(user, candidate.id, { + includeInstanceCredentials: true, + }); } } @@ -1829,24 +1877,20 @@ export class SourceControlImportService { localOwner, fallbackProject, repository, + transactionManager, + targetOwnerProject, }: { resourceId: string; remoteOwner: RemoteResourceOwner | null | undefined; localOwner: { projectId: string } | undefined; fallbackProject: Project; repository: SharedWorkflowRepository | SharedCredentialsRepository; + transactionManager?: EntityManager; + targetOwnerProject?: Project; }): Promise { - let targetOwnerProject = await this.findOwnerProjectInLocalDb(remoteOwner ?? undefined); - if (!targetOwnerProject) { - const isSharedResource = - remoteOwner && typeof remoteOwner !== 'string' && remoteOwner.type === 'team'; + targetOwnerProject ??= await this.resolveTargetOwnerProject(remoteOwner, fallbackProject); - targetOwnerProject = isSharedResource - ? await this.createTeamProject(remoteOwner) - : fallbackProject; - } - - const trx = this.workflowRepository.manager; + const trx = transactionManager ?? this.workflowRepository.manager; // remove old ownership if it changed const shouldRemoveOldOwner = localOwner && localOwner.projectId !== targetOwnerProject.id; @@ -1858,6 +1902,18 @@ export class SourceControlImportService { await repository.makeOwner([resourceId], targetOwnerProject.id, trx); } + private async resolveTargetOwnerProject( + remoteOwner: RemoteResourceOwner | null | undefined, + fallbackProject: Project, + ): Promise { + const targetOwnerProject = await this.findOwnerProjectInLocalDb(remoteOwner ?? undefined); + if (targetOwnerProject) return targetOwnerProject; + + const isSharedResource = + remoteOwner && typeof remoteOwner !== 'string' && remoteOwner.type === 'team'; + return isSharedResource ? await this.createTeamProject(remoteOwner) : fallbackProject; + } + private async findOwnerProjectInLocalDb(owner: RemoteResourceOwner | IWorkflowToImport['owner']) { if (!owner) { return null; diff --git a/packages/cli/src/modules/source-control.ee/source-control-scoped.service.ts b/packages/cli/src/modules/source-control.ee/source-control-scoped.service.ts index 6fe3bb8b93e..330c2279605 100644 --- a/packages/cli/src/modules/source-control.ee/source-control-scoped.service.ts +++ b/packages/cli/src/modules/source-control.ee/source-control-scoped.service.ts @@ -119,7 +119,8 @@ export class SourceControlScopedService { context: SourceControlContext, ): FindOptionsWhere { if (context.hasAccessToAllProjects()) { - return {}; + // Instance credentials (provider connections) are instance-local and never synced + return { usageScope: 'project' }; } return { diff --git a/packages/cli/src/modules/source-control.ee/types/exportable-credential.ts b/packages/cli/src/modules/source-control.ee/types/exportable-credential.ts index 4bfb8e14c8b..af56cd7f411 100644 --- a/packages/cli/src/modules/source-control.ee/types/exportable-credential.ts +++ b/packages/cli/src/modules/source-control.ee/types/exportable-credential.ts @@ -1,3 +1,4 @@ +import type { CredentialUsageScope } from '@n8n/db'; import type { ICredentialDataDecryptedObject } from 'n8n-workflow'; import type { RemoteResourceOwner, StatusResourceOwner } from './resource-owner'; @@ -32,6 +33,7 @@ export interface ExportableCredential { * resolution fails. Travels with `isResolvable`. */ resolvableAllowFallback?: boolean; + usageScope?: CredentialUsageScope; } export type StatusExportableCredential = ExportableCredential & { diff --git a/packages/cli/src/modules/workflow-index/workflow-dependency-query.service.ts b/packages/cli/src/modules/workflow-index/workflow-dependency-query.service.ts index 0be1eb9a18d..cef7ff23b3a 100644 --- a/packages/cli/src/modules/workflow-index/workflow-dependency-query.service.ts +++ b/packages/cli/src/modules/workflow-index/workflow-dependency-query.service.ts @@ -93,7 +93,7 @@ export class WorkflowDependencyQueryService { const [credentials, workflows, dataTables] = await Promise.all([ maps.allCredIds.size > 0 ? this.credentialsRepository.find({ - where: { id: In([...maps.allCredIds]) }, + where: { id: In([...maps.allCredIds]), usageScope: 'project' }, select: ['id', 'name'], }) : [], diff --git a/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-request.service.test.ts b/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-request.service.test.ts index 9fb96e8c7b1..5d7510c6a32 100644 --- a/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-request.service.test.ts +++ b/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-request.service.test.ts @@ -92,7 +92,7 @@ describe('WorkflowReviewRequestService', () => { // Feature enabled by default; the disabled path is exercised explicitly. workflowReviewPolicyService.get.mockResolvedValue({ enabled: true }); // By default, run the critical section against the mocked transaction. - dbLockService.withLock.mockImplementation(async (_id, fn) => await fn(tx)); + dbLockService.withLock.mockImplementation(async (_id, fn) => await fn(tx, {})); collaborationService.broadcastWorkflowReviewStateChanged.mockResolvedValue(undefined); }); @@ -307,7 +307,7 @@ describe('WorkflowReviewRequestService', () => { mockSuccessfulCreatePath(); let lockResolved = false; dbLockService.withLock.mockImplementation(async (_id, fn) => { - const result = await fn(tx); + const result = await fn(tx, {}); lockResolved = true; return result; }); diff --git a/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-request.update.service.test.ts b/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-request.update.service.test.ts index bed4c993f9c..7a3a7e50f26 100644 --- a/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-request.update.service.test.ts +++ b/packages/cli/src/modules/workflow-reviews.ee/__tests__/workflow-review-request.update.service.test.ts @@ -106,7 +106,7 @@ describe('WorkflowReviewRequestService.updateVersion', () => { licenseState.isWorkflowReviewsLicensed.mockReturnValue(true); workflowReviewPolicyService.get.mockResolvedValue({ enabled: true }); // By default, run the critical section against the mocked transaction. - dbLockService.withLock.mockImplementation(async (_id, fn) => await fn(tx)); + dbLockService.withLock.mockImplementation(async (_id, fn) => await fn(tx, {})); collaborationService.broadcastWorkflowReviewStateChanged.mockResolvedValue(undefined); }); @@ -298,7 +298,7 @@ describe('WorkflowReviewRequestService.updateVersion', () => { mockSuccessfulUpdatePath(); let lockResolved = false; dbLockService.withLock.mockImplementation(async (_id, fn) => { - const result = await fn(tx); + const result = await fn(tx, {}); lockResolved = true; return result; }); diff --git a/packages/cli/src/oauth/__tests__/oauth.service.test.ts b/packages/cli/src/oauth/__tests__/oauth.service.test.ts index ed04e932473..1073738875a 100644 --- a/packages/cli/src/oauth/__tests__/oauth.service.test.ts +++ b/packages/cli/src/oauth/__tests__/oauth.service.test.ts @@ -282,6 +282,7 @@ describe('OauthService', () => { 'credential-id', req.user, ['credential:update'], + { includeInstanceCredentials: true }, ); }); @@ -302,6 +303,7 @@ describe('OauthService', () => { 'credential-id', req.user, ['credential:connect'], + { includeInstanceCredentials: true }, ); }); }); @@ -460,6 +462,20 @@ describe('OauthService', () => { expect(result).toBeNull(); }); + + it('should restrict the lookup to project credentials when requested', async () => { + credentialsRepository.findOneBy.mockResolvedValue(null); + + const result = await (service as any).getCredentialWithoutUser('1', { + onlyProjectCredentials: true, + }); + + expect(result).toBeNull(); + expect(credentialsRepository.findOneBy).toHaveBeenCalledWith({ + id: '1', + usageScope: 'project', + }); + }); }); describe('createCsrfState', () => { @@ -531,6 +547,7 @@ describe('OauthService', () => { 'credential-id', req.user, ['credential:update'], + { includeInstanceCredentials: true }, ); }); diff --git a/packages/cli/src/oauth/oauth.service.ts b/packages/cli/src/oauth/oauth.service.ts index a6d1df5ddea..231d3382bd4 100644 --- a/packages/cli/src/oauth/oauth.service.ts +++ b/packages/cli/src/oauth/oauth.service.ts @@ -264,7 +264,12 @@ export class OauthService { // Private credentials are connected per-user, so executing users can authorize // their own account without edit rights. Shared/static credentials store the // token on the shared credential itself, so connecting them still requires edit. - const existingCredential = await this.credentialsFinderService.findCredentialById(credentialId); + const existingCredential = await this.credentialsFinderService.findCredentialById( + credentialId, + { + includeInstanceCredentials: true, + }, + ); const requiredScope = existingCredential?.isResolvable ? 'credential:connect' : 'credential:update'; @@ -273,6 +278,7 @@ export class OauthService { credentialId, req.user, [requiredScope], + { includeInstanceCredentials: true }, ); if (!credential) { @@ -427,8 +433,12 @@ export class OauthService { /** Get a credential without user check */ protected async getCredentialWithoutUser( credentialId: string, + options: { onlyProjectCredentials?: boolean } = {}, ): Promise { - return await this.credentialsRepository.findOneBy({ id: credentialId }); + return await this.credentialsRepository.findOneBy({ + id: credentialId, + ...(options.onlyProjectCredentials ? { usageScope: 'project' as const } : {}), + }); } /** @@ -549,7 +559,7 @@ export class OauthService { } return [ { ...decoded, ...decryptedState }, - await this.getCredentialWithoutUser(decryptedState.cid), + await this.getCredentialWithoutUser(decryptedState.cid, { onlyProjectCredentials: true }), ]; } @@ -569,6 +579,7 @@ export class OauthService { decryptedState.cid, req.user, ['credential:update'], + { includeInstanceCredentials: true }, ); return [{ ...decoded, ...decryptedState }, credential]; @@ -733,7 +744,7 @@ export class OauthService { projectId: string, ): Promise | null> { const credential = await this.credentialsRepository.findOne({ - where: { id: credentialId }, + where: { id: credentialId, usageScope: 'project' }, relations: { shared: true }, }); if (!credential) return null; diff --git a/packages/cli/src/permissions.ee/__tests__/check-access.test.ts b/packages/cli/src/permissions.ee/__tests__/check-access.test.ts index cdb51da4498..479394a530d 100644 --- a/packages/cli/src/permissions.ee/__tests__/check-access.test.ts +++ b/packages/cli/src/permissions.ee/__tests__/check-access.test.ts @@ -21,6 +21,7 @@ describe('userHasScopes', () => { let findByWorkflowMock: Mock; let findByCredentialMock: Mock; let findByGlobalCredentialMock: Mock; + let instanceCredentialExistsMock: Mock; let findGlobalCredentialByIdMock: Mock; let hasGlobalReadOnlyAccessMock: Mock; let roleServiceMock: Mock; @@ -30,6 +31,7 @@ describe('userHasScopes', () => { findByWorkflowMock = vi.fn(); findByCredentialMock = vi.fn(); findByGlobalCredentialMock = vi.fn(); + instanceCredentialExistsMock = vi.fn(); findGlobalCredentialByIdMock = vi.fn(); hasGlobalReadOnlyAccessMock = vi.fn(); roleServiceMock = vi.fn(); @@ -52,6 +54,7 @@ describe('userHasScopes', () => { CredentialsRepository, mock({ findBy: findByGlobalCredentialMock, + existsBy: instanceCredentialExistsMock, }), ); @@ -93,6 +96,7 @@ describe('userHasScopes', () => { findByWorkflowMock.mockReset(); findByCredentialMock.mockReset(); findByGlobalCredentialMock.mockReset(); + instanceCredentialExistsMock.mockReset(); findGlobalCredentialByIdMock.mockReset(); hasGlobalReadOnlyAccessMock.mockReset(); roleServiceMock.mockReset(); @@ -101,6 +105,43 @@ describe('userHasScopes', () => { mockQueryBuilder.getRawMany.mockResolvedValue([{ id: 'projectId' }]); }); + describe('instance credential management', () => { + it('allows the dedicated global scope through legacy credential route checks', async () => { + instanceCredentialExistsMock.mockResolvedValue(true); + const user = { + id: 'userId', + role: { + ...GLOBAL_MEMBER_ROLE, + scopes: [{ slug: 'credential:manageInstance' }], + }, + } as unknown as User; + + await expect( + userHasScopes(user, ['credential:update'], false, { credentialId: 'instance-cred' }), + ).resolves.toBe(true); + expect(instanceCredentialExistsMock).toHaveBeenCalledWith({ + id: 'instance-cred', + usageScope: 'instance', + }); + }); + + it('does not use the dedicated scope for sharing operations', async () => { + findByCredentialMock.mockResolvedValue([]); + const user = { + id: 'userId', + role: { + ...GLOBAL_MEMBER_ROLE, + scopes: [{ slug: 'credential:manageInstance' }], + }, + } as unknown as User; + + await expect( + userHasScopes(user, ['credential:share'], false, { credentialId: 'instance-cred' }), + ).rejects.toThrow(NotFoundError); + expect(instanceCredentialExistsMock).not.toHaveBeenCalled(); + }); + }); + describe('resource not found scenarios', () => { it.each<{ type: 'workflow' | 'credential'; id: string }>([ { type: 'workflow', id: 'workflowId' }, diff --git a/packages/cli/src/permissions.ee/check-access.ts b/packages/cli/src/permissions.ee/check-access.ts index 0368e5c5468..488caf2d4d4 100644 --- a/packages/cli/src/permissions.ee/check-access.ts +++ b/packages/cli/src/permissions.ee/check-access.ts @@ -1,6 +1,8 @@ import { ModuleRegistry } from '@n8n/backend-common'; import type { User, EntityManager } from '@n8n/db'; import { + CredentialsEntity, + CredentialsRepository, Project, ProjectRepository, SharedCredentials, @@ -16,6 +18,12 @@ import { CredentialsFinderService } from '@/credentials/credentials-finder.servi import { NotFoundError } from '@/errors/response-errors/not-found.error'; import { RoleService } from '@/services/role.service'; +const INSTANCE_CREDENTIAL_MANAGEMENT_SCOPES = new Set([ + 'credential:read', + 'credential:update', + 'credential:delete', +]); + /** * Check if a user has the required scopes. The check can be: * @@ -43,6 +51,24 @@ export async function userHasScopes( ): Promise { if (hasGlobalScope(user, scopes, { mode: 'allOf' })) return true; + if ( + credentialId && + hasGlobalScope(user, 'credential:manageInstance') && + scopes.every((scope) => INSTANCE_CREDENTIAL_MANAGEMENT_SCOPES.has(scope)) + ) { + const credentialsRepository = entityManager + ? entityManager.getRepository(CredentialsEntity) + : Container.get(CredentialsRepository); + if ( + await credentialsRepository.existsBy({ + id: credentialId, + usageScope: 'instance', + }) + ) { + return true; + } + } + if (globalOnly) return false; // Find which projects the user has access to with the required scopes. diff --git a/packages/cli/src/public-api/types.ts b/packages/cli/src/public-api/types.ts index 875aae1ecf4..66336f831d1 100644 --- a/packages/cli/src/public-api/types.ts +++ b/packages/cli/src/public-api/types.ts @@ -235,7 +235,13 @@ export declare namespace CredentialRequest { type Create = AuthenticatedRequest< {}, {}, - { type: string; name: string; data: ICredentialDataDecryptedObject; projectId?: string }, + { + type: string; + name: string; + data: ICredentialDataDecryptedObject; + projectId?: string; + isResolvable?: boolean; + }, {} >; diff --git a/packages/cli/src/public-api/v1/handlers/credentials/__tests__/credentials.service.test.ts b/packages/cli/src/public-api/v1/handlers/credentials/__tests__/credentials.service.test.ts index 59c96c5e4c0..49f34868fc9 100644 --- a/packages/cli/src/public-api/v1/handlers/credentials/__tests__/credentials.service.test.ts +++ b/packages/cli/src/public-api/v1/handlers/credentials/__tests__/credentials.service.test.ts @@ -1,5 +1,10 @@ import type { CredentialsEntity, Project, SharedCredentials, User } from '@n8n/db'; -import { CredentialsRepository, GLOBAL_OWNER_ROLE, GLOBAL_MEMBER_ROLE } from '@n8n/db'; +import { + CredentialsRepository, + GLOBAL_OWNER_ROLE, + GLOBAL_MEMBER_ROLE, + SharedCredentialsRepository, +} from '@n8n/db'; import { Container } from '@n8n/di'; import { mock } from 'vitest-mock-extended'; import { validate, type Schema } from 'jsonschema'; @@ -7,9 +12,15 @@ import { Cipher, CipherAes256GCM, CipherAes256CBC, EncryptionKeyProxy } from 'n8 import type { InstanceSettings } from 'n8n-core'; import type { GenericValue, IDataObject, INodeProperties } from 'n8n-workflow'; -import { buildSharedForCredential, toJsonSchema, updateCredential } from '../credentials.service'; +import { + buildSharedForCredential, + saveCredential, + toJsonSchema, + updateCredential, +} from '../credentials.service'; import { CredentialsService } from '@/credentials/credentials.service'; +import { EventService } from '@/events/event.service'; import { ExternalSecretsConfig } from '@/modules/external-secrets.ee/external-secrets.config'; import { SecretsProviderAccessCheckService } from '@/modules/external-secrets.ee/secret-provider-access-check.service.ee'; import * as checkAccess from '@/permissions.ee/check-access'; @@ -103,6 +114,51 @@ describe('CredentialsService', () => { }); }); + describe('saveCredential', () => { + it('forces public API credentials to project usage scope', async () => { + const credentialsService = mock(); + const sharedCredentialsRepository = mock(); + const eventService = mock(); + Container.set(CredentialsService, credentialsService); + Container.set(SharedCredentialsRepository, sharedCredentialsRepository); + Container.set(EventService, eventService); + credentialsService.createUnmanagedCredential.mockResolvedValue({ + id: 'credential-id', + name: 'Credential', + type: 'testApi', + isManaged: false, + isGlobal: false, + isResolvable: false, + resolvableAllowFallback: false, + resolverId: null, + createdAt: new Date(), + updatedAt: new Date(), + scopes: [], + } as never); + sharedCredentialsRepository.findCredentialOwningProject.mockResolvedValue(undefined); + const payload = { + type: 'testApi', + name: 'Credential', + data: { apiKey: 'secret' }, + usageScope: 'instance', + }; + + await saveCredential(payload, mock()); + + expect(credentialsService.createUnmanagedCredential).toHaveBeenCalledWith( + { + type: payload.type, + name: payload.name, + data: payload.data, + projectId: undefined, + isResolvable: undefined, + usageScope: 'project', + }, + expect.anything(), + ); + }); + }); + describe('toJsonSchema', () => { it('should create separate conditionals for different values of the same dependant field', () => { // This test simulates the JWT auth credential scenario where @@ -508,6 +564,9 @@ describe('CredentialsService', () => { mock(), // externalSecretsConfig mock(), // externalSecretsProviderAccessCheckService mock(), // connectionStatusProxy + mock(), // instanceCredentialAssignmentRepository + mock(), // instanceCredentialUseRegistry + mock(), // dbLockService ); beforeEach(() => { diff --git a/packages/cli/src/public-api/v1/handlers/credentials/credentials.handler.ts b/packages/cli/src/public-api/v1/handlers/credentials/credentials.handler.ts index 977e63ca7bc..67089f2a2b2 100644 --- a/packages/cli/src/public-api/v1/handlers/credentials/credentials.handler.ts +++ b/packages/cli/src/public-api/v1/handlers/credentials/credentials.handler.ts @@ -68,6 +68,7 @@ const credentialsHandlers: CredentialsHandlers = { select: ['id', 'name', 'type', 'createdAt', 'updatedAt'], relations: ['shared', 'shared.project'], order: { createdAt: 'DESC' }, + where: { usageScope: 'project' }, }); const data = credentials.map((credential: CredentialsEntity) => { diff --git a/packages/cli/src/public-api/v1/handlers/credentials/credentials.service.ts b/packages/cli/src/public-api/v1/handlers/credentials/credentials.service.ts index 92108b44b66..d398acd3446 100644 --- a/packages/cli/src/public-api/v1/handlers/credentials/credentials.service.ts +++ b/packages/cli/src/public-api/v1/handlers/credentials/credentials.service.ts @@ -69,7 +69,7 @@ export function buildSharedForCredential( export async function getCredential(credentialId: string): Promise { return await Container.get(CredentialsRepository).findOne({ - where: { id: credentialId }, + where: { id: credentialId, usageScope: 'project' }, relations: ['shared', 'shared.project'], }); } @@ -96,12 +96,28 @@ export async function getSharedCredentials( * resolution, validation, and encryption. */ export async function saveCredential( - payload: { type: string; name: string; data: ICredentialDataDecryptedObject; projectId?: string }, + payload: { + type: string; + name: string; + data: ICredentialDataDecryptedObject; + projectId?: string; + isResolvable?: boolean; + }, user: User, ): Promise { const { scopes: _scopes, ...credential } = await Container.get( CredentialsService, - ).createUnmanagedCredential({ ...payload, projectId: payload.projectId ?? undefined }, user); + ).createUnmanagedCredential( + { + type: payload.type, + name: payload.name, + data: payload.data, + projectId: payload.projectId, + isResolvable: payload.isResolvable, + usageScope: 'project', + }, + user, + ); const project = await Container.get(SharedCredentialsRepository).findCredentialOwningProject( credential.id, diff --git a/packages/cli/src/scaling/pubsub/pubsub.event-map.ts b/packages/cli/src/scaling/pubsub/pubsub.event-map.ts index a66c77d72fc..c3191d6be87 100644 --- a/packages/cli/src/scaling/pubsub/pubsub.event-map.ts +++ b/packages/cli/src/scaling/pubsub/pubsub.event-map.ts @@ -38,6 +38,7 @@ export type PubSubCommandMap = { 'reload-mcp-registry': never; 'reload-otel-config': never; + 'reload-instance-ai-settings': never; // #region Community packages diff --git a/packages/cli/src/scaling/pubsub/pubsub.types.ts b/packages/cli/src/scaling/pubsub/pubsub.types.ts index 5b5d26c6b78..906b5451c1e 100644 --- a/packages/cli/src/scaling/pubsub/pubsub.types.ts +++ b/packages/cli/src/scaling/pubsub/pubsub.types.ts @@ -70,6 +70,7 @@ export namespace PubSub { export type ReloadSourceControlConfiguration = ToCommand<'reload-source-control-config'>; export type ReloadMcpRegistry = ToCommand<'reload-mcp-registry'>; export type ReloadOtelConfig = ToCommand<'reload-otel-config'>; + export type ReloadInstanceAiSettings = ToCommand<'reload-instance-ai-settings'>; export type CancelTestRun = ToCommand<'cancel-test-run'>; export type CancelCollection = ToCommand<'cancel-collection'>; export type AgentChatIntegrationChanged = ToCommand<'agent-chat-integration-changed'>; @@ -111,6 +112,7 @@ export namespace PubSub { | Commands.ReloadSourceControlConfiguration | Commands.ReloadMcpRegistry | Commands.ReloadOtelConfig + | Commands.ReloadInstanceAiSettings | Commands.CancelTestRun | Commands.CancelCollection | Commands.AgentChatIntegrationChanged diff --git a/packages/cli/src/security-audit/risk-reporters/credentials-risk-reporter.ts b/packages/cli/src/security-audit/risk-reporters/credentials-risk-reporter.ts index 8a22a546ec6..02f2aaf5d24 100644 --- a/packages/cli/src/security-audit/risk-reporters/credentials-risk-reporter.ts +++ b/packages/cli/src/security-audit/risk-reporters/credentials-risk-reporter.ts @@ -110,7 +110,10 @@ export class CredentialsRiskReporter implements RiskReporter { } private async getAllExistingCreds() { - const credentials = await this.credentialsRepository.find({ select: ['id', 'name'] }); + const credentials = await this.credentialsRepository.find({ + select: ['id', 'name'], + where: { usageScope: 'project' }, + }); return credentials.map(({ id, name }) => ({ kind: 'credential' as const, id, name })); } diff --git a/packages/cli/src/services/__tests__/credentials-finder.service.test.ts b/packages/cli/src/services/__tests__/credentials-finder.service.test.ts index 7bc67654937..e69f44119fd 100644 --- a/packages/cli/src/services/__tests__/credentials-finder.service.test.ts +++ b/packages/cli/src/services/__tests__/credentials-finder.service.test.ts @@ -61,13 +61,117 @@ describe('CredentialsFinderService', () => { }); }); + describe('findCredentialById', () => { + it('queries project credentials by default', async () => { + credentialsRepository.findOne.mockResolvedValueOnce(null); + + await credentialsFinderService.findCredentialById('credential-id'); + + expect(credentialsRepository.findOne).toHaveBeenCalledWith({ + where: { id: 'credential-id', usageScope: 'project' }, + }); + }); + + it('includes instance credentials only when explicitly requested', async () => { + credentialsRepository.findOne.mockResolvedValueOnce(null); + + await credentialsFinderService.findCredentialById('credential-id', { + includeInstanceCredentials: true, + }); + + expect(credentialsRepository.findOne).toHaveBeenCalledWith({ + where: { + id: 'credential-id', + usageScope: In(['project', 'instance']), + }, + }); + }); + }); + describe('findCredentialForUser', () => { const credentialsId = 'cred_123'; const sharedCredential = mock(); - sharedCredential.credentials = mock({ id: credentialsId }); + sharedCredential.credentials = mock({ + id: credentialsId, + usageScope: 'project', + }); const owner = mock({ role: GLOBAL_OWNER_ROLE }); const member = mock({ role: GLOBAL_MEMBER_ROLE, id: 'test' }); + test('should return instance credentials only when explicitly requested by a manager', async () => { + const instanceCredential = mock({ + id: credentialsId, + usageScope: 'instance', + }); + credentialsRepository.findOneBy.mockResolvedValueOnce(instanceCredential); + + const credential = await credentialsFinderService.findCredentialForUser( + credentialsId, + owner, + ['credential:read' as const], + { includeInstanceCredentials: true }, + ); + + expect(credentialsRepository.findOneBy).toHaveBeenCalledWith({ + id: credentialsId, + usageScope: 'instance', + }); + expect(credential).toBe(instanceCredential); + expect(sharedCredentialsRepository.findOne).not.toHaveBeenCalled(); + }); + + test('should not return instance credentials through generic access checks', async () => { + sharedCredentialsRepository.findOne.mockResolvedValueOnce( + mock({ + credentials: mock({ + id: credentialsId, + usageScope: 'instance', + }), + }), + ); + + const credential = await credentialsFinderService.findCredentialForUser( + credentialsId, + owner, + ['credential:read' as const], + ); + + expect(credentialsRepository.findOneBy).not.toHaveBeenCalled(); + expect(credential).toBeNull(); + }); + + test('should not return instance credentials to members', async () => { + sharedCredentialsRepository.findOne.mockResolvedValueOnce(null); + credentialsRepository.findOne.mockResolvedValueOnce(null); + + const credential = await credentialsFinderService.findCredentialForUser( + credentialsId, + member, + ['credential:read' as const], + ); + + expect(credentialsRepository.findOneBy).not.toHaveBeenCalled(); + expect(credential).toBeFalsy(); + }); + + test('should ignore stale sharing rows for instance credentials', async () => { + const staleSharedCredential = mock({ + credentials: mock({ + id: credentialsId, + usageScope: 'instance', + }), + }); + sharedCredentialsRepository.findOne.mockResolvedValueOnce(staleSharedCredential); + + const credential = await credentialsFinderService.findCredentialForUser( + credentialsId, + member, + ['credential:read' as const], + ); + + expect(credential).toBeNull(); + }); + test('should allow instance owner access to all credentials', async () => { sharedCredentialsRepository.findOne.mockResolvedValueOnce(sharedCredential); const credential = await credentialsFinderService.findCredentialForUser( @@ -158,6 +262,7 @@ describe('CredentialsFinderService', () => { where: { id: credentialsId, isGlobal: true, + usageScope: 'project', }, relations: { shared: { project: true }, @@ -181,6 +286,7 @@ describe('CredentialsFinderService', () => { where: { id: credentialsId, isGlobal: true, + usageScope: 'project', }, relations: { shared: { project: true }, @@ -284,11 +390,11 @@ describe('CredentialsFinderService', () => { ]); expect(credentialsRepository.find).toHaveBeenCalledWith({ - where: { isGlobal: false }, + where: { isGlobal: false, usageScope: 'project' }, relations: { shared: true }, }); expect(credentialsRepository.manager.find).toHaveBeenCalledWith(CredentialsEntity, { - where: { isGlobal: true }, + where: { isGlobal: true, usageScope: 'project' }, relations: { shared: true }, }); expect(roleService.rolesWithScope).not.toHaveBeenCalled(); @@ -307,6 +413,7 @@ describe('CredentialsFinderService', () => { expect(credentialsRepository.find).toHaveBeenCalledWith({ where: { isGlobal: false, + usageScope: 'project', shared: { role: In(['credential:owner', 'credential:user']), project: { @@ -396,7 +503,7 @@ describe('CredentialsFinderService', () => { expect(result).toEqual(credentials); // Should call fetchGlobalCredentials when read-only expect(credentialsRepository.manager.find).toHaveBeenCalledWith(CredentialsEntity, { - where: { isGlobal: true }, + where: { isGlobal: true, usageScope: 'project' }, relations: { shared: true }, }); }); @@ -418,6 +525,7 @@ describe('CredentialsFinderService', () => { expect(credentialsRepository.find).toHaveBeenCalledWith({ where: { isGlobal: false, + usageScope: 'project', shared: { role: In(['custom:cred-admin-789']), project: { @@ -481,7 +589,7 @@ describe('CredentialsFinderService', () => { ]); expect(sharedCredentialsRepository.findCredentialsWithOptions).toHaveBeenCalledWith( - {}, + { credentials: { usageScope: 'project' } }, undefined, ); expect(roleService.rolesWithScope).not.toHaveBeenCalled(); @@ -504,6 +612,7 @@ describe('CredentialsFinderService', () => { expect(roleService.rolesWithScope).toHaveBeenCalledWith('credential', ['credential:read']); expect(sharedCredentialsRepository.findCredentialsWithOptions).toHaveBeenCalledWith( { + credentials: { usageScope: 'project' }, role: In(['credential:owner', 'credential:user']), project: { projectRelations: { @@ -564,7 +673,7 @@ describe('CredentialsFinderService', () => { ); expect(credentialsRepository.manager.find).toHaveBeenCalledWith(CredentialsEntity, { - where: { isGlobal: true }, + where: { isGlobal: true, usageScope: 'project' }, relations: { shared: true }, }); expect(result).toHaveLength(3); @@ -694,7 +803,7 @@ describe('CredentialsFinderService', () => { ); expect(mockFind).toHaveBeenCalledWith(CredentialsEntity, { - where: { isGlobal: true }, + where: { isGlobal: true, usageScope: 'project' }, relations: { shared: true }, }); }); @@ -735,7 +844,10 @@ describe('CredentialsFinderService', () => { expect(roleService.rolesWithScope).not.toHaveBeenCalled(); expect(sharedCredentialsRepository.find).toHaveBeenCalledWith({ select: { credentialsId: true }, - where: { credentialsId: In(ids) }, + where: { + credentialsId: In(ids), + credentials: { usageScope: 'project' }, + }, }); }); @@ -757,6 +869,7 @@ describe('CredentialsFinderService', () => { select: { credentialsId: true }, where: { credentialsId: In(ids), + credentials: { usageScope: 'project' }, role: In(['credential:owner', 'credential:user']), project: { projectRelations: { @@ -788,7 +901,7 @@ describe('CredentialsFinderService', () => { expect(result).toEqual(new Set(['cred-1', 'global-1'])); expect(credentialsRepository.find).toHaveBeenCalledWith({ - where: { id: In(ids), isGlobal: true }, + where: { id: In(ids), isGlobal: true, usageScope: 'project' }, select: ['id'], }); }); @@ -944,6 +1057,7 @@ describe('CredentialsFinderService', () => { expect(credentialsRepository.find).toHaveBeenCalledWith({ where: { isGlobal: false, + usageScope: 'project', shared: { role: In([]), project: { @@ -996,6 +1110,7 @@ describe('CredentialsFinderService', () => { expect(credentialsRepository.find).toHaveBeenCalledWith({ where: { isGlobal: false, + usageScope: 'project', shared: { role: In(['project:admin']), // Uses what RoleService returned for credential namespace project: { @@ -1071,6 +1186,7 @@ describe('CredentialsFinderService', () => { where: { id: credentialId, isGlobal: true, + usageScope: 'project', }, relations: undefined, }); @@ -1090,6 +1206,7 @@ describe('CredentialsFinderService', () => { where: { id: credentialId, isGlobal: true, + usageScope: 'project', }, relations, }); @@ -1105,6 +1222,7 @@ describe('CredentialsFinderService', () => { where: { id: 'non-existent-id', isGlobal: true, + usageScope: 'project', }, relations: undefined, }); diff --git a/packages/cli/src/services/__tests__/import.service.test.ts b/packages/cli/src/services/__tests__/import.service.test.ts index aa9524d68e7..c17733553fa 100644 --- a/packages/cli/src/services/__tests__/import.service.test.ts +++ b/packages/cli/src/services/__tests__/import.service.test.ts @@ -117,6 +117,19 @@ describe('ImportService', () => { ); }); + describe('initRecords', () => { + it('loads only credentials available to workflows', async () => { + vi.mocked(mockCredentialsRepository.find).mockResolvedValue([]); + vi.mocked(mockTagRepository.find).mockResolvedValue([]); + + await importService.initRecords(); + + expect(mockCredentialsRepository.find).toHaveBeenCalledWith({ + where: { usageScope: 'project' }, + }); + }); + }); + describe('isTableEmpty', () => { it('should return true for empty table', async () => { const mockQueryBuilder = { diff --git a/packages/cli/src/services/import.service.ts b/packages/cli/src/services/import.service.ts index 0dbad9e150d..f3c53c41e02 100644 --- a/packages/cli/src/services/import.service.ts +++ b/packages/cli/src/services/import.service.ts @@ -76,7 +76,9 @@ export class ImportService { ) {} async initRecords() { - this.dbCredentials = await this.credentialsRepository.find(); + this.dbCredentials = await this.credentialsRepository.find({ + where: { usageScope: 'project' }, + }); this.dbTags = await this.tagRepository.find(); } diff --git a/packages/cli/src/workflows/workflow-validation.service.ts b/packages/cli/src/workflows/workflow-validation.service.ts index 91f50689644..05e9ca02b71 100644 --- a/packages/cli/src/workflows/workflow-validation.service.ts +++ b/packages/cli/src/workflows/workflow-validation.service.ts @@ -314,7 +314,7 @@ export class WorkflowValidationService { } const resolvableCredentials = await this.credentialsRepository.find({ - where: { id: In([...credentialIds]), isResolvable: true }, + where: { id: In([...credentialIds]), isResolvable: true, usageScope: 'project' }, select: ['id', 'name'], }); diff --git a/packages/cli/src/workflows/workflow.service.ee.ts b/packages/cli/src/workflows/workflow.service.ee.ts index 4e7414d41f0..a46a0e2d3f5 100644 --- a/packages/cli/src/workflows/workflow.service.ee.ts +++ b/packages/cli/src/workflows/workflow.service.ee.ts @@ -365,6 +365,7 @@ export class EnterpriseWorkflowService { where: { id: In(Array.from(credentialIdToWorkflowIds.keys())), isResolvable: true, + usageScope: 'project', }, select: ['id'], }); diff --git a/packages/cli/test/integration/ai/ai.api.test.ts b/packages/cli/test/integration/ai/ai.api.test.ts index 6bda21f7169..3a3c3eae517 100644 --- a/packages/cli/test/integration/ai/ai.api.test.ts +++ b/packages/cli/test/integration/ai/ai.api.test.ts @@ -77,6 +77,7 @@ describe('POST /ai/free-credits', () => { 'credential:createEndUser', 'credential:delete', 'credential:list', + 'credential:manageInstance', 'credential:move', 'credential:read', 'credential:share', diff --git a/packages/cli/test/integration/commands/credentials.cmd.test.ts b/packages/cli/test/integration/commands/credentials.cmd.test.ts index d6235d0962c..6573444f353 100644 --- a/packages/cli/test/integration/commands/credentials.cmd.test.ts +++ b/packages/cli/test/integration/commands/credentials.cmd.test.ts @@ -1,15 +1,25 @@ import { getPersonalProject, mockInstance, testDb } from '@n8n/backend-test-utils'; +import { CredentialsEntity, DbLock, DbLockService, InstanceCredentialAssignment } from '@n8n/db'; +import { Container } from '@n8n/di'; import * as fs from 'fs'; import { jsonParse } from 'n8n-workflow'; import { nanoid } from 'nanoid'; +import * as os from 'os'; import * as path from 'path'; import '@/zod-alias-support'; import { ImportCredentialsCommand } from '@/commands/import/credentials'; +import { InstanceCredentialBroker } from '@/credentials/instance-credential-broker'; +import type { InstanceCredentialUse } from '@/credentials/instance-credential-use.registry'; import { LoadNodesAndCredentials } from '@/load-nodes-and-credentials'; import { setupTestCommand } from '@test-integration/utils/test-command'; -import { getAllCredentials, getAllSharedCredentials } from '../shared/db/credentials'; +import { + createCredentials, + encryptCredentialData, + getAllCredentials, + getAllSharedCredentials, +} from '../shared/db/credentials'; import { createMember, createOwner } from '../shared/db/users'; type CredentialFixture = { @@ -24,9 +34,25 @@ const [credentialsFixture] = jsonParse( mockInstance(LoadNodesAndCredentials); const command = setupTestCommand(ImportCredentialsCommand); +const IMPORT_CREDENTIAL_USE = { + id: 'test:credential-import', + credentialTypes: ['aws'], + validate: ({ data }) => { + if (data.region !== 'us-east-1') throw new Error('Invalid imported provider connection'); + }, +} satisfies InstanceCredentialUse; + +beforeAll(() => { + Container.get(InstanceCredentialBroker).registerUse(IMPORT_CREDENTIAL_USE); +}); beforeEach(async () => { - await testDb.truncate(['CredentialsEntity', 'SharedCredentials', 'User']); + await testDb.truncate([ + 'InstanceCredentialAssignment', + 'CredentialsEntity', + 'SharedCredentials', + 'User', + ]); }); test('import:credentials should import a credential', async () => { @@ -425,6 +451,140 @@ test('`import:credential --projectId ...` should fail if the credential already }); }); +test('import:credentials should preserve the usage scope of existing instance credentials', async () => { + await createOwner(); + await createCredentials({ + id: '123', + name: 'instance-model-cred', + type: 'aws', + data: 'encrypted', + usageScope: 'instance', + }); + await command.run(['--input=./test/integration/commands/import-credentials/credentials.json']); + + const after = { + credentials: await getAllCredentials(), + sharings: await getAllSharedCredentials(), + }; + expect(after.credentials).toEqual([ + expect.objectContaining({ id: '123', usageScope: 'instance' }), + ]); + expect(after.sharings).toEqual([]); +}); + +test('import:credentials should reject changing an existing provider connection type', async () => { + await createOwner(); + await createCredentials({ + id: '123', + name: 'instance-model-cred', + type: 'apiKey', + data: 'encrypted', + usageScope: 'instance', + }); + + await expect( + command.run(['--input=./test/integration/commands/import-credentials/credentials.json']), + ).rejects.toThrow('Provider connection type cannot be changed'); +}); + +test('import:credentials should validate an assigned provider connection', async () => { + await createOwner(); + const entity = new CredentialsEntity(); + Object.assign(entity, { + id: '123', + name: 'instance-model-cred', + type: 'aws', + data: { region: 'us-east-1' }, + usageScope: 'instance', + }); + await encryptCredentialData(entity); + entity.id = '123'; + const credential = await createCredentials(entity); + const dbLockService = Container.get(DbLockService); + await dbLockService.withLock(DbLock.INSTANCE_AI_SETTINGS, async (transactionManager) => { + await transactionManager.insert(InstanceCredentialAssignment, { + credentialUseId: IMPORT_CREDENTIAL_USE.id, + credentialId: credential.id, + }); + }); + + let importSettled = false; + let importPromise: ReturnType | undefined; + const withLockSpy = vi.spyOn(dbLockService, 'withLock'); + try { + await dbLockService.withLock(DbLock.INSTANCE_AI_SETTINGS, async () => { + importPromise = command.run([ + '--input=./test/integration/commands/import-credentials/credentials.json', + ]); + void importPromise.then( + () => { + importSettled = true; + }, + () => { + importSettled = true; + }, + ); + await vi.waitFor(() => expect(withLockSpy).toHaveBeenCalledTimes(2)); + expect(importSettled).toBe(false); + }); + } finally { + withLockSpy.mockRestore(); + } + + if (!importPromise) throw new Error('Expected credential import to start'); + const importError: unknown = await importPromise.then( + () => undefined, + (error: unknown) => error, + ); + expect(importError).toBeInstanceOf(Error); + if (!(importError instanceof Error)) throw new Error('Expected credential import to fail'); + expect(importError.message).toContain('Invalid imported provider connection'); + await expect(getAllCredentials()).resolves.toEqual([ + expect.objectContaining({ + id: credential.id, + name: 'instance-model-cred', + data: credential.data, + }), + ]); +}); + +test.each([ + ['null', null], + ['empty', ''], + ['invalid', 'not-encrypted'], +])('import:credentials should reject %s provider connection data', async (_label, data) => { + await createOwner(); + const entity = new CredentialsEntity(); + Object.assign(entity, { + id: '123', + name: 'instance-model-cred', + type: 'aws', + data: { region: 'us-east-1' }, + usageScope: 'instance', + }); + await encryptCredentialData(entity); + const credential = await createCredentials(entity); + const temporaryDirectory = fs.mkdtempSync(path.join(os.tmpdir(), 'n8n-credential-import-')); + const inputPath = path.join(temporaryDirectory, 'credentials.json'); + fs.writeFileSync( + inputPath, + JSON.stringify([{ id: credential.id, name: 'changed', type: credential.type, data }]), + ); + + try { + await expect(command.run([`--input=${inputPath}`])).rejects.toThrow(); + } finally { + fs.rmSync(temporaryDirectory, { recursive: true, force: true }); + } + await expect(getAllCredentials()).resolves.toEqual([ + expect.objectContaining({ + id: credential.id, + name: 'instance-model-cred', + data: credential.data, + }), + ]); +}); + test('`import:credential --projectId ... --userId ...` fails explaining that only one of the options can be used at a time', async () => { await expect( command.run([ diff --git a/packages/cli/test/integration/credentials/credentials.api.test.ts b/packages/cli/test/integration/credentials/credentials.api.test.ts index c1d61ba4f19..732d67ee7d9 100644 --- a/packages/cli/test/integration/credentials/credentials.api.test.ts +++ b/packages/cli/test/integration/credentials/credentials.api.test.ts @@ -268,6 +268,7 @@ describe('GET /credentials', () => { 'credential:createEndUser', 'credential:delete', 'credential:list', + 'credential:manageInstance', 'credential:move', 'credential:read', 'credential:share', @@ -286,6 +287,7 @@ describe('GET /credentials', () => { 'credential:createEndUser', 'credential:delete', 'credential:list', + 'credential:manageInstance', 'credential:move', 'credential:read', 'credential:share', @@ -421,6 +423,7 @@ describe('GET /credentials', () => { 'credential:createEndUser', 'credential:delete', 'credential:list', + 'credential:manageInstance', 'credential:move', 'credential:read', 'credential:share', @@ -439,6 +442,7 @@ describe('GET /credentials', () => { 'credential:createEndUser', 'credential:delete', 'credential:list', + 'credential:manageInstance', 'credential:move', 'credential:read', 'credential:share', @@ -460,6 +464,7 @@ describe('GET /credentials', () => { 'credential:createEndUser', 'credential:delete', 'credential:list', + 'credential:manageInstance', 'credential:move', 'credential:read', 'credential:share', @@ -1267,6 +1272,7 @@ describe('PATCH /credentials/:id', () => { 'credential:createEndUser', 'credential:delete', 'credential:list', + 'credential:manageInstance', 'credential:move', 'credential:read', 'credential:share', diff --git a/packages/cli/test/integration/credentials/instance-credentials.api.test.ts b/packages/cli/test/integration/credentials/instance-credentials.api.test.ts new file mode 100644 index 00000000000..788f972df3b --- /dev/null +++ b/packages/cli/test/integration/credentials/instance-credentials.api.test.ts @@ -0,0 +1,194 @@ +import { mockInstance, randomCredentialPayload, testDb } from '@n8n/backend-test-utils'; +import type { ICredentialsDb, User } from '@n8n/db'; +import { + CredentialsEntity, + CredentialsRepository, + InstanceCredentialAssignmentRepository, + SharedCredentialsRepository, +} from '@n8n/db'; +import { Container } from '@n8n/di'; + +import { CredentialsService } from '@/credentials/credentials.service'; +import { InstanceCredentialBroker } from '@/credentials/instance-credential-broker'; +import type { InstanceCredentialUse } from '@/credentials/instance-credential-use.registry'; +import { UnprocessableRequestError } from '@/errors/response-errors/unprocessable.error'; +import { ExternalHooks } from '@/external-hooks'; + +import { + createCredentials, + decryptCredentialData, + encryptCredentialData, +} from '../shared/db/credentials'; +import { createMember, createOwner } from '../shared/db/users'; +import type { SuperAgentTest } from '../shared/types'; +import { initCredentialsTypes, setupTestServer } from '../shared/utils'; + +const externalHooks = mockInstance(ExternalHooks); +const testServer = setupTestServer({ endpointGroups: ['credentials'] }); +const TEST_CREDENTIAL_USE = { + id: 'test:instance-credential', + credentialTypes: ['httpHeaderAuth'], + validate: ({ data }) => { + if (data.name !== 'x-api-key') throw new UnprocessableRequestError('Invalid header name'); + }, +} satisfies InstanceCredentialUse; + +let owner: User; +let member: User; +let authOwnerAgent: SuperAgentTest; +let authMemberAgent: SuperAgentTest; + +beforeAll(async () => { + await initCredentialsTypes(); + Container.get(InstanceCredentialBroker).registerUse(TEST_CREDENTIAL_USE); + owner = await createOwner(); + member = await createMember(); + authOwnerAgent = testServer.authAgentFor(owner); + authMemberAgent = testServer.authAgentFor(member); +}); + +beforeEach(async () => { + externalHooks.run.mockReset(); + await testDb.truncate(['InstanceCredentialAssignment', 'SharedCredentials', 'CredentialsEntity']); +}); + +async function createInstanceCredential(payload = randomCredentialPayload()) { + const entity = new CredentialsEntity(); + Object.assign(entity, payload, { usageScope: 'instance' }); + await encryptCredentialData(entity); + return await createCredentials(entity); +} + +describe('instance credentials', () => { + test('owner creates one without any sharing rows', async () => { + const payload = { ...randomCredentialPayload(), usageScope: 'instance' }; + + const response = await authOwnerAgent.post('/credentials').send(payload); + + expect(response.statusCode).toBe(200); + const { id } = response.body.data; + await expect( + Container.get(SharedCredentialsRepository).findBy({ credentialsId: id }), + ).resolves.toEqual([]); + }); + + test('member cannot create one', async () => { + const payload = { ...randomCredentialPayload(), usageScope: 'instance' }; + + await authMemberAgent.post('/credentials').send(payload).expect(403); + }); + + test('never appear in the credentials list', async () => { + const credential = await createInstanceCredential(); + + for (const agent of [authOwnerAgent, authMemberAgent]) { + const response = await agent.get('/credentials').expect(200); + const ids = response.body.data.map((c: { id: string }) => c.id); + expect(ids).not.toContain(credential.id); + } + }); + + test('member cannot read, update, or delete one', async () => { + const credential = await createInstanceCredential(); + + await authMemberAgent.get(`/credentials/${credential.id}`).expect(404); + await authMemberAgent + .patch(`/credentials/${credential.id}`) + .send(randomCredentialPayload()) + .expect(404); + await authMemberAgent.delete(`/credentials/${credential.id}`).expect(404); + }); + + test('owner reads one without exposing plain secrets', async () => { + const payload = randomCredentialPayload(); + const credential = await createInstanceCredential(payload); + + const response = await authOwnerAgent + .get(`/credentials/${credential.id}`) + .query({ includeData: true }) + .expect(200); + + expect(response.body.data.id).toBe(credential.id); + expect(response.body.data.data?.accessToken).not.toBe(payload.data.accessToken); + }); + + test('owner can delete one', async () => { + const credential = await createInstanceCredential(); + + await authOwnerAgent.delete(`/credentials/${credential.id}`).expect(200); + }); + + describe('assigned to an instance credential use', () => { + const sandboxPayload = { + name: 'AI Assistant sandbox', + type: 'httpHeaderAuth', + data: { name: 'x-api-key', value: 'secret' }, + }; + + async function createAssignedSandboxCredential() { + const credential = await createInstanceCredential(sandboxPayload); + await Container.get(InstanceCredentialAssignmentRepository).assignCredential( + TEST_CREDENTIAL_USE.id, + credential.id, + TEST_CREDENTIAL_USE.credentialTypes, + ); + return credential; + } + + test('owner can update an assigned credential', async () => { + const credential = await createAssignedSandboxCredential(); + + await authOwnerAgent + .patch(`/credentials/${credential.id}`) + .send({ ...sandboxPayload, data: { name: 'x-api-key', value: 'rotated' } }) + .expect(200); + + const row = await Container.get(CredentialsRepository).findOneByOrFail({ + id: credential.id, + }); + expect(await decryptCredentialData(row)).toMatchObject({ value: 'rotated' }); + }); + + test('owner cannot update an assigned credential with data rejected by its use', async () => { + const credential = await createAssignedSandboxCredential(); + + await authOwnerAgent + .patch(`/credentials/${credential.id}`) + .send({ ...sandboxPayload, data: { name: 'Authorization', value: 'secret' } }) + .expect(422); + + const row = await Container.get(CredentialsRepository).findOneByOrFail({ + id: credential.id, + }); + expect(await decryptCredentialData(row)).toMatchObject({ name: 'x-api-key' }); + }); + + test('owner cannot update one when a hook makes it invalid for its use', async () => { + const credential = await createAssignedSandboxCredential(); + const invalid = await Container.get(CredentialsService).createEncryptedData({ + id: credential.id, + name: credential.name, + type: credential.type, + data: { name: 'Authorization', value: 'hooked' }, + }); + externalHooks.run.mockImplementation(async (event, hookParameters) => { + if (event !== 'credentials.update') return; + const [hooked] = hookParameters as [ICredentialsDb]; + hooked.data = invalid.data; + }); + + await authOwnerAgent + .patch(`/credentials/${credential.id}`) + .send({ ...sandboxPayload, data: { name: 'x-api-key', value: 'rotated' } }) + .expect(422); + + const row = await Container.get(CredentialsRepository).findOneByOrFail({ + id: credential.id, + }); + expect(await decryptCredentialData(row)).toMatchObject({ + name: 'x-api-key', + value: 'secret', + }); + }); + }); +}); diff --git a/packages/cli/test/integration/database/services/db-lock.service.test.ts b/packages/cli/test/integration/database/services/db-lock.service.test.ts index c6c04348544..c7dd46bd1ca 100644 --- a/packages/cli/test/integration/database/services/db-lock.service.test.ts +++ b/packages/cli/test/integration/database/services/db-lock.service.test.ts @@ -1,9 +1,16 @@ import { testDb } from '@n8n/backend-test-utils'; import { GlobalConfig } from '@n8n/config'; -import { DbConnectionOptions, DbLock, DbLockService } from '@n8n/db'; +import { + CredentialsRepository, + DbConnectionOptions, + DbLock, + DbLockService, + SettingsRepository, +} from '@n8n/db'; import { Container } from '@n8n/di'; import { DataSource } from '@n8n/typeorm'; import { OperationalError, sleep } from 'n8n-workflow'; +import { randomUUID } from 'node:crypto'; let dbLockService: DbLockService; let isPostgres: boolean; @@ -61,6 +68,82 @@ describe('DbLockService', () => { }), ).rejects.toThrow('rollback me'); }); + + it('should roll back credential writes when the settings write fails', async () => { + const credentialsRepository = Container.get(CredentialsRepository); + const settingsRepository = Container.get(SettingsRepository); + const oldCredential = await credentialsRepository.save( + credentialsRepository.create({ + id: randomUUID(), + name: 'Old provider connection', + type: 'openAiApi', + data: 'old-encrypted', + usageScope: 'instance', + }), + ); + const newCredentialId = randomUUID(); + const settingsKey = `test.atomic-settings.${randomUUID()}`; + + await expect( + dbLockService.withLockContext(DbLock.TEST, async (ctx) => { + await credentialsRepository.saveInstanceCredential( + credentialsRepository.create({ + id: newCredentialId, + name: 'Atomic provider connection', + type: 'openAiApi', + data: 'encrypted', + usageScope: 'instance', + }), + ctx, + ); + await credentialsRepository.deleteInstanceCredentialIfUnassigned(oldCredential.id, ctx); + await settingsRepository.upsertByKey(settingsKey, '{}', false, ctx); + throw new Error('rollback both'); + }), + ).rejects.toThrow('rollback both'); + + expect(await credentialsRepository.findOneBy({ id: oldCredential.id })).not.toBeNull(); + expect(await credentialsRepository.findOneBy({ id: newCredentialId })).toBeNull(); + expect(await settingsRepository.findByKey(settingsKey)).toBeNull(); + }); + + it('should roll back an in-place credential update when the settings write fails', async () => { + const credentialsRepository = Container.get(CredentialsRepository); + const settingsRepository = Container.get(SettingsRepository); + const credential = await credentialsRepository.save( + credentialsRepository.create({ + id: randomUUID(), + name: 'Original provider connection', + type: 'openAiApi', + data: 'old-encrypted', + usageScope: 'instance', + }), + ); + const settingsKey = `test.atomic-settings.${randomUUID()}`; + + await expect( + dbLockService.withLockContext(DbLock.TEST, async (ctx) => { + await credentialsRepository.updateInstanceCredential( + credential.id, + { + id: credential.id, + name: 'Updated provider connection', + type: credential.type, + data: 'new-encrypted', + }, + ctx, + ); + await settingsRepository.upsertByKey(settingsKey, '{}', false, ctx); + throw new Error('rollback both'); + }), + ).rejects.toThrow('rollback both'); + + expect(await credentialsRepository.findOneByOrFail({ id: credential.id })).toMatchObject({ + name: 'Original provider connection', + data: 'old-encrypted', + }); + expect(await settingsRepository.findByKey(settingsKey)).toBeNull(); + }); }); describe('tryWithLock', () => { diff --git a/packages/frontend/@n8n/i18n/src/locales/en.json b/packages/frontend/@n8n/i18n/src/locales/en.json index 1dca1c35ea2..7910b5c8ef4 100644 --- a/packages/frontend/@n8n/i18n/src/locales/en.json +++ b/packages/frontend/@n8n/i18n/src/locales/en.json @@ -5008,7 +5008,7 @@ "settings.n8nConnect.wallet.topUp": "Top up balance", "settings.n8nConnect.usage.refresh.tooltip": "Refresh usage records", "settings.n8nAgent": "AI Assistant", - "settings.n8nAgent.description": "Enable or disable AI Assistant for this instance and control permissions", + "settings.n8nAgent.description": "Configure AI Assistant for everyone on this instance", "settings.agentBuilder.title": "Agents", "settings.agentBuilder.description": "Choose which model the agent builder runs on. By default it uses the n8n AI assistant; admins can switch to a custom provider and credential.", "settings.agentBuilder.section.model": "Builder model", @@ -5036,15 +5036,42 @@ "agents.modelSelector.freeCredits.description": "Get {credits} free OpenAI API credits. Try it with gpt-5-mini.", "settings.n8nAgent.enable.label": "Enable AI Assistant", "settings.n8nAgent.enable.description": "Enables the feature for all users in the instance", - "settings.n8nAgent.computerUse.label": "Enable Computer Use for AI Assistant", - "settings.n8nAgent.computerUse.description": "Allows users on this instance to connect AI Assistant to their computer to work with files and the browser", + "settings.n8nAgent.status.label": "AI Assistant status", + "settings.n8nAgent.status.description": "Let users chat with AI Assistant to build and manage workflows", + "settings.n8nAgent.status.enabled": "Enabled", + "settings.n8nAgent.status.manage": "Manage AI Assistant status", + "settings.n8nAgent.status.disable": "Disable", + "settings.n8nAgent.status.disable.title": "Disable AI Assistant?", + "settings.n8nAgent.status.disable.description": "This prevents everyone on this instance from using AI Assistant. Existing conversations won't be deleted. You can turn it back on later.", + "settings.n8nAgent.empty.title": "Build and manage workflows with AI Assistant", + "settings.n8nAgent.empty.description": "Let users ask AI Assistant to create, update, and run workflows across this instance.", + "settings.n8nAgent.empty.enable": "Enable AI Assistant", + "settings.n8nAgent.credentials.label": "AI service credentials", + "settings.n8nAgent.credentials.description": "Connect the model, sandbox, and web search services AI Assistant uses", + "settings.n8nAgent.credentials.title": "AI service credentials", + "settings.n8nAgent.credentials.pageDescription": "Create or select provider connections for the services AI Assistant uses. These connections aren't available in workflows.", + "settings.n8nAgent.credentials.back": "Back to AI Assistant", + "settings.n8nAgent.credentials.edit": "Edit", + "settings.n8nAgent.credentials.model.title": "Model", + "settings.n8nAgent.credentials.model.description": "Choose the model provider credential used for everyone on this instance", + "settings.n8nAgent.credentials.unavailable.title": "Credential settings aren't available", + "settings.n8nAgent.credentials.unavailable.description": "This deployment manages AI Assistant credentials outside n8n.", + "settings.n8nAgent.capabilities.title": "Capabilities", + "settings.n8nAgent.capabilities.description": "Choose which tools users can connect to AI Assistant", + "settings.n8nAgent.computerUse.label": "Computer use", + "settings.n8nAgent.computerUse.description": "Let users connect AI Assistant to their computer to work with files and the browser", "settings.n8nAgent.computerUse.disabled.warning": "Computer use has been disabled by your administrator", - "settings.n8nAgent.browserUse.label": "Enable Browser Use for AI Assistant", - "settings.n8nAgent.browserUse.description": "Allows users on this instance to connect AI Assistant to their browser to view pages and automate tasks", - "settings.n8nAgent.mcpAccess.label": "Allow users to extend AI Assistant with MCP Servers", - "settings.n8nAgent.mcpAccess.description": "Allows users on this instance to connect AI Assistant to MCP servers from the registry", + "settings.n8nAgent.browserUse.label": "Browser use", + "settings.n8nAgent.browserUse.description": "Let users connect AI Assistant to their browser to view pages and automate tasks", + "settings.n8nAgent.mcpAccess.label": "MCP server access", + "settings.n8nAgent.mcpAccess.description": "Let users extend AI Assistant with MCP servers from the registry", + "settings.n8nAgent.modelCredential.label": "Model credential", + "settings.n8nAgent.modelCredential.description": "Choose a provider credential or create one", + "settings.n8nAgent.modelCredential.placeholder": "Select a credential", + "settings.n8nAgent.modelCredential.none": "Use environment configuration", + "settings.n8nAgent.modelCredential.createNew": "Create credential", "settings.n8nAgent.permissions.title": "Permissions", - "settings.n8nAgent.permissions.description": "Control which actions need approval before agent executes them. These settings don't grant users any extra permissions", + "settings.n8nAgent.permissions.description": "Choose which actions AI Assistant can take automatically. These settings don't give users any extra permissions", "settings.n8nAgent.permissions.alwaysAllow": "Always allow", "settings.n8nAgent.permissions.needsApproval": "Needs approval", "settings.n8nAgent.permissions.blocked": "Blocked", diff --git a/packages/frontend/editor-ui/src/Interface.ts b/packages/frontend/editor-ui/src/Interface.ts index c7340abab06..ca347706363 100644 --- a/packages/frontend/editor-ui/src/Interface.ts +++ b/packages/frontend/editor-ui/src/Interface.ts @@ -677,6 +677,7 @@ export interface NewCredentialsModal extends ModalState { contextNode?: INodeUi; hideAskAssistant?: boolean; appendToBody?: boolean; + usageScope?: 'project' | 'instance'; /** Behavior for the Instance AI credential setup-help button, supplied by the * surface that opened the modal (an editor capability, or the credentials list). * Resolves to whether the credential modal should close (false keeps it open for diff --git a/packages/frontend/editor-ui/src/app/stores/ui.store.ts b/packages/frontend/editor-ui/src/app/stores/ui.store.ts index 1171b6ef0fe..efcb4724d4c 100644 --- a/packages/frontend/editor-ui/src/app/stores/ui.store.ts +++ b/packages/frontend/editor-ui/src/app/stores/ui.store.ts @@ -562,6 +562,7 @@ export const useUIStore = defineStore(STORES.UI, () => { appendToBody?: boolean; closeOnSave?: boolean; instanceAiCredentialHelp?: NewCredentialsModal['instanceAiCredentialHelp']; + usageScope?: NewCredentialsModal['usageScope']; } = {}, ) => { setActiveId(CREDENTIAL_EDIT_MODAL_KEY, type); @@ -577,6 +578,7 @@ export const useUIStore = defineStore(STORES.UI, () => { hideAskAssistant: options.hideAskAssistant, appendToBody: options.appendToBody, instanceAiCredentialHelp: options.instanceAiCredentialHelp, + usageScope: options.usageScope, } as NewCredentialsModal; setMode(CREDENTIAL_EDIT_MODAL_KEY, 'new'); openModal(CREDENTIAL_EDIT_MODAL_KEY); diff --git a/packages/frontend/editor-ui/src/features/ai/instanceAi/__tests__/SettingsInstanceAiView.test.ts b/packages/frontend/editor-ui/src/features/ai/instanceAi/__tests__/SettingsInstanceAiView.test.ts index dd146ed8ad7..bee209e87b3 100644 --- a/packages/frontend/editor-ui/src/features/ai/instanceAi/__tests__/SettingsInstanceAiView.test.ts +++ b/packages/frontend/editor-ui/src/features/ai/instanceAi/__tests__/SettingsInstanceAiView.test.ts @@ -57,6 +57,11 @@ const { mcpConnectionsExperimentMock, computerUseExperimentMock, browserUseExper computerUseExperimentMock: vi.fn(), })); +vi.mock('vue-router', async (importOriginal) => ({ + ...(await importOriginal()), + useRouter: () => ({ push: vi.fn() }), +})); + vi.mock('@/experiments/instanceAiMcpConnections', () => ({ useInstanceAiMcpConnectionsExperiment: mcpConnectionsExperimentMock, })); @@ -69,19 +74,7 @@ vi.mock('@/experiments/instanceAiComputerUse', () => ({ useInstanceAiComputerUseExperiment: computerUseExperimentMock, })); -const renderComponent = createComponentRenderer(SettingsInstanceAiView, { - global: { - stubs: { - // jsdom can't drive element-plus's ElSwitch (it touches a null input ref), - // so stub it with a button that emits the toggled value. - ElSwitch: { - props: ['modelValue', 'disabled'], - template: - '