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.

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

Refactorings from user management walkthrough #2856

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
79a162a
:zap: Make `getTemplate` async
ivov Feb 21, 2022
2ad6ed8
:zap: Remove query builder from `getCredentials`
ivov Feb 21, 2022
295f21d
:zap: Add save manual executions log message
ivov Feb 21, 2022
95a6295
:rewind: Restore and hide migrations logs
ivov Feb 21, 2022
32d599b
:zap: Centralize ignore paths check
ivov Feb 21, 2022
34a97ce
:shirt: Fix build
ivov Feb 21, 2022
f6861b6
:twisted_rightwards_arrows: Merge parent branch
ivov Feb 22, 2022
c275012
:truck: Rename `hasOwner` to `isInstanceOwnerSetUp`
ivov Feb 22, 2022
0296ab2
:zap: Add `isSetUp` flag to `User`
ivov Feb 22, 2022
c00a560
:zap: Add `isSetUp` to FE interface
ivov Feb 22, 2022
4a29405
:zap: Adjust `isSetUp` checks on FE
ivov Feb 22, 2022
027e0bb
:shirt: Fix build
ivov Feb 22, 2022
df79383
:twisted_rightwards_arrows: Merge parent branch
ivov Feb 23, 2022
0d00aff
:twisted_rightwards_arrows: Merge parent branch
ivov Feb 23, 2022
497440d
:zap: Adjust `isPendingUser()` check
ivov Feb 23, 2022
75514aa
:truck: Shorten helper name
ivov Feb 23, 2022
a1b9338
:twisted_rightwards_arrows: Merge parent branch
ivov Feb 23, 2022
d27ee4d
:zap: Refactor as `isPending` per feedback
ivov Feb 24, 2022
fd84ac4
:twisted_rightwards_arrows: Merge parent branch
ivov Feb 25, 2022
edfb0b2
:pencil2: Update log message
ivov Feb 28, 2022
409fb0f
:zap: Broaden check
ivov Feb 28, 2022
2490716
:fire: Remove unneeded relation
ivov Feb 28, 2022
152d112
:zap: Refactor query
ivov Feb 28, 2022
6ca49b9
:twisted_rightwards_arrows: Merge parent branch
ivov Feb 28, 2022
d460107
:fire: Re-remove logs from migrations
ivov Feb 28, 2022
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
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 @@ -219,32 +219,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