From 0b30d7266763b57b7379da23e7498eda62071fff Mon Sep 17 00:00:00 2001 From: Joe Portner <5295965+jportner@users.noreply.github.com> Date: Tue, 24 Aug 2021 15:53:47 -0400 Subject: [PATCH 1/8] Refactor SpacesSavedObjectsClient unit tests --- .../spaces_saved_objects_client.test.ts | 46 +++++-------------- 1 file changed, 12 insertions(+), 34 deletions(-) diff --git a/x-pack/plugins/spaces/server/saved_objects/spaces_saved_objects_client.test.ts b/x-pack/plugins/spaces/server/saved_objects/spaces_saved_objects_client.test.ts index b94113436d7ad..9693bd9d56287 100644 --- a/x-pack/plugins/spaces/server/saved_objects/spaces_saved_objects_client.test.ts +++ b/x-pack/plugins/spaces/server/saved_objects/spaces_saved_objects_client.test.ts @@ -11,7 +11,6 @@ import { SavedObjectTypeRegistry } from 'src/core/server'; import { savedObjectsClientMock } from 'src/core/server/mocks'; import { DEFAULT_SPACE_ID } from '../../common/constants'; -import type { SpacesClient } from '../spaces_client'; import { spacesClientMock } from '../spaces_client/spaces_client.mock'; import { spacesServiceMock } from '../spaces_service/spaces_service.mock'; import { SpacesSavedObjectsClient } from './spaces_saved_objects_client'; @@ -68,6 +67,8 @@ const ERROR_NAMESPACE_SPECIFIED = 'Spaces currently determines the namespaces'; const request = createMockRequest(); const baseClient = createMockClient(); const spacesService = createSpacesService(currentSpace.id); + const spacesClient = spacesClientMock.create(); + spacesService.createSpacesClient.mockReturnValue(spacesClient); const client = new SpacesSavedObjectsClient({ request, @@ -75,7 +76,7 @@ const ERROR_NAMESPACE_SPECIFIED = 'Spaces currently determines the namespaces'; getSpacesService: () => spacesService, typeRegistry, }); - return { client, baseClient, spacesService }; + return { client, baseClient, spacesClient }; }; describe('#get', () => { @@ -168,10 +169,8 @@ const ERROR_NAMESPACE_SPECIFIED = 'Spaces currently determines the namespaces'; const EMPTY_RESPONSE = { saved_objects: [], total: 0, per_page: 20, page: 1 }; test(`returns empty result if user is unauthorized in this space`, async () => { - const { client, baseClient, spacesService } = createSpacesSavedObjectsClient(); - const spacesClient = spacesClientMock.create(); + const { client, baseClient, spacesClient } = createSpacesSavedObjectsClient(); spacesClient.getAll.mockResolvedValue([]); - spacesService.createSpacesClient.mockReturnValue(spacesClient); const options = Object.freeze({ type: 'foo', namespaces: ['some-ns'] }); const actualReturnValue = await client.find(options); @@ -181,10 +180,8 @@ const ERROR_NAMESPACE_SPECIFIED = 'Spaces currently determines the namespaces'; }); test(`returns empty result if user is unauthorized in any space`, async () => { - const { client, baseClient, spacesService } = createSpacesSavedObjectsClient(); - const spacesClient = spacesClientMock.create(); + const { client, baseClient, spacesClient } = createSpacesSavedObjectsClient(); spacesClient.getAll.mockRejectedValue(Boom.unauthorized()); - spacesService.createSpacesClient.mockReturnValue(spacesClient); const options = Object.freeze({ type: 'foo', namespaces: ['some-ns'] }); const actualReturnValue = await client.find(options); @@ -234,7 +231,7 @@ const ERROR_NAMESPACE_SPECIFIED = 'Spaces currently determines the namespaces'; }); test(`passes options.namespaces along`, async () => { - const { client, baseClient, spacesService } = createSpacesSavedObjectsClient(); + const { client, baseClient, spacesClient } = createSpacesSavedObjectsClient(); const expectedReturnValue = { saved_objects: [createMockResponse()], total: 1, @@ -243,9 +240,6 @@ const ERROR_NAMESPACE_SPECIFIED = 'Spaces currently determines the namespaces'; }; baseClient.find.mockReturnValue(Promise.resolve(expectedReturnValue)); - const spacesClient = spacesService.createSpacesClient( - null as any - ) as jest.Mocked; spacesClient.getAll.mockImplementation(() => Promise.resolve([ { id: 'ns-1', name: '', disabledFeatures: [] }, @@ -265,7 +259,7 @@ const ERROR_NAMESPACE_SPECIFIED = 'Spaces currently determines the namespaces'; }); test(`filters options.namespaces based on authorization`, async () => { - const { client, baseClient, spacesService } = createSpacesSavedObjectsClient(); + const { client, baseClient, spacesClient } = createSpacesSavedObjectsClient(); const expectedReturnValue = { saved_objects: [createMockResponse()], total: 1, @@ -274,9 +268,6 @@ const ERROR_NAMESPACE_SPECIFIED = 'Spaces currently determines the namespaces'; }; baseClient.find.mockReturnValue(Promise.resolve(expectedReturnValue)); - const spacesClient = spacesService.createSpacesClient( - null as any - ) as jest.Mocked; spacesClient.getAll.mockImplementation(() => Promise.resolve([ { id: 'ns-1', name: '', disabledFeatures: [] }, @@ -296,7 +287,7 @@ const ERROR_NAMESPACE_SPECIFIED = 'Spaces currently determines the namespaces'; }); test(`translates options.namespace: ['*']`, async () => { - const { client, baseClient, spacesService } = createSpacesSavedObjectsClient(); + const { client, baseClient, spacesClient } = createSpacesSavedObjectsClient(); const expectedReturnValue = { saved_objects: [createMockResponse()], total: 1, @@ -305,9 +296,6 @@ const ERROR_NAMESPACE_SPECIFIED = 'Spaces currently determines the namespaces'; }; baseClient.find.mockReturnValue(Promise.resolve(expectedReturnValue)); - const spacesClient = spacesService.createSpacesClient( - null as any - ) as jest.Mocked; spacesClient.getAll.mockImplementation(() => Promise.resolve([ { id: 'ns-1', name: '', disabledFeatures: [] }, @@ -534,10 +522,8 @@ const ERROR_NAMESPACE_SPECIFIED = 'Spaces currently determines the namespaces'; describe('#openPointInTimeForType', () => { test(`throws error if if user is unauthorized in this space`, async () => { - const { client, baseClient, spacesService } = createSpacesSavedObjectsClient(); - const spacesClient = spacesClientMock.create(); + const { client, baseClient, spacesClient } = createSpacesSavedObjectsClient(); spacesClient.getAll.mockResolvedValue([]); - spacesService.createSpacesClient.mockReturnValue(spacesClient); await expect( client.openPointInTimeForType('foo', { namespaces: ['bar'] }) @@ -547,10 +533,8 @@ const ERROR_NAMESPACE_SPECIFIED = 'Spaces currently determines the namespaces'; }); test(`throws error if if user is unauthorized in any space`, async () => { - const { client, baseClient, spacesService } = createSpacesSavedObjectsClient(); - const spacesClient = spacesClientMock.create(); + const { client, baseClient, spacesClient } = createSpacesSavedObjectsClient(); spacesClient.getAll.mockRejectedValue(Boom.unauthorized()); - spacesService.createSpacesClient.mockReturnValue(spacesClient); await expect( client.openPointInTimeForType('foo', { namespaces: ['bar'] }) @@ -560,13 +544,10 @@ const ERROR_NAMESPACE_SPECIFIED = 'Spaces currently determines the namespaces'; }); test(`filters options.namespaces based on authorization`, async () => { - const { client, baseClient, spacesService } = createSpacesSavedObjectsClient(); + const { client, baseClient, spacesClient } = createSpacesSavedObjectsClient(); const expectedReturnValue = { id: 'abc123' }; baseClient.openPointInTimeForType.mockReturnValue(Promise.resolve(expectedReturnValue)); - const spacesClient = spacesService.createSpacesClient( - null as any - ) as jest.Mocked; spacesClient.getAll.mockImplementation(() => Promise.resolve([ { id: 'ns-1', name: '', disabledFeatures: [] }, @@ -585,13 +566,10 @@ const ERROR_NAMESPACE_SPECIFIED = 'Spaces currently determines the namespaces'; }); test(`translates options.namespaces: ['*']`, async () => { - const { client, baseClient, spacesService } = createSpacesSavedObjectsClient(); + const { client, baseClient, spacesClient } = createSpacesSavedObjectsClient(); const expectedReturnValue = { id: 'abc123' }; baseClient.openPointInTimeForType.mockReturnValue(Promise.resolve(expectedReturnValue)); - const spacesClient = spacesService.createSpacesClient( - null as any - ) as jest.Mocked; spacesClient.getAll.mockImplementation(() => Promise.resolve([ { id: 'ns-1', name: '', disabledFeatures: [] }, From e6588b79d59ba9d41d1b212e3a30b3deccc851e8 Mon Sep 17 00:00:00 2001 From: Joe Portner <5295965+jportner@users.noreply.github.com> Date: Tue, 24 Aug 2021 16:01:01 -0400 Subject: [PATCH 2/8] Fix broken unit tests These unit tests were not using mocks correctly, so they passed when they should not have: * '#find returns empty result if user is unauthorized in any space' * '#openPointInTimeForType throws error if if user is unauthorized in any space' The previous refactor fixed the mocks, which caused these two tests to start failing. In this commit, I have fixed the code so the tests pass as written. --- .../saved_objects/spaces_saved_objects_client.ts | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/x-pack/plugins/spaces/server/saved_objects/spaces_saved_objects_client.ts b/x-pack/plugins/spaces/server/saved_objects/spaces_saved_objects_client.ts index 9c51f22e280d8..3be1bb3cad10c 100644 --- a/x-pack/plugins/spaces/server/saved_objects/spaces_saved_objects_client.ts +++ b/x-pack/plugins/spaces/server/saved_objects/spaces_saved_objects_client.ts @@ -179,7 +179,7 @@ export class SpacesSavedObjectsClient implements SavedObjectsClientContract { try { namespaces = await this.getSearchableSpaces(options.namespaces); } catch (err) { - if (Boom.isBoom(err) && err.output.payload.statusCode === 403) { + if (Boom.isBoom(err) && err.output.payload.statusCode === 401) { // return empty response, since the user is unauthorized in any space, but we don't return forbidden errors for `find` operations return SavedObjectsUtils.createEmptyFindResponse(options); } @@ -383,7 +383,16 @@ export class SpacesSavedObjectsClient implements SavedObjectsClientContract { type: string | string[], options: SavedObjectsOpenPointInTimeOptions = {} ) { - const namespaces = await this.getSearchableSpaces(options.namespaces); + let namespaces: string[]; + try { + namespaces = await this.getSearchableSpaces(options.namespaces); + } catch (err) { + if (Boom.isBoom(err) && err.output.payload.statusCode === 401) { + // throw bad request since the user is unauthorized in any space + throw SavedObjectsErrorHelpers.createBadRequestError(); + } + throw err; + } if (namespaces.length === 0) { // throw bad request if no valid spaces were found. throw SavedObjectsErrorHelpers.createBadRequestError(); From 9f6efb65a2cfb79d4a87e01919e2f3c07461bdc5 Mon Sep 17 00:00:00 2001 From: Joe Portner <5295965+jportner@users.noreply.github.com> Date: Tue, 24 Aug 2021 17:26:03 -0400 Subject: [PATCH 3/8] Add per-object namespaces field for bulkGet --- docs/api/saved-objects/bulk_get.asciidoc | 10 ++ ...n-core-server.savedobjectsbulkgetobject.md | 1 + ...er.savedobjectsbulkgetobject.namespaces.md | 15 +++ .../server/saved_objects/routes/bulk_get.ts | 1 + .../service/lib/internal_utils.test.ts | 84 +++++++++++++++++ .../service/lib/internal_utils.ts | 37 ++++++++ .../service/lib/repository.test.js | 93 +++++++++++++------ .../saved_objects/service/lib/repository.ts | 63 +++++++++++-- .../service/saved_objects_client.ts | 11 +++ src/core/server/server.api.md | 1 + ...ecure_saved_objects_client_wrapper.test.ts | 13 ++- .../secure_saved_objects_client_wrapper.ts | 6 +- .../spaces_saved_objects_client.test.ts | 28 +++++- .../spaces_saved_objects_client.ts | 20 +++- 14 files changed, 341 insertions(+), 42 deletions(-) create mode 100644 docs/development/core/server/kibana-plugin-core-server.savedobjectsbulkgetobject.namespaces.md diff --git a/docs/api/saved-objects/bulk_get.asciidoc b/docs/api/saved-objects/bulk_get.asciidoc index e31bbf145218d..4c6bf4c19a76c 100644 --- a/docs/api/saved-objects/bulk_get.asciidoc +++ b/docs/api/saved-objects/bulk_get.asciidoc @@ -31,6 +31,16 @@ experimental[] Retrieve multiple {kib} saved objects by ID. `fields`:: (Optional, array) The fields to return in the `attributes` key of the object response. +`namespaces`:: + (Optional, string array) Identifiers for the <> in which to search for this object. If this is provided, the object + is searched for only in the explicitly defined spaces. If this is not provided, the object is searched for in the current space (default + behavior). +* For shareable object types (registered with `namespaceType: 'multiple'`): this option can be used to specify one or more spaces, including +the "All spaces" identifier (`'*'`). +* For isolated object types (registered with `namespaceType: 'single'` or `namespaceType: 'multiple-isolated'`): this option can only be +used to specify a single space, and the "All spaces" identifier (`'*'`) is not allowed. +* For global object types (registered with `namespaceType: 'agnostic'`): this option cannot be used. + [[saved-objects-api-bulk-get-response-body]] ==== Response body diff --git a/docs/development/core/server/kibana-plugin-core-server.savedobjectsbulkgetobject.md b/docs/development/core/server/kibana-plugin-core-server.savedobjectsbulkgetobject.md index ab2c0c1110255..0ad5f1d66ee52 100644 --- a/docs/development/core/server/kibana-plugin-core-server.savedobjectsbulkgetobject.md +++ b/docs/development/core/server/kibana-plugin-core-server.savedobjectsbulkgetobject.md @@ -17,5 +17,6 @@ export interface SavedObjectsBulkGetObject | --- | --- | --- | | [fields](./kibana-plugin-core-server.savedobjectsbulkgetobject.fields.md) | string[] | SavedObject fields to include in the response | | [id](./kibana-plugin-core-server.savedobjectsbulkgetobject.id.md) | string | | +| [namespaces](./kibana-plugin-core-server.savedobjectsbulkgetobject.namespaces.md) | string[] | Optional namespace(s) for the object to be retrieved in. If this is defined, it will supersede the namespace ID that is in the top-level options.\* For shareable object types (registered with namespaceType: 'multiple'): this option can be used to specify one or more spaces, including the "All spaces" identifier ('*'). \* For isolated object types (registered with namespaceType: 'single' or namespaceType: 'multiple-isolated'): this option can only be used to specify a single space, and the "All spaces" identifier ('*') is not allowed. \* For global object types (registered with namespaceType: 'agnostic'): this option cannot be used. | | [type](./kibana-plugin-core-server.savedobjectsbulkgetobject.type.md) | string | | diff --git a/docs/development/core/server/kibana-plugin-core-server.savedobjectsbulkgetobject.namespaces.md b/docs/development/core/server/kibana-plugin-core-server.savedobjectsbulkgetobject.namespaces.md new file mode 100644 index 0000000000000..5add0ad1bdf95 --- /dev/null +++ b/docs/development/core/server/kibana-plugin-core-server.savedobjectsbulkgetobject.namespaces.md @@ -0,0 +1,15 @@ + + +[Home](./index.md) > [kibana-plugin-core-server](./kibana-plugin-core-server.md) > [SavedObjectsBulkGetObject](./kibana-plugin-core-server.savedobjectsbulkgetobject.md) > [namespaces](./kibana-plugin-core-server.savedobjectsbulkgetobject.namespaces.md) + +## SavedObjectsBulkGetObject.namespaces property + +Optional namespace(s) for the object to be retrieved in. If this is defined, it will supersede the namespace ID that is in the top-level options. + +\* For shareable object types (registered with `namespaceType: 'multiple'`): this option can be used to specify one or more spaces, including the "All spaces" identifier (`'*'`). \* For isolated object types (registered with `namespaceType: 'single'` or `namespaceType: 'multiple-isolated'`): this option can only be used to specify a single space, and the "All spaces" identifier (`'*'`) is not allowed. \* For global object types (registered with `namespaceType: 'agnostic'`): this option cannot be used. + +Signature: + +```typescript +namespaces?: string[]; +``` diff --git a/src/core/server/saved_objects/routes/bulk_get.ts b/src/core/server/saved_objects/routes/bulk_get.ts index 3838e4d3b3c8e..cf051d6cd25cc 100644 --- a/src/core/server/saved_objects/routes/bulk_get.ts +++ b/src/core/server/saved_objects/routes/bulk_get.ts @@ -25,6 +25,7 @@ export const registerBulkGetRoute = (router: IRouter, { coreUsageData }: RouteDe type: schema.string(), id: schema.string(), fields: schema.maybe(schema.arrayOf(schema.string())), + namespaces: schema.maybe(schema.arrayOf(schema.string())), }) ), }, diff --git a/src/core/server/saved_objects/service/lib/internal_utils.test.ts b/src/core/server/saved_objects/service/lib/internal_utils.test.ts index d1fd067990f07..200271964b3cb 100644 --- a/src/core/server/saved_objects/service/lib/internal_utils.test.ts +++ b/src/core/server/saved_objects/service/lib/internal_utils.test.ts @@ -13,6 +13,7 @@ import { getBulkOperationError, getSavedObjectFromSource, rawDocExistsInNamespace, + rawDocExistsInNamespaces, } from './internal_utils'; import { ALL_NAMESPACES_STRING } from './utils'; @@ -241,3 +242,86 @@ describe('#rawDocExistsInNamespace', () => { }); }); }); + +describe('#rawDocExistsInNamespaces', () => { + const SINGLE_NAMESPACE_TYPE = 'single-type'; + const MULTI_NAMESPACE_TYPE = 'multi-type'; + const NAMESPACE_AGNOSTIC_TYPE = 'agnostic-type'; + + const registry = typeRegistryMock.create(); + registry.isSingleNamespace.mockImplementation((type) => type === SINGLE_NAMESPACE_TYPE); + registry.isMultiNamespace.mockImplementation((type) => type === MULTI_NAMESPACE_TYPE); + registry.isNamespaceAgnostic.mockImplementation((type) => type === NAMESPACE_AGNOSTIC_TYPE); + + function createRawDoc( + type: string, + namespaceAttrs: { namespace?: string; namespaces?: string[] } + ) { + return { + // other fields exist on the raw document, but they are not relevant to these test cases + _source: { + type, + ...namespaceAttrs, + }, + } as SavedObjectsRawDoc; + } + + describe('single-namespace type', () => { + it('returns true regardless of namespace or namespaces fields', () => { + // Technically, a single-namespace type does not exist in a space unless it has a namespace prefix in its raw ID and a matching + // 'namespace' field. However, historically we have not enforced the latter, we have just relied on searching for and deserializing + // documents with the correct namespace prefix. We may revisit this in the future. + const doc1 = createRawDoc(SINGLE_NAMESPACE_TYPE, { namespace: 'some-space' }); // the namespace field is ignored + const doc2 = createRawDoc(SINGLE_NAMESPACE_TYPE, { namespaces: ['some-space'] }); // the namespaces field is ignored + expect(rawDocExistsInNamespaces(registry, doc1, [])).toBe(true); + expect(rawDocExistsInNamespaces(registry, doc1, ['some-space'])).toBe(true); + expect(rawDocExistsInNamespaces(registry, doc1, ['other-space'])).toBe(true); + expect(rawDocExistsInNamespaces(registry, doc2, [])).toBe(true); + expect(rawDocExistsInNamespaces(registry, doc2, ['some-space'])).toBe(true); + expect(rawDocExistsInNamespaces(registry, doc2, ['other-space'])).toBe(true); + }); + }); + + describe('multi-namespace type', () => { + const docInDefaultSpace = createRawDoc(MULTI_NAMESPACE_TYPE, { namespaces: ['default'] }); + const docInSomeSpace = createRawDoc(MULTI_NAMESPACE_TYPE, { namespaces: ['some-space'] }); + const docInAllSpaces = createRawDoc(MULTI_NAMESPACE_TYPE, { + namespaces: [ALL_NAMESPACES_STRING], + }); + const docInNoSpace = createRawDoc(MULTI_NAMESPACE_TYPE, { namespaces: [] }); + + it('returns true when the document namespaces matches', () => { + expect(rawDocExistsInNamespaces(registry, docInDefaultSpace, ['default'])).toBe(true); + expect(rawDocExistsInNamespaces(registry, docInAllSpaces, ['default'])).toBe(true); + expect(rawDocExistsInNamespaces(registry, docInSomeSpace, ['some-space'])).toBe(true); + expect(rawDocExistsInNamespaces(registry, docInAllSpaces, ['some-space'])).toBe(true); + expect(rawDocExistsInNamespaces(registry, docInDefaultSpace, ['*'])).toBe(true); + expect(rawDocExistsInNamespaces(registry, docInSomeSpace, ['*'])).toBe(true); + expect(rawDocExistsInNamespaces(registry, docInAllSpaces, ['*'])).toBe(true); + }); + + it('returns false when the document namespace does not match', () => { + expect(rawDocExistsInNamespaces(registry, docInSomeSpace, ['default'])).toBe(false); + expect(rawDocExistsInNamespaces(registry, docInNoSpace, ['default'])).toBe(false); + expect(rawDocExistsInNamespaces(registry, docInDefaultSpace, ['some-space'])).toBe(false); + expect(rawDocExistsInNamespaces(registry, docInNoSpace, ['some-space'])).toBe(false); + expect(rawDocExistsInNamespaces(registry, docInDefaultSpace, [])).toBe(false); + expect(rawDocExistsInNamespaces(registry, docInSomeSpace, [])).toBe(false); + expect(rawDocExistsInNamespaces(registry, docInAllSpaces, [])).toBe(false); + expect(rawDocExistsInNamespaces(registry, docInNoSpace, [])).toBe(false); + }); + }); + + describe('namespace-agnostic type', () => { + it('returns true regardless of namespace or namespaces fields', () => { + const doc1 = createRawDoc(NAMESPACE_AGNOSTIC_TYPE, { namespace: 'some-space' }); // the namespace field is ignored + const doc2 = createRawDoc(NAMESPACE_AGNOSTIC_TYPE, { namespaces: ['some-space'] }); // the namespaces field is ignored + expect(rawDocExistsInNamespaces(registry, doc1, [])).toBe(true); + expect(rawDocExistsInNamespaces(registry, doc1, ['some-space'])).toBe(true); + expect(rawDocExistsInNamespaces(registry, doc1, ['other-space'])).toBe(true); + expect(rawDocExistsInNamespaces(registry, doc2, [])).toBe(true); + expect(rawDocExistsInNamespaces(registry, doc2, ['some-space'])).toBe(true); + expect(rawDocExistsInNamespaces(registry, doc2, ['other-space'])).toBe(true); + }); + }); +}); diff --git a/src/core/server/saved_objects/service/lib/internal_utils.ts b/src/core/server/saved_objects/service/lib/internal_utils.ts index feaaea15649c7..9f8b2d44ae325 100644 --- a/src/core/server/saved_objects/service/lib/internal_utils.ts +++ b/src/core/server/saved_objects/service/lib/internal_utils.ts @@ -141,3 +141,40 @@ export function rawDocExistsInNamespace( namespaces?.includes(ALL_NAMESPACES_STRING); return existsInNamespace ?? false; } + +/** + * Check to ensure that a raw document exists in at least one of the given namespaces. If the document is not a multi-namespace type, then + * this returns `true` as we rely on the guarantees of the document ID format. If the document is a multi-namespace type, this checks to + * ensure that the document's `namespaces` value includes the string representation of at least one of the given namespaces. + * + * WARNING: This should only be used for documents that were retrieved from Elasticsearch. Otherwise, the guarantees of the document ID + * format mentioned above do not apply. + * + * @param registry + * @param raw + * @param namespaces + */ +export function rawDocExistsInNamespaces( + registry: ISavedObjectTypeRegistry, + raw: SavedObjectsRawDoc, + namespaces: string[] +) { + const rawDocType = raw._source.type; + + // if the type is namespace isolated, or namespace agnostic, we can continue to rely on the guarantees + // of the document ID format and don't need to check this + if (!registry.isMultiNamespace(rawDocType)) { + return true; + } + + const namespacesToCheck = new Set(namespaces); + const existingNamespaces = raw._source.namespaces ?? []; + + if (namespacesToCheck.size === 0 || existingNamespaces.length === 0) { + return false; + } else if (namespacesToCheck.has(ALL_NAMESPACES_STRING)) { + return true; + } + + return existingNamespaces.some((x) => x === ALL_NAMESPACES_STRING || namespacesToCheck.has(x)); +} 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 efae5bd737020..d4d794c165fe2 100644 --- a/src/core/server/saved_objects/service/lib/repository.test.js +++ b/src/core/server/saved_objects/service/lib/repository.test.js @@ -953,7 +953,7 @@ describe('SavedObjectsRepository', () => { const bulkGet = async (objects, options) => savedObjectsRepository.bulkGet( - objects.map(({ type, id }) => ({ type, id })), // bulkGet only uses type and id + objects.map(({ type, id, namespaces }) => ({ type, id, namespaces })), // bulkGet only uses type, id, and optionally namespaces options ); const bulkGetSuccess = async (objects, options) => { @@ -992,6 +992,13 @@ describe('SavedObjectsRepository', () => { _expectClientCallArgs([obj1, obj2], { getId }); }); + it(`prepends namespace to the id when providing namespaces for single-namespace type`, async () => { + const getId = (type, id) => `${namespace}:${type}:${id}`; // test that the raw document ID equals this (e.g., has a namespace prefix) + const objects = [obj1, obj2].map((obj) => ({ ...obj, namespaces: [namespace] })); + await bulkGetSuccess(objects, { namespace: 'some-other-ns' }); + _expectClientCallArgs([obj1, obj2], { getId }); + }); + it(`doesn't prepend namespace to the id when providing no namespace for single-namespace type`, async () => { const getId = (type, id) => `${type}:${id}`; // test that the raw document ID equals this (e.g., does not have a namespace prefix) await bulkGetSuccess([obj1, obj2]); @@ -1011,33 +1018,35 @@ describe('SavedObjectsRepository', () => { _expectClientCallArgs(objects, { getId }); client.mget.mockClear(); - objects = [obj1, obj2].map((obj) => ({ ...obj, type: MULTI_NAMESPACE_ISOLATED_TYPE })); + objects = [obj1, { ...obj2, namespaces: ['some-other-ns'] }].map((obj) => ({ + ...obj, + type: MULTI_NAMESPACE_ISOLATED_TYPE, + })); await bulkGetSuccess(objects, { namespace }); _expectClientCallArgs(objects, { getId }); }); }); describe('errors', () => { - const bulkGetErrorInvalidType = async ([obj1, obj, obj2]) => { - const response = getMockMgetResponse([obj1, obj2]); + const bulkGetError = async (obj, isBulkError, expectedErrorResult) => { + let response; + if (isBulkError) { + // mock the bulk error for only the second object + mockGetBulkOperationError.mockReturnValueOnce(undefined); + mockGetBulkOperationError.mockReturnValueOnce(expectedErrorResult.error); + response = getMockMgetResponse([obj1, obj, obj2]); + } else { + response = getMockMgetResponse([obj1, obj2]); + } client.mget.mockResolvedValueOnce( elasticsearchClientMock.createSuccessTransportRequestPromise(response) ); - const result = await bulkGet([obj1, obj, obj2]); - expect(client.mget).toHaveBeenCalled(); - expect(result).toEqual({ - saved_objects: [expectSuccess(obj1), expectErrorInvalidType(obj), expectSuccess(obj2)], - }); - }; - const bulkGetErrorNotFound = async ([obj1, obj, obj2], options, response) => { - client.mget.mockResolvedValueOnce( - elasticsearchClientMock.createSuccessTransportRequestPromise(response) - ); - const result = await bulkGet([obj1, obj, obj2], options); + const objects = [obj1, obj, obj2]; + const result = await bulkGet(objects); expect(client.mget).toHaveBeenCalled(); expect(result).toEqual({ - saved_objects: [expectSuccess(obj1), expectErrorNotFound(obj), expectSuccess(obj2)], + saved_objects: [expectSuccess(obj1), expectedErrorResult, expectSuccess(obj2)], }); }; @@ -1063,33 +1072,65 @@ describe('SavedObjectsRepository', () => { ).rejects.toThrowError(createBadRequestError('"options.namespace" cannot be "*"')); }); + it(`returns error when namespaces is used with a space-agnostic object`, async () => { + const obj = { type: NAMESPACE_AGNOSTIC_TYPE, id: 'three', namespaces: [] }; + await bulkGetError( + obj, + undefined, + expectErrorResult( + obj, + createBadRequestError('"namespaces" cannot be used on space-agnostic types') + ) + ); + }); + + it(`returns error when namespaces is used with a space-isolated object and does not specify a single space`, async () => { + const doTest = async (objType, namespaces) => { + const obj = { type: objType, id: 'three', namespaces }; + await bulkGetError( + obj, + undefined, + expectErrorResult( + obj, + createBadRequestError( + '"namespaces" can only specify a single space when used with space-isolated types' + ) + ) + ); + }; + await doTest('dashboard', ['spacex', 'spacey']); + await doTest('dashboard', ['*']); + await doTest(MULTI_NAMESPACE_ISOLATED_TYPE, ['spacex', 'spacey']); + await doTest(MULTI_NAMESPACE_ISOLATED_TYPE, ['*']); + }); + it(`returns error when type is invalid`, async () => { const obj = { type: 'unknownType', id: 'three' }; - await bulkGetErrorInvalidType([obj1, obj, obj2]); + await bulkGetError(obj, undefined, expectErrorInvalidType(obj)); }); it(`returns error when type is hidden`, async () => { const obj = { type: HIDDEN_TYPE, id: 'three' }; - await bulkGetErrorInvalidType([obj1, obj, obj2]); + await bulkGetError(obj, undefined, expectErrorInvalidType(obj)); }); it(`returns error when document is not found`, async () => { const obj = { type: 'dashboard', id: 'three', found: false }; - const response = getMockMgetResponse([obj1, obj, obj2]); - await bulkGetErrorNotFound([obj1, obj, obj2], undefined, response); + await bulkGetError(obj, true, expectErrorNotFound(obj)); }); it(`handles missing ids gracefully`, async () => { const obj = { type: 'dashboard', id: undefined, found: false }; - const response = getMockMgetResponse([obj1, obj, obj2]); - await bulkGetErrorNotFound([obj1, obj, obj2], undefined, response); + await bulkGetError(obj, true, expectErrorNotFound(obj)); }); it(`returns error when type is multi-namespace and the document exists, but not in this namespace`, async () => { - const obj = { type: MULTI_NAMESPACE_ISOLATED_TYPE, id: 'three' }; - const response = getMockMgetResponse([obj1, obj, obj2]); - response.docs[1].namespaces = ['bar-namespace']; - await bulkGetErrorNotFound([obj1, obj, obj2], { namespace }, response); + const obj = { + type: MULTI_NAMESPACE_ISOLATED_TYPE, + id: 'three', + namespace: 'bar-namespace', + }; + await bulkGetError(obj, true, expectErrorNotFound(obj)); }); it(`throws when ES mget action responds with a 404 and a missing Elasticsearch product header`, async () => { diff --git a/src/core/server/saved_objects/service/lib/repository.ts b/src/core/server/saved_objects/service/lib/repository.ts index 365fc6a3734e4..11b95b6f9b1de 100644 --- a/src/core/server/saved_objects/service/lib/repository.ts +++ b/src/core/server/saved_objects/service/lib/repository.ts @@ -74,6 +74,7 @@ import { getExpectedVersionProperties, getSavedObjectFromSource, rawDocExistsInNamespace, + rawDocExistsInNamespaces, } from './internal_utils'; import { ALL_NAMESPACES_STRING, @@ -966,16 +967,23 @@ export class SavedObjectsRepository { let bulkGetRequestIndexCounter = 0; const expectedBulkGetResults: Either[] = objects.map((object) => { - const { type, id, fields } = object; + const { type, id, fields, namespaces } = object; + let error: DecoratedError | undefined; if (!this._allowedTypes.includes(type)) { + error = SavedObjectsErrorHelpers.createUnsupportedTypeError(type); + } else { + try { + this.validateObjectNamespaces(type, id, namespaces); + } catch (e) { + error = e; + } + } + + if (error) { return { tag: 'Left' as 'Left', - error: { - id, - type, - error: errorContent(SavedObjectsErrorHelpers.createUnsupportedTypeError(type)), - }, + error: { id, type, error: errorContent(error) }, }; } @@ -985,15 +993,18 @@ export class SavedObjectsRepository { type, id, fields, + namespaces, esRequestIndex: bulkGetRequestIndexCounter++, }, }; }); + const getNamespaceId = (namespaces?: string[]) => + namespaces !== undefined ? SavedObjectsUtils.namespaceStringToId(namespaces[0]) : namespace; const bulkGetDocs = expectedBulkGetResults .filter(isRight) - .map(({ value: { type, id, fields } }) => ({ - _id: this._serializer.generateRawId(namespace, type, id), + .map(({ value: { type, id, fields, namespaces } }) => ({ + _id: this._serializer.generateRawId(getNamespaceId(namespaces), type, id), _index: this.getIndexForType(type), _source: { includes: includedFields(type, fields) }, })); @@ -1023,11 +1034,16 @@ export class SavedObjectsRepository { return expectedResult.error as any; } - const { type, id, esRequestIndex } = expectedResult.value; + const { + type, + id, + namespaces = [SavedObjectsUtils.namespaceIdToString(namespace)], + esRequestIndex, + } = expectedResult.value; const doc = bulkGetResponse?.body.docs[esRequestIndex]; // @ts-expect-error MultiGetHit._source is optional - if (!doc?.found || !this.rawDocExistsInNamespace(doc, namespace)) { + if (!doc?.found || !this.rawDocExistsInNamespaces(doc, namespaces)) { return ({ id, type, @@ -2116,6 +2132,10 @@ export class SavedObjectsRepository { return omit(savedObject, ['namespace']) as SavedObject; } + private rawDocExistsInNamespaces(raw: SavedObjectsRawDoc, namespaces: string[]) { + return rawDocExistsInNamespaces(this._registry, raw, namespaces); + } + private rawDocExistsInNamespace(raw: SavedObjectsRawDoc, namespace: string | undefined) { return rawDocExistsInNamespace(this._registry, raw, namespace); } @@ -2228,6 +2248,7 @@ export class SavedObjectsRepository { ).catch(() => {}); // if the call fails for some reason, intentionally swallow the error } + /** The `initialNamespaces` field (create, bulkCreate) is used to create an object in an initial set of spaces. */ private validateInitialNamespaces(type: string, initialNamespaces: string[] | undefined) { if (!initialNamespaces) { return; @@ -2250,6 +2271,28 @@ export class SavedObjectsRepository { ); } } + + /** The object-specific `namespaces` field (bulkGet) is used to check if an object exists in any of a given number of spaces. */ + private validateObjectNamespaces(type: string, id: string, namespaces: string[] | undefined) { + if (!namespaces) { + return; + } + + if (this._registry.isNamespaceAgnostic(type)) { + throw SavedObjectsErrorHelpers.createBadRequestError( + '"namespaces" cannot be used on space-agnostic types' + ); + } else if (!namespaces.length) { + throw SavedObjectsErrorHelpers.createGenericNotFoundError(type, id); + } else if ( + !this._registry.isShareable(type) && + (namespaces.length > 1 || namespaces.includes(ALL_NAMESPACES_STRING)) + ) { + throw SavedObjectsErrorHelpers.createBadRequestError( + '"namespaces" can only specify a single space when used with space-isolated types' + ); + } + } } /** diff --git a/src/core/server/saved_objects/service/saved_objects_client.ts b/src/core/server/saved_objects/service/saved_objects_client.ts index 1564df2969ecc..dd9a8393e0624 100644 --- a/src/core/server/saved_objects/service/saved_objects_client.ts +++ b/src/core/server/saved_objects/service/saved_objects_client.ts @@ -278,6 +278,17 @@ export interface SavedObjectsBulkGetObject { type: string; /** SavedObject fields to include in the response */ fields?: string[]; + /** + * Optional namespace(s) for the object to be retrieved in. If this is defined, it will supersede the namespace ID that is in the + * top-level options. + * + * * For shareable object types (registered with `namespaceType: 'multiple'`): this option can be used to specify one or more spaces, + * including the "All spaces" identifier (`'*'`). + * * For isolated object types (registered with `namespaceType: 'single'` or `namespaceType: 'multiple-isolated'`): this option can only + * be used to specify a single space, and the "All spaces" identifier (`'*'`) is not allowed. + * * For global object types (registered with `namespaceType: 'agnostic'`): this option cannot be used. + */ + namespaces?: string[]; } /** diff --git a/src/core/server/server.api.md b/src/core/server/server.api.md index b4f07bc393e25..cb99d1f024054 100644 --- a/src/core/server/server.api.md +++ b/src/core/server/server.api.md @@ -1858,6 +1858,7 @@ export interface SavedObjectsBulkGetObject { fields?: string[]; // (undocumented) id: string; + namespaces?: string[]; // (undocumented) type: string; } diff --git a/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.test.ts b/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.test.ts index 2f622d9e8a0e1..96f8c5ff02d24 100644 --- a/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.test.ts +++ b/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.test.ts @@ -426,10 +426,17 @@ describe('#bulkGet', () => { expect(result).toEqual(apiCallReturnValue); }); - test(`checks privileges for user, actions, and namespace`, async () => { - const objects = [obj1, obj2]; + test(`checks privileges for user, actions, namespace, and (object) namespaces`, async () => { + const objects = [ + { ...obj1, namespaces: ['another-ns'] }, + { ...obj2, namespaces: ['yet-another-ns'] }, + ]; const options = { namespace }; - await expectPrivilegeCheck(client.bulkGet, { objects, options }, namespace); + await expectPrivilegeCheck(client.bulkGet, { objects, options }, [ + namespace, + 'another-ns', + 'yet-another-ns', + ]); }); test(`filters namespaces that the user doesn't have access to`, async () => { diff --git a/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.ts b/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.ts index 6f2b8d28a5601..11eca287cd4f5 100644 --- a/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.ts +++ b/x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.ts @@ -287,11 +287,15 @@ export class SecureSavedObjectsClientWrapper implements SavedObjectsClientContra options: SavedObjectsBaseOptions = {} ) { try { + const namespaces = objects.reduce( + (acc, { namespaces: objNamespaces = [] }) => acc.concat(objNamespaces), + [options.namespace] + ); const args = { objects, options }; await this.legacyEnsureAuthorized( this.getUniqueObjectTypes(objects), 'bulk_get', - options.namespace, + namespaces, { args, } diff --git a/x-pack/plugins/spaces/server/saved_objects/spaces_saved_objects_client.test.ts b/x-pack/plugins/spaces/server/saved_objects/spaces_saved_objects_client.test.ts index 9693bd9d56287..c1c5e63d5ae6e 100644 --- a/x-pack/plugins/spaces/server/saved_objects/spaces_saved_objects_client.test.ts +++ b/x-pack/plugins/spaces/server/saved_objects/spaces_saved_objects_client.test.ts @@ -148,7 +148,7 @@ const ERROR_NAMESPACE_SPECIFIED = 'Spaces currently determines the namespaces'; }); test(`supplements options with the current namespace`, async () => { - const { client, baseClient } = createSpacesSavedObjectsClient(); + const { client, baseClient, spacesClient } = createSpacesSavedObjectsClient(); const expectedReturnValue = { saved_objects: [createMockResponse()] }; baseClient.bulkGet.mockReturnValue(Promise.resolve(expectedReturnValue)); @@ -162,6 +162,32 @@ const ERROR_NAMESPACE_SPECIFIED = 'Spaces currently determines the namespaces'; foo: 'bar', namespace: currentSpace.expectedNamespace, }); + expect(spacesClient.getAll).not.toHaveBeenCalled(); + }); + + test(`replaces object namespaces '*' with available spaces`, async () => { + const { client, baseClient, spacesClient } = createSpacesSavedObjectsClient(); + spacesClient.getAll.mockResolvedValue([ + { id: 'available-space-a', name: 'a', disabledFeatures: [] }, + { id: 'available-space-b', name: 'b', disabledFeatures: [] }, + ]); + + const objects = [ + { type: 'foo', id: '1', namespaces: ['*'] }, + { type: 'bar', id: '2', namespaces: ['*', 'this-is-ignored'] }, + { type: 'baz', id: '3', namespaces: ['another-space'] }, + ]; + await client.bulkGet(objects); + + expect(baseClient.bulkGet).toHaveBeenCalledWith( + [ + { type: 'foo', id: '1', namespaces: ['available-space-a', 'available-space-b'] }, + { type: 'bar', id: '2', namespaces: ['available-space-a', 'available-space-b'] }, + { type: 'baz', id: '3', namespaces: ['another-space'] }, // even if another space doesn't exist, it can be specified explicitly + ], + { namespace: currentSpace.expectedNamespace } + ); + expect(spacesClient.getAll).toHaveBeenCalledTimes(1); }); }); diff --git a/x-pack/plugins/spaces/server/saved_objects/spaces_saved_objects_client.ts b/x-pack/plugins/spaces/server/saved_objects/spaces_saved_objects_client.ts index 3be1bb3cad10c..868c70b30306d 100644 --- a/x-pack/plugins/spaces/server/saved_objects/spaces_saved_objects_client.ts +++ b/x-pack/plugins/spaces/server/saved_objects/spaces_saved_objects_client.ts @@ -219,7 +219,25 @@ export class SpacesSavedObjectsClient implements SavedObjectsClientContract { ) { throwErrorIfNamespaceSpecified(options); - return await this.client.bulkGet(objects, { + let availableSpaces: string[] | undefined; + const getAvailableSpaces = async () => { + if (!availableSpaces) { + availableSpaces = await this.getSearchableSpaces([ALL_SPACES_ID]); + } + return availableSpaces; + }; + + const objectsToGet: SavedObjectsBulkGetObject[] = []; + for (const object of objects) { + const { namespaces } = object; + if (namespaces?.includes(ALL_SPACES_ID)) { + objectsToGet.push({ ...object, namespaces: await getAvailableSpaces() }); + } else { + objectsToGet.push(object); + } + } + + return await this.client.bulkGet(objectsToGet, { ...options, namespace: spaceIdToNamespace(this.spaceId), }); From beedf6f75dd30ccc1adb4b7963e2d52c1f55abf6 Mon Sep 17 00:00:00 2001 From: Joe Portner <5295965+jportner@users.noreply.github.com> Date: Wed, 25 Aug 2021 10:08:52 -0400 Subject: [PATCH 4/8] Add missing bulkCreate test cases --- .../security_and_spaces/apis/bulk_create.ts | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/x-pack/test/saved_object_api_integration/security_and_spaces/apis/bulk_create.ts b/x-pack/test/saved_object_api_integration/security_and_spaces/apis/bulk_create.ts index e048a4abc8ccc..e17d33102b995 100644 --- a/x-pack/test/saved_object_api_integration/security_and_spaces/apis/bulk_create.ts +++ b/x-pack/test/saved_object_api_integration/security_and_spaces/apis/bulk_create.ts @@ -92,7 +92,7 @@ const createTestCases = (overwrite: boolean, spaceId: string) => { CASES.INITIAL_NS_MULTI_NAMESPACE_OBJ_ALL_SPACES, ]; const hiddenType = [{ ...CASES.HIDDEN, ...fail400() }]; - const allTypes = normalTypes.concat(hiddenType); + const allTypes = [...normalTypes, ...crossNamespace, ...hiddenType]; return { normalTypes, crossNamespace, hiddenType, allTypes }; }; @@ -118,18 +118,17 @@ export default function ({ getService }: FtrProviderContext) { singleRequest: true, }), createTestDefinitions(hiddenType, true, overwrite, { spaceId, user }), - createTestDefinitions(allTypes, true, overwrite, { - spaceId, - user, - singleRequest: true, - responseBodyOverride: expectSavedObjectForbidden(['hiddentype']), - }), ].flat(); return { unauthorized: createTestDefinitions(allTypes, true, overwrite, { spaceId, user }), authorizedAtSpace: [ authorizedCommon, createTestDefinitions(crossNamespace, true, overwrite, { spaceId, user }), + createTestDefinitions(allTypes, true, overwrite, { + spaceId, + user, + singleRequest: true, + }), ].flat(), authorizedEverywhere: [ authorizedCommon, @@ -138,6 +137,12 @@ export default function ({ getService }: FtrProviderContext) { user, singleRequest: true, }), + createTestDefinitions(allTypes, true, overwrite, { + spaceId, + user, + singleRequest: true, + responseBodyOverride: expectSavedObjectForbidden(['hiddentype']), + }), ].flat(), superuser: createTestDefinitions(allTypes, false, overwrite, { spaceId, From e90b9005de3b3d49f4a16c6d529086e724423d02 Mon Sep 17 00:00:00 2001 From: Joe Portner <5295965+jportner@users.noreply.github.com> Date: Wed, 25 Aug 2021 11:09:28 -0400 Subject: [PATCH 5/8] Update authZ checks for SpacesSavedObjectsClient 1. Removed previous changes to pass unit tests, changed the tests instead. The tests were meant to be asserting for the "403 Forbidden" error. 2. Updated bulkGet to replace `'*'` with an empty namespaces array when a user is not authorized to access any spaces. This is primarily so that the API is consistent (the Security plugin will still check that they are authorized to access the type in the active space and return a more specific 403 error if not). --- .../spaces_saved_objects_client.test.ts | 26 +++++++++++++++++-- .../spaces_saved_objects_client.ts | 14 +++++++--- 2 files changed, 35 insertions(+), 5 deletions(-) diff --git a/x-pack/plugins/spaces/server/saved_objects/spaces_saved_objects_client.test.ts b/x-pack/plugins/spaces/server/saved_objects/spaces_saved_objects_client.test.ts index c1c5e63d5ae6e..980682fcfce3c 100644 --- a/x-pack/plugins/spaces/server/saved_objects/spaces_saved_objects_client.test.ts +++ b/x-pack/plugins/spaces/server/saved_objects/spaces_saved_objects_client.test.ts @@ -189,6 +189,28 @@ const ERROR_NAMESPACE_SPECIFIED = 'Spaces currently determines the namespaces'; ); expect(spacesClient.getAll).toHaveBeenCalledTimes(1); }); + + test(`replaces object namespaces '*' with an empty array when the user doesn't have access to any spaces`, async () => { + const { client, baseClient, spacesClient } = createSpacesSavedObjectsClient(); + spacesClient.getAll.mockRejectedValue(Boom.forbidden()); + + const objects = [ + { type: 'foo', id: '1', namespaces: ['*'] }, + { type: 'bar', id: '2', namespaces: ['*', 'this-is-ignored'] }, + { type: 'baz', id: '3', namespaces: ['another-space'] }, + ]; + await client.bulkGet(objects); + + expect(baseClient.bulkGet).toHaveBeenCalledWith( + [ + { type: 'foo', id: '1', namespaces: [] }, + { type: 'bar', id: '2', namespaces: [] }, + { type: 'baz', id: '3', namespaces: ['another-space'] }, // even if another space doesn't exist, it can be specified explicitly + ], + { namespace: currentSpace.expectedNamespace } + ); + expect(spacesClient.getAll).toHaveBeenCalledTimes(1); + }); }); describe('#find', () => { @@ -207,7 +229,7 @@ const ERROR_NAMESPACE_SPECIFIED = 'Spaces currently determines the namespaces'; test(`returns empty result if user is unauthorized in any space`, async () => { const { client, baseClient, spacesClient } = createSpacesSavedObjectsClient(); - spacesClient.getAll.mockRejectedValue(Boom.unauthorized()); + spacesClient.getAll.mockRejectedValue(Boom.forbidden()); const options = Object.freeze({ type: 'foo', namespaces: ['some-ns'] }); const actualReturnValue = await client.find(options); @@ -560,7 +582,7 @@ const ERROR_NAMESPACE_SPECIFIED = 'Spaces currently determines the namespaces'; test(`throws error if if user is unauthorized in any space`, async () => { const { client, baseClient, spacesClient } = createSpacesSavedObjectsClient(); - spacesClient.getAll.mockRejectedValue(Boom.unauthorized()); + spacesClient.getAll.mockRejectedValue(Boom.forbidden()); await expect( client.openPointInTimeForType('foo', { namespaces: ['bar'] }) diff --git a/x-pack/plugins/spaces/server/saved_objects/spaces_saved_objects_client.ts b/x-pack/plugins/spaces/server/saved_objects/spaces_saved_objects_client.ts index 868c70b30306d..c3b208029c8ac 100644 --- a/x-pack/plugins/spaces/server/saved_objects/spaces_saved_objects_client.ts +++ b/x-pack/plugins/spaces/server/saved_objects/spaces_saved_objects_client.ts @@ -179,7 +179,7 @@ export class SpacesSavedObjectsClient implements SavedObjectsClientContract { try { namespaces = await this.getSearchableSpaces(options.namespaces); } catch (err) { - if (Boom.isBoom(err) && err.output.payload.statusCode === 401) { + if (Boom.isBoom(err) && err.output.payload.statusCode === 403) { // return empty response, since the user is unauthorized in any space, but we don't return forbidden errors for `find` operations return SavedObjectsUtils.createEmptyFindResponse(options); } @@ -222,7 +222,15 @@ export class SpacesSavedObjectsClient implements SavedObjectsClientContract { let availableSpaces: string[] | undefined; const getAvailableSpaces = async () => { if (!availableSpaces) { - availableSpaces = await this.getSearchableSpaces([ALL_SPACES_ID]); + try { + availableSpaces = await this.getSearchableSpaces([ALL_SPACES_ID]); + } catch (err) { + if (Boom.isBoom(err) && err.output.payload.statusCode === 403) { + availableSpaces = []; // the user doesn't have access to any spaces + } else { + throw err; + } + } } return availableSpaces; }; @@ -405,7 +413,7 @@ export class SpacesSavedObjectsClient implements SavedObjectsClientContract { try { namespaces = await this.getSearchableSpaces(options.namespaces); } catch (err) { - if (Boom.isBoom(err) && err.output.payload.statusCode === 401) { + if (Boom.isBoom(err) && err.output.payload.statusCode === 403) { // throw bad request since the user is unauthorized in any space throw SavedObjectsErrorHelpers.createBadRequestError(); } From 5e5e00cccdf0c026ce531c0607691d2a9a5a7b1c Mon Sep 17 00:00:00 2001 From: Joe Portner <5295965+jportner@users.noreply.github.com> Date: Wed, 25 Aug 2021 15:57:17 -0400 Subject: [PATCH 6/8] Handle bad requests to bulkGet in the SecureSavedObjectsClient At the repository level, if a user tries to bulkGet an isolated object in multiple spaces (explicitly defined, or '*') they will get a 400 Bad Request error. However, in the Spaces SOC wrapper we are deconstructing the '*' identifier and replacing it with all available spaces. This can cause a problem when the user has access to only one space, the API response would be inconsistent (the repository would treat this as a perfectly valid request and attempt to fetch the object.) So, now in the Spaces SOC wrapper we modify the results based on what the request, leaving any 403 errors from the Security SOC wrapper intact. --- .../spaces_saved_objects_client.test.ts | 97 +++++++++++++------ .../spaces_saved_objects_client.ts | 86 +++++++++++----- 2 files changed, 129 insertions(+), 54 deletions(-) diff --git a/x-pack/plugins/spaces/server/saved_objects/spaces_saved_objects_client.test.ts b/x-pack/plugins/spaces/server/saved_objects/spaces_saved_objects_client.test.ts index 980682fcfce3c..25952b8b0eb44 100644 --- a/x-pack/plugins/spaces/server/saved_objects/spaces_saved_objects_client.test.ts +++ b/x-pack/plugins/spaces/server/saved_objects/spaces_saved_objects_client.test.ts @@ -7,36 +7,15 @@ import Boom from '@hapi/boom'; -import { SavedObjectTypeRegistry } from 'src/core/server'; -import { savedObjectsClientMock } from 'src/core/server/mocks'; +import type { SavedObject, SavedObjectsType } from 'src/core/server'; +import { SavedObjectsErrorHelpers } from 'src/core/server'; +import { savedObjectsClientMock, savedObjectsTypeRegistryMock } from 'src/core/server/mocks'; import { DEFAULT_SPACE_ID } from '../../common/constants'; import { spacesClientMock } from '../spaces_client/spaces_client.mock'; import { spacesServiceMock } from '../spaces_service/spaces_service.mock'; import { SpacesSavedObjectsClient } from './spaces_saved_objects_client'; -const typeRegistry = new SavedObjectTypeRegistry(); -typeRegistry.registerType({ - name: 'foo', - namespaceType: 'single', - hidden: false, - mappings: { properties: {} }, -}); - -typeRegistry.registerType({ - name: 'bar', - namespaceType: 'single', - hidden: false, - mappings: { properties: {} }, -}); - -typeRegistry.registerType({ - name: 'space', - namespaceType: 'agnostic', - hidden: true, - mappings: { properties: {} }, -}); - const createMockRequest = () => ({}); const createMockClient = () => savedObjectsClientMock.create(); @@ -69,6 +48,13 @@ const ERROR_NAMESPACE_SPECIFIED = 'Spaces currently determines the namespaces'; const spacesService = createSpacesService(currentSpace.id); const spacesClient = spacesClientMock.create(); spacesService.createSpacesClient.mockReturnValue(spacesClient); + const typeRegistry = savedObjectsTypeRegistryMock.create(); + typeRegistry.getAllTypes.mockReturnValue(([ + // for test purposes we only need the names of the object types + { name: 'foo' }, + { name: 'bar' }, + { name: 'space' }, + ] as unknown) as SavedObjectsType[]); const client = new SpacesSavedObjectsClient({ request, @@ -76,7 +62,7 @@ const ERROR_NAMESPACE_SPECIFIED = 'Spaces currently determines the namespaces'; getSpacesService: () => spacesService, typeRegistry, }); - return { client, baseClient, spacesClient }; + return { client, baseClient, spacesClient, typeRegistry }; }; describe('#get', () => { @@ -157,7 +143,7 @@ const ERROR_NAMESPACE_SPECIFIED = 'Spaces currently determines the namespaces'; // @ts-expect-error const actualReturnValue = await client.bulkGet(objects, options); - expect(actualReturnValue).toBe(expectedReturnValue); + expect(actualReturnValue).toEqual(expectedReturnValue); expect(baseClient.bulkGet).toHaveBeenCalledWith(objects, { foo: 'bar', namespace: currentSpace.expectedNamespace, @@ -166,24 +152,70 @@ const ERROR_NAMESPACE_SPECIFIED = 'Spaces currently determines the namespaces'; }); test(`replaces object namespaces '*' with available spaces`, async () => { - const { client, baseClient, spacesClient } = createSpacesSavedObjectsClient(); + const { client, baseClient, spacesClient, typeRegistry } = createSpacesSavedObjectsClient(); spacesClient.getAll.mockResolvedValue([ { id: 'available-space-a', name: 'a', disabledFeatures: [] }, { id: 'available-space-b', name: 'b', disabledFeatures: [] }, ]); + typeRegistry.isNamespaceAgnostic.mockImplementation((type) => type === 'foo'); + typeRegistry.isShareable.mockImplementation((type) => type === 'bar'); + // 'baz' is neither agnostic nor shareable, so it is isolated (namespaceType: 'single' or namespaceType: 'multiple-isolated') + baseClient.bulkGet.mockResolvedValue({ + saved_objects: ([ + { type: 'foo', id: '1', key: 'val' }, + { type: 'bar', id: '2', key: 'val' }, + { type: 'baz', id: '3', key: 'val' }, // this should be replaced with a 400 error + { type: 'foo', id: '4', error: { statusCode: 403 } }, + { type: 'bar', id: '5', error: { statusCode: 403 } }, + { type: 'baz', id: '6', error: { statusCode: 403 } }, // this should be not be replaced with a 400 error because it has a 403 error which is higher priority + { type: 'foo', id: '7', key: 'val' }, + { type: 'bar', id: '8', key: 'val' }, + { type: 'baz', id: '9', key: 'val' }, // this should not be replaced with a 400 error because the user did not search for it in '*' all spaces + ] as unknown) as SavedObject[], + }); const objects = [ - { type: 'foo', id: '1', namespaces: ['*'] }, + { type: 'foo', id: '1', namespaces: ['*', 'this-is-ignored'] }, { type: 'bar', id: '2', namespaces: ['*', 'this-is-ignored'] }, - { type: 'baz', id: '3', namespaces: ['another-space'] }, + { type: 'baz', id: '3', namespaces: ['*', 'this-is-ignored'] }, + { type: 'foo', id: '4', namespaces: ['*'] }, + { type: 'bar', id: '5', namespaces: ['*'] }, + { type: 'baz', id: '6', namespaces: ['*'] }, + { type: 'foo', id: '7', namespaces: ['another-space'] }, + { type: 'bar', id: '8', namespaces: ['another-space'] }, + { type: 'baz', id: '9', namespaces: ['another-space'] }, ]; - await client.bulkGet(objects); - + const result = await client.bulkGet(objects); + + expect(result.saved_objects).toEqual([ + { type: 'foo', id: '1', key: 'val' }, + { type: 'bar', id: '2', key: 'val' }, + { + type: 'baz', + id: '3', + error: SavedObjectsErrorHelpers.createBadRequestError( + '"namespaces" can only specify a single space when used with space-isolated types' + ).output.payload, + }, + { type: 'foo', id: '4', error: { statusCode: 403 } }, + { type: 'bar', id: '5', error: { statusCode: 403 } }, + { type: 'baz', id: '6', error: { statusCode: 403 } }, + { type: 'foo', id: '7', key: 'val' }, + { type: 'bar', id: '8', key: 'val' }, + { type: 'baz', id: '9', key: 'val' }, + ]); expect(baseClient.bulkGet).toHaveBeenCalledWith( [ { type: 'foo', id: '1', namespaces: ['available-space-a', 'available-space-b'] }, { type: 'bar', id: '2', namespaces: ['available-space-a', 'available-space-b'] }, - { type: 'baz', id: '3', namespaces: ['another-space'] }, // even if another space doesn't exist, it can be specified explicitly + { type: 'baz', id: '3', namespaces: ['available-space-a', 'available-space-b'] }, + { type: 'foo', id: '4', namespaces: ['available-space-a', 'available-space-b'] }, + { type: 'bar', id: '5', namespaces: ['available-space-a', 'available-space-b'] }, + { type: 'baz', id: '6', namespaces: ['available-space-a', 'available-space-b'] }, + // even if another space doesn't exist, it can be specified explicitly + { type: 'foo', id: '7', namespaces: ['another-space'] }, + { type: 'bar', id: '8', namespaces: ['another-space'] }, + { type: 'baz', id: '9', namespaces: ['another-space'] }, ], { namespace: currentSpace.expectedNamespace } ); @@ -193,6 +225,7 @@ const ERROR_NAMESPACE_SPECIFIED = 'Spaces currently determines the namespaces'; test(`replaces object namespaces '*' with an empty array when the user doesn't have access to any spaces`, async () => { const { client, baseClient, spacesClient } = createSpacesSavedObjectsClient(); spacesClient.getAll.mockRejectedValue(Boom.forbidden()); + baseClient.bulkGet.mockResolvedValue({ saved_objects: [] }); // doesn't matter for this test const objects = [ { type: 'foo', id: '1', namespaces: ['*'] }, diff --git a/x-pack/plugins/spaces/server/saved_objects/spaces_saved_objects_client.ts b/x-pack/plugins/spaces/server/saved_objects/spaces_saved_objects_client.ts index c3b208029c8ac..2d662f9e2ba67 100644 --- a/x-pack/plugins/spaces/server/saved_objects/spaces_saved_objects_client.ts +++ b/x-pack/plugins/spaces/server/saved_objects/spaces_saved_objects_client.ts @@ -9,6 +9,7 @@ import Boom from '@hapi/boom'; import type { ISavedObjectTypeRegistry, + SavedObject, SavedObjectsBaseOptions, SavedObjectsBulkCreateObject, SavedObjectsBulkGetObject, @@ -36,6 +37,19 @@ import { spaceIdToNamespace } from '../lib/utils/namespace'; import type { ISpacesClient } from '../spaces_client'; import type { SpacesServiceStart } from '../spaces_service/spaces_service'; +interface Left { + tag: 'Left'; + value: L; +} + +interface Right { + tag: 'Right'; + value: R; +} + +type Either = Left | Right; +const isLeft = (either: Either): either is Left => either.tag === 'Left'; + interface SpacesSavedObjectsClientOptions { baseClient: SavedObjectsClientContract; request: any; @@ -59,6 +73,7 @@ const throwErrorIfNamespaceSpecified = (options: any) => { export class SpacesSavedObjectsClient implements SavedObjectsClientContract { private readonly client: SavedObjectsClientContract; + private readonly typeRegistry: ISavedObjectTypeRegistry; private readonly spaceId: string; private readonly types: string[]; private readonly spacesClient: ISpacesClient; @@ -70,6 +85,7 @@ export class SpacesSavedObjectsClient implements SavedObjectsClientContract { const spacesService = getSpacesService(); this.client = baseClient; + this.typeRegistry = typeRegistry; this.spacesClient = spacesService.createSpacesClient(request); this.spaceId = spacesService.getSpaceId(request); this.types = typeRegistry.getAllTypes().map((t) => t.name); @@ -219,36 +235,62 @@ export class SpacesSavedObjectsClient implements SavedObjectsClientContract { ) { throwErrorIfNamespaceSpecified(options); - let availableSpaces: string[] | undefined; + let availableSpacesPromise: Promise | undefined; const getAvailableSpaces = async () => { - if (!availableSpaces) { - try { - availableSpaces = await this.getSearchableSpaces([ALL_SPACES_ID]); - } catch (err) { + if (!availableSpacesPromise) { + availableSpacesPromise = this.getSearchableSpaces([ALL_SPACES_ID]).catch((err) => { if (Boom.isBoom(err) && err.output.payload.statusCode === 403) { - availableSpaces = []; // the user doesn't have access to any spaces + return []; // the user doesn't have access to any spaces } else { throw err; } - } + }); } - return availableSpaces; + return availableSpacesPromise; }; - const objectsToGet: SavedObjectsBulkGetObject[] = []; - for (const object of objects) { - const { namespaces } = object; - if (namespaces?.includes(ALL_SPACES_ID)) { - objectsToGet.push({ ...object, namespaces: await getAvailableSpaces() }); - } else { - objectsToGet.push(object); - } - } - - return await this.client.bulkGet(objectsToGet, { - ...options, - namespace: spaceIdToNamespace(this.spaceId), - }); + const expectedResults = await Promise.all( + objects.map>>(async (object) => { + const { namespaces, type } = object; + if (namespaces?.includes(ALL_SPACES_ID)) { + // If searching for an isolated object in all spaces, we may need to return a 400 error for consistency with the validation at the + // repository level. This is needed if there is only one space available *and* the user is authorized to access the object in that + // space; in that case, we don't want to unintentionally bypass the repository's validation by deconstructing the '*' identifier + // into all available spaces. + const tag = + !this.typeRegistry.isNamespaceAgnostic(type) && !this.typeRegistry.isShareable(type) + ? 'Left' + : 'Right'; + return { tag, value: { ...object, namespaces: await getAvailableSpaces() } }; + } else { + return { tag: 'Right', value: object }; + } + }) + ); + + const objectsToGet = expectedResults.map(({ value }) => value); + const { saved_objects: responseObjects } = objectsToGet.length + ? await this.client.bulkGet(objectsToGet, { + ...options, + namespace: spaceIdToNamespace(this.spaceId), + }) + : { saved_objects: [] }; + return { + saved_objects: expectedResults.map((expectedResult, i) => { + const actualResult = responseObjects[i]; + if (isLeft(expectedResult) && actualResult?.error?.statusCode !== 403) { + const { type, id } = expectedResult.value; + return ({ + type, + id, + error: SavedObjectsErrorHelpers.createBadRequestError( + '"namespaces" can only specify a single space when used with space-isolated types' + ).output.payload, + } as unknown) as SavedObject; + } + return actualResult; + }), + }; } /** From f224966be223eb8f563d3330d3ec6cd6ada51e1f Mon Sep 17 00:00:00 2001 From: Joe Portner <5295965+jportner@users.noreply.github.com> Date: Wed, 25 Aug 2021 16:01:22 -0400 Subject: [PATCH 7/8] Add API integration test cases --- .../common/suites/bulk_get.ts | 15 ++-- .../security_and_spaces/apis/bulk_get.ts | 79 +++++++++++++++---- .../security_only/apis/bulk_get.ts | 20 +++++ .../spaces_only/apis/bulk_get.ts | 17 +++- 4 files changed, 107 insertions(+), 24 deletions(-) diff --git a/x-pack/test/saved_object_api_integration/common/suites/bulk_get.ts b/x-pack/test/saved_object_api_integration/common/suites/bulk_get.ts index 26a693349496d..abfb1f12a2771 100644 --- a/x-pack/test/saved_object_api_integration/common/suites/bulk_get.ts +++ b/x-pack/test/saved_object_api_integration/common/suites/bulk_get.ts @@ -9,12 +9,7 @@ import expect from '@kbn/expect'; import { SuperTest } from 'supertest'; import { SAVED_OBJECT_TEST_CASES as CASES } from '../lib/saved_object_test_cases'; import { SPACES } from '../lib/spaces'; -import { - createRequest, - expectResponses, - getUrlPrefix, - getTestTitle, -} from '../lib/saved_object_test_utils'; +import { expectResponses, getUrlPrefix, getTestTitle } from '../lib/saved_object_test_utils'; import { ExpectResponseBody, TestCase, TestDefinition, TestSuite } from '../lib/types'; export interface BulkGetTestDefinition extends TestDefinition { @@ -22,6 +17,7 @@ export interface BulkGetTestDefinition extends TestDefinition { } export type BulkGetTestSuite = TestSuite; export interface BulkGetTestCase extends TestCase { + namespaces?: string[]; // used to define individual "object namespaces" string arrays, e.g., bulkGet across multiple namespaces failure?: 400 | 404; // only used for permitted response case } @@ -31,6 +27,12 @@ export const TEST_CASES: Record = Object.freeze({ DOES_NOT_EXIST, }); +const createRequest = ({ type, id, namespaces }: BulkGetTestCase) => ({ + type, + id, + ...(namespaces && { namespaces }), // individual "object namespaces" string array +}); + export function bulkGetTestSuiteFactory(esArchiver: any, supertest: SuperTest) { const expectSavedObjectForbidden = expectResponses.forbiddenTypes('bulk_get'); const expectResponseBody = ( @@ -49,6 +51,7 @@ export function bulkGetTestSuiteFactory(esArchiver: any, supertest: SuperTest { CASES.NAMESPACE_AGNOSTIC, { ...CASES.DOES_NOT_EXIST, ...fail404() }, ]; + const crossNamespace = [ + { + ...CASES.SINGLE_NAMESPACE_SPACE_2, + namespaces: ['x', 'y'], + ...fail400(), // cannot be searched for in multiple spaces + }, + { ...CASES.SINGLE_NAMESPACE_SPACE_2, namespaces: [SPACE_2_ID] }, // second try searches for it in a single other space, which is valid + { + ...CASES.MULTI_NAMESPACE_ISOLATED_ONLY_SPACE_1, + namespaces: [ALL_SPACES_ID], + ...fail400(), // cannot be searched for in multiple spaces + }, + { ...CASES.MULTI_NAMESPACE_ISOLATED_ONLY_SPACE_1, namespaces: [SPACE_1_ID] }, // second try searches for it in a single other space, which is valid + { ...CASES.MULTI_NAMESPACE_DEFAULT_AND_SPACE_1, namespaces: [SPACE_2_ID], ...fail404() }, + { ...CASES.MULTI_NAMESPACE_ALL_SPACES, namespaces: [SPACE_2_ID, 'x'] }, // unknown space is allowed / ignored + { ...CASES.MULTI_NAMESPACE_ALL_SPACES, namespaces: [ALL_SPACES_ID] }, // this is different than the same test case in the spaces_only and security_only suites, since MULTI_NAMESPACE_ONLY_SPACE_1 *may* return a 404 error to a partially authorized user + ]; const hiddenType = [{ ...CASES.HIDDEN, ...fail400() }]; - const allTypes = normalTypes.concat(hiddenType); - return { normalTypes, hiddenType, allTypes }; + const allTypes = [...normalTypes, ...crossNamespace, ...hiddenType]; + return { normalTypes, crossNamespace, hiddenType, allTypes }; }; export default function ({ getService }: FtrProviderContext) { @@ -58,13 +76,39 @@ export default function ({ getService }: FtrProviderContext) { supertest ); const createTests = (spaceId: string) => { - const { normalTypes, hiddenType, allTypes } = createTestCases(spaceId); + const { normalTypes, crossNamespace, hiddenType, allTypes } = createTestCases(spaceId); // use singleRequest to reduce execution time and/or test combined cases + const authorizedCommon = [ + createTestDefinitions(normalTypes, false, { singleRequest: true }), + createTestDefinitions(hiddenType, true), + ].flat(); + return { unauthorized: createTestDefinitions(allTypes, true), - authorized: [ - createTestDefinitions(normalTypes, false, { singleRequest: true }), - createTestDefinitions(hiddenType, true), + authorizedAtSpace: [ + authorizedCommon, + ...(() => { + const [authorized, unauthorized] = crossNamespace.reduce< + [BulkGetTestCase[], BulkGetTestCase[]] + >( + ([left, right], cur) => { + if (cur.namespaces.some((x) => ![ALL_SPACES_ID, spaceId].includes(x))) { + return [left, [...right, cur]]; + } + return [[...left, cur], right]; + }, + [[], []] + ); + return [ + ...createTestDefinitions(authorized, false, { singleRequest: true }), + ...createTestDefinitions(unauthorized, true), + ]; + })(), + createTestDefinitions(allTypes, true, { singleRequest: true }), + ].flat(), + authorizedEverywhere: [ + authorizedCommon, + createTestDefinitions(crossNamespace, false, { singleRequest: true }), createTestDefinitions(allTypes, true, { singleRequest: true, responseBodyOverride: expectSavedObjectForbidden(['hiddentype']), @@ -77,7 +121,9 @@ export default function ({ getService }: FtrProviderContext) { describe('_bulk_get', () => { getTestScenarios().securityAndSpaces.forEach(({ spaceId, users }) => { const suffix = ` within the ${spaceId} space`; - const { unauthorized, authorized, superuser } = createTests(spaceId); + const { unauthorized, authorizedAtSpace, authorizedEverywhere, superuser } = createTests( + spaceId + ); const _addTests = (user: TestUser, tests: BulkGetTestDefinition[]) => { addTests(`${user.description}${suffix}`, { user, spaceId, tests }); }; @@ -85,16 +131,15 @@ export default function ({ getService }: FtrProviderContext) { [users.noAccess, users.legacyAll, users.allAtOtherSpace].forEach((user) => { _addTests(user, unauthorized); }); - [ - users.dualAll, - users.dualRead, - users.allGlobally, - users.readGlobally, - users.allAtSpace, - users.readAtSpace, - ].forEach((user) => { - _addTests(user, authorized); + + [users.allAtSpace, users.readAtSpace].forEach((user) => { + _addTests(user, authorizedAtSpace); }); + + [users.dualAll, users.dualRead, users.allGlobally, users.readGlobally].forEach((user) => { + _addTests(user, authorizedEverywhere); + }); + _addTests(users.superuser, superuser); }); }); diff --git a/x-pack/test/saved_object_api_integration/security_only/apis/bulk_get.ts b/x-pack/test/saved_object_api_integration/security_only/apis/bulk_get.ts index 18edb7502c65a..4aa722bfc6b07 100644 --- a/x-pack/test/saved_object_api_integration/security_only/apis/bulk_get.ts +++ b/x-pack/test/saved_object_api_integration/security_only/apis/bulk_get.ts @@ -5,6 +5,7 @@ * 2.0. */ +import { SPACES, ALL_SPACES_ID } from '../../common/lib/spaces'; import { testCaseFailures, getTestScenarios } from '../../common/lib/saved_object_test_utils'; import { TestUser } from '../../common/lib/types'; import { FtrProviderContext } from '../../common/ftr_provider_context'; @@ -14,6 +15,10 @@ import { BulkGetTestDefinition, } from '../../common/suites/bulk_get'; +const { + SPACE_1: { spaceId: SPACE_1_ID }, + SPACE_2: { spaceId: SPACE_2_ID }, +} = SPACES; const { fail400, fail404 } = testCaseFailures; const createTestCases = () => { @@ -31,6 +36,21 @@ const createTestCases = () => { { ...CASES.MULTI_NAMESPACE_ISOLATED_ONLY_SPACE_1, ...fail404() }, CASES.NAMESPACE_AGNOSTIC, { ...CASES.DOES_NOT_EXIST, ...fail404() }, + { + ...CASES.SINGLE_NAMESPACE_SPACE_2, + namespaces: ['x', 'y'], + ...fail400(), // cannot be searched for in multiple spaces + }, + { ...CASES.SINGLE_NAMESPACE_SPACE_2, namespaces: [SPACE_2_ID] }, // second try searches for it in a single other space, which is valid + { + ...CASES.MULTI_NAMESPACE_ISOLATED_ONLY_SPACE_1, + namespaces: [ALL_SPACES_ID], + ...fail400(), // cannot be searched for in multiple spaces + }, + { ...CASES.MULTI_NAMESPACE_ISOLATED_ONLY_SPACE_1, namespaces: [SPACE_1_ID] }, // second try searches for it in a single other space, which is valid + { ...CASES.MULTI_NAMESPACE_DEFAULT_AND_SPACE_1, namespaces: [SPACE_2_ID], ...fail404() }, + { ...CASES.MULTI_NAMESPACE_ALL_SPACES, namespaces: [SPACE_2_ID, 'x'] }, // unknown space is allowed / ignored + { ...CASES.MULTI_NAMESPACE_ONLY_SPACE_1, namespaces: [ALL_SPACES_ID] }, ]; const hiddenType = [{ ...CASES.HIDDEN, ...fail400() }]; const allTypes = normalTypes.concat(hiddenType); diff --git a/x-pack/test/saved_object_api_integration/spaces_only/apis/bulk_get.ts b/x-pack/test/saved_object_api_integration/spaces_only/apis/bulk_get.ts index e1d0243377b8e..41fa4749cc48e 100644 --- a/x-pack/test/saved_object_api_integration/spaces_only/apis/bulk_get.ts +++ b/x-pack/test/saved_object_api_integration/spaces_only/apis/bulk_get.ts @@ -5,7 +5,7 @@ * 2.0. */ -import { SPACES } from '../../common/lib/spaces'; +import { SPACES, ALL_SPACES_ID } from '../../common/lib/spaces'; import { testCaseFailures, getTestScenarios } from '../../common/lib/saved_object_test_utils'; import { FtrProviderContext } from '../../common/ftr_provider_context'; import { bulkGetTestSuiteFactory, TEST_CASES as CASES } from '../../common/suites/bulk_get'; @@ -38,6 +38,21 @@ const createTestCases = (spaceId: string) => [ CASES.NAMESPACE_AGNOSTIC, { ...CASES.HIDDEN, ...fail400() }, { ...CASES.DOES_NOT_EXIST, ...fail404() }, + { + ...CASES.SINGLE_NAMESPACE_SPACE_2, + namespaces: ['x', 'y'], + ...fail400(), // cannot be searched for in multiple spaces + }, + { ...CASES.SINGLE_NAMESPACE_SPACE_2, namespaces: [SPACE_2_ID] }, // second try searches for it in a single other space, which is valid + { + ...CASES.MULTI_NAMESPACE_ISOLATED_ONLY_SPACE_1, + namespaces: [ALL_SPACES_ID], + ...fail400(), // cannot be searched for in multiple spaces + }, + { ...CASES.MULTI_NAMESPACE_ISOLATED_ONLY_SPACE_1, namespaces: [SPACE_1_ID] }, // second try searches for it in a single other space, which is valid + { ...CASES.MULTI_NAMESPACE_DEFAULT_AND_SPACE_1, namespaces: [SPACE_2_ID], ...fail404() }, + { ...CASES.MULTI_NAMESPACE_ALL_SPACES, namespaces: [SPACE_2_ID, 'x'] }, // unknown space is allowed / ignored + { ...CASES.MULTI_NAMESPACE_ONLY_SPACE_1, namespaces: [ALL_SPACES_ID] }, ]; export default function ({ getService }: FtrProviderContext) { From fab5c21cbd700ad2644d93c3deff7c3cc3f54eea Mon Sep 17 00:00:00 2001 From: Joe Portner <5295965+jportner@users.noreply.github.com> Date: Thu, 26 Aug 2021 09:08:43 -0400 Subject: [PATCH 8/8] PR review feedback --- .../service/lib/internal_utils.test.ts | 1 + .../service/lib/internal_utils.ts | 3 +- .../saved_objects/service/lib/repository.ts | 2 +- .../spaces_saved_objects_client.test.ts | 36 +++++++------------ .../spaces_saved_objects_client.ts | 5 ++- .../security_and_spaces/apis/bulk_get.ts | 34 +++++++++--------- 6 files changed, 35 insertions(+), 46 deletions(-) diff --git a/src/core/server/saved_objects/service/lib/internal_utils.test.ts b/src/core/server/saved_objects/service/lib/internal_utils.test.ts index 200271964b3cb..a0b8581e582f6 100644 --- a/src/core/server/saved_objects/service/lib/internal_utils.test.ts +++ b/src/core/server/saved_objects/service/lib/internal_utils.test.ts @@ -305,6 +305,7 @@ describe('#rawDocExistsInNamespaces', () => { expect(rawDocExistsInNamespaces(registry, docInNoSpace, ['default'])).toBe(false); expect(rawDocExistsInNamespaces(registry, docInDefaultSpace, ['some-space'])).toBe(false); expect(rawDocExistsInNamespaces(registry, docInNoSpace, ['some-space'])).toBe(false); + expect(rawDocExistsInNamespaces(registry, docInNoSpace, ['*'])).toBe(false); expect(rawDocExistsInNamespaces(registry, docInDefaultSpace, [])).toBe(false); expect(rawDocExistsInNamespaces(registry, docInSomeSpace, [])).toBe(false); expect(rawDocExistsInNamespaces(registry, docInAllSpaces, [])).toBe(false); diff --git a/src/core/server/saved_objects/service/lib/internal_utils.ts b/src/core/server/saved_objects/service/lib/internal_utils.ts index 9f8b2d44ae325..ed6ede0fe6d49 100644 --- a/src/core/server/saved_objects/service/lib/internal_utils.ts +++ b/src/core/server/saved_objects/service/lib/internal_utils.ts @@ -172,7 +172,8 @@ export function rawDocExistsInNamespaces( if (namespacesToCheck.size === 0 || existingNamespaces.length === 0) { return false; - } else if (namespacesToCheck.has(ALL_NAMESPACES_STRING)) { + } + if (namespacesToCheck.has(ALL_NAMESPACES_STRING)) { return true; } diff --git a/src/core/server/saved_objects/service/lib/repository.ts b/src/core/server/saved_objects/service/lib/repository.ts index 11b95b6f9b1de..c2ae6ebab00e9 100644 --- a/src/core/server/saved_objects/service/lib/repository.ts +++ b/src/core/server/saved_objects/service/lib/repository.ts @@ -1004,7 +1004,7 @@ export class SavedObjectsRepository { const bulkGetDocs = expectedBulkGetResults .filter(isRight) .map(({ value: { type, id, fields, namespaces } }) => ({ - _id: this._serializer.generateRawId(getNamespaceId(namespaces), type, id), + _id: this._serializer.generateRawId(getNamespaceId(namespaces), type, id), // the namespace prefix is only used for single-namespace object types _index: this.getIndexForType(type), _source: { includes: includedFields(type, fields) }, })); diff --git a/x-pack/plugins/spaces/server/saved_objects/spaces_saved_objects_client.test.ts b/x-pack/plugins/spaces/server/saved_objects/spaces_saved_objects_client.test.ts index 25952b8b0eb44..85c6ce74763b2 100644 --- a/x-pack/plugins/spaces/server/saved_objects/spaces_saved_objects_client.test.ts +++ b/x-pack/plugins/spaces/server/saved_objects/spaces_saved_objects_client.test.ts @@ -165,12 +165,9 @@ const ERROR_NAMESPACE_SPECIFIED = 'Spaces currently determines the namespaces'; { type: 'foo', id: '1', key: 'val' }, { type: 'bar', id: '2', key: 'val' }, { type: 'baz', id: '3', key: 'val' }, // this should be replaced with a 400 error - { type: 'foo', id: '4', error: { statusCode: 403 } }, - { type: 'bar', id: '5', error: { statusCode: 403 } }, - { type: 'baz', id: '6', error: { statusCode: 403 } }, // this should be not be replaced with a 400 error because it has a 403 error which is higher priority - { type: 'foo', id: '7', key: 'val' }, - { type: 'bar', id: '8', key: 'val' }, - { type: 'baz', id: '9', key: 'val' }, // this should not be replaced with a 400 error because the user did not search for it in '*' all spaces + { type: 'foo', id: '4', key: 'val' }, + { type: 'bar', id: '5', key: 'val' }, + { type: 'baz', id: '6', key: 'val' }, // this should not be replaced with a 400 error because the user did not search for it in '*' all spaces ] as unknown) as SavedObject[], }); @@ -178,12 +175,9 @@ const ERROR_NAMESPACE_SPECIFIED = 'Spaces currently determines the namespaces'; { type: 'foo', id: '1', namespaces: ['*', 'this-is-ignored'] }, { type: 'bar', id: '2', namespaces: ['*', 'this-is-ignored'] }, { type: 'baz', id: '3', namespaces: ['*', 'this-is-ignored'] }, - { type: 'foo', id: '4', namespaces: ['*'] }, - { type: 'bar', id: '5', namespaces: ['*'] }, - { type: 'baz', id: '6', namespaces: ['*'] }, - { type: 'foo', id: '7', namespaces: ['another-space'] }, - { type: 'bar', id: '8', namespaces: ['another-space'] }, - { type: 'baz', id: '9', namespaces: ['another-space'] }, + { type: 'foo', id: '4', namespaces: ['another-space'] }, + { type: 'bar', id: '5', namespaces: ['another-space'] }, + { type: 'baz', id: '6', namespaces: ['another-space'] }, ]; const result = await client.bulkGet(objects); @@ -197,25 +191,19 @@ const ERROR_NAMESPACE_SPECIFIED = 'Spaces currently determines the namespaces'; '"namespaces" can only specify a single space when used with space-isolated types' ).output.payload, }, - { type: 'foo', id: '4', error: { statusCode: 403 } }, - { type: 'bar', id: '5', error: { statusCode: 403 } }, - { type: 'baz', id: '6', error: { statusCode: 403 } }, - { type: 'foo', id: '7', key: 'val' }, - { type: 'bar', id: '8', key: 'val' }, - { type: 'baz', id: '9', key: 'val' }, + { type: 'foo', id: '4', key: 'val' }, + { type: 'bar', id: '5', key: 'val' }, + { type: 'baz', id: '6', key: 'val' }, ]); expect(baseClient.bulkGet).toHaveBeenCalledWith( [ { type: 'foo', id: '1', namespaces: ['available-space-a', 'available-space-b'] }, { type: 'bar', id: '2', namespaces: ['available-space-a', 'available-space-b'] }, { type: 'baz', id: '3', namespaces: ['available-space-a', 'available-space-b'] }, - { type: 'foo', id: '4', namespaces: ['available-space-a', 'available-space-b'] }, - { type: 'bar', id: '5', namespaces: ['available-space-a', 'available-space-b'] }, - { type: 'baz', id: '6', namespaces: ['available-space-a', 'available-space-b'] }, // even if another space doesn't exist, it can be specified explicitly - { type: 'foo', id: '7', namespaces: ['another-space'] }, - { type: 'bar', id: '8', namespaces: ['another-space'] }, - { type: 'baz', id: '9', namespaces: ['another-space'] }, + { type: 'foo', id: '4', namespaces: ['another-space'] }, + { type: 'bar', id: '5', namespaces: ['another-space'] }, + { type: 'baz', id: '6', namespaces: ['another-space'] }, ], { namespace: currentSpace.expectedNamespace } ); diff --git a/x-pack/plugins/spaces/server/saved_objects/spaces_saved_objects_client.ts b/x-pack/plugins/spaces/server/saved_objects/spaces_saved_objects_client.ts index 2d662f9e2ba67..6cfd784042317 100644 --- a/x-pack/plugins/spaces/server/saved_objects/spaces_saved_objects_client.ts +++ b/x-pack/plugins/spaces/server/saved_objects/spaces_saved_objects_client.ts @@ -262,9 +262,8 @@ export class SpacesSavedObjectsClient implements SavedObjectsClientContract { ? 'Left' : 'Right'; return { tag, value: { ...object, namespaces: await getAvailableSpaces() } }; - } else { - return { tag: 'Right', value: object }; } + return { tag: 'Right', value: object }; }) ); @@ -278,7 +277,7 @@ export class SpacesSavedObjectsClient implements SavedObjectsClientContract { return { saved_objects: expectedResults.map((expectedResult, i) => { const actualResult = responseObjects[i]; - if (isLeft(expectedResult) && actualResult?.error?.statusCode !== 403) { + if (isLeft(expectedResult)) { const { type, id } = expectedResult.value; return ({ type, diff --git a/x-pack/test/saved_object_api_integration/security_and_spaces/apis/bulk_get.ts b/x-pack/test/saved_object_api_integration/security_and_spaces/apis/bulk_get.ts index e027e69ccdb1d..bfea8e558010b 100644 --- a/x-pack/test/saved_object_api_integration/security_and_spaces/apis/bulk_get.ts +++ b/x-pack/test/saved_object_api_integration/security_and_spaces/apis/bulk_get.ts @@ -82,28 +82,28 @@ export default function ({ getService }: FtrProviderContext) { createTestDefinitions(normalTypes, false, { singleRequest: true }), createTestDefinitions(hiddenType, true), ].flat(); + const crossNamespaceAuthorizedAtSpace = crossNamespace.reduce<{ + authorized: BulkGetTestCase[]; + unauthorized: BulkGetTestCase[]; + }>( + ({ authorized, unauthorized }, cur) => { + // A user who is only authorized in a single space will be authorized to execute some of the cross-namespace test cases, but not all + if (cur.namespaces.some((x) => ![ALL_SPACES_ID, spaceId].includes(x))) { + return { authorized, unauthorized: [...unauthorized, cur] }; + } + return { authorized: [...authorized, cur], unauthorized }; + }, + { authorized: [], unauthorized: [] } + ); return { unauthorized: createTestDefinitions(allTypes, true), authorizedAtSpace: [ authorizedCommon, - ...(() => { - const [authorized, unauthorized] = crossNamespace.reduce< - [BulkGetTestCase[], BulkGetTestCase[]] - >( - ([left, right], cur) => { - if (cur.namespaces.some((x) => ![ALL_SPACES_ID, spaceId].includes(x))) { - return [left, [...right, cur]]; - } - return [[...left, cur], right]; - }, - [[], []] - ); - return [ - ...createTestDefinitions(authorized, false, { singleRequest: true }), - ...createTestDefinitions(unauthorized, true), - ]; - })(), + createTestDefinitions(crossNamespaceAuthorizedAtSpace.authorized, false, { + singleRequest: true, + }), + createTestDefinitions(crossNamespaceAuthorizedAtSpace.unauthorized, true), createTestDefinitions(allTypes, true, { singleRequest: true }), ].flat(), authorizedEverywhere: [