Feat remove management permission mode in acl permission #195

wanglam · 2023-09-22T03:08:25Z

Description

1.Remove management permission mode
2.Update workspaces permission check use LibraryRead / LibraryWrite
3.Update ACL permission check use Read / Write
4.Change workspace permission form use below options
- Read: LibraryRead + Read
- Read+Write: LibraryWrite + Write
- Admin: LibraryWrite + Write

Issues Resolved

Screenshot

Testing the changes

Check List

…on mode usage Signed-off-by: Lin Wang <wonglam@amazon.com>

codecov-commenter · 2023-09-22T03:30:27Z

Codecov Report

Merging #195 (d8aa1d1) into workspace (a2abcf2) will increase coverage by 0.02%.
Report is 3 commits behind head on workspace.
The diff coverage is n/a.

@@              Coverage Diff              @@
##           workspace     #195      +/-   ##
=============================================
+ Coverage      66.09%   66.12%   +0.02%     
=============================================
  Files           3420     3420              
  Lines          65752    65753       +1     
  Branches       10589    10590       +1     
=============================================
+ Hits           43461    43480      +19     
+ Misses         19650    19633      -17     
+ Partials        2641     2640       -1

Flag	Coverage Δ
Linux_1	`30.42% <ø> (+<0.01%)`	⬆️
Linux_2	`55.12% <ø> (+0.06%)`	⬆️
Linux_3	`42.76% <ø> (+0.01%)`	⬆️
Linux_4	`34.52% <ø> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed	Coverage Δ
src/core/utils/constants.ts	`100.00% <ø> (ø)`

... and 6 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

ruanyl · 2023-09-22T08:37:33Z

...plugins/workspace/public/components/workspace_creator/workspace_permission_setting_panel.tsx

@@ -28,42 +28,55 @@ export type WorkspacePermissionSetting = (
  modes: Array<
    | WorkspacePermissionMode.LibraryRead
    | WorkspacePermissionMode.LibraryWrite
-    | WorkspacePermissionMode.Management


Should WorkspacePermissionSetting just be

export type WorkspacePermissionSetting = | { type: 'user'; userId: string; modes: Array<WorkspacePermissionMode> } | { type: 'group'; group: string; modes: Array<WorkspacePermissionMode> };

ruanyl · 2023-09-22T08:42:58Z

...plugins/workspace/public/components/workspace_creator/workspace_permission_setting_panel.tsx

@@ -227,63 +239,89 @@ export const WorkspacePermissionSettingPanel = ({
  onChange,
  firstRowDeletable,
 }: WorkspacePermissionSettingPanelProps) => {
-  const valueRef = useRef(value);
-  valueRef.current = value;
+  const transferredValue = useMemo(() => {


You mean transformed?

ruanyl · 2023-09-22T08:47:48Z

...plugins/workspace/public/components/workspace_creator/workspace_permission_setting_panel.tsx

+    for (let i = 0; i < value.length; i++) {
+      const valueItem = value[i];
+      if (
+        !valueItem.modes ||
+        !valueItem.type ||
+        (valueItem.type === 'user' && !valueItem.userId) ||
+        (valueItem.type === 'group' && !valueItem.group)
+      ) {
+        result.push(valueItem);
+        continue;
+      }
+      for (const key in optionIdToWorkspacePermissionModesMap) {
+        if (!Object.prototype.hasOwnProperty.call(optionIdToWorkspacePermissionModesMap, key)) {
+          continue;
+        }
+        const modesForCertainPermissionId = optionIdToWorkspacePermissionModesMap[key];
+        if (modesForCertainPermissionId.every((mode) => valueItem.modes?.includes(mode))) {
+          result.push({ ...valueItem, modes: modesForCertainPermissionId });
+        }
+      }
+    }


Could you help me to understand this piece of code? from L247-L267

The passed permission records were based user & group level. One user or group will contains multi permission modes. Since each permission mode in the UI was single select, we can't display multi permission option in one row. So we need to separate these permission modes into multi rows. Will add some annotation to clarify the logic.

ruanyl · 2023-09-22T08:58:16Z

src/plugins/workspace/public/workspace_client.ts

@@ -33,7 +33,8 @@ type WorkspaceRoutePermissionItem = {
  modes: Array<
    | WorkspacePermissionMode.LibraryRead
    | WorkspacePermissionMode.LibraryWrite
-    | WorkspacePermissionMode.Management
+    | WorkspacePermissionMode.Read


Should it be like this?

modes: WorkspacePermissionMode[]

ruanyl · 2023-09-22T09:07:10Z

src/plugins/workspace/server/saved_objects/workspace_saved_objects_client_wrapper.ts

@@ -286,8 +280,8 @@ export class WorkspaceSavedObjectsClientWrapper {
        !(await this.validateWorkspacesAndSavedObjectsPermissions(
          await wrapperOptions.client.get(type, options.id),


I have a question on this line, it's not related to this PR. What if options.id doesn't exist?

I think it will throw an error about document doesn't exists.

ruanyl · 2023-09-22T09:10:52Z

src/plugins/workspace/server/saved_objects/workspace_saved_objects_client_wrapper.ts

@@ -383,7 +357,12 @@ export class WorkspaceSavedObjectsClientWrapper {
            perPage: 999,
            ACLSearchParams: {
              principals,
-              permissionModes: workspaceInnerPermissionModes,
+              permissionModes: options.ACLSearchParams.permissionModes
+                ? intersection(options.ACLSearchParams.permissionModes, [


Would be nice to leave a comment on why intersection is needed

Signed-off-by: Lin Wang <wonglam@amazon.com>

* feat: remove management permission mode and clearify library permission mode usage Signed-off-by: Lin Wang <wonglam@amazon.com> * address PR comments and add annotations Signed-off-by: Lin Wang <wonglam@amazon.com> --------- Signed-off-by: Lin Wang <wonglam@amazon.com> (cherry picked from commit 29ccc20) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

feat: remove management permission mode and clearify library permissi…

eb5629d

…on mode usage Signed-off-by: Lin Wang <wonglam@amazon.com>

wanglam added the backport workspace-2.9 label Sep 22, 2023

github-actions bot added the seasoned-contributor label Sep 22, 2023

wanglam marked this pull request as ready for review September 22, 2023 03:38

ruanyl reviewed Sep 25, 2023

View reviewed changes

address PR comments and add annotations

d8aa1d1

Signed-off-by: Lin Wang <wonglam@amazon.com>

wanglam requested a review from ruanyl September 25, 2023 07:50

ruanyl approved these changes Sep 25, 2023

View reviewed changes

wanglam merged commit 29ccc20 into ruanyl:workspace Sep 25, 2023
23 checks passed

opensearch-workspace-development bot mentioned this pull request Sep 25, 2023

[Backport workspace-2.9] Feat remove management permission mode in acl permission #203

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat remove management permission mode in acl permission #195

Feat remove management permission mode in acl permission #195

wanglam commented Sep 22, 2023

codecov-commenter commented Sep 22, 2023 •

edited

Loading

ruanyl Sep 22, 2023

ruanyl Sep 22, 2023

ruanyl Sep 22, 2023

wanglam Sep 25, 2023

ruanyl Sep 22, 2023

ruanyl Sep 22, 2023

wanglam Sep 25, 2023

ruanyl Sep 22, 2023

		@@ -286,8 +280,8 @@ export class WorkspaceSavedObjectsClientWrapper {
		!(await this.validateWorkspacesAndSavedObjectsPermissions(
		await wrapperOptions.client.get(type, options.id),

Feat remove management permission mode in acl permission #195

Feat remove management permission mode in acl permission #195

Conversation

wanglam commented Sep 22, 2023

Description

Issues Resolved

Screenshot

Testing the changes

Check List

codecov-commenter commented Sep 22, 2023 • edited Loading

Codecov Report

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-commenter commented Sep 22, 2023 •

edited

Loading