Skip to content

Commit a6d0eef

Browse files
despairbluedespairblue
committed
refactor credential import and remove redundant tests
There is no need to test the `--separate` flag in combination with the `--userId` flag, because the check for the relationships is now happening in one place
1 parent c2d4fad commit a6d0eef

File tree

3 files changed

+179
-240
lines changed

3 files changed

+179
-240
lines changed

‎packages/cli/src/commands/import/credentials.ts

Lines changed: 86 additions & 67 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,6 @@ import type { ICredentialsEncrypted } from 'n8n-workflow';
1515
import { ApplicationError, jsonParse } from 'n8n-workflow';
1616
import { UM_FIX_INSTRUCTION } from '@/constants';
1717
import { UserRepository } from '@db/repositories/user.repository';
18-
import { SharedCredentialsRepository } from '@/databases/repositories/sharedCredentials.repository';
1918

2019
export class ImportCredentialsCommand extends BaseCommand {
2120
static description = 'Import credentials';
@@ -65,87 +64,103 @@ export class ImportCredentialsCommand extends BaseCommand {
6564
}
6665
}
6766

68-
let totalImported = 0;
69-
70-
const cipher = Container.get(Cipher);
7167
const user = flags.userId ? await this.getAssignee(flags.userId) : await this.getOwner();
7268

73-
if (flags.separate) {
74-
let { input: inputPath } = flags;
69+
const credentials = await this.readCredentials(flags.input, flags.separate);
70+
71+
await Db.getConnection().transaction(async (transactionManager) => {
72+
this.transactionManager = transactionManager;
73+
74+
const result = await this.checkRelations(credentials, flags.userId);
7575

76-
if (process.platform === 'win32') {
77-
inputPath = inputPath.replace(/\\/g, '/');
76+
if (!result.success) {
77+
throw new ApplicationError(result.message);
7878
}
7979

80-
const files = await glob('*.json', {
81-
cwd: inputPath,
82-
absolute: true,
83-
});
80+
for (const credential of credentials) {
81+
await this.storeCredential(credential, user);
82+
}
83+
});
8484

85-
totalImported = files.length;
86-
87-
await Db.getConnection().transaction(async (transactionManager) => {
88-
this.transactionManager = transactionManager;
89-
for (const file of files) {
90-
const credential = jsonParse<ICredentialsEncrypted>(
91-
fs.readFileSync(file, { encoding: 'utf8' }),
92-
);
93-
94-
if (credential.id && flags.userId) {
95-
const credentialOwner = await this.getCredentialOwner(credential.id);
96-
97-
if (credentialOwner && credentialOwner !== flags.userId) {
98-
throw new ApplicationError(
99-
`The credential with id "${credential.id}" is already owned by the user with the id "${credentialOwner}". It can't be re-owned by the user with the id "${flags.userId}"`,
100-
);
101-
}
102-
}
103-
104-
if (typeof credential.data === 'object') {
105-
// plain data / decrypted input. Should be encrypted first.
106-
credential.data = cipher.encrypt(credential.data);
107-
}
108-
await this.storeCredential(credential, user);
109-
}
110-
});
85+
this.reportSuccess(credentials.length);
86+
}
11187

112-
this.reportSuccess(totalImported);
113-
return;
88+
private async checkRelations(credentials: ICredentialsEncrypted[], userId?: string) {
89+
if (!userId) {
90+
return {
91+
success: true as const,
92+
message: undefined,
93+
};
11494
}
11595

116-
const credentials = jsonParse<ICredentialsEncrypted[]>(
117-
fs.readFileSync(flags.input, { encoding: 'utf8' }),
118-
);
96+
for (const credential of credentials) {
97+
if (credential.id === undefined) {
98+
continue;
99+
}
119100

120-
totalImported = credentials.length;
101+
if (!(await this.credentialExists(credential.id))) {
102+
continue;
103+
}
121104

122-
if (!Array.isArray(credentials)) {
123-
throw new ApplicationError(
124-
'File does not seem to contain credentials. Make sure the credentials are contained in an array.',
125-
);
105+
const ownerId = await this.getCredentialOwner(credential.id);
106+
if (!ownerId) {
107+
continue;
108+
}
109+
110+
if (ownerId !== userId) {
111+
return {
112+
success: false as const,
113+
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}"`,
114+
};
115+
}
126116
}
127117

128-
await Db.getConnection().transaction(async (transactionManager) => {
129-
this.transactionManager = transactionManager;
130-
for (const credential of credentials) {
131-
if (credential.id && flags.userId) {
132-
const credentialOwner = await this.getCredentialOwner(credential.id);
133-
if (credentialOwner && credentialOwner !== flags.userId) {
134-
throw new ApplicationError(
135-
`The credential with id "${credential.id}" is already owned by the user with the id "${credentialOwner}". It can't be re-owned by the user with the id "${flags.userId}"`,
136-
);
137-
}
138-
}
118+
return {
119+
success: true as const,
120+
message: undefined,
121+
};
122+
}
139123

140-
if (typeof credential.data === 'object') {
141-
// plain data / decrypted input. Should be encrypted first.
142-
credential.data = cipher.encrypt(credential.data);
143-
}
144-
await this.storeCredential(credential, user);
124+
private async readCredentials(path: string, separate: boolean): Promise<ICredentialsEncrypted[]> {
125+
const cipher = Container.get(Cipher);
126+
127+
if (process.platform === 'win32') {
128+
path = path.replace(/\\/g, '/');
129+
}
130+
131+
let credentials: ICredentialsEncrypted[];
132+
133+
if (separate) {
134+
const files = await glob('*.json', {
135+
cwd: path,
136+
absolute: true,
137+
});
138+
139+
credentials = files.map((file) =>
140+
jsonParse<ICredentialsEncrypted>(fs.readFileSync(file, { encoding: 'utf8' })),
141+
);
142+
} else {
143+
const credentialsUnchecked = jsonParse<ICredentialsEncrypted[]>(
144+
fs.readFileSync(path, { encoding: 'utf8' }),
145+
);
146+
147+
if (!Array.isArray(credentialsUnchecked)) {
148+
throw new ApplicationError(
149+
'File does not seem to contain credentials. Make sure the credentials are contained in an array.',
150+
);
145151
}
146-
});
147152

148-
this.reportSuccess(totalImported);
153+
credentials = credentialsUnchecked;
154+
}
155+
156+
return credentials.map((credential) => {
157+
if (typeof credential.data === 'object') {
158+
// plain data / decrypted input. Should be encrypted first.
159+
credential.data = cipher.encrypt(credential.data);
160+
}
161+
162+
return credential;
163+
});
149164
}
150165

151166
async catch(error: Error) {
@@ -202,11 +217,15 @@ export class ImportCredentialsCommand extends BaseCommand {
202217
}
203218

204219
private async getCredentialOwner(credentialsId: string) {
205-
const sharedCredential = await Container.get(SharedCredentialsRepository).findOneBy({
220+
const sharedCredential = await this.transactionManager.findOneBy(SharedCredentials, {
206221
credentialsId,
207222
role: 'credential:owner',
208223
});
209224

210225
return sharedCredential?.userId;
211226
}
227+
228+
private async credentialExists(credentialId: string) {
229+
return await this.transactionManager.existsBy(CredentialsEntity, { id: credentialId });
230+
}
212231
}

0 commit comments

Comments
 (0)