From d6a9aa5f2fbeedb72216201e345cd9c0d334a69d Mon Sep 17 00:00:00 2001 From: "opensearch-trigger-bot[bot]" <98922864+opensearch-trigger-bot[bot]@users.noreply.github.com> Date: Fri, 6 Sep 2024 14:14:18 +0800 Subject: [PATCH] [Manual][Backport 2.x] Backport 7884 and 7954 to 2.x (#7984) (#8036) * [Workspace]Validate features parameter in workspace create and update API (#7884) * Add validate for features field in workspace create and update API * Changeset file for PR #7884 created/updated * Changeset file for PR #7884 created/updated * Move router outside align with other plugins * Add ut and fix integration tests * Import use case id from default nav groups * Fix workspace routes UT * Share feature config generator between publicn and server * Fix osd server crashed due to import from public --------- * [Workspace]Fix dynamicConfigServiceMock import path in workspace routes UT (#7954) * Fix dynamicConfigServiceMock import path in workspace routes UT * Changeset file for PR #7954 created/updated --------- --------- (cherry picked from commit d3ce40f10c42eb51ea317f47e20f4e607212b2bc) Signed-off-by: Lin Wang Signed-off-by: github-actions[bot] Co-authored-by: github-actions[bot] Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com> --- changelogs/fragments/7884.yml | 2 + changelogs/fragments/7954.yml | 2 + src/core/server/index.ts | 2 +- src/plugins/workspace/common/constants.ts | 2 + src/plugins/workspace/common/utils.ts | 3 + .../workspace_creator/workspace_creator.tsx | 2 +- .../workspace_form/use_workspace_form.ts | 7 +- src/plugins/workspace/public/utils.test.ts | 3 +- src/plugins/workspace/public/utils.ts | 7 +- .../server/integration_tests/routes.test.ts | 1 + src/plugins/workspace/server/plugin.ts | 3 +- .../workspace/server/routes/index.test.ts | 127 ++++++++++++++++++ src/plugins/workspace/server/routes/index.ts | 36 ++++- ...apper_for_check_workspace_conflict.test.ts | 3 + .../workspace_id_consumer_wrapper.test.ts | 3 + .../workspace_ui_settings_wrapper.test.ts | 2 +- .../workspace/server/workspace_client.mock.ts | 16 +++ 17 files changed, 200 insertions(+), 21 deletions(-) create mode 100644 changelogs/fragments/7884.yml create mode 100644 changelogs/fragments/7954.yml create mode 100644 src/plugins/workspace/server/routes/index.test.ts create mode 100644 src/plugins/workspace/server/workspace_client.mock.ts diff --git a/changelogs/fragments/7884.yml b/changelogs/fragments/7884.yml new file mode 100644 index 000000000000..7d690efde4a6 --- /dev/null +++ b/changelogs/fragments/7884.yml @@ -0,0 +1,2 @@ +feat: +- [Workspace]Validate features parameter in workspace create and update API ([#7884](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/7884)) \ No newline at end of file diff --git a/changelogs/fragments/7954.yml b/changelogs/fragments/7954.yml new file mode 100644 index 000000000000..2f353d46242f --- /dev/null +++ b/changelogs/fragments/7954.yml @@ -0,0 +1,2 @@ +fix: +- [Workspace]dynamicConfigServiceMock not found in workspace routes UT ([#7954](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/7954)) \ No newline at end of file diff --git a/src/core/server/index.ts b/src/core/server/index.ts index 921228416f42..67c8c6645f9f 100644 --- a/src/core/server/index.ts +++ b/src/core/server/index.ts @@ -370,7 +370,7 @@ export { } from './metrics'; export { AppCategory, WorkspaceAttribute } from '../types'; -export { DEFAULT_APP_CATEGORIES, WORKSPACE_TYPE } from '../utils'; +export { DEFAULT_APP_CATEGORIES, WORKSPACE_TYPE, DEFAULT_NAV_GROUPS } from '../utils'; export { SavedObject, diff --git a/src/plugins/workspace/common/constants.ts b/src/plugins/workspace/common/constants.ts index d6009bed1482..9556fea42b89 100644 --- a/src/plugins/workspace/common/constants.ts +++ b/src/plugins/workspace/common/constants.ts @@ -147,3 +147,5 @@ export const CURRENT_USER_PLACEHOLDER = '%me%'; export const MAX_WORKSPACE_NAME_LENGTH = 40; export const MAX_WORKSPACE_DESCRIPTION_LENGTH = 200; + +export const USE_CASE_PREFIX = 'use-case-'; diff --git a/src/plugins/workspace/common/utils.ts b/src/plugins/workspace/common/utils.ts index 8cc67a44644e..081aa9c21dc4 100644 --- a/src/plugins/workspace/common/utils.ts +++ b/src/plugins/workspace/common/utils.ts @@ -2,7 +2,10 @@ * Copyright OpenSearch Contributors * SPDX-License-Identifier: Apache-2.0 */ +import { USE_CASE_PREFIX } from './constants'; // Reference https://github.com/opensearch-project/oui/blob/main/src/services/color/is_valid_hex.ts export const validateWorkspaceColor = (color?: string) => !!color && /(^#[0-9A-F]{6}$)|(^#[0-9A-F]{3}$)/i.test(color); + +export const getUseCaseFeatureConfig = (useCaseId: string) => `${USE_CASE_PREFIX}${useCaseId}`; diff --git a/src/plugins/workspace/public/components/workspace_creator/workspace_creator.tsx b/src/plugins/workspace/public/components/workspace_creator/workspace_creator.tsx index 3ba8b7850753..2d02654e5ed6 100644 --- a/src/plugins/workspace/public/components/workspace_creator/workspace_creator.tsx +++ b/src/plugins/workspace/public/components/workspace_creator/workspace_creator.tsx @@ -11,13 +11,13 @@ import { BehaviorSubject } from 'rxjs'; import { useOpenSearchDashboards } from '../../../../opensearch_dashboards_react/public'; import { WorkspaceFormSubmitData, WorkspaceOperationType } from '../workspace_form'; import { WORKSPACE_DETAIL_APP_ID } from '../../../common/constants'; +import { getUseCaseFeatureConfig } from '../../../common/utils'; import { formatUrlWithWorkspaceId } from '../../../../../core/public/utils'; import { WorkspaceClient } from '../../workspace_client'; import { convertPermissionSettingsToPermissions } from '../workspace_form'; import { DataSource } from '../../../common/types'; import { DataSourceManagementPluginSetup } from '../../../../../plugins/data_source_management/public'; import { WorkspaceUseCase } from '../../types'; -import { getUseCaseFeatureConfig } from '../../utils'; import { useFormAvailableUseCases } from '../workspace_form/use_form_available_use_cases'; import { NavigationPublicPluginStart } from '../../../../../plugins/navigation/public'; import { WorkspaceCreatorForm } from './workspace_creator_form'; diff --git a/src/plugins/workspace/public/components/workspace_form/use_workspace_form.ts b/src/plugins/workspace/public/components/workspace_form/use_workspace_form.ts index 55f5c5af1607..703255455ec4 100644 --- a/src/plugins/workspace/public/components/workspace_form/use_workspace_form.ts +++ b/src/plugins/workspace/public/components/workspace_form/use_workspace_form.ts @@ -7,12 +7,9 @@ import { useCallback, useState, FormEventHandler, useRef, useMemo } from 'react' import { htmlIdGenerator, EuiColorPickerProps } from '@elastic/eui'; import { useApplications } from '../../hooks'; -import { - getFirstUseCaseOfFeatureConfigs, - getUseCaseFeatureConfig, - isUseCaseFeatureConfig, -} from '../../utils'; +import { getFirstUseCaseOfFeatureConfigs, isUseCaseFeatureConfig } from '../../utils'; import { DataSource } from '../../../common/types'; +import { getUseCaseFeatureConfig } from '../../../common/utils'; import { WorkspaceFormProps, WorkspaceFormErrors, diff --git a/src/plugins/workspace/public/utils.test.ts b/src/plugins/workspace/public/utils.test.ts index dfca65fcaf98..d744218a1aba 100644 --- a/src/plugins/workspace/public/utils.test.ts +++ b/src/plugins/workspace/public/utils.test.ts @@ -13,13 +13,12 @@ import { getDataSourcesList, convertNavGroupToWorkspaceUseCase, isEqualWorkspaceUseCase, - USE_CASE_PREFIX, prependWorkspaceToBreadcrumbs, getIsOnlyAllowEssentialUseCase, } from './utils'; import { WorkspaceAvailability } from '../../../core/public'; import { coreMock } from '../../../core/public/mocks'; -import { WORKSPACE_DETAIL_APP_ID } from '../common/constants'; +import { WORKSPACE_DETAIL_APP_ID, USE_CASE_PREFIX } from '../common/constants'; import { SigV4ServiceName } from '../../../plugins/data_source/common/data_sources'; import { createMockedRegisteredUseCases } from './mocks'; diff --git a/src/plugins/workspace/public/utils.ts b/src/plugins/workspace/public/utils.ts index 01969d15159c..4d0ab311d7de 100644 --- a/src/plugins/workspace/public/utils.ts +++ b/src/plugins/workspace/public/utils.ts @@ -24,7 +24,8 @@ import { WorkspaceObject, WorkspaceAvailability, } from '../../../core/public'; -import { WORKSPACE_DETAIL_APP_ID } from '../common/constants'; +import { WORKSPACE_DETAIL_APP_ID, USE_CASE_PREFIX } from '../common/constants'; +import { getUseCaseFeatureConfig } from '../common/utils'; import { WorkspaceUseCase, WorkspaceUseCaseFeature } from './types'; import { formatUrlWithWorkspaceId } from '../../../core/public/utils'; import { SigV4ServiceName } from '../../../plugins/data_source/common/data_sources'; @@ -33,10 +34,6 @@ import { ESSENTIAL_OVERVIEW_PAGE_ID, } from '../../../plugins/content_management/public'; -export const USE_CASE_PREFIX = 'use-case-'; - -export const getUseCaseFeatureConfig = (useCaseId: string) => `${USE_CASE_PREFIX}${useCaseId}`; - export const isUseCaseFeatureConfig = (featureConfig: string) => featureConfig.startsWith(USE_CASE_PREFIX); diff --git a/src/plugins/workspace/server/integration_tests/routes.test.ts b/src/plugins/workspace/server/integration_tests/routes.test.ts index e3de40309e72..589a4ccbe326 100644 --- a/src/plugins/workspace/server/integration_tests/routes.test.ts +++ b/src/plugins/workspace/server/integration_tests/routes.test.ts @@ -16,6 +16,7 @@ const testWorkspace: WorkspaceAttribute = { id: 'fake_id', name: 'test_workspace', description: 'test_workspace_description', + features: ['use-case-all'], }; describe('workspace service api integration test', () => { diff --git a/src/plugins/workspace/server/plugin.ts b/src/plugins/workspace/server/plugin.ts index 21f90fbe0d91..ad45b1f7fa08 100644 --- a/src/plugins/workspace/server/plugin.ts +++ b/src/plugins/workspace/server/plugin.ts @@ -202,9 +202,10 @@ export class WorkspacePlugin implements Plugin>; +const mockDynamicConfigService = dynamicConfigServiceMock.createInternalStartContract(); + +describe(`Workspace routes`, () => { + let server: SetupServerReturn['server']; + let httpSetup: SetupServerReturn['httpSetup']; + + beforeEach(async () => { + ({ server, httpSetup } = await setupServer()); + + const router = httpSetup.createRouter(''); + + registerRoutes({ + router, + client: workspaceClientMock, + logger: loggingSystemMock.create().get(), + maxImportExportSize: Number.MAX_SAFE_INTEGER, + isPermissionControlEnabled: false, + }); + + await server.start({ dynamicConfigService: mockDynamicConfigService }); + }); + + afterEach(async () => { + await server.stop(); + }); + + it('creates a workspace successfully', async () => { + const result = await supertest(httpSetup.server.listener) + .post(WORKSPACES_API_BASE_URL) + .send({ + attributes: { + name: 'Observability', + features: ['use-case-observability'], + }, + }) + .expect(200); + expect(result.body).toEqual({ id: expect.any(String) }); + expect(workspaceClientMock.create).toHaveBeenCalledWith( + expect.any(Object), + expect.objectContaining({ + name: 'Observability', + features: ['use-case-observability'], + }) + ); + }); + + describe('feature validation', () => { + it('returns 400 when no features is provided during workspace creation', async () => { + await supertest(httpSetup.server.listener) + .post(WORKSPACES_API_BASE_URL) + .send({ + attributes: { + name: 'Observability', + }, + }) + .expect(400); + }); + it('returns 400 when no valid use case is provided during workspace creation', async () => { + const result = await supertest(httpSetup.server.listener) + .post(WORKSPACES_API_BASE_URL) + .send({ + attributes: { + name: 'Observability', + features: ['use-case-valid'], + }, + }) + .expect(400); + expect(result.body.message).toEqual( + '[request body.attributes.features]: At least one use case is required. Valid options: use-case-all, use-case-observability, use-case-security-analytics, use-case-essentials, use-case-search' + ); + }); + it('returns 400 when multiple use cases are provided during workspace creation', async () => { + const result = await supertest(httpSetup.server.listener) + .post(WORKSPACES_API_BASE_URL) + .send({ + attributes: { + name: 'Observability', + features: ['use-case-observability', 'use-case-all'], + }, + }) + .expect(400); + expect(result.body.message).toEqual( + '[request body.attributes.features]: Only one use case is allowed per workspace.' + ); + }); + it('returns 400 when no valid use case is provided during workspace update', async () => { + const result = await supertest(httpSetup.server.listener) + .put(`${WORKSPACES_API_BASE_URL}/mock-workspace-id`) + .send({ + attributes: { + name: 'Observability', + features: ['feature1', 'feature2'], + }, + }) + .expect(400); + expect(result.body.message).toEqual( + '[request body.attributes.features]: At least one use case is required. Valid options: use-case-all, use-case-observability, use-case-security-analytics, use-case-essentials, use-case-search' + ); + }); + it('updates workspace name successfully without modifying features', async () => { + await supertest(httpSetup.server.listener) + .put(`${WORKSPACES_API_BASE_URL}/mock-workspace-id`) + .send({ + attributes: { + name: 'Observability', + }, + }) + .expect(200); + }); + }); +}); diff --git a/src/plugins/workspace/server/routes/index.ts b/src/plugins/workspace/server/routes/index.ts index 549d915af892..43004d82824f 100644 --- a/src/plugins/workspace/server/routes/index.ts +++ b/src/plugins/workspace/server/routes/index.ts @@ -4,7 +4,8 @@ */ import { schema } from '@osd/config-schema'; -import { CoreSetup, Logger, PrincipalType, ACL } from '../../../../core/server'; +import { IRouter, Logger, PrincipalType, ACL, DEFAULT_NAV_GROUPS } from '../../../../core/server'; +import { getUseCaseFeatureConfig } from '../../common/utils'; import { WorkspacePermissionMode, MAX_WORKSPACE_NAME_LENGTH, @@ -42,9 +43,33 @@ const settingsSchema = schema.object({ dataSources: schema.maybe(dataSourceIds), }); +const featuresSchema = schema.arrayOf(schema.string(), { + minSize: 1, + validate: (featureConfigs) => { + const validateUseCaseConfigs = [ + DEFAULT_NAV_GROUPS.all, + DEFAULT_NAV_GROUPS.observability, + DEFAULT_NAV_GROUPS['security-analytics'], + DEFAULT_NAV_GROUPS.essentials, + DEFAULT_NAV_GROUPS.search, + ].map(({ id }) => getUseCaseFeatureConfig(id)); + + const useCaseConfigCount = featureConfigs.filter((config) => + validateUseCaseConfigs.includes(config) + ).length; + + if (useCaseConfigCount === 0) { + return `At least one use case is required. Valid options: ${validateUseCaseConfigs.join( + ', ' + )}`; + } else if (useCaseConfigCount > 1) { + return 'Only one use case is allowed per workspace.'; + } + }, +}); + const workspaceOptionalAttributesSchema = { description: schema.maybe(schema.string({ maxLength: MAX_WORKSPACE_DESCRIPTION_LENGTH })), - features: schema.maybe(schema.arrayOf(schema.string())), color: schema.maybe( schema.string({ validate: (color) => { @@ -70,30 +95,31 @@ const workspaceNameSchema = schema.string({ const createWorkspaceAttributesSchema = schema.object({ name: workspaceNameSchema, + features: featuresSchema, ...workspaceOptionalAttributesSchema, }); const updateWorkspaceAttributesSchema = schema.object({ name: schema.maybe(workspaceNameSchema), + features: schema.maybe(featuresSchema), ...workspaceOptionalAttributesSchema, }); export function registerRoutes({ client, logger, - http, + router, maxImportExportSize, permissionControlClient, isPermissionControlEnabled, }: { client: IWorkspaceClientImpl; logger: Logger; - http: CoreSetup['http']; + router: IRouter; maxImportExportSize: number; permissionControlClient?: SavedObjectsPermissionControlContract; isPermissionControlEnabled: boolean; }) { - const router = http.createRouter(); router.post( { path: `${WORKSPACES_API_BASE_URL}/_list`, diff --git a/src/plugins/workspace/server/saved_objects/integration_tests/saved_objects_wrapper_for_check_workspace_conflict.test.ts b/src/plugins/workspace/server/saved_objects/integration_tests/saved_objects_wrapper_for_check_workspace_conflict.test.ts index 9405c59a4796..58f7e1e7f525 100644 --- a/src/plugins/workspace/server/saved_objects/integration_tests/saved_objects_wrapper_for_check_workspace_conflict.test.ts +++ b/src/plugins/workspace/server/saved_objects/integration_tests/saved_objects_wrapper_for_check_workspace_conflict.test.ts @@ -38,6 +38,7 @@ const advancedSettings: Omit = { interface WorkspaceAttributes { id: string; name?: string; + features?: string[]; } describe('saved_objects_wrapper_for_check_workspace_conflict integration test', () => { @@ -76,9 +77,11 @@ describe('saved_objects_wrapper_for_check_workspace_conflict integration test', createdFooWorkspace = await createWorkspace({ name: 'foo', + features: ['use-case-all'], }).then((resp) => resp.body.result); createdBarWorkspace = await createWorkspace({ name: 'bar', + features: ['use-case-all'], }).then((resp) => resp.body.result); }, 30000); afterAll(async () => { diff --git a/src/plugins/workspace/server/saved_objects/integration_tests/workspace_id_consumer_wrapper.test.ts b/src/plugins/workspace/server/saved_objects/integration_tests/workspace_id_consumer_wrapper.test.ts index ac643d59e641..84d09af94362 100644 --- a/src/plugins/workspace/server/saved_objects/integration_tests/workspace_id_consumer_wrapper.test.ts +++ b/src/plugins/workspace/server/saved_objects/integration_tests/workspace_id_consumer_wrapper.test.ts @@ -16,6 +16,7 @@ const dashboard: Omit = { interface WorkspaceAttributes { id: string; name?: string; + features?: string[]; } describe('workspace_id_consumer integration test', () => { @@ -56,11 +57,13 @@ describe('workspace_id_consumer integration test', () => { createdFooWorkspace = await createWorkspace({ name: 'foo', + features: ['use-case-all'], }).then((resp) => { return resp.body.result; }); createdBarWorkspace = await createWorkspace({ name: 'bar', + features: ['use-case-all'], }).then((resp) => resp.body.result); }, 30000); afterAll(async () => { diff --git a/src/plugins/workspace/server/saved_objects/integration_tests/workspace_ui_settings_wrapper.test.ts b/src/plugins/workspace/server/saved_objects/integration_tests/workspace_ui_settings_wrapper.test.ts index 35e771780dc6..00a487989cf2 100644 --- a/src/plugins/workspace/server/saved_objects/integration_tests/workspace_ui_settings_wrapper.test.ts +++ b/src/plugins/workspace/server/saved_objects/integration_tests/workspace_ui_settings_wrapper.test.ts @@ -45,7 +45,7 @@ describe('workspace ui settings saved object client wrapper', () => { globalUiSettingsClient = osd.coreStart.uiSettings.asScopedToClient(savedObjectsClient); const res = await osdTestServer.request.post(osd.root, '/api/workspaces').send({ - attributes: { name: 'test workspace' }, + attributes: { name: 'test workspace', features: ['use-case-all'] }, }); testWorkspace = res.body.result; }, 30000); diff --git a/src/plugins/workspace/server/workspace_client.mock.ts b/src/plugins/workspace/server/workspace_client.mock.ts new file mode 100644 index 000000000000..b6dce61cedcf --- /dev/null +++ b/src/plugins/workspace/server/workspace_client.mock.ts @@ -0,0 +1,16 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +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(), +};