Merge remote-tracking branch 'origin/main' into bundle-size-optimization

frontend/src/component/user/PasswordAuth.tsx

@@ -135,7 +135,7 @@ const PasswordAuth: VFC<IPasswordAuthProps> = ({ authDetails, redirect }) => {
                                 value={password}
                                 error={Boolean(passwordError)}
                                 helperText={passwordError}
-                                autoComplete="current-password"
+                                autoComplete="off"
                                 data-testid={LOGIN_PASSWORD_ID}
                             />
                             <Button

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');
+});

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"

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 = () => {

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));
         })
     );
 };

src/lib/openapi/spec/password-schema.ts

@@ -9,6 +9,9 @@ export const passwordSchema = {
         password: {
             type: 'string',
         },
+        oldPassword: {
+            type: 'string',
+        },
         confirmPassword: {
             type: 'string',
         },

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();

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();

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,

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": [

website/docs/contributing/ADRs/ADRs.md

@@ -14,6 +14,7 @@ One such example is the decision to re-write Unleash to TypeScript.
 These ADRs describe decisions that concern the entire codebase. They apply to back-end code, front-end code, and code that doesn't neatly fit into either of those categories.
 
 * [Domain language](./overarching/domain-language.md)
+* [Separation of request and response schemas](./overarching/separation-request-response-schemas.md)
 
 ## Back-end ADRs
 

website/docs/contributing/ADRs/overarching/separation-request-response-schemas.md

@@ -0,0 +1,42 @@
+---
+title: "ADR: Separation of request and response schemas"
+---
+
+## Background
+
+During the updating of our OpenAPI documentation, we have encountered issues related to the scope and strictness of our schemas for requests and responses. Currently, we are reusing the same schema for both request and response, which has led to situations where the schemas are either too broad for a response or too strict for a request. This has caused difficulties in accurately defining the expected data structures for API interactions.
+
+## Decision
+
+After careful consideration and discussion, it has been decided to separate the request and response schemas to address the challenges we have encountered. By creating distinct schemas for requests and responses, we aim to improve the flexibility and precision of our API documentation.
+
+### Advantages
+
+Separating the schemas will allow us to establish more precise and constrained response types while enabling more forgiving request types. This approach will facilitate better alignment between the expected data structures and the actual data transmitted in API interactions.
+
+The separation of request and response schemas will provide the following benefits:
+
+1. **Enhanced clarity and correctness**: With dedicated schemas for requests and responses, we can define the precise structure and constraints for each interaction. This will help prevent situations where the schemas are overly permissive or restrictive, reducing ambiguity and ensuring that the code handling the requests and responses is more reliable and easier to understand. By separating the schemas, we can define specific and precise structures for requests and responses, minimizing the use of undefined values and improving the overall correctness of the codebase and API implementation. The client knows exactly what data to send in the requests, and the server knows what to expect in the responses, ensuring that both parties are aligned in terms of data structures and expectations.
+
+2. **Improved maintainability**: By avoiding the reuse of schemas between requests and responses, we can modify and update them independently. This decoupling of schemas will simplify maintenance efforts and minimize the risk of unintended side effects caused by changes in one context affecting the other.
+
+3. **Flexibility for future enhancements**: Separating request and response schemas lays the foundation for introducing additional validation or transformation logic specific to each type of interaction. This modularity will enable us to incorporate future enhancements, such as custom validation rules or middleware, with ease.
+
+### Concerns
+
+While this decision brings several benefits, we acknowledge the following concerns that may arise from the separation of request and response schemas:
+
+1. **Increased schema maintenance**: By having separate schemas for requests and responses, there will be a need to maintain and update two sets of schemas instead of a single shared schema. This could potentially increase the maintenance overhead and introduce the possibility of inconsistencies between the two schemas.
+
+2. **Data duplication and redundancy**: With the separation of schemas, there might be instances where certain data fields or structures are duplicated between the request and response schemas. This redundancy could lead to code duplication and increase the risk of inconsistencies if changes are not carefully synchronized between the two schemas.
+
+
+## Conclusion
+
+By implementing the separation of request and response schemas, we aim to improve the robustness and maintainability of our API documentation. This decision will empower developers to build more reliable integrations by providing clearer guidelines for both request and response data structures.
+
+Furthermore, this separation of schemas brings valuable benefits to our internal development process. It allows us to write more robust code and reduces the need for extensive manipulation of incoming requests to fit the correct shapes. By clearly defining the structure and constraints for each interaction, we minimize the likelihood of bugs and make the code significantly easier to reason about and work with.
+
+While a big bang migration replacing all schemas at once is not feasible, we will follow the Boy Scout Rule and aim to complete the migration to separated schemas by the release of version 6.0. This means that as developers make changes or additions to the code, they will incorporate the separation of schemas, gradually updating the existing codebase over time. This approach ensures a smooth transition to the separated schemas, allowing us to continually improve the code handling requests and responses, reducing reliance on undefined values and promoting clarity, correctness, and maintainability throughout the development process.
+
+Overall, this approach will facilitate better communication, reduce confusion, and enhance the overall developer experience when interacting with our APIs, while providing the aforementioned benefits of more robust code, correctness and improved maintainability.