[Workspace][Bug] Check if workspaces exists when creating saved objec…

…ts (#8739) (#8997) * Check if workspaces exists when creating saved objects * Changeset file for PR #8739 created/updated * optimize the code * fix test error * optimize the code * fix test errors * add integration tests --------- (cherry picked from commit 340326f) Signed-off-by: yubonluo <[email protected]> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>
opensearch-project · Dec 27, 2024 · 544cf40 · 544cf40
1 parent cdf0010
commit 544cf40
Show file tree

Hide file tree

Showing 5 changed files with 199 additions and 80 deletions.
diff --git a/changelogs/fragments/8739.yml b/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))
diff --git a/...ns/workspace/server/saved_objects/integration_tests/workspace_id_consumer_wrapper.test.ts b/...ns/workspace/server/saved_objects/integration_tests/workspace_id_consumer_wrapper.test.ts
@@ -150,10 +150,35 @@ describe('workspace_id_consumer integration test', () => {
         `/api/saved_objects/${config.type}/${packageInfo.version}`
       );
 
-      // workspaces arrtibutes should not be append
+      // workspaces attributes should not be append
       expect(!getConfigResult.body.workspaces).toEqual(true);
     });
 
+    it('should return error when create with a not existing workspace', async () => {
+      await clearFooAndBar();
+      const createResultWithNonExistRequestWorkspace = await osdTestServer.request
+        .post(root, `/w/not_exist_workspace_id/api/saved_objects/${dashboard.type}`)
+        .send({
+          attributes: dashboard.attributes,
+        })
+        .expect(400);
+
+      expect(createResultWithNonExistRequestWorkspace.body.message).toEqual(
+        'Exist invalid workspaces'
+      );
+
+      const createResultWithNonExistOptionsWorkspace = await osdTestServer.request
+        .post(root, `/api/saved_objects/${dashboard.type}`)
+        .send({
+          attributes: dashboard.attributes,
+          workspaces: ['not_exist_workspace_id'],
+        })
+        .expect(400);
+      expect(createResultWithNonExistOptionsWorkspace.body.message).toEqual(
+        'Exist invalid workspaces'
+      );
+    });
+
     it('bulk create', async () => {
       await clearFooAndBar();
       const createResultFoo = await osdTestServer.request
@@ -184,6 +209,37 @@ describe('workspace_id_consumer integration test', () => {
       );
     });
 
+    it('should return error when bulk create with a not existing workspace', async () => {
+      await clearFooAndBar();
+      const bulkCreateResultWithNonExistRequestWorkspace = await osdTestServer.request
+        .post(root, `/w/not_exist_workspace_id/api/saved_objects/_bulk_create`)
+        .send([
+          {
+            ...dashboard,
+            id: 'foo',
+          },
+        ])
+        .expect(400);
+
+      expect(bulkCreateResultWithNonExistRequestWorkspace.body.message).toEqual(
+        'Exist invalid workspaces'
+      );
+
+      const bulkCreateResultWithNonExistOptionsWorkspace = await osdTestServer.request
+        .post(root, `/api/saved_objects/_bulk_create?workspaces=not_exist_workspace_id`)
+        .send([
+          {
+            ...dashboard,
+            id: 'foo',
+          },
+        ])
+        .expect(400);
+
+      expect(bulkCreateResultWithNonExistOptionsWorkspace.body.message).toEqual(
+        'Exist invalid workspaces'
+      );
+    });
+
     it('checkConflicts when importing ndjson', async () => {
       await clearFooAndBar();
       const createResultFoo = await osdTestServer.request
@@ -288,7 +344,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('Exist invalid workspaces');
     });
 
     it('import within workspace', async () => {

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
@@ -250,7 +250,7 @@ describe('WorkspaceSavedObjectsClientWrapper', () => {
           perPage: 999,
           page: 1,
         })
-      ).rejects.toMatchInlineSnapshot(`[Error: Invalid workspaces]`);
+      ).rejects.toMatchInlineSnapshot(`[Error: Exist invalid workspaces]`);
     });
 
     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: Exist invalid workspaces]`);
     });
 
     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: Exist invalid workspaces]`);
     });
 
     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) {

diff --git a/src/plugins/workspace/server/saved_objects/workspace_id_consumer_wrapper.test.ts b/src/plugins/workspace/server/saved_objects/workspace_id_consumer_wrapper.test.ts
@@ -38,8 +38,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',
       });
@@ -68,13 +75,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: Exist invalid workspaces]`);
+      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',
@@ -88,6 +136,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: Exist invalid workspaces]`);
+      expect(mockedWorkspaceClient.get).toBeCalledTimes(1);
+      expect(mockedWorkspaceClient.list).toBeCalledTimes(0);
+    });
   });
 
   describe('checkConflict', () => {
@@ -174,7 +239,7 @@ describe('WorkspaceIdConsumerWrapper', () => {
           type: ['dashboard', 'visualization'],
           workspaces: ['foo', 'not-exist'],
         })
-      ).rejects.toMatchInlineSnapshot(`[Error: Invalid workspaces]`);
+      ).rejects.toMatchInlineSnapshot(`[Error: Exist invalid workspaces]`);
       expect(mockedWorkspaceClient.get).toBeCalledTimes(0);
       expect(mockedWorkspaceClient.list).toBeCalledTimes(1);
     });
Original file line number	Diff line number	Diff line change
		@@ -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))