Feat disable duplicate user or group permission (#242)

* feat: throw error when create or update with invalid permission modes Signed-off-by: Lin Wang <[email protected]> * feat: show error when duplicate permissions for spefic user or group Signed-off-by: Lin Wang <[email protected]> * feat: mark permissions optional Signed-off-by: Lin Wang <[email protected]> * feat: change to WorkpsacePermissionItems for workspace client interfaces Signed-off-by: Lin Wang <[email protected]> --------- Signed-off-by: Lin Wang <[email protected]>
ruanyl · Oct 26, 2023 · 1d679f8 · 1d679f8
1 parent 1ac7ec4
commit 1d679f8
Show file tree

Hide file tree

Showing 8 changed files with 170 additions and 109 deletions.
diff --git a/src/plugins/workspace/public/components/workspace_creator/workspace_creator.test.tsx b/src/plugins/workspace/public/components/workspace_creator/workspace_creator.test.tsx
@@ -169,7 +169,8 @@ describe('WorkspaceCreator', () => {
         color: '#000000',
         description: 'test workspace description',
         defaultVISTheme: 'categorical',
-      })
+      }),
+      expect.any(Array)
     );
     await waitFor(() => {
       expect(notificationToastsAddSuccess).toHaveBeenCalled();
@@ -190,7 +191,8 @@ describe('WorkspaceCreator', () => {
       expect.objectContaining({
         name: 'test workspace name',
         features: expect.arrayContaining(['app1', 'app2', 'app3']),
-      })
+      }),
+      expect.any(Array)
     );
     await waitFor(() => {
       expect(notificationToastsAddSuccess).toHaveBeenCalled();
@@ -216,10 +218,8 @@ describe('WorkspaceCreator', () => {
     expect(workspaceClientCreate).toHaveBeenCalledWith(
       expect.objectContaining({
         name: 'test workspace name',
-        permissions: expect.arrayContaining([
-          expect.objectContaining({ type: 'user', userId: 'test user id' }),
-        ]),
-      })
+      }),
+      expect.arrayContaining([expect.objectContaining({ type: 'user', userId: 'test user id' })])
     );
     await waitFor(() => {
       expect(notificationToastsAddSuccess).toHaveBeenCalled();

diff --git a/src/plugins/workspace/public/components/workspace_creator/workspace_creator.tsx b/src/plugins/workspace/public/components/workspace_creator/workspace_creator.tsx
@@ -22,7 +22,8 @@ export const WorkspaceCreator = () => {
     async (data: WorkspaceFormSubmitData) => {
       let result;
       try {
-        result = await workspaceClient.create(data);
+        const { permissions, ...attributes } = data;
+        result = await workspaceClient.create(attributes, permissions);
       } catch (error) {
         notifications?.toasts.addDanger({
           title: i18n.translate('workspace.create.failed', {

diff --git a/src/plugins/workspace/public/components/workspace_creator/workspace_form.tsx b/src/plugins/workspace/public/components/workspace_creator/workspace_form.tsx
@@ -130,6 +130,20 @@ const getNumberOfErrors = (formErrors: WorkspaceFormErrors) => {
   return numberOfErrors;
 };
 
+const isUserOrGroupPermissionSettingDuplicated = (
+  permissionSettings: Array<Partial<WorkspacePermissionSetting>>,
+  permissionSettingToCheck: WorkspacePermissionSetting
+) =>
+  permissionSettings.some(
+    (permissionSetting) =>
+      (permissionSettingToCheck.type === WorkspacePermissionItemType.User &&
+        permissionSetting.type === WorkspacePermissionItemType.User &&
+        permissionSettingToCheck.userId === permissionSetting.userId) ||
+      (permissionSettingToCheck.type === WorkspacePermissionItemType.Group &&
+        permissionSetting.type === WorkspacePermissionItemType.Group &&
+        permissionSettingToCheck.group === permissionSetting.group)
+  );
+
 const workspaceHtmlIdGenerator = htmlIdGenerator();
 
 const defaultVISThemeOptions = [{ value: 'categorical', text: 'Categorical' }];
@@ -377,6 +391,14 @@ export const WorkspaceForm = ({
       for (let i = 0; i < formData.permissions.length; i++) {
         const permission = formData.permissions[i];
         if (isValidWorkspacePermissionSetting(permission)) {
+          if (
+            isUserOrGroupPermissionSettingDuplicated(formData.permissions.slice(0, i), permission)
+          ) {
+            permissionErrors[i] = i18n.translate('workspace.form.permission.invalidate.group', {
+              defaultMessage: 'Duplicate permission setting',
+            });
+            continue;
+          }
           continue;
         }
         if (!permission.type) {

diff --git a/src/plugins/workspace/public/components/workspace_updater/workspace_updater.tsx b/src/plugins/workspace/public/components/workspace_updater/workspace_updater.tsx
@@ -72,7 +72,8 @@ export const WorkspaceUpdater = () => {
         return;
       }
       try {
-        result = await workspaceClient.update(currentWorkspace?.id, data);
+        const { permissions, ...attributes } = data;
+        result = await workspaceClient.update(currentWorkspace?.id, attributes, permissions);
       } catch (error) {
         notifications?.toasts.addDanger({
           title: i18n.translate('workspace.update.failed', {

diff --git a/src/plugins/workspace/public/workspace_client.ts b/src/plugins/workspace/public/workspace_client.ts
@@ -176,16 +176,16 @@ export class WorkspaceClient {
    * @returns
    */
   public async create(
-    attributes: Omit<WorkspaceAttribute, 'id'> & {
-      permissions: WorkspaceRoutePermissionItem[];
-    }
+    attributes: Omit<WorkspaceAttribute, 'id'>,
+    permissions?: WorkspaceRoutePermissionItem[]
   ): Promise<IResponse<WorkspaceAttribute>> {
     const path = this.getPath();
 
     const result = await this.safeFetch<WorkspaceAttribute>(path, {
       method: 'POST',
       body: JSON.stringify({
         attributes,
+        permissions,
       }),
     });
 
@@ -264,15 +264,13 @@ export class WorkspaceClient {
    */
   public async update(
     id: string,
-    attributes: Partial<
-      WorkspaceAttribute & {
-        permissions: WorkspaceRoutePermissionItem[];
-      }
-    >
+    attributes: Partial<WorkspaceAttribute>,
+    permissions?: WorkspaceRoutePermissionItem[]
   ): Promise<IResponse<boolean>> {
     const path = this.getPath(id);
     const body = {
       attributes,
+      permissions,
     };
 
     const result = await this.safeFetch(path, {

diff --git a/src/plugins/workspace/server/routes/index.ts b/src/plugins/workspace/server/routes/index.ts
@@ -6,14 +6,8 @@
 import { schema } from '@osd/config-schema';
 import { ensureRawRequest } from '../../../../core/server';
 
-import {
-  ACL,
-  Permissions,
-  CoreSetup,
-  Logger,
-  WorkspacePermissionMode,
-} from '../../../../core/server';
-import { IWorkspaceDBImpl, WorkspaceRoutePermissionItem } from '../types';
+import { CoreSetup, Logger, WorkspacePermissionMode } from '../../../../core/server';
+import { IWorkspaceDBImpl, WorkspacePermissionItem } from '../types';
 
 export const WORKSPACES_API_BASE_URL = '/api/workspaces';
 
@@ -22,8 +16,6 @@ const workspacePermissionMode = schema.oneOf([
   schema.literal(WorkspacePermissionMode.Write),
   schema.literal(WorkspacePermissionMode.LibraryRead),
   schema.literal(WorkspacePermissionMode.LibraryWrite),
-  schema.literal(WorkspacePermissionMode.Read),
-  schema.literal(WorkspacePermissionMode.Write),
 ]);
 
 const workspacePermission = schema.oneOf([
@@ -47,44 +39,8 @@ const workspaceAttributesSchema = schema.object({
   icon: schema.maybe(schema.string()),
   reserved: schema.maybe(schema.boolean()),
   defaultVISTheme: schema.maybe(schema.string()),
-  permissions: schema.maybe(
-    schema.oneOf([workspacePermission, schema.arrayOf(workspacePermission)])
-  ),
 });
 
-const convertToACL = (
-  workspacePermissions: WorkspaceRoutePermissionItem | WorkspaceRoutePermissionItem[]
-) => {
-  workspacePermissions = Array.isArray(workspacePermissions)
-    ? workspacePermissions
-    : [workspacePermissions];
-
-  const acl = new ACL();
-
-  workspacePermissions.forEach((permission) => {
-    switch (permission.type) {
-      case 'user':
-        acl.addPermission(permission.modes, { users: [permission.userId] });
-        return;
-      case 'group':
-        acl.addPermission(permission.modes, { groups: [permission.group] });
-        return;
-    }
-  });
-
-  return acl.getPermissions() || {};
-};
-
-const convertFromACL = (permissions: Permissions) => {
-  const acl = new ACL(permissions);
-
-  return acl.toFlatList().map(({ name, permissions: modes, type }) => ({
-    type: type === 'users' ? 'user' : 'group',
-    modes,
-    ...{ [type === 'users' ? 'userId' : 'group']: name },
-  }));
-};
-
 export function registerRoutes({
   client,
   logger,
@@ -123,18 +79,7 @@ export function registerRoutes({
         return res.ok({ body: result });
       }
       return res.ok({
-        body: {
-          ...result,
-          result: {
-            ...result.result,
-            workspaces: result.result.workspaces.map((workspace) => ({
-              ...workspace,
-              ...(workspace.permissions
-                ? { permissions: convertFromACL(workspace.permissions) }
-                : {}),
-            })),
-          },
-        },
+        body: result,
       });
     })
   );
@@ -162,15 +107,7 @@ export function registerRoutes({
       }
 
       return res.ok({
-        body: {
-          ...result,
-          result: {
-            ...result.result,
-            ...(result.result.permissions
-              ? { permissions: convertFromACL(result.result.permissions) }
-              : {}),
-          },
-        },
+        body: result,
       });
     })
   );
@@ -180,31 +117,29 @@ export function registerRoutes({
       validate: {
         body: schema.object({
           attributes: workspaceAttributesSchema,
+          permissions: schema.maybe(
+            schema.oneOf([workspacePermission, schema.arrayOf(workspacePermission)])
+          ),
         }),
       },
     },
     router.handleLegacyErrors(async (context, req, res) => {
-      const { attributes } = req.body;
+      const { attributes, permissions: permissionsInRequest } = req.body;
       const rawRequest = ensureRawRequest(req);
       const authInfo = rawRequest?.auth?.credentials?.authInfo as { user_name?: string } | null;
-      const { permissions: permissionsInAttributes, ...others } = attributes;
-      let permissions: WorkspaceRoutePermissionItem[] = [];
-      if (permissionsInAttributes) {
-        permissions = Array.isArray(permissionsInAttributes)
-          ? permissionsInAttributes
-          : [permissionsInAttributes];
+      let permissions: WorkspacePermissionItem[] = [];
+      if (permissionsInRequest) {
+        permissions = Array.isArray(permissionsInRequest)
+          ? permissionsInRequest
+          : [permissionsInRequest];
       }
 
+      // Assign workspace owner to current user
       if (!!authInfo?.user_name) {
         permissions.push({
           type: 'user',
           userId: authInfo.user_name,
-          modes: [WorkspacePermissionMode.LibraryWrite],
-        });
-        permissions.push({
-          type: 'user',
-          userId: authInfo.user_name,
-          modes: [WorkspacePermissionMode.Write],
+          modes: [WorkspacePermissionMode.LibraryWrite, WorkspacePermissionMode.Write],
         });
       }
 
@@ -215,8 +150,8 @@ export function registerRoutes({
           logger,
         },
         {
-          ...others,
-          ...(permissions.length ? { permissions: convertToACL(permissions) } : {}),
+          ...attributes,
+          ...(permissions.length ? { permissions } : {}),
         }
       );
       return res.ok({ body: result });
@@ -231,14 +166,16 @@ export function registerRoutes({
         }),
         body: schema.object({
           attributes: workspaceAttributesSchema,
+          permissions: schema.maybe(
+            schema.oneOf([workspacePermission, schema.arrayOf(workspacePermission)])
+          ),
         }),
       },
     },
     router.handleLegacyErrors(async (context, req, res) => {
       const { id } = req.params;
-      const { attributes } = req.body;
-      const { permissions, ...others } = attributes;
-      let finalPermissions: WorkspaceRoutePermissionItem[] = [];
+      const { attributes, permissions } = req.body;
+      let finalPermissions: WorkspacePermissionItem[] = [];
       if (permissions) {
         finalPermissions = Array.isArray(permissions) ? permissions : [permissions];
       }
@@ -251,8 +188,8 @@ export function registerRoutes({
         },
         id,
         {
-          ...others,
-          ...(finalPermissions.length ? { permissions: convertToACL(finalPermissions) } : {}),
+          ...attributes,
+          ...(finalPermissions.length ? { finalPermissions } : {}),
         }
       );
       return res.ok({ body: result });

diff --git a/src/plugins/workspace/server/types.ts b/src/plugins/workspace/server/types.ts
@@ -10,13 +10,12 @@ import {
   SavedObjectsFindResponse,
   CoreSetup,
   WorkspacePermissionMode,
-  Permissions,
   WorkspaceAttribute,
   SavedObjectsServiceStart,
 } from '../../../core/server';
 
 export interface WorkspaceAttributeWithPermission extends WorkspaceAttribute {
-  permissions?: Permissions;
+  permissions?: WorkspacePermissionItem[];
 }
 
 export interface WorkspaceFindOptions {
@@ -75,7 +74,7 @@ export type IResponse<T> =
       error?: string;
     };
 
-export type WorkspaceRoutePermissionItem = {
+export type WorkspacePermissionItem = {
   modes: Array<
     | WorkspacePermissionMode.LibraryRead
     | WorkspacePermissionMode.LibraryWrite