diff --git a/src/lib/db/user-store.ts b/src/lib/db/user-store.ts index f8bf03eaf539..5ac64ef445d6 100644 --- a/src/lib/db/user-store.ts +++ b/src/lib/db/user-store.ts @@ -11,8 +11,10 @@ import type { IUserUpdateFields, } from '../types/stores/user-store'; import type { Db } from './db'; +import bcrypt from 'bcryptjs'; const TABLE = 'users'; +const PASSWORD_HASH_TABLE = 'used_passwords'; const USER_COLUMNS_PUBLIC = [ 'id', @@ -25,6 +27,8 @@ const USER_COLUMNS_PUBLIC = [ 'scim_id', ]; +const USED_PASSWORDS = ['user_id', 'password_hash', 'used_at']; + const USER_COLUMNS = [...USER_COLUMNS_PUBLIC, 'login_attempts', 'created_at']; const emptify = (value) => { @@ -71,6 +75,35 @@ class UserStore implements IUserStore { this.logger = getLogger('user-store.ts'); } + async passwordPreviouslyUsed( + userId: number, + password: string, + ): Promise { + const previouslyUsedPasswords = await this.db( + PASSWORD_HASH_TABLE, + ).where({ user_id: userId }); + return previouslyUsedPasswords.some((row) => + bcrypt.compareSync(password, row.password_hash), + ); + } + + async deletePasswordsUsedMoreThanNTimesAgo( + userId: number, + keepLastN: number, + ): Promise { + await this.db.raw( + ` + WITH UserPasswords AS ( + SELECT user_id, password_hash, used_at, ROW_NUMBER() OVER (PARTITION BY user_id ORDER BY used_at DESC) AS rn + FROM ${PASSWORD_HASH_TABLE} + WHERE user_id = ?) + DELETE FROM ${PASSWORD_HASH_TABLE} WHERE user_id = ? AND (user_id, password_hash, used_at) NOT IN (SELECT user_id, password_hash, used_at FROM UserPasswords WHERE rn <= ? + ); + `, + [userId, userId, keepLastN], + ); + } + async update(id: number, fields: IUserUpdateFields): Promise { await this.activeUsers() .where('id', id) @@ -177,10 +210,24 @@ class UserStore implements IUserStore { return item.password_hash; } - async setPasswordHash(userId: number, passwordHash: string): Promise { - return this.activeUsers().where('id', userId).update({ + async setPasswordHash( + userId: number, + passwordHash: string, + disallowNPreviousPasswords: number, + ): Promise { + await this.activeUsers().where('id', userId).update({ password_hash: passwordHash, }); + if (passwordHash) { + await this.db(PASSWORD_HASH_TABLE).insert({ + user_id: userId, + password_hash: passwordHash, + }); + await this.deletePasswordsUsedMoreThanNTimesAgo( + userId, + disallowNPreviousPasswords, + ); + } } async incLoginAttempts(user: User): Promise { diff --git a/src/lib/error/password-previously-used.ts b/src/lib/error/password-previously-used.ts new file mode 100644 index 000000000000..a55e902769b6 --- /dev/null +++ b/src/lib/error/password-previously-used.ts @@ -0,0 +1,11 @@ +import { UnleashError } from './unleash-error'; + +export class PasswordPreviouslyUsed extends UnleashError { + statusCode = 400; + + constructor( + message: string = `You've previously used this password, please use a new password`, + ) { + super(message); + } +} diff --git a/src/lib/routes/admin-api/user/user.test.ts b/src/lib/routes/admin-api/user/user.test.ts index 0d488e8959aa..2c3e8b31ce54 100644 --- a/src/lib/routes/admin-api/user/user.test.ts +++ b/src/lib/routes/admin-api/user/user.test.ts @@ -17,6 +17,7 @@ async function getSetup() { await stores.userStore.setPasswordHash( currentUser.id, await bcrypt.hash(oldPassword, 10), + 5, ); const config = createTestConfig({ diff --git a/src/lib/services/user-service.ts b/src/lib/services/user-service.ts index c0d1a1c87b21..84b68f35bf5a 100644 --- a/src/lib/services/user-service.ts +++ b/src/lib/services/user-service.ts @@ -38,6 +38,7 @@ import PasswordMismatch from '../error/password-mismatch'; import type EventService from '../features/events/event-service'; import { SYSTEM_USER, SYSTEM_USER_AUDIT } from '../types'; +import { PasswordPreviouslyUsed } from '../error/password-previously-used'; export interface ICreateUser { name?: string; @@ -62,6 +63,7 @@ export interface ILoginUserRequest { } const saltRounds = 10; +const disallowNPreviousPasswords = 5; class UserService { private logger: Logger; @@ -164,7 +166,11 @@ class UserService { username, }); const passwordHash = await bcrypt.hash(password, saltRounds); - await this.store.setPasswordHash(user.id, passwordHash); + await this.store.setPasswordHash( + user.id, + passwordHash, + disallowNPreviousPasswords, + ); await this.accessService.setUserRootRole( user.id, RoleName.ADMIN, @@ -232,7 +238,11 @@ class UserService { if (password) { const passwordHash = await bcrypt.hash(password, saltRounds); - await this.store.setPasswordHash(user.id, passwordHash); + await this.store.setPasswordHash( + user.id, + passwordHash, + disallowNPreviousPasswords, + ); } const userCreated = await this.getUser(user.id); @@ -387,8 +397,20 @@ class UserService { async changePassword(userId: number, password: string): Promise { this.validatePassword(password); + const previouslyUsed = await this.store.passwordPreviouslyUsed( + userId, + password, + ); + if (previouslyUsed) { + throw new PasswordPreviouslyUsed(); + } const passwordHash = await bcrypt.hash(password, saltRounds); - await this.store.setPasswordHash(userId, passwordHash); + + await this.store.setPasswordHash( + userId, + passwordHash, + disallowNPreviousPasswords, + ); await this.sessionService.deleteSessionsForUser(userId); await this.resetTokenService.expireExistingTokensForUser(userId); } diff --git a/src/lib/types/stores/user-store.ts b/src/lib/types/stores/user-store.ts index 96f9abed9943..e738af7ccbf8 100644 --- a/src/lib/types/stores/user-store.ts +++ b/src/lib/types/stores/user-store.ts @@ -28,7 +28,12 @@ export interface IUserStore extends Store { getAllWithId(userIdList: number[]): Promise; getByQuery(idQuery: IUserLookup): Promise; getPasswordHash(userId: number): Promise; - setPasswordHash(userId: number, passwordHash: string): Promise; + setPasswordHash( + userId: number, + passwordHash: string, + disallowNPreviousPasswords: number, + ): Promise; + passwordPreviouslyUsed(userId: number, password: string): Promise; incLoginAttempts(user: IUser): Promise; successfullyLogin(user: IUser): Promise; count(): Promise; diff --git a/src/migrations/20240705111827-used-passwords-table.js b/src/migrations/20240705111827-used-passwords-table.js new file mode 100644 index 000000000000..0528e5156ba6 --- /dev/null +++ b/src/migrations/20240705111827-used-passwords-table.js @@ -0,0 +1,15 @@ +exports.up = function(db, cb) { + db.runSql(` + CREATE TABLE used_passwords(user_id INTEGER REFERENCES users(id) ON DELETE CASCADE, + password_hash TEXT NOT NULL, + used_at TIMESTAMP WITH TIME ZONE DEFAULT (now() AT time zone 'utc'), + PRIMARY KEY (user_id, password_hash) + ); + INSERT INTO used_passwords(user_id, password_hash) SELECT id, password_hash FROM users WHERE password_hash IS NOT NULL; + CREATE INDEX used_passwords_pw_hash_idx ON used_passwords(password_hash); +`, cb) +}; + +exports.down = function(db, cb) { + db.runSql(`DROP TABLE used_passwords;`, cb); +}; diff --git a/src/test/config/test-config.ts b/src/test/config/test-config.ts index cce41a368e2a..ca115f3f88c5 100644 --- a/src/test/config/test-config.ts +++ b/src/test/config/test-config.ts @@ -13,6 +13,7 @@ function mergeAll(objects: Partial[]): T { } export function createTestConfig(config?: IUnleashOptions): IUnleashConfig { + getLogger.setMuteError(true); const testConfig: IUnleashOptions = { getLogger, authentication: { type: IAuthType.NONE, createAdminUser: false }, diff --git a/src/test/e2e/api/auth/reset-password-controller.e2e.test.ts b/src/test/e2e/api/auth/reset-password-controller.e2e.test.ts index 79dcad26fdaa..13a852e42315 100644 --- a/src/test/e2e/api/auth/reset-password-controller.e2e.test.ts +++ b/src/test/e2e/api/auth/reset-password-controller.e2e.test.ts @@ -177,14 +177,14 @@ test('Trying to reset password with same token twice does not work', async () => .post('/auth/reset/password') .send({ token, - password, + password: `${password}test`, }) .expect(200); await app.request .post('/auth/reset/password') .send({ token, - password, + password: `${password}othertest`, }) .expect(401) .expect((res) => { @@ -245,7 +245,7 @@ test('Calling reset endpoint with already existing session should logout/destroy .post('/auth/reset/password') .send({ token, - password, + password: `${password}newpassword`, }) .expect(200); await request.get('/api/admin/projects').expect(401); // we no longer have a valid session after using the reset password endpoint diff --git a/src/test/e2e/services/user-service.e2e.test.ts b/src/test/e2e/services/user-service.e2e.test.ts index b69a724d4655..2a44c0edeaef 100644 --- a/src/test/e2e/services/user-service.e2e.test.ts +++ b/src/test/e2e/services/user-service.e2e.test.ts @@ -27,6 +27,7 @@ import { USER_UPDATED, } from '../../../lib/types'; import { CUSTOM_ROOT_ROLE_TYPE } from '../../../lib/util'; +import { PasswordPreviouslyUsed } from '../../../lib/error/password-previously-used'; let db: ITestDb; let stores: IUnleashStores; @@ -511,3 +512,51 @@ test('should support a custom root role id when logging in and creating user via expect(permissions).toHaveLength(1); expect(permissions[0].permission).toBe(CREATE_ADDON); }); + +describe('Should not be able to use any of previous 5 passwords', () => { + test('throws exception when trying to use a previously used password', async () => { + const name = 'same-password-is-not-allowed'; + const email = `${name}@test.com`; + const password = 'externalScreaming$123'; + const user = await userService.createUser({ + email, + rootRole: customRole.id, + name, + password, + }); + await expect( + userService.changePassword(user.id, password), + ).rejects.toThrow(new PasswordPreviouslyUsed()); + }); + test('Is still able to change password to one not used', async () => { + const name = 'new-password-is-allowed'; + const email = `${name}@test.com`; + const password = 'externalScreaming$123'; + const user = await userService.createUser({ + email, + rootRole: customRole.id, + name, + password, + }); + await expect( + userService.changePassword(user.id, 'internalScreaming$123'), + ).resolves.not.toThrow(); + }); + test('Remembers 5 passwords', async () => { + const name = 'remembers-5-passwords-like-a-boss'; + const email = `${name}@test.com`; + const password = 'externalScreaming$123'; + const user = await userService.createUser({ + email, + rootRole: customRole.id, + name, + password, + }); + for (let i = 0; i < 5; i++) { + await userService.changePassword(user.id, `${password}${i}`); + } + await expect( + userService.changePassword(user.id, `${password}`), + ).resolves.not.toThrow(); // We've added 5 new passwords, so the original should work again + }); +}); diff --git a/src/test/e2e/stores/user-store.e2e.test.ts b/src/test/e2e/stores/user-store.e2e.test.ts index fd4a30460dd2..5ba6dae57550 100644 --- a/src/test/e2e/stores/user-store.e2e.test.ts +++ b/src/test/e2e/stores/user-store.e2e.test.ts @@ -60,7 +60,7 @@ test('Should require email or username', async () => { test('should set password_hash for user', async () => { const store = stores.userStore; const user = await store.insert({ email: 'admin@mail.com' }); - await store.setPasswordHash(user.id, 'rubbish'); + await store.setPasswordHash(user.id, 'rubbish', 5); const hash = await store.getPasswordHash(user.id); expect(hash).toBe('rubbish'); diff --git a/src/test/fixtures/fake-user-store.ts b/src/test/fixtures/fake-user-store.ts index c2832c3c39df..70cc5db8ebfa 100644 --- a/src/test/fixtures/fake-user-store.ts +++ b/src/test/fixtures/fake-user-store.ts @@ -15,6 +15,13 @@ class UserStoreMock implements IUserStore { this.idSeq = 1; this.data = []; } + + passwordPreviouslyUsed( + _userId: number, + _passwordHash: string, + ): Promise { + return Promise.resolve(false); + } countServiceAccounts(): Promise { return Promise.resolve(0); }