Mirrors/unleash.unleash
1
0
Fork 0
mirror of https://github.com/Unleash/unleash.git synced 2025-01-20 00:08:02 +01:00
Code Issues Projects Releases Wiki Activity

fix: take into account project segments permission (#5304)

Browse Source 
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.
This commit is contained in:
Nuno Góis 2023-11-09 09:37:47 +00:00 committed by GitHub
parent 8c2a052a68
commit 4d1f76e61b
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
7 changed files with 105 additions and 24 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

8
frontend/src/component/segments/CreateSegment/CreateSegment.tsx
View File

@ -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}
/>

8
frontend/src/component/segments/EditSegment/EditSegment.tsx
View File

@ -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}
>

4
frontend/src/hooks/api/actions/useApi/useApi.ts
View File

@ -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;

2
src/lib/error/permission-error.ts
View File

@ -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(', ')}`;

7
src/lib/features/segment/segment-controller.ts
View File

@ -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',

82
src/lib/middleware/rbac-middleware.test.ts
View File

@ -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 = {

18
src/lib/middleware/rbac-middleware.ts
View File

@ -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<IUnleashConfig, 'getLogger'>,
{ featureToggleStore }: Pick<IUnleashStores, 'featureToggleStore'>,
{
featureToggleStore,
segmentStore,
}: Pick<IUnleashStores, 'featureToggleStore' | 'segmentStore'>,
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,