Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: take into account project segments permission #5304

Merged
merged 3 commits into from
Nov 9, 2023
Merged

fix: take into account project segments permission #5304

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cbf217d
fix: take into account project segments permission
nunogois Nov 8, 2023
8519bd1
refactor: simplify permission check
nunogois Nov 8, 2023
c49bd05
refactor: simplify project assignment
nunogois Nov 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions frontend/src/component/segments/CreateSegment/CreateSegment.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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';
Expand Down Expand Up @@ -112,7 +115,8 @@ export const CreateSegment = ({ modal }: ICreateSegmentProps) => {
>
<CreateButton
name='segment'
permission={CREATE_SEGMENT}
permission={[CREATE_SEGMENT, UPDATE_PROJECT_SEGMENT]}
projectId={projectId}
Copy link
Member Author

@nunogois nunogois Nov 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These 2 lines fix the bug on the UI, where the "Create" button was shown as being disabled.

disabled={!hasValidConstraints || overSegmentValuesLimit}
data-testid={SEGMENT_CREATE_BTN_ID}
/>
Expand Down
8 changes: 6 additions & 2 deletions frontend/src/component/segments/EditSegment/EditSegment.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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';
Expand Down Expand Up @@ -135,7 +138,8 @@ export const EditSegment = ({ modal }: IEditSegmentProps) => {
mode='edit'
>
<UpdateButton
permission={UPDATE_SEGMENT}
permission={[UPDATE_SEGMENT, UPDATE_PROJECT_SEGMENT]}
projectId={projectId}
Copy link
Member Author

@nunogois nunogois Nov 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These 2 lines fix the bug on the UI, where the "Save" button was shown as being disabled.

disabled={!hasValidConstraints || overSegmentValuesLimit}
data-testid={SEGMENT_SAVE_BTN_ID}
>
Expand Down
4 changes: 2 additions & 2 deletions frontend/src/hooks/api/actions/useApi/useApi.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}`,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Boyscouting: Noticed the typo and fixed it.

);

return res;
Expand Down
2 changes: 1 addition & 1 deletion src/lib/error/permission-error.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Boyscouting: I think "one" instead of "all" is more honest and reflective of the real behavior.

.map((perm) => `"${perm}"`)
.join(', ')}`;

Expand Down
7 changes: 4 additions & 3 deletions src/lib/features/segment/segment-controller.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import {
IFlagResolver,
NONE,
UPDATE_FEATURE_STRATEGY,
UPDATE_PROJECT_SEGMENT,
UPDATE_SEGMENT,
serializeDates,
} from '../../types';
Expand Down Expand Up @@ -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({
Expand All @@ -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',
Expand Down Expand Up @@ -225,7 +226,7 @@ export class SegmentsController extends Controller {
method: 'post',
path: '',
handler: this.createSegment,
permission: CREATE_SEGMENT,
permission: [CREATE_SEGMENT, UPDATE_PROJECT_SEGMENT],
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes add the missing permission on the API level.

middleware: [
openApiService.validPath({
summary: 'Create a new segment',
Expand Down
82 changes: 69 additions & 13 deletions src/lib/middleware/rbac-middleware.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
});

Expand All @@ -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();

Expand All @@ -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 = {
Expand All @@ -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 = {
Expand Down Expand Up @@ -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 = {
Expand All @@ -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 = {};
Expand All @@ -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 = {
Expand Down Expand Up @@ -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 = {
Expand Down Expand Up @@ -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 = {
Expand Down Expand Up @@ -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 = {
Expand Down Expand Up @@ -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 }),
Expand All @@ -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 }),
Expand All @@ -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 }),
Expand All @@ -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,
);
Copy link
Member Author

@nunogois nunogois Nov 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change requires injecting a segmentStore in the rbacMiddleware.


const cb = jest.fn();
const req: any = {
Expand Down
18 changes: 17 additions & 1 deletion src/lib/middleware/rbac-middleware.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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');
Expand Down Expand Up @@ -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;
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is what I mean by:

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.

return accessService.hasPermission(
user,
permissionsArray,
Expand Down
Loading