From 268744d18c989199eb8d388c6c06e121f0d7e7df Mon Sep 17 00:00:00 2001 From: Christopher Kolstad Date: Fri, 5 Jul 2024 15:07:24 +0200 Subject: [PATCH 01/12] feat: Disallow repeating last 5 passwords. We'll store hashes for the last 5 passwords, fetch them all for the user wanting to change their password, and make sure the password does not verify against any of the 5 stored hashes. --- src/lib/db/user-store.ts | 51 ++++++++++++++++++- src/lib/error/password-previously-used.ts | 11 ++++ src/lib/routes/admin-api/user/user.test.ts | 1 + src/lib/services/user-service.ts | 28 ++++++++-- src/lib/types/stores/user-store.ts | 7 ++- .../20240705111827-used-passwords-table.js | 15 ++++++ src/test/config/test-config.ts | 1 + .../reset-password-controller.e2e.test.ts | 6 +-- .../e2e/services/user-service.e2e.test.ts | 49 ++++++++++++++++++ src/test/e2e/stores/user-store.e2e.test.ts | 2 +- src/test/fixtures/fake-user-store.ts | 7 +++ 11 files changed, 168 insertions(+), 10 deletions(-) create mode 100644 src/lib/error/password-previously-used.ts create mode 100644 src/migrations/20240705111827-used-passwords-table.js 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); } From d7735cd8db8499be2ece7dc3e2053c6d2dd85233 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nuno=20G=C3=B3is?= Date: Tue, 9 Jul 2024 10:25:27 +0100 Subject: [PATCH 02/12] refactor: new changePasswordWithPreviouslyUsedPasswordCheck method --- src/lib/services/user-service.ts | 32 ++++++++++++----- .../e2e/services/user-service.e2e.test.ts | 34 ++++++++++++++++--- 2 files changed, 53 insertions(+), 13 deletions(-) diff --git a/src/lib/services/user-service.ts b/src/lib/services/user-service.ts index 84b68f35bf5a..5a97a7fd912a 100644 --- a/src/lib/services/user-service.ts +++ b/src/lib/services/user-service.ts @@ -397,13 +397,6 @@ 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( @@ -415,6 +408,21 @@ class UserService { await this.resetTokenService.expireExistingTokensForUser(userId); } + async changePasswordWithPreviouslyUsedPasswordCheck( + userId: number, + password: string, + ): Promise { + const previouslyUsed = await this.store.passwordPreviouslyUsed( + userId, + password, + ); + if (previouslyUsed) { + throw new PasswordPreviouslyUsed(); + } + + await this.changePassword(userId, password); + } + async changePasswordWithVerification( userId: number, newPassword: string, @@ -428,7 +436,10 @@ class UserService { ); } - await this.changePassword(userId, newPassword); + await this.changePasswordWithPreviouslyUsedPasswordCheck( + userId, + newPassword, + ); } async getUserForToken(token: string): Promise { @@ -464,7 +475,10 @@ class UserService { token, }); if (allowed) { - await this.changePassword(user.id, password); + await this.changePasswordWithPreviouslyUsedPasswordCheck( + user.id, + password, + ); } else { throw new InvalidTokenError(); } diff --git a/src/test/e2e/services/user-service.e2e.test.ts b/src/test/e2e/services/user-service.e2e.test.ts index 2a44c0edeaef..98048cfa5b79 100644 --- a/src/test/e2e/services/user-service.e2e.test.ts +++ b/src/test/e2e/services/user-service.e2e.test.ts @@ -525,7 +525,10 @@ describe('Should not be able to use any of previous 5 passwords', () => { password, }); await expect( - userService.changePassword(user.id, password), + userService.changePasswordWithPreviouslyUsedPasswordCheck( + user.id, + password, + ), ).rejects.toThrow(new PasswordPreviouslyUsed()); }); test('Is still able to change password to one not used', async () => { @@ -539,7 +542,10 @@ describe('Should not be able to use any of previous 5 passwords', () => { password, }); await expect( - userService.changePassword(user.id, 'internalScreaming$123'), + userService.changePasswordWithPreviouslyUsedPasswordCheck( + user.id, + 'internalScreaming$123', + ), ).resolves.not.toThrow(); }); test('Remembers 5 passwords', async () => { @@ -553,10 +559,30 @@ describe('Should not be able to use any of previous 5 passwords', () => { password, }); for (let i = 0; i < 5; i++) { - await userService.changePassword(user.id, `${password}${i}`); + await userService.changePasswordWithPreviouslyUsedPasswordCheck( + user.id, + `${password}${i}`, + ); } await expect( - userService.changePassword(user.id, `${password}`), + userService.changePasswordWithPreviouslyUsedPasswordCheck( + user.id, + `${password}`, + ), ).resolves.not.toThrow(); // We've added 5 new passwords, so the original should work again }); + test('Can bypass check by directly calling the changePassword method', async () => { + const name = 'can-bypass-check-like-a-boss'; + 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}`), + ).resolves.not.toThrow(); // By bypassing the check, we can still set the same password as currently set + }); }); From 34da1a165b07696a0e4a3c25169e6f24026a9bef Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nuno=20G=C3=B3is?= Date: Tue, 9 Jul 2024 11:37:00 +0100 Subject: [PATCH 03/12] chore: password ui-ux improvements --- .../ChangePassword/ChangePassword.tsx | 8 ++- .../src/component/user/NewUser/NewUser.tsx | 17 +++-- .../user/Profile/PasswordTab/PasswordTab.tsx | 18 ++++- .../user/ResetPassword/ResetPassword.tsx | 19 +++--- .../ResetPasswordError/ResetPasswordError.tsx | 12 ++-- .../ResetPasswordForm/PasswordMatcher.tsx | 68 +++++++++---------- 6 files changed, 83 insertions(+), 59 deletions(-) diff --git a/frontend/src/component/admin/users/UsersList/ChangePassword/ChangePassword.tsx b/frontend/src/component/admin/users/UsersList/ChangePassword/ChangePassword.tsx index c0ebb44fa530..ea936328eba9 100644 --- a/frontend/src/component/admin/users/UsersList/ChangePassword/ChangePassword.tsx +++ b/frontend/src/component/admin/users/UsersList/ChangePassword/ChangePassword.tsx @@ -14,6 +14,7 @@ import type { IUser } from 'interfaces/user'; import useAdminUsersApi from 'hooks/api/actions/useAdminUsersApi/useAdminUsersApi'; import { UserAvatar } from 'component/common/UserAvatar/UserAvatar'; import useToast from 'hooks/useToast'; +import { formatUnknownError } from 'utils/formatUnknownError'; const StyledUserAvatar = styled(UserAvatar)(({ theme }) => ({ width: theme.spacing(5), @@ -37,7 +38,7 @@ const ChangePassword = ({ const [validPassword, setValidPassword] = useState(false); const { classes: themeStyles } = useThemeStyles(); const { changePassword } = useAdminUsersApi(); - const { setToastData } = useToast(); + const { setToastData, setToastApiError } = useToast(); const updateField: React.ChangeEventHandler = (event) => { setError(undefined); @@ -66,8 +67,9 @@ const ChangePassword = ({ type: 'success', }); } catch (error: unknown) { - console.warn(error); - setError(PASSWORD_FORMAT_MESSAGE); + const formattedError = formatUnknownError(error); + setError(formattedError); + setToastApiError(formattedError); } }; diff --git a/frontend/src/component/user/NewUser/NewUser.tsx b/frontend/src/component/user/NewUser/NewUser.tsx index 19adf66c200d..b7eae2f05989 100644 --- a/frontend/src/component/user/NewUser/NewUser.tsx +++ b/frontend/src/component/user/NewUser/NewUser.tsx @@ -21,7 +21,7 @@ export const NewUser = () => { const { authDetails } = useAuthDetails(); const { setToastApiError } = useToast(); const navigate = useNavigate(); - const [apiError, setApiError] = useState(false); + const [apiError, setApiError] = useState(''); const [email, setEmail] = useState(''); const [name, setName] = useState(''); const { @@ -61,10 +61,13 @@ export const NewUser = () => { if (res.status === OK) { navigate('/login?reset=true'); } else { - setApiError(true); + setApiError( + 'Something went wrong when attempting to update your password. This could be due to unstable internet connectivity. If retrying the request does not work, please try again later.', + ); } } catch (e) { - setApiError(true); + const error = formatUnknownError(e); + setApiError(error); } }; @@ -199,8 +202,12 @@ export const NewUser = () => { Set a password for your account. } + condition={isValidToken} + show={ + + {apiError} + + } /> diff --git a/frontend/src/component/user/Profile/PasswordTab/PasswordTab.tsx b/frontend/src/component/user/Profile/PasswordTab/PasswordTab.tsx index 3f39f4446175..89b4d3f42c5f 100644 --- a/frontend/src/component/user/Profile/PasswordTab/PasswordTab.tsx +++ b/frontend/src/component/user/Profile/PasswordTab/PasswordTab.tsx @@ -34,10 +34,20 @@ export const PasswordTab = () => { const [oldPassword, setOldPassword] = useState(''); const { changePassword } = usePasswordApi(); + const passwordsDoNotMatch = password !== confirmPassword; + const sameAsOldPassword = oldPassword === confirmPassword; + const allPasswordsFilled = + password.length > 0 && + confirmPassword.length > 0 && + oldPassword.length > 0; + + const hasError = + !allPasswordsFilled || passwordsDoNotMatch || sameAsOldPassword; + const submit = async (e: SyntheticEvent) => { e.preventDefault(); - if (password !== confirmPassword) { + if (hasError) { return; } else if (!validPassword) { setError(PASSWORD_FORMAT_MESSAGE); @@ -125,8 +135,9 @@ export const PasswordTab = () => { /> diff --git a/frontend/src/component/user/ResetPassword/ResetPassword.tsx b/frontend/src/component/user/ResetPassword/ResetPassword.tsx index 9604673ab397..51b895688810 100644 --- a/frontend/src/component/user/ResetPassword/ResetPassword.tsx +++ b/frontend/src/component/user/ResetPassword/ResetPassword.tsx @@ -11,6 +11,7 @@ import ResetPasswordForm from '../common/ResetPasswordForm/ResetPasswordForm'; import ResetPasswordError from '../common/ResetPasswordError/ResetPasswordError'; import { useAuthResetPasswordApi } from 'hooks/api/actions/useAuthResetPasswordApi/useAuthResetPasswordApi'; import { useAuthDetails } from 'hooks/api/getters/useAuth/useAuthDetails'; +import { formatUnknownError } from 'utils/formatUnknownError'; const StyledDiv = styled('div')(({ theme }) => ({ width: '350px', @@ -32,7 +33,7 @@ const ResetPassword = () => { const { authDetails } = useAuthDetails(); const ref = useLoading(loading || actionLoading); const navigate = useNavigate(); - const [hasApiError, setHasApiError] = useState(false); + const [apiError, setApiError] = useState(''); const passwordDisabled = authDetails?.defaultHidden === true; const onSubmit = async (password: string) => { @@ -40,12 +41,15 @@ const ResetPassword = () => { const res = await resetPassword({ token, password }); if (res.status === OK) { navigate('/login?reset=true'); - setHasApiError(false); + setApiError(''); } else { - setHasApiError(true); + setApiError( + 'Something went wrong when attempting to update your password. This could be due to unstable internet connectivity. If retrying the request does not work, please try again later.', + ); } } catch (e) { - setHasApiError(true); + const error = formatUnknownError(e); + setApiError(error); } }; @@ -62,10 +66,9 @@ const ResetPassword = () => { Reset password - } - /> + + {apiError} + } diff --git a/frontend/src/component/user/common/ResetPasswordError/ResetPasswordError.tsx b/frontend/src/component/user/common/ResetPasswordError/ResetPasswordError.tsx index 58924f90d95f..a7f274868cb2 100644 --- a/frontend/src/component/user/common/ResetPasswordError/ResetPasswordError.tsx +++ b/frontend/src/component/user/common/ResetPasswordError/ResetPasswordError.tsx @@ -1,12 +1,16 @@ import { Alert, AlertTitle } from '@mui/material'; -const ResetPasswordError = () => { +interface IResetPasswordErrorProps { + children: string; +} + +const ResetPasswordError = ({ children }: IResetPasswordErrorProps) => { + if (!children) return null; + return ( Unable to reset password - Something went wrong when attempting to update your password. This - could be due to unstable internet connectivity. If retrying the - request does not work, please try again later. + {children} ); }; diff --git a/frontend/src/component/user/common/ResetPasswordForm/PasswordMatcher.tsx b/frontend/src/component/user/common/ResetPasswordForm/PasswordMatcher.tsx index b06a576f3acf..856c5f334a69 100644 --- a/frontend/src/component/user/common/ResetPasswordForm/PasswordMatcher.tsx +++ b/frontend/src/component/user/common/ResetPasswordForm/PasswordMatcher.tsx @@ -1,59 +1,55 @@ -import { styled, Typography } from '@mui/material'; +import { styled } from '@mui/material'; import { ConditionallyRender } from 'component/common/ConditionallyRender/ConditionallyRender'; import CheckIcon from '@mui/icons-material/Check'; +import CloseIcon from '@mui/icons-material/Close'; interface IPasswordMatcherProps { started: boolean; - matchingPasswords: boolean; + passwordsDoNotMatch: boolean; + sameAsOldPassword?: boolean; } -const StyledMatcherContainer = styled('div')(({ theme }) => ({ - position: 'relative', - paddingTop: theme.spacing(0.5), -})); - -const StyledMatcher = styled(Typography, { - shouldForwardProp: (prop) => prop !== 'matchingPasswords', -})<{ matchingPasswords: boolean }>(({ theme, matchingPasswords }) => ({ - position: 'absolute', - bottom: '-8px', +const StyledMatcher = styled('div', { + shouldForwardProp: (prop) => prop !== 'error', +})<{ error: boolean }>(({ theme, error }) => ({ display: 'flex', alignItems: 'center', - color: matchingPasswords - ? theme.palette.primary.main - : theme.palette.error.main, + lineHeight: 1, + color: error ? theme.palette.error.main : theme.palette.primary.main, })); const StyledMatcherCheckIcon = styled(CheckIcon)(({ theme }) => ({ marginRight: '5px', })); +const StyledMatcherErrorIcon = styled(CloseIcon)({ + marginRight: '5px', +}); + const PasswordMatcher = ({ started, - matchingPasswords, + passwordsDoNotMatch, + sameAsOldPassword = false, }: IPasswordMatcherProps) => { + const error = passwordsDoNotMatch || sameAsOldPassword; + + if (!started) return null; + + const label = passwordsDoNotMatch + ? 'Passwords do not match' + : sameAsOldPassword + ? 'Cannot be the same as the old password' + : 'Passwords match'; + return ( - + - {' '} - Passwords match} - elseShow={ - Passwords do not match - } - /> - - } - /> - + condition={error} + show={} + elseShow={} + />{' '} + {label} + ); }; From 91a88d47cae7ace239eaf110ee66d567894561b6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nuno=20G=C3=B3is?= Date: Tue, 9 Jul 2024 11:39:10 +0100 Subject: [PATCH 04/12] fix: rate limit on reset password, no longer expire token on error --- src/lib/error/password-previously-used.ts | 2 +- src/lib/error/rate-limit-error.ts | 11 ++++++++ src/lib/error/unleash-error.ts | 2 ++ src/lib/services/user-service.ts | 26 +++++++++---------- .../e2e/services/user-service.e2e.test.ts | 4 +-- 5 files changed, 29 insertions(+), 16 deletions(-) create mode 100644 src/lib/error/rate-limit-error.ts diff --git a/src/lib/error/password-previously-used.ts b/src/lib/error/password-previously-used.ts index a55e902769b6..153f42bfda91 100644 --- a/src/lib/error/password-previously-used.ts +++ b/src/lib/error/password-previously-used.ts @@ -1,6 +1,6 @@ import { UnleashError } from './unleash-error'; -export class PasswordPreviouslyUsed extends UnleashError { +export class PasswordPreviouslyUsedError extends UnleashError { statusCode = 400; constructor( diff --git a/src/lib/error/rate-limit-error.ts b/src/lib/error/rate-limit-error.ts new file mode 100644 index 000000000000..20093f1e5463 --- /dev/null +++ b/src/lib/error/rate-limit-error.ts @@ -0,0 +1,11 @@ +import { UnleashError } from './unleash-error'; + +export class RateLimitError extends UnleashError { + statusCode = 429; + + constructor( + message: string = `We're currently receiving too much traffic. Please try again later.`, + ) { + super(message); + } +} diff --git a/src/lib/error/unleash-error.ts b/src/lib/error/unleash-error.ts index a3409bf3b87c..7fbda0e58ba9 100644 --- a/src/lib/error/unleash-error.ts +++ b/src/lib/error/unleash-error.ts @@ -29,6 +29,8 @@ export const UnleashApiErrorTypes = [ 'OwaspValidationError', 'ForbiddenError', 'ExceedsLimitError', + 'PasswordPreviouslyUsedError', + 'RateLimitError', // server errors; not the end user's fault 'InternalError', ] as const; diff --git a/src/lib/services/user-service.ts b/src/lib/services/user-service.ts index 5a97a7fd912a..c48a025e464e 100644 --- a/src/lib/services/user-service.ts +++ b/src/lib/services/user-service.ts @@ -12,7 +12,6 @@ import User, { import isEmail from '../util/is-email'; import type { AccessService } from './access-service'; import type ResetTokenService from './reset-token-service'; -import InvalidTokenError from '../error/invalid-token-error'; import NotFoundError from '../error/notfound-error'; import OwaspValidationError from '../error/owasp-validation-error'; import type { EmailService } from './email-service'; @@ -38,7 +37,8 @@ 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'; +import { PasswordPreviouslyUsedError } from '../error/password-previously-used'; +import { RateLimitError } from '../error/rate-limit-error'; export interface ICreateUser { name?: string; @@ -417,7 +417,7 @@ class UserService { password, ); if (previouslyUsed) { - throw new PasswordPreviouslyUsed(); + throw new PasswordPreviouslyUsedError(); } await this.changePassword(userId, password); @@ -470,18 +470,16 @@ class UserService { async resetPassword(token: string, password: string): Promise { this.validatePassword(password); const user = await this.getUserForToken(token); - const allowed = await this.resetTokenService.useAccessToken({ + + await this.changePasswordWithPreviouslyUsedPasswordCheck( + user.id, + password, + ); + + await this.resetTokenService.useAccessToken({ userId: user.id, token, }); - if (allowed) { - await this.changePasswordWithPreviouslyUsedPasswordCheck( - user.id, - password, - ); - } else { - throw new InvalidTokenError(); - } } async createResetPasswordEmail( @@ -496,7 +494,9 @@ class UserService { throw new NotFoundError(`Could not find ${receiverEmail}`); } if (this.passwordResetTimeouts[receiver.id]) { - return; + throw new RateLimitError( + 'You can only send one new reset password email per minute, per user. Please try again later.', + ); } const resetLink = await this.resetTokenService.createResetPasswordUrl( diff --git a/src/test/e2e/services/user-service.e2e.test.ts b/src/test/e2e/services/user-service.e2e.test.ts index 98048cfa5b79..5e284a0ee4c9 100644 --- a/src/test/e2e/services/user-service.e2e.test.ts +++ b/src/test/e2e/services/user-service.e2e.test.ts @@ -27,7 +27,7 @@ import { USER_UPDATED, } from '../../../lib/types'; import { CUSTOM_ROOT_ROLE_TYPE } from '../../../lib/util'; -import { PasswordPreviouslyUsed } from '../../../lib/error/password-previously-used'; +import { PasswordPreviouslyUsedError } from '../../../lib/error/password-previously-used'; let db: ITestDb; let stores: IUnleashStores; @@ -529,7 +529,7 @@ describe('Should not be able to use any of previous 5 passwords', () => { user.id, password, ), - ).rejects.toThrow(new PasswordPreviouslyUsed()); + ).rejects.toThrow(new PasswordPreviouslyUsedError()); }); test('Is still able to change password to one not used', async () => { const name = 'new-password-is-allowed'; From f69c552156c9bdbfd47854cc7772cafe1049c022 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nuno=20G=C3=B3is?= Date: Tue, 9 Jul 2024 11:44:32 +0100 Subject: [PATCH 05/12] fix: PasswordMatcher new passwordsDoNotMatch prop --- .../admin/users/UsersList/ChangePassword/ChangePassword.tsx | 2 +- .../user/common/ResetPasswordForm/ResetPasswordForm.tsx | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/frontend/src/component/admin/users/UsersList/ChangePassword/ChangePassword.tsx b/frontend/src/component/admin/users/UsersList/ChangePassword/ChangePassword.tsx index ea936328eba9..3534529650b0 100644 --- a/frontend/src/component/admin/users/UsersList/ChangePassword/ChangePassword.tsx +++ b/frontend/src/component/admin/users/UsersList/ChangePassword/ChangePassword.tsx @@ -136,7 +136,7 @@ const ChangePassword = ({ /> diff --git a/frontend/src/component/user/common/ResetPasswordForm/ResetPasswordForm.tsx b/frontend/src/component/user/common/ResetPasswordForm/ResetPasswordForm.tsx index fba63f9b2bd4..47b9dcd0cbee 100644 --- a/frontend/src/component/user/common/ResetPasswordForm/ResetPasswordForm.tsx +++ b/frontend/src/component/user/common/ResetPasswordForm/ResetPasswordForm.tsx @@ -95,7 +95,7 @@ const ResetPasswordForm = ({ onSubmit }: IResetPasswordProps) => { Date: Tue, 9 Jul 2024 11:46:20 +0100 Subject: [PATCH 06/12] refactor: unneeded theme arg --- .../user/common/ResetPasswordForm/PasswordMatcher.tsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frontend/src/component/user/common/ResetPasswordForm/PasswordMatcher.tsx b/frontend/src/component/user/common/ResetPasswordForm/PasswordMatcher.tsx index 856c5f334a69..9fd7b6df9fe6 100644 --- a/frontend/src/component/user/common/ResetPasswordForm/PasswordMatcher.tsx +++ b/frontend/src/component/user/common/ResetPasswordForm/PasswordMatcher.tsx @@ -18,9 +18,9 @@ const StyledMatcher = styled('div', { color: error ? theme.palette.error.main : theme.palette.primary.main, })); -const StyledMatcherCheckIcon = styled(CheckIcon)(({ theme }) => ({ +const StyledMatcherCheckIcon = styled(CheckIcon)({ marginRight: '5px', -})); +}); const StyledMatcherErrorIcon = styled(CloseIcon)({ marginRight: '5px', From 4d0c61dca3c12035704ceddb937e65aa53636ff5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nuno=20G=C3=B3is?= Date: Tue, 9 Jul 2024 12:01:16 +0100 Subject: [PATCH 07/12] test: fix password reset email throttle test --- src/lib/services/user-service.test.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/lib/services/user-service.test.ts b/src/lib/services/user-service.test.ts index 2884a11364bf..bb174e58b2ec 100644 --- a/src/lib/services/user-service.test.ts +++ b/src/lib/services/user-service.test.ts @@ -549,7 +549,9 @@ test('Should throttle password reset email', async () => { await expect(attempt1).resolves.toBeInstanceOf(URL); const attempt2 = service.createResetPasswordEmail('known@example.com'); - await expect(attempt2).resolves.toBe(undefined); + await expect(attempt2).rejects.toThrow( + 'You can only send one new reset password email per minute, per user. Please try again later.', + ); jest.runAllTimers(); From 8603aad1a218771d1f0a483641949b57ae484815 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nuno=20G=C3=B3is?= Date: Tue, 9 Jul 2024 12:17:59 +0100 Subject: [PATCH 08/12] test: remove test that is no longer relevant --- .../Profile/PasswordTab/PasswordTab.test.tsx | 36 ------------------- 1 file changed, 36 deletions(-) delete mode 100644 frontend/src/component/user/Profile/PasswordTab/PasswordTab.test.tsx diff --git a/frontend/src/component/user/Profile/PasswordTab/PasswordTab.test.tsx b/frontend/src/component/user/Profile/PasswordTab/PasswordTab.test.tsx deleted file mode 100644 index 42c8a7577157..000000000000 --- a/frontend/src/component/user/Profile/PasswordTab/PasswordTab.test.tsx +++ /dev/null @@ -1,36 +0,0 @@ -import { screen } from '@testing-library/react'; -import { render } from 'utils/testRenderer'; -import { testServerRoute, testServerSetup } from 'utils/testServer'; -import { PasswordTab } from './PasswordTab'; -import userEvent from '@testing-library/user-event'; - -const server = testServerSetup(); -testServerRoute(server, '/api/admin/ui-config', {}); -testServerRoute(server, '/api/admin/auth/simple/settings', { - disabled: false, -}); -testServerRoute(server, '/api/admin/user/change-password', {}, 'post', 401); -testServerRoute(server, '/auth/reset/validate-password', {}, 'post'); - -test('should render authorization error on missing old password', async () => { - const user = userEvent.setup(); - - render(); - - await screen.findByText('Change password'); - const passwordInput = await screen.findByLabelText('Password'); - await user.clear(passwordInput); - await user.type(passwordInput, 'IAmThePass1!@'); - - const confirmPasswordInput = - await screen.findByLabelText('Confirm password'); - await user.clear(confirmPasswordInput); - await user.type(confirmPasswordInput, 'IAmThePass1!@'); - - await screen.findByText('Passwords match'); - - const saveButton = await screen.findByText('Save'); - await user.click(saveButton); - - await screen.findByText('Authentication required'); -}); From 65e00bc50c47183890672f1b40245588ac7ff9f7 Mon Sep 17 00:00:00 2001 From: Christopher Kolstad Date: Tue, 9 Jul 2024 14:55:13 +0200 Subject: [PATCH 09/12] fix: moved check for already used hash to service --- src/lib/db/user-store.ts | 16 +++++---------- src/lib/routes/admin-api/user/user.test.ts | 1 - src/lib/services/user-service.ts | 9 ++++---- src/lib/types/stores/user-store.ts | 2 +- src/test/fixtures/fake-user-store.ts | 24 ++++++++++++---------- 5 files changed, 24 insertions(+), 28 deletions(-) diff --git a/src/lib/db/user-store.ts b/src/lib/db/user-store.ts index 5ac64ef445d6..06a69d439b60 100644 --- a/src/lib/db/user-store.ts +++ b/src/lib/db/user-store.ts @@ -11,7 +11,6 @@ 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'; @@ -75,16 +74,11 @@ 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 getPasswordsPreviouslyUsed(userId: number): Promise { + const previouslyUsedPasswords = await this.db(PASSWORD_HASH_TABLE) + .select('password_hash') + .where({ user_id: userId }); + return previouslyUsedPasswords.map((row) => row.password_hash); } async deletePasswordsUsedMoreThanNTimesAgo( diff --git a/src/lib/routes/admin-api/user/user.test.ts b/src/lib/routes/admin-api/user/user.test.ts index 2c3e8b31ce54..d92fac9113fe 100644 --- a/src/lib/routes/admin-api/user/user.test.ts +++ b/src/lib/routes/admin-api/user/user.test.ts @@ -54,7 +54,6 @@ test('should return current user', async () => { const owaspPassword = 't7GTx&$Y9pcsnxRv6'; test('should allow user to change password', async () => { - expect.assertions(1); const { request, base, userStore } = await getSetup(); await request .post(`${base}/api/admin/user/change-password`) diff --git a/src/lib/services/user-service.ts b/src/lib/services/user-service.ts index c48a025e464e..d09091879208 100644 --- a/src/lib/services/user-service.ts +++ b/src/lib/services/user-service.ts @@ -412,11 +412,12 @@ class UserService { userId: number, password: string, ): Promise { - const previouslyUsed = await this.store.passwordPreviouslyUsed( - userId, - password, + const previouslyUsed = + await this.store.getPasswordsPreviouslyUsed(userId); + const usedBefore = previouslyUsed.some((previouslyUsed) => + bcrypt.compareSync(password, previouslyUsed), ); - if (previouslyUsed) { + if (usedBefore) { throw new PasswordPreviouslyUsedError(); } diff --git a/src/lib/types/stores/user-store.ts b/src/lib/types/stores/user-store.ts index e738af7ccbf8..5cdb907c490c 100644 --- a/src/lib/types/stores/user-store.ts +++ b/src/lib/types/stores/user-store.ts @@ -33,7 +33,7 @@ export interface IUserStore extends Store { passwordHash: string, disallowNPreviousPasswords: number, ): Promise; - passwordPreviouslyUsed(userId: number, password: string): Promise; + getPasswordsPreviouslyUsed(userId: number): Promise; incLoginAttempts(user: IUser): Promise; successfullyLogin(user: IUser): Promise; count(): Promise; diff --git a/src/test/fixtures/fake-user-store.ts b/src/test/fixtures/fake-user-store.ts index 70cc5db8ebfa..01a2d161554d 100644 --- a/src/test/fixtures/fake-user-store.ts +++ b/src/test/fixtures/fake-user-store.ts @@ -8,19 +8,17 @@ import type { class UserStoreMock implements IUserStore { data: IUser[]; - + previousPasswords: Map; idSeq: number; constructor() { this.idSeq = 1; this.data = []; + this.previousPasswords = new Map(); } - passwordPreviouslyUsed( - _userId: number, - _passwordHash: string, - ): Promise { - return Promise.resolve(false); + getPasswordsPreviouslyUsed(userId: number): Promise { + return Promise.resolve(this.previousPasswords.get(userId) || []); } countServiceAccounts(): Promise { return Promise.resolve(0); @@ -54,7 +52,7 @@ class UserStoreMock implements IUserStore { } async get(key: number): Promise { - return this.data.find((u) => u.id === key); + return this.data.find((u) => u.id === key)!; } async insert(user: User): Promise { @@ -93,6 +91,9 @@ class UserStoreMock implements IUserStore { const u = this.data.find((a) => a.id === userId); // @ts-expect-error u.passwordHash = passwordHash; + const previousPasswords = this.previousPasswords.get(userId) || []; + previousPasswords.push(passwordHash); + this.previousPasswords.set(userId, previousPasswords.slice(1, 6)); return Promise.resolve(); } @@ -139,7 +140,7 @@ class UserStoreMock implements IUserStore { upsert(user: ICreateUser): Promise { this.data.splice(this.data.findIndex((u) => u.email === user.email)); - this.data.push({ + const userToReturn = { id: this.data.length + 1, createdAt: new Date(), isAPI: false, @@ -150,13 +151,14 @@ class UserStoreMock implements IUserStore { username: user.username ?? '', email: user.email ?? '', ...user, - }); - return Promise.resolve(undefined); + }; + this.data.push(userToReturn); + return Promise.resolve(userToReturn); } // eslint-disable-next-line @typescript-eslint/no-unused-vars getUserByPersonalAccessToken(secret: string): Promise { - return Promise.resolve(undefined); + throw new Error('Not implemented'); } // eslint-disable-next-line @typescript-eslint/no-unused-vars From 602de8af8431378b71efe75a05f58750477231e8 Mon Sep 17 00:00:00 2001 From: Christopher Kolstad Date: Tue, 9 Jul 2024 15:03:18 +0200 Subject: [PATCH 10/12] remove unnecessary if check --- src/lib/db/user-store.ts | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/src/lib/db/user-store.ts b/src/lib/db/user-store.ts index 06a69d439b60..71d5d9ba064c 100644 --- a/src/lib/db/user-store.ts +++ b/src/lib/db/user-store.ts @@ -212,16 +212,14 @@ class UserStore implements IUserStore { 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, - ); - } + await this.db(PASSWORD_HASH_TABLE).insert({ + user_id: userId, + password_hash: passwordHash, + }); + await this.deletePasswordsUsedMoreThanNTimesAgo( + userId, + disallowNPreviousPasswords, + ); } async incLoginAttempts(user: User): Promise { From f8e19f16b149ccb54d735ca10d84bb569d589649 Mon Sep 17 00:00:00 2001 From: Christopher Kolstad Date: Tue, 9 Jul 2024 15:56:09 +0200 Subject: [PATCH 11/12] fix: readd if for passwordHash --- src/lib/db/user-store.ts | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/src/lib/db/user-store.ts b/src/lib/db/user-store.ts index 71d5d9ba064c..80272b7aecf3 100644 --- a/src/lib/db/user-store.ts +++ b/src/lib/db/user-store.ts @@ -212,14 +212,17 @@ class UserStore implements IUserStore { await this.activeUsers().where('id', userId).update({ password_hash: passwordHash, }); - await this.db(PASSWORD_HASH_TABLE).insert({ - user_id: userId, - password_hash: passwordHash, - }); - await this.deletePasswordsUsedMoreThanNTimesAgo( - userId, - disallowNPreviousPasswords, - ); + // We apparently set this to null, but you should be allowed to have null, so need to allow this + 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 { From d9204517ad50efaf3d819262aa038c0dc3453f9e Mon Sep 17 00:00:00 2001 From: Christopher Kolstad Date: Tue, 9 Jul 2024 16:01:42 +0200 Subject: [PATCH 12/12] Update src/lib/error/password-previously-used.ts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Nuno Góis --- src/lib/error/password-previously-used.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/lib/error/password-previously-used.ts b/src/lib/error/password-previously-used.ts index 153f42bfda91..6916d570fdcd 100644 --- a/src/lib/error/password-previously-used.ts +++ b/src/lib/error/password-previously-used.ts @@ -4,7 +4,7 @@ export class PasswordPreviouslyUsedError extends UnleashError { statusCode = 400; constructor( - message: string = `You've previously used this password, please use a new password`, + message: string = `You've previously used this password. Please use a new password.`, ) { super(message); }