Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(core): Don't create multiple owners when importing credentials or workflows #9112

Merged
merged 10 commits into from
Apr 12, 2024
Merged

fix(core): Don't create multiple owners when importing credentials or workflows #9112

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
910a1aa
don't allow to create multiple owners for the same credential
despairblue Apr 10, 2024
acb8671
fix grammar
despairblue Apr 11, 2024
4be1941
don't allow to create multiple owners for the same workflow
despairblue Apr 11, 2024
c2d4fad
refactor workflow tests and make sure the owner sharing is created wh…
despairblue Apr 11, 2024
a6d0eef
refactor credential import and remove redundant tests
despairblue Apr 11, 2024
9872849
simplify and unify the tests
despairblue Apr 11, 2024
0aaddc0
fix lints
despairblue Apr 11, 2024
f87da4a
add missing test case
despairblue Apr 11, 2024
e25b5f9
remove unused test file
despairblue Apr 12, 2024
65c9426
reorganize class to make diff more readable
despairblue Apr 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
165 changes: 111 additions & 54 deletions packages/cli/src/commands/import/credentials.ts
View file
Open in desktop
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file has been refactored so that reading the input files, checking and preparing the credentials only happens in one place.

Original file line number Diff line number Diff line change
Expand Up @@ -64,67 +64,103 @@ export class ImportCredentialsCommand extends BaseCommand {
}
}

let totalImported = 0;

const cipher = Container.get(Cipher);
const user = flags.userId ? await this.getAssignee(flags.userId) : await this.getOwner();

if (flags.separate) {
let { input: inputPath } = flags;
const credentials = await this.readCredentials(flags.input, flags.separate);

await Db.getConnection().transaction(async (transactionManager) => {
this.transactionManager = transactionManager;

if (process.platform === 'win32') {
inputPath = inputPath.replace(/\\/g, '/');
const result = await this.checkRelations(credentials, flags.userId);

if (!result.success) {
throw new ApplicationError(result.message);
}

const files = await glob('*.json', {
cwd: inputPath,
absolute: true,
});
for (const credential of credentials) {
await this.storeCredential(credential, user);
}
});

totalImported = files.length;

await Db.getConnection().transaction(async (transactionManager) => {
this.transactionManager = transactionManager;
for (const file of files) {
const credential = jsonParse<ICredentialsEncrypted>(
fs.readFileSync(file, { encoding: 'utf8' }),
);
if (typeof credential.data === 'object') {
// plain data / decrypted input. Should be encrypted first.
credential.data = cipher.encrypt(credential.data);
}
await this.storeCredential(credential, user);
}
});
this.reportSuccess(credentials.length);
}

this.reportSuccess(totalImported);
return;
private async checkRelations(credentials: ICredentialsEncrypted[], userId?: string) {
if (!userId) {
return {
success: true as const,
message: undefined,
};
}

const credentials = jsonParse<ICredentialsEncrypted[]>(
fs.readFileSync(flags.input, { encoding: 'utf8' }),
);
for (const credential of credentials) {
if (credential.id === undefined) {
continue;
}

if (!(await this.credentialExists(credential.id))) {
continue;
}

const ownerId = await this.getCredentialOwner(credential.id);
if (!ownerId) {
continue;
}

if (ownerId !== userId) {
return {
success: false as const,
message: `The credential with id "${credential.id}" is already owned by the user with the id "${ownerId}". It can't be re-owned by the user with the id "${userId}"`,
};
}
}

return {
success: true as const,
message: undefined,
};
}

private async readCredentials(path: string, separate: boolean): Promise<ICredentialsEncrypted[]> {
const cipher = Container.get(Cipher);

if (process.platform === 'win32') {
path = path.replace(/\\/g, '/');
}

let credentials: ICredentialsEncrypted[];

totalImported = credentials.length;
if (separate) {
const files = await glob('*.json', {
cwd: path,
absolute: true,
});

if (!Array.isArray(credentials)) {
throw new ApplicationError(
'File does not seem to contain credentials. Make sure the credentials are contained in an array.',
credentials = files.map((file) =>
jsonParse<ICredentialsEncrypted>(fs.readFileSync(file, { encoding: 'utf8' })),
);
} else {
const credentialsUnchecked = jsonParse<ICredentialsEncrypted[]>(
fs.readFileSync(path, { encoding: 'utf8' }),
);

if (!Array.isArray(credentialsUnchecked)) {
throw new ApplicationError(
'File does not seem to contain credentials. Make sure the credentials are contained in an array.',
);
}

credentials = credentialsUnchecked;
}

await Db.getConnection().transaction(async (transactionManager) => {
this.transactionManager = transactionManager;
for (const credential of credentials) {
if (typeof credential.data === 'object') {
// plain data / decrypted input. Should be encrypted first.
credential.data = cipher.encrypt(credential.data);
}
await this.storeCredential(credential, user);
return credentials.map((credential) => {
if (typeof credential.data === 'object') {
// plain data / decrypted input. Should be encrypted first.
credential.data = cipher.encrypt(credential.data);
}
});

this.reportSuccess(totalImported);
return credential;
});
}

async catch(error: Error) {
Expand All @@ -142,15 +178,23 @@ export class ImportCredentialsCommand extends BaseCommand {

private async storeCredential(credential: Partial<CredentialsEntity>, user: User) {
const result = await this.transactionManager.upsert(CredentialsEntity, credential, ['id']);
await this.transactionManager.upsert(
SharedCredentials,
{
credentialsId: result.identifiers[0].id as string,
userId: user.id,
role: 'credential:owner',
},
['credentialsId', 'userId'],
);

const sharingExists = await this.transactionManager.existsBy(SharedCredentials, {
credentialsId: credential.id,
role: 'credential:owner',
});

if (!sharingExists) {
await this.transactionManager.upsert(
SharedCredentials,
{
credentialsId: result.identifiers[0].id as string,
userId: user.id,
role: 'credential:owner',
},
['credentialsId', 'userId'],
);
}
}

private async getOwner() {
Expand All @@ -171,4 +215,17 @@ export class ImportCredentialsCommand extends BaseCommand {

return user;
}

private async getCredentialOwner(credentialsId: string) {
const sharedCredential = await this.transactionManager.findOneBy(SharedCredentials, {
credentialsId,
role: 'credential:owner',
});

return sharedCredential?.userId;
}

private async credentialExists(credentialId: string) {
return await this.transactionManager.existsBy(CredentialsEntity, { id: credentialId });
}
}
131 changes: 83 additions & 48 deletions packages/cli/src/commands/import/workflow.ts
View file
Open in desktop
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file has been refactored so that reading the input files and checking them happens in only one place.

Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,16 @@ import { ApplicationError, jsonParse } from 'n8n-workflow';
import fs from 'fs';
import glob from 'fast-glob';

import { UM_FIX_INSTRUCTION } from '@/constants';
import { WorkflowEntity } from '@db/entities/WorkflowEntity';
import { disableAutoGeneratedIds } from '@db/utils/commandHelpers';
import { generateNanoId } from '@db/utils/generators';
import { UserRepository } from '@db/repositories/user.repository';
import { WorkflowRepository } from '@db/repositories/workflow.repository';
import type { IWorkflowToImport } from '@/Interfaces';
import { ImportService } from '@/services/import.service';
import { BaseCommand } from '../BaseCommand';
import { SharedWorkflowRepository } from '@/databases/repositories/sharedWorkflow.repository';
import { UserRepository } from '@/databases/repositories/user.repository';
import { UM_FIX_INSTRUCTION } from '@/constants';

function assertHasWorkflowsToImport(workflows: unknown): asserts workflows is IWorkflowToImport[] {
if (!Array.isArray(workflows)) {
Expand Down Expand Up @@ -78,62 +79,61 @@ export class ImportWorkflowsCommand extends BaseCommand {
}
}

const user = flags.userId ? await this.getAssignee(flags.userId) : await this.getOwner();
const owner = await this.getOwner();

let totalImported = 0;
const workflows = await this.readWorkflows(flags.input, flags.separate);

if (flags.separate) {
let { input: inputPath } = flags;
const result = await this.checkRelations(workflows, flags.userId);
if (!result.success) {
throw new ApplicationError(result.message);
}

if (process.platform === 'win32') {
inputPath = inputPath.replace(/\\/g, '/');
}
this.logger.info(`Importing ${workflows.length} workflows...`);

const files = await glob('*.json', {
cwd: inputPath,
absolute: true,
});
await Container.get(ImportService).importWorkflows(workflows, flags.userId ?? owner.id);

totalImported = files.length;
this.logger.info(`Importing ${totalImported} workflows...`);
this.reportSuccess(workflows.length);
}

for (const file of files) {
const workflow = jsonParse<IWorkflowToImport>(fs.readFileSync(file, { encoding: 'utf8' }));
if (!workflow.id) {
workflow.id = generateNanoId();
}
private async checkRelations(workflows: WorkflowEntity[], userId: string | undefined) {
if (!userId) {
return {
success: true as const,
message: undefined,
};
}

const _workflow = Container.get(WorkflowRepository).create(workflow);
for (const workflow of workflows) {
if (!(await this.workflowExists(workflow))) {
continue;
}

await Container.get(ImportService).importWorkflows([_workflow], user.id);
const ownerId = await this.getWorkflowOwner(workflow);
if (!ownerId) {
continue;
}

this.reportSuccess(totalImported);
process.exit();
if (ownerId !== userId) {
return {
success: false as const,
message: `The credential with id "${workflow.id}" is already owned by the user with the id "${ownerId}". It can't be re-owned by the user with the id "${userId}"`,
};
}
}

const workflows = jsonParse<IWorkflowToImport[]>(
fs.readFileSync(flags.input, { encoding: 'utf8' }),
);

const _workflows = workflows.map((w) => Container.get(WorkflowRepository).create(w));

assertHasWorkflowsToImport(workflows);

totalImported = workflows.length;

await Container.get(ImportService).importWorkflows(_workflows, user.id);

this.reportSuccess(totalImported);
return {
success: true as const,
message: undefined,
};
}

async catch(error: Error) {
this.logger.error('An error occurred while importing workflows. See log messages for details.');
this.logger.error(error.message);
}
private async getWorkflowOwner(workflow: WorkflowEntity) {
const sharing = await Container.get(SharedWorkflowRepository).findOneBy({
workflowId: workflow.id,
role: 'workflow:owner',
});

private reportSuccess(total: number) {
this.logger.info(`Successfully imported ${total} ${total === 1 ? 'workflow.' : 'workflows.'}`);
return sharing?.userId;
}

private async getOwner() {
Expand All @@ -145,13 +145,48 @@ export class ImportWorkflowsCommand extends BaseCommand {
return owner;
}

private async getAssignee(userId: string) {
const user = await Container.get(UserRepository).findOneBy({ id: userId });
private async workflowExists(workflow: WorkflowEntity) {
return await Container.get(WorkflowRepository).existsBy({ id: workflow.id });
}

private async readWorkflows(path: string, separate: boolean): Promise<WorkflowEntity[]> {
if (process.platform === 'win32') {
path = path.replace(/\\/g, '/');
}

if (separate) {
const files = await glob('*.json', {
cwd: path,
absolute: true,
});
const workflowInstances = files.map((file) => {
const workflow = jsonParse<IWorkflowToImport>(fs.readFileSync(file, { encoding: 'utf8' }));
if (!workflow.id) {
workflow.id = generateNanoId();
}

const workflowInstance = Container.get(WorkflowRepository).create(workflow);

return workflowInstance;
});

return workflowInstances;
} else {
const workflows = jsonParse<IWorkflowToImport[]>(fs.readFileSync(path, { encoding: 'utf8' }));

if (!user) {
throw new ApplicationError('Failed to find user', { extra: { userId } });
const workflowInstances = workflows.map((w) => Container.get(WorkflowRepository).create(w));
assertHasWorkflowsToImport(workflows);

return workflowInstances;
}
}

return user;
async catch(error: Error) {
this.logger.error('An error occurred while importing workflows. See log messages for details.');
this.logger.error(error.message);
}

private reportSuccess(total: number) {
this.logger.info(`Successfully imported ${total} ${total === 1 ? 'workflow.' : 'workflows.'}`);
}
}
14 changes: 9 additions & 5 deletions packages/cli/src/services/import.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,14 +53,18 @@ export class ImportService {
this.logger.info(`Deactivating workflow "${workflow.name}". Remember to activate later.`);
}

const upsertResult = await tx.upsert(WorkflowEntity, workflow, ['id']);
const exists = workflow.id ? await tx.existsBy(WorkflowEntity, { id: workflow.id }) : false;

const upsertResult = await tx.upsert(WorkflowEntity, workflow, ['id']);
const workflowId = upsertResult.identifiers.at(0)?.id as string;

await tx.upsert(SharedWorkflow, { workflowId, userId, role: 'workflow:owner' }, [
'workflowId',
'userId',
]);
// Create relationship if the workflow was inserted instead of updated.
if (!exists) {
await tx.upsert(SharedWorkflow, { workflowId, userId, role: 'workflow:owner' }, [
'workflowId',
'userId',
]);
}
Comment on lines +62 to +67
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Part of the fix is here. This function will only create an owner if the workflow is imported for the first time.


if (!workflow.tags?.length) continue;

Expand Down
Loading
Loading