diff --git a/packages/cli/src/modules/external-secrets.ee/__tests__/secrets-providers-connections.service.ee.test.ts b/packages/cli/src/modules/external-secrets.ee/__tests__/secrets-providers-connections.service.ee.test.ts index 308ba22ab60..9702bd90de8 100644 --- a/packages/cli/src/modules/external-secrets.ee/__tests__/secrets-providers-connections.service.ee.test.ts +++ b/packages/cli/src/modules/external-secrets.ee/__tests__/secrets-providers-connections.service.ee.test.ts @@ -1,9 +1,11 @@ import { mockLogger } from '@n8n/backend-test-utils'; import type { + ProjectSecretsProviderAccess, ProjectSecretsProviderAccessRepository, SecretsProviderConnection, SecretsProviderConnectionRepository, } from '@n8n/db'; +import { In } from '@n8n/typeorm'; import { mock } from 'jest-mock-extended'; import { CREDENTIAL_BLANKING_VALUE, type IDataObject, type INodeProperties } from 'n8n-workflow'; import { NotFoundError } from '@/errors/response-errors/not-found.error'; @@ -386,7 +388,7 @@ describe('SecretsProvidersConnectionsService', () => { it('should sync provider connection after deleteConnection', async () => { mockRepository.findOne.mockResolvedValueOnce(savedConnection); - mockRepository.remove.mockResolvedValue(savedConnection); + mockRepository.remove.mockResolvedValueOnce(savedConnection); await service.deleteConnection('my-aws', 'user-123'); @@ -394,6 +396,61 @@ describe('SecretsProvidersConnectionsService', () => { }); }); + describe('cleanupConnectionsForProjectDeletion', () => { + it('runs owner and non-owner mutations inside a single transaction', async () => { + const entityManager = { + delete: jest.fn().mockResolvedValueOnce(undefined), + update: jest.fn().mockResolvedValueOnce(undefined), + }; + const transaction = jest.fn( + async (callback: (em: typeof entityManager) => Promise) => + await callback(entityManager), + ); + Object.defineProperty(mockRepository, 'manager', { + value: { transaction }, + configurable: true, + }); + + mockProjectAccessRepository.findByProjectId.mockResolvedValue([ + mock({ + projectId: 'project-1', + role: 'secretsProviderConnection:owner', + secretsProviderConnectionId: 1, + secretsProviderConnection: { providerKey: 'provider-a' }, + }), + mock({ + projectId: 'project-1', + role: 'secretsProviderConnection:user', + secretsProviderConnectionId: 2, + secretsProviderConnection: { providerKey: 'provider-b' }, + }), + ]); + + await service.cleanupConnectionsForProjectDeletion('project-1'); + + expect(transaction).toHaveBeenCalledTimes(1); + expect(entityManager.delete).toHaveBeenNthCalledWith(1, mockRepository.target, { + id: In([1]), + }); + expect(entityManager.delete).toHaveBeenNthCalledWith(2, mockProjectAccessRepository.target, { + projectId: 'project-1', + secretsProviderConnectionId: In([2]), + }); + expect(entityManager.update).toHaveBeenCalledWith( + mockRepository.target, + { id: In([2]) }, + { isEnabled: false }, + ); + + expect(mockRepository.delete).not.toHaveBeenCalled(); + expect(mockRepository.update).not.toHaveBeenCalled(); + expect(mockProjectAccessRepository.delete).not.toHaveBeenCalled(); + expect(mockExternalSecretsManager.syncProviderConnection).toHaveBeenCalledTimes(2); + expect(mockExternalSecretsManager.syncProviderConnection).toHaveBeenCalledWith('provider-a'); + expect(mockExternalSecretsManager.syncProviderConnection).toHaveBeenCalledWith('provider-b'); + }); + }); + describe('event emissions', () => { const connectionWithProjects = { id: 1, diff --git a/packages/cli/src/modules/external-secrets.ee/secrets-providers-connections.service.ee.ts b/packages/cli/src/modules/external-secrets.ee/secrets-providers-connections.service.ee.ts index f3a61374a3c..0fd32377607 100644 --- a/packages/cli/src/modules/external-secrets.ee/secrets-providers-connections.service.ee.ts +++ b/packages/cli/src/modules/external-secrets.ee/secrets-providers-connections.service.ee.ts @@ -16,6 +16,7 @@ import { SecretsProviderConnectionRepository, } from '@n8n/db'; import { Service } from '@n8n/di'; +import { In } from '@n8n/typeorm'; import { Cipher } from 'n8n-core'; import type { IDataObject } from 'n8n-workflow'; import { jsonParse } from 'n8n-workflow'; @@ -380,6 +381,54 @@ export class SecretsProvidersConnectionsService { }; } + /** + * Cleans up external-secrets connections when a project is deleted. + * - If this project owns the connection, delete it entirely (access rows cascade). + * - Otherwise, remove this project's access and disable the shared connection. + * - Sync each affected provider key once after cleanup. + */ + async cleanupConnectionsForProjectDeletion(projectId: string): Promise { + const accessEntries = await this.projectAccessRepository.findByProjectId(projectId); + const providerKeysToSync = new Set(); + const ownerConnectionIds = new Set(); + const nonOwnerConnectionIds = new Set(); + + for (const access of accessEntries) { + providerKeysToSync.add(access.secretsProviderConnection.providerKey); + + if (access.role === 'secretsProviderConnection:owner') { + ownerConnectionIds.add(access.secretsProviderConnectionId); + } else { + nonOwnerConnectionIds.add(access.secretsProviderConnectionId); + } + } + + // Wrap deletion + update ops in a transaction for consistency + await this.repository.manager.transaction(async (entityManager) => { + if (ownerConnectionIds.size > 0) { + // Delete owned connections entirely; DB cascade removes access entries + await entityManager.delete(this.repository.target, { id: In([...ownerConnectionIds]) }); + } + + if (nonOwnerConnectionIds.size > 0) { + // Remove only this project's access and disable shared connections + await entityManager.delete(this.projectAccessRepository.target, { + projectId, + secretsProviderConnectionId: In([...nonOwnerConnectionIds]), + }); + await entityManager.update( + this.repository.target, + { id: In([...nonOwnerConnectionIds]) }, + { isEnabled: false }, + ); + } + }); + + for (const providerKey of providerKeysToSync) { + await this.externalSecretsManager.syncProviderConnection(providerKey); + } + } + private encryptConnectionSettings(settings: IDataObject): string { return this.cipher.encrypt(settings); } diff --git a/packages/cli/src/modules/insights/__tests__/insights.service.integration.test.ts b/packages/cli/src/modules/insights/__tests__/insights.service.integration.test.ts index 80b85bd2ae4..e94128f5475 100644 --- a/packages/cli/src/modules/insights/__tests__/insights.service.integration.test.ts +++ b/packages/cli/src/modules/insights/__tests__/insights.service.integration.test.ts @@ -419,7 +419,7 @@ describe('InsightsService (Integration)', () => { }); } - const startDate = now.minus({ days: 14 }).toJSDate(); + const startDate = now.minus({ days: 14 }).startOf('day').toJSDate(); const endDate = now.minus({ days: 1 }).toJSDate(); // ACT @@ -486,7 +486,7 @@ describe('InsightsService (Integration)', () => { }); } - const startDate = now.minus({ days: 14 }).toJSDate(); + const startDate = now.minus({ days: 14 }).startOf('day').toJSDate(); // ACT const byWorkflow = await insightsService.getInsightsByWorkflow({ @@ -513,7 +513,7 @@ describe('InsightsService (Integration)', () => { }); } - const startDate = now.minus({ days: 14 }).toJSDate(); + const startDate = now.minus({ days: 14 }).startOf('day').toJSDate(); // ACT const byWorkflow = await insightsService.getInsightsByWorkflow({ @@ -585,7 +585,7 @@ describe('InsightsService (Integration)', () => { }); } - const startDate = now.minus({ days: 14 }).toJSDate(); + const startDate = now.minus({ days: 14 }).startOf('day').toJSDate(); // ACT const byWorkflow = await insightsService.getInsightsByWorkflow({ @@ -683,7 +683,7 @@ describe('InsightsService (Integration)', () => { periodStart: now.minus({ days: 30 }), }); - const startDate = now.minus({ days: 14 }).toJSDate(); + const startDate = now.minus({ days: 14 }).startOf('day').toJSDate(); const byTime = await insightsService.getInsightsByTime({ startDate, @@ -746,7 +746,7 @@ describe('InsightsService (Integration)', () => { }); } - const startDate = now.minus({ days: 14 }).toJSDate(); + const startDate = now.minus({ days: 14 }).startOf('day').toJSDate(); // ACT const byTime = await insightsService.getInsightsByTime({ @@ -824,7 +824,7 @@ describe('InsightsService (Integration)', () => { }); } - const startDate = now.minus({ days: 14 }).toJSDate(); + const startDate = now.minus({ days: 14 }).startOf('day').toJSDate(); // ACT const byTime = await insightsService.getInsightsByTime({ @@ -904,7 +904,7 @@ describe('InsightsService (Integration)', () => { }); } - const startDate = now.minus({ days: 14 }).toJSDate(); + const startDate = now.minus({ days: 14 }).startOf('day').toJSDate(); // ACT const byTime = await insightsService.getInsightsByTime({ diff --git a/packages/cli/src/services/project.service.ee.ts b/packages/cli/src/services/project.service.ee.ts index f85f6e64ebd..3237bf5a574 100644 --- a/packages/cli/src/services/project.service.ee.ts +++ b/packages/cli/src/services/project.service.ee.ts @@ -97,6 +97,12 @@ export class ProjectService { ); } + private get secretsProvidersConnectionsService() { + return import('@/modules/external-secrets.ee/secrets-providers-connections.service.ee').then( + ({ SecretsProvidersConnectionsService }) => Container.get(SecretsProvidersConnectionsService), + ); + } + async deleteProject( user: User, projectId: string, @@ -192,10 +198,16 @@ export class ProjectService { } } - // 7. delete project + // 7. delete secrets providers connections that are owned by this project + if (this.moduleRegistry.isActive('external-secrets')) { + const secretsProvidersConnectionsService = await this.secretsProvidersConnectionsService; + await secretsProvidersConnectionsService.cleanupConnectionsForProjectDeletion(project.id); + } + + // 8. delete project await this.projectRepository.remove(project); - // 8. delete project relations + // 9. delete project relations // Cascading deletes take care of this. } diff --git a/packages/cli/test/integration/external-secrets/project-deletion.api.test.ts b/packages/cli/test/integration/external-secrets/project-deletion.api.test.ts new file mode 100644 index 00000000000..29e95712f4f --- /dev/null +++ b/packages/cli/test/integration/external-secrets/project-deletion.api.test.ts @@ -0,0 +1,272 @@ +import { LicenseState } from '@n8n/backend-common'; +import { + createTeamProject, + linkUserToProject, + mockInstance, + testDb, +} from '@n8n/backend-test-utils'; +import { + ProjectRepository, + ProjectSecretsProviderAccessRepository, + SecretsProviderConnectionRepository, +} from '@n8n/db'; +import { Container } from '@n8n/di'; +import { mock } from 'jest-mock-extended'; + +import { ExternalSecretsConfig } from '@/modules/external-secrets.ee/external-secrets.config'; +import { ExternalSecretsProviders } from '@/modules/external-secrets.ee/external-secrets-providers.ee'; + +import { MockProviders, createDummyProvider } from '../../shared/external-secrets/utils'; +import { createMember, createOwner } from '../shared/db/users'; +import { setupTestServer } from '../shared/utils'; + +const mockProvidersInstance = new MockProviders(); +mockProvidersInstance.setProviders({ + awsSecretsManager: createDummyProvider({ name: 'awsSecretsManager' }), +}); +mockInstance(ExternalSecretsProviders, mockProvidersInstance); + +const licenseMock = mock(); +licenseMock.isLicensed.mockReturnValue(true); +Container.set(LicenseState, licenseMock); + +mockInstance(ExternalSecretsConfig, { + externalSecretsForProjects: true, +}); + +describe('Project deletion with external secrets', () => { + const testServer = setupTestServer({ + endpointGroups: ['project', 'externalSecrets'], + enabledFeatures: [ + 'feat:externalSecrets', + 'feat:advancedPermissions', + 'feat:projectRole:admin', + 'feat:projectRole:editor', + 'feat:projectRole:viewer', + ], + modules: ['external-secrets'], + }); + + let connectionRepository: SecretsProviderConnectionRepository; + let projectAccessRepository: ProjectSecretsProviderAccessRepository; + let projectRepository: ProjectRepository; + + beforeEach(async () => { + connectionRepository = Container.get(SecretsProviderConnectionRepository); + projectAccessRepository = Container.get(ProjectSecretsProviderAccessRepository); + projectRepository = Container.get(ProjectRepository); + await testDb.truncate([ + 'User', + 'Project', + 'SecretsProviderConnection', + 'ProjectSecretsProviderAccess', + ]); + }); + + type TestUser = Awaited>; + + const createGlobalSharedConnection = async ( + owner: TestUser, + projectId: string, + providerKey: string, + ) => { + await testServer + .authAgentFor(owner) + .post('/secret-providers/connections') + .send({ + providerKey, + type: 'awsSecretsManager', + projectIds: [projectId], + settings: { region: 'us-east-1' }, + }) + .expect(200); + }; + + const createProjectConnection = async ( + owner: TestUser, + projectId: string, + providerKey: string, + ) => { + await testServer + .authAgentFor(owner) + .post(`/secret-providers/projects/${projectId}/connections`) + .send({ + providerKey, + type: 'awsSecretsManager', + projectIds: [], + settings: { region: 'us-east-1' }, + }) + .expect(200); + }; + + test('deletes owner-role connection when project is deleted', async () => { + const owner = await createOwner(); + const project = await createTeamProject('Owner Role Project', owner); + + await createProjectConnection(owner, project.id, 'ownerRoleConnection'); + const connection = await connectionRepository.findOneByOrFail({ + providerKey: 'ownerRoleConnection', + }); + + const ownerAccess = await projectAccessRepository.findOneByOrFail({ + projectId: project.id, + secretsProviderConnectionId: connection.id, + }); + expect(ownerAccess.role).toBe('secretsProviderConnection:owner'); + + await testServer.authAgentFor(owner).delete(`/projects/${project.id}`).expect(200); + + const deletedConnection = await connectionRepository.findOneBy({ + providerKey: 'ownerRoleConnection', + }); + expect(deletedConnection).toBeNull(); + }); + + test('disables user-role connection and removes project access when project is deleted', async () => { + const owner = await createOwner(); + const projectAdmin = await createMember(); + const project = await createTeamProject('User Role Project', owner); + await linkUserToProject(projectAdmin, project, 'project:admin'); + + await createGlobalSharedConnection(owner, project.id, 'userRoleConnection'); + const connection = await connectionRepository.findOneByOrFail({ + providerKey: 'userRoleConnection', + }); + + const userAccess = await projectAccessRepository.findOneByOrFail({ + projectId: project.id, + secretsProviderConnectionId: connection.id, + }); + expect(userAccess.role).toBe('secretsProviderConnection:user'); + + await testServer.authAgentFor(projectAdmin).delete(`/projects/${project.id}`).expect(200); + + const remainingConnection = await connectionRepository.findOneBy({ + providerKey: 'userRoleConnection', + }); + expect(remainingConnection).not.toBeNull(); + expect(remainingConnection?.isEnabled).toBe(false); + + const accessEntries = await projectAccessRepository.find({ + where: { secretsProviderConnectionId: connection.id }, + }); + expect(accessEntries).toHaveLength(0); + }); + + test('deletes owner-role connections and disables user-role connections for the same deleted project', async () => { + const owner = await createOwner(); + const project = await createTeamProject('Mixed Role Project', owner); + + await createProjectConnection(owner, project.id, 'mixedOwnerConnection'); + await createGlobalSharedConnection(owner, project.id, 'mixedUserConnection'); + + const userConnection = await connectionRepository.findOneByOrFail({ + providerKey: 'mixedUserConnection', + }); + + await testServer.authAgentFor(owner).delete(`/projects/${project.id}`).expect(200); + + const [deletedOwnerConnection, remainingUserConnection] = await Promise.all([ + connectionRepository.findOneBy({ providerKey: 'mixedOwnerConnection' }), + connectionRepository.findOneBy({ providerKey: 'mixedUserConnection' }), + ]); + + expect(deletedOwnerConnection).toBeNull(); + expect(remainingUserConnection).not.toBeNull(); + expect(remainingUserConnection?.isEnabled).toBe(false); + + const userAccessEntries = await projectAccessRepository.find({ + where: { secretsProviderConnectionId: userConnection.id }, + }); + expect(userAccessEntries).toHaveLength(0); + }); + + test('if owner-connection deletion fails, access rows are not deleted', async () => { + const owner = await createOwner(); + const project = await createTeamProject('Rollback Owner Delete Project', owner); + + await createProjectConnection(owner, project.id, 'rollbackOwnerConnection'); + const connection = await connectionRepository.findOneByOrFail({ + providerKey: 'rollbackOwnerConnection', + }); + + const originalConnectionTarget = connectionRepository.target; + Object.defineProperty(connectionRepository, 'target', { + value: 'does_not_exist_connection_target', + configurable: true, + }); + + try { + await testServer.authAgentFor(owner).delete(`/projects/${project.id}`).expect(500); + } finally { + Object.defineProperty(connectionRepository, 'target', { + value: originalConnectionTarget, + configurable: true, + }); + } + + const [remainingProject, remainingConnection, remainingAccess] = await Promise.all([ + projectRepository.findOneBy({ id: project.id }), + connectionRepository.findOneBy({ providerKey: 'rollbackOwnerConnection' }), + projectAccessRepository.findOneBy({ + projectId: project.id, + secretsProviderConnectionId: connection.id, + }), + ]); + + expect(remainingProject).not.toBeNull(); + expect(remainingConnection).not.toBeNull(); + expect(remainingAccess).not.toBeNull(); + }); + + test('if access deletion fails, owner connection deletion is rolled back', async () => { + const owner = await createOwner(); + const project = await createTeamProject('Rollback Access Delete Project', owner); + + await createProjectConnection(owner, project.id, 'rollbackOwnerConnection2'); + await createGlobalSharedConnection(owner, project.id, 'rollbackUserConnection2'); + + const [ownerConnection, userConnection] = await Promise.all([ + connectionRepository.findOneByOrFail({ + providerKey: 'rollbackOwnerConnection2', + }), + connectionRepository.findOneByOrFail({ + providerKey: 'rollbackUserConnection2', + }), + ]); + + const originalProjectAccessTarget = projectAccessRepository.target; + Object.defineProperty(projectAccessRepository, 'target', { + value: 'does_not_exist_access_target', + configurable: true, + }); + + try { + await testServer.authAgentFor(owner).delete(`/projects/${project.id}`).expect(500); + } finally { + Object.defineProperty(projectAccessRepository, 'target', { + value: originalProjectAccessTarget, + configurable: true, + }); + } + + const [remainingProject, remainingOwnerConnection, remainingOwnerAccess, remainingUserAccess] = + await Promise.all([ + projectRepository.findOneBy({ id: project.id }), + connectionRepository.findOneBy({ providerKey: 'rollbackOwnerConnection2' }), + projectAccessRepository.findOneBy({ + projectId: project.id, + secretsProviderConnectionId: ownerConnection.id, + }), + projectAccessRepository.findOneBy({ + projectId: project.id, + secretsProviderConnectionId: userConnection.id, + }), + ]); + + expect(remainingProject).not.toBeNull(); + expect(remainingOwnerConnection).not.toBeNull(); + expect(remainingOwnerAccess).not.toBeNull(); + expect(remainingUserAccess).not.toBeNull(); + }); +}); diff --git a/packages/cli/test/integration/project.api.test.ts b/packages/cli/test/integration/project.api.test.ts index 3f3a57ecaa5..b58d396d3b1 100644 --- a/packages/cli/test/integration/project.api.test.ts +++ b/packages/cli/test/integration/project.api.test.ts @@ -311,7 +311,7 @@ describe('GET /projects/my-projects', () => { const p = respProjects.find((p) => p.id === project.id)!; expect(p.role).toBe(expected.role); - expect(expected.scopes.every((s) => p.scopes?.includes(s as Scope))).toBe(true); + expect(expected.scopes.every((s) => p.scopes?.includes(s))).toBe(true); } expect(respProjects).not.toContainEqual(expect.objectContaining({ id: personalProject2.id })); @@ -438,7 +438,7 @@ describe('GET /projects/my-projects', () => { const p = respProjects.find((p) => p.id === project.id)!; expect(p.role).toBe(expected.role); - expect(expected.scopes.every((s) => p.scopes?.includes(s as Scope))).toBe(true); + expect(expected.scopes.every((s) => p.scopes?.includes(s))).toBe(true); } expect(respProjects).not.toContainEqual(expect.objectContaining({ id: personalProject1.id })); @@ -656,10 +656,7 @@ describe('PATCH /projects/:projectId', () => { relations: [ { userId: testUser3.id, role: 'project:editor' }, { userId: ownerUser.id, role: 'project:viewer' }, - ] as Array<{ - userId: string; - role: ProjectRole; - }>, + ], }); expect(addResp.status).toBe(201); @@ -714,10 +711,7 @@ describe('PATCH /projects/:projectId', () => { // add a user to the project { userId: userToBeInvited.id, role: 'project:editor' }, // implicitly remove the project editor - ] as Array<{ - userId: string; - role: ProjectRole; - }>, + ], }); //.expect(403); @@ -760,10 +754,7 @@ describe('PATCH /projects/:projectId', () => { .authAgentFor(projectAdmin) .post(`/projects/${teamProject.id}/users`) .send({ - relations: [{ userId: userToBeInvited.id, role }] as Array<{ - userId: string; - role: ProjectRole; - }>, + relations: [{ userId: userToBeInvited.id, role }], }) .expect(400); @@ -851,10 +842,7 @@ describe('PATCH /projects/:projectId', () => { const memberAgent = testServer.authAgentFor(testUser1); const resp = await memberAgent.post(`/projects/${personalProject.id}/users`).send({ - relations: [{ userId: testUser2.id, role: 'project:admin' }] as Array<{ - userId: string; - role: ProjectRole; - }>, + relations: [{ userId: testUser2.id, role: 'project:admin' }], }); expect(resp.status).toBe(403); diff --git a/packages/workflow/package.json b/packages/workflow/package.json index bc1101d75ba..e65cdb03982 100644 --- a/packages/workflow/package.json +++ b/packages/workflow/package.json @@ -69,6 +69,7 @@ "transliteration": "2.3.5", "xml2js": "catalog:", "zod": "catalog:", - "jsonrepair": "catalog:" + "jsonrepair": "catalog:", + "uuid": "catalog:" } } diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 1e93db599c2..5413b644c7f 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -3990,6 +3990,9 @@ importers: transliteration: specifier: 2.3.5 version: 2.3.5 + uuid: + specifier: 'catalog:' + version: 10.0.0 xml2js: specifier: 'catalog:' version: 0.6.2