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] 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; + }), + }; } /**