mirror of
				https://github.com/Unleash/unleash.git
				synced 2025-10-27 11:02:16 +01:00 
			
		
		
		
	feat: Disallow repeating last 5 passwords. (#7552)
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. Includes some password-related UI/UX improvements and refactors. Also some fixes related to reset password rate limiting (instead of an unhandled exception), and token expiration on error. --------- Co-authored-by: Nuno Góis <github@nunogois.com>
This commit is contained in:
		
							parent
							
								
									ef3ef877b3
								
							
						
					
					
						commit
						f65afff6c1
					
				| @ -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<HTMLInputElement> = (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 = ({ | ||||
|                 /> | ||||
|                 <PasswordMatcher | ||||
|                     started={Boolean(data.password && data.confirm)} | ||||
|                     matchingPasswords={data.password === data.confirm} | ||||
|                     passwordsDoNotMatch={data.password !== data.confirm} | ||||
|                 /> | ||||
|             </form> | ||||
|         </Dialogue> | ||||
|  | ||||
| @ -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. | ||||
|                         </Typography> | ||||
|                         <ConditionallyRender | ||||
|                             condition={apiError && isValidToken} | ||||
|                             show={<ResetPasswordError />} | ||||
|                             condition={isValidToken} | ||||
|                             show={ | ||||
|                                 <ResetPasswordError> | ||||
|                                     {apiError} | ||||
|                                 </ResetPasswordError> | ||||
|                             } | ||||
|                         /> | ||||
|                         <ResetPasswordForm onSubmit={onSubmit} /> | ||||
|                     </> | ||||
|  | ||||
| @ -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(<PasswordTab />); | ||||
| 
 | ||||
|     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'); | ||||
| }); | ||||
| @ -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 = () => { | ||||
|                         /> | ||||
|                         <PasswordMatcher | ||||
|                             data-loading | ||||
|                             started={Boolean(password && confirmPassword)} | ||||
|                             matchingPasswords={password === confirmPassword} | ||||
|                             started={allPasswordsFilled} | ||||
|                             passwordsDoNotMatch={passwordsDoNotMatch} | ||||
|                             sameAsOldPassword={sameAsOldPassword} | ||||
|                         /> | ||||
|                         <Button | ||||
|                             data-loading | ||||
| @ -134,6 +145,7 @@ export const PasswordTab = () => { | ||||
|                             color='primary' | ||||
|                             type='submit' | ||||
|                             onClick={submit} | ||||
|                             disabled={hasError} | ||||
|                         > | ||||
|                             Save | ||||
|                         </Button> | ||||
|  | ||||
| @ -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 | ||||
|                                 </StyledTypography> | ||||
| 
 | ||||
|                                 <ConditionallyRender | ||||
|                                     condition={hasApiError} | ||||
|                                     show={<ResetPasswordError />} | ||||
|                                 /> | ||||
|                                 <ResetPasswordError> | ||||
|                                     {apiError} | ||||
|                                 </ResetPasswordError> | ||||
|                                 <ResetPasswordForm onSubmit={onSubmit} /> | ||||
|                             </> | ||||
|                         } | ||||
|  | ||||
| @ -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 ( | ||||
|         <Alert severity='error' data-loading> | ||||
|             <AlertTitle>Unable to reset password</AlertTitle> | ||||
|             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} | ||||
|         </Alert> | ||||
|     ); | ||||
| }; | ||||
|  | ||||
| @ -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 ( | ||||
|         <StyledMatcherContainer> | ||||
|         <StyledMatcher data-loading error={error}> | ||||
|             <ConditionallyRender | ||||
|                 condition={started} | ||||
|                 show={ | ||||
|                     <StyledMatcher | ||||
|                         variant='body2' | ||||
|                         data-loading | ||||
|                         matchingPasswords={matchingPasswords} | ||||
|                     > | ||||
|                         <StyledMatcherCheckIcon />{' '} | ||||
|                         <ConditionallyRender | ||||
|                             condition={matchingPasswords} | ||||
|                             show={<Typography> Passwords match</Typography>} | ||||
|                             elseShow={ | ||||
|                                 <Typography> Passwords do not match</Typography> | ||||
|                             } | ||||
|                         /> | ||||
|                     </StyledMatcher> | ||||
|                 } | ||||
|             /> | ||||
|         </StyledMatcherContainer> | ||||
|                 condition={error} | ||||
|                 show={<StyledMatcherErrorIcon />} | ||||
|                 elseShow={<StyledMatcherCheckIcon />} | ||||
|             />{' '} | ||||
|             <span>{label}</span> | ||||
|         </StyledMatcher> | ||||
|     ); | ||||
| }; | ||||
| 
 | ||||
|  | ||||
| @ -95,7 +95,7 @@ const ResetPasswordForm = ({ onSubmit }: IResetPasswordProps) => { | ||||
| 
 | ||||
|             <PasswordMatcher | ||||
|                 started={started} | ||||
|                 matchingPasswords={matchingPasswords} | ||||
|                 passwordsDoNotMatch={!matchingPasswords} | ||||
|             /> | ||||
|             <StyledButton | ||||
|                 variant='contained' | ||||
|  | ||||
| @ -13,6 +13,7 @@ import type { | ||||
| import type { Db } from './db'; | ||||
| 
 | ||||
| const TABLE = 'users'; | ||||
| const PASSWORD_HASH_TABLE = 'used_passwords'; | ||||
| 
 | ||||
| const USER_COLUMNS_PUBLIC = [ | ||||
|     'id', | ||||
| @ -25,6 +26,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 +74,30 @@ class UserStore implements IUserStore { | ||||
|         this.logger = getLogger('user-store.ts'); | ||||
|     } | ||||
| 
 | ||||
|     async getPasswordsPreviouslyUsed(userId: number): Promise<string[]> { | ||||
|         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<void> { | ||||
|         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<User> { | ||||
|         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<void> { | ||||
|         return this.activeUsers().where('id', userId).update({ | ||||
|     async setPasswordHash( | ||||
|         userId: number, | ||||
|         passwordHash: string, | ||||
|         disallowNPreviousPasswords: number, | ||||
|     ): Promise<void> { | ||||
|         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<void> { | ||||
|  | ||||
							
								
								
									
										11
									
								
								src/lib/error/password-previously-used.ts
									
									
									
									
									
										Normal file
									
								
							
							
						
						
									
										11
									
								
								src/lib/error/password-previously-used.ts
									
									
									
									
									
										Normal file
									
								
							| @ -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); | ||||
|     } | ||||
| } | ||||
							
								
								
									
										11
									
								
								src/lib/error/rate-limit-error.ts
									
									
									
									
									
										Normal file
									
								
							
							
						
						
									
										11
									
								
								src/lib/error/rate-limit-error.ts
									
									
									
									
									
										Normal file
									
								
							| @ -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); | ||||
|     } | ||||
| } | ||||
| @ -29,6 +29,8 @@ export const UnleashApiErrorTypes = [ | ||||
|     'OwaspValidationError', | ||||
|     'ForbiddenError', | ||||
|     'ExceedsLimitError', | ||||
|     'PasswordPreviouslyUsedError', | ||||
|     'RateLimitError', | ||||
|     // server errors; not the end user's fault
 | ||||
|     'InternalError', | ||||
| ] as const; | ||||
|  | ||||
| @ -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`) | ||||
|  | ||||
| @ -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(); | ||||
| 
 | ||||
|  | ||||
| @ -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<void> { | ||||
|         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<void> { | ||||
|         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<TokenUserSchema> { | ||||
| @ -437,15 +471,16 @@ class UserService { | ||||
|     async resetPassword(token: string, password: string): Promise<void> { | ||||
|         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( | ||||
|  | ||||
| @ -28,7 +28,12 @@ export interface IUserStore extends Store<IUser, number> { | ||||
|     getAllWithId(userIdList: number[]): Promise<IUser[]>; | ||||
|     getByQuery(idQuery: IUserLookup): Promise<IUser>; | ||||
|     getPasswordHash(userId: number): Promise<string>; | ||||
|     setPasswordHash(userId: number, passwordHash: string): Promise<void>; | ||||
|     setPasswordHash( | ||||
|         userId: number, | ||||
|         passwordHash: string, | ||||
|         disallowNPreviousPasswords: number, | ||||
|     ): Promise<void>; | ||||
|     getPasswordsPreviouslyUsed(userId: number): Promise<string[]>; | ||||
|     incLoginAttempts(user: IUser): Promise<void>; | ||||
|     successfullyLogin(user: IUser): Promise<void>; | ||||
|     count(): Promise<number>; | ||||
|  | ||||
							
								
								
									
										15
									
								
								src/migrations/20240705111827-used-passwords-table.js
									
									
									
									
									
										Normal file
									
								
							
							
						
						
									
										15
									
								
								src/migrations/20240705111827-used-passwords-table.js
									
									
									
									
									
										Normal file
									
								
							| @ -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); | ||||
| }; | ||||
| @ -13,6 +13,7 @@ function mergeAll<T>(objects: Partial<T>[]): T { | ||||
| } | ||||
| 
 | ||||
| export function createTestConfig(config?: IUnleashOptions): IUnleashConfig { | ||||
|     getLogger.setMuteError(true); | ||||
|     const testConfig: IUnleashOptions = { | ||||
|         getLogger, | ||||
|         authentication: { type: IAuthType.NONE, createAdminUser: false }, | ||||
|  | ||||
| @ -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
 | ||||
|  | ||||
| @ -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
 | ||||
|     }); | ||||
| }); | ||||
|  | ||||
| @ -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'); | ||||
|  | ||||
							
								
								
									
										21
									
								
								src/test/fixtures/fake-user-store.ts
									
									
									
									
										vendored
									
									
								
							
							
						
						
									
										21
									
								
								src/test/fixtures/fake-user-store.ts
									
									
									
									
										vendored
									
									
								
							| @ -8,12 +8,17 @@ import type { | ||||
| 
 | ||||
| class UserStoreMock implements IUserStore { | ||||
|     data: IUser[]; | ||||
| 
 | ||||
|     previousPasswords: Map<number, string[]>; | ||||
|     idSeq: number; | ||||
| 
 | ||||
|     constructor() { | ||||
|         this.idSeq = 1; | ||||
|         this.data = []; | ||||
|         this.previousPasswords = new Map(); | ||||
|     } | ||||
| 
 | ||||
|     getPasswordsPreviouslyUsed(userId: number): Promise<string[]> { | ||||
|         return Promise.resolve(this.previousPasswords.get(userId) || []); | ||||
|     } | ||||
|     countServiceAccounts(): Promise<number> { | ||||
|         return Promise.resolve(0); | ||||
| @ -47,7 +52,7 @@ class UserStoreMock implements IUserStore { | ||||
|     } | ||||
| 
 | ||||
|     async get(key: number): Promise<IUser> { | ||||
|         return this.data.find((u) => u.id === key); | ||||
|         return this.data.find((u) => u.id === key)!; | ||||
|     } | ||||
| 
 | ||||
|     async insert(user: User): Promise<User> { | ||||
| @ -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<IUser> { | ||||
|         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<IUser> { | ||||
|         return Promise.resolve(undefined); | ||||
|         throw new Error('Not implemented'); | ||||
|     } | ||||
| 
 | ||||
|     // eslint-disable-next-line @typescript-eslint/no-unused-vars
 | ||||
|  | ||||
		Loading…
	
		Reference in New Issue
	
	Block a user