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

Optimize UM tests #3066

Merged
merged 57 commits into from
Apr 8, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
57 commits
Select commit Hold shift + click to select a range
aee27d0
:zap: Declutter test logs
ivov Mar 28, 2022
bcdbb88
:bug: Fix random passwords length
ivov Mar 28, 2022
16c7222
:bug: Fix password hashing in test user creation
ivov Mar 28, 2022
618aa2a
:bug: Hash leftover password
ivov Mar 29, 2022
b721c83
:zap: Improve error message for `compare`
ivov Mar 29, 2022
7d16de8
:zap: Restore `randomInvalidPassword` contant
ivov Mar 29, 2022
d1c2693
:zap: Mock Telemetry module to prevent `--forceExit`
ivov Mar 29, 2022
230aa05
:fire: Remove unused imports
ivov Mar 29, 2022
625475d
:fire: Remove unused import
ivov Mar 29, 2022
68034b4
:zap: Add util for configuring test SMTP
ivov Mar 29, 2022
f86ee92
:zap: Isolate user creation
ivov Mar 29, 2022
c38826e
:fire: De-duplicate `createFullUser`
ivov Mar 29, 2022
86f94b9
:zap: Centralize hashing
ivov Mar 29, 2022
e2ae754
:fire: Remove superfluous arg
ivov Mar 29, 2022
11aa992
:fire: Remove outdated comment
ivov Mar 29, 2022
edcfa89
:zap: Prioritize shared tables during trucation
ivov Mar 29, 2022
d1840ca
:test_tube: Add login tests
ivov Mar 29, 2022
9a053f7
:zap: Use token helper
ivov Mar 30, 2022
31c5696
:pencil2: Improve naming
ivov Mar 30, 2022
f117972
:zap: Make `createMemberShell` consistent
ivov Mar 30, 2022
0c4d454
:fire: Remove unneeded helper
ivov Mar 30, 2022
7cb82cd
:fire: De-duplicate `beforeEach`
ivov Mar 30, 2022
8cdad81
:pencil2: Improve naming
ivov Mar 30, 2022
a16677b
:truck: Move `categorize` to utils
ivov Mar 30, 2022
2d8195a
:pencil2: Update comment
ivov Mar 30, 2022
ec7d265
:test_tube: Simplify test
ivov Mar 30, 2022
db76b78
:blue_book: Improve `User.password` type
ivov Mar 30, 2022
0badb80
:zap: Silence logger
ivov Apr 1, 2022
d481dcc
:zap: Simplify condition
ivov Apr 1, 2022
331be2f
:zap: Unhash password in payload
ivov Apr 1, 2022
6c536a6
:twisted_rightwards_arrows: Merge parent branch
ivov Apr 1, 2022
8128ccb
:twisted_rightwards_arrows: Merge master
ivov Apr 3, 2022
a73cca5
:bug: Fix comparison against unhashed password
ivov Apr 3, 2022
7da4ba5
:zap: Increase timeout for fake SMTP service
ivov Apr 3, 2022
2d88ec5
:fire: Remove unneeded import
ivov Apr 3, 2022
cad685a
:zap: Use `isNull()`
ivov Apr 3, 2022
5c61796
:test_tube: Use `Promise.all()` in creds tests
ivov Apr 3, 2022
28de008
:test_tube: Use `Promise.all()` in me tests
ivov Apr 3, 2022
f5959a1
:test_tube: Use `Promise.all()` in owner tests
ivov Apr 3, 2022
c4efb32
:test_tube: Use `Promise.all()` in password tests
ivov Apr 3, 2022
00b3d71
:test_tube: Use `Promise.all()` in users tests
ivov Apr 3, 2022
801badc
:zap: Re-set cookie if UM disabled
ivov Apr 3, 2022
ae0be45
:fire: Remove repeated line
ivov Apr 3, 2022
0959d5d
:zap: Refactor out shared owner data
ivov Apr 3, 2022
bda4f46
:fire: Remove unneeded import
ivov Apr 3, 2022
554da21
:fire: Remove repeated lines
ivov Apr 3, 2022
694977a
:zap: Organize imports
ivov Apr 3, 2022
aec9044
:zap: Reuse helper
ivov Apr 3, 2022
b107ad2
:truck: Rename tests to match routers
ivov Apr 4, 2022
1af77ff
:truck: Rename `createFullUser()` to `createUser()`
ivov Apr 4, 2022
8941829
:zap: Consolidate user shell creation
ivov Apr 4, 2022
f8867c4
:zap: Make hashing async
ivov Apr 4, 2022
1fe9c80
:zap: Add email to user shell
ivov Apr 4, 2022
b94d402
:zap: Optimize array building
ivov Apr 4, 2022
8d02cde
🛠 refactor user shell factory
BHesseldieck Apr 4, 2022
ea42686
:bug: Fix MySQL tests
ivov Apr 6, 2022
6692156
:zap: Silence logger in other DBs
ivov Apr 6, 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
4 changes: 2 additions & 2 deletions packages/cli/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,8 @@
"start:windows": "cd bin && n8n",
"test": "npm run test:sqlite",
"test:sqlite": "export N8N_LOG_LEVEL='silent'; export DB_TYPE=sqlite; jest",
"test:postgres": "export DB_TYPE=postgresdb && jest",
"test:mysql": "export DB_TYPE=mysqldb && jest",
"test:postgres": "export N8N_LOG_LEVEL='silent'; export DB_TYPE=postgresdb && jest",
"test:mysql": "export N8N_LOG_LEVEL='silent'; export DB_TYPE=mysqldb && jest",
"watch": "tsc --watch",
"typeorm": "ts-node ../../node_modules/typeorm/cli.js"
},
Expand Down
14 changes: 6 additions & 8 deletions packages/cli/src/UserManagement/UserManagementHelper.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
import { Workflow } from 'n8n-workflow';
import { In, IsNull, Not } from 'typeorm';
import express = require('express');
import { compare } from 'bcryptjs';
import { compare, genSaltSync, hash } from 'bcryptjs';

import { PublicUser } from './Interfaces';
import { Db, ResponseHelper } from '..';
Expand Down Expand Up @@ -63,11 +63,6 @@ export function getInstanceBaseUrl(): string {
return n8nBaseUrl.endsWith('/') ? n8nBaseUrl.slice(0, n8nBaseUrl.length - 1) : n8nBaseUrl;
}

export async function isInstanceOwnerSetup(): Promise<boolean> {
const users = await Db.collections.User!.find({ email: Not(IsNull()) });
return users.length !== 0;
}

// TODO: Enforce at model level
export function validatePassword(password?: string): string {
if (!password) {
Expand Down Expand Up @@ -223,9 +218,12 @@ export function isAuthenticatedRequest(request: express.Request): request is Aut
// hashing
// ----------------------------------

export async function compareHash(str: string, hash: string): Promise<boolean | undefined> {
export const hashPassword = async (validPassword: string): Promise<string> =>
hash(validPassword, genSaltSync(10));

export async function compareHash(plaintext: string, hashed: string): Promise<boolean | undefined> {
try {
return await compare(str, hash);
return await compare(plaintext, hashed);
} catch (error) {
if (error instanceof Error && error.message.includes('Invalid salt version')) {
error.message +=
Expand Down
10 changes: 8 additions & 2 deletions packages/cli/src/UserManagement/routes/auth.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,10 @@ import { Db, ResponseHelper } from '../..';
import { AUTH_COOKIE_NAME } from '../../constants';
import { issueCookie, resolveJwt } from '../auth/jwt';
import { N8nApp, PublicUser } from '../Interfaces';
import { compareHash, isInstanceOwnerSetup, sanitizeUser } from '../UserManagementHelper';
import { compareHash, sanitizeUser } from '../UserManagementHelper';
import { User } from '../../databases/entities/User';
import type { LoginRequest } from '../../requests';
import config = require('../../../config');

export function authenticationMethods(this: N8nApp): void {
/**
Expand Down Expand Up @@ -71,13 +72,18 @@ export function authenticationMethods(this: N8nApp): void {
// If logged in, return user
try {
user = await resolveJwt(cookieContents);

if (!config.get('userManagement.isInstanceOwnerSetUp')) {
res.cookie(AUTH_COOKIE_NAME, cookieContents);
}

return sanitizeUser(user);
} catch (error) {
res.clearCookie(AUTH_COOKIE_NAME);
}
}

if (await isInstanceOwnerSetup()) {
if (config.get('userManagement.isInstanceOwnerSetUp')) {
const error = new Error('Not logged in');
// @ts-ignore
error.httpStatusCode = 401;
Expand Down
7 changes: 3 additions & 4 deletions packages/cli/src/UserManagement/routes/me.ts
Original file line number Diff line number Diff line change
@@ -1,15 +1,14 @@
/* eslint-disable @typescript-eslint/no-non-null-assertion */
/* eslint-disable import/no-cycle */

import { compare, genSaltSync, hashSync } from 'bcryptjs';
import express = require('express');
import validator from 'validator';
import { LoggerProxy as Logger } from 'n8n-workflow';

import { Db, InternalHooksManager, ResponseHelper } from '../..';
import { issueCookie } from '../auth/jwt';
import { N8nApp, PublicUser } from '../Interfaces';
import { validatePassword, sanitizeUser } from '../UserManagementHelper';
import { validatePassword, sanitizeUser, compareHash, hashPassword } from '../UserManagementHelper';
import type { AuthenticatedRequest, MeRequest } from '../../requests';
import { validateEntity } from '../../GenericHelpers';
import { User } from '../../databases/entities/User';
Expand Down Expand Up @@ -87,7 +86,7 @@ export function meNamespace(this: N8nApp): void {
throw new ResponseHelper.ResponseError('Requesting user not set up.');
}

const isCurrentPwCorrect = await compare(currentPassword, req.user.password);
const isCurrentPwCorrect = await compareHash(currentPassword, req.user.password);
if (!isCurrentPwCorrect) {
throw new ResponseHelper.ResponseError(
'Provided current password is incorrect.',
Expand All @@ -98,7 +97,7 @@ export function meNamespace(this: N8nApp): void {

const validPassword = validatePassword(newPassword);

req.user.password = hashSync(validPassword, genSaltSync(10));
req.user.password = await hashPassword(validPassword);

const user = await Db.collections.User!.save(req.user);
Logger.info('Password updated successfully', { userId: user.id });
Expand Down
5 changes: 2 additions & 3 deletions packages/cli/src/UserManagement/routes/owner.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
/* eslint-disable import/no-cycle */
/* eslint-disable @typescript-eslint/no-non-null-assertion */
import { hashSync, genSaltSync } from 'bcryptjs';
import * as express from 'express';
import validator from 'validator';
import { LoggerProxy as Logger } from 'n8n-workflow';
Expand All @@ -11,7 +10,7 @@ import { validateEntity } from '../../GenericHelpers';
import { AuthenticatedRequest, OwnerRequest } from '../../requests';
import { issueCookie } from '../auth/jwt';
import { N8nApp } from '../Interfaces';
import { sanitizeUser, validatePassword } from '../UserManagementHelper';
import { hashPassword, sanitizeUser, validatePassword } from '../UserManagementHelper';

export function ownerNamespace(this: N8nApp): void {
/**
Expand Down Expand Up @@ -74,7 +73,7 @@ export function ownerNamespace(this: N8nApp): void {
email,
firstName,
lastName,
password: hashSync(validPassword, genSaltSync(10)),
password: await hashPassword(validPassword),
});

await validateEntity(owner);
Expand Down
5 changes: 2 additions & 3 deletions packages/cli/src/UserManagement/routes/passwordReset.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,13 @@
import express = require('express');
import { v4 as uuid } from 'uuid';
import { URL } from 'url';
import { genSaltSync, hashSync } from 'bcryptjs';
import validator from 'validator';
import { IsNull, MoreThanOrEqual, Not } from 'typeorm';
import { LoggerProxy as Logger } from 'n8n-workflow';

import { Db, InternalHooksManager, ResponseHelper } from '../..';
import { N8nApp } from '../Interfaces';
import { getInstanceBaseUrl, validatePassword } from '../UserManagementHelper';
import { getInstanceBaseUrl, hashPassword, validatePassword } from '../UserManagementHelper';
import * as UserManagementMailer from '../email';
import type { PasswordResetRequest } from '../../requests';
import { issueCookie } from '../auth/jwt';
Expand Down Expand Up @@ -206,7 +205,7 @@ export function passwordResetNamespace(this: N8nApp): void {
}

await Db.collections.User!.update(userId, {
password: hashSync(validPassword, genSaltSync(10)),
password: await hashPassword(validPassword),
resetPasswordToken: null,
resetPasswordTokenExpiration: null,
});
Expand Down
4 changes: 2 additions & 2 deletions packages/cli/src/UserManagement/routes/users.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
/* eslint-disable @typescript-eslint/no-non-null-assertion */
import { Response } from 'express';
import { In } from 'typeorm';
import { genSaltSync, hashSync } from 'bcryptjs';
import validator from 'validator';
import { LoggerProxy as Logger } from 'n8n-workflow';

Expand All @@ -12,6 +11,7 @@ import { N8nApp, PublicUser } from '../Interfaces';
import { UserRequest } from '../../requests';
import {
getInstanceBaseUrl,
hashPassword,
isEmailSetUp,
sanitizeUser,
validatePassword,
Expand Down Expand Up @@ -349,7 +349,7 @@ export function usersNamespace(this: N8nApp): void {

invitee.firstName = firstName;
invitee.lastName = lastName;
invitee.password = hashSync(validPassword, genSaltSync(10));
invitee.password = await hashPassword(validPassword);

const updatedUser = await Db.collections.User!.save(invitee);

Expand Down
4 changes: 2 additions & 2 deletions packages/cli/src/databases/entities/User.ts
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ export class User {
@PrimaryGeneratedColumn('uuid')
id: string;

@Column({ length: 254 })
@Column({ length: 254, nullable: true })
@Index({ unique: true })
@IsEmail()
email: string;
BHesseldieck marked this conversation as resolved.
Show resolved Hide resolved
Expand All @@ -81,7 +81,7 @@ export class User {

@Column({ nullable: true })
@IsString({ message: 'Password must be of type string.' })
password?: string;
password: string;

@Column({ type: String, nullable: true })
resetPasswordToken?: string | null;
Expand Down
Loading