Skip to content

Commit

Permalink
feat: Synchronize deletions when pulling from source control (#12170)
Browse files Browse the repository at this point in the history
Co-authored-by: r00gm <raul00gm@gmail.com>
despairblue and r00gm authored Jan 20, 2025
1 parent f167578 commit 967ee4b
Showing 21 changed files with 899 additions and 364 deletions.
2 changes: 1 addition & 1 deletion packages/cli/src/commands/ldap/reset.ts
Original file line number Diff line number Diff line change
@@ -110,7 +110,7 @@ export class Reset extends BaseCommand {
}

for (const credential of ownedCredentials) {
await Container.get(CredentialsService).delete(credential);
await Container.get(CredentialsService).delete(owner, credential.id);
}

await Container.get(AuthProviderSyncHistoryRepository).delete({ providerType: 'ldap' });
2 changes: 1 addition & 1 deletion packages/cli/src/controllers/users.controller.ts
Original file line number Diff line number Diff line change
@@ -240,7 +240,7 @@ export class UsersController {
}

for (const credential of ownedCredentials) {
await this.credentialsService.delete(credential);
await this.credentialsService.delete(userToDelete, credential.id);
}

await this.userService.getManager().transaction(async (trx) => {
2 changes: 1 addition & 1 deletion packages/cli/src/credentials/credentials.controller.ts
Original file line number Diff line number Diff line change
@@ -251,7 +251,7 @@ export class CredentialsController {
);
}

await this.credentialsService.delete(credential);
await this.credentialsService.delete(req.user, credential.id);

this.eventService.emit('credentials-deleted', {
user: req.user,
22 changes: 19 additions & 3 deletions packages/cli/src/credentials/credentials.service.ts
Original file line number Diff line number Diff line change
@@ -406,10 +406,26 @@ export class CredentialsService {
return result;
}

async delete(credentials: CredentialsEntity) {
await this.externalHooks.run('credentials.delete', [credentials.id]);
/**
* Deletes a credential.
*
* 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]);

const credential = await this.sharedCredentialsRepository.findCredentialForUser(
credentialId,
user,
['credential:delete'],
);

if (!credential) {
return;
}

await this.credentialsRepository.remove(credentials);
await this.credentialsRepository.remove(credential);
}

async test(user: User, credentials: ICredentialsDecrypted) {
Original file line number Diff line number Diff line change
@@ -26,6 +26,9 @@ describe('SourceControlImportService', () => {
mock(),
workflowRepository,
mock(),
mock(),
mock(),
mock(),
mock<InstanceSettings>({ n8nFolder: '/mock/n8n' }),
);

Original file line number Diff line number Diff line change
@@ -1,10 +1,19 @@
import type { SourceControlledFile } from '@n8n/api-types';
import { Container } from '@n8n/di';
import { mock } from 'jest-mock-extended';
import { InstanceSettings } from 'n8n-core';

import type { TagEntity } from '@/databases/entities/tag-entity';
import type { User } from '@/databases/entities/user';
import type { Variables } from '@/databases/entities/variables';
import type { TagRepository } from '@/databases/repositories/tag.repository';
import { SourceControlPreferencesService } from '@/environments.ee/source-control/source-control-preferences.service.ee';
import { SourceControlService } from '@/environments.ee/source-control/source-control.service.ee';

import type { SourceControlImportService } from '../source-control-import.service.ee';
import type { ExportableCredential } from '../types/exportable-credential';
import type { SourceControlWorkflowVersionId } from '../types/source-control-workflow-version-id';

describe('SourceControlService', () => {
const preferencesService = new SourceControlPreferencesService(
Container.get(InstanceSettings),
@@ -13,20 +22,95 @@ describe('SourceControlService', () => {
mock(),
mock(),
);
const sourceControlImportService = mock<SourceControlImportService>();
const tagRepository = mock<TagRepository>();
const sourceControlService = new SourceControlService(
mock(),
mock(),
preferencesService,
mock(),
mock(),
mock(),
sourceControlImportService,
tagRepository,
mock(),
);

beforeEach(() => {
jest.resetAllMocks();
jest.spyOn(sourceControlService, 'sanityCheck').mockResolvedValue(undefined);
});

describe('pushWorkfolder', () => {
it('should throw an error if a file is given that is not in the workfolder', async () => {
jest.spyOn(sourceControlService, 'sanityCheck').mockResolvedValue(undefined);
await expect(
sourceControlService.pushWorkfolder({
fileNames: [
{
file: '/etc/passwd',
id: 'test',
name: 'secret-file',
type: 'file',
status: 'modified',
location: 'local',
conflict: false,
updatedAt: new Date().toISOString(),
pushed: false,
},
],
}),
).rejects.toThrow('File path /etc/passwd is invalid');
});
});

describe('pullWorkfolder', () => {
it('does not filter locally created credentials', async () => {
// ARRANGE
const user = mock<User>();
const statuses = [
mock<SourceControlledFile>({
status: 'created',
location: 'local',
type: 'credential',
}),
mock<SourceControlledFile>({
status: 'created',
location: 'local',
type: 'workflow',
}),
];
jest.spyOn(sourceControlService, 'getStatus').mockResolvedValueOnce(statuses);

// ACT
const result = await sourceControlService.pullWorkfolder(user, {});

// ASSERT
expect(result).toMatchObject({ statusCode: 409, statusResult: statuses });
});

it('does not filter remotely deleted credentials', async () => {
// ARRANGE
const user = mock<User>();
const statuses = [
mock<SourceControlledFile>({
status: 'deleted',
location: 'remote',
type: 'credential',
}),
mock<SourceControlledFile>({
status: 'created',
location: 'local',
type: 'workflow',
}),
];
jest.spyOn(sourceControlService, 'getStatus').mockResolvedValueOnce(statuses);

// ACT
const result = await sourceControlService.pullWorkfolder(user, {});

// ASSERT
expect(result).toMatchObject({ statusCode: 409, statusResult: statuses });
});

it('should throw an error if a file is given that is not in the workfolder', async () => {
await expect(
sourceControlService.pushWorkfolder({
fileNames: [
@@ -46,4 +130,85 @@ describe('SourceControlService', () => {
).rejects.toThrow('File path /etc/passwd is invalid');
});
});

describe('getStatus', () => {
it('conflict depends on the value of `direction`', async () => {
// ARRANGE

// Define a credential that does only exist locally.
// Pulling this would delete it so it should be marked as a conflict.
// Pushing this is conflict free.
sourceControlImportService.getRemoteVersionIdsFromFiles.mockResolvedValue([]);
sourceControlImportService.getLocalVersionIdsFromDb.mockResolvedValue([
mock<SourceControlWorkflowVersionId>(),
]);

// Define a credential that does only exist locally.
// Pulling this would delete it so it should be marked as a conflict.
// Pushing this is conflict free.
sourceControlImportService.getRemoteCredentialsFromFiles.mockResolvedValue([]);
sourceControlImportService.getLocalCredentialsFromDb.mockResolvedValue([
mock<ExportableCredential & { filename: string }>(),
]);

// Define a variable that does only exist locally.
// Pulling this would delete it so it should be marked as a conflict.
// Pushing this is conflict free.
sourceControlImportService.getRemoteVariablesFromFile.mockResolvedValue([]);
sourceControlImportService.getLocalVariablesFromDb.mockResolvedValue([mock<Variables>()]);

// Define a tag that does only exist locally.
// Pulling this would delete it so it should be marked as a conflict.
// Pushing this is conflict free.
const tag = mock<TagEntity>({ updatedAt: new Date() });
tagRepository.find.mockResolvedValue([tag]);
sourceControlImportService.getRemoteTagsAndMappingsFromFile.mockResolvedValue({
tags: [],
mappings: [],
});
sourceControlImportService.getLocalTagsAndMappingsFromDb.mockResolvedValue({
tags: [tag],
mappings: [],
});

// ACT
const pullResult = await sourceControlService.getStatus({
direction: 'pull',
verbose: false,
preferLocalVersion: false,
});

const pushResult = await sourceControlService.getStatus({
direction: 'push',
verbose: false,
preferLocalVersion: false,
});

// ASSERT
console.log(pullResult);
console.log(pushResult);

if (!Array.isArray(pullResult)) {
fail('Expected pullResult to be an array.');
}
if (!Array.isArray(pushResult)) {
fail('Expected pushResult to be an array.');
}

expect(pullResult).toHaveLength(4);
expect(pushResult).toHaveLength(4);

expect(pullResult.find((i) => i.type === 'workflow')).toHaveProperty('conflict', true);
expect(pushResult.find((i) => i.type === 'workflow')).toHaveProperty('conflict', false);

expect(pullResult.find((i) => i.type === 'credential')).toHaveProperty('conflict', true);
expect(pushResult.find((i) => i.type === 'credential')).toHaveProperty('conflict', false);

expect(pullResult.find((i) => i.type === 'variables')).toHaveProperty('conflict', true);
expect(pushResult.find((i) => i.type === 'variables')).toHaveProperty('conflict', false);

expect(pullResult.find((i) => i.type === 'tags')).toHaveProperty('conflict', true);
expect(pushResult.find((i) => i.type === 'tags')).toHaveProperty('conflict', false);
});
});
});
Original file line number Diff line number Diff line change
@@ -9,9 +9,11 @@ import { readFile as fsReadFile } from 'node:fs/promises';
import path from 'path';

import { ActiveWorkflowManager } from '@/active-workflow-manager';
import { CredentialsService } from '@/credentials/credentials.service';
import type { Project } from '@/databases/entities/project';
import { SharedCredentials } from '@/databases/entities/shared-credentials';
import type { TagEntity } from '@/databases/entities/tag-entity';
import type { User } from '@/databases/entities/user';
import type { Variables } from '@/databases/entities/variables';
import type { WorkflowTagMapping } from '@/databases/entities/workflow-tag-mapping';
import { CredentialsRepository } from '@/databases/repositories/credentials.repository';
@@ -25,7 +27,9 @@ import { WorkflowTagMappingRepository } from '@/databases/repositories/workflow-
import { WorkflowRepository } from '@/databases/repositories/workflow.repository';
import type { IWorkflowToImport } from '@/interfaces';
import { isUniqueConstraintError } from '@/response-helper';
import { TagService } from '@/services/tag.service';
import { assertNever } from '@/utils';
import { WorkflowService } from '@/workflows/workflow.service';

import {
SOURCE_CONTROL_CREDENTIAL_EXPORT_FOLDER,
@@ -62,6 +66,9 @@ export class SourceControlImportService {
private readonly variablesRepository: VariablesRepository,
private readonly workflowRepository: WorkflowRepository,
private readonly workflowTagMappingRepository: WorkflowTagMappingRepository,
private readonly workflowService: WorkflowService,
private readonly credentialsService: CredentialsService,
private readonly tagService: TagService,
instanceSettings: InstanceSettings,
) {
this.gitFolder = path.join(instanceSettings.n8nFolder, SOURCE_CONTROL_GIT_FOLDER);
@@ -500,6 +507,30 @@ export class SourceControlImportService {
return result;
}

async deleteWorkflowsNotInWorkfolder(user: User, candidates: SourceControlledFile[]) {
for (const candidate of candidates) {
await this.workflowService.delete(user, candidate.id);
}
}

async deleteCredentialsNotInWorkfolder(user: User, candidates: SourceControlledFile[]) {
for (const candidate of candidates) {
await this.credentialsService.delete(user, candidate.id);
}
}

async deleteVariablesNotInWorkfolder(candidates: SourceControlledFile[]) {
for (const candidate of candidates) {
await this.variablesService.delete(candidate.id);
}
}

async deleteTagsNotInWorkfolder(candidates: SourceControlledFile[]) {
for (const candidate of candidates) {
await this.tagService.delete(candidate.id);
}
}

private async findOrCreateOwnerProject(owner: ResourceOwner): Promise<Project | null> {
if (typeof owner === 'string' || owner.type === 'personal') {
const email = typeof owner === 'string' ? owner : owner.personalEmail;
Original file line number Diff line number Diff line change
@@ -191,7 +191,7 @@ export class SourceControlController {
@Body payload: PullWorkFolderRequestDto,
): Promise<SourceControlledFile[] | ImportResult | PullResult | undefined> {
try {
const result = await this.sourceControlService.pullWorkfolder(req.user.id, payload);
const result = await this.sourceControlService.pullWorkfolder(req.user, payload);
res.statusCode = result.statusCode;
return result.statusResult;
} catch (error) {
Loading
Oops, something went wrong.

0 comments on commit 967ee4b

Please sign in to comment.