test: Fix randomly failing UM tests (#3061)

* ⚡ Declutter test logs * 🐛 Fix random passwords length * 🐛 Fix password hashing in test user creation * 🐛 Hash leftover password * ⚡ Improve error message for `compare` * ⚡ Restore `randomInvalidPassword` contant * ⚡ Mock Telemetry module to prevent `--forceExit` * ⚡ Silence logger * ⚡ Simplify condition * ⚡ Unhash password in payload
n8n-io · Apr 1, 2022 · 7625421 · 7625421
1 parent 0a75539
commit 7625421
Show file tree

Hide file tree

Showing 16 changed files with 63 additions and 30 deletions.
diff --git a/packages/cli/config/index.ts b/packages/cli/config/index.ts
@@ -728,7 +728,7 @@ const config = convict({
 	logs: {
 		level: {
 			doc: 'Log output level',
-			format: ['error', 'warn', 'info', 'verbose', 'debug'],
+			format: ['error', 'warn', 'info', 'verbose', 'debug', 'silent'],
 			default: 'info',
 			env: 'N8N_LOG_LEVEL',
 		},

diff --git a/packages/cli/package.json b/packages/cli/package.json
@@ -30,7 +30,7 @@
     "start:default": "cd bin && ./n8n",
     "start:windows": "cd bin && n8n",
     "test": "npm run test:sqlite",
-    "test:sqlite": "export DB_TYPE=sqlite && jest --forceExit",
+    "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",
     "watch": "tsc --watch",

diff --git a/packages/cli/src/Logger.ts b/packages/cli/src/Logger.ts
@@ -11,14 +11,15 @@ class Logger implements ILogger {
 	private logger: winston.Logger;
 
 	constructor() {
-		// eslint-disable-next-line @typescript-eslint/no-unsafe-assignment
-		const level = config.get('logs.level');
+		const level = config.get('logs.level') as string;
+
 		// eslint-disable-next-line @typescript-eslint/no-shadow
 		const output = (config.get('logs.output') as string).split(',').map((output) => output.trim());
 
 		this.logger = winston.createLogger({
 			// eslint-disable-next-line @typescript-eslint/no-unsafe-assignment
 			level,
+			silent: level === 'silent',
 		});
 
 		if (output.includes('console')) {

diff --git a/packages/cli/src/ResponseHelper.ts b/packages/cli/src/ResponseHelper.ts
@@ -96,15 +96,13 @@ export function sendSuccessResponse(
 	}
 }
 
-export function sendErrorResponse(res: Response, error: ResponseError, shouldLog = true) {
+export function sendErrorResponse(res: Response, error: ResponseError) {
 	let httpStatusCode = 500;
 	if (error.httpStatusCode) {
 		httpStatusCode = error.httpStatusCode;
 	}
 
-	shouldLog = !process.argv[1].split('/').includes('jest');
-
-	if (process.env.NODE_ENV !== 'production' && shouldLog) {
+	if (!process.env.NODE_ENV || process.env.NODE_ENV === 'development') {
 		console.error('ERROR RESPONSE');
 		console.error(error);
 	}

diff --git a/packages/cli/src/UserManagement/UserManagementHelper.ts b/packages/cli/src/UserManagement/UserManagementHelper.ts
@@ -4,8 +4,10 @@
 import { Workflow } from 'n8n-workflow';
 import { In, IsNull, Not } from 'typeorm';
 import express = require('express');
+import { compare } from 'bcryptjs';
+
 import { PublicUser } from './Interfaces';
-import { Db, GenericHelpers, ResponseHelper } from '..';
+import { Db, ResponseHelper } from '..';
 import { MAX_PASSWORD_LENGTH, MIN_PASSWORD_LENGTH, User } from '../databases/entities/User';
 import { Role } from '../databases/entities/Role';
 import { AuthenticatedRequest } from '../requests';
@@ -216,3 +218,20 @@ export function isPostUsersId(req: express.Request, restEndpoint: string): boole
 export function isAuthenticatedRequest(request: express.Request): request is AuthenticatedRequest {
 	return request.user !== undefined;
 }
+
+// ----------------------------------
+//            hashing
+// ----------------------------------
+
+export async function compareHash(str: string, hash: string): Promise<boolean | undefined> {
+	try {
+		return await compare(str, hash);
+	} catch (error) {
+		if (error instanceof Error && error.message.includes('Invalid salt version')) {
+			error.message +=
+				'. Comparison against unhashed string. Please check that the value compared against has been hashed.';
+		}
+
+		throw new Error(error);
+	}
+}
diff --git a/packages/cli/src/UserManagement/routes/auth.ts b/packages/cli/src/UserManagement/routes/auth.ts
@@ -3,13 +3,12 @@
 /* eslint-disable @typescript-eslint/no-non-null-assertion */
 /* eslint-disable @typescript-eslint/no-unsafe-member-access */
 import { Request, Response } from 'express';
-import { compare } from 'bcryptjs';
 import { IDataObject } from 'n8n-workflow';
 import { Db, ResponseHelper } from '../..';
 import { AUTH_COOKIE_NAME } from '../../constants';
 import { issueCookie, resolveJwt } from '../auth/jwt';
 import { N8nApp, PublicUser } from '../Interfaces';
-import { isInstanceOwnerSetup, sanitizeUser } from '../UserManagementHelper';
+import { compareHash, isInstanceOwnerSetup, sanitizeUser } from '../UserManagementHelper';
 import { User } from '../../databases/entities/User';
 import type { LoginRequest } from '../../requests';
 
@@ -43,7 +42,8 @@ export function authenticationMethods(this: N8nApp): void {
 			} catch (error) {
 				throw new Error('Unable to access database.');
 			}
-			if (!user || !user.password || !(await compare(req.body.password, user.password))) {
+
+			if (!user || !user.password || !(await compareHash(req.body.password, user.password))) {
 				// password is empty until user signs up
 				const error = new Error('Wrong username or password. Do you have caps lock on?');
 				// @ts-ignore

diff --git a/packages/cli/test/integration/auth.endpoints.test.ts b/packages/cli/test/integration/auth.endpoints.test.ts
@@ -12,6 +12,8 @@ import { randomEmail, randomValidPassword, randomName } from './shared/random';
 import { getGlobalOwnerRole } from './shared/testDb';
 import * as testDb from './shared/testDb';
 
+jest.mock('../../src/telemetry');
+
 let globalOwnerRole: Role;
 
 let app: express.Application;
@@ -35,7 +37,7 @@ beforeEach(async () => {
 		email: TEST_USER.email,
 		firstName: TEST_USER.firstName,
 		lastName: TEST_USER.lastName,
-		password: hashSync(TEST_USER.password, genSaltSync(10)),
+		password: TEST_USER.password,
 		globalRole: globalOwnerRole,
 	});
 

diff --git a/packages/cli/test/integration/auth.middleware.test.ts b/packages/cli/test/integration/auth.middleware.test.ts
@@ -1,14 +1,16 @@
 import express = require('express');
+
 import * as request from 'supertest';
 import {
 	REST_PATH_SEGMENT,
 	ROUTES_REQUIRING_AUTHORIZATION,
 	ROUTES_REQUIRING_AUTHENTICATION,
 } from './shared/constants';
-
 import * as utils from './shared/utils';
 import * as testDb from './shared/testDb';
 
+jest.mock('../../src/telemetry');
+
 let app: express.Application;
 let testDbName = '';
 

diff --git a/packages/cli/test/integration/credentials.api.test.ts b/packages/cli/test/integration/credentials.api.test.ts
@@ -8,6 +8,8 @@ import { Role } from '../../src/databases/entities/Role';
 import { User } from '../../src/databases/entities/User';
 import * as testDb from './shared/testDb';
 
+jest.mock('../../src/telemetry');
+
 let app: express.Application;
 let testDbName = '';
 let saveCredential: SaveCredentialFunction;

diff --git a/packages/cli/test/integration/me.endpoints.test.ts b/packages/cli/test/integration/me.endpoints.test.ts
@@ -10,6 +10,8 @@ import { Role } from '../../src/databases/entities/Role';
 import { randomValidPassword, randomEmail, randomName, randomString } from './shared/random';
 import * as testDb from './shared/testDb';
 
+jest.mock('../../src/telemetry');
+
 let app: express.Application;
 let testDbName = '';
 let globalOwnerRole: Role;
@@ -275,7 +277,7 @@ describe('Member', () => {
 	test('PATCH /me/password should succeed with valid inputs', async () => {
 		const memberPassword = randomValidPassword();
 		const member = await testDb.createUser({
-			password: hashSync(memberPassword, genSaltSync(10)),
+			password: memberPassword,
 		});
 		const authMemberAgent = utils.createAgent(app, { auth: true, user: member });
 

diff --git a/packages/cli/test/integration/owner.endpoints.test.ts b/packages/cli/test/integration/owner.endpoints.test.ts
@@ -12,6 +12,8 @@ import {
 	randomInvalidPassword,
 } from './shared/random';
 
+jest.mock('../../src/telemetry');
+
 let app: express.Application;
 let testDbName = '';
 
@@ -82,7 +84,7 @@ test('POST /owner should create owner and enable isInstanceOwnerSetUp', async ()
 
 test('POST /owner should fail with invalid inputs', async () => {
 	const owner = await Db.collections.User!.findOneOrFail();
-	const authOwnerAgent = await utils.createAgent(app, { auth: true, user: owner });
+	const authOwnerAgent = utils.createAgent(app, { auth: true, user: owner });
 
 	for (const invalidPayload of INVALID_POST_OWNER_PAYLOADS) {
 		const response = await authOwnerAgent.post('/owner').send(invalidPayload);
@@ -92,7 +94,7 @@ test('POST /owner should fail with invalid inputs', async () => {
 
 test('POST /owner/skip-setup should persist skipping setup to the DB', async () => {
 	const owner = await Db.collections.User!.findOneOrFail();
-	const authOwnerAgent = await utils.createAgent(app, { auth: true, user: owner });
+	const authOwnerAgent = utils.createAgent(app, { auth: true, user: owner });
 
 	const response = await authOwnerAgent.post('/owner/skip-setup').send();
 

diff --git a/packages/cli/test/integration/passwordReset.endpoints.test.ts b/packages/cli/test/integration/passwordReset.endpoints.test.ts
@@ -14,6 +14,8 @@ import {
 import { Role } from '../../src/databases/entities/Role';
 import * as testDb from './shared/testDb';
 
+jest.mock('../../src/telemetry');
+
 let app: express.Application;
 let globalOwnerRole: Role;
 let testDbName = '';

diff --git a/packages/cli/test/integration/shared/random.ts b/packages/cli/test/integration/shared/random.ts
@@ -17,7 +17,7 @@ const randomDigit = () => Math.floor(Math.random() * 10);
 const randomUppercaseLetter = () => chooseRandomly('ABCDEFGHIJKLMNOPQRSTUVWXYZ'.split(''));
 
 export const randomValidPassword = () =>
-	randomString(MIN_PASSWORD_LENGTH, MAX_PASSWORD_LENGTH) + randomUppercaseLetter() + randomDigit();
+	randomString(MIN_PASSWORD_LENGTH, MAX_PASSWORD_LENGTH - 2) + randomUppercaseLetter() + randomDigit();
 
 export const randomInvalidPassword = () =>
 	chooseRandomly([

diff --git a/packages/cli/test/integration/shared/testDb.ts b/packages/cli/test/integration/shared/testDb.ts
@@ -16,6 +16,7 @@ import { sqliteMigrations } from '../../../src/databases/sqlite/migrations';
 import type { Role } from '../../../src/databases/entities/Role';
 import type { User } from '../../../src/databases/entities/User';
 import type { CredentialPayload } from './types';
+import { genSaltSync, hashSync } from 'bcryptjs';
 
 /**
  * Initialize one test DB per suite run, with bootstrap connection if needed.
@@ -188,7 +189,7 @@ export async function createUser(attributes: Partial<User> = {}): Promise<User>
 	const { email, password, firstName, lastName, globalRole, ...rest } = attributes;
 	const user = {
 		email: email ?? randomEmail(),
-		password: password ?? randomValidPassword(),
+		password: hashSync(password ?? randomValidPassword(), genSaltSync(10)),
 		firstName: firstName ?? randomName(),
 		lastName: lastName ?? randomName(),
 		globalRole: globalRole ?? (await getGlobalMemberRole()),

diff --git a/packages/cli/test/integration/shared/utils.ts b/packages/cli/test/integration/shared/utils.ts
@@ -89,12 +89,13 @@ export function initTestServer({
 	return testServer.app;
 }
 
+/**
+ * Pre-requisite: Mock the telemetry module before calling.
+ */
 export function initTestTelemetry() {
 	const mockNodeTypes = { nodeTypes: {} } as INodeTypes;
 
 	void InternalHooksManager.init('test-instance-id', 'test-version', mockNodeTypes);
-
-	jest.spyOn(Telemetry.prototype, 'track').mockResolvedValue();
 }
 
 /**
@@ -117,10 +118,9 @@ const classifyEndpointGroups = (endpointGroups: string[]) => {
 // ----------------------------------
 
 /**
- * Initialize a silent logger for test runs.
+ * Initialize a logger for test runs.
  */
 export function initTestLogger() {
-	config.set('logs.output', 'file'); // declutter console output
 	LoggerProxy.init(getLogger());
 }
 

diff --git a/packages/cli/test/integration/users.endpoints.test.ts b/packages/cli/test/integration/users.endpoints.test.ts
@@ -1,7 +1,7 @@
 import express = require('express');
 import validator from 'validator';
 import { v4 as uuid } from 'uuid';
-import { compare } from 'bcryptjs';
+import { compare, genSaltSync, hashSync } from 'bcryptjs';
 
 import { Db } from '../../src';
 import config = require('../../config');
@@ -18,6 +18,8 @@ import { WorkflowEntity } from '../../src/databases/entities/WorkflowEntity';
 import * as utils from './shared/utils';
 import * as testDb from './shared/testDb';
 
+jest.mock('../../src/telemetry');
+
 let app: express.Application;
 let testDbName = '';
 let globalOwnerRole: Role;
@@ -404,7 +406,7 @@ test('POST /users/:id should fail with invalid inputs', async () => {
 	}
 });
 
-test.skip('POST /users/:id should fail with already accepted invite', async () => {
+test('POST /users/:id should fail with already accepted invite', async () => {
 	const authlessAgent = utils.createAgent(app);
 
 	const globalMemberRole = await Db.collections.Role!.findOneOrFail({
@@ -414,7 +416,7 @@ test.skip('POST /users/:id should fail with already accepted invite', async () =
 
 	const shell = await Db.collections.User!.save({
 		email: randomEmail(),
-		password: randomValidPassword(), // simulate accepted invite
+		password: hashSync(randomValidPassword(), genSaltSync(10)), // simulate accepted invite
 		globalRole: globalMemberRole,
 	});
 
@@ -424,7 +426,7 @@ test.skip('POST /users/:id should fail with already accepted invite', async () =
 		inviterId: INITIAL_TEST_USER.id,
 		firstName: randomName(),
 		lastName: randomName(),
-		password: newPassword,
+		password: randomValidPassword(),
 	});
 
 	expect(response.statusCode).toBe(400);
@@ -458,7 +460,7 @@ test('POST /users should fail if user management is disabled', async () => {
 	expect(response.statusCode).toBe(500);
 });
 
-test.skip('POST /users should email invites and create user shells', async () => {
+test('POST /users should email invites and create user shells', async () => {
 	const owner = await Db.collections.User!.findOneOrFail();
 	const authOwnerAgent = utils.createAgent(app, { auth: true, user: owner });
 
@@ -502,7 +504,7 @@ test.skip('POST /users should email invites and create user shells', async () =>
 	}
 });
 
-test.skip('POST /users should fail with invalid inputs', async () => {
+test('POST /users should fail with invalid inputs', async () => {
 	const owner = await Db.collections.User!.findOneOrFail();
 	const authOwnerAgent = utils.createAgent(app, { auth: true, user: owner });
 
@@ -525,7 +527,7 @@ test.skip('POST /users should fail with invalid inputs', async () => {
 	}
 });
 
-test.skip('POST /users should ignore an empty payload', async () => {
+test('POST /users should ignore an empty payload', async () => {
 	const owner = await Db.collections.User!.findOneOrFail();
 	const authOwnerAgent = utils.createAgent(app, { auth: true, user: owner });