diff --git a/lib/helpers/password.js b/lib/helpers/password.js new file mode 100644 index 000000000..3c6edb82b --- /dev/null +++ b/lib/helpers/password.js @@ -0,0 +1,25 @@ +/** + * Password hashing helpers (bcrypt). Dependency-free leaf — breaks the auth↔users service cycle. + * Source of truth for hashPassword / comparePassword; both auth.service and users.service import from here. + */ +import bcrypt from 'bcrypt'; + +const saltRounds = 10; + +/** + * @desc Hash a plaintext password using bcrypt. + * @param {string|number} password - The plaintext password to hash. + * @returns {Promise} The bcrypt hash. + */ +const hashPassword = (password) => bcrypt.hash(String(password), saltRounds); + +/** + * @desc Compare a plaintext password against a stored bcrypt hash. + * @param {string|number} userPassword - The plaintext candidate password. + * @param {string} storedPassword - The stored bcrypt hash. + * @returns {Promise} True if the password matches the hash. + */ +const comparePassword = async (userPassword, storedPassword) => bcrypt.compare(String(userPassword), String(storedPassword)); + +export { hashPassword, comparePassword }; +export default { hashPassword, comparePassword }; diff --git a/lib/helpers/tests/password.unit.tests.js b/lib/helpers/tests/password.unit.tests.js new file mode 100644 index 000000000..513a03cc1 --- /dev/null +++ b/lib/helpers/tests/password.unit.tests.js @@ -0,0 +1,67 @@ +/** + * Unit tests for the password helper (bcrypt wrappers). + * Dependency-free leaf — mocks only bcrypt. + */ +import { jest, describe, test, expect, beforeEach } from '@jest/globals'; + +const mockBcryptHash = jest.fn(); +const mockBcryptCompare = jest.fn(); + +jest.unstable_mockModule('bcrypt', () => ({ + default: { + hash: (...args) => mockBcryptHash(...args), + compare: (...args) => mockBcryptCompare(...args), + }, +})); + +const { default: passwordHelper, hashPassword, comparePassword } = await import('../password.js'); + +describe('password helper', () => { + beforeEach(() => { + mockBcryptHash.mockReset(); + mockBcryptCompare.mockReset(); + }); + + describe('hashPassword()', () => { + test('hashes the stringified password with saltRounds = 10', async () => { + mockBcryptHash.mockResolvedValueOnce('$2b$hashed'); + const result = await hashPassword('mypassword'); + expect(result).toBe('$2b$hashed'); + expect(mockBcryptHash).toHaveBeenCalledWith('mypassword', 10); + }); + + test('coerces a non-string password to a string before hashing', async () => { + mockBcryptHash.mockResolvedValueOnce('$2b$num'); + await hashPassword(12345); + expect(mockBcryptHash).toHaveBeenCalledWith('12345', 10); + }); + }); + + describe('comparePassword()', () => { + test('compares the stringified candidate against the stored hash', async () => { + mockBcryptCompare.mockResolvedValueOnce(true); + const result = await comparePassword('plain', 'hashed'); + expect(result).toBe(true); + expect(mockBcryptCompare).toHaveBeenCalledWith('plain', 'hashed'); + }); + + test('coerces non-string args to strings before comparing', async () => { + mockBcryptCompare.mockResolvedValueOnce(false); + await comparePassword(12345, 67890); + expect(mockBcryptCompare).toHaveBeenCalledWith('12345', '67890'); + }); + + test('returns false when the candidate does not match the stored hash', async () => { + mockBcryptCompare.mockResolvedValueOnce(false); + const result = await comparePassword('wrong', 'hashed'); + expect(result).toBe(false); + }); + }); + + describe('default export', () => { + test('exposes hashPassword and comparePassword', () => { + expect(typeof passwordHelper.hashPassword).toBe('function'); + expect(typeof passwordHelper.comparePassword).toBe('function'); + }); + }); +}); diff --git a/modules/auth/services/auth.service.js b/modules/auth/services/auth.service.js index 2cc4f9ab7..dad2130c8 100644 --- a/modules/auth/services/auth.service.js +++ b/modules/auth/services/auth.service.js @@ -2,16 +2,14 @@ * Module dependencies */ import _ from 'lodash'; -import bcrypt from 'bcrypt'; import generatePassword from 'generate-password'; import zxcvbn from 'zxcvbn'; import config from '../../../config/index.js'; import AppError from '../../../lib/helpers/AppError.js'; +import { hashPassword, comparePassword } from '../../../lib/helpers/password.js'; import UserService from '../../users/services/users.service.js'; -const saltRounds = 10; - /** * @desc Local function to removeSensitive data from user * @param {Object} user @@ -24,14 +22,6 @@ const removeSensitive = (user, conf) => { return _.pick(plain, keys); }; -/** - * @desc Function to compare passwords - * @param {String} userPassword - * @param {String} storedPassword - * @return {Boolean} true/false - */ -const comparePassword = async (userPassword, storedPassword) => bcrypt.compare(String(userPassword), String(storedPassword)); - /** * @desc Check whether the user account is currently locked and throw if so * @param {Object} user - Mongoose user document @@ -109,13 +99,6 @@ const authenticate = async (email, password) => { throw new AppError('invalid user or password.', { code: 'SERVICE_ERROR' }); }; -/** - * @desc Function to hash passwords - * @param {String} password - * @return {String} password hashed - */ -const hashPassword = (password) => bcrypt.hash(String(password), saltRounds); - /** * @desc Function to check password strength using zxcvbn * @param {String} password diff --git a/modules/auth/tests/auth.unit.tests.js b/modules/auth/tests/auth.unit.tests.js index 75f6c9b6d..0c8a43791 100644 --- a/modules/auth/tests/auth.unit.tests.js +++ b/modules/auth/tests/auth.unit.tests.js @@ -70,44 +70,21 @@ describe('Auth service unit tests:', () => { }); // --------------------------------------------------------------------------- - // comparePassword + // re-exported bcrypt helpers // --------------------------------------------------------------------------- - describe('comparePassword()', () => { - test('should return true when passwords match', async () => { - mockBcryptCompare.mockResolvedValueOnce(true); - const result = await AuthService.comparePassword('plain', 'hashed'); - expect(result).toBe(true); - expect(mockBcryptCompare).toHaveBeenCalledWith('plain', 'hashed'); - }); - - test('should return false when passwords do not match', async () => { - mockBcryptCompare.mockResolvedValueOnce(false); - const result = await AuthService.comparePassword('wrong', 'hashed'); - expect(result).toBe(false); - }); - - test('should coerce arguments to strings before comparing', async () => { - mockBcryptCompare.mockResolvedValueOnce(true); - await AuthService.comparePassword(12345, 67890); - expect(mockBcryptCompare).toHaveBeenCalledWith('12345', '67890'); - }); - }); - - // --------------------------------------------------------------------------- - // hashPassword - // --------------------------------------------------------------------------- - describe('hashPassword()', () => { - test('should resolve with the hashed string', async () => { + describe('re-exported bcrypt helpers', () => { + test('hashPassword is re-exported and delegates to the helper', async () => { mockBcryptHash.mockResolvedValueOnce('$2b$hashed'); const result = await AuthService.hashPassword('mypassword'); expect(result).toBe('$2b$hashed'); expect(mockBcryptHash).toHaveBeenCalledWith('mypassword', 10); }); - test('should coerce the password to a string', async () => { - mockBcryptHash.mockResolvedValueOnce('$2b$hashed'); - await AuthService.hashPassword(42); - expect(mockBcryptHash).toHaveBeenCalledWith('42', 10); + test('comparePassword is re-exported and delegates to the helper', async () => { + mockBcryptCompare.mockResolvedValueOnce(true); + const result = await AuthService.comparePassword('plain', 'hashed'); + expect(result).toBe(true); + expect(mockBcryptCompare).toHaveBeenCalledWith('plain', 'hashed'); }); }); diff --git a/modules/users/services/users.service.js b/modules/users/services/users.service.js index e388302cd..92155e52f 100644 --- a/modules/users/services/users.service.js +++ b/modules/users/services/users.service.js @@ -8,7 +8,7 @@ import config from '../../../config/index.js'; import logger from '../../../lib/services/logger.js'; import getBaseUrl from '../../../lib/helpers/getBaseUrl.js'; import mailer from '../../../lib/helpers/mailer/index.js'; -import AuthService from '../../auth/services/auth.service.js'; +import passwordHelper from '../../../lib/helpers/password.js'; import UserRepository from '../repositories/users.repository.js'; import MembershipService from '../../organizations/services/organizations.membership.service.js'; import OrganizationsRepository from '../../organizations/repositories/organizations.repository.js'; @@ -56,7 +56,7 @@ const create = async (user) => { // throw new AppError(`${validPassword.feedback.warning}. ${validPassword.feedback.suggestions.join('. ')}`); // } // When password is provided we need to make sure we are hashing it - user.password = await AuthService.hashPassword(user.password); + user.password = await passwordHelper.hashPassword(user.password); } const result = await UserRepository.create(user); // Remove sensitive data before return diff --git a/modules/users/tests/users.service.count.unit.tests.js b/modules/users/tests/users.service.count.unit.tests.js index 2965096ac..d689f6957 100644 --- a/modules/users/tests/users.service.count.unit.tests.js +++ b/modules/users/tests/users.service.count.unit.tests.js @@ -26,8 +26,10 @@ jest.unstable_mockModule('../repositories/users.repository.js', () => ({ }, })); -jest.unstable_mockModule('../../auth/services/auth.service.js', () => ({ +jest.unstable_mockModule('../../../lib/helpers/password.js', () => ({ default: { hashPassword: jest.fn(), comparePassword: jest.fn() }, + hashPassword: jest.fn(), + comparePassword: jest.fn(), })); jest.unstable_mockModule('../../organizations/services/organizations.membership.service.js', () => ({ diff --git a/modules/users/tests/users.service.remove.cascade.unit.tests.js b/modules/users/tests/users.service.remove.cascade.unit.tests.js index b49ad36bb..1fc166f7d 100644 --- a/modules/users/tests/users.service.remove.cascade.unit.tests.js +++ b/modules/users/tests/users.service.remove.cascade.unit.tests.js @@ -67,10 +67,6 @@ jest.unstable_mockModule('../../organizations/repositories/organizations.members }, })); -jest.unstable_mockModule('../../auth/services/auth.service.js', () => ({ - default: { signOut: jest.fn() }, -})); - jest.unstable_mockModule('../../../config/index.js', () => ({ default: { organizations: { enabled: true } }, })); diff --git a/modules/users/tests/users.service.remove.pendingSweep.unit.tests.js b/modules/users/tests/users.service.remove.pendingSweep.unit.tests.js index b78c1cb89..3c738f5b1 100644 --- a/modules/users/tests/users.service.remove.pendingSweep.unit.tests.js +++ b/modules/users/tests/users.service.remove.pendingSweep.unit.tests.js @@ -69,10 +69,6 @@ jest.unstable_mockModule('../../organizations/repositories/organizations.members }, })); -jest.unstable_mockModule('../../auth/services/auth.service.js', () => ({ - default: { signOut: jest.fn() }, -})); - jest.unstable_mockModule('../../../config/index.js', () => ({ default: { organizations: { enabled: true } }, }));