fix: take into account project segments permission #5304

nunogois · 2023-11-08T14:24:43Z

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.

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.

vercel · 2023-11-08T14:24:49Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
unleash-monorepo-frontend	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Nov 8, 2023 2:38pm

1 Ignored Deployment

Name	Status	Preview	Comments	Updated (UTC)
unleash-docs	⬜️ Ignored (Inspect)	Visit Preview		Nov 8, 2023 2:38pm

nunogois · 2023-11-08T14:25:52Z

frontend/src/component/segments/CreateSegment/CreateSegment.tsx

@@ -112,7 +115,8 @@ export const CreateSegment = ({ modal }: ICreateSegmentProps) => {
            >
                <CreateButton
                    name='segment'
-                    permission={CREATE_SEGMENT}
+                    permission={[CREATE_SEGMENT, UPDATE_PROJECT_SEGMENT]}
+                    projectId={projectId}


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

nunogois · 2023-11-08T14:25:58Z

frontend/src/component/segments/EditSegment/EditSegment.tsx

@@ -135,7 +138,8 @@ export const EditSegment = ({ modal }: IEditSegmentProps) => {
                mode='edit'
            >
                <UpdateButton
-                    permission={UPDATE_SEGMENT}
+                    permission={[UPDATE_SEGMENT, UPDATE_PROJECT_SEGMENT]}
+                    projectId={projectId}


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

nunogois · 2023-11-08T14:26:13Z

frontend/src/hooks/api/actions/useApi/useApi.ts

            );

            const res = await requestFunction(apiCaller, requestId, loadingOn);

            timeApiCallEnd(
                start,
-                requestId || `Uknown request happening on ${apiCaller}`,
+                requestId || `Unknown request happening on ${apiCaller}`,


Boyscouting: Noticed the typo and fixed it.

nunogois · 2023-11-08T14:26:37Z

src/lib/features/segment/segment-controller.ts

@@ -225,7 +226,7 @@ export class SegmentsController extends Controller {
            method: 'post',
            path: '',
            handler: this.createSegment,
-            permission: CREATE_SEGMENT,
+            permission: [CREATE_SEGMENT, UPDATE_PROJECT_SEGMENT],


These changes add the missing permission on the API level.

nunogois · 2023-11-08T14:29:12Z

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


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

nunogois · 2023-11-08T14:29:56Z

src/lib/middleware/rbac-middleware.test.ts

+        config,
+        { featureToggleStore, segmentStore },
+        accessService,
+    );


Change requires injecting a segmentStore in the rbacMiddleware.

nunogois · 2023-11-08T14:31:13Z

src/lib/middleware/rbac-middleware.ts

+                const { project } = await segmentStore.get(id);
+                projectId = project;
+            }
+


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.

sighphyre

This is the cleanest hack PR I've ever seen in my life xD

But I do agree, I think this is a hack. I'm okay to approve this because I do think it solves the problem but the cleaner design would be to split this into separate APIs.

Shall we get more eyes than just the 3 of us on this one?

chriswk

This feels like it looks good, the frontend changes at least was a nice catch. The logic in the segment controller as well seems sane to me.

This fixes an edge case not caught originally in #5304 - When creating a new segment on the global level: - There is no `projectId`, either in the params or body - The `UPDATE_PROJECT_SEGMENT` is still a part of the permissions checked on the endpoint - There is no `id` on the params This made it so that we would run `segmentStore.get(id)` with an undefined `id`, causing issues. The fix was simply checking for the presence of `params.id` before proceeding.

https://linear.app/unleash/issue/SR-184/ticket-1106-users-with-createedit-project-segment-dont-see-all-the #5304 did not take into account permissions further into the Segment form. This PR fixes the remaining permission checks to take into consideration the project-level permission: `UPDATE_PROJECT_SEGMENT`.

https://linear.app/unleash/issue/SR-184/ticket-1106-users-with-createedit-project-segment-dont-see-all-the #5304 did not take into account permissions further into the Segment form. This PR fixes the remaining permission checks to take into consideration the project-level permission: `UPDATE_PROJECT_SEGMENT`. (cherry picked from commit f8a9d7f)

fix: take into account project segments permission

cbf217d

nunogois requested review from Tymek and sighphyre November 8, 2023 14:24

nunogois commented Nov 8, 2023

View reviewed changes

vercel bot deployed to Preview – unleash-monorepo-frontend November 8, 2023 14:25 View deployment

github-actions bot assigned nunogois Nov 8, 2023

nunogois commented Nov 8, 2023

View reviewed changes

nunogois added 2 commits November 8, 2023 14:27

refactor: simplify permission check

8519bd1

refactor: simplify project assignment

c49bd05

nunogois commented Nov 8, 2023

View reviewed changes

vercel bot deployed to Preview – unleash-monorepo-frontend November 8, 2023 14:38 View deployment

sighphyre reviewed Nov 9, 2023

View reviewed changes

chriswk approved these changes Nov 9, 2023

View reviewed changes

nunogois merged commit 4d1f76e into main Nov 9, 2023
13 checks passed

nunogois deleted the fix-project-segments-permission branch November 9, 2023 09:37

sjaanus added a commit that referenced this pull request Nov 9, 2023

cherry-pick #5304

1e6c3d7

sjaanus added a commit that referenced this pull request Nov 9, 2023

cherry-pick #5304 (#5310)

3b23719

nunogois mentioned this pull request Nov 9, 2023

fix: segment project fetch when global #5311

Merged

nunogois mentioned this pull request Nov 16, 2023

fix: take into account project segments permission in form #5352

Merged

kwasniew mentioned this pull request Nov 16, 2023

fix: take into account project segments permission in form (#5352) #5354

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: take into account project segments permission #5304

fix: take into account project segments permission #5304

nunogois commented Nov 8, 2023 •

edited

Loading

vercel bot commented Nov 8, 2023 •

edited

Loading

nunogois Nov 8, 2023 •

edited

Loading

nunogois Nov 8, 2023 •

edited

Loading

nunogois Nov 8, 2023

nunogois Nov 8, 2023

nunogois Nov 8, 2023

nunogois Nov 8, 2023 •

edited

Loading

nunogois Nov 8, 2023

sighphyre left a comment

chriswk left a comment

fix: take into account project segments permission #5304

fix: take into account project segments permission #5304

Conversation

nunogois commented Nov 8, 2023 • edited Loading

vercel bot commented Nov 8, 2023 • edited Loading

nunogois Nov 8, 2023 • edited Loading

Choose a reason for hiding this comment

nunogois Nov 8, 2023 • edited Loading

Choose a reason for hiding this comment

nunogois Nov 8, 2023

Choose a reason for hiding this comment

nunogois Nov 8, 2023

Choose a reason for hiding this comment

nunogois Nov 8, 2023

Choose a reason for hiding this comment

nunogois Nov 8, 2023 • edited Loading

Choose a reason for hiding this comment

nunogois Nov 8, 2023

Choose a reason for hiding this comment

sighphyre left a comment

Choose a reason for hiding this comment

chriswk left a comment

Choose a reason for hiding this comment

nunogois commented Nov 8, 2023 •

edited

Loading

vercel bot commented Nov 8, 2023 •

edited

Loading

nunogois Nov 8, 2023 •

edited

Loading

nunogois Nov 8, 2023 •

edited

Loading

nunogois Nov 8, 2023 •

edited

Loading