[workspace] refactor: refactor the bulk_get handler in permission wra…

…pper when item has permission error (opensearch-project#8906) * return response with error Signed-off-by: Qxisylolo <[email protected]> * set Id Signed-off-by: Qxisylolo <[email protected]> * resolve tests Signed-off-by: Qxisylolo <[email protected]> * Changeset file for PR opensearch-project#8906 created/updated * typo Signed-off-by: Qxisylolo <[email protected]> * fix integration tests Signed-off-by: Qxisylolo <[email protected]> * add try catch Signed-off-by: Qxisylolo <[email protected]> * resolve conflicts Signed-off-by: Qxisylolo <[email protected]> * delete data source permission Signed-off-by: Qxisylolo <[email protected]> * add try catch Signed-off-by: Qxisylolo <[email protected]> --------- Signed-off-by: Qxisylolo <[email protected]> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>
AMoo-Miki · Jan 10, 2025 · 57daaf2 · 57daaf2
1 parent 37f1536
commit 57daaf2
Show file tree

Hide file tree

Showing 4 changed files with 81 additions and 46 deletions.
diff --git a/changelogs/fragments/8906.yml b/changelogs/fragments/8906.yml
@@ -0,0 +1,2 @@
+feat:
+- Refactor the bulk_get handler in permission wrapper when item has permission error ([#8906](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/8906))
diff --git a/...ace/server/saved_objects/integration_tests/workspace_saved_objects_client_wrapper.test.ts b/...ace/server/saved_objects/integration_tests/workspace_saved_objects_client_wrapper.test.ts
@@ -200,27 +200,24 @@ describe('WorkspaceSavedObjectsClientWrapper', () => {
 
   describe('bulkGet', () => {
     it('should throw forbidden error when user not permitted', async () => {
-      let error;
-      try {
-        await notPermittedSavedObjectedClient.bulkGet([
-          { type: 'dashboard', id: 'inner-workspace-dashboard-1' },
-        ]);
-      } catch (e) {
-        error = e;
-      }
-      expect(error).not.toBeUndefined();
-      expect(SavedObjectsErrorHelpers.isForbiddenError(error)).toBe(true);
+      const result = await notPermittedSavedObjectedClient.bulkGet([
+        { type: 'dashboard', id: 'acl-controlled-dashboard-2' },
+      ]);
 
-      error = undefined;
-      try {
-        await notPermittedSavedObjectedClient.bulkGet([
-          { type: 'dashboard', id: 'acl-controlled-dashboard-2' },
-        ]);
-      } catch (e) {
-        error = e;
-      }
-      expect(error).not.toBeUndefined();
-      expect(SavedObjectsErrorHelpers.isForbiddenError(error)).toBe(true);
+      expect(result.saved_objects).toEqual([
+        {
+          ...result.saved_objects[0],
+          id: 'acl-controlled-dashboard-2',
+          type: 'dashboard',
+          attributes: {},
+          error: {
+            error: 'Forbidden',
+            statusCode: 403,
+            message: 'Invalid saved objects permission',
+          },
+          workspaces: [],
+        },
+      ]);
     });
 
     it('should return consistent dashboard when user permitted', async () => {

diff --git a/src/plugins/workspace/server/saved_objects/workspace_saved_objects_client_wrapper.test.ts b/src/plugins/workspace/server/saved_objects/workspace_saved_objects_client_wrapper.test.ts
@@ -660,12 +660,11 @@ describe('WorkspaceSavedObjectsClientWrapper', () => {
           permissionControlMock,
           requestMock,
         } = generateWorkspaceSavedObjectsClientWrapper();
-        let errorCatched;
-        try {
-          await wrapper.bulkGet([{ type: 'dashboard', id: 'not-permitted-dashboard' }]);
-        } catch (e) {
-          errorCatched = e;
-        }
+
+        const result = await wrapper.bulkGet([
+          { type: 'dashboard', id: 'not-permitted-dashboard' },
+        ]);
+
         expect(permissionControlMock.validate).toHaveBeenCalledWith(
           requestMock,
           {
@@ -674,7 +673,22 @@ describe('WorkspaceSavedObjectsClientWrapper', () => {
           },
           ['library_read', 'library_write']
         );
-        expect(errorCatched?.message).toEqual('Invalid saved objects permission');
+
+        expect(result.saved_objects).toEqual([
+          {
+            id: 'not-permitted-dashboard',
+            type: 'dashboard',
+            attributes: {},
+            error: {
+              error: 'Forbidden',
+              message: 'Invalid saved objects permission',
+              statusCode: 403,
+            },
+            workspaces: [],
+            permissions: {},
+            references: [],
+          },
+        ]);
       });
       it('should call permission validateSavedObjectsACL with object', async () => {
         const { wrapper, permissionControlMock } = generateWorkspaceSavedObjectsClientWrapper();

diff --git a/src/plugins/workspace/server/saved_objects/workspace_saved_objects_client_wrapper.ts b/src/plugins/workspace/server/saved_objects/workspace_saved_objects_client_wrapper.ts
@@ -450,26 +450,48 @@ export class WorkspaceSavedObjectsClientWrapper {
         wrapperOptions.request,
         getWorkspacesFromSavedObjects(objectToBulkGet.saved_objects)
       );
-
-      for (const object of objectToBulkGet.saved_objects) {
-        if (
-          !(await this.validateWorkspacesAndSavedObjectsPermissions(
-            object,
-            wrapperOptions.request,
-            [WorkspacePermissionMode.LibraryRead, WorkspacePermissionMode.LibraryWrite],
-            [WorkspacePermissionMode.Write, WorkspacePermissionMode.Read],
-            false
-          ))
-        ) {
-          ACLAuditor?.increment(ACLAuditorStateKey.VALIDATE_FAILURE, 1);
-          throw generateSavedObjectsPermissionError();
-        }
-      }
-      ACLAuditor?.increment(
-        ACLAuditorStateKey.VALIDATE_SUCCESS,
-        objectToBulkGet.saved_objects.length
+      const processedObjects = await Promise.all(
+        objectToBulkGet.saved_objects.map(async (object) => {
+          try {
+            const hasPermission = await this.validateWorkspacesAndSavedObjectsPermissions(
+              object,
+              wrapperOptions.request,
+              [WorkspacePermissionMode.LibraryRead, WorkspacePermissionMode.LibraryWrite],
+              [WorkspacePermissionMode.Write, WorkspacePermissionMode.Read],
+              false
+            );
+            if (hasPermission) {
+              ACLAuditor?.increment(ACLAuditorStateKey.VALIDATE_SUCCESS, 1);
+              return object;
+            } else {
+              ACLAuditor?.increment(ACLAuditorStateKey.VALIDATE_FAILURE, 1);
+              return {
+                ...object,
+                workspaces: [],
+                attributes: {} as T,
+                error: {
+                  ...generateSavedObjectsPermissionError().output.payload,
+                  statusCode: 403,
+                },
+              };
+            }
+          } catch (error) {
+            ACLAuditor?.increment(ACLAuditorStateKey.VALIDATE_FAILURE, 1);
+            return {
+              ...object,
+              workspaces: [],
+              attributes: {} as T,
+              error: {
+                ...generateWorkspacePermissionError().output.payload,
+                statusCode: error.statusCode,
+                message: error.message,
+              },
+            };
+          }
+        })
       );
-      return objectToBulkGet;
+
+      return { saved_objects: processedObjects };
     };
 
     const findWithWorkspacePermissionControl = async <T = unknown>(
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1,2 @@
		feat:
		- Refactor the bulk_get handler in permission wrapper when item has permission error ([#8906](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/8906))