Skip to content

Commit a3be432

Browse files
refactor(users,auth): extract password hashing to lib/helpers to break auth↔users cycle (#3864)
* test(auth): unit-test extracted bcrypt password helper * refactor(users): hash via lib/helpers/password — drop auth.service import (#3862) * refactor(auth): source hashPassword/comparePassword from lib helper, keep re-export (#3862) * test(users): drop dead auth.service mock from remove suites (#3862) * test(auth): restore comparePassword false-match assertion (#3862) * docs(password): add per-function JSDoc to hashPassword and comparePassword (#3862)
1 parent dceba95 commit a3be432

8 files changed

Lines changed: 106 additions & 60 deletions

File tree

lib/helpers/password.js

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
/**
2+
* Password hashing helpers (bcrypt). Dependency-free leaf — breaks the auth↔users service cycle.
3+
* Source of truth for hashPassword / comparePassword; both auth.service and users.service import from here.
4+
*/
5+
import bcrypt from 'bcrypt';
6+
7+
const saltRounds = 10;
8+
9+
/**
10+
* @desc Hash a plaintext password using bcrypt.
11+
* @param {string|number} password - The plaintext password to hash.
12+
* @returns {Promise<string>} The bcrypt hash.
13+
*/
14+
const hashPassword = (password) => bcrypt.hash(String(password), saltRounds);
15+
16+
/**
17+
* @desc Compare a plaintext password against a stored bcrypt hash.
18+
* @param {string|number} userPassword - The plaintext candidate password.
19+
* @param {string} storedPassword - The stored bcrypt hash.
20+
* @returns {Promise<boolean>} True if the password matches the hash.
21+
*/
22+
const comparePassword = async (userPassword, storedPassword) => bcrypt.compare(String(userPassword), String(storedPassword));
23+
24+
export { hashPassword, comparePassword };
25+
export default { hashPassword, comparePassword };
Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,67 @@
1+
/**
2+
* Unit tests for the password helper (bcrypt wrappers).
3+
* Dependency-free leaf — mocks only bcrypt.
4+
*/
5+
import { jest, describe, test, expect, beforeEach } from '@jest/globals';
6+
7+
const mockBcryptHash = jest.fn();
8+
const mockBcryptCompare = jest.fn();
9+
10+
jest.unstable_mockModule('bcrypt', () => ({
11+
default: {
12+
hash: (...args) => mockBcryptHash(...args),
13+
compare: (...args) => mockBcryptCompare(...args),
14+
},
15+
}));
16+
17+
const { default: passwordHelper, hashPassword, comparePassword } = await import('../password.js');
18+
19+
describe('password helper', () => {
20+
beforeEach(() => {
21+
mockBcryptHash.mockReset();
22+
mockBcryptCompare.mockReset();
23+
});
24+
25+
describe('hashPassword()', () => {
26+
test('hashes the stringified password with saltRounds = 10', async () => {
27+
mockBcryptHash.mockResolvedValueOnce('$2b$hashed');
28+
const result = await hashPassword('mypassword');
29+
expect(result).toBe('$2b$hashed');
30+
expect(mockBcryptHash).toHaveBeenCalledWith('mypassword', 10);
31+
});
32+
33+
test('coerces a non-string password to a string before hashing', async () => {
34+
mockBcryptHash.mockResolvedValueOnce('$2b$num');
35+
await hashPassword(12345);
36+
expect(mockBcryptHash).toHaveBeenCalledWith('12345', 10);
37+
});
38+
});
39+
40+
describe('comparePassword()', () => {
41+
test('compares the stringified candidate against the stored hash', async () => {
42+
mockBcryptCompare.mockResolvedValueOnce(true);
43+
const result = await comparePassword('plain', 'hashed');
44+
expect(result).toBe(true);
45+
expect(mockBcryptCompare).toHaveBeenCalledWith('plain', 'hashed');
46+
});
47+
48+
test('coerces non-string args to strings before comparing', async () => {
49+
mockBcryptCompare.mockResolvedValueOnce(false);
50+
await comparePassword(12345, 67890);
51+
expect(mockBcryptCompare).toHaveBeenCalledWith('12345', '67890');
52+
});
53+
54+
test('returns false when the candidate does not match the stored hash', async () => {
55+
mockBcryptCompare.mockResolvedValueOnce(false);
56+
const result = await comparePassword('wrong', 'hashed');
57+
expect(result).toBe(false);
58+
});
59+
});
60+
61+
describe('default export', () => {
62+
test('exposes hashPassword and comparePassword', () => {
63+
expect(typeof passwordHelper.hashPassword).toBe('function');
64+
expect(typeof passwordHelper.comparePassword).toBe('function');
65+
});
66+
});
67+
});

modules/auth/services/auth.service.js

Lines changed: 1 addition & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -2,16 +2,14 @@
22
* Module dependencies
33
*/
44
import _ from 'lodash';
5-
import bcrypt from 'bcrypt';
65
import generatePassword from 'generate-password';
76
import zxcvbn from 'zxcvbn';
87

98
import config from '../../../config/index.js';
109
import AppError from '../../../lib/helpers/AppError.js';
10+
import { hashPassword, comparePassword } from '../../../lib/helpers/password.js';
1111
import UserService from '../../users/services/users.service.js';
1212

13-
const saltRounds = 10;
14-
1513
/**
1614
* @desc Local function to removeSensitive data from user
1715
* @param {Object} user
@@ -24,14 +22,6 @@ const removeSensitive = (user, conf) => {
2422
return _.pick(plain, keys);
2523
};
2624

27-
/**
28-
* @desc Function to compare passwords
29-
* @param {String} userPassword
30-
* @param {String} storedPassword
31-
* @return {Boolean} true/false
32-
*/
33-
const comparePassword = async (userPassword, storedPassword) => bcrypt.compare(String(userPassword), String(storedPassword));
34-
3525
/**
3626
* @desc Check whether the user account is currently locked and throw if so
3727
* @param {Object} user - Mongoose user document
@@ -109,13 +99,6 @@ const authenticate = async (email, password) => {
10999
throw new AppError('invalid user or password.', { code: 'SERVICE_ERROR' });
110100
};
111101

112-
/**
113-
* @desc Function to hash passwords
114-
* @param {String} password
115-
* @return {String} password hashed
116-
*/
117-
const hashPassword = (password) => bcrypt.hash(String(password), saltRounds);
118-
119102
/**
120103
* @desc Function to check password strength using zxcvbn
121104
* @param {String} password

modules/auth/tests/auth.unit.tests.js

Lines changed: 8 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -70,44 +70,21 @@ describe('Auth service unit tests:', () => {
7070
});
7171

7272
// ---------------------------------------------------------------------------
73-
// comparePassword
73+
// re-exported bcrypt helpers
7474
// ---------------------------------------------------------------------------
75-
describe('comparePassword()', () => {
76-
test('should return true when passwords match', async () => {
77-
mockBcryptCompare.mockResolvedValueOnce(true);
78-
const result = await AuthService.comparePassword('plain', 'hashed');
79-
expect(result).toBe(true);
80-
expect(mockBcryptCompare).toHaveBeenCalledWith('plain', 'hashed');
81-
});
82-
83-
test('should return false when passwords do not match', async () => {
84-
mockBcryptCompare.mockResolvedValueOnce(false);
85-
const result = await AuthService.comparePassword('wrong', 'hashed');
86-
expect(result).toBe(false);
87-
});
88-
89-
test('should coerce arguments to strings before comparing', async () => {
90-
mockBcryptCompare.mockResolvedValueOnce(true);
91-
await AuthService.comparePassword(12345, 67890);
92-
expect(mockBcryptCompare).toHaveBeenCalledWith('12345', '67890');
93-
});
94-
});
95-
96-
// ---------------------------------------------------------------------------
97-
// hashPassword
98-
// ---------------------------------------------------------------------------
99-
describe('hashPassword()', () => {
100-
test('should resolve with the hashed string', async () => {
75+
describe('re-exported bcrypt helpers', () => {
76+
test('hashPassword is re-exported and delegates to the helper', async () => {
10177
mockBcryptHash.mockResolvedValueOnce('$2b$hashed');
10278
const result = await AuthService.hashPassword('mypassword');
10379
expect(result).toBe('$2b$hashed');
10480
expect(mockBcryptHash).toHaveBeenCalledWith('mypassword', 10);
10581
});
10682

107-
test('should coerce the password to a string', async () => {
108-
mockBcryptHash.mockResolvedValueOnce('$2b$hashed');
109-
await AuthService.hashPassword(42);
110-
expect(mockBcryptHash).toHaveBeenCalledWith('42', 10);
83+
test('comparePassword is re-exported and delegates to the helper', async () => {
84+
mockBcryptCompare.mockResolvedValueOnce(true);
85+
const result = await AuthService.comparePassword('plain', 'hashed');
86+
expect(result).toBe(true);
87+
expect(mockBcryptCompare).toHaveBeenCalledWith('plain', 'hashed');
11188
});
11289
});
11390

modules/users/services/users.service.js

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ import config from '../../../config/index.js';
88
import logger from '../../../lib/services/logger.js';
99
import getBaseUrl from '../../../lib/helpers/getBaseUrl.js';
1010
import mailer from '../../../lib/helpers/mailer/index.js';
11-
import AuthService from '../../auth/services/auth.service.js';
11+
import passwordHelper from '../../../lib/helpers/password.js';
1212
import UserRepository from '../repositories/users.repository.js';
1313
import MembershipService from '../../organizations/services/organizations.membership.service.js';
1414
import OrganizationsRepository from '../../organizations/repositories/organizations.repository.js';
@@ -56,7 +56,7 @@ const create = async (user) => {
5656
// throw new AppError(`${validPassword.feedback.warning}. ${validPassword.feedback.suggestions.join('. ')}`);
5757
// }
5858
// When password is provided we need to make sure we are hashing it
59-
user.password = await AuthService.hashPassword(user.password);
59+
user.password = await passwordHelper.hashPassword(user.password);
6060
}
6161
const result = await UserRepository.create(user);
6262
// Remove sensitive data before return

modules/users/tests/users.service.count.unit.tests.js

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,8 +26,10 @@ jest.unstable_mockModule('../repositories/users.repository.js', () => ({
2626
},
2727
}));
2828

29-
jest.unstable_mockModule('../../auth/services/auth.service.js', () => ({
29+
jest.unstable_mockModule('../../../lib/helpers/password.js', () => ({
3030
default: { hashPassword: jest.fn(), comparePassword: jest.fn() },
31+
hashPassword: jest.fn(),
32+
comparePassword: jest.fn(),
3133
}));
3234

3335
jest.unstable_mockModule('../../organizations/services/organizations.membership.service.js', () => ({

modules/users/tests/users.service.remove.cascade.unit.tests.js

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -67,10 +67,6 @@ jest.unstable_mockModule('../../organizations/repositories/organizations.members
6767
},
6868
}));
6969

70-
jest.unstable_mockModule('../../auth/services/auth.service.js', () => ({
71-
default: { signOut: jest.fn() },
72-
}));
73-
7470
jest.unstable_mockModule('../../../config/index.js', () => ({
7571
default: { organizations: { enabled: true } },
7672
}));

modules/users/tests/users.service.remove.pendingSweep.unit.tests.js

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -69,10 +69,6 @@ jest.unstable_mockModule('../../organizations/repositories/organizations.members
6969
},
7070
}));
7171

72-
jest.unstable_mockModule('../../auth/services/auth.service.js', () => ({
73-
default: { signOut: jest.fn() },
74-
}));
75-
7672
jest.unstable_mockModule('../../../config/index.js', () => ({
7773
default: { organizations: { enabled: true } },
7874
}));

0 commit comments

Comments
 (0)