From 4d1f76e61b5510aadb8fa21199907680738b6dc3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nuno=20G=C3=B3is?= Date: Thu, 9 Nov 2023 09:37:47 +0000 Subject: [PATCH] fix: take into account project segments permission (#5304) https://linear.app/unleash/issue/SR-164/ticket-1106-user-with-createedit-project-segment-is-not-able-to-edit-a Fixes a bug where the `UPDATE_PROJECT_SEGMENT` permission is not respected, both on the UI and on the API. The original intention was stated [here](https://github.com/Unleash/unleash/pull/3346#discussion_r1140434517). This was easy to fix on the UI, since we were simply missing the extra permission on the button permission checks. Unfortunately the API can be tricky. Our auth middleware tries to grab the `project` information from either the params or body object, but our `DELETE` method does not contain this information. There is no body and the endpoint looks like `/admin/segments/:id`, only including the segment id. This means that, in the rbac middleware when we check the permissions, we need to figure out if we're in such a scenario and fetch the project information from the DB, which feels a bit hacky, but it's something we're seemingly already doing for features, so at least it's somewhat consistent. Ideally what we could do is leave this API alone and create a separate one for project segments, with endpoints where we would have project as a param, like so: `http://localhost:4242/api/admin/projects/:projectId/segments/1`. This PR opts to go with the quick and hacky solution for now since this is an issue we want to fix quickly, but this is something that we should be aware of. I'm also unsure if we want to create a new API for project segments. If we decide that we want a different solution I don't mind either adapting this PR or creating a follow up. --- .../segments/CreateSegment/CreateSegment.tsx | 8 +- .../segments/EditSegment/EditSegment.tsx | 8 +- .../src/hooks/api/actions/useApi/useApi.ts | 4 +- src/lib/error/permission-error.ts | 2 +- .../features/segment/segment-controller.ts | 7 +- src/lib/middleware/rbac-middleware.test.ts | 82 ++++++++++++++++--- src/lib/middleware/rbac-middleware.ts | 18 +++- 7 files changed, 105 insertions(+), 24 deletions(-) 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 b8ff9c6628..1bc0627870 100644 --- a/frontend/src/hooks/api/actions/useApi/useApi.ts +++ b/frontend/src/hooks/api/actions/useApi/useApi.ts @@ -194,14 +194,14 @@ const useAPI = ({ loadingOn: boolean = true, ) => { const start = timeApiCallStart( - requestId || `Uknown request happening on ${apiCaller}`, + requestId || `Unknown request happening on ${apiCaller}`, ); const res = await requestFunction(apiCaller, requestId, loadingOn); timeApiCallEnd( start, - requestId || `Uknown request happening on ${apiCaller}`, + requestId || `Unknown request happening on ${apiCaller}`, ); return res; 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,