diff --git a/changelogs/fragments/7125.yml b/changelogs/fragments/7125.yml new file mode 100644 index 000000000000..21250e159e89 --- /dev/null +++ b/changelogs/fragments/7125.yml @@ -0,0 +1,2 @@ +fix: +- [Workspace]Restrict saved objects finding when workspace enabled ([#7125](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/7125)) \ No newline at end of file diff --git a/src/core/server/saved_objects/service/lib/search_dsl/query_params.test.ts b/src/core/server/saved_objects/service/lib/search_dsl/query_params.test.ts index 5af816a1d8f5..762a8a3c5e94 100644 --- a/src/core/server/saved_objects/service/lib/search_dsl/query_params.test.ts +++ b/src/core/server/saved_objects/service/lib/search_dsl/query_params.test.ts @@ -656,7 +656,7 @@ describe('#getQueryParams', () => { expect(result.query.bool.filter[1]).toEqual(undefined); }); - it('workspacesSearchOperator prvided as "OR"', () => { + it('workspacesSearchOperator provided as "OR"', () => { const result: Result = getQueryParams({ registry, workspaces: ['foo'], @@ -665,22 +665,6 @@ describe('#getQueryParams', () => { expect(result.query.bool.filter[1]).toEqual({ bool: { should: [ - { - bool: { - must_not: [ - { - exists: { - field: 'workspaces', - }, - }, - { - exists: { - field: 'permissions', - }, - }, - ], - }, - }, { bool: { minimum_should_match: 1, @@ -718,22 +702,6 @@ describe('#getQueryParams', () => { expect(result.query.bool.filter[1]).toEqual({ bool: { should: [ - { - bool: { - must_not: [ - { - exists: { - field: 'workspaces', - }, - }, - { - exists: { - field: 'permissions', - }, - }, - ], - }, - }, { bool: { filter: [ @@ -771,6 +739,36 @@ describe('#getQueryParams', () => { }); }); }); + + describe('when using empty workspace search', () => { + it('workspacesSearchOperator provided as "OR"', () => { + const result: Result = getQueryParams({ + registry, + workspaces: [], + workspacesSearchOperator: 'OR', + }); + expect(result.query.bool.filter[1]).toEqual({ + bool: { + should: [ + { + match_none: {}, + }, + ], + }, + }); + }); + + it('workspacesSearchOperator provided as "AND"', () => { + const result: Result = getQueryParams({ + registry, + workspaces: [], + workspacesSearchOperator: 'AND', + }); + expect(result.query.bool.filter[1]).toEqual({ + match_none: {}, + }); + }); + }); }); describe('namespaces property', () => { diff --git a/src/core/server/saved_objects/service/lib/search_dsl/query_params.ts b/src/core/server/saved_objects/service/lib/search_dsl/query_params.ts index 318768fd83c2..ba6e62d877b6 100644 --- a/src/core/server/saved_objects/service/lib/search_dsl/query_params.ts +++ b/src/core/server/saved_objects/service/lib/search_dsl/query_params.ts @@ -286,53 +286,32 @@ export function getQueryParams({ } } - if (workspaces?.length) { - if (workspacesSearchOperator === 'OR') { - ACLSearchParamsShouldClause.push({ - bool: { - should: workspaces.map((workspace) => { - return getClauseForWorkspace(workspace); - }), - minimum_should_match: 1, - }, - }); - } else { - bool.filter.push({ - bool: { - should: workspaces.map((workspace) => { - return getClauseForWorkspace(workspace); - }), - minimum_should_match: 1, - }, - }); + if (workspaces) { + const conditions = + workspacesSearchOperator === 'OR' ? ACLSearchParamsShouldClause : bool.filter; + + switch (workspaces.length) { + case 0: + conditions.push({ + match_none: {}, + }); + break; + default: + conditions.push({ + bool: { + should: workspaces.map((workspace) => { + return getClauseForWorkspace(workspace); + }), + minimum_should_match: 1, + }, + }); } } if (ACLSearchParamsShouldClause.length) { bool.filter.push({ bool: { - should: [ - /** - * Return those objects without workspaces field and permissions field to keep find API backward compatible - */ - { - bool: { - must_not: [ - { - exists: { - field: 'workspaces', - }, - }, - { - exists: { - field: 'permissions', - }, - }, - ], - }, - }, - ...ACLSearchParamsShouldClause, - ], + should: ACLSearchParamsShouldClause, }, }); } 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 7967992690d0..9405c59a4796 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 @@ -381,10 +381,10 @@ describe('saved_objects_wrapper_for_check_workspace_conflict integration test', ], }); - const findAdvancedSettings = await osdTestServer.request - .get(root, `/api/saved_objects/_find?type=${advancedSettings.type}`) + const getAdvancedSettingsResult = await osdTestServer.request + .get(root, `/api/saved_objects/${advancedSettings.type}/${packageInfo.version}`) .expect(200); - expect(findAdvancedSettings.body.total).toEqual(1); + expect(getAdvancedSettingsResult.body.id).toBe(packageInfo.version); }); it('checkConflicts when importing ndjson', async () => { 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 b5399de9fb5d..6aeb2ebc8610 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 @@ -241,20 +241,20 @@ describe('WorkspaceSavedObjectsClientWrapper', () => { }); describe('find', () => { - it('should throw not authorized error when user not permitted', async () => { - let error; - try { - await notPermittedSavedObjectedClient.find({ - type: 'dashboard', - workspaces: ['workspace-1'], - perPage: 999, - page: 1, - }); - } catch (e) { - error = e; - } + it('should return empty result if user not permitted', async () => { + const result = await notPermittedSavedObjectedClient.find({ + type: 'dashboard', + workspaces: ['workspace-1'], + perPage: 999, + page: 1, + }); - expect(SavedObjectsErrorHelpers.isNotAuthorizedError(error)).toBe(true); + expect(result).toEqual({ + saved_objects: [], + total: 0, + page: 1, + per_page: 999, + }); }); it('should return consistent inner workspace data when user permitted', async () => { @@ -269,6 +269,36 @@ describe('WorkspaceSavedObjectsClientWrapper', () => { true ); }); + + it('should return consistent result when workspaces and ACLSearchParams not provided', async () => { + const result = await permittedSavedObjectedClient.find({ + type: 'dashboard', + perPage: 999, + page: 1, + }); + + expect(result.saved_objects).toEqual( + expect.arrayContaining([ + expect.objectContaining({ id: 'inner-workspace-dashboard-1' }), + expect.objectContaining({ id: 'acl-controlled-dashboard-2' }), + ]) + ); + }); + + it('should return acl controled dashboards when only ACLSearchParams provided', async () => { + const result = await permittedSavedObjectedClient.find({ + type: 'dashboard', + perPage: 999, + page: 1, + ACLSearchParams: { + permissionModes: ['read', 'write'], + }, + }); + + expect(result.saved_objects).toEqual( + expect.arrayContaining([expect.objectContaining({ id: 'acl-controlled-dashboard-2' })]) + ); + }); }); describe('create', () => { 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 79e1b25cd823..2190e0fcec27 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 @@ -7,6 +7,7 @@ import { getWorkspaceState, updateWorkspaceState } from '../../../../core/server import { SavedObjectsErrorHelpers } from '../../../../core/server'; import { WorkspaceSavedObjectsClientWrapper } from './workspace_saved_objects_client_wrapper'; import { httpServerMock } from '../../../../core/server/mocks'; +import { DATA_SOURCE_SAVED_OBJECT_TYPE } from '../../../data_source/common'; const DASHBOARD_ADMIN = 'dashnoard_admin'; const NO_DASHBOARD_ADMIN = 'no_dashnoard_admin'; @@ -46,6 +47,17 @@ const generateWorkspaceSavedObjectsClientWrapper = (role = NO_DASHBOARD_ADMIN) = id: 'not-permitted-workspace', attributes: { name: 'Not permitted workspace' }, }, + { + type: DATA_SOURCE_SAVED_OBJECT_TYPE, + id: 'global-data-source', + attributes: { title: 'Global data source' }, + }, + { + type: DATA_SOURCE_SAVED_OBJECT_TYPE, + id: 'workspace-1-data-source', + attributes: { title: 'Workspace 1 data source' }, + workspaces: ['workspace-1'], + }, ]; const clientMock = { get: jest.fn().mockImplementation(async (type, id) => { @@ -77,7 +89,17 @@ const generateWorkspaceSavedObjectsClientWrapper = (role = NO_DASHBOARD_ADMIN) = ), }; }), - find: jest.fn(), + find: jest.fn().mockImplementation(({ type, workspaces }) => { + const savedObjects = savedObjectsStore.filter( + (item) => + item.type === type && + (!workspaces || item.workspaces?.some((workspaceId) => workspaces.includes(workspaceId))) + ); + return { + saved_objects: savedObjects, + total: savedObjects.length, + }; + }), deleteByWorkspace: jest.fn(), }; const requestMock = httpServerMock.createOpenSearchDashboardsRequest(); @@ -602,7 +624,7 @@ describe('WorkspaceSavedObjectsClientWrapper', () => { }); }); describe('find', () => { - it('should call client.find with ACLSearchParams for workspace type', async () => { + it('should call client.find with consistent params when ACLSearchParams and workspaceOperator not provided', async () => { const { wrapper, clientMock } = generateWorkspaceSavedObjectsClientWrapper(); await wrapper.find({ type: 'workspace', @@ -615,58 +637,11 @@ describe('WorkspaceSavedObjectsClientWrapper', () => { }, permissionModes: ['read', 'write'], }, + workspaces: ['workspace-1'], + workspacesSearchOperator: 'OR', }); }); - it('should call client.find with ACLSearchParams if no workspaces is provided', async () => { - const { wrapper, clientMock } = generateWorkspaceSavedObjectsClientWrapper(); - // no workspaces - await wrapper.find({ - type: 'dashboards', - }); - expect(clientMock.find).toHaveBeenCalledWith({ - type: 'dashboards', - ACLSearchParams: { - principals: { - users: ['user-1'], - }, - permissionModes: ['read', 'write'], - }, - }); - - // workspaces parameter is undefined - clientMock.find.mockReset(); - await wrapper.find({ - type: 'dashboards', - workspaces: undefined, - }); - expect(clientMock.find).toHaveBeenLastCalledWith({ - type: 'dashboards', - ACLSearchParams: { - principals: { - users: ['user-1'], - }, - permissionModes: ['read', 'write'], - }, - }); - - // empty workspaces array - clientMock.find.mockReset(); - await wrapper.find({ - type: 'dashboards', - workspaces: [], - }); - expect(clientMock.find).toHaveBeenLastCalledWith({ - type: 'dashboards', - workspaces: [], - ACLSearchParams: { - principals: { - users: ['user-1'], - }, - permissionModes: ['read', 'write'], - }, - }); - }); - it('should call client.find with only read permission if find workspace and permissionModes provided', async () => { + it('should call client.find with ACLSearchParams when only ACLSearchParams provided', async () => { const { wrapper, clientMock } = generateWorkspaceSavedObjectsClientWrapper(); await wrapper.find({ type: 'workspace', @@ -684,60 +659,15 @@ describe('WorkspaceSavedObjectsClientWrapper', () => { }, }); }); - it('should throw workspace permission error if provided workspaces not permitted', async () => { - const { wrapper } = generateWorkspaceSavedObjectsClientWrapper(); - let errorCatched; - try { - errorCatched = await wrapper.find({ - type: 'dashboard', - workspaces: ['not-permitted-workspace'], - }); - } catch (e) { - errorCatched = e; - } - expect(errorCatched?.message).toEqual('Invalid workspace permission'); - }); - it('should remove not permitted workspace and call client.find with arguments', async () => { + it('should call client.find with filtered workspaces when only workspaces provided', async () => { const { wrapper, clientMock } = generateWorkspaceSavedObjectsClientWrapper(); await wrapper.find({ type: 'dashboard', - workspaces: ['not-permitted-workspace', 'workspace-1'], + workspaces: ['workspace-1', 'workspace-2'], }); expect(clientMock.find).toHaveBeenCalledWith({ type: 'dashboard', workspaces: ['workspace-1'], - 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({ - type: 'dashboard', - }); - expect(clientMock.find).toHaveBeenCalledWith({ - type: 'dashboard', - 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 06701fba338a..9a156aa6501f 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 @@ -62,16 +62,6 @@ const generateOSDAdminPermissionError = () => ) ); -const intersection = (...args: T[][]) => { - const occursCountMap: { [key: string]: number } = {}; - for (let i = 0; i < args.length; i++) { - new Set(args[i]).forEach((key) => { - occursCountMap[key] = (occursCountMap[key] || 0) + 1; - }); - } - return Object.keys(occursCountMap).filter((key) => occursCountMap[key] === args.length); -}; - const getDefaultValuesForEmpty = (values: T[] | undefined, defaultValues: T[]) => { return !values || values.length === 0 ? defaultValues : values; }; @@ -141,16 +131,6 @@ export class WorkspaceSavedObjectsClientWrapper { return false; }; - /** - * check if the type include workspace - * Workspace permission check is totally different from object permission check. - * @param type - * @returns - */ - private isRelatedToWorkspace(type: string | string[]): boolean { - return type === WORKSPACE_TYPE || (Array.isArray(type) && type.includes(WORKSPACE_TYPE)); - } - private async validateWorkspacesAndSavedObjectsPermissions( savedObject: Pick, request: OpenSearchDashboardsRequest, @@ -439,92 +419,47 @@ export class WorkspaceSavedObjectsClientWrapper { options: SavedObjectsFindOptions ) => { const principals = this.permissionControl.getPrincipalsFromRequest(wrapperOptions.request); - if (!options.ACLSearchParams) { - options.ACLSearchParams = {}; - } - - if (this.isRelatedToWorkspace(options.type)) { - /** - * - * This case is for finding workspace saved objects, will use passed permissionModes - * and override passed principals from request to get all readable workspaces. - * - */ - options.ACLSearchParams.permissionModes = getDefaultValuesForEmpty( - options.ACLSearchParams.permissionModes, - [WorkspacePermissionMode.Read, WorkspacePermissionMode.Write] - ); - options.ACLSearchParams.principals = principals; + const permittedWorkspaceIds = ( + await this.getWorkspaceTypeEnabledClient(wrapperOptions.request).find({ + type: WORKSPACE_TYPE, + perPage: 999, + ACLSearchParams: { + principals, + permissionModes: [ + WorkspacePermissionMode.LibraryRead, + WorkspacePermissionMode.LibraryWrite, + ], + }, + // By declaring workspaces as null, + // workspaces won't be appended automatically into the options. + // or workspaces can not be found because workspace object do not have `workspaces` field. + workspaces: null, + }) + ).saved_objects.map((item) => item.id); + + if (!options.workspaces && !options.ACLSearchParams) { + options.workspaces = permittedWorkspaceIds; + options.ACLSearchParams = { + permissionModes: [WorkspacePermissionMode.Read, WorkspacePermissionMode.Write], + principals, + }; + options.workspacesSearchOperator = 'OR'; } else { - /** - * Workspace is a hidden type so that we need to - * initialize a new saved objects client with workspace enabled to retrieve all the workspaces with permission. - */ - const permittedWorkspaceIds = ( - await this.getWorkspaceTypeEnabledClient(wrapperOptions.request).find({ - type: WORKSPACE_TYPE, - perPage: 999, - ACLSearchParams: { - principals, - /** - * The permitted workspace ids will be passed to the options.workspaces - * or options.ACLSearchParams.workspaces. These two were indicated the saved - * objects data inner specific workspaces. We use Library related permission here. - * For outside passed permission modes, it may contains other permissions. Add a intersection - * here to make sure only Library related permission modes will be used. - */ - permissionModes: getDefaultValuesForEmpty( - options.ACLSearchParams.permissionModes - ? intersection(options.ACLSearchParams.permissionModes, [ - WorkspacePermissionMode.LibraryRead, - WorkspacePermissionMode.LibraryWrite, - ]) - : [], - [WorkspacePermissionMode.LibraryRead, WorkspacePermissionMode.LibraryWrite] - ), - }, - // By declaring workspaces as null, - // workspaces won't be appended automatically into the options. - // or workspaces can not be found because workspace object do not have `workspaces` field. - workspaces: null, - }) - ).saved_objects.map((item) => item.id); - - if (options.workspaces && options.workspaces.length > 0) { - const permittedWorkspaces = options.workspaces.filter((item) => - permittedWorkspaceIds.includes(item) - ); - if (!permittedWorkspaces.length) { - /** - * If user does not have any one workspace access - * deny the request - */ - throw SavedObjectsErrorHelpers.decorateNotAuthorizedError( - new Error( - i18n.translate('workspace.permission.invalidate', { - defaultMessage: 'Invalid workspace permission', - }) - ) - ); - } - - /** - * Overwrite the options.workspaces when user has access on partial workspaces. - */ - options.workspaces = permittedWorkspaces; - } else { - /** - * If no workspaces present, find all the docs that - * ACL matches read / write / user passed permission - */ - options.ACLSearchParams.permissionModes = getDefaultValuesForEmpty( - options.ACLSearchParams.permissionModes, - [WorkspacePermissionMode.Read, WorkspacePermissionMode.Write] + if (options.workspaces) { + options.workspaces = options.workspaces.filter((workspaceId) => + permittedWorkspaceIds.includes(workspaceId) ); - options.ACLSearchParams.principals = principals; + } + if (options.ACLSearchParams) { + options.ACLSearchParams = { + permissionModes: getDefaultValuesForEmpty(options.ACLSearchParams.permissionModes, [ + WorkspacePermissionMode.Read, + WorkspacePermissionMode.Write, + ]), + principals, + }; } } - return await wrapperOptions.client.find(options); };