diff --git a/frontend/src/component/segments/CreateSegment/CreateSegment.tsx b/frontend/src/component/segments/CreateSegment/CreateSegment.tsx index c72af26d5f..86007eab5a 100644 --- 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) => { > diff --git a/frontend/src/component/segments/EditSegment/EditSegment.tsx b/frontend/src/component/segments/EditSegment/EditSegment.tsx index 107991208c..912da57cee 100644 --- 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' > diff --git a/frontend/src/hooks/api/actions/useApi/useApi.ts b/frontend/src/hooks/api/actions/useApi/useApi.ts index ef7d5f60b1..1bc0627870 100644 --- 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; +type RequestFunction = ( + apiCaller: ApiCaller, + requestId: string, + loadingOn?: boolean, +) => Promise; + 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, + 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, @@ -187,6 +238,27 @@ const useAPI = ({ [handleResponses], ); + const makeLightRequest = useCallback( + async ( + apiCaller: () => Promise, + requestId: string, + loadingOn: boolean = true, + ): Promise => { + 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 index 7b17064c1b..8b5fb0094e 100644 --- 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 index 345ef66439..c79728ca10 100644 --- 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 index 53dfbe08a4..ad34d8c853 100644 --- 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 = { diff --git a/src/lib/middleware/rbac-middleware.ts b/src/lib/middleware/rbac-middleware.ts index 78adb57b38..ff6a2daf45 100644 --- a/src/lib/middleware/rbac-middleware.ts +++ b/src/lib/middleware/rbac-middleware.ts @@ -3,6 +3,8 @@ import { DELETE_FEATURE, ADMIN, UPDATE_FEATURE, + DELETE_SEGMENT, + UPDATE_PROJECT_SEGMENT, } from '../types/permissions'; import { IUnleashConfig } from '../types/option'; import { IUnleashStores } from '../types/stores'; @@ -32,7 +34,10 @@ export function findParam( const rbacMiddleware = ( config: Pick, - { featureToggleStore }: Pick, + { + featureToggleStore, + segmentStore, + }: Pick, accessService: PermissionChecker, ): any => { const logger = config.getLogger('/middleware/rbac-middleware.ts'); @@ -87,6 +92,17 @@ const rbacMiddleware = ( projectId = 'default'; } + // DELETE segment does not include information about the segment's project + // This is needed to check if the user has the right permissions on a project level + if ( + !projectId && + permissionsArray.includes(UPDATE_PROJECT_SEGMENT) + ) { + const { id } = params; + const { project } = await segmentStore.get(id); + projectId = project; + } + return accessService.hasPermission( user, permissionsArray,