diff --git a/frontend/src/component/admin/users/UsersList/ChangePassword/ChangePassword.tsx b/frontend/src/component/admin/users/UsersList/ChangePassword/ChangePassword.tsx index c0ebb44fa530..3534529650b0 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); } }; @@ -134,7 +136,7 @@ const ChangePassword = ({ /> 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.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'); -}); 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..9fd7b6df9fe6 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 }) => ({ +const StyledMatcherCheckIcon = styled(CheckIcon)({ 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} + ); }; 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) => { { @@ -71,6 +74,30 @@ class UserStore implements IUserStore { this.logger = getLogger('user-store.ts'); } + 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( + 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 +204,25 @@ 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, }); + // 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 { diff --git a/src/lib/error/password-previously-used.ts b/src/lib/error/password-previously-used.ts new file mode 100644 index 000000000000..6916d570fdcd --- /dev/null +++ b/src/lib/error/password-previously-used.ts @@ -0,0 +1,11 @@ +import { UnleashError } from './unleash-error'; + +export class PasswordPreviouslyUsedError 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/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/routes/admin-api/user/user.test.ts b/src/lib/routes/admin-api/user/user.test.ts index 0d488e8959aa..d92fac9113fe 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({ @@ -53,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.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(); diff --git a/src/lib/services/user-service.ts b/src/lib/services/user-service.ts index c0d1a1c87b21..d09091879208 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,6 +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 { PasswordPreviouslyUsedError } from '../error/password-previously-used'; +import { RateLimitError } from '../error/rate-limit-error'; 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); @@ -388,11 +398,32 @@ class UserService { async changePassword(userId: number, password: string): Promise { this.validatePassword(password); 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); } + async changePasswordWithPreviouslyUsedPasswordCheck( + userId: number, + password: string, + ): Promise { + const previouslyUsed = + await this.store.getPasswordsPreviouslyUsed(userId); + const usedBefore = previouslyUsed.some((previouslyUsed) => + bcrypt.compareSync(password, previouslyUsed), + ); + if (usedBefore) { + throw new PasswordPreviouslyUsedError(); + } + + await this.changePassword(userId, password); + } + async changePasswordWithVerification( userId: number, newPassword: string, @@ -406,7 +437,10 @@ class UserService { ); } - await this.changePassword(userId, newPassword); + await this.changePasswordWithPreviouslyUsedPasswordCheck( + userId, + newPassword, + ); } async getUserForToken(token: string): Promise { @@ -437,15 +471,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.changePassword(user.id, password); - } else { - throw new InvalidTokenError(); - } } async createResetPasswordEmail( @@ -460,7 +495,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/lib/types/stores/user-store.ts b/src/lib/types/stores/user-store.ts index 96f9abed9943..5cdb907c490c 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; + getPasswordsPreviouslyUsed(userId: number): 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..5e284a0ee4c9 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 { PasswordPreviouslyUsedError } from '../../../lib/error/password-previously-used'; let db: ITestDb; let stores: IUnleashStores; @@ -511,3 +512,77 @@ 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.changePasswordWithPreviouslyUsedPasswordCheck( + user.id, + password, + ), + ).rejects.toThrow(new PasswordPreviouslyUsedError()); + }); + 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.changePasswordWithPreviouslyUsedPasswordCheck( + 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.changePasswordWithPreviouslyUsedPasswordCheck( + user.id, + `${password}${i}`, + ); + } + await expect( + 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 + }); +}); 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..01a2d161554d 100644 --- a/src/test/fixtures/fake-user-store.ts +++ b/src/test/fixtures/fake-user-store.ts @@ -8,12 +8,17 @@ import type { class UserStoreMock implements IUserStore { data: IUser[]; - + previousPasswords: Map; idSeq: number; constructor() { this.idSeq = 1; this.data = []; + this.previousPasswords = new Map(); + } + + getPasswordsPreviouslyUsed(userId: number): Promise { + return Promise.resolve(this.previousPasswords.get(userId) || []); } countServiceAccounts(): Promise { return Promise.resolve(0); @@ -47,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 { @@ -86,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(); } @@ -132,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, @@ -143,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