Skip to content

Commit

Permalink
🛠 refactorings from walkthrough (#2856)
Browse files Browse the repository at this point in the history
* ⚡ Make `getTemplate` async

* ⚡ Remove query builder from `getCredentials`

* ⚡ Add save manual executions log message

* ⏪ Restore and hide migrations logs

* ⚡ Centralize ignore paths check

* 👕 Fix build

* 🚚 Rename `hasOwner` to `isInstanceOwnerSetUp`

* ⚡ Add `isSetUp` flag to `User`

* ⚡ Add `isSetUp` to FE interface

* ⚡ Adjust `isSetUp` checks on FE

* 👕 Fix build

* ⚡ Adjust `isPendingUser()` check

* 🚚 Shorten helper name

* ⚡ Refactor as `isPending` per feedback

* ✏️ Update log message

* ⚡ Broaden check

* 🔥 Remove unneeded relation

* ⚡ Refactor query

* 🔥 Re-remove logs from migrations
  • Loading branch information
ivov authored Feb 28, 2022
1 parent 110f249 commit 2dbf7bb
Show file tree
Hide file tree
Showing 25 changed files with 92 additions and 74 deletions.
6 changes: 6 additions & 0 deletions packages/cli/commands/start.ts
Original file line number Diff line number Diff line change
Expand Up @@ -389,6 +389,12 @@ export class Start extends Command {
const editorUrl = GenericHelpers.getBaseUrl();
this.log(`\nEditor is now accessible via:\n${editorUrl}`);

const saveManualExecutions = config.get('executions.saveDataManualExecutions') as boolean;

if (saveManualExecutions) {
this.log('\nManual executions will be visible only for the owner');
}

// Allow to open n8n editor by pressing "o"
if (Boolean(process.stdout.isTTY) && process.stdin.setRawMode) {
process.stdin.setRawMode(true);
Expand Down
5 changes: 4 additions & 1 deletion packages/cli/commands/user-management/reset.ts
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,10 @@ export class Reset extends Command {
await Db.collections.User!.delete({ id: Not(owner.id) });
await Db.collections.User!.save(Object.assign(owner, this.defaultUserProps));

await Db.collections.Settings!.update({ key: 'userManagement.hasOwner' }, { value: 'false' });
await Db.collections.Settings!.update(
{ key: 'userManagement.isInstanceOwnerSetUp' },
{ value: 'false' },
);
} catch (error) {
console.error('Error resetting database. See log messages for details.');
if (error instanceof Error) logger.error(error.message);
Expand Down
32 changes: 20 additions & 12 deletions packages/cli/src/CredentialsHelper.ts
Original file line number Diff line number Diff line change
Expand Up @@ -212,32 +212,40 @@ export class CredentialsHelper extends ICredentialsHelper {
/**
* Returns the credentials instance
*
* @param {INodeCredentialsDetails} nodeCredentials id and name to return instance of
* @param {string} type Type of the credentials to return instance of
* @param {INodeCredentialsDetails} nodeCredential id and name to return instance of
* @param {string} type Type of the credential to return instance of
* @returns {Credentials}
* @memberof CredentialsHelper
*/
async getCredentials(
nodeCredentials: INodeCredentialsDetails,
nodeCredential: INodeCredentialsDetails,
type: string,
userId?: string,
): Promise<Credentials> {
if (!nodeCredentials.id) {
throw new Error(`Credentials "${nodeCredentials.name}" for type "${type}" don't have an ID.`);
if (!nodeCredential.id) {
throw new Error(`Credential "${nodeCredential.name}" of type "${type}" has no ID.`);
}

const credentials = await Db.collections.Credentials!.findOne(nodeCredentials.id);
const credential = userId
? await Db.collections
.SharedCredentials!.findOneOrFail({
relations: ['credentials'],
where: { credentials: { id: nodeCredential.id, type }, user: { id: userId } },
})
.then((shared) => shared.credentials)
: await Db.collections.Credentials!.findOneOrFail({ id: nodeCredential.id, type });

if (!credentials) {
if (!credential) {
throw new Error(
`Credentials with ID "${nodeCredentials.id}" don't exist for type "${type}".`,
`Credential with ID "${nodeCredential.id}" does not exist for type "${type}".`,
);
}

return new Credentials(
{ id: credentials.id.toString(), name: credentials.name },
credentials.type,
credentials.nodesAccess,
credentials.data,
{ id: credential.id.toString(), name: credential.name },
credential.type,
credential.nodesAccess,
credential.data,
);
}

Expand Down
2 changes: 1 addition & 1 deletion packages/cli/src/Server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -317,7 +317,7 @@ class App {
userManagement: {
enabled:
config.get('userManagement.disabled') === false ||
config.get('userManagement.hasOwner') === true,
config.get('userManagement.isInstanceOwnerSetUp') === true,
// showSetupOnFirstLoad: config.get('userManagement.disabled') === false, // && config.get('userManagement.skipOwnerSetup') === true
smtpSetup: config.get('userManagement.emails.mode') === 'smtp',
},
Expand Down
9 changes: 9 additions & 0 deletions packages/cli/src/UserManagement/UserManagementHelper.ts
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,15 @@ export async function checkPermissionsForExecution(
return true;
}

/**
* Check if a URL contains an auth-excluded endpoint.
*/
export function isAuthExcluded(url: string, ignoredEndpoints: string[]): boolean {
return !!ignoredEndpoints
.filter(Boolean) // skip empty paths
.find((ignoredEndpoint) => url.includes(ignoredEndpoint));
}

/**
* Check if the endpoint is `POST /users/:id`.
*/
Expand Down
13 changes: 9 additions & 4 deletions packages/cli/src/UserManagement/email/UserManagementMailer.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
/* eslint-disable import/no-cycle */
import { existsSync, readFileSync } from 'fs';
import { IDataObject } from 'n8n-workflow';
import { join as pathJoin } from 'path';
import { GenericHelpers } from '../..';
import config = require('../../../config');
import {
InviteEmailData,
Expand All @@ -10,8 +12,11 @@ import {
} from './Interfaces';
import { NodeMailer } from './NodeMailer';

function getTemplate(configKeyName: string, defaultFilename: string) {
const templateOverride = config.get(`userManagement.emails.templates.${configKeyName}`) as string;
async function getTemplate(configKeyName: string, defaultFilename: string) {
const templateOverride = (await GenericHelpers.getConfigValue(
`userManagement.emails.templates.${configKeyName}`,
)) as string;

let template;
if (templateOverride && existsSync(templateOverride)) {
template = readFileSync(templateOverride, {
Expand Down Expand Up @@ -46,7 +51,7 @@ export class UserManagementMailer {
}

async invite(inviteEmailData: InviteEmailData): Promise<SendEmailResult> {
let template = getTemplate('invite', 'invite.html');
let template = await getTemplate('invite', 'invite.html');
template = replaceStrings(template, inviteEmailData);

// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
Expand All @@ -61,7 +66,7 @@ export class UserManagementMailer {
}

async passwordReset(passwordResetData: PasswordResetData): Promise<SendEmailResult> {
let template = getTemplate('passwordReset', 'passwordReset.html');
let template = await getTemplate('passwordReset', 'passwordReset.html');
template = replaceStrings(template, passwordResetData);

// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
Expand Down
1 change: 1 addition & 0 deletions packages/cli/src/UserManagement/email/index.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
/* eslint-disable import/no-cycle */
import { getInstance, UserManagementMailer } from './UserManagementMailer';

export { getInstance, UserManagementMailer };
16 changes: 3 additions & 13 deletions packages/cli/src/UserManagement/routes/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import { usersNamespace } from './users';
import { passwordResetNamespace } from './passwordReset';
import { AuthenticatedRequest } from '../../requests';
import { ownerNamespace } from './owner';
import { isAuthenticatedRequest, isPostUsersId } from '../UserManagementHelper';
import { isAuthExcluded, isPostUsersId, isAuthenticatedRequest } from '../UserManagementHelper';

export function addRoutes(this: N8nApp, ignoredEndpoints: string[], restEndpoint: string): void {
// needed for testing; not adding overhead since it directly returns if req.cookies exists
Expand Down Expand Up @@ -65,22 +65,12 @@ export function addRoutes(this: N8nApp, ignoredEndpoints: string[], restEndpoint
isPostUsersId(req, restEndpoint) ||
req.url.startsWith(`/${restEndpoint}/forgot-password`) ||
req.url.startsWith(`/${restEndpoint}/resolve-password-token`) ||
req.url.startsWith(`/${restEndpoint}/change-password`)
req.url.startsWith(`/${restEndpoint}/change-password`) ||
isAuthExcluded(req.url, ignoredEndpoints)
) {
return next();
}

for (let i = 0; i < ignoredEndpoints.length; i++) {
const path = ignoredEndpoints[i];
if (!path) {
// Skip empty paths (they might exist)
// eslint-disable-next-line no-continue
continue;
}
if (req.url.includes(path)) {
return next();
}
}
return passport.authenticate('jwt', { session: false })(req, res, next);
});

Expand Down
10 changes: 5 additions & 5 deletions packages/cli/src/UserManagement/routes/owner.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,15 +17,15 @@ import { sanitizeUser, validatePassword } from '../UserManagementHelper';
export function ownerNamespace(this: N8nApp): void {
/**
* Promote a shell into the owner of the n8n instance,
* and enable `hasOwner` instance setting.
* and enable `isInstanceOwnerSetUp` setting.
*/
this.app.post(
`/${this.restEndpoint}/owner`,
ResponseHelper.send(async (req: OwnerRequest.Post, res: express.Response) => {
const { email, firstName, lastName, password } = req.body;
const { id: userId } = req.user;

if (config.get('userManagement.hasOwner')) {
if (config.get('userManagement.isInstanceOwnerSetUp')) {
Logger.debug(
'Request to claim instance ownership failed because instance owner already exists',
{
Expand Down Expand Up @@ -79,14 +79,14 @@ export function ownerNamespace(this: N8nApp): void {

Logger.info('Owner updated successfully', { userId: req.user.id });

config.set('userManagement.hasOwner', true);
config.set('userManagement.isInstanceOwnerSetUp', true);

await Db.collections.Settings!.update(
{ key: 'userManagement.hasOwner' },
{ key: 'userManagement.isInstanceOwnerSetUp' },
{ value: JSON.stringify(true) },
);

Logger.debug('Setting hasOwner updated successfully', { userId: req.user.id });
Logger.debug('Setting isInstanceOwnerSetUp updated successfully', { userId: req.user.id });

await issueCookie(res, owner);

Expand Down
2 changes: 1 addition & 1 deletion packages/cli/src/UserManagement/routes/users.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ export function usersNamespace(this: N8nApp): void {
);
}

if (!config.get('userManagement.hasOwner')) {
if (!config.get('userManagement.isInstanceOwnerSetUp')) {
Logger.debug(
'Request to send email invite(s) to user(s) failed because emailing was not set up',
);
Expand Down
11 changes: 11 additions & 0 deletions packages/cli/src/databases/entities/User.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import {
ManyToOne,
PrimaryGeneratedColumn,
UpdateDateColumn,
AfterLoad,
} from 'typeorm';
import { IsEmail, IsString, Length } from 'class-validator';
import config = require('../../../config');
Expand Down Expand Up @@ -119,4 +120,14 @@ export class User {
setUpdateDate(): void {
this.updatedAt = new Date();
}

/**
* Whether the user is pending setup completion.
*/
isPending: boolean;

@AfterLoad()
computeIsPending(): void {
this.isPending = !!this.password;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,7 @@ export class CreateUserManagement1636626154933 implements MigrationInterface {
'INSERT INTO `' +
tablePrefix +
'settings` (`key`, value, loadOnStartup) values ' +
'("userManagement.hasOwner", "false", 1)',
'("userManagement.isInstanceOwnerSetUp", "false", 1)',
);
}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,16 +1,10 @@
import { MigrationInterface, QueryRunner } from 'typeorm';
import * as config from '../../../../config';
import { isTestRun } from '../../../../test/integration/shared/utils';

export class AddExecutionEntityIndexes1644424784709 implements MigrationInterface {
name = 'AddExecutionEntityIndexes1644424784709';

public async up(queryRunner: QueryRunner): Promise<void> {
!isTestRun &&
console.log(
'\n\nINFO: Started migration for execution entity indexes.\n Depending on the number of saved executions, it may take a while.\n\n',
);

const tablePrefix = config.get('database.tablePrefix');

await queryRunner.query(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -230,7 +230,7 @@ export class CreateUserManagement1636626154934 implements MigrationInterface {
'INSERT INTO "' +
tablePrefix +
'settings" ("key", "value", "loadOnStartup") values ' +
"('userManagement.hasOwner', 'false', true)",
"('userManagement.isInstanceOwnerSetUp', 'false', true)",
);
}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,15 +1,10 @@
import { MigrationInterface, QueryRunner } from 'typeorm';
import * as config from '../../../../config';
import { isTestRun } from '../../../../test/integration/shared/utils';

export class AddExecutionEntityIndexes1644422880309 implements MigrationInterface {
name = 'AddExecutionEntityIndexes1644422880309';

public async up(queryRunner: QueryRunner): Promise<void> {
!isTestRun &&
console.log(
'\n\nINFO: Started migration for execution entity indexes.\n Depending on the number of saved executions, it may take a while.\n\n',
);

let tablePrefix = config.get('database.tablePrefix');
const tablePrefixPure = tablePrefix;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ export class CreateUserManagement1636626154932 implements MigrationInterface {

await queryRunner.query(`
INSERT INTO "${tablePrefix}settings" (key, value, loadOnStartup) values
('userManagement.hasOwner', 'false', true)
('userManagement.isInstanceOwnerSetUp', 'false', true)
`);
}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,16 +1,10 @@
import { MigrationInterface, QueryRunner } from 'typeorm';
import * as config from '../../../../config';
import { isTestRun } from '../../../../test/integration/shared/utils';

export class AddExecutionEntityIndexes1644421939510 implements MigrationInterface {
name = 'AddExecutionEntityIndexes1644421939510';

public async up(queryRunner: QueryRunner): Promise<void> {
!isTestRun &&
console.log(
'\n\nINFO: Started migration for execution entity indexes.\n Depending on the number of saved executions, it may take a while.\n\n',
);

const tablePrefix = config.get('database.tablePrefix');

await queryRunner.query(`DROP INDEX IF EXISTS 'IDX_${tablePrefix}ca4a71b47f28ac6ea88293a8e2'`);
Expand Down
4 changes: 2 additions & 2 deletions packages/cli/test/integration/auth.endpoints.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,10 +35,10 @@ beforeEach(async () => {
role: globalOwnerRole,
});

config.set('userManagement.hasOwner', true);
config.set('userManagement.isInstanceOwnerSetUp', true);

await Db.collections.Settings!.update(
{ key: 'userManagement.hasOwner' },
{ key: 'userManagement.isInstanceOwnerSetUp' },
{ value: JSON.stringify(true) },
);
});
Expand Down
7 changes: 4 additions & 3 deletions packages/cli/test/integration/me.endpoints.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -203,10 +203,10 @@ describe('Member', () => {

await Db.collections.User!.save(newMember);

config.set('userManagement.hasOwner', true);
config.set('userManagement.isInstanceOwnerSetUp', true);

await Db.collections.Settings!.update(
{ key: 'userManagement.hasOwner' },
{ key: 'userManagement.isInstanceOwnerSetUp' },
{ value: JSON.stringify(true) },
);
});
Expand Down Expand Up @@ -367,9 +367,10 @@ describe('Owner', () => {
globalRole: globalOwnerRole,
});

config.set('userManagement.hasOwner', true);
config.set('userManagement.isInstanceOwnerSetUp', true);
});


afterEach(async () => {
await utils.truncate(['User']);
});
Expand Down
10 changes: 5 additions & 5 deletions packages/cli/test/integration/owner.endpoints.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ afterAll(() => {
return getConnection().close();
});

test('POST /owner should create owner and enable hasOwner setting', async () => {
test('POST /owner should create owner and enable isInstanceOwnerSetUp', async () => {
const owner = await Db.collections.User!.findOneOrFail();
const authOwnerAgent = await utils.createAgent(app, { auth: true, user: owner });

Expand Down Expand Up @@ -68,11 +68,11 @@ test('POST /owner should create owner and enable hasOwner setting', async () =>
expect(storedOwner.firstName).toBe(TEST_USER.firstName);
expect(storedOwner.lastName).toBe(TEST_USER.lastName);

const hasOwnerConfig = config.get('userManagement.hasOwner');
expect(hasOwnerConfig).toBe(true);
const isInstanceOwnerSetUpConfig = config.get('userManagement.isInstanceOwnerSetUp');
expect(isInstanceOwnerSetUpConfig).toBe(true);

const hasOwnerSetting = await utils.getHasOwnerSetting();
expect(hasOwnerSetting).toBe(true);
const isInstanceOwnerSetUpSetting = await utils.isInstanceOwnerSetUp();
expect(isInstanceOwnerSetUpSetting).toBe(true);
});

test('POST /owner should fail with invalid inputs', async () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ beforeEach(async () => {
jest.mock('../../config');
});

config.set('userManagement.hasOwner', true);
config.set('userManagement.isInstanceOwnerSetUp', true);
config.set('userManagement.emails.mode', '');

await utils.createUser({
Expand Down
Loading

0 comments on commit 2dbf7bb

Please sign in to comment.