Skip to content

Commit

Permalink
Handle bad requests to bulkGet in the SecureSavedObjectsClient
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@jportner
jportner committed Aug 25, 2021
1 parent e90b900 commit 5e5e00c
Show file tree
Hide file tree
Showing 2 changed files with 129 additions and 54 deletions.
97 changes: 65 additions & 32 deletions x-pack/plugins/spaces/server/saved_objects/spaces_saved_objects_client.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -69,14 +48,21 @@ 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,
baseClient,
getSpacesService: () => spacesService,
typeRegistry,
});
return { client, baseClient, spacesClient };
return { client, baseClient, spacesClient, typeRegistry };
};

describe('#get', () => {
Expand Down Expand Up @@ -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,
Expand All @@ -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 }
);
Expand All @@ -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: ['*'] },
Expand Down
86 changes: 64 additions & 22 deletions x-pack/plugins/spaces/server/saved_objects/spaces_saved_objects_client.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import Boom from '@hapi/boom';

import type {
ISavedObjectTypeRegistry,
SavedObject,
SavedObjectsBaseOptions,
SavedObjectsBulkCreateObject,
SavedObjectsBulkGetObject,
Expand Down Expand Up @@ -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<L> {
tag: 'Left';
value: L;
}

interface Right<R> {
tag: 'Right';
value: R;
}

type Either<L = unknown, R = L> = Left<L> | Right<R>;
const isLeft = <L, R>(either: Either<L, R>): either is Left<L> => either.tag === 'Left';

interface SpacesSavedObjectsClientOptions {
baseClient: SavedObjectsClientContract;
request: any;
Expand All @@ -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;
Expand All @@ -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);
Expand Down Expand Up @@ -219,36 +235,62 @@ export class SpacesSavedObjectsClient implements SavedObjectsClientContract {
) {
throwErrorIfNamespaceSpecified(options);

let availableSpaces: string[] | undefined;
let availableSpacesPromise: Promise<string[]> | 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<T>(objectsToGet, {
...options,
namespace: spaceIdToNamespace(this.spaceId),
});
const expectedResults = await Promise.all(
objects.map<Promise<Either<SavedObjectsBulkGetObject>>>(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<T>(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<T>;
}
return actualResult;
}),
};
}

/**
Expand Down

0 comments on commit 5e5e00c

Please sign in to comment.