[workspace]feat: validate workspace when find objects (#8268) (#8431)

* feat: validate workspace when find objects * Changeset file for PR #8268 created/updated * fix: type error * feat: add unit test * feat: address some comments * feat: optimize performance --------- (cherry picked from commit b6eb1a0) Signed-off-by: SuZhou-Joe <[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 · Oct 2, 2024 · d42bb65 · d42bb65
1 parent c0711ac
commit d42bb65
Show file tree

Hide file tree

Showing 11 changed files with 154 additions and 44 deletions.
diff --git a/changelogs/fragments/8268.yml b/changelogs/fragments/8268.yml
@@ -0,0 +1,2 @@
+feat:
+- Validate if the workspaces param is valid or not when consume it in the wrapper. ([#8268](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/8268))
diff --git a/src/plugins/workspace/server/plugin.ts b/src/plugins/workspace/server/plugin.ts
@@ -200,7 +200,7 @@ export class WorkspacePlugin implements Plugin<WorkspacePluginSetup, WorkspacePl
     core.savedObjects.addClientWrapper(
       PRIORITY_FOR_WORKSPACE_ID_CONSUMER_WRAPPER,
       WORKSPACE_ID_CONSUMER_WRAPPER_ID,
-      new WorkspaceIdConsumerWrapper().wrapperFactory
+      new WorkspaceIdConsumerWrapper(this.client).wrapperFactory
     );
 
     const maxImportExportSize = core.savedObjects.getImportExportObjectLimit();

diff --git a/src/plugins/workspace/server/routes/duplicate.ts b/src/plugins/workspace/server/routes/duplicate.ts
@@ -59,7 +59,6 @@ export const registerDuplicateRoute = (
       const getTargetWorkspaceResult = await client.get(
         {
           request: req,
-          logger,
         },
         targetWorkspace
       );

diff --git a/src/plugins/workspace/server/routes/index.test.ts b/src/plugins/workspace/server/routes/index.test.ts
@@ -12,22 +12,26 @@ import { loggingSystemMock, dynamicConfigServiceMock } from '../../../../core/se
 import { workspaceClientMock } from '../workspace_client.mock';
 
 import { registerRoutes, WORKSPACES_API_BASE_URL } from './index';
+import { IWorkspaceClientImpl } from '../types';
 
 type SetupServerReturn = UnwrapPromise<ReturnType<typeof setupServer>>;
 const mockDynamicConfigService = dynamicConfigServiceMock.createInternalStartContract();
 
 describe(`Workspace routes`, () => {
   let server: SetupServerReturn['server'];
   let httpSetup: SetupServerReturn['httpSetup'];
+  let mockedWorkspaceClient: IWorkspaceClientImpl;
 
   beforeEach(async () => {
     ({ server, httpSetup } = await setupServer());
 
     const router = httpSetup.createRouter('');
 
+    mockedWorkspaceClient = workspaceClientMock.create();
+
     registerRoutes({
       router,
-      client: workspaceClientMock,
+      client: mockedWorkspaceClient,
       logger: loggingSystemMock.create().get(),
       maxImportExportSize: Number.MAX_SAFE_INTEGER,
       isPermissionControlEnabled: false,
@@ -51,7 +55,7 @@ describe(`Workspace routes`, () => {
       })
       .expect(200);
     expect(result.body).toEqual({ id: expect.any(String) });
-    expect(workspaceClientMock.create).toHaveBeenCalledWith(
+    expect(mockedWorkspaceClient.create).toHaveBeenCalledWith(
       expect.any(Object),
       expect.objectContaining({
         name: 'Observability',

diff --git a/src/plugins/workspace/server/routes/index.ts b/src/plugins/workspace/server/routes/index.ts
@@ -141,7 +141,6 @@ export function registerRoutes({
       const result = await client.list(
         {
           request: req,
-          logger,
         },
         req.body
       );
@@ -180,7 +179,6 @@ export function registerRoutes({
       const result = await client.get(
         {
           request: req,
-          logger,
         },
         id
       );
@@ -225,7 +223,6 @@ export function registerRoutes({
       const result = await client.create(
         {
           request: req,
-          logger,
         },
         createPayload
       );
@@ -252,7 +249,6 @@ export function registerRoutes({
       const result = await client.update(
         {
           request: req,
-          logger,
         },
         id,
         {
@@ -280,7 +276,6 @@ export function registerRoutes({
       const result = await client.delete(
         {
           request: req,
-          logger,
         },
         id
       );

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
@@ -277,6 +277,14 @@ describe('workspace_id_consumer integration test', () => {
       );
     });
 
+    it('should return error when find with a not existing workspace', async () => {
+      const findResult = await osdTestServer.request
+        .get(root, `/w/not_exist_workspace_id/api/saved_objects/_find?type=${dashboard.type}`)
+        .expect(400);
+
+      expect(findResult.body.message).toEqual('Invalid workspaces');
+    });
+
     it('import within workspace', async () => {
       await clearFooAndBar();
 

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
@@ -243,19 +243,14 @@ describe('WorkspaceSavedObjectsClientWrapper', () => {
 
   describe('find', () => {
     it('should return empty result if user not permitted', async () => {
-      const result = await notPermittedSavedObjectedClient.find({
-        type: 'dashboard',
-        workspaces: ['workspace-1'],
-        perPage: 999,
-        page: 1,
-      });
-
-      expect(result).toEqual({
-        saved_objects: [],
-        total: 0,
-        page: 1,
-        per_page: 999,
-      });
+      await expect(
+        notPermittedSavedObjectedClient.find({
+          type: 'dashboard',
+          workspaces: ['workspace-1'],
+          perPage: 999,
+          page: 1,
+        })
+      ).rejects.toMatchInlineSnapshot(`[Error: Invalid workspaces]`);
     });
 
     it('should return consistent inner workspace data when user permitted', async () => {
@@ -758,7 +753,7 @@ describe('WorkspaceSavedObjectsClientWrapper', () => {
         {
           id: deleteWorkspaceId,
           permissions: {
-            library_read: { users: ['foo'] },
+            read: { users: ['foo'] },
             library_write: { users: ['foo'] },
           },
         }

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
@@ -7,10 +7,12 @@ import { updateWorkspaceState } from '../../../../core/server/utils';
 import { SavedObject } from '../../../../core/public';
 import { httpServerMock, savedObjectsClientMock, coreMock } from '../../../../core/server/mocks';
 import { WorkspaceIdConsumerWrapper } from './workspace_id_consumer_wrapper';
+import { workspaceClientMock } from '../workspace_client.mock';
 
 describe('WorkspaceIdConsumerWrapper', () => {
   const requestHandlerContext = coreMock.createRequestHandlerContext();
-  const wrapperInstance = new WorkspaceIdConsumerWrapper();
+  const mockedWorkspaceClient = workspaceClientMock.create();
+  const wrapperInstance = new WorkspaceIdConsumerWrapper(mockedWorkspaceClient);
   const mockedClient = savedObjectsClientMock.create();
   const workspaceEnabledMockRequest = httpServerMock.createOpenSearchDashboardsRequest();
   updateWorkspaceState(workspaceEnabledMockRequest, {
@@ -103,6 +105,29 @@ describe('WorkspaceIdConsumerWrapper', () => {
   describe('find', () => {
     beforeEach(() => {
       mockedClient.find.mockClear();
+      mockedWorkspaceClient.get.mockImplementation((requestContext, id) => {
+        if (id === 'foo') {
+          return {
+            success: true,
+          };
+        }
+
+        return {
+          success: false,
+        };
+      });
+      mockedWorkspaceClient.list.mockResolvedValue({
+        success: true,
+        result: {
+          workspaces: [
+            {
+              id: 'foo',
+            },
+          ],
+        },
+      });
+      mockedWorkspaceClient.get.mockClear();
+      mockedWorkspaceClient.list.mockClear();
     });
 
     it(`Should add workspaces parameters when find`, async () => {
@@ -113,10 +138,48 @@ describe('WorkspaceIdConsumerWrapper', () => {
         type: 'dashboard',
         workspaces: ['foo'],
       });
+      expect(mockedWorkspaceClient.get).toBeCalledTimes(1);
+      expect(mockedWorkspaceClient.list).toBeCalledTimes(0);
     });
 
     it(`Should pass a empty workspace array`, async () => {
-      const workspaceIdConsumerWrapper = new WorkspaceIdConsumerWrapper();
+      const workspaceIdConsumerWrapper = new WorkspaceIdConsumerWrapper(mockedWorkspaceClient);
+      const mockRequest = httpServerMock.createOpenSearchDashboardsRequest();
+      updateWorkspaceState(mockRequest, {});
+      const mockedWrapperClient = workspaceIdConsumerWrapper.wrapperFactory({
+        client: mockedClient,
+        typeRegistry: requestHandlerContext.savedObjects.typeRegistry,
+        request: mockRequest,
+      });
+      await mockedWrapperClient.find({
+        type: ['dashboard', 'visualization'],
+      });
+      expect(mockedClient.find).toBeCalledWith({
+        type: ['dashboard', 'visualization'],
+      });
+    });
+
+    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,
+      });
+      expect(
+        mockedWrapperClient.find({
+          type: ['dashboard', 'visualization'],
+          workspaces: ['foo', 'not-exist'],
+        })
+      ).rejects.toMatchInlineSnapshot(`[Error: Invalid workspaces]`);
+      expect(mockedWorkspaceClient.get).toBeCalledTimes(0);
+      expect(mockedWorkspaceClient.list).toBeCalledTimes(1);
+    });
+
+    it(`Should not throw error when passing in '*'`, async () => {
+      const workspaceIdConsumerWrapper = new WorkspaceIdConsumerWrapper(mockedWorkspaceClient);
       const mockRequest = httpServerMock.createOpenSearchDashboardsRequest();
       updateWorkspaceState(mockRequest, {});
       const mockedWrapperClient = workspaceIdConsumerWrapper.wrapperFactory({
@@ -126,6 +189,7 @@ describe('WorkspaceIdConsumerWrapper', () => {
       });
       await mockedWrapperClient.find({
         type: ['dashboard', 'visualization'],
+        workspaces: ['*'],
       });
       expect(mockedClient.find).toBeCalledWith({
         type: ['dashboard', 'visualization'],

diff --git a/src/plugins/workspace/server/saved_objects/workspace_id_consumer_wrapper.ts b/src/plugins/workspace/server/saved_objects/workspace_id_consumer_wrapper.ts
@@ -2,6 +2,7 @@
  * Copyright OpenSearch Contributors
  * SPDX-License-Identifier: Apache-2.0
  */
+import { i18n } from '@osd/i18n';
 
 import { getWorkspaceState } from '../../../../core/server/utils';
 import {
@@ -12,8 +13,9 @@ import {
   SavedObjectsCheckConflictsObject,
   OpenSearchDashboardsRequest,
   SavedObjectsFindOptions,
-  SavedObject,
+  SavedObjectsErrorHelpers,
 } from '../../../../core/server';
+import { IWorkspaceClientImpl } from '../types';
 
 const UI_SETTINGS_SAVED_OBJECTS_TYPE = 'config';
 
@@ -74,15 +76,55 @@ export class WorkspaceIdConsumerWrapper {
           this.formatWorkspaceIdParams(wrapperOptions.request, options)
         ),
       delete: wrapperOptions.client.delete,
-      find: (options: SavedObjectsFindOptions) => {
-        return wrapperOptions.client.find(
-          // Based on https://github.com/opensearch-project/OpenSearch-Dashboards/blob/main/src/core/server/ui_settings/create_or_upgrade_saved_config/get_upgradeable_config.ts#L49
-          // we need to make sure the find call for upgrade config should be able to find all the global configs as it was before.
-          // It is a workaround for 2.17, should be optimized in the upcoming 2.18 release.
+      find: async (options: SavedObjectsFindOptions) => {
+        // Based on https://github.com/opensearch-project/OpenSearch-Dashboards/blob/main/src/core/server/ui_settings/create_or_upgrade_saved_config/get_upgradeable_config.ts#L49
+        // we need to make sure the find call for upgrade config should be able to find all the global configs as it was before.
+        // It is a workaround for 2.17, should be optimized in the upcoming 2.18 release.
+        const finalOptions =
           this.isConfigType(options.type as string) && options.sortField === 'buildNum'
             ? options
-            : this.formatWorkspaceIdParams(wrapperOptions.request, 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',
+                })
+              )
+            );
+          }
+        }
+        return wrapperOptions.client.find(finalOptions);
       },
       bulkGet: wrapperOptions.client.bulkGet,
       get: wrapperOptions.client.get,
@@ -94,5 +136,5 @@ export class WorkspaceIdConsumerWrapper {
     };
   };
 
-  constructor() {}
+  constructor(private readonly workspaceClient: IWorkspaceClientImpl) {}
 }
diff --git a/src/plugins/workspace/server/types.ts b/src/plugins/workspace/server/types.ts
@@ -32,7 +32,6 @@ export interface WorkspaceFindOptions {
 
 export interface IRequestDetail {
   request: OpenSearchDashboardsRequest;
-  logger: Logger;
 }
 
 export interface IWorkspaceClientImpl {

diff --git a/src/plugins/workspace/server/workspace_client.mock.ts b/src/plugins/workspace/server/workspace_client.mock.ts
@@ -4,13 +4,15 @@
  */
 
 export const workspaceClientMock = {
-  setup: jest.fn(),
-  setSavedObjects: jest.fn(),
-  setUiSettings: jest.fn(),
-  create: jest.fn().mockResolvedValue({ id: 'mock-workspace-id' }),
-  list: jest.fn(),
-  get: jest.fn(),
-  update: jest.fn(),
-  delete: jest.fn(),
-  destroy: jest.fn(),
+  create: () => ({
+    setup: jest.fn(),
+    setSavedObjects: jest.fn(),
+    setUiSettings: jest.fn(),
+    create: jest.fn().mockResolvedValue({ id: 'mock-workspace-id' }),
+    list: jest.fn(),
+    get: jest.fn(),
+    update: jest.fn(),
+    delete: jest.fn(),
+    destroy: jest.fn(),
+  }),
 };