refactor(core): Introduce password utility (no-changelog) (#7979)

## Summary
Provide details about your pull request and what it adds, fixes, or
changes. Photos and videos are recommended.
Continue breaking down `UserManagementHelper.ts`
...

#### How to test the change:
1. ...


## Issues fixed
Include links to Github issue or Community forum post or **Linear
ticket**:
> Important in order to close automatically and provide context to
reviewers

...


## Review / Merge checklist
- [ ] PR title and summary are descriptive. **Remember, the title
automatically goes into the changelog. Use `(no-changelog)` otherwise.**
([conventions](https://github.com/n8n-io/n8n/blob/master/.github/pull_request_title_conventions.md))
- [ ] [Docs updated](https://github.com/n8n-io/n8n-docs) or follow-up
ticket created.
- [ ] Tests included.
> A bug is not considered fixed, unless a test is added to prevent it
from happening again. A feature is not complete without tests.
  >
> *(internal)* You can use Slack commands to trigger [e2e
tests](https://www.notion.so/n8n/How-to-use-Test-Instances-d65f49dfc51f441ea44367fb6f67eb0a?pvs=4#a39f9e5ba64a48b58a71d81c837e8227)
or [deploy test
instance](https://www.notion.so/n8n/How-to-use-Test-Instances-d65f49dfc51f441ea44367fb6f67eb0a?pvs=4#f6a177d32bde4b57ae2da0b8e454bfce)
or [deploy early access version on
Cloud](https://www.notion.so/n8n/Cloudbot-3dbe779836004972b7057bc989526998?pvs=4#fef2d36ab02247e1a0f65a74f6fb534e).
This commit is contained in:
Iván Ovejero 2023-12-11 18:23:42 +01:00 committed by GitHub
parent 240d259260
commit c378f60a25
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
18 changed files with 206 additions and 98 deletions

View file

@ -120,6 +120,7 @@ import { CollaborationService } from './collaboration/collaboration.service';
import { RoleController } from './controllers/role.controller';
import { BadRequestError } from './errors/response-errors/bad-request.error';
import { NotFoundError } from './errors/response-errors/not-found.error';
import { PasswordUtility } from './services/password.utility';
const exec = promisify(callbackExec);
@ -264,6 +265,7 @@ export class Server extends AbstractServer {
internalHooks,
Container.get(SettingsRepository),
userService,
Container.get(PasswordUtility),
postHog,
),
Container.get(MeController),
@ -298,6 +300,7 @@ export class Server extends AbstractServer {
externalHooks,
Container.get(UserService),
Container.get(License),
Container.get(PasswordUtility),
postHog,
),
Container.get(VariablesController),

View file

@ -1,17 +1,13 @@
import { In } from 'typeorm';
import { compare, genSaltSync, hash } from 'bcryptjs';
import { Container } from 'typedi';
import type { WhereClause } from '@/Interfaces';
import type { User } from '@db/entities/User';
import { MAX_PASSWORD_LENGTH, MIN_PASSWORD_LENGTH } from '@db/entities/User';
import config from '@/config';
import { License } from '@/License';
import { getWebhookBaseUrl } from '@/WebhookHelpers';
import { UserRepository } from '@db/repositories/user.repository';
import type { Scope } from '@n8n/permissions';
import { BadRequestError } from '@/errors/response-errors/bad-request.error';
import { ApplicationError } from 'n8n-workflow';
export function isSharingEnabled(): boolean {
return Container.get(License).isSharingEnabled();
@ -30,42 +26,6 @@ export function generateUserInviteUrl(inviterId: string, inviteeId: string): str
return `${getInstanceBaseUrl()}/signup?inviterId=${inviterId}&inviteeId=${inviteeId}`;
}
// TODO: Enforce at model level
export function validatePassword(password?: string): string {
if (!password) {
throw new BadRequestError('Password is mandatory');
}
const hasInvalidLength =
password.length < MIN_PASSWORD_LENGTH || password.length > MAX_PASSWORD_LENGTH;
const hasNoNumber = !/\d/.test(password);
const hasNoUppercase = !/[A-Z]/.test(password);
if (hasInvalidLength || hasNoNumber || hasNoUppercase) {
const message: string[] = [];
if (hasInvalidLength) {
message.push(
`Password must be ${MIN_PASSWORD_LENGTH} to ${MAX_PASSWORD_LENGTH} characters long.`,
);
}
if (hasNoNumber) {
message.push('Password must contain at least 1 number.');
}
if (hasNoUppercase) {
message.push('Password must contain at least 1 uppercase letter.');
}
throw new BadRequestError(message.join(' '));
}
return password;
}
export async function getUserById(userId: string): Promise<User> {
const user = await Container.get(UserRepository).findOneOrFail({
where: { id: userId },
@ -74,28 +34,6 @@ export async function getUserById(userId: string): Promise<User> {
return user;
}
// ----------------------------------
// hashing
// ----------------------------------
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(plaintext, hashed);
} catch (e) {
const error = e instanceof Error ? e : new Error(`${e}`);
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 ApplicationError(error.message, { cause: error });
}
}
// return the difference between two arrays
export function rightDiff<T1, T2>(
[arr1, keyExtractor1]: [T1[], (item: T1) => string],

View file

@ -1,5 +1,5 @@
import type { User } from '@db/entities/User';
import { compareHash } from '@/UserManagement/UserManagementHelper';
import { PasswordUtility } from '@/services/password.utility';
import { Container } from 'typedi';
import { InternalHooks } from '@/InternalHooks';
import { isLdapLoginEnabled } from '@/Ldap/helpers';
@ -15,7 +15,7 @@ export const handleEmailLogin = async (
relations: ['globalRole', 'authIdentities'],
});
if (user?.password && (await compareHash(password, user.password))) {
if (user?.password && (await Container.get(PasswordUtility).compare(password, user.password))) {
return user;
}

View file

@ -110,3 +110,7 @@ export const TIME = {
HOUR: 60 * 60 * 1000,
DAY: 24 * 60 * 60 * 1000,
};
export const MIN_PASSWORD_CHAR_LENGTH = 8;
export const MAX_PASSWORD_CHAR_LENGTH = 64;

View file

@ -7,7 +7,6 @@ import { RoleRepository } from '@db/repositories/role.repository';
import { SettingsRepository } from '@db/repositories/settings.repository';
import { UserRepository } from '@db/repositories/user.repository';
import { ActiveWorkflowRunner } from '@/ActiveWorkflowRunner';
import { hashPassword } from '@/UserManagement/UserManagementHelper';
import { eventBus } from '@/eventbus/MessageEventBus/MessageEventBus';
import { License } from '@/License';
import { LICENSE_FEATURES, inE2ETests } from '@/constants';
@ -17,6 +16,7 @@ import type { BooleanLicenseFeature, IPushDataType } from '@/Interfaces';
import { MfaService } from '@/Mfa/mfa.service';
import { Push } from '@/push';
import { CacheService } from '@/services/cache.service';
import { PasswordUtility } from '@/services/password.utility';
if (!inE2ETests) {
console.error('E2E endpoints only allowed during E2E tests');
@ -95,6 +95,7 @@ export class E2EController {
private workflowRunner: ActiveWorkflowRunner,
private mfaService: MfaService,
private cacheService: CacheService,
private readonly passwordUtility: PasswordUtility,
) {
license.isFeatureEnabled = (feature: BooleanLicenseFeature) =>
this.enabledFeatures[feature] ?? false;
@ -187,7 +188,7 @@ export class E2EController {
const instanceOwner = {
id: uuid(),
...owner,
password: await hashPassword(owner.password),
password: await this.passwordUtility.hash(owner.password),
globalRoleId: globalOwnerRoleId,
};
@ -201,7 +202,7 @@ export class E2EController {
const adminUser = {
id: uuid(),
...admin,
password: await hashPassword(admin.password),
password: await this.passwordUtility.hash(admin.password),
globalRoleId: globalAdminRoleId,
};
@ -214,7 +215,7 @@ export class E2EController {
this.userRepo.create({
id: uuid(),
...payload,
password: await hashPassword(password),
password: await this.passwordUtility.hash(password),
globalRoleId: globalMemberRoleId,
}),
);

View file

@ -11,7 +11,7 @@ import { License } from '@/License';
import { UserService } from '@/services/user.service';
import { Logger } from '@/Logger';
import { isSamlLicensedAndEnabled } from '@/sso/saml/samlHelpers';
import { hashPassword, validatePassword } from '@/UserManagement/UserManagementHelper';
import { PasswordUtility } from '@/services/password.utility';
import { PostHogClient } from '@/posthog';
import type { User } from '@/databases/entities/User';
import validator from 'validator';
@ -29,6 +29,7 @@ export class InvitationController {
private readonly externalHooks: IExternalHooksClass,
private readonly userService: UserService,
private readonly license: License,
private readonly passwordUtility: PasswordUtility,
private readonly postHog?: PostHogClient,
) {}
@ -133,7 +134,7 @@ export class InvitationController {
throw new BadRequestError('Invalid payload');
}
const validPassword = validatePassword(password);
const validPassword = this.passwordUtility.validate(password);
const users = await this.userService.findMany({
where: { id: In([inviterId, inviteeId]) },
@ -163,7 +164,7 @@ export class InvitationController {
invitee.firstName = firstName;
invitee.lastName = lastName;
invitee.password = await hashPassword(validPassword);
invitee.password = await this.passwordUtility.hash(validPassword);
const updatedUser = await this.userService.save(invitee);

View file

@ -4,7 +4,7 @@ import { Response } from 'express';
import { Service } from 'typedi';
import { randomBytes } from 'crypto';
import { Authorized, Delete, Get, Patch, Post, RestController } from '@/decorators';
import { compareHash, hashPassword, validatePassword } from '@/UserManagement/UserManagementHelper';
import { PasswordUtility } from '@/services/password.utility';
import { validateEntity } from '@/GenericHelpers';
import { issueCookie } from '@/auth/jwt';
import type { User } from '@db/entities/User';
@ -31,6 +31,7 @@ export class MeController {
private readonly externalHooks: ExternalHooks,
private readonly internalHooks: InternalHooks,
private readonly userService: UserService,
private readonly passwordUtility: PasswordUtility,
) {}
/**
@ -119,14 +120,17 @@ export class MeController {
throw new BadRequestError('Requesting user not set up.');
}
const isCurrentPwCorrect = await compareHash(currentPassword, req.user.password);
const isCurrentPwCorrect = await this.passwordUtility.compare(
currentPassword,
req.user.password,
);
if (!isCurrentPwCorrect) {
throw new BadRequestError('Provided current password is incorrect.');
}
const validPassword = validatePassword(newPassword);
const validPassword = this.passwordUtility.validate(newPassword);
req.user.password = await hashPassword(validPassword);
req.user.password = await this.passwordUtility.hash(validPassword);
const user = await this.userService.save(req.user);
this.logger.info('Password updated successfully', { userId: user.id });

View file

@ -1,7 +1,7 @@
import validator from 'validator';
import { validateEntity } from '@/GenericHelpers';
import { Authorized, Post, RestController } from '@/decorators';
import { hashPassword, validatePassword } from '@/UserManagement/UserManagementHelper';
import { PasswordUtility } from '@/services/password.utility';
import { issueCookie } from '@/auth/jwt';
import { Response } from 'express';
import { Config } from '@/config';
@ -22,6 +22,7 @@ export class OwnerController {
private readonly internalHooks: IInternalHooksClass,
private readonly settingsRepository: SettingsRepository,
private readonly userService: UserService,
private readonly passwordUtility: PasswordUtility,
private readonly postHog?: PostHogClient,
) {}
@ -52,7 +53,7 @@ export class OwnerController {
throw new BadRequestError('Invalid email address');
}
const validPassword = validatePassword(password);
const validPassword = this.passwordUtility.validate(password);
if (!firstName || !lastName) {
this.logger.debug(
@ -79,7 +80,7 @@ export class OwnerController {
email,
firstName,
lastName,
password: await hashPassword(validPassword),
password: await this.passwordUtility.hash(validPassword),
});
await validateEntity(owner);

View file

@ -5,11 +5,8 @@ import { IsNull, Not } from 'typeorm';
import validator from 'validator';
import { Get, Post, RestController } from '@/decorators';
import {
getInstanceBaseUrl,
hashPassword,
validatePassword,
} from '@/UserManagement/UserManagementHelper';
import { getInstanceBaseUrl } from '@/UserManagement/UserManagementHelper';
import { PasswordUtility } from '@/services/password.utility';
import { UserManagementMailer } from '@/UserManagement/email';
import { PasswordResetRequest } from '@/requests';
import { issueCookie } from '@/auth/jwt';
@ -45,6 +42,7 @@ export class PasswordResetController {
private readonly userService: UserService,
private readonly mfaService: MfaService,
private readonly license: License,
private readonly passwordUtility: PasswordUtility,
) {}
/**
@ -204,7 +202,7 @@ export class PasswordResetController {
throw new BadRequestError('Missing user ID or password or reset password token');
}
const validPassword = validatePassword(password);
const validPassword = this.passwordUtility.validate(password);
const user = await this.userService.resolvePasswordResetToken(token);
if (!user) throw new NotFoundError('');
@ -219,7 +217,7 @@ export class PasswordResetController {
if (!validToken) throw new BadRequestError('Invalid MFA token.');
}
const passwordHash = await hashPassword(validPassword);
const passwordHash = await this.passwordUtility.hash(validPassword);
await this.userService.update(user.id, { password: passwordHash });

View file

@ -23,10 +23,6 @@ import type { AuthIdentity } from './AuthIdentity';
import { ownerPermissions, memberPermissions, adminPermissions } from '@/permissions/roles';
import { hasScope, type ScopeOptions, type Scope } from '@n8n/permissions';
export const MIN_PASSWORD_LENGTH = 8;
export const MAX_PASSWORD_LENGTH = 64;
const STATIC_SCOPE_MAP: Record<string, Scope[]> = {
owner: ownerPermissions,
member: memberPermissions,

View file

@ -0,0 +1,45 @@
import { BadRequestError } from '@/errors/response-errors/bad-request.error';
import { Service as Utility } from 'typedi';
import { compare, genSaltSync, hash } from 'bcryptjs';
import {
MAX_PASSWORD_CHAR_LENGTH as maxLength,
MIN_PASSWORD_CHAR_LENGTH as minLength,
} from '@/constants';
@Utility()
export class PasswordUtility {
async hash(plaintext: string) {
const SALT_ROUNDS = 10;
const salt = genSaltSync(SALT_ROUNDS);
return hash(plaintext, salt);
}
async compare(plaintext: string, hashed: string) {
return compare(plaintext, hashed);
}
validate(plaintext?: string) {
if (!plaintext) throw new BadRequestError('Password is mandatory');
const errorMessages: string[] = [];
if (plaintext.length < minLength || plaintext.length > maxLength) {
errorMessages.push(`Password must be ${minLength} to ${maxLength} characters long.`);
}
if (!/\d/.test(plaintext)) {
errorMessages.push('Password must contain at least 1 number.');
}
if (!/[A-Z]/.test(plaintext)) {
errorMessages.push('Password must contain at least 1 uppercase letter.');
}
if (errorMessages.length > 0) {
throw new BadRequestError(errorMessages.join(' '));
}
return plaintext;
}
}

View file

@ -3,7 +3,7 @@ import config from '@/config';
import { AuthIdentity } from '@db/entities/AuthIdentity';
import { User } from '@db/entities/User';
import { License } from '@/License';
import { hashPassword } from '@/UserManagement/UserManagementHelper';
import { PasswordUtility } from '@/services/password.utility';
import type { SamlPreferences } from './types/samlPreferences';
import type { SamlUserAttributes } from './types/samlUserAttributes';
import type { FlowResult } from 'samlify/types/src/flow';
@ -106,7 +106,7 @@ export async function createUserFromSamlAttributes(attributes: SamlUserAttribute
user.lastName = attributes.lastName;
user.globalRole = await Container.get(RoleService).findGlobalMemberRole();
// generates a password that is not used or known to the user
user.password = await hashPassword(generatePassword());
user.password = await Container.get(PasswordUtility).hash(generatePassword());
authIdentity.providerId = attributes.userPrincipalName;
authIdentity.providerType = 'saml';
authIdentity.user = user;

View file

@ -2,7 +2,7 @@ import validator from 'validator';
import type { SuperAgentTest } from 'supertest';
import type { User } from '@db/entities/User';
import { compareHash } from '@/UserManagement/UserManagementHelper';
import { PasswordUtility } from '@/services/password.utility';
import { UserManagementMailer } from '@/UserManagement/email/UserManagementMailer';
import Container from 'typedi';
@ -239,7 +239,10 @@ describe('POST /invitations/:id/accept', () => {
expect(storedMember.lastName).not.toBe(memberData.lastName);
expect(storedMember.password).not.toBe(memberData.password);
const comparisonResult = await compareHash(member.password, storedMember.password);
const comparisonResult = await Container.get(PasswordUtility).compare(
member.password,
storedMember.password,
);
expect(comparisonResult).toBe(false);
});

View file

@ -26,6 +26,7 @@ import {
import * as testDb from './shared/testDb';
import { getGlobalMemberRole, getGlobalOwnerRole } from './shared/db/roles';
import { createUser } from './shared/db/users';
import { PasswordUtility } from '@/services/password.utility';
config.set('userManagement.jwtSecret', randomString(5, 10));
@ -207,7 +208,10 @@ describe('POST /change-password', () => {
id: owner.id,
});
const comparisonResult = await compare(passwordToStore, storedPassword);
const comparisonResult = await Container.get(PasswordUtility).compare(
passwordToStore,
storedPassword,
);
expect(comparisonResult).toBe(true);
expect(storedPassword).not.toBe(passwordToStore);

View file

@ -1,5 +1,5 @@
import { randomBytes } from 'crypto';
import { MAX_PASSWORD_LENGTH, MIN_PASSWORD_LENGTH } from '@db/entities/User';
import { MIN_PASSWORD_CHAR_LENGTH, MAX_PASSWORD_CHAR_LENGTH } from '@/constants';
import type { CredentialPayload } from './types';
import { v4 as uuid } from 'uuid';
@ -31,14 +31,14 @@ export const randomPositiveDigit = (): number => {
const randomUppercaseLetter = () => chooseRandomly('ABCDEFGHIJKLMNOPQRSTUVWXYZ'.split(''));
export const randomValidPassword = () =>
randomString(MIN_PASSWORD_LENGTH, MAX_PASSWORD_LENGTH - 2) +
randomString(MIN_PASSWORD_CHAR_LENGTH, MAX_PASSWORD_CHAR_LENGTH - 2) +
randomUppercaseLetter() +
randomDigit();
export const randomInvalidPassword = () =>
chooseRandomly([
randomString(1, MIN_PASSWORD_LENGTH - 1),
randomString(MAX_PASSWORD_LENGTH + 2, MAX_PASSWORD_LENGTH + 100),
randomString(1, MIN_PASSWORD_CHAR_LENGTH - 1),
randomString(MAX_PASSWORD_CHAR_LENGTH + 2, MAX_PASSWORD_CHAR_LENGTH + 100),
'abcdefgh', // valid length, no number, no uppercase
'abcdefg1', // valid length, has number, no uppercase
'abcdefgA', // valid length, no number, has uppercase

View file

@ -21,6 +21,7 @@ import { AUTHLESS_ENDPOINTS, PUBLIC_API_REST_PATH_SEGMENT, REST_PATH_SEGMENT } f
import type { SetupProps, TestServer } from '../types';
import { InternalHooks } from '@/InternalHooks';
import { LicenseMocker } from '../license';
import { PasswordUtility } from '@/services/password.utility';
/**
* Plugin to prefix a path segment into a request URL pathname.
@ -229,6 +230,7 @@ export const setupTestServer = ({
Container.get(InternalHooks),
Container.get(SettingsRepository),
Container.get(UserService),
Container.get(PasswordUtility),
),
);
break;
@ -277,6 +279,7 @@ export const setupTestServer = ({
Container.get(EHS),
Container.get(USE),
Container.get(License),
Container.get(PasswordUtility),
),
);
break;

View file

@ -14,6 +14,8 @@ import { License } from '@/License';
import { mockInstance } from '../../shared/mocking';
import { badPasswords } from '../shared/testData';
import { BadRequestError } from '@/errors/response-errors/bad-request.error';
import { PasswordUtility } from '@/services/password.utility';
import Container from 'typedi';
describe('OwnerController', () => {
const config = mock<Config>();
@ -27,6 +29,7 @@ describe('OwnerController', () => {
internalHooks,
settingsRepository,
userService,
Container.get(PasswordUtility),
);
describe('setupOwner', () => {

View file

@ -0,0 +1,104 @@
import { PasswordUtility } from '@/services/password.utility';
import Container from 'typedi';
function toComponents(hash: string) {
const BCRYPT_HASH_REGEX =
/^\$(?<version>.{2})\$(?<costFactor>\d{2})\$(?<salt>.{22})(?<hashedPassword>.{31})$/;
const match = hash.match(BCRYPT_HASH_REGEX);
if (!match?.groups) throw new Error('Invalid bcrypt hash format');
return match.groups;
}
describe('PasswordUtility', () => {
const passwordUtility = Container.get(PasswordUtility);
describe('hash()', () => {
test('should hash a plaintext password', async () => {
const plaintext = 'abcd1234X';
const hashed = await passwordUtility.hash(plaintext);
const { version, costFactor, salt, hashedPassword } = toComponents(hashed);
expect(version).toBe('2a');
expect(costFactor).toBe('10');
expect(salt).toHaveLength(22);
expect(hashedPassword).toHaveLength(31);
});
});
describe('compare()', () => {
test('should return true on match', async () => {
const plaintext = 'abcd1234X';
const hashed = await passwordUtility.hash(plaintext);
const isMatch = await passwordUtility.compare(plaintext, hashed);
expect(isMatch).toBe(true);
});
test('should return false on mismatch', async () => {
const secondPlaintext = 'abcd1234Y';
const hashed = await passwordUtility.hash('abcd1234X');
const isMatch = await passwordUtility.compare(secondPlaintext, hashed);
expect(isMatch).toBe(false);
});
});
describe('validate()', () => {
test('should throw on empty password', () => {
const check = () => passwordUtility.validate();
expect(check).toThrowError('Password is mandatory');
});
test('should return same password if valid', () => {
const validPassword = 'abcd1234X';
const validated = passwordUtility.validate(validPassword);
expect(validated).toBe(validPassword);
});
test('should require at least one uppercase letter', () => {
const invalidPassword = 'abcd1234';
const failingCheck = () => passwordUtility.validate(invalidPassword);
expect(failingCheck).toThrowError('Password must contain at least 1 uppercase letter.');
});
test('should require at least one number', () => {
const validPassword = 'abcd1234X';
const invalidPassword = 'abcdEFGH';
const validated = passwordUtility.validate(validPassword);
expect(validated).toBe(validPassword);
const check = () => passwordUtility.validate(invalidPassword);
expect(check).toThrowError('Password must contain at least 1 number.');
});
test('should require a minimum length of 8 characters', () => {
const invalidPassword = 'a'.repeat(7);
const check = () => passwordUtility.validate(invalidPassword);
expect(check).toThrowError('Password must be 8 to 64 characters long.');
});
test('should require a maximum length of 64 characters', () => {
const invalidPassword = 'a'.repeat(65);
const check = () => passwordUtility.validate(invalidPassword);
expect(check).toThrowError('Password must be 8 to 64 characters long.');
});
});
});