[Workspace][Bug] Check if workspaces exists when creating saved objects

changelogs/fragments/8739.yml

@@ -0,0 +1,2 @@
+fix:
+- [Workspace] [Bug] Check if workspaces exists when creating saved objects. ([#8739](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/8739))

src/plugins/workspace/server/saved_objects/integration_tests/workspace_id_consumer_wrapper.test.ts

@@ -282,7 +282,7 @@ describe('workspace_id_consumer integration test', () => {
         .get(root, `/w/not_exist_workspace_id/api/saved_objects/_find?type=${dashboard.type}`)
         .expect(400);
 
-      expect(findResult.body.message).toEqual('Invalid workspaces');
+      expect(findResult.body.message).toEqual('Invalid workspaces: not_exist_workspace_id');
     });
 
     it('import within workspace', async () => {

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

@@ -250,7 +250,7 @@ describe('WorkspaceSavedObjectsClientWrapper', () => {
           perPage: 999,
           page: 1,
         })
-      ).rejects.toMatchInlineSnapshot(`[Error: Invalid workspaces]`);
+      ).rejects.toMatchInlineSnapshot(`[Error: Invalid workspaces: workspace-1]`);
     });
 
     it('should return consistent inner workspace data when user permitted', async () => {
@@ -349,21 +349,16 @@ describe('WorkspaceSavedObjectsClientWrapper', () => {
   });
 
   describe('create', () => {
-    it('should throw forbidden error when workspace not permitted and create called', async () => {
-      let error;
-      try {
-        await notPermittedSavedObjectedClient.create(
+    it('should throw bad request error when workspace is invalid and create called', async () => {
+      await expect(
+        notPermittedSavedObjectedClient.create(
           'dashboard',
           {},
           {
             workspaces: ['workspace-1'],
           }
-        );
-      } catch (e) {
-        error = e;
-      }
-
-      expect(SavedObjectsErrorHelpers.isForbiddenError(error)).toBe(true);
+        )
+      ).rejects.toMatchInlineSnapshot(`[Error: Invalid workspaces: workspace-1]`);
     });
 
     it('should able to create saved objects into permitted workspaces after create called', async () => {
@@ -427,7 +422,7 @@ describe('WorkspaceSavedObjectsClientWrapper', () => {
       expect(createResult.error).toBeUndefined();
     });
 
-    it('should throw forbidden error when user create a workspce and is not OSD admin', async () => {
+    it('should throw forbidden error when user create a workspace and is not OSD admin', async () => {
       let error;
       try {
         await permittedSavedObjectedClient.create('workspace', {}, {});
@@ -468,17 +463,12 @@ describe('WorkspaceSavedObjectsClientWrapper', () => {
   });
 
   describe('bulkCreate', () => {
-    it('should throw forbidden error when workspace not permitted and bulkCreate called', async () => {
-      let error;
-      try {
-        await notPermittedSavedObjectedClient.bulkCreate([{ type: 'dashboard', attributes: {} }], {
+    it('should throw bad request error when workspace is invalid and bulkCreate called', async () => {
+      await expect(
+        notPermittedSavedObjectedClient.bulkCreate([{ type: 'dashboard', attributes: {} }], {
           workspaces: ['workspace-1'],
-        });
-      } catch (e) {
-        error = e;
-      }
-
-      expect(SavedObjectsErrorHelpers.isForbiddenError(error)).toBe(true);
+        })
+      ).rejects.toMatchInlineSnapshot(`[Error: Invalid workspaces: workspace-1]`);
     });
 
     it('should able to create saved objects into permitted workspaces after bulkCreate called', async () => {
@@ -506,7 +496,6 @@ describe('WorkspaceSavedObjectsClientWrapper', () => {
           ],
           {
             overwrite: true,
-            workspaces: ['workspace-1'],
           }
         );
       } catch (e) {

src/plugins/workspace/server/saved_objects/workspace_id_consumer_wrapper.test.ts

@@ -37,8 +37,15 @@ describe('WorkspaceIdConsumerWrapper', () => {
   describe('create', () => {
     beforeEach(() => {
       mockedClient.create.mockClear();
+      mockedWorkspaceClient.get.mockClear();
+      mockedWorkspaceClient.list.mockClear();
     });
     it(`Should add workspaces parameters when create`, async () => {
+      mockedWorkspaceClient.get.mockImplementationOnce((requestContext, id) => {
+        return {
+          success: true,
+        };
+      });
       await wrapperClient.create('dashboard', {
         name: 'foo',
       });
@@ -67,13 +74,54 @@ describe('WorkspaceIdConsumerWrapper', () => {
 
       expect(mockedClient.create.mock.calls[0][2]?.hasOwnProperty('workspaces')).toEqual(false);
     });
+
+    it(`Should throw error when passing in invalid workspaces`, async () => {
+      const workspaceIdConsumerWrapper = new WorkspaceIdConsumerWrapper(mockedWorkspaceClient);
+      const mockRequest = httpServerMock.createOpenSearchDashboardsRequest();
+      updateWorkspaceState(mockRequest, {});
+      const mockedWrapperClient = workspaceIdConsumerWrapper.wrapperFactory({
+        client: mockedClient,
+        typeRegistry: requestHandlerContext.savedObjects.typeRegistry,
+        request: mockRequest,
+      });
+
+      mockedWorkspaceClient.list.mockResolvedValueOnce({
+        success: true,
+        result: {
+          workspaces: [
+            {
+              id: 'foo',
+            },
+          ],
+        },
+      });
+
+      expect(
+        mockedWrapperClient.create(
+          'dashboard',
+          {
+            name: 'foo',
+          },
+          { workspaces: ['zoo', 'noo'] }
+        )
+      ).rejects.toMatchInlineSnapshot(`[Error: Invalid workspaces: zoo, noo]`);
+      expect(mockedWorkspaceClient.get).toBeCalledTimes(0);
+      expect(mockedWorkspaceClient.list).toBeCalledTimes(1);
+    });
   });
 
   describe('bulkCreate', () => {
     beforeEach(() => {
       mockedClient.bulkCreate.mockClear();
+      mockedWorkspaceClient.get.mockClear();
+      mockedWorkspaceClient.list.mockClear();
     });
     it(`Should add workspaces parameters when bulk create`, async () => {
+      mockedWorkspaceClient.get.mockImplementationOnce((requestContext, id) => {
+        return {
+          success: true,
+        };
+      });
       await wrapperClient.bulkCreate([
         getSavedObject({
           id: 'foo',
@@ -87,6 +135,23 @@ describe('WorkspaceIdConsumerWrapper', () => {
         }
       );
     });
+
+    it(`Should throw error when passing in invalid workspaces`, async () => {
+      mockedWorkspaceClient.get.mockImplementationOnce((requestContext, id) => {
+        return {
+          success: false,
+        };
+      });
+      expect(
+        wrapperClient.bulkCreate([
+          getSavedObject({
+            id: 'foo',
+          }),
+        ])
+      ).rejects.toMatchInlineSnapshot(`[Error: Invalid workspaces: foo]`);
+      expect(mockedWorkspaceClient.get).toBeCalledTimes(1);
+      expect(mockedWorkspaceClient.list).toBeCalledTimes(0);
+    });
   });
 
   describe('checkConflict', () => {
@@ -173,7 +238,7 @@ describe('WorkspaceIdConsumerWrapper', () => {
           type: ['dashboard', 'visualization'],
           workspaces: ['foo', 'not-exist'],
         })
-      ).rejects.toMatchInlineSnapshot(`[Error: Invalid workspaces]`);
+      ).rejects.toMatchInlineSnapshot(`[Error: Invalid workspaces: not-exist]`);
       expect(mockedWorkspaceClient.get).toBeCalledTimes(0);
       expect(mockedWorkspaceClient.list).toBeCalledTimes(1);
     });

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

@@ -14,6 +14,7 @@ import {
   OpenSearchDashboardsRequest,
   SavedObjectsFindOptions,
   SavedObjectsErrorHelpers,
+  SavedObjectsClientWrapperOptions,
 } from '../../../../core/server';
 import { IWorkspaceClientImpl } from '../types';
 
@@ -48,25 +49,71 @@ export class WorkspaceIdConsumerWrapper {
     return type === UI_SETTINGS_SAVED_OBJECTS_TYPE;
   }
 
+  private async checkWorkspacesExist(
+    finalOptions: SavedObjectsCreateOptions,
+    wrapperOptions: SavedObjectsClientWrapperOptions
+  ) {
+    if (finalOptions.workspaces?.length) {
+      let invalidWorkspaces: string[] = [];
+      // If only has one workspace, we should use get to optimize performance
+      if (finalOptions.workspaces.length === 1) {
+        const workspaceGet = await this.workspaceClient.get(
+          { request: wrapperOptions.request },
+          finalOptions.workspaces[0]
+        );
+        if (!workspaceGet.success) {
+          invalidWorkspaces = [finalOptions.workspaces[0]];
+        }
+      } else {
+        const workspaceList = await this.workspaceClient.list(
+          {
+            request: wrapperOptions.request,
+          },
+          {
+            perPage: 9999,
+          }
+        );
+        if (workspaceList.success) {
+          const workspaceIdsSet = new Set(
+            workspaceList.result.workspaces.map((workspace) => workspace.id)
+          );
+          invalidWorkspaces = finalOptions.workspaces.filter(
+            (targetWorkspace) => !workspaceIdsSet.has(targetWorkspace)
+          );
+        }
+      }
+
+      if (invalidWorkspaces.length > 0) {
+        throw SavedObjectsErrorHelpers.decorateBadRequestError(
+          new Error(
+            i18n.translate('workspace.id_consumer.invalid', {
+              defaultMessage: 'Invalid workspaces: {invalidWorkspaces}',
+              values: { invalidWorkspaces: invalidWorkspaces.join(', ') },
+            })
+          )
+        );
+      }
+    }
+  }
+
   public wrapperFactory: SavedObjectsClientWrapperFactory = (wrapperOptions) => {
     return {
       ...wrapperOptions.client,
-      create: <T>(type: string, attributes: T, options: SavedObjectsCreateOptions = {}) =>
-        wrapperOptions.client.create(
-          type,
-          attributes,
-          this.isConfigType(type)
-            ? options
-            : this.formatWorkspaceIdParams(wrapperOptions.request, options)
-        ),
-      bulkCreate: <T = unknown>(
+      create: async <T>(type: string, attributes: T, options: SavedObjectsCreateOptions = {}) => {
+        const finalOptions = this.isConfigType(type)
+          ? options
+          : this.formatWorkspaceIdParams(wrapperOptions.request, options);
+        await this.checkWorkspacesExist(finalOptions, wrapperOptions);
+        return wrapperOptions.client.create(type, attributes, finalOptions);
+      },
+      bulkCreate: async <T = unknown>(
         objects: Array<SavedObjectsBulkCreateObject<T>>,
         options: SavedObjectsCreateOptions = {}
-      ) =>
-        wrapperOptions.client.bulkCreate(
-          objects,
-          this.formatWorkspaceIdParams(wrapperOptions.request, options)
-        ),
+      ) => {
+        const finalOptions = this.formatWorkspaceIdParams(wrapperOptions.request, options);
+        await this.checkWorkspacesExist(finalOptions, wrapperOptions);
+        return wrapperOptions.client.bulkCreate(objects, finalOptions);
+      },
       checkConflicts: (
         objects: SavedObjectsCheckConflictsObject[] = [],
         options: SavedObjectsBaseOptions = {}
@@ -84,46 +131,7 @@ export class WorkspaceIdConsumerWrapper {
           this.isConfigType(options.type as string) && options.sortField === 'buildNum'
             ? options
             : this.formatWorkspaceIdParams(wrapperOptions.request, options);
-        if (finalOptions.workspaces?.length) {
-          let isAllTargetWorkspaceExisting = false;
-          // If only has one workspace, we should use get to optimize performance
-          if (finalOptions.workspaces.length === 1) {
-            const workspaceGet = await this.workspaceClient.get(
-              { request: wrapperOptions.request },
-              finalOptions.workspaces[0]
-            );
-            if (workspaceGet.success) {
-              isAllTargetWorkspaceExisting = true;
-            }
-          } else {
-            const workspaceList = await this.workspaceClient.list(
-              {
-                request: wrapperOptions.request,
-              },
-              {
-                perPage: 9999,
-              }
-            );
-            if (workspaceList.success) {
-              const workspaceIdsSet = new Set(
-                workspaceList.result.workspaces.map((workspace) => workspace.id)
-              );
-              isAllTargetWorkspaceExisting = finalOptions.workspaces.every((targetWorkspace) =>
-                workspaceIdsSet.has(targetWorkspace)
-              );
-            }
-          }
-
-          if (!isAllTargetWorkspaceExisting) {
-            throw SavedObjectsErrorHelpers.decorateBadRequestError(
-              new Error(
-                i18n.translate('workspace.id_consumer.invalid', {
-                  defaultMessage: 'Invalid workspaces',
-                })
-              )
-            );
-          }
-        }
+        await this.checkWorkspacesExist(finalOptions, wrapperOptions);
         return wrapperOptions.client.find(finalOptions);
       },
       bulkGet: wrapperOptions.client.bulkGet,