Mrc-6385 Manage Read Access for packet groups #189

absternator · 2025-04-03T19:24:55Z

This PR adds the ability to manage read permissions on the packet groups page (home page) as seen on figma. There is slight difference in the modal dialog, the pills being different color currently not possible with multi select we use, thus have decided to color code the drop down itself and add a literal "Role" | "User" to help decide. This can all be changed when/if we choose to overhaul the permission forms as per figma discussions, ticket here.

This is branched off the API for managing permissions, thus the endpoint itself may change slightly (shouldn't affect this review)

Below are the changes and assumptions made about how the logic is working:

Note: Permission logic can be seen in hasPermission.ts, should make things clearer than me explaining.

On the page, show the update icon only if the user has packet. manage permission
On modal, a user can only grant/remove packet.read scoped to packetGroup (eg. packet.read:packetGroup:groupA)
- To grant permission, user/role must currently not be able to read that packetGroup. If they have global permission then the user/role wont show up in select dropdown(as they already have permission to read).
- To remove permission, user/role must have the read permission scoped to the packet group. If they have global access they will not show up (as we are only allowed to remove scoped to the permission group)

Testing:
Sign in with admin and create users and roles. Play around adding permissions and removing permissions in the user manage page and then also in the home page modal with managing read access. Ensure the behaviour is as expected. (as in roles/users should be added/removed and show up as expected in dropdowns)

Below are images of changes:

…nd PacketGroupSummaryListItem

…e of packetGroupName

…ide/packit into mrc-6385-manage-packet-groups

…s dialogs

…roles/users

…ide/packit into mrc-6385-manage-packet-groups

…nForm tests

absternator · 2025-04-03T19:25:35Z

app/src/app/components/contents/home/PacketGroupSummaryListItem.tsx


  return (
-    <li key={packetGroup.latestId} className="p-4 flex flex-col border-b">


just put inside div to add button no changes

absternator · 2025-04-03T19:25:52Z

app/src/app/components/contents/home/UpdatePacketReadPermissionForm.tsx

+
+    try {
+      await fetcher({
+        url: `${appConfig.apiUrl()}/roles/${packetGroupName}/read-permissions`,


subject to change as per pauls review

absternator · 2025-04-03T19:26:40Z

app/src/lib/auth/hasPermission.ts

good file to understand permission logic

absternator · 2025-04-03T19:29:14Z

app/src/app/components/contents/home/utils/getRolesAndUsersCantReadGroup.ts

+};
+
+/**
+ * Checks if a user has direct read access to the given packet group


below are just helper funcs for top one

codecov · 2025-04-03T19:29:24Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.33%. Comparing base (351798b) to head (eefa283).
Report is 3 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #189      +/-   ##
==========================================
+ Coverage   97.15%   97.33%   +0.17%     
==========================================
  Files         150      155       +5     
  Lines        1476     1575      +99     
  Branches      425      462      +37     
==========================================
+ Hits         1434     1533      +99     
  Misses         41       41              
  Partials        1        1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

absternator · 2025-04-03T19:29:40Z

app/src/app/components/contents/home/utils/getRolesUsersWithReadGroupPermission.ts

+ * This function retrieves roles and users that have read permissions for a specific packet group.
+ * It does NOT include those with global read permissions, manage permissions, etc.
+ */
+export const getRolesUsersWithReadGroupPermission = (


is used in remove read permissions multi-select

absternator · 2025-04-03T19:29:56Z

app/src/app/components/contents/home/utils/getRolesAndUsersCantReadGroup.ts

+ * This function retrieves roles and users that cannot read a specific packet group.
+ * It includes those with global read permissions, manage permissions, etc.
+ */
+export const getRolesAndUsersCantReadGroup = (


is used in grant read permissions multi-select

…ide/packit into mrc-6385-manage-packet-groups

… body structure

…ide/packit into mrc-6385-manage-packet-groups

…group context

…ide/packit into mrc-6385-manage-packet-groups

… form for consistency

EmmaLRussell

Looks good! Works as expected.

I definitely think it would be worth putting in the the single select style dialogs sooner rather than later as it will simplify the UI and make all permission holders immediately visible.

Could you put a tooltip on the Update access icons?

Could add an e2e test! 😀

EmmaLRussell · 2025-04-08T09:08:19Z

app/src/app/components/contents/home/UpdatePacketReadPermissionForm.tsx

+          render={({ field }) => (
+            <FormItem>
+              <FormDescription className="text-xs mb-0.5">
+                Select roles or specific users to grant read access to the packet group. Roles or users with global read


Suggested change

Select roles or specific users to grant read access to the packet group. Roles or users with global read

Select roles or specific users to grant read access to this packet group. Roles or users with global read

EmmaLRussell · 2025-04-08T09:09:35Z

app/src/app/components/contents/home/UpdatePacketReadPermissionForm.tsx

+              <FormDescription className="text-xs mb-0.5">
+                Select roles or specific users to grant read access to the packet group. Roles or users with global read
+                access cannot be added here as they already have the required permissions. Similarly, roles or users
+                with global permissions cannot have their access removed.


Suggested change

with global permissions cannot have their access removed.

with global access cannot have their access removed here.

EmmaLRussell · 2025-04-08T09:33:52Z

app/src/app/components/contents/home/UpdatePacketReadPermissionForm.tsx

+        url: `${appConfig.apiUrl()}/packetGroups/${packetGroupName}/read-permission`,
+        body: {
+          roleNamesToAdd: values.roleNamesToAdd,
+          roleNamesToRemove: values.roleNamesToRemove
+        },
+        method: "PUT"
+      });


So this is just updating "roles" because users are roles too...?

I'm guessing when we change the dialogs to be single-select we'll probably want to change the API to just accept a single list of updates roles with permssions - if that's not already being done in this round.

yup beacuse we have username roles....

sounds good with API changes when we do single select

EmmaLRussell · 2025-04-08T09:48:04Z

app/src/app/components/contents/home/utils/getRolesUsersWithReadGroupPermission.ts

+  const rolesWithRead = roles.filter((role) =>
+    hasPacketReadPermissionForGroup(mapPermissionsToNames(role.rolePermissions), packetGroupName)
+  );
+
+  const usersWithRead = users.filter((user) =>
+    hasPacketReadPermissionForGroup(mapPermissionsToNames(user.specificPermissions), packetGroupName)
+  );


These don't actually exclude users or roles with global permissions as they may have been granted specific read permission as well as having global permissions. It's a bit messy that that is allowed at all, but it does seem misleading to include those in the list that you can remove access from here, when they will still have global permission even when specific permission is removed.

ohh yesss good catch updated...

EmmaLRussell · 2025-04-08T09:53:53Z

app/src/app/components/contents/home/utils/getRolesAndUsersCantReadGroup.ts

+
+  const usersCantRead = users.filter(
+    (user) =>
+      !userHasDirectReadPermission(user, packetGroupName) && !userHasReadPermissionViaRole(user, roles, packetGroupName)


So we don't include users as available to add permissions for if they already have permission by virtue of a role they hold? I feel like that maybe should go into the blurb in dialog too explaining about global permissions. Something like

"You also cannot add or remove access here for specific users who have access via a role.".. with maybe a link the the Manage access page ("Role membership should be managed here")

good idea ive added the first line... didn't add link to manage access because they may not have user.manage permission page to visit link...

EmmaLRussell · 2025-04-08T09:56:40Z

app/src/app/components/contents/home/utils/getRolesAndUsersCantReadGroup.ts

+ * This function retrieves roles and users that cannot read a specific packet group.
+ * It includes those with global read permissions, manage permissions, etc.
+ */
+export const getRolesAndUsersCantReadGroup = (


Suggested change

export const getRolesAndUsersCantReadGroup = (

export const getRolesAndUsersWithoutReadGroupPermission = (

With and Without should really be consistent with each other.

i intentionally didnt make it consistent because they are not really opposite.. used cantReadGroup here because it means exactly that they cant read group at all (could be due to global permissions).. if I used withoutReadGroup that would be different as they could still read with global permissions without specifically having the read Group permission

EmmaLRussell · 2025-04-08T09:56:57Z

app/src/app/components/contents/home/utils/getRolesUsersWithReadGroupPermission.ts

+ * This function retrieves roles and users that have read permissions for a specific packet group.
+ * It does NOT include those with global read permissions, manage permissions, etc.
+ */
+export const getRolesUsersWithReadGroupPermission = (


Suggested change

export const getRolesUsersWithReadGroupPermission = (

export const getRolesAndUsersWithReadGroupPermission = (

have updated to getRolesAndUsersWithOnlyReadGroupPermission

EmmaLRussell · 2025-04-08T10:02:11Z

app/src/lib/constructPermissionName.ts

+  if ([packetGroupName, tag].filter((x) => x !== undefined).length > 1) {
+    throw new Error("Only one of packetGroupName or tag can be provided");
+  }


Should also be an error if both tag and packetId are defined?

plietar · 2025-04-08T11:15:05Z

app/src/lib/auth/hasPermission.ts

@@ -1,5 +1,35 @@
-import { UserState } from "../../app/components/providers/types/UserTypes";
+import { buildScopedPermission } from "../constructPermissionName";


The frontend really should not have this much authorization logic in it. Doing a bit of authorization on the current user to decide which bits to turn on and off in the UI may be okay, but doing authorization on other users is pretty strange.

IMO the backend should have a GET /read-permission that tells you which roles have read access to the packet group, and then you'd use that to filter the list of roles that can be added/removed. The response to the GET could also tell you whether they have scoped access to the packet group (and therefore can be revoked) or if they have a more global access to it (eg. they have packet.read).

In #189 you wrote

allow reading of all roles in packet.manage permission (this will be needed by FE so they can decide what roles to add/remove)

In retrospect I think this was maybe not a good direction. Someone with packet.manage.foo should only be able to list all the roles and get the read-permission of foo, but they shouldn't be able to list all the permissions that all the roles have.

lets have a chat about this tommroow

…tility function

…clarity

absternator added 10 commits April 1, 2025 14:04

feat: add role management and permissions to PacketGroupSummaryList a…

605e180

…nd PacketGroupSummaryListItem

fix: update comment in UpdatePacketReadPermissionForm to clarify usag…

530bfbb

…e of packetGroupName

Merge branch 'mrc-6369-update-read-access' of https://github.com/mrc-…

dbc4170

…ide/packit into mrc-6385-manage-packet-groups

feat: add user management permissions and refactor packet group acces…

a9a591e

…s dialogs

get mostly working version ++ upadte haspermission logic

4fd08cc

feat: refactor packet read permission logic and update selectors for …

99380cb

…roles/users

Merge branch 'mrc-6369-update-read-access' of https://github.com/mrc-…

3084758

…ide/packit into mrc-6385-manage-packet-groups

refactor and clean up code

30ab941

add tests + fix up issues

87d844a

feat: add user with specific permissions to UpdatePacketReadPermissio…

02cf3cc

…nForm tests

absternator requested a review from EmmaLRussell April 3, 2025 19:24

absternator commented Apr 3, 2025

View reviewed changes

app/src/lib/auth/hasPermission.ts

Copy link

Contributor Author

absternator Apr 3, 2025

Choose a reason for hiding this comment

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

good file to understand permission logic

absternator commented Apr 3, 2025

View reviewed changes

absternator added 6 commits April 4, 2025 12:08

Merge branch 'mrc-6369-update-read-access' of https://github.com/mrc-…

9d3ccba

…ide/packit into mrc-6385-manage-packet-groups

refactor: update API endpoint for read permissions and adjust request…

6f5587f

… body structure

Merge branch 'mrc-6369-update-read-access' of https://github.com/mrc-…

52c103e

…ide/packit into mrc-6385-manage-packet-groups

refactor: update API endpoint for read permissions to include packet …

2f30f70

…group context

Merge branch 'mrc-6369-update-read-access' of https://github.com/mrc-…

b94de2a

…ide/packit into mrc-6385-manage-packet-groups

refactor: update API endpoint for packet read permissions to singular…

645c4f3

… form for consistency

EmmaLRussell reviewed Apr 8, 2025

View reviewed changes

plietar reviewed Apr 8, 2025

View reviewed changes

Base automatically changed from mrc-6369-update-read-access to main April 9, 2025 07:44

absternator added 3 commits April 9, 2025 11:07

refactor: update permission validation logic and replace deprecated u…

4178901

…tility function

fix PR comments

10f2f33

refactor: reorganize imports in UpdatePermissionDialog component for …

eefa283

…clarity

absternator requested a review from EmmaLRussell April 9, 2025 14:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mrc-6385 Manage Read Access for packet groups #189

Mrc-6385 Manage Read Access for packet groups #189

absternator commented Apr 3, 2025

absternator Apr 3, 2025

absternator Apr 3, 2025

absternator Apr 3, 2025

absternator Apr 3, 2025

codecov bot commented Apr 3, 2025 •

edited

Loading

absternator Apr 3, 2025 •

edited

Loading

absternator Apr 3, 2025

EmmaLRussell left a comment

EmmaLRussell Apr 8, 2025

EmmaLRussell Apr 8, 2025

EmmaLRussell Apr 8, 2025

absternator Apr 9, 2025

EmmaLRussell Apr 8, 2025

absternator Apr 9, 2025

EmmaLRussell Apr 8, 2025

absternator Apr 9, 2025

EmmaLRussell Apr 8, 2025

absternator Apr 9, 2025

EmmaLRussell Apr 8, 2025

absternator Apr 9, 2025

EmmaLRussell Apr 8, 2025

plietar Apr 8, 2025

absternator Apr 9, 2025


		return (
		<li key={packetGroup.latestId} className="p-4 flex flex-col border-b">

	Select roles or specific users to grant read access to the packet group. Roles or users with global read
	Select roles or specific users to grant read access to this packet group. Roles or users with global read

	with global permissions cannot have their access removed.
	with global access cannot have their access removed here.

	export const getRolesAndUsersCantReadGroup = (
	export const getRolesAndUsersWithoutReadGroupPermission = (

	export const getRolesUsersWithReadGroupPermission = (
	export const getRolesAndUsersWithReadGroupPermission = (

		@@ -1,5 +1,35 @@
		import { UserState } from "../../app/components/providers/types/UserTypes";
		import { buildScopedPermission } from "../constructPermissionName";

Mrc-6385 Manage Read Access for packet groups #189

Are you sure you want to change the base?

Mrc-6385 Manage Read Access for packet groups #189

Conversation

absternator commented Apr 3, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Apr 3, 2025 • edited Loading

Codecov Report

absternator Apr 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EmmaLRussell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Apr 3, 2025 •

edited

Loading

absternator Apr 3, 2025 •

edited

Loading