feat(core): Delete secrets provider connections on project deletion (#26706)

This commit is contained in:
Ali Elkhateeb 2026-03-16 12:08:49 +01:00 committed by GitHub
parent 6023d46fd1
commit 7827ae0e74
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
8 changed files with 412 additions and 30 deletions

View file

@ -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<void>) =>
await callback(entityManager),
);
Object.defineProperty(mockRepository, 'manager', {
value: { transaction },
configurable: true,
});
mockProjectAccessRepository.findByProjectId.mockResolvedValue([
mock<ProjectSecretsProviderAccess>({
projectId: 'project-1',
role: 'secretsProviderConnection:owner',
secretsProviderConnectionId: 1,
secretsProviderConnection: { providerKey: 'provider-a' },
}),
mock<ProjectSecretsProviderAccess>({
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,

View file

@ -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<void> {
const accessEntries = await this.projectAccessRepository.findByProjectId(projectId);
const providerKeysToSync = new Set<string>();
const ownerConnectionIds = new Set<number>();
const nonOwnerConnectionIds = new Set<number>();
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);
}

View file

@ -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({

View file

@ -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.
}

View file

@ -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<LicenseState>();
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<ReturnType<typeof createOwner>>;
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();
});
});

View file

@ -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);

View file

@ -69,6 +69,7 @@
"transliteration": "2.3.5",
"xml2js": "catalog:",
"zod": "catalog:",
"jsonrepair": "catalog:"
"jsonrepair": "catalog:",
"uuid": "catalog:"
}
}

View file

@ -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