diff --git a/frontend/src/component/user/Profile/PasswordTab/PasswordTab.test.tsx b/frontend/src/component/user/Profile/PasswordTab/PasswordTab.test.tsx new file mode 100644 index 0000000000..40153bb1c7 --- /dev/null +++ b/frontend/src/component/user/Profile/PasswordTab/PasswordTab.test.tsx @@ -0,0 +1,42 @@ +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'; +import { UIProviderContainer } from '../../../providers/UIProvider/UIProviderContainer'; + +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 3725f92544..53760df5d2 100644 --- a/frontend/src/component/user/Profile/PasswordTab/PasswordTab.tsx +++ b/frontend/src/component/user/Profile/PasswordTab/PasswordTab.tsx @@ -11,6 +11,7 @@ import useAuthSettings from 'hooks/api/getters/useAuthSettings/useAuthSettings'; import useToast from 'hooks/useToast'; import { SyntheticEvent, useState } from 'react'; import { formatUnknownError } from 'utils/formatUnknownError'; +import { AuthenticationError } from 'utils/apiUtils'; const StyledForm = styled('form')(({ theme }) => ({ display: 'flex', @@ -27,8 +28,10 @@ export const PasswordTab = () => { const { setToastData, setToastApiError } = useToast(); const [validPassword, setValidPassword] = useState(false); const [error, setError] = useState(); + const [authenticationError, setAuthenticationError] = useState(); const [password, setPassword] = useState(''); const [confirmPassword, setConfirmPassword] = useState(''); + const [oldPassword, setOldPassword] = useState(''); const { changePassword } = usePasswordApi(); const submit = async (e: SyntheticEvent) => { @@ -41,10 +44,12 @@ export const PasswordTab = () => { } else { setLoading(true); setError(undefined); + setAuthenticationError(undefined); try { await changePassword({ password, confirmPassword, + oldPassword, }); setToastData({ title: 'Password changed successfully', @@ -53,7 +58,12 @@ export const PasswordTab = () => { }); } catch (error: unknown) { const formattedError = formatUnknownError(error); - setError(formattedError); + if (error instanceof AuthenticationError) { + setAuthenticationError(formattedError); + } else { + setError(formattedError); + } + setToastApiError(formattedError); } } @@ -79,6 +89,18 @@ export const PasswordTab = () => { callback={setValidPassword} data-loading /> + + ) => setOldPassword(e.target.value)} + /> { diff --git a/frontend/src/utils/testServer.ts b/frontend/src/utils/testServer.ts index 03bac95529..8e5c3259df 100644 --- a/frontend/src/utils/testServer.ts +++ b/frontend/src/utils/testServer.ts @@ -15,11 +15,12 @@ export const testServerRoute = ( server: SetupServerApi, path: string, json: object, - method: 'get' | 'post' | 'put' | 'delete' = 'get' + method: 'get' | 'post' | 'put' | 'delete' = 'get', + status: number = 200 ) => { server.use( rest[method](path, (req, res, ctx) => { - return res(ctx.json(json)); + return res(ctx.status(status), ctx.json(json)); }) ); }; diff --git a/src/lib/openapi/spec/password-schema.ts b/src/lib/openapi/spec/password-schema.ts index ff4118e398..05f481c05c 100644 --- a/src/lib/openapi/spec/password-schema.ts +++ b/src/lib/openapi/spec/password-schema.ts @@ -9,6 +9,9 @@ export const passwordSchema = { password: { type: 'string', }, + oldPassword: { + type: 'string', + }, confirmPassword: { type: 'string', }, diff --git a/src/lib/routes/admin-api/user/user.test.ts b/src/lib/routes/admin-api/user/user.test.ts index 313448c361..0d488e8959 100644 --- a/src/lib/routes/admin-api/user/user.test.ts +++ b/src/lib/routes/admin-api/user/user.test.ts @@ -5,13 +5,19 @@ import { createTestConfig } from '../../../../test/config/test-config'; import createStores from '../../../../test/fixtures/store'; import getApp from '../../../app'; import User from '../../../types/user'; +import bcrypt from 'bcryptjs'; const currentUser = new User({ id: 1337, email: 'test@mail.com' }); +const oldPassword = 'old-pass'; async function getSetup() { const base = `/random${Math.round(Math.random() * 1000)}`; const stores = createStores(); await stores.userStore.insert(currentUser); + await stores.userStore.setPasswordHash( + currentUser.id, + await bcrypt.hash(oldPassword, 10), + ); const config = createTestConfig({ preHook: (a) => { @@ -47,20 +53,44 @@ test('should return current user', async () => { const owaspPassword = 't7GTx&$Y9pcsnxRv6'; test('should allow user to change password', async () => { - expect.assertions(2); + expect.assertions(1); const { request, base, userStore } = await getSetup(); - const before = await userStore.get(currentUser.id); - // @ts-ignore - expect(before.passwordHash).toBeFalsy(); await request .post(`${base}/api/admin/user/change-password`) - .send({ password: owaspPassword, confirmPassword: owaspPassword }) + .send({ + password: owaspPassword, + confirmPassword: owaspPassword, + oldPassword, + }) .expect(200); const updated = await userStore.get(currentUser.id); // @ts-ignore expect(updated.passwordHash).toBeTruthy(); }); +test('should not allow user to change password with incorrect old password', async () => { + const { request, base } = await getSetup(); + await request + .post(`${base}/api/admin/user/change-password`) + .send({ + password: owaspPassword, + confirmPassword: owaspPassword, + oldPassword: 'incorrect', + }) + .expect(401); +}); + +test('should not allow user to change password without providing old password', async () => { + const { request, base } = await getSetup(); + await request + .post(`${base}/api/admin/user/change-password`) + .send({ + password: owaspPassword, + confirmPassword: owaspPassword, + }) + .expect(400); +}); + test('should deny if password and confirmPassword are not equal', async () => { expect.assertions(0); const { request, base } = await getSetup(); diff --git a/src/lib/routes/admin-api/user/user.ts b/src/lib/routes/admin-api/user/user.ts index d09c53c8b8..4b71b2a887 100644 --- a/src/lib/routes/admin-api/user/user.ts +++ b/src/lib/routes/admin-api/user/user.ts @@ -102,7 +102,8 @@ class UserController extends Controller { requestBody: createRequestSchema('passwordSchema'), responses: { 200: emptyResponse, - 400: { description: 'passwordMismatch' }, + 400: { description: 'password mismatch' }, + 401: { description: 'incorrect old password' }, }, }), ], @@ -167,10 +168,14 @@ class UserController extends Controller { res: Response, ): Promise { const { user } = req; - const { password, confirmPassword } = req.body; - if (password === confirmPassword) { + const { password, confirmPassword, oldPassword } = req.body; + if (password === confirmPassword && oldPassword != null) { this.userService.validatePassword(password); - await this.userService.changePassword(user.id, password); + await this.userService.changePasswordWithVerification( + user.id, + password, + oldPassword, + ); res.status(200).end(); } else { res.status(400).end(); diff --git a/src/lib/services/user-service.ts b/src/lib/services/user-service.ts index 9eed2c7d9c..11a6daeb7e 100644 --- a/src/lib/services/user-service.ts +++ b/src/lib/services/user-service.ts @@ -362,6 +362,22 @@ class UserService { await this.resetTokenService.expireExistingTokensForUser(userId); } + async changePasswordWithVerification( + userId: number, + newPassword: string, + oldPassword: string, + ): Promise { + const currentPasswordHash = await this.store.getPasswordHash(userId); + const match = await bcrypt.compare(oldPassword, currentPasswordHash); + if (!match) { + throw new PasswordMismatch( + `The old password you provided is invalid. If you have forgotten your password, visit ${this.baseUriPath}/forgotten-password or get in touch with your instance administrator.`, + ); + } + + await this.changePassword(userId, newPassword); + } + async getUserForToken(token: string): Promise { const { createdBy, userId } = await this.resetTokenService.isValid( token, diff --git a/src/test/e2e/api/openapi/__snapshots__/openapi.e2e.test.ts.snap b/src/test/e2e/api/openapi/__snapshots__/openapi.e2e.test.ts.snap index 1c53163698..d76938edfa 100644 --- a/src/test/e2e/api/openapi/__snapshots__/openapi.e2e.test.ts.snap +++ b/src/test/e2e/api/openapi/__snapshots__/openapi.e2e.test.ts.snap @@ -3146,6 +3146,9 @@ The provider you choose for your addon dictates what properties the \`parameters "confirmPassword": { "type": "string", }, + "oldPassword": { + "type": "string", + }, "password": { "type": "string", }, @@ -12784,7 +12787,10 @@ If the provided project does not exist, the list of events will be empty.", "description": "This response has no body.", }, "400": { - "description": "passwordMismatch", + "description": "password mismatch", + }, + "401": { + "description": "incorrect old password", }, }, "tags": [