Skip to content

Commit

Permalink
[Manual][Backport 2.x] Backport 7884 and 7954 to 2.x (#7984)
Browse files Browse the repository at this point in the history
* [Workspace]Validate features parameter in workspace create and update API (#7884)

* Add validate for features field in workspace create and update API

Signed-off-by: Lin Wang <wonglam@amazon.com>

* Changeset file for PR #7884 created/updated

* Changeset file for PR #7884 created/updated

* Move router outside align with other plugins

Signed-off-by: Lin Wang <wonglam@amazon.com>

* Add ut and fix integration tests

Signed-off-by: Lin Wang <wonglam@amazon.com>

* Import use case id from default nav groups

Signed-off-by: Lin Wang <wonglam@amazon.com>

* Fix workspace routes UT

Signed-off-by: Lin Wang <wonglam@amazon.com>

* Share feature config generator between publicn and server

Signed-off-by: Lin Wang <wonglam@amazon.com>

* Fix osd server crashed due to import from public

Signed-off-by: Lin Wang <wonglam@amazon.com>

---------

Signed-off-by: Lin Wang <wonglam@amazon.com>
Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>

* [Workspace]Fix dynamicConfigServiceMock import path in workspace routes UT (#7954)

* Fix dynamicConfigServiceMock import path in workspace routes UT

Signed-off-by: Lin Wang <wonglam@amazon.com>

* Changeset file for PR #7954 created/updated

---------

Signed-off-by: Lin Wang <wonglam@amazon.com>
Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>

---------

Signed-off-by: Lin Wang <wonglam@amazon.com>
Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>
(cherry picked from commit d3ce40f)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
  • Loading branch information
1 parent 263cf25 commit 7ec7fbe
Show file tree
Hide file tree
Showing 17 changed files with 200 additions and 21 deletions.
2 changes: 2 additions & 0 deletions changelogs/fragments/7884.yml
Original file line number Diff line number Diff line change
@@ -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))
2 changes: 2 additions & 0 deletions changelogs/fragments/7954.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
fix:
- [Workspace]dynamicConfigServiceMock not found in workspace routes UT ([#7954](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/7954))
2 changes: 1 addition & 1 deletion src/core/server/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
2 changes: 2 additions & 0 deletions src/plugins/workspace/common/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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-';
3 changes: 3 additions & 0 deletions src/plugins/workspace/common/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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}`;
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
3 changes: 1 addition & 2 deletions src/plugins/workspace/public/utils.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand Down
7 changes: 2 additions & 5 deletions src/plugins/workspace/public/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -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);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand Down
3 changes: 2 additions & 1 deletion src/plugins/workspace/server/plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -202,9 +202,10 @@ export class WorkspacePlugin implements Plugin<WorkspacePluginSetup, WorkspacePl
const maxImportExportSize = core.savedObjects.getImportExportObjectLimit();
this.logger.info('Workspace permission control enabled:' + isPermissionControlEnabled);
if (isPermissionControlEnabled) this.setupPermission(core);
const router = core.http.createRouter();

registerRoutes({
http: core.http,
router,
logger: this.logger,
client: this.client as IWorkspaceClientImpl,
maxImportExportSize,
Expand Down
127 changes: 127 additions & 0 deletions src/plugins/workspace/server/routes/index.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,127 @@
/*
* Copyright OpenSearch Contributors
* SPDX-License-Identifier: Apache-2.0
*/

import supertest from 'supertest';
import { UnwrapPromise } from '@osd/utility-types';

import { setupServer } from '../../../../core/server/test_utils';
import { loggingSystemMock, dynamicConfigServiceMock } from '../../../../core/server/mocks';

import { workspaceClientMock } from '../workspace_client.mock';

import { registerRoutes, WORKSPACES_API_BASE_URL } from './index';

type SetupServerReturn = UnwrapPromise<ReturnType<typeof setupServer>>;
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);
});
});
});
36 changes: 31 additions & 5 deletions src/plugins/workspace/server/routes/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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) => {
Expand All @@ -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`,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ const advancedSettings: Omit<SavedObject, 'id'> = {
interface WorkspaceAttributes {
id: string;
name?: string;
features?: string[];
}

describe('saved_objects_wrapper_for_check_workspace_conflict integration test', () => {
Expand Down Expand Up @@ -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 () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ const dashboard: Omit<SavedObject, 'id'> = {
interface WorkspaceAttributes {
id: string;
name?: string;
features?: string[];
}

describe('workspace_id_consumer integration test', () => {
Expand Down Expand Up @@ -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 () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
16 changes: 16 additions & 0 deletions src/plugins/workspace/server/workspace_client.mock.ts
Original file line number Diff line number Diff line change
@@ -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(),
};

0 comments on commit 7ec7fbe

Please sign in to comment.