Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Disallow repeating last 5 passwords. #7552

Merged
merged 12 commits into from
Jul 9, 2024
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand All @@ -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);
Expand Down Expand Up @@ -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);
}
};

Expand Down Expand Up @@ -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>
Expand Down
17 changes: 12 additions & 5 deletions frontend/src/component/user/NewUser/NewUser.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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);
}
};

Expand Down Expand Up @@ -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} />
</>
Expand Down
18 changes: 15 additions & 3 deletions frontend/src/component/user/Profile/PasswordTab/PasswordTab.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -125,15 +135,17 @@ export const PasswordTab = () => {
/>
<PasswordMatcher
data-loading
started={Boolean(password && confirmPassword)}
matchingPasswords={password === confirmPassword}
started={allPasswordsFilled}
passwordsDoNotMatch={passwordsDoNotMatch}
sameAsOldPassword={sameAsOldPassword}
/>
<Button
data-loading
variant='contained'
color='primary'
type='submit'
onClick={submit}
disabled={hasError}
>
Save
</Button>
Expand Down
19 changes: 11 additions & 8 deletions frontend/src/component/user/ResetPassword/ResetPassword.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand All @@ -32,20 +33,23 @@ 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) => {
try {
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);
}
};

Expand All @@ -62,10 +66,9 @@ const ResetPassword = () => {
Reset password
</StyledTypography>

<ConditionallyRender
condition={hasApiError}
show={<ResetPasswordError />}
/>
<ResetPasswordError>
{apiError}
</ResetPasswordError>
<ResetPasswordForm onSubmit={onSubmit} />
</>
}
Expand Down
Original file line number Diff line number Diff line change
@@ -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>
);
};
Expand Down
Original file line number Diff line number Diff line change
@@ -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>
);
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ const ResetPasswordForm = ({ onSubmit }: IResetPasswordProps) => {

<PasswordMatcher
started={started}
matchingPasswords={matchingPasswords}
passwordsDoNotMatch={!matchingPasswords}
/>
<StyledButton
variant='contained'
Expand Down
51 changes: 49 additions & 2 deletions src/lib/db/user-store.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand All @@ -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) => {
Expand Down Expand Up @@ -71,6 +75,35 @@ class UserStore implements IUserStore {
this.logger = getLogger('user-store.ts');
}

async passwordPreviouslyUsed(
userId: number,
password: string,
): Promise<boolean> {
const previouslyUsedPasswords = await this.db(
PASSWORD_HASH_TABLE,
).where({ user_id: userId });
return previouslyUsedPasswords.some((row) =>
bcrypt.compareSync(password, row.password_hash),
);
}
chriswk marked this conversation as resolved.
Show resolved Hide resolved

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)
Expand Down Expand Up @@ -177,10 +210,24 @@ 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,
Copy link
Member

@nunogois nunogois Jul 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this have a default so we don't need to pass it every time?

): Promise<void> {
await this.activeUsers().where('id', userId).update({
password_hash: passwordHash,
});
if (passwordHash) {
chriswk marked this conversation as resolved.
Show resolved Hide resolved
await this.db(PASSWORD_HASH_TABLE).insert({
user_id: userId,
password_hash: passwordHash,
});
await this.deletePasswordsUsedMoreThanNTimesAgo(
userId,
disallowNPreviousPasswords,
);
}
}

async incLoginAttempts(user: User): Promise<void> {
Expand Down
11 changes: 11 additions & 0 deletions src/lib/error/password-previously-used.ts
Original file line number Diff line number Diff line change
@@ -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`,
chriswk marked this conversation as resolved.
Show resolved Hide resolved
) {
super(message);
}
}
Loading
Loading