From 227a0d8e1f1a2ffec5f4c6eef14f14d9e859d69b Mon Sep 17 00:00:00 2001 From: Lin Wang Date: Wed, 3 Apr 2024 13:56:27 +0800 Subject: [PATCH 1/3] [Workspace]Add permission control logic for workspace (#6052) * Add permission control for workspace Signed-off-by: Lin Wang * Add changelog for permission control in workspace Signed-off-by: Lin Wang * Fix integration tests and remove no need type Signed-off-by: Lin Wang * Update permission enabled for workspace CRUD integration tests Signed-off-by: Lin Wang * Change back to config schema Signed-off-by: Lin Wang * feat: do not append workspaces field when no workspaces present (#6) * feat: do not append workspaces field when no workspaces present Signed-off-by: SuZhou-Joe * feat: do not append workspaces field when no workspaces present Signed-off-by: SuZhou-Joe --------- Signed-off-by: SuZhou-Joe * fix: authInfo destructure (#7) * fix: authInfo destructure Signed-off-by: SuZhou-Joe * fix: unit test error Signed-off-by: SuZhou-Joe --------- Signed-off-by: SuZhou-Joe * Fix permissions assign in attributes Signed-off-by: Lin Wang * Remove deleteByWorkspace since not exists Signed-off-by: Lin Wang * refactor: remove formatWorkspacePermissionModeToStringArray Signed-off-by: Lin Wang * Remove current not used code Signed-off-by: Lin Wang * Add missing unit tests for permission control Signed-off-by: Lin Wang * Update workspaces API test describe Signed-off-by: Lin Wang * Fix workspace CRUD API integration tests failed Signed-off-by: Lin Wang * Address PR comments Signed-off-by: Lin Wang * Store permissions when savedObjects.permissions.enabled Signed-off-by: Lin Wang * Add permission control for deleteByWorkspace Signed-off-by: Lin Wang * Update src/plugins/workspace/server/permission_control/client.ts Signed-off-by: SuZhou-Joe * Update src/plugins/workspace/server/permission_control/client.ts Signed-off-by: SuZhou-Joe * Refactor permissions field in workspace create and update API Signed-off-by: Lin Wang * Fix workspace CRUD API integration tests Signed-off-by: Lin Wang --------- Signed-off-by: Lin Wang Signed-off-by: SuZhou-Joe Co-authored-by: SuZhou-Joe Signed-off-by: Lin Wang --- src/core/server/index.ts | 3 +- src/core/server/mocks.ts | 3 + .../server/plugins/plugin_context.test.ts | 7 +- src/core/server/plugins/types.ts | 2 +- src/core/server/saved_objects/index.ts | 8 +- .../service/lib/repository.test.js | 15 +- .../saved_objects/service/lib/repository.ts | 4 +- .../server/integration_tests/routes.test.ts | 177 +++++++++++------- .../server/permission_control/client.test.ts | 77 ++++++++ .../server/permission_control/client.ts | 62 +++--- src/plugins/workspace/server/plugin.test.ts | 3 - src/plugins/workspace/server/plugin.ts | 18 +- src/plugins/workspace/server/routes/index.ts | 77 ++++---- ...space_saved_objects_client_wrapper.test.ts | 66 +++++++ ...space_saved_objects_client_wrapper.test.ts | 82 +++++++- .../workspace_saved_objects_client_wrapper.ts | 32 +--- src/plugins/workspace/server/types.ts | 25 +-- .../workspace/server/workspace_client.ts | 27 +-- 18 files changed, 461 insertions(+), 227 deletions(-) diff --git a/src/core/server/index.ts b/src/core/server/index.ts index 84ee65dcb199..f497bed22755 100644 --- a/src/core/server/index.ts +++ b/src/core/server/index.ts @@ -321,12 +321,11 @@ export { exportSavedObjectsToStream, importSavedObjectsFromStream, resolveSavedObjectsImportErrors, - SavedObjectsDeleteByWorkspaceOptions, ACL, Principals, - TransformedPermission, PrincipalType, Permissions, + SavedObjectsDeleteByWorkspaceOptions, } from './saved_objects'; export { diff --git a/src/core/server/mocks.ts b/src/core/server/mocks.ts index 687d408e40a6..dce39d03da7f 100644 --- a/src/core/server/mocks.ts +++ b/src/core/server/mocks.ts @@ -89,6 +89,9 @@ export function pluginInitializerContextConfigMock(config: T) { path: { data: '/tmp' }, savedObjects: { maxImportPayloadBytes: new ByteSizeValue(26214400), + permission: { + enabled: true, + }, }, }; diff --git a/src/core/server/plugins/plugin_context.test.ts b/src/core/server/plugins/plugin_context.test.ts index 7a8ba042825b..57aa372514de 100644 --- a/src/core/server/plugins/plugin_context.test.ts +++ b/src/core/server/plugins/plugin_context.test.ts @@ -108,7 +108,12 @@ describe('createPluginInitializerContext', () => { pingTimeout: duration(30, 's'), }, path: { data: fromRoot('data') }, - savedObjects: { maxImportPayloadBytes: new ByteSizeValue(26214400) }, + savedObjects: { + maxImportPayloadBytes: new ByteSizeValue(26214400), + permission: { + enabled: false, + }, + }, }); }); diff --git a/src/core/server/plugins/types.ts b/src/core/server/plugins/types.ts index 59b9881279c3..c225a24aa386 100644 --- a/src/core/server/plugins/types.ts +++ b/src/core/server/plugins/types.ts @@ -295,7 +295,7 @@ export const SharedGlobalConfigKeys = { ] as const, opensearch: ['shardTimeout', 'requestTimeout', 'pingTimeout'] as const, path: ['data'] as const, - savedObjects: ['maxImportPayloadBytes'] as const, + savedObjects: ['maxImportPayloadBytes', 'permission'] as const, }; /** diff --git a/src/core/server/saved_objects/index.ts b/src/core/server/saved_objects/index.ts index 11809c5b88c9..dccf63d4dcf4 100644 --- a/src/core/server/saved_objects/index.ts +++ b/src/core/server/saved_objects/index.ts @@ -85,10 +85,4 @@ export { export { savedObjectsConfig, savedObjectsMigrationConfig } from './saved_objects_config'; export { SavedObjectTypeRegistry, ISavedObjectTypeRegistry } from './saved_objects_type_registry'; -export { - Permissions, - ACL, - Principals, - TransformedPermission, - PrincipalType, -} from './permission_control/acl'; +export { Permissions, ACL, Principals, PrincipalType } from './permission_control/acl'; diff --git a/src/core/server/saved_objects/service/lib/repository.test.js b/src/core/server/saved_objects/service/lib/repository.test.js index d26589882273..087ff2e458d9 100644 --- a/src/core/server/saved_objects/service/lib/repository.test.js +++ b/src/core/server/saved_objects/service/lib/repository.test.js @@ -167,7 +167,7 @@ describe('SavedObjectsRepository', () => { }); const getMockGetResponse = ( - { type, id, references, namespace: objectNamespace, originId, workspaces, permissions }, + { type, id, references, namespace: objectNamespace, originId, permissions, workspaces }, namespace ) => { const namespaceId = objectNamespace === 'default' ? undefined : objectNamespace ?? namespace; @@ -181,9 +181,9 @@ describe('SavedObjectsRepository', () => { _source: { ...(registry.isSingleNamespace(type) && { namespace: namespaceId }), ...(registry.isMultiNamespace(type) && { namespaces: [namespaceId ?? 'default'] }), - workspaces, ...(originId && { originId }), ...(permissions && { permissions }), + ...(workspaces && { workspaces }), type, [type]: { title: 'Testing' }, references, @@ -3169,7 +3169,7 @@ describe('SavedObjectsRepository', () => { const namespace = 'foo-namespace'; const originId = 'some-origin-id'; - const getSuccess = async (type, id, options, includeOriginId, permissions) => { + const getSuccess = async (type, id, options, includeOriginId, permissions, workspaces) => { const response = getMockGetResponse( { type, @@ -3178,6 +3178,7 @@ describe('SavedObjectsRepository', () => { // operation will return it in the result. This flag is just used for test purposes to modify the mock cluster call response. ...(includeOriginId && { originId }), ...(permissions && { permissions }), + ...(workspaces && { workspaces }), }, options?.namespace ); @@ -3343,6 +3344,14 @@ describe('SavedObjectsRepository', () => { permissions: permissions, }); }); + + it(`includes workspaces property if present`, async () => { + const workspaces = ['workspace-1']; + const result = await getSuccess(type, id, { namespace }, undefined, undefined, workspaces); + expect(result).toMatchObject({ + workspaces: workspaces, + }); + }); }); }); diff --git a/src/core/server/saved_objects/service/lib/repository.ts b/src/core/server/saved_objects/service/lib/repository.ts index c3de94bf84b9..34ff9b1e0d8f 100644 --- a/src/core/server/saved_objects/service/lib/repository.ts +++ b/src/core/server/saved_objects/service/lib/repository.ts @@ -1044,7 +1044,7 @@ export class SavedObjectsRepository { throw SavedObjectsErrorHelpers.createGenericNotFoundError(type, id); } - const { originId, updated_at: updatedAt, workspaces, permissions } = body._source; + const { originId, updated_at: updatedAt, permissions, workspaces } = body._source; let namespaces: string[] = []; if (!this._registry.isNamespaceAgnostic(type)) { @@ -1059,8 +1059,8 @@ export class SavedObjectsRepository { namespaces, ...(originId && { originId }), ...(updatedAt && { updated_at: updatedAt }), - ...(workspaces && { workspaces }), ...(permissions && { permissions }), + ...(workspaces && { workspaces }), version: encodeHitVersion(body), attributes: body._source[type], references: body._source.references || [], diff --git a/src/plugins/workspace/server/integration_tests/routes.test.ts b/src/plugins/workspace/server/integration_tests/routes.test.ts index 0c6e55101b7f..832c43c66399 100644 --- a/src/plugins/workspace/server/integration_tests/routes.test.ts +++ b/src/plugins/workspace/server/integration_tests/routes.test.ts @@ -5,8 +5,7 @@ import { WorkspaceAttribute } from 'src/core/types'; import * as osdTestServer from '../../../../core/test_helpers/osd_server'; -import { WORKSPACE_TYPE } from '../../../../core/server'; -import { WorkspacePermissionItem } from '../types'; +import { WORKSPACE_TYPE, Permissions } from '../../../../core/server'; const omitId = (object: T): Omit => { const { id, ...others } = object; @@ -19,7 +18,7 @@ const testWorkspace: WorkspaceAttribute = { description: 'test_workspace_description', }; -describe('workspace service', () => { +describe('workspace service api integration test', () => { let root: ReturnType; let opensearchServer: osdTestServer.TestOpenSearchUtils; let osd: osdTestServer.TestOpenSearchDashboardsUtils; @@ -36,7 +35,7 @@ describe('workspace service', () => { }, savedObjects: { permission: { - enabled: true, + enabled: false, }, }, migrations: { skip: false }, @@ -89,39 +88,6 @@ describe('workspace service', () => { expect(result.body.success).toEqual(true); expect(typeof result.body.result.id).toBe('string'); }); - it('create with permissions', async () => { - await osdTestServer.request - .post(root, `/api/workspaces`) - .send({ - attributes: omitId(testWorkspace), - permissions: [{ type: 'invalid-type', userId: 'foo', modes: ['read'] }], - }) - .expect(400); - - const result: any = await osdTestServer.request - .post(root, `/api/workspaces`) - .send({ - attributes: omitId(testWorkspace), - permissions: [{ type: 'user', userId: 'foo', modes: ['read'] }], - }) - .expect(200); - - expect(result.body.success).toEqual(true); - expect(typeof result.body.result.id).toBe('string'); - expect( - ( - await osd.coreStart.savedObjects - .createInternalRepository([WORKSPACE_TYPE]) - .get<{ permissions: WorkspacePermissionItem[] }>(WORKSPACE_TYPE, result.body.result.id) - ).attributes.permissions - ).toEqual([ - { - modes: ['read'], - type: 'user', - userId: 'foo', - }, - ]); - }); it('get', async () => { const result = await osdTestServer.request .post(root, `/api/workspaces`) @@ -162,39 +128,6 @@ describe('workspace service', () => { expect(getResult.body.success).toEqual(true); expect(getResult.body.result.name).toEqual('updated'); }); - it('update with permissions', async () => { - const result: any = await osdTestServer.request - .post(root, `/api/workspaces`) - .send({ - attributes: omitId(testWorkspace), - permissions: [{ type: 'user', userId: 'foo', modes: ['read'] }], - }) - .expect(200); - - await osdTestServer.request - .put(root, `/api/workspaces/${result.body.result.id}`) - .send({ - attributes: { - ...omitId(testWorkspace), - }, - permissions: [{ type: 'user', userId: 'foo', modes: ['write'] }], - }) - .expect(200); - - expect( - ( - await osd.coreStart.savedObjects - .createInternalRepository([WORKSPACE_TYPE]) - .get<{ permissions: WorkspacePermissionItem[] }>(WORKSPACE_TYPE, result.body.result.id) - ).attributes.permissions - ).toEqual([ - { - modes: ['write'], - type: 'user', - userId: 'foo', - }, - ]); - }); it('delete', async () => { const result: any = await osdTestServer.request .post(root, `/api/workspaces`) @@ -339,3 +272,107 @@ describe('workspace service', () => { }); }); }); + +describe('workspace service api integration test when savedObjects.permission.enabled equal true', () => { + let root: ReturnType; + let opensearchServer: osdTestServer.TestOpenSearchUtils; + let osd: osdTestServer.TestOpenSearchDashboardsUtils; + beforeAll(async () => { + const { startOpenSearch, startOpenSearchDashboards } = osdTestServer.createTestServers({ + adjustTimeout: (t: number) => jest.setTimeout(t), + settings: { + osd: { + workspace: { + enabled: true, + }, + savedObjects: { + permission: { + enabled: true, + }, + }, + migrations: { skip: false }, + }, + }, + }); + opensearchServer = await startOpenSearch(); + osd = await startOpenSearchDashboards(); + root = osd.root; + }); + afterAll(async () => { + await root.shutdown(); + await opensearchServer.stop(); + }); + describe('Workspace CRUD APIs', () => { + afterEach(async () => { + const listResult = await osdTestServer.request + .post(root, `/api/workspaces/_list`) + .send({ + page: 1, + }) + .expect(200); + const savedObjectsRepository = osd.coreStart.savedObjects.createInternalRepository([ + WORKSPACE_TYPE, + ]); + await Promise.all( + listResult.body.result.workspaces.map((item: WorkspaceAttribute) => + // this will delete reserved workspace + savedObjectsRepository.delete(WORKSPACE_TYPE, item.id) + ) + ); + }); + it('create', async () => { + await osdTestServer.request + .post(root, `/api/workspaces`) + .send({ + attributes: omitId(testWorkspace), + permissions: { invalid_type: { users: ['foo'] } }, + }) + .expect(400); + + const result: any = await osdTestServer.request + .post(root, `/api/workspaces`) + .send({ + attributes: omitId(testWorkspace), + permissions: { read: { users: ['foo'] } }, + }) + .expect(200); + + expect(result.body.success).toEqual(true); + expect(typeof result.body.result.id).toBe('string'); + expect( + ( + await osd.coreStart.savedObjects + .createInternalRepository([WORKSPACE_TYPE]) + .get<{ permissions: Permissions }>(WORKSPACE_TYPE, result.body.result.id) + ).permissions + ).toEqual({ read: { users: ['foo'] } }); + }); + it('update', async () => { + const result: any = await osdTestServer.request + .post(root, `/api/workspaces`) + .send({ + attributes: omitId(testWorkspace), + }) + .expect(200); + + const updateResult = await osdTestServer.request + .put(root, `/api/workspaces/${result.body.result.id}`) + .send({ + attributes: { + ...omitId(testWorkspace), + }, + permissions: { write: { users: ['foo'] } }, + }) + .expect(200); + expect(updateResult.body.result).toBe(true); + + expect( + ( + await osd.coreStart.savedObjects + .createInternalRepository([WORKSPACE_TYPE]) + .get<{ permissions: Permissions }>(WORKSPACE_TYPE, result.body.result.id) + ).permissions + ).toEqual({ write: { users: ['foo'] } }); + }); + }); +}); diff --git a/src/plugins/workspace/server/permission_control/client.test.ts b/src/plugins/workspace/server/permission_control/client.test.ts index e05e299c153b..4d041cc7df56 100644 --- a/src/plugins/workspace/server/permission_control/client.test.ts +++ b/src/plugins/workspace/server/permission_control/client.test.ts @@ -109,6 +109,47 @@ describe('PermissionControl', () => { expect(batchValidateResult.result).toEqual(true); }); + it('should return false and log not permitted saved objects', async () => { + const logger = loggerMock.create(); + const permissionControlClient = new SavedObjectsPermissionControl(logger); + const getScopedClient = jest.fn(); + const clientMock = savedObjectsClientMock.create(); + getScopedClient.mockImplementation((request) => { + return clientMock; + }); + permissionControlClient.setup(getScopedClient, mockAuth); + + clientMock.bulkGet.mockResolvedValue({ + saved_objects: [ + { + id: 'foo', + type: 'dashboard', + references: [], + attributes: {}, + }, + { + id: 'bar', + type: 'dashboard', + references: [], + attributes: {}, + permissions: { + read: { + users: ['foo'], + }, + }, + }, + ], + }); + const batchValidateResult = await permissionControlClient.batchValidate( + httpServerMock.createOpenSearchDashboardsRequest(), + [], + ['read'] + ); + expect(batchValidateResult.success).toEqual(true); + expect(batchValidateResult.result).toEqual(false); + expect(logger.debug).toHaveBeenCalledTimes(1); + }); + describe('getPrincipalsFromRequest', () => { const permissionControlClient = new SavedObjectsPermissionControl(loggerMock.create()); const getScopedClient = jest.fn(); @@ -120,4 +161,40 @@ describe('PermissionControl', () => { expect(result.users).toEqual(['bar']); }); }); + + describe('validateSavedObjectsACL', () => { + it("should return true if saved objects don't have permissions property", () => { + const permissionControlClient = new SavedObjectsPermissionControl(loggerMock.create()); + expect( + permissionControlClient.validateSavedObjectsACL([{ type: 'workspace', id: 'foo' }], {}, []) + ).toBe(true); + }); + it('should return true if principals permitted to saved objects', () => { + const permissionControlClient = new SavedObjectsPermissionControl(loggerMock.create()); + expect( + permissionControlClient.validateSavedObjectsACL( + [{ type: 'workspace', id: 'foo', permissions: { write: { users: ['bar'] } } }], + { users: ['bar'] }, + ['write'] + ) + ).toBe(true); + }); + it('should return false and log saved objects if not permitted', () => { + const logger = loggerMock.create(); + const permissionControlClient = new SavedObjectsPermissionControl(logger); + expect( + permissionControlClient.validateSavedObjectsACL( + [{ type: 'workspace', id: 'foo', permissions: { write: { users: ['bar'] } } }], + { users: ['foo'] }, + ['write'] + ) + ).toBe(false); + expect(logger.debug).toHaveBeenCalledTimes(1); + expect(logger.debug).toHaveBeenCalledWith( + expect.stringMatching( + /Authorization failed, principals:.*has no.*permissions on the requested saved object:.*foo/ + ) + ); + }); + }); }); diff --git a/src/plugins/workspace/server/permission_control/client.ts b/src/plugins/workspace/server/permission_control/client.ts index bad46eb156a6..ea404e42974f 100644 --- a/src/plugins/workspace/server/permission_control/client.ts +++ b/src/plugins/workspace/server/permission_control/client.ts @@ -6,7 +6,6 @@ import { i18n } from '@osd/i18n'; import { ACL, - TransformedPermission, SavedObjectsBulkGetObject, SavedObjectsServiceStart, Logger, @@ -14,7 +13,6 @@ import { Principals, SavedObject, WORKSPACE_TYPE, - Permissions, HttpAuth, } from '../../../../core/server'; import { WORKSPACE_SAVED_OBJECTS_CLIENT_WRAPPER_ID } from '../../common/constants'; @@ -31,6 +29,13 @@ export class SavedObjectsPermissionControl { private readonly logger: Logger; private _getScopedClient?: SavedObjectsServiceStart['getScopedClient']; private auth?: HttpAuth; + /** + * Returns a saved objects client that is able to: + * 1. Read objects whose type is `workspace` because workspace is a hidden type and the permission control client will need to get the metadata of a specific workspace to do the permission check. + * 2. Bypass saved objects permission control wrapper because the permission control client is a dependency of the wrapper to provide the ACL validation capability. It will run into infinite loop if not bypass. + * @param request + * @returns SavedObjectsContract + */ private getScopedClient(request: OpenSearchDashboardsRequest) { return this._getScopedClient?.(request, { excludedWrappers: [WORKSPACE_SAVED_OBJECTS_CLIENT_WRAPPER_ID], @@ -83,6 +88,15 @@ export class SavedObjectsPermissionControl { return getPrincipalsFromRequest(request, this.auth); } + /** + * Validates the permissions for a collection of saved objects based on their Access Control Lists (ACL). + * This method checks whether the provided principals have the specified permission modes for each saved object. + * If any saved object lacks the required permissions, the function logs details of unauthorized access. + * + * @remarks + * If a saved object doesn't have an ACL (e.g., config objects), it is considered as having the required permissions. + * The function logs detailed information when unauthorized access is detected, including the list of denied saved objects. + */ public validateSavedObjectsACL( savedObjects: Array, 'id' | 'type' | 'workspaces' | 'permissions'>>, principals: Principals, @@ -101,7 +115,12 @@ export class SavedObjectsPermissionControl { const aclInstance = new ACL(savedObject.permissions); const hasPermission = aclInstance.hasPermission(permissionModes, principals); if (!hasPermission) { - notPermittedSavedObjects.push(savedObject); + notPermittedSavedObjects.push({ + id: savedObject.id, + type: savedObject.type, + workspaces: savedObject.workspaces, + permissions: savedObject.permissions, + }); } return hasPermission; }); @@ -145,38 +164,11 @@ export class SavedObjectsPermissionControl { } const principals = this.getPrincipalsFromRequest(request); - const deniedObjects: Array< - Pick & { - workspaces?: string[]; - permissions?: Permissions; - } - > = []; - const hasPermissionToAllObjects = savedObjectsGet.every((item) => { - // for object that doesn't contain ACL like config, return true - if (!item.permissions) { - return true; - } - const aclInstance = new ACL(item.permissions); - const hasPermission = aclInstance.hasPermission(permissionModes, principals); - if (!hasPermission) { - deniedObjects.push({ - id: item.id, - type: item.type, - workspaces: item.workspaces, - permissions: item.permissions, - }); - } - return hasPermission; - }); - if (!hasPermissionToAllObjects) { - this.logger.debug( - `Authorization failed, principals: ${JSON.stringify( - principals - )} has no [${permissionModes}] permissions on the requested saved object: ${JSON.stringify( - deniedObjects - )}` - ); - } + const hasPermissionToAllObjects = this.validateSavedObjectsACL( + savedObjectsGet, + principals, + permissionModes + ); return { success: true, result: hasPermissionToAllObjects, diff --git a/src/plugins/workspace/server/plugin.test.ts b/src/plugins/workspace/server/plugin.test.ts index c448fcf209f9..684f754ce9dd 100644 --- a/src/plugins/workspace/server/plugin.test.ts +++ b/src/plugins/workspace/server/plugin.test.ts @@ -12,9 +12,6 @@ describe('Workspace server plugin', () => { const setupMock = coreMock.createSetup(); const initializerContextConfigMock = coreMock.createPluginInitializerContext({ enabled: true, - permission: { - enabled: true, - }, }); setupMock.capabilities.registerProvider.mockImplementationOnce((fn) => (value = fn())); const workspacePlugin = new WorkspacePlugin(initializerContextConfigMock); diff --git a/src/plugins/workspace/server/plugin.ts b/src/plugins/workspace/server/plugin.ts index 0fff0082476b..8e3da6a8cfe5 100644 --- a/src/plugins/workspace/server/plugin.ts +++ b/src/plugins/workspace/server/plugin.ts @@ -11,29 +11,29 @@ import { Plugin, Logger, CoreStart, + SharedGlobalConfig, } from '../../../core/server'; import { WORKSPACE_SAVED_OBJECTS_CLIENT_WRAPPER_ID, WORKSPACE_CONFLICT_CONTROL_SAVED_OBJECTS_CLIENT_WRAPPER_ID, } from '../common/constants'; -import { IWorkspaceClientImpl } from './types'; +import { cleanWorkspaceId, getWorkspaceIdFromUrl } from '../../../core/server/utils'; +import { IWorkspaceClientImpl, WorkspacePluginSetup, WorkspacePluginStart } from './types'; import { WorkspaceClient } from './workspace_client'; import { registerRoutes } from './routes'; import { WorkspaceSavedObjectsClientWrapper } from './saved_objects'; -import { cleanWorkspaceId, getWorkspaceIdFromUrl } from '../../../core/server/utils'; import { WorkspaceConflictSavedObjectsClientWrapper } from './saved_objects/saved_objects_wrapper_for_check_workspace_conflict'; import { SavedObjectsPermissionControl, SavedObjectsPermissionControlContract, } from './permission_control/client'; -import { WorkspacePluginConfigType } from '../config'; -export class WorkspacePlugin implements Plugin<{}, {}> { +export class WorkspacePlugin implements Plugin { private readonly logger: Logger; private client?: IWorkspaceClientImpl; private workspaceConflictControl?: WorkspaceConflictSavedObjectsClientWrapper; private permissionControl?: SavedObjectsPermissionControlContract; - private readonly config$: Observable; + private readonly globalConfig$: Observable; private workspaceSavedObjectsClientWrapper?: WorkspaceSavedObjectsClientWrapper; private proxyWorkspaceTrafficToRealHandler(setupDeps: CoreSetup) { @@ -57,14 +57,13 @@ export class WorkspacePlugin implements Plugin<{}, {}> { constructor(initializerContext: PluginInitializerContext) { this.logger = initializerContext.logger.get('plugins', 'workspace'); - this.config$ = initializerContext.config.create(); + this.globalConfig$ = initializerContext.config.legacy.globalConfig$; } public async setup(core: CoreSetup) { this.logger.debug('Setting up Workspaces service'); - const config: WorkspacePluginConfigType = await this.config$.pipe(first()).toPromise(); - const isPermissionControlEnabled = - config.permission.enabled === undefined ? true : config.permission.enabled; + const globalConfig = await this.globalConfig$.pipe(first()).toPromise(); + const isPermissionControlEnabled = globalConfig.savedObjects.permission.enabled === true; this.client = new WorkspaceClient(core, this.logger); @@ -99,6 +98,7 @@ export class WorkspacePlugin implements Plugin<{}, {}> { logger: this.logger, client: this.client as IWorkspaceClientImpl, permissionControlClient: this.permissionControl, + isPermissionControlEnabled, }); core.capabilities.registerProvider(() => ({ diff --git a/src/plugins/workspace/server/routes/index.ts b/src/plugins/workspace/server/routes/index.ts index b02bff76c132..d48a257bf6dc 100644 --- a/src/plugins/workspace/server/routes/index.ts +++ b/src/plugins/workspace/server/routes/index.ts @@ -4,9 +4,9 @@ */ import { schema } from '@osd/config-schema'; -import { CoreSetup, Logger } from '../../../../core/server'; +import { CoreSetup, Logger, PrincipalType, ACL } from '../../../../core/server'; import { WorkspacePermissionMode } from '../../common/constants'; -import { IWorkspaceClientImpl, WorkspacePermissionItem } from '../types'; +import { IWorkspaceClientImpl, WorkspaceAttributeWithPermission } from '../types'; import { SavedObjectsPermissionControlContract } from '../permission_control/client'; const WORKSPACES_API_BASE_URL = '/api/workspaces'; @@ -18,19 +18,16 @@ const workspacePermissionMode = schema.oneOf([ schema.literal(WorkspacePermissionMode.LibraryWrite), ]); -const workspacePermission = schema.oneOf([ - schema.object({ - type: schema.literal('user'), - userId: schema.string(), - modes: schema.arrayOf(workspacePermissionMode), - }), - schema.object({ - type: schema.literal('group'), - group: schema.string(), - modes: schema.arrayOf(workspacePermissionMode), - }), +const principalType = schema.oneOf([ + schema.literal(PrincipalType.Users), + schema.literal(PrincipalType.Groups), ]); +const workspacePermissions = schema.recordOf( + workspacePermissionMode, + schema.recordOf(principalType, schema.arrayOf(schema.string()), {}) +); + const workspaceAttributesSchema = schema.object({ description: schema.maybe(schema.string()), name: schema.string(), @@ -46,11 +43,13 @@ export function registerRoutes({ logger, http, permissionControlClient, + isPermissionControlEnabled, }: { client: IWorkspaceClientImpl; logger: Logger; http: CoreSetup['http']; permissionControlClient?: SavedObjectsPermissionControlContract; + isPermissionControlEnabled: boolean; }) { const router = http.createRouter(); router.post( @@ -119,29 +118,30 @@ export function registerRoutes({ validate: { body: schema.object({ attributes: workspaceAttributesSchema, - permissions: schema.maybe( - schema.oneOf([workspacePermission, schema.arrayOf(workspacePermission)]) - ), + permissions: schema.maybe(workspacePermissions), }), }, }, router.handleLegacyErrors(async (context, req, res) => { - const { attributes, permissions: permissionsInRequest } = req.body; - const authInfo = permissionControlClient?.getPrincipalsFromRequest(req); - let permissions: WorkspacePermissionItem[] = []; - if (permissionsInRequest) { - permissions = Array.isArray(permissionsInRequest) - ? permissionsInRequest - : [permissionsInRequest]; - } + const { attributes, permissions } = req.body; + const principals = permissionControlClient?.getPrincipalsFromRequest(req); + const createPayload: Omit = attributes; - // Assign workspace owner to current user - if (!!authInfo?.users?.length) { - permissions.push({ - type: 'user', - userId: authInfo.users[0], - modes: [WorkspacePermissionMode.LibraryWrite, WorkspacePermissionMode.Write], - }); + if (isPermissionControlEnabled) { + createPayload.permissions = permissions; + // Assign workspace owner to current user + if (!!principals?.users?.length) { + const acl = new ACL(permissions); + const currentUserId = principals.users[0]; + [WorkspacePermissionMode.Write, WorkspacePermissionMode.LibraryWrite].forEach( + (permissionMode) => { + if (!acl.hasPermission([permissionMode], { users: [currentUserId] })) { + acl.addPermission([permissionMode], { users: [currentUserId] }); + } + } + ); + createPayload.permissions = acl.getPermissions(); + } } const result = await client.create( @@ -150,10 +150,7 @@ export function registerRoutes({ request: req, logger, }, - { - ...attributes, - ...(permissions.length ? { permissions } : {}), - } + createPayload ); return res.ok({ body: result }); }) @@ -167,19 +164,13 @@ export function registerRoutes({ }), body: schema.object({ attributes: workspaceAttributesSchema, - permissions: schema.maybe( - schema.oneOf([workspacePermission, schema.arrayOf(workspacePermission)]) - ), + permissions: schema.maybe(workspacePermissions), }), }, }, router.handleLegacyErrors(async (context, req, res) => { const { id } = req.params; const { attributes, permissions } = req.body; - let finalPermissions: WorkspacePermissionItem[] = []; - if (permissions) { - finalPermissions = Array.isArray(permissions) ? permissions : [permissions]; - } const result = await client.update( { @@ -190,7 +181,7 @@ export function registerRoutes({ id, { ...attributes, - ...(finalPermissions.length ? { permissions: finalPermissions } : {}), + ...(isPermissionControlEnabled ? { permissions } : {}), } ); return res.ok({ body: result }); diff --git a/src/plugins/workspace/server/saved_objects/integration_tests/workspace_saved_objects_client_wrapper.test.ts b/src/plugins/workspace/server/saved_objects/integration_tests/workspace_saved_objects_client_wrapper.test.ts index 2a7fb0e440b5..b6ea26456f0e 100644 --- a/src/plugins/workspace/server/saved_objects/integration_tests/workspace_saved_objects_client_wrapper.test.ts +++ b/src/plugins/workspace/server/saved_objects/integration_tests/workspace_saved_objects_client_wrapper.test.ts @@ -527,4 +527,70 @@ describe('WorkspaceSavedObjectsClientWrapper', () => { expect(SavedObjectsErrorHelpers.isNotFoundError(error)).toBe(true); }); }); + + describe('deleteByWorkspace', () => { + it('should throw forbidden error when workspace not permitted', async () => { + let error; + try { + await notPermittedSavedObjectedClient.deleteByWorkspace('workspace-1'); + } catch (e) { + error = e; + } + + expect(SavedObjectsErrorHelpers.isForbiddenError(error)).toBe(true); + }); + + it('should be able to delete all data in permitted workspace', async () => { + const deleteWorkspaceId = 'workspace-to-delete'; + await repositoryKit.create( + internalSavedObjectsRepository, + 'workspace', + {}, + { + id: deleteWorkspaceId, + permissions: { + library_read: { users: ['foo'] }, + library_write: { users: ['foo'] }, + }, + } + ); + const dashboardIds = [ + 'inner-delete-workspace-dashboard-1', + 'inner-delete-workspace-dashboard-2', + ]; + await Promise.all( + dashboardIds.map((dashboardId) => + repositoryKit.create( + internalSavedObjectsRepository, + 'dashboard', + {}, + { + id: dashboardId, + workspaces: [deleteWorkspaceId], + } + ) + ) + ); + + expect( + ( + await permittedSavedObjectedClient.find({ + type: 'dashboard', + workspaces: [deleteWorkspaceId], + }) + ).total + ).toBe(2); + + await permittedSavedObjectedClient.deleteByWorkspace(deleteWorkspaceId, { refresh: true }); + + expect( + ( + await permittedSavedObjectedClient.find({ + type: 'dashboard', + workspaces: [deleteWorkspaceId], + }) + ).total + ).toBe(0); + }); + }); }); diff --git a/src/plugins/workspace/server/saved_objects/workspace_saved_objects_client_wrapper.test.ts b/src/plugins/workspace/server/saved_objects/workspace_saved_objects_client_wrapper.test.ts index 6b40f6e60fa0..07d1e6aff40c 100644 --- a/src/plugins/workspace/server/saved_objects/workspace_saved_objects_client_wrapper.test.ts +++ b/src/plugins/workspace/server/saved_objects/workspace_saved_objects_client_wrapper.test.ts @@ -26,6 +26,15 @@ const generateWorkspaceSavedObjectsClientWrapper = () => { }, permissions: {}, }, + { + type: 'dashboard', + id: 'dashboard-with-empty-workspace-property', + workspaces: [], + attributes: { + bar: 'baz', + }, + permissions: {}, + }, { type: 'workspace', id: 'workspace-1', attributes: { name: 'Workspace - 1' } }, { type: 'workspace', @@ -40,6 +49,9 @@ const generateWorkspaceSavedObjectsClientWrapper = () => { type: 'config', }; } + if (id === 'unknown-error-dashboard') { + throw new Error('Unknown error'); + } return ( savedObjectsStore.find((item) => item.type === type && item.id === id) || SavedObjectsErrorHelpers.createGenericNotFoundError() @@ -82,14 +94,16 @@ const generateWorkspaceSavedObjectsClientWrapper = () => { getPrincipalsFromRequest: jest.fn().mockImplementation(() => ({ users: ['user-1'] })), }; const wrapper = new WorkspaceSavedObjectsClientWrapper(permissionControlMock); - wrapper.setScopedClient(() => ({ + const scopedClientMock = { find: jest.fn().mockImplementation(async () => ({ saved_objects: [{ id: 'workspace-1', type: 'workspace' }], })), - })); + }; + wrapper.setScopedClient(() => scopedClientMock); return { wrapper: wrapper.wrapperFactory(wrapperOptions), clientMock, + scopedClientMock, permissionControlMock, requestMock, }; @@ -122,6 +136,16 @@ describe('WorkspaceSavedObjectsClientWrapper', () => { ); expect(errorCatched?.message).toEqual('Invalid saved objects permission'); }); + it("should throw permission error if deleting saved object's workspace property is empty", async () => { + const { wrapper } = generateWorkspaceSavedObjectsClientWrapper(); + let errorCatched; + try { + await wrapper.delete('dashboard', 'dashboard-with-empty-workspace-property'); + } catch (e) { + errorCatched = e; + } + expect(errorCatched?.message).toEqual('Invalid saved objects permission'); + }); it('should call client.delete with arguments if permitted', async () => { const { wrapper, clientMock } = generateWorkspaceSavedObjectsClientWrapper(); const deleteArgs = ['dashboard', 'foo', { force: true }] as const; @@ -157,6 +181,18 @@ describe('WorkspaceSavedObjectsClientWrapper', () => { ); expect(errorCatched?.message).toEqual('Invalid saved objects permission'); }); + it("should throw permission error if updating saved object's workspace property is empty", async () => { + const { wrapper } = generateWorkspaceSavedObjectsClientWrapper(); + let errorCatched; + try { + await wrapper.update('dashboard', 'dashboard-with-empty-workspace-property', { + bar: 'foo', + }); + } catch (e) { + errorCatched = e; + } + expect(errorCatched?.message).toEqual('Invalid saved objects permission'); + }); it('should call client.update with arguments if permitted', async () => { const { wrapper, clientMock } = generateWorkspaceSavedObjectsClientWrapper(); const updateArgs = [ @@ -260,6 +296,26 @@ describe('WorkspaceSavedObjectsClientWrapper', () => { ); expect(errorCatched?.message).toEqual('Invalid workspace permission'); }); + it('should throw error if unable to get object when override', async () => { + const { + wrapper, + permissionControlMock, + requestMock, + } = generateWorkspaceSavedObjectsClientWrapper(); + permissionControlMock.validate.mockResolvedValueOnce({ success: true, result: false }); + let errorCatched; + try { + await wrapper.bulkCreate( + [{ type: 'dashboard', id: 'unknown-error-dashboard', attributes: { bar: 'baz' } }], + { + overwrite: true, + } + ); + } catch (e) { + errorCatched = e; + } + expect(errorCatched?.message).toBe('Unknown error'); + }); it('should call client.bulkCreate with arguments if some objects not found', async () => { const { wrapper, clientMock } = generateWorkspaceSavedObjectsClientWrapper(); const objectsToBulkCreate = [ @@ -268,11 +324,9 @@ describe('WorkspaceSavedObjectsClientWrapper', () => { ]; await wrapper.bulkCreate(objectsToBulkCreate, { overwrite: true, - workspaces: ['workspace-1'], }); expect(clientMock.bulkCreate).toHaveBeenCalledWith(objectsToBulkCreate, { overwrite: true, - workspaces: ['workspace-1'], }); }); it('should call client.bulkCreate with arguments if permitted', async () => { @@ -549,6 +603,24 @@ describe('WorkspaceSavedObjectsClientWrapper', () => { ACLSearchParams: {}, }); }); + it('should find permitted workspaces with filtered permission modes', async () => { + const { wrapper, scopedClientMock } = generateWorkspaceSavedObjectsClientWrapper(); + await wrapper.find({ + type: 'dashboard', + ACLSearchParams: { + permissionModes: ['read', 'library_read'], + }, + }); + expect(scopedClientMock.find).toHaveBeenCalledWith( + expect.objectContaining({ + type: 'workspace', + ACLSearchParams: { + permissionModes: ['library_read'], + principals: { users: ['user-1'] }, + }, + }) + ); + }); it('should call client.find with arguments if not workspace type and no options.workspace', async () => { const { wrapper, clientMock } = generateWorkspaceSavedObjectsClientWrapper(); await wrapper.find({ @@ -556,8 +628,6 @@ describe('WorkspaceSavedObjectsClientWrapper', () => { }); expect(clientMock.find).toHaveBeenCalledWith({ type: 'dashboard', - workspaces: ['workspace-1'], - workspacesSearchOperator: 'OR', ACLSearchParams: { permissionModes: ['read', 'write'], principals: { users: ['user-1'] }, diff --git a/src/plugins/workspace/server/saved_objects/workspace_saved_objects_client_wrapper.ts b/src/plugins/workspace/server/saved_objects/workspace_saved_objects_client_wrapper.ts index c515f555fa4b..30c1c91c4223 100644 --- a/src/plugins/workspace/server/saved_objects/workspace_saved_objects_client_wrapper.ts +++ b/src/plugins/workspace/server/saved_objects/workspace_saved_objects_client_wrapper.ts @@ -22,10 +22,10 @@ import { SavedObjectsBulkUpdateResponse, SavedObjectsBulkUpdateOptions, WORKSPACE_TYPE, - SavedObjectsDeleteByWorkspaceOptions, SavedObjectsErrorHelpers, SavedObjectsServiceStart, SavedObjectsClientContract, + SavedObjectsDeleteByWorkspaceOptions, } from '../../../../core/server'; import { SavedObjectsPermissionControlContract } from '../permission_control/client'; import { @@ -68,22 +68,13 @@ const getDefaultValuesForEmpty = (values: T[] | undefined, defaultValues: T[] export class WorkspaceSavedObjectsClientWrapper { private getScopedClient?: SavedObjectsServiceStart['getScopedClient']; - private formatWorkspacePermissionModeToStringArray( - permission: WorkspacePermissionMode | WorkspacePermissionMode[] - ): string[] { - if (Array.isArray(permission)) { - return permission; - } - - return [permission]; - } private async validateObjectsPermissions( objects: Array>, request: OpenSearchDashboardsRequest, - permissionMode: WorkspacePermissionMode | WorkspacePermissionMode[] + permissionModes: WorkspacePermissionMode[] ) { - // PermissionMode here is an array which is merged by workspace type required permission and other saved object required permission. + // PermissionModes here is an array which is merged by workspace type required permission and other saved object required permission. // So we only need to do one permission check no matter its type. for (const { id, type } of objects) { const validateResult = await this.permissionControl.validate( @@ -92,7 +83,7 @@ export class WorkspaceSavedObjectsClientWrapper { type, id, }, - this.formatWorkspacePermissionModeToStringArray(permissionMode) + permissionModes ); if (!validateResult?.result) { return false; @@ -105,20 +96,20 @@ export class WorkspaceSavedObjectsClientWrapper { private validateMultiWorkspacesPermissions = async ( workspacesIds: string[], request: OpenSearchDashboardsRequest, - permissionMode: WorkspacePermissionMode | WorkspacePermissionMode[] + permissionModes: WorkspacePermissionMode[] ) => { // for attributes and options passed in this function, the num of workspaces may be 0.This case should not be passed permission check. if (workspacesIds.length === 0) { return false; } const workspaces = workspacesIds.map((id) => ({ id, type: WORKSPACE_TYPE })); - return await this.validateObjectsPermissions(workspaces, request, permissionMode); + return await this.validateObjectsPermissions(workspaces, request, permissionModes); }; private validateAtLeastOnePermittedWorkspaces = async ( workspaces: string[] | undefined, request: OpenSearchDashboardsRequest, - permissionMode: WorkspacePermissionMode | WorkspacePermissionMode[] + permissionModes: WorkspacePermissionMode[] ) => { // for attributes and options passed in this function, the num of workspaces attribute may be 0.This case should not be passed permission check. if (!workspaces || workspaces.length === 0) { @@ -131,7 +122,7 @@ export class WorkspaceSavedObjectsClientWrapper { type: WORKSPACE_TYPE, id: workspaceId, }, - this.formatWorkspacePermissionModeToStringArray(permissionMode) + permissionModes ); if (validateResult?.result) { return true; @@ -495,12 +486,9 @@ export class WorkspaceSavedObjectsClientWrapper { options.workspaces = permittedWorkspaces; } else { /** - * Select all the docs that - * 1. ACL matches read / write / user passed permission OR - * 2. workspaces matches library_read or library_write OR + * If no workspaces present, find all the docs that + * ACL matches read / write / user passed permission */ - options.workspaces = permittedWorkspaceIds; - options.workspacesSearchOperator = 'OR'; options.ACLSearchParams.permissionModes = getDefaultValuesForEmpty( options.ACLSearchParams.permissionModes, [WorkspacePermissionMode.Read, WorkspacePermissionMode.Write] diff --git a/src/plugins/workspace/server/types.ts b/src/plugins/workspace/server/types.ts index ba1ff8a9cd47..2973ea4dbc31 100644 --- a/src/plugins/workspace/server/types.ts +++ b/src/plugins/workspace/server/types.ts @@ -11,8 +11,12 @@ import { CoreSetup, WorkspaceAttribute, SavedObjectsServiceStart, + Permissions, } from '../../../core/server'; -import { WorkspacePermissionMode } from '../common/constants'; + +export interface WorkspaceAttributeWithPermission extends WorkspaceAttribute { + permissions?: Permissions; +} export interface WorkspaceFindOptions { page?: number; @@ -53,7 +57,7 @@ export interface IWorkspaceClientImpl { */ create( requestDetail: IRequestDetail, - payload: Omit + payload: Omit ): Promise>; /** * List workspaces @@ -91,7 +95,7 @@ export interface IWorkspaceClientImpl { update( requestDetail: IRequestDetail, id: string, - payload: Omit + payload: Omit ): Promise>; /** * Delete a given workspace @@ -124,11 +128,10 @@ export interface AuthInfo { user_name?: string; } -export type WorkspacePermissionItem = { - modes: Array< - | WorkspacePermissionMode.LibraryRead - | WorkspacePermissionMode.LibraryWrite - | WorkspacePermissionMode.Read - | WorkspacePermissionMode.Write - >; -} & ({ type: 'user'; userId: string } | { type: 'group'; group: string }); +export interface WorkspacePluginSetup { + client: IWorkspaceClientImpl; +} + +export interface WorkspacePluginStart { + client: IWorkspaceClientImpl; +} diff --git a/src/plugins/workspace/server/workspace_client.ts b/src/plugins/workspace/server/workspace_client.ts index e7bdf97b54ec..7bdb9f2bcad9 100644 --- a/src/plugins/workspace/server/workspace_client.ts +++ b/src/plugins/workspace/server/workspace_client.ts @@ -4,7 +4,7 @@ */ import { i18n } from '@osd/i18n'; -import type { +import { SavedObject, SavedObjectsClientContract, CoreSetup, @@ -17,14 +17,16 @@ import { WORKSPACE_TYPE, Logger, } from '../../../core/server'; -import { IWorkspaceClientImpl, WorkspaceFindOptions, IResponse, IRequestDetail } from './types'; +import { + IWorkspaceClientImpl, + WorkspaceFindOptions, + IResponse, + IRequestDetail, + WorkspaceAttributeWithPermission, +} from './types'; import { workspace } from './saved_objects'; import { generateRandomId } from './utils'; -import { - WORKSPACE_OVERVIEW_APP_ID, - WORKSPACE_SAVED_OBJECTS_CLIENT_WRAPPER_ID, - WORKSPACE_UPDATE_APP_ID, -} from '../common/constants'; +import { WORKSPACE_SAVED_OBJECTS_CLIENT_WRAPPER_ID } from '../common/constants'; const WORKSPACE_ID_SIZE = 6; @@ -114,10 +116,10 @@ export class WorkspaceClient implements IWorkspaceClientImpl { } public async create( requestDetail: IRequestDetail, - payload: Omit + payload: Omit ): ReturnType { try { - const attributes = payload; + const { permissions, ...attributes } = payload; const id = generateRandomId(WORKSPACE_ID_SIZE); const client = this.getSavedObjectClientsFromRequestDetail(requestDetail); const existingWorkspaceRes = await this.getScopedClientWithoutPermission(requestDetail)?.find( @@ -135,6 +137,7 @@ export class WorkspaceClient implements IWorkspaceClientImpl { attributes, { id, + permissions, } ); return { @@ -233,9 +236,9 @@ export class WorkspaceClient implements IWorkspaceClientImpl { public async update( requestDetail: IRequestDetail, id: string, - payload: Omit + payload: Omit ): Promise> { - const attributes = payload; + const { permissions, ...attributes } = payload; try { const client = this.getSavedObjectClientsFromRequestDetail(requestDetail); const workspaceInDB: SavedObject = await client.get(WORKSPACE_TYPE, id); @@ -255,9 +258,9 @@ export class WorkspaceClient implements IWorkspaceClientImpl { throw new Error(DUPLICATE_WORKSPACE_NAME_ERROR); } } - await client.create>(WORKSPACE_TYPE, attributes, { id, + permissions, overwrite: true, version: workspaceInDB.version, }); From b914e9a24969a009977b8a7504b102923444c62c Mon Sep 17 00:00:00 2001 From: Lin Wang Date: Wed, 3 Apr 2024 18:41:41 +0800 Subject: [PATCH 2/3] Convert permission settings in client side Signed-off-by: Lin Wang --- src/core/types/saved_objects.ts | 2 + src/core/types/workspace.ts | 8 +- .../workspace_creator.test.tsx | 13 +++- .../workspace_creator/workspace_creator.tsx | 8 +- .../public/components/workspace_form/types.ts | 4 +- .../workspace_form/use_workspace_form.ts | 24 +++--- .../public/components/workspace_form/utils.ts | 76 +++++++++++++++++++ .../workspace_form/workspace_form.tsx | 2 +- .../workspace_updater.test.tsx | 9 ++- .../workspace_updater/workspace_updater.tsx | 37 +++++---- .../workspace/public/workspace_client.ts | 23 ++---- src/plugins/workspace/server/routes/index.ts | 3 +- src/plugins/workspace/server/types.ts | 15 ++-- .../workspace/server/workspace_client.ts | 14 ++-- 14 files changed, 170 insertions(+), 68 deletions(-) diff --git a/src/core/types/saved_objects.ts b/src/core/types/saved_objects.ts index b37309338c9e..74890bb624a3 100644 --- a/src/core/types/saved_objects.ts +++ b/src/core/types/saved_objects.ts @@ -126,3 +126,5 @@ export interface SavedObjectError { statusCode: number; metadata?: Record; } + +export type SavedObjectPermissions = Permissions; diff --git a/src/core/types/workspace.ts b/src/core/types/workspace.ts index ffad76fb48a2..7cdc3f92382b 100644 --- a/src/core/types/workspace.ts +++ b/src/core/types/workspace.ts @@ -3,6 +3,8 @@ * SPDX-License-Identifier: Apache-2.0 */ +import { Permissions } from '../server/saved_objects'; + export interface WorkspaceAttribute { id: string; name: string; @@ -14,6 +16,10 @@ export interface WorkspaceAttribute { defaultVISTheme?: string; } -export interface WorkspaceObject extends WorkspaceAttribute { +export interface WorkspaceAttributeWithPermission extends WorkspaceAttribute { + permissions?: Permissions; +} + +export interface WorkspaceObject extends WorkspaceAttributeWithPermission { readonly?: boolean; } diff --git a/src/plugins/workspace/public/components/workspace_creator/workspace_creator.test.tsx b/src/plugins/workspace/public/components/workspace_creator/workspace_creator.test.tsx index f10fd39cfe9d..b67870b55294 100644 --- a/src/plugins/workspace/public/components/workspace_creator/workspace_creator.test.tsx +++ b/src/plugins/workspace/public/components/workspace_creator/workspace_creator.test.tsx @@ -152,7 +152,7 @@ describe('WorkspaceCreator', () => { color: '#000000', description: 'test workspace description', }), - expect.any(Array) + undefined ); await waitFor(() => { expect(notificationToastsAddSuccess).toHaveBeenCalled(); @@ -174,7 +174,7 @@ describe('WorkspaceCreator', () => { name: 'test workspace name', features: expect.arrayContaining(['app1', 'app2', 'app3']), }), - expect.any(Array) + undefined ); await waitFor(() => { expect(notificationToastsAddSuccess).toHaveBeenCalled(); @@ -201,7 +201,14 @@ describe('WorkspaceCreator', () => { expect.objectContaining({ name: 'test workspace name', }), - expect.arrayContaining([expect.objectContaining({ type: 'user', userId: 'test user id' })]) + { + read: { + users: ['test user id'], + }, + library_read: { + users: ['test user id'], + }, + } ); await waitFor(() => { expect(notificationToastsAddSuccess).toHaveBeenCalled(); 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 2b3511f18b8b..4b3e6e57c486 100644 --- a/src/plugins/workspace/public/components/workspace_creator/workspace_creator.tsx +++ b/src/plugins/workspace/public/components/workspace_creator/workspace_creator.tsx @@ -11,6 +11,7 @@ import { WorkspaceForm, WorkspaceFormSubmitData, WorkspaceOperationType } from ' import { WORKSPACE_OVERVIEW_APP_ID } from '../../../common/constants'; import { formatUrlWithWorkspaceId } from '../../../../../core/public/utils'; import { WorkspaceClient } from '../../workspace_client'; +import { convertPermissionSettingsToPermissions } from '../workspace_form/utils'; export const WorkspaceCreator = () => { const { @@ -22,8 +23,11 @@ export const WorkspaceCreator = () => { async (data: WorkspaceFormSubmitData) => { let result; try { - const { permissions, ...attributes } = data; - result = await workspaceClient.create(attributes, permissions); + const { permissionSettings, ...attributes } = data; + result = await workspaceClient.create( + attributes, + convertPermissionSettingsToPermissions(permissionSettings) + ); } catch (error) { notifications?.toasts.addDanger({ title: i18n.translate('workspace.create.failed', { diff --git a/src/plugins/workspace/public/components/workspace_form/types.ts b/src/plugins/workspace/public/components/workspace_form/types.ts index 15af85965943..5f81ba3e6daa 100644 --- a/src/plugins/workspace/public/components/workspace_form/types.ts +++ b/src/plugins/workspace/public/components/workspace_form/types.ts @@ -5,7 +5,7 @@ import type { WorkspacePermissionItemType, WorkspaceOperationType } from './constants'; import type { WorkspacePermissionMode } from '../../../common/constants'; -import type { App, ApplicationStart } from '../../../../../core/public'; +import type { ApplicationStart } from '../../../../../core/public'; export type WorkspacePermissionSetting = | { type: WorkspacePermissionItemType.User; userId: string; modes: WorkspacePermissionMode[] } @@ -18,7 +18,7 @@ export interface WorkspaceFormSubmitData { color?: string; icon?: string; defaultVISTheme?: string; - permissions: WorkspacePermissionSetting[]; + permissionSettings?: WorkspacePermissionSetting[]; } export interface WorkspaceFormData extends WorkspaceFormSubmitData { 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 7158693aedff..00bee7b3ab37 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 @@ -46,8 +46,8 @@ export const useWorkspaceForm = ({ application, defaultValues, onSubmit }: Works const [permissionSettings, setPermissionSettings] = useState< Array> >( - defaultValues?.permissions && defaultValues.permissions.length > 0 - ? defaultValues.permissions + defaultValues?.permissionSettings && defaultValues.permissionSettings.length > 0 + ? defaultValues.permissionSettings : [] ); @@ -58,7 +58,7 @@ export const useWorkspaceForm = ({ application, defaultValues, onSubmit }: Works description, features: selectedFeatureIds, color, - permissions: permissionSettings, + permissionSettings, }); const getFormDataRef = useRef(getFormData); getFormDataRef.current = getFormData; @@ -96,12 +96,15 @@ export const useWorkspaceForm = ({ application, defaultValues, onSubmit }: Works }), }; } - const permissionErrors: string[] = new Array(formData.permissions.length); - for (let i = 0; i < formData.permissions.length; i++) { - const permission = formData.permissions[i]; + const permissionErrors: string[] = new Array(formData.permissionSettings.length); + for (let i = 0; i < formData.permissionSettings.length; i++) { + const permission = formData.permissionSettings[i]; if (isValidWorkspacePermissionSetting(permission)) { if ( - isUserOrGroupPermissionSettingDuplicated(formData.permissions.slice(0, i), permission) + isUserOrGroupPermissionSettingDuplicated( + formData.permissionSettings.slice(0, i), + permission + ) ) { permissionErrors[i] = i18n.translate('workspace.form.permission.invalidate.group', { defaultMessage: 'Duplicate permission setting', @@ -162,8 +165,11 @@ export const useWorkspaceForm = ({ application, defaultValues, onSubmit }: Works formData.features = defaultValues?.features ?? []; } - const permissions = formData.permissions.filter(isValidWorkspacePermissionSetting); - onSubmit?.({ ...formData, name: formData.name!, permissions }); + onSubmit?.({ + ...formData, + name: formData.name!, + permissionSettings: formData.permissionSettings.filter(isValidWorkspacePermissionSetting), + }); }, [defaultFeatures, onSubmit, defaultValues?.features] ); diff --git a/src/plugins/workspace/public/components/workspace_form/utils.ts b/src/plugins/workspace/public/components/workspace_form/utils.ts index 133a3bc563de..8f06581b7ab0 100644 --- a/src/plugins/workspace/public/components/workspace_form/utils.ts +++ b/src/plugins/workspace/public/components/workspace_form/utils.ts @@ -4,6 +4,7 @@ */ import { WorkspacePermissionMode, DEFAULT_CHECKED_FEATURES_IDS } from '../../../common/constants'; +import type { SavedObjectPermissions } from '../../../../../core/types'; import { WorkspaceFeature, @@ -95,3 +96,78 @@ export const getPermissionModeId = (modes: WorkspacePermissionMode[]) => { } return PermissionModeId.Read; }; + +export const convertPermissionSettingsToPermissions = ( + permissionItems: WorkspacePermissionSetting[] | undefined +) => { + if (!permissionItems || permissionItems.length === 0) { + return undefined; + } + return permissionItems.reduce((previous, current) => { + current.modes.forEach((mode) => { + if (!previous[mode]) { + previous[mode] = {}; + } + switch (current.type) { + case 'user': + previous[mode].users = [...(previous[mode].users || []), current.userId]; + break; + case 'group': + previous[mode].groups = [...(previous[mode].groups || []), current.group]; + break; + } + }); + return previous; + }, {}); +}; + +const isWorkspacePermissionMode = (test: string): test is WorkspacePermissionMode => + test === WorkspacePermissionMode.LibraryRead || + test === WorkspacePermissionMode.LibraryWrite || + test === WorkspacePermissionMode.Read || + test === WorkspacePermissionMode.Write; + +export const convertPermissionsToPermissionSettings = (permissions: SavedObjectPermissions) => { + const userPermissionSettings: WorkspacePermissionSetting[] = []; + const groupPermissionSettings: WorkspacePermissionSetting[] = []; + const settingType2Modes: { [key: string]: WorkspacePermissionMode[] } = {}; + + Object.keys(permissions).forEach((mode) => { + if (!isWorkspacePermissionMode(mode)) { + return; + } + if (permissions[mode].users) { + permissions[mode].users?.forEach((userId) => { + const settingTypeKey = `userId-${userId}`; + const modes = settingType2Modes[settingTypeKey] ? settingType2Modes[settingTypeKey] : []; + + modes.push(mode); + if (modes.length === 1) { + userPermissionSettings.push({ + type: WorkspacePermissionItemType.User, + userId, + modes, + }); + settingType2Modes[settingTypeKey] = modes; + } + }); + permissions[mode].groups?.forEach((group) => { + const settingTypeKey = `group-${group}`; + const modes = settingType2Modes[settingTypeKey] ? settingType2Modes[settingTypeKey] : []; + + modes.push(mode); + if (modes.length === 1) { + userPermissionSettings.push({ + type: WorkspacePermissionItemType.Group, + group, + modes, + }); + } + }); + } + }); + + return [...userPermissionSettings, ...groupPermissionSettings].filter( + isValidWorkspacePermissionSetting + ); +}; diff --git a/src/plugins/workspace/public/components/workspace_form/workspace_form.tsx b/src/plugins/workspace/public/components/workspace_form/workspace_form.tsx index ec4f2bfed3e0..b340a71588c9 100644 --- a/src/plugins/workspace/public/components/workspace_form/workspace_form.tsx +++ b/src/plugins/workspace/public/components/workspace_form/workspace_form.tsx @@ -170,7 +170,7 @@ export const WorkspaceForm = (props: WorkspaceFormProps) => { diff --git a/src/plugins/workspace/public/components/workspace_updater/workspace_updater.test.tsx b/src/plugins/workspace/public/components/workspace_updater/workspace_updater.test.tsx index 3c113a71e948..ff07076d99d1 100644 --- a/src/plugins/workspace/public/components/workspace_updater/workspace_updater.test.tsx +++ b/src/plugins/workspace/public/components/workspace_updater/workspace_updater.test.tsx @@ -169,7 +169,14 @@ describe('WorkspaceUpdater', () => { description: 'test workspace description', features: expect.arrayContaining(['app1', 'app2', 'app3']), }), - expect.arrayContaining([expect.objectContaining({ type: 'user', userId: 'test user id' })]) + { + read: { + users: ['test user id'], + }, + library_read: { + users: ['test user id'], + }, + } ); await waitFor(() => { expect(notificationToastsAddSuccess).toHaveBeenCalled(); diff --git a/src/plugins/workspace/public/components/workspace_updater/workspace_updater.tsx b/src/plugins/workspace/public/components/workspace_updater/workspace_updater.tsx index dcc750f18be8..1f67f2063d9b 100644 --- a/src/plugins/workspace/public/components/workspace_updater/workspace_updater.tsx +++ b/src/plugins/workspace/public/components/workspace_updater/workspace_updater.tsx @@ -6,27 +6,30 @@ import React, { useCallback, useEffect, useState } from 'react'; import { EuiPage, EuiPageBody, EuiPageHeader, EuiPageContent } from '@elastic/eui'; import { i18n } from '@osd/i18n'; -import { WorkspaceAttribute } from 'opensearch-dashboards/public'; import { useObservable } from 'react-use'; import { of } from 'rxjs'; import { useOpenSearchDashboards } from '../../../../opensearch_dashboards_react/public'; import { WorkspaceForm, WorkspaceFormSubmitData, WorkspaceOperationType } from '../workspace_form'; import { WORKSPACE_OVERVIEW_APP_ID } from '../../../common/constants'; import { formatUrlWithWorkspaceId } from '../../../../../core/public/utils'; +import { WorkspaceAttributeWithPermission } from '../../../../../core/types'; import { WorkspaceClient } from '../../workspace_client'; -import { WorkspaceFormData, WorkspacePermissionSetting } from '../workspace_form/types'; - -interface WorkspaceWithPermission extends WorkspaceAttribute { - permissions?: WorkspacePermissionSetting[]; -} +import { + convertPermissionSettingsToPermissions, + convertPermissionsToPermissionSettings, +} from '../workspace_form/utils'; function getFormDataFromWorkspace( - currentWorkspace: WorkspaceAttribute | null | undefined -): WorkspaceFormData { - const currentWorkspaceWithPermission = (currentWorkspace || {}) as WorkspaceWithPermission; + currentWorkspace: WorkspaceAttributeWithPermission | null | undefined +) { + if (!currentWorkspace) { + return null; + } return { - ...currentWorkspaceWithPermission, - permissions: currentWorkspaceWithPermission.permissions || [], + ...currentWorkspace, + permissionSettings: currentWorkspace.permissions + ? convertPermissionsToPermissionSettings(currentWorkspace.permissions) + : currentWorkspace.permissions, }; } @@ -38,7 +41,7 @@ export const WorkspaceUpdater = () => { const isPermissionEnabled = application?.capabilities.workspaces.permissionEnabled; const currentWorkspace = useObservable(workspaces ? workspaces.currentWorkspace$ : of(null)); - const [currentWorkspaceFormData, setCurrentWorkspaceFormData] = useState( + const [currentWorkspaceFormData, setCurrentWorkspaceFormData] = useState( getFormDataFromWorkspace(currentWorkspace) ); @@ -59,8 +62,12 @@ export const WorkspaceUpdater = () => { } try { - const { permissions, ...attributes } = data; - result = await workspaceClient.update(currentWorkspace.id, attributes, permissions); + const { permissionSettings, ...attributes } = data; + result = await workspaceClient.update( + currentWorkspace.id, + attributes, + convertPermissionSettingsToPermissions(permissionSettings) + ); } catch (error) { notifications?.toasts.addDanger({ title: i18n.translate('workspace.update.failed', { @@ -100,7 +107,7 @@ export const WorkspaceUpdater = () => { [notifications?.toasts, currentWorkspace, http, application, workspaceClient] ); - if (!currentWorkspaceFormData.name) { + if (!currentWorkspaceFormData) { return null; } diff --git a/src/plugins/workspace/public/workspace_client.ts b/src/plugins/workspace/public/workspace_client.ts index 31a08b6ae9c2..76bbb618b506 100644 --- a/src/plugins/workspace/public/workspace_client.ts +++ b/src/plugins/workspace/public/workspace_client.ts @@ -11,7 +11,7 @@ import { WorkspaceAttribute, WorkspacesSetup, } from '../../../core/public'; -import { WorkspacePermissionMode } from '../common/constants'; +import { SavedObjectPermissions, WorkspaceAttributeWithPermission } from '../../../core/types'; const WORKSPACES_API_BASE_URL = '/api/workspaces'; @@ -31,15 +31,6 @@ type IResponse = error?: string; }; -type WorkspacePermissionItem = { - modes: Array< - | WorkspacePermissionMode.LibraryRead - | WorkspacePermissionMode.LibraryWrite - | WorkspacePermissionMode.Read - | WorkspacePermissionMode.Write - >; -} & ({ type: 'user'; userId: string } | { type: 'group'; group: string }); - interface WorkspaceFindOptions { page?: number; perPage?: number; @@ -195,7 +186,7 @@ export class WorkspaceClient { */ public async create( attributes: Omit, - permissions?: WorkspacePermissionItem[] + permissions?: SavedObjectPermissions ): Promise>> { const path = this.getPath(); @@ -246,7 +237,7 @@ export class WorkspaceClient { options?: WorkspaceFindOptions ): Promise< IResponse<{ - workspaces: WorkspaceAttribute[]; + workspaces: WorkspaceAttributeWithPermission[]; total: number; per_page: number; page: number; @@ -263,9 +254,9 @@ export class WorkspaceClient { * Fetches a single workspace by a workspace id * * @param {string} id - * @returns {Promise>} The metadata of the workspace for the given id. + * @returns {Promise>} The metadata of the workspace for the given id. */ - public get(id: string): Promise> { + public get(id: string): Promise> { const path = this.getPath(id); return this.safeFetch(path, { method: 'GET', @@ -277,13 +268,13 @@ export class WorkspaceClient { * * @param {string} id * @param {object} attributes - * @param {WorkspacePermissionItem[]} permissions + * @param {WorkspacePermissionItem[]} permissionItems * @returns {Promise>} result for this operation */ public async update( id: string, attributes: Partial, - permissions?: WorkspacePermissionItem[] + permissions?: SavedObjectPermissions ): Promise> { const path = this.getPath(id); const body = { diff --git a/src/plugins/workspace/server/routes/index.ts b/src/plugins/workspace/server/routes/index.ts index d48a257bf6dc..701eb8888130 100644 --- a/src/plugins/workspace/server/routes/index.ts +++ b/src/plugins/workspace/server/routes/index.ts @@ -5,8 +5,9 @@ import { schema } from '@osd/config-schema'; import { CoreSetup, Logger, PrincipalType, ACL } from '../../../../core/server'; +import { WorkspaceAttributeWithPermission } from '../../../../core/types'; import { WorkspacePermissionMode } from '../../common/constants'; -import { IWorkspaceClientImpl, WorkspaceAttributeWithPermission } from '../types'; +import { IWorkspaceClientImpl } from '../types'; import { SavedObjectsPermissionControlContract } from '../permission_control/client'; const WORKSPACES_API_BASE_URL = '/api/workspaces'; diff --git a/src/plugins/workspace/server/types.ts b/src/plugins/workspace/server/types.ts index 2973ea4dbc31..b506bb493a4c 100644 --- a/src/plugins/workspace/server/types.ts +++ b/src/plugins/workspace/server/types.ts @@ -11,12 +11,8 @@ import { CoreSetup, WorkspaceAttribute, SavedObjectsServiceStart, - Permissions, } from '../../../core/server'; - -export interface WorkspaceAttributeWithPermission extends WorkspaceAttribute { - permissions?: Permissions; -} +import { WorkspaceAttributeWithPermission } from '../../../core/types'; export interface WorkspaceFindOptions { page?: number; @@ -72,7 +68,7 @@ export interface IWorkspaceClientImpl { ): Promise< IResponse< { - workspaces: WorkspaceAttribute[]; + workspaces: WorkspaceAttributeWithPermission[]; } & Pick > >; @@ -80,10 +76,13 @@ export interface IWorkspaceClientImpl { * Get the detail of a given workspace id * @param requestDetail {@link IRequestDetail} * @param id workspace id - * @returns a Promise with the detail of {@link WorkspaceAttribute} + * @returns a Promise with the detail of {@link WorkspaceAttributeWithPermission} * @public */ - get(requestDetail: IRequestDetail, id: string): Promise>; + get( + requestDetail: IRequestDetail, + id: string + ): Promise>; /** * Update the detail of a given workspace * @param requestDetail {@link IRequestDetail} diff --git a/src/plugins/workspace/server/workspace_client.ts b/src/plugins/workspace/server/workspace_client.ts index 7bdb9f2bcad9..f9da4130921b 100644 --- a/src/plugins/workspace/server/workspace_client.ts +++ b/src/plugins/workspace/server/workspace_client.ts @@ -17,13 +17,8 @@ import { WORKSPACE_TYPE, Logger, } from '../../../core/server'; -import { - IWorkspaceClientImpl, - WorkspaceFindOptions, - IResponse, - IRequestDetail, - WorkspaceAttributeWithPermission, -} from './types'; +import { WorkspaceAttributeWithPermission } from '../../../core/types'; +import { IWorkspaceClientImpl, WorkspaceFindOptions, IResponse, IRequestDetail } from './types'; import { workspace } from './saved_objects'; import { generateRandomId } from './utils'; import { WORKSPACE_SAVED_OBJECTS_CLIENT_WRAPPER_ID } from '../common/constants'; @@ -67,10 +62,11 @@ export class WorkspaceClient implements IWorkspaceClientImpl { } private getFlattenedResultWithSavedObject( savedObject: SavedObject - ): WorkspaceAttribute { + ): WorkspaceAttributeWithPermission { return { ...savedObject.attributes, id: savedObject.id, + permissions: savedObject.permissions, }; } private formatError(error: Error | any): string { @@ -217,7 +213,7 @@ export class WorkspaceClient implements IWorkspaceClientImpl { public async get( requestDetail: IRequestDetail, id: string - ): Promise> { + ): ReturnType { try { const result = await this.getSavedObjectClientsFromRequestDetail(requestDetail).get< WorkspaceAttribute From 79b7271e0724f0b2ab936eae947b10779a90a142 Mon Sep 17 00:00:00 2001 From: Lin Wang Date: Wed, 3 Apr 2024 23:12:20 +0800 Subject: [PATCH 3/3] Fix workspace list always render Signed-off-by: Lin Wang --- .../workspace/public/components/workspace_list/index.tsx | 9 +++++++-- src/plugins/workspace/public/hooks.ts | 4 +++- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/src/plugins/workspace/public/components/workspace_list/index.tsx b/src/plugins/workspace/public/components/workspace_list/index.tsx index aac721c236a6..f529524d797e 100644 --- a/src/plugins/workspace/public/components/workspace_list/index.tsx +++ b/src/plugins/workspace/public/components/workspace_list/index.tsx @@ -17,7 +17,7 @@ import { import useObservable from 'react-use/lib/useObservable'; import { of } from 'rxjs'; import { i18n } from '@osd/i18n'; -import { debounce } from '../../../../../core/public'; +import { debounce, WorkspaceObject } from '../../../../../core/public'; import { WorkspaceAttribute } from '../../../../../core/public'; import { useOpenSearchDashboards } from '../../../../../plugins/opensearch_dashboards_react/public'; import { switchWorkspace, updateWorkspace } from '../utils/workspace'; @@ -34,6 +34,8 @@ const WORKSPACE_LIST_PAGE_DESCRIPTIOIN = i18n.translate('workspace.list.descript 'Workspace allow you to save and organize library items, such as index patterns, visualizations, dashboards, saved searches, and share them with other OpenSearch Dashboards users. You can control which features are visible in each workspace, and which users and groups have read and write access to the library items in the workspace.', }); +const emptyWorkspaceList: WorkspaceObject[] = []; + export const WorkspaceList = () => { const { services: { workspaces, application, http }, @@ -41,7 +43,10 @@ export const WorkspaceList = () => { const initialSortField = 'name'; const initialSortDirection = 'asc'; - const workspaceList = useObservable(workspaces?.workspaceList$ ?? of([]), []); + const workspaceList = useObservable( + workspaces?.workspaceList$ ?? of(emptyWorkspaceList), + emptyWorkspaceList + ); const [queryInput, setQueryInput] = useState(''); const [pagination, setPagination] = useState({ pageIndex: 0, diff --git a/src/plugins/workspace/public/hooks.ts b/src/plugins/workspace/public/hooks.ts index a63dc8f83d3d..4309dab2e5b0 100644 --- a/src/plugins/workspace/public/hooks.ts +++ b/src/plugins/workspace/public/hooks.ts @@ -8,8 +8,10 @@ import { useMemo } from 'react'; import { of } from 'rxjs'; import { ApplicationStart, PublicAppInfo } from '../../../core/public'; +const emptyMap = new Map(); + export function useApplications(application?: ApplicationStart) { - const applications = useObservable(application?.applications$ ?? of(new Map()), new Map()); + const applications = useObservable(application?.applications$ ?? of(emptyMap), emptyMap); return useMemo(() => { const apps: PublicAppInfo[] = []; applications.forEach((app) => {