Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bulkGet saved objects across spaces #109967

Merged
merged 12 commits into from
Aug 26, 2021
Merged
10 changes: 10 additions & 0 deletions docs/api/saved-objects/bulk_get.asciidoc
Original file line number Diff line number Diff line change
Expand Up @@ -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 <<xpack-spaces,spaces>> 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

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,5 +17,6 @@ export interface SavedObjectsBulkGetObject
| --- | --- | --- |
| [fields](./kibana-plugin-core-server.savedobjectsbulkgetobject.fields.md) | <code>string[]</code> | SavedObject fields to include in the response |
| [id](./kibana-plugin-core-server.savedobjectsbulkgetobject.id.md) | <code>string</code> | |
| [namespaces](./kibana-plugin-core-server.savedobjectsbulkgetobject.namespaces.md) | <code>string[]</code> | 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 <code>namespaceType: 'multiple'</code>): this option can be used to specify one or more spaces, including the "All spaces" identifier (<code>'*'</code>). \* For isolated object types (registered with <code>namespaceType: 'single'</code> or <code>namespaceType: 'multiple-isolated'</code>): this option can only be used to specify a single space, and the "All spaces" identifier (<code>'*'</code>) is not allowed. \* For global object types (registered with <code>namespaceType: 'agnostic'</code>): this option cannot be used. |
| [type](./kibana-plugin-core-server.savedobjectsbulkgetobject.type.md) | <code>string</code> | |

Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
<!-- Do not edit this file. It is automatically generated by API Documenter. -->

[Home](./index.md) &gt; [kibana-plugin-core-server](./kibana-plugin-core-server.md) &gt; [SavedObjectsBulkGetObject](./kibana-plugin-core-server.savedobjectsbulkgetobject.md) &gt; [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.

<b>Signature:</b>

```typescript
namespaces?: string[];
```
1 change: 1 addition & 0 deletions src/core/server/saved_objects/routes/bulk_get.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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())),
})
),
},
Expand Down
84 changes: 84 additions & 0 deletions src/core/server/saved_objects/service/lib/internal_utils.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import {
getBulkOperationError,
getSavedObjectFromSource,
rawDocExistsInNamespace,
rawDocExistsInNamespaces,
} from './internal_utils';
import { ALL_NAMESPACES_STRING } from './utils';

Expand Down Expand Up @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a remark: We already talked about it in a similar function from a previous PR, but I still don't like the way single-NS docs are short-circuiting these functions, as it could lead to errors if a developer decides to use it elsewhere than in a part of the code where we're already assured that the fetched single-NS objects are of the correct requested space.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll one-up you, I don't like single-NS docs at all 😄

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', () => {
jportner marked this conversation as resolved.
Show resolved Hide resolved
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);
});
});
});
37 changes: 37 additions & 0 deletions src/core/server/saved_objects/service/lib/internal_utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT: I don't think there's any need to convert to a set here? Can't we just work with the initial array?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, had the same thought. The only corner case I can imagine where it'd be beneficial to have a Set is if we have large existingNamespaces and namespaces lists that have intersection only at the end of these lists (and bulkGet is large enough to have a significant compound effect), but it probably doesn't make sense to cater for that.

Having a set here will not completely protect us from a malicious actor anyway, they can just do something like this...., for every ID:

const namespaces = [...Array.from({ length: 1000000 }).map(() => Math.random().toString()), 'valid-ns']

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The purpose of the Set here is to reduce the time complexity, this takes us down from O(n^2) to O(n). This is because line 179 below loops through existingNamespaces and checks to see if any existing space is present in namespacesToCheck.

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));
jportner marked this conversation as resolved.
Show resolved Hide resolved
}
93 changes: 67 additions & 26 deletions src/core/server/saved_objects/service/lib/repository.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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) => {
Expand Down Expand Up @@ -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]);
Expand All @@ -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)],
});
};

Expand All @@ -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 () => {
Expand Down
Loading