cherry-pick #5304 (#5310)

Unleash · Nov 9, 2023 · 3b23719 · 3b23719
1 parent d44e00f
commit 3b23719
Show file tree

Hide file tree

Showing 7 changed files with 184 additions and 23 deletions.
diff --git a/frontend/src/component/segments/CreateSegment/CreateSegment.tsx b/frontend/src/component/segments/CreateSegment/CreateSegment.tsx
@@ -1,7 +1,10 @@
 import React, { useContext } from 'react';
 import { CreateButton } from 'component/common/CreateButton/CreateButton';
 import FormTemplate from 'component/common/FormTemplate/FormTemplate';
-import { CREATE_SEGMENT } from 'component/providers/AccessProvider/permissions';
+import {
+    CREATE_SEGMENT,
+    UPDATE_PROJECT_SEGMENT,
+} from 'component/providers/AccessProvider/permissions';
 import { useSegmentsApi } from 'hooks/api/actions/useSegmentsApi/useSegmentsApi';
 import { useConstraintsValidation } from 'hooks/api/getters/useConstraintsValidation/useConstraintsValidation';
 import { useSegments } from 'hooks/api/getters/useSegments/useSegments';
@@ -112,7 +115,8 @@ export const CreateSegment = ({ modal }: ICreateSegmentProps) => {
             >
                 <CreateButton
                     name='segment'
-                    permission={CREATE_SEGMENT}
+                    permission={[CREATE_SEGMENT, UPDATE_PROJECT_SEGMENT]}
+                    projectId={projectId}
                     disabled={!hasValidConstraints || overSegmentValuesLimit}
                     data-testid={SEGMENT_CREATE_BTN_ID}
                 />

diff --git a/frontend/src/component/segments/EditSegment/EditSegment.tsx b/frontend/src/component/segments/EditSegment/EditSegment.tsx
@@ -1,5 +1,8 @@
 import FormTemplate from 'component/common/FormTemplate/FormTemplate';
-import { UPDATE_SEGMENT } from 'component/providers/AccessProvider/permissions';
+import {
+    UPDATE_PROJECT_SEGMENT,
+    UPDATE_SEGMENT,
+} from 'component/providers/AccessProvider/permissions';
 import { useSegmentsApi } from 'hooks/api/actions/useSegmentsApi/useSegmentsApi';
 import { useConstraintsValidation } from 'hooks/api/getters/useConstraintsValidation/useConstraintsValidation';
 import { useSegment } from 'hooks/api/getters/useSegment/useSegment';
@@ -135,7 +138,8 @@ export const EditSegment = ({ modal }: IEditSegmentProps) => {
                 mode='edit'
             >
                 <UpdateButton
-                    permission={UPDATE_SEGMENT}
+                    permission={[UPDATE_SEGMENT, UPDATE_PROJECT_SEGMENT]}
+                    projectId={projectId}
                     disabled={!hasValidConstraints || overSegmentValuesLimit}
                     data-testid={SEGMENT_SAVE_BTN_ID}
                 >

diff --git a/frontend/src/hooks/api/actions/useApi/useApi.ts b/frontend/src/hooks/api/actions/useApi/useApi.ts
@@ -24,6 +24,13 @@ type ApiErrorHandler = (
     requestId: string,
 ) => void;
 
+type ApiCaller = () => Promise<Response>;
+type RequestFunction = (
+    apiCaller: ApiCaller,
+    requestId: string,
+    loadingOn?: boolean,
+) => Promise<Response>;
+
 interface IUseAPI {
     handleBadRequest?: ApiErrorHandler;
     handleNotFound?: ApiErrorHandler;
@@ -33,6 +40,29 @@ interface IUseAPI {
     propagateErrors?: boolean;
 }
 
+const timeApiCallStart = (requestId: string) => {
+    // Store the start time in milliseconds
+    console.log(`Starting timing for request: ${requestId}`);
+    return Date.now();
+};
+
+const timeApiCallEnd = (startTime: number, requestId: string) => {
+    // Calculate the end time and subtract the start time
+    const endTime = Date.now();
+    const duration = endTime - startTime;
+    console.log(`Timing for request ${requestId}: ${duration} ms`);
+
+    if (duration > 500) {
+        console.error(
+            'API call took over 500ms. This may indicate a rendering performance problem in your React component.',
+            requestId,
+            duration,
+        );
+    }
+
+    return duration;
+};
+
 const useAPI = ({
     handleBadRequest,
     handleNotFound,
@@ -157,6 +187,27 @@ const useAPI = ({
         ],
     );
 
+    const requestWithTimer = (requestFunction: RequestFunction) => {
+        return async (
+            apiCaller: () => Promise<Response>,
+            requestId: string,
+            loadingOn: boolean = true,
+        ) => {
+            const start = timeApiCallStart(
+                requestId || `Unknown request happening on ${apiCaller}`,
+            );
+
+            const res = await requestFunction(apiCaller, requestId, loadingOn);
+
+            timeApiCallEnd(
+                start,
+                requestId || `Unknown request happening on ${apiCaller}`,
+            );
+
+            return res;
+        };
+    };
+
     const makeRequest = useCallback(
         async (
             apiCaller: () => Promise<Response>,
@@ -187,6 +238,27 @@ const useAPI = ({
         [handleResponses],
     );
 
+    const makeLightRequest = useCallback(
+        async (
+            apiCaller: () => Promise<Response>,
+            requestId: string,
+            loadingOn: boolean = true,
+        ): Promise<Response> => {
+            try {
+                const res = await apiCaller();
+
+                if (!res.ok) {
+                    throw new Error();
+                }
+
+                return res;
+            } catch (e) {
+                throw new Error('Could not make request | makeLightRequest');
+            }
+        },
+        [],
+    );
+
     const createRequest = useCallback(
         (path: string, options: any, requestId: string = '') => {
             const defaultOptions: RequestInit = {
@@ -207,9 +279,17 @@ const useAPI = ({
         [],
     );
 
+    const makeRequestWithTimer = requestWithTimer(makeRequest);
+    const makeLightRequestWithTimer = requestWithTimer(makeLightRequest);
+
+    const isDevelopment = process.env.NODE_ENV === 'development';
+
     return {
         loading,
-        makeRequest,
+        makeRequest: isDevelopment ? makeRequestWithTimer : makeRequest,
+        makeLightRequest: isDevelopment
+            ? makeLightRequestWithTimer
+            : makeLightRequest,
         createRequest,
         errors,
     };

diff --git a/src/lib/error/permission-error.ts b/src/lib/error/permission-error.ts
@@ -15,7 +15,7 @@ class PermissionError extends UnleashError {
         const permissionsMessage =
             permissions.length === 1
                 ? `the "${permissions[0]}" permission`
-                : `all of the following permissions: ${permissions
+                : `one of the following permissions: ${permissions
                       .map((perm) => `"${perm}"`)
                       .join(', ')}`;
 

diff --git a/src/lib/features/segment/segment-controller.ts b/src/lib/features/segment/segment-controller.ts
@@ -30,6 +30,7 @@ import {
     IFlagResolver,
     NONE,
     UPDATE_FEATURE_STRATEGY,
+    UPDATE_PROJECT_SEGMENT,
     UPDATE_SEGMENT,
     serializeDates,
 } from '../../types';
@@ -165,7 +166,7 @@ export class SegmentsController extends Controller {
             method: 'delete',
             path: '/:id',
             handler: this.removeSegment,
-            permission: DELETE_SEGMENT,
+            permission: [DELETE_SEGMENT, UPDATE_PROJECT_SEGMENT],
             acceptAnyContentType: true,
             middleware: [
                 openApiService.validPath({
@@ -186,7 +187,7 @@ export class SegmentsController extends Controller {
             method: 'put',
             path: '/:id',
             handler: this.updateSegment,
-            permission: UPDATE_SEGMENT,
+            permission: [UPDATE_SEGMENT, UPDATE_PROJECT_SEGMENT],
             middleware: [
                 openApiService.validPath({
                     summary: 'Update segment by id',
@@ -225,7 +226,7 @@ export class SegmentsController extends Controller {
             method: 'post',
             path: '',
             handler: this.createSegment,
-            permission: CREATE_SEGMENT,
+            permission: [CREATE_SEGMENT, UPDATE_PROJECT_SEGMENT],
             middleware: [
                 openApiService.validPath({
                     summary: 'Create a new segment',

diff --git a/src/lib/middleware/rbac-middleware.test.ts b/src/lib/middleware/rbac-middleware.test.ts
@@ -7,12 +7,16 @@ import ApiUser from '../types/api-user';
 import { IFeatureToggleStore } from '../features/feature-toggle/types/feature-toggle-store-type';
 import FakeFeatureToggleStore from '../features/feature-toggle/fakes/fake-feature-toggle-store';
 import { ApiTokenType } from '../types/models/api-token';
+import { ISegmentStore } from '../types';
+import FakeSegmentStore from '../../test/fixtures/fake-segment-store';
 
 let config: IUnleashConfig;
 let featureToggleStore: IFeatureToggleStore;
+let segmentStore: ISegmentStore;
 
 beforeEach(() => {
     featureToggleStore = new FakeFeatureToggleStore();
+    segmentStore = new FakeSegmentStore();
     config = createTestConfig();
 });
 
@@ -21,7 +25,11 @@ test('should add checkRbac to request', () => {
         hasPermission: jest.fn(),
     };
 
-    const func = rbacMiddleware(config, { featureToggleStore }, accessService);
+    const func = rbacMiddleware(
+        config,
+        { featureToggleStore, segmentStore },
+        accessService,
+    );
 
     const cb = jest.fn();
 
@@ -40,7 +48,11 @@ test('should give api-user ADMIN permission', async () => {
         hasPermission: jest.fn(),
     };
 
-    const func = rbacMiddleware(config, { featureToggleStore }, accessService);
+    const func = rbacMiddleware(
+        config,
+        { featureToggleStore, segmentStore },
+        accessService,
+    );
 
     const cb = jest.fn();
     const req: any = {
@@ -66,7 +78,11 @@ test('should not give api-user ADMIN permission', async () => {
         hasPermission: jest.fn(),
     };
 
-    const func = rbacMiddleware(config, { featureToggleStore }, accessService);
+    const func = rbacMiddleware(
+        config,
+        { featureToggleStore, segmentStore },
+        accessService,
+    );
 
     const cb = jest.fn();
     const req: any = {
@@ -94,7 +110,11 @@ test('should not allow user to miss userId', async () => {
         hasPermission: jest.fn(),
     };
 
-    const func = rbacMiddleware(config, { featureToggleStore }, accessService);
+    const func = rbacMiddleware(
+        config,
+        { featureToggleStore, segmentStore },
+        accessService,
+    );
 
     const cb = jest.fn();
     const req: any = {
@@ -116,7 +136,11 @@ test('should return false for missing user', async () => {
         hasPermission: jest.fn(),
     };
 
-    const func = rbacMiddleware(config, { featureToggleStore }, accessService);
+    const func = rbacMiddleware(
+        config,
+        { featureToggleStore, segmentStore },
+        accessService,
+    );
 
     const cb = jest.fn();
     const req: any = {};
@@ -134,7 +158,11 @@ test('should verify permission for root resource', async () => {
         hasPermission: jest.fn(),
     };
 
-    const func = rbacMiddleware(config, { featureToggleStore }, accessService);
+    const func = rbacMiddleware(
+        config,
+        { featureToggleStore, segmentStore },
+        accessService,
+    );
 
     const cb = jest.fn();
     const req: any = {
@@ -163,7 +191,11 @@ test('should lookup projectId from params', async () => {
         hasPermission: jest.fn(),
     };
 
-    const func = rbacMiddleware(config, { featureToggleStore }, accessService);
+    const func = rbacMiddleware(
+        config,
+        { featureToggleStore, segmentStore },
+        accessService,
+    );
 
     const cb = jest.fn();
     const req: any = {
@@ -198,7 +230,11 @@ test('should lookup projectId from feature toggle', async () => {
 
     featureToggleStore.getProjectId = jest.fn().mockReturnValue(projectId);
 
-    const func = rbacMiddleware(config, { featureToggleStore }, accessService);
+    const func = rbacMiddleware(
+        config,
+        { featureToggleStore, segmentStore },
+        accessService,
+    );
 
     const cb = jest.fn();
     const req: any = {
@@ -231,7 +267,11 @@ test('should lookup projectId from data', async () => {
         hasPermission: jest.fn(),
     };
 
-    const func = rbacMiddleware(config, { featureToggleStore }, accessService);
+    const func = rbacMiddleware(
+        config,
+        { featureToggleStore, segmentStore },
+        accessService,
+    );
 
     const cb = jest.fn();
     const req: any = {
@@ -266,7 +306,11 @@ test('Does not double check permission if not changing project when updating tog
     };
     featureToggleStore.getProjectId = jest.fn().mockReturnValue(oldProjectId);
 
-    const func = rbacMiddleware(config, { featureToggleStore }, accessService);
+    const func = rbacMiddleware(
+        config,
+        { featureToggleStore, segmentStore },
+        accessService,
+    );
     const cb = jest.fn();
     const req: any = {
         user: new User({ username: 'user', id: 1 }),
@@ -290,7 +334,11 @@ test('UPDATE_TAG_TYPE does not need projectId', async () => {
         hasPermission: jest.fn().mockReturnValue(true),
     };
 
-    const func = rbacMiddleware(config, { featureToggleStore }, accessService);
+    const func = rbacMiddleware(
+        config,
+        { featureToggleStore, segmentStore },
+        accessService,
+    );
     const cb = jest.fn();
     const req: any = {
         user: new User({ username: 'user', id: 1 }),
@@ -314,7 +362,11 @@ test('DELETE_TAG_TYPE does not need projectId', async () => {
         hasPermission: jest.fn().mockReturnValue(true),
     };
 
-    const func = rbacMiddleware(config, { featureToggleStore }, accessService);
+    const func = rbacMiddleware(
+        config,
+        { featureToggleStore, segmentStore },
+        accessService,
+    );
     const cb = jest.fn();
     const req: any = {
         user: new User({ username: 'user', id: 1 }),
@@ -340,7 +392,11 @@ test('should not expect featureName for UPDATE_FEATURE when projectId specified'
         hasPermission: jest.fn(),
     };
 
-    const func = rbacMiddleware(config, { featureToggleStore }, accessService);
+    const func = rbacMiddleware(
+        config,
+        { featureToggleStore, segmentStore },
+        accessService,
+    );
 
     const cb = jest.fn();
     const req: any = {