feat: change own password confirmation (#3894)

Unleash · Jun 5, 2023 · 5ec59c6 · 5ec59c6
1 parent ae11360
commit 5ec59c6
Show file tree

Hide file tree

Showing 9 changed files with 139 additions and 13 deletions.
diff --git a/frontend/src/component/user/Profile/PasswordTab/PasswordTab.test.tsx 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(
+        <UIProviderContainer>
+            <PasswordTab />
+        </UIProviderContainer>
+    );
+
+    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
@@ -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<string>();
+    const [authenticationError, setAuthenticationError] = useState<string>();
     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
                         />
+                        <PasswordField
+                            data-loading
+                            label="Old password"
+                            name="oldPassword"
+                            value={oldPassword}
+                            error={Boolean(authenticationError)}
+                            helperText={authenticationError}
+                            autoComplete="current-password"
+                            onChange={(
+                                e: React.ChangeEvent<HTMLInputElement>
+                            ) => setOldPassword(e.target.value)}
+                        />
                         <PasswordField
                             data-loading
                             label="Password"

diff --git a/frontend/src/hooks/api/actions/usePasswordApi/usePasswordApi.ts b/frontend/src/hooks/api/actions/usePasswordApi/usePasswordApi.ts
@@ -3,6 +3,7 @@ import useAPI from '../useApi/useApi';
 interface IChangePasswordPayload {
     password: string;
     confirmPassword: string;
+    oldPassword: string;
 }
 
 export const usePasswordApi = () => {

diff --git 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
@@ -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
@@ -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
@@ -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<void> {
         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
@@ -362,6 +362,22 @@ class UserService {
         await this.resetTokenService.expireExistingTokensForUser(userId);
     }
 
+    async changePasswordWithVerification(
+        userId: number,
+        newPassword: string,
+        oldPassword: string,
+    ): Promise<void> {
+        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<TokenUserSchema> {
         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
@@ -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": [