[AN-149] Add ability to edit namespace permissions #5203

salonishah11 · 2024-12-16T15:06:36Z

Jira Ticket: https://broadworkbench.atlassian.net/browse/AN-149

Summary of changes:

What

Add ability to edit namespace permissions

Why

FC UI migration

Testing strategy

manual testing
unit tests

Workflows List

Edit modal

Error message is shown when user doesn't have edit permissions

LizBaldo · 2024-12-18T14:25:02Z

src/libs/ajax/methods/Methods.ts

@@ -47,6 +47,19 @@ export const Methods = (signal?: AbortSignal) => ({
    return res.json();
  },

+  getNamespacePermissions: async (namespace: string): Promise<MethodConfigACL> => {


Just for me, could you clarify what a namespace is, and what the new name will be if there will be a change as part of the PR Adam is working on?
It looks to me like a namespace is the same thing as a workflow / modal but I think I am missing a critical nuance here sorry

So it is similar to workspace namespace where one namespace can have many different workflows/methods inside a namespace. And similar to its workspace friend, a workflow/method namespace can have separate set of permissions (which are maintained in Agora and not Sam).

In the new world, its new name based on mockups is going to be collection.

Ohhh, TIL that workspaces had namespaces 🫨
But the collection idea makes sense to me, thanks!

LizBaldo

LGTM! My understanding from reading the code change is that before, a user could only edit permissions for a snapshot, not the namespace, and so you refactored the permission provider and modal to be able to handle both?

I only have some minor clarifying requests but none should be a blocker for merging, thank you!

LizBaldo · 2024-12-18T14:45:07Z

src/libs/ajax/methods/providers/PermissionsProvider.test.ts

+
+type MethodsNamespaceAjaxNeeds = Pick<MethodsAjaxContract, 'getNamespacePermissions' | 'setNamespacePermissions'>;
+
+const mockSnapshotAjax = () => {


Just curious as to why these two mocks have to be set up in a lightly different way? the mockNamespaceAjax seems a bit more straightforward?

The Ajax calls inside the mockSnapshotAjax are inside a nested method object and hence they were harder to mock out. I had issue previously (in another PR) where I had difficulty mocking similar nested calls and hence I just did here what I had done there.

LizBaldo · 2024-12-18T14:46:07Z

src/libs/ajax/methods/Methods.ts

@@ -47,6 +47,19 @@ export const Methods = (signal?: AbortSignal) => ({
    return res.json();
  },

+  getNamespacePermissions: async (namespace: string): Promise<MethodConfigACL> => {


Ohhh, TIL that workspaces had namespaces 🫨
But the collection idea makes sense to me, thanks!

LizBaldo · 2024-12-18T14:52:44Z

src/libs/ajax/methods/providers/PermissionsProvider.ts

+  getPermissions: async (
+    namespace: string,
+    _name?: string | undefined,
+    _snapshotId?: number | undefined,


The idea here is that a snapshotID would never be provided correct?

Yes for namespace permissions editing, both name and snapshotId are not required. But since the PermissionsProvider is the same for both namespace and snapshot I need to have it here in the function parameters. As a result I have marked both name and snapshotId with _ which indicated that they are "purposefully not being used"

LizBaldo · 2024-12-18T14:53:44Z

src/libs/ajax/methods/providers/PermissionsProvider.ts

+export interface PermissionsProvider {
+  getPermissions: (
+    namespace: string,
+    name?: string | undefined,


Is this the name of the namespace or the snapshot, or something else? It feels a bit ambiguous to me

This is the name of the method. Namespace parameter itself is the name of the namespace

salonishah11 · 2024-12-18T19:37:35Z

My understanding from reading the code change is that before, a user could only edit permissions for a snapshot, not the namespace, and so you refactored the permission provider and modal to be able to handle both?

@LizBaldo yes that is correct. Hence the PermissionsProvider has parameters that handle for both editing namespace as well as snapshot permissions.

src/libs/ajax/methods/providers/PermissionsProvider.ts

sam-schu · 2024-12-18T21:27:49Z

src/libs/ajax/methods/providers/PermissionsProvider.test.ts

+
+type MethodsNamespaceAjaxNeeds = Pick<MethodsAjaxContract, 'getNamespacePermissions' | 'setNamespacePermissions'>;
+
+const mockSnapshotAjax = () => {


It might be possible to type the mocks and remove the typecasts here as was done in #5187

Right. That is more for readability right? I don't have strong opinion either way. If you think its a good idea I can create a type for it.

Yes, for readability and to help make sure your mocks are set up correctly I believe. Miguel would know more about the benefits.

src/pages/methods/WorkflowList.tsx

sam-schu · 2024-12-18T21:40:58Z

src/pages/methods/WorkflowList.tsx

+      headerRenderer: () => <HeaderCell>Actions</HeaderCell>,
+      cellRenderer: ({ rowIndex }) => {
+        const { namespace, managers } = paginatedWorkflows[rowIndex];
+        const isNamespaceOwner = _.includes(getTerraUser().email?.toLowerCase(), _.map(_.toLower, managers));


This checks if the user is a method owner, not a namespace owner. I verified that if there is a method that I am not an owner of in a namespace that I am an owner of, Firecloud UI allows me to edit the namespace permissions from that method's row in the table, but this component does not.

Huh that is interesting. I will take a look. Thanks!

I see what happens in FC UI. It doesn't stop me from opening the permissions modal to edit namespace permissions even when I don't have permissions. And then in the modal when it realizes I don't have permission to edit it fails with below error

I have updated this behavior to be similar to FC UI. So now the Edit namespace permissions in the Actions menu will always be enabled. But if the user doesn't have edit permissions they will be show an error message in the modal that they don't have permissions to edit the settings.

For post-holidays:

We had a talk during standup and came to a conclusion that we might need to have a discussion on how we can make namespaces more useful. It seems that when User A gives User B 'Owner' permissions on a namespace, it essentially only allows User B to create new methods in that namespace. User B does not inherit the permission to read/edit any pre-existing methods that were created by User A in that namespace. Similarly any new methods created by User B is not visible to User A. Users can only read/edit methods of other users if they have permissions on a snapshot of that method.

Also, the 'Reader' role on namespace setting is unclear as User B doesn't even have read access to methods created by User A in that namespace.

src/workflows/methods/modals/PermissionsModal.tsx

sam-schu · 2024-12-18T22:08:27Z

src/pages/methods/WorkflowList.test.tsx

+    await user.click(screen.getByRole('button', { name: 'Edit namespace permissions' }));
+
+    // Assert
+    expect(


It wouldn't hurt to add something after this that checks that the workflow list actually uses the namespace permissions provider and not the snapshot one for its permissions modal.

I am not sure what would that test prove? Because in the code we actually set it to namespace vs snapshot one right? And the check to ensure right methods were called I think is already covered in PermissionsModal and PermissionsProvider test

Yeah, it would be more of a regression unit test. I suggested it because none of the WorkflowList tests make sure the right API call is ultimately made for this particular permissions modal, but we don't need it if you don't think it's necessary

sonarqubecloud · 2024-12-20T00:02:49Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
78.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

sam-schu

Approving given there will be a team decision on where to go with namespace permissions after the break, and the most important issue (not being able to edit namespace permissions from a method you don't own) has been addressed. Thanks for picking up where I left off with the Firecloud UI migration, we're so close!

salonishah11 added 4 commits December 6, 2024 17:35

wip; functional namespace permissions modal

d2e5f97

fix tests

ef08d90

added tests

5ae82a0

Merge branch 'dev' into sps_namespace_modal

5e96646

broadbot temporarily deployed to pr-0 December 16, 2024 15:22 Inactive

fix sonarcloud issue

36cab2a

broadbot temporarily deployed to pr-0 December 16, 2024 16:06 Inactive

jgainerdewar requested review from sam-schu and LizBaldo December 16, 2024 16:08

LizBaldo reviewed Dec 18, 2024

View reviewed changes

LizBaldo approved these changes Dec 18, 2024

View reviewed changes

sam-schu requested changes Dec 18, 2024

View reviewed changes

PR feedback

6c65045

broadbot temporarily deployed to pr-0 December 19, 2024 15:53 Inactive

salonishah11 added 2 commits December 19, 2024 17:39

refactor PermissionsProvider

b3a4798

PR feedback

6db9c3c

salonishah11 requested a review from sam-schu December 19, 2024 23:40

fix test

c1d5204

broadbot deployed to pr-0 December 20, 2024 00:03 View deployment

sam-schu approved these changes Dec 20, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[AN-149] Add ability to edit namespace permissions #5203

[AN-149] Add ability to edit namespace permissions #5203

salonishah11 commented Dec 16, 2024 •

edited

Loading

LizBaldo Dec 18, 2024

salonishah11 Dec 18, 2024

LizBaldo Dec 18, 2024

LizBaldo left a comment

LizBaldo Dec 18, 2024

salonishah11 Dec 18, 2024

LizBaldo Dec 18, 2024

LizBaldo Dec 18, 2024

salonishah11 Dec 18, 2024

LizBaldo Dec 18, 2024

salonishah11 Dec 18, 2024

salonishah11 commented Dec 18, 2024

sam-schu Dec 18, 2024

salonishah11 Dec 19, 2024

sam-schu Dec 20, 2024

sam-schu Dec 18, 2024

salonishah11 Dec 19, 2024 •

edited

Loading

salonishah11 Dec 19, 2024 •

edited

Loading

salonishah11 Dec 19, 2024

salonishah11 Dec 19, 2024 •

edited

Loading

sam-schu Dec 18, 2024

salonishah11 Dec 19, 2024

sam-schu Dec 20, 2024

sonarqubecloud bot commented Dec 20, 2024

sam-schu left a comment


		type MethodsNamespaceAjaxNeeds = Pick<MethodsAjaxContract, 'getNamespacePermissions' \| 'setNamespacePermissions'>;

		const mockSnapshotAjax = () => {

[AN-149] Add ability to edit namespace permissions #5203

Are you sure you want to change the base?

[AN-149] Add ability to edit namespace permissions #5203

Conversation

salonishah11 commented Dec 16, 2024 • edited Loading

Jira Ticket: https://broadworkbench.atlassian.net/browse/AN-149

Summary of changes:

What

Why

Testing strategy

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LizBaldo 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

salonishah11 commented Dec 18, 2024

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

salonishah11 Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

salonishah11 Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

salonishah11 Dec 19, 2024 • edited Loading

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

sonarqubecloud bot commented Dec 20, 2024

Quality Gate passed

sam-schu left a comment

Choose a reason for hiding this comment

salonishah11 commented Dec 16, 2024 •

edited

Loading

salonishah11 Dec 19, 2024 •

edited

Loading

salonishah11 Dec 19, 2024 •

edited

Loading

salonishah11 Dec 19, 2024 •

edited

Loading