Change bulkUpdate method to allow per-object namespaces

Includes docs changes and integration tests.
elastic · Aug 20, 2020 · 341052a · 341052a
1 parent fa6ae1c
commit 341052a
Show file tree

Hide file tree

Showing 13 changed files with 266 additions and 99 deletions.
diff --git a/...velopment/core/server/kibana-plugin-core-server.savedobjectsbulkupdateobject.md b/...velopment/core/server/kibana-plugin-core-server.savedobjectsbulkupdateobject.md
@@ -17,5 +17,6 @@ export interface SavedObjectsBulkUpdateObject<T = unknown> extends Pick<SavedObj
 |  --- | --- | --- |
 |  [attributes](./kibana-plugin-core-server.savedobjectsbulkupdateobject.attributes.md) | <code>Partial&lt;T&gt;</code> | The data for a Saved Object is stored as an object in the <code>attributes</code> property. |
 |  [id](./kibana-plugin-core-server.savedobjectsbulkupdateobject.id.md) | <code>string</code> | The ID of this Saved Object, guaranteed to be unique for all objects of the same <code>type</code> |
+|  [namespace](./kibana-plugin-core-server.savedobjectsbulkupdateobject.namespace.md) | <code>string</code> | Optional namespace string to use for this document. If this is defined, it will supersede the namespace ID that is in [SavedObjectsUpdateOptions](./kibana-plugin-core-server.savedobjectsupdateoptions.md)<!-- -->.<!-- -->Note: the default namespace's string representation is <code>'default'</code>, and its ID representation is <code>undefined</code>. |
 |  [type](./kibana-plugin-core-server.savedobjectsbulkupdateobject.type.md) | <code>string</code> | The type of this Saved Object. Each plugin can define it's own custom Saved Object types. |
 
diff --git a/...core/server/kibana-plugin-core-server.savedobjectsbulkupdateobject.namespace.md b/...core/server/kibana-plugin-core-server.savedobjectsbulkupdateobject.namespace.md
@@ -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; [SavedObjectsBulkUpdateObject](./kibana-plugin-core-server.savedobjectsbulkupdateobject.md) &gt; [namespace](./kibana-plugin-core-server.savedobjectsbulkupdateobject.namespace.md)
+
+## SavedObjectsBulkUpdateObject.namespace property
+
+Optional namespace string to use for this document. If this is defined, it will supersede the namespace ID that is in [SavedObjectsUpdateOptions](./kibana-plugin-core-server.savedobjectsupdateoptions.md)<!-- -->.
+
+Note: the default namespace's string representation is `'default'`<!-- -->, and its ID representation is `undefined`<!-- -->.
+
+<b>Signature:</b>
+
+```typescript
+namespace?: string;
+```
diff --git a/src/core/server/saved_objects/routes/bulk_update.ts b/src/core/server/saved_objects/routes/bulk_update.ts
@@ -40,6 +40,7 @@ export const registerBulkUpdateRoute = (router: IRouter) => {
                 })
               )
             ),
+            namespace: schema.maybe(schema.string({ minLength: 1 })),
           })
         ),
       },

diff --git a/src/core/server/saved_objects/service/lib/repository.test.js b/src/core/server/saved_objects/service/lib/repository.test.js
@@ -154,26 +154,35 @@ describe('SavedObjectsRepository', () => {
     validateDoc: jest.fn(),
   });
 
-  const getMockGetResponse = ({ type, id, references, namespace }) => ({
-    // NOTE: Elasticsearch returns more fields (_index, _type) but the SavedObjectsRepository method ignores these
-    found: true,
-    _id: `${registry.isSingleNamespace(type) && namespace ? `${namespace}:` : ''}${type}:${id}`,
-    ...mockVersionProps,
-    _source: {
-      ...(registry.isSingleNamespace(type) && { namespace }),
-      ...(registry.isMultiNamespace(type) && { namespaces: [namespace ?? 'default'] }),
-      type,
-      [type]: { title: 'Testing' },
-      references,
-      specialProperty: 'specialValue',
-      ...mockTimestampFields,
-    },
-  });
+  const getMockGetResponse = ({ type, id, references, namespace: objectNamespace }, namespace) => {
+    const namespaceId =
+      // eslint-disable-next-line no-nested-ternary
+      objectNamespace !== undefined
+        ? objectNamespace !== 'default'
+          ? objectNamespace
+          : undefined
+        : namespace;
+    return {
+      // NOTE: Elasticsearch returns more fields (_index, _type) but the SavedObjectsRepository method ignores these
+      found: true,
+      _id: `${
+        registry.isSingleNamespace(type) && namespaceId ? `${namespaceId}:` : ''
+      }${type}:${id}`,
+      ...mockVersionProps,
+      _source: {
+        ...(registry.isSingleNamespace(type) && { namespace: namespaceId }),
+        ...(registry.isMultiNamespace(type) && { namespaces: [namespaceId ?? 'default'] }),
+        type,
+        [type]: { title: 'Testing' },
+        references,
+        specialProperty: 'specialValue',
+        ...mockTimestampFields,
+      },
+    };
+  };
 
   const getMockMgetResponse = (objects, namespace) => ({
-    docs: objects.map((obj) =>
-      obj.found === false ? obj : getMockGetResponse({ ...obj, namespace })
-    ),
+    docs: objects.map((obj) => (obj.found === false ? obj : getMockGetResponse(obj, namespace))),
   });
 
   expect.extend({
@@ -1311,29 +1320,57 @@ describe('SavedObjectsRepository', () => {
         const getId = (type, id) => `${namespace}:${type}:${id}`;
         await bulkUpdateSuccess([obj1, obj2], { namespace });
         expectClientCallArgsAction([obj1, obj2], { method: 'update', getId });
+
+        jest.clearAllMocks();
+        // test again with object namespace string that supersedes the operation's namespace ID
+        await bulkUpdateSuccess([
+          { ...obj1, namespace },
+          { ...obj2, namespace },
+        ]);
+        expectClientCallArgsAction([obj1, obj2], { method: 'update', getId });
       });
 
       it(`doesn't prepend namespace to the id when providing no namespace for single-namespace type`, async () => {
         const getId = (type, id) => `${type}:${id}`;
         await bulkUpdateSuccess([obj1, obj2]);
         expectClientCallArgsAction([obj1, obj2], { method: 'update', getId });
+
+        jest.clearAllMocks();
+        // test again with object namespace string that supersedes the operation's namespace ID
+        await bulkUpdateSuccess(
+          [
+            { ...obj1, namespace: 'default' },
+            { ...obj2, namespace: 'default' },
+          ],
+          { namespace }
+        );
+        expectClientCallArgsAction([obj1, obj2], { method: 'update', getId });
       });
 
       it(`doesn't prepend namespace to the id when not using single-namespace type`, async () => {
         const getId = (type, id) => `${type}:${id}`;
-        const objects1 = [{ ...obj1, type: NAMESPACE_AGNOSTIC_TYPE }];
-        await bulkUpdateSuccess(objects1, { namespace });
-        expectClientCallArgsAction(objects1, { method: 'update', getId });
-        client.bulk.mockClear();
         const overrides = {
           // bulkUpdate uses a preflight `get` request for multi-namespace saved objects, and specifies that version on `update`
           // we aren't testing for this here, but we need to include Jest assertions so this test doesn't fail
           if_primary_term: expect.any(Number),
           if_seq_no: expect.any(Number),
         };
-        const objects2 = [{ ...obj2, type: MULTI_NAMESPACE_TYPE }];
-        await bulkUpdateSuccess(objects2, { namespace });
-        expectClientCallArgsAction(objects2, { method: 'update', getId, overrides }, 2);
+        const _obj1 = { ...obj1, type: NAMESPACE_AGNOSTIC_TYPE };
+        const _obj2 = { ...obj2, type: MULTI_NAMESPACE_TYPE };
+
+        await bulkUpdateSuccess([_obj1], { namespace });
+        expectClientCallArgsAction([_obj1], { method: 'update', getId });
+        client.bulk.mockClear();
+        await bulkUpdateSuccess([_obj2], { namespace });
+        expectClientCallArgsAction([_obj2], { method: 'update', getId, overrides }, 2);
+
+        jest.clearAllMocks();
+        // test again with object namespace string that supersedes the operation's namespace ID
+        await bulkUpdateSuccess([{ ..._obj1, namespace }]);
+        expectClientCallArgsAction([_obj1], { method: 'update', getId });
+        client.bulk.mockClear();
+        await bulkUpdateSuccess([{ ..._obj2, namespace }]);
+        expectClientCallArgsAction([_obj2], { method: 'update', getId, overrides }, 2);
       });
     });
 
@@ -1684,11 +1721,7 @@ describe('SavedObjectsRepository', () => {
       });
 
       it(`throws when there is a conflict with an existing multi-namespace saved object (get)`, async () => {
-        const response = getMockGetResponse({
-          type: MULTI_NAMESPACE_TYPE,
-          id,
-          namespace: 'bar-namespace',
-        });
+        const response = getMockGetResponse({ type: MULTI_NAMESPACE_TYPE, id }, 'bar-namespace');
         client.get.mockResolvedValueOnce(
           elasticsearchClientMock.createSuccessTransportRequestPromise(response)
         );
@@ -1785,7 +1818,7 @@ describe('SavedObjectsRepository', () => {
 
     const deleteSuccess = async (type, id, options) => {
       if (registry.isMultiNamespace(type)) {
-        const mockGetResponse = getMockGetResponse({ type, id, namespace: options?.namespace });
+        const mockGetResponse = getMockGetResponse({ type, id }, options?.namespace);
         client.get.mockResolvedValueOnce(
           elasticsearchClientMock.createSuccessTransportRequestPromise(mockGetResponse)
         );
@@ -1911,7 +1944,7 @@ describe('SavedObjectsRepository', () => {
       });
 
       it(`throws when the type is multi-namespace and the document exists, but not in this namespace`, async () => {
-        const response = getMockGetResponse({ type: MULTI_NAMESPACE_TYPE, id, namespace });
+        const response = getMockGetResponse({ type: MULTI_NAMESPACE_TYPE, id }, namespace);
         client.get.mockResolvedValueOnce(
           elasticsearchClientMock.createSuccessTransportRequestPromise(response)
         );
@@ -2440,7 +2473,7 @@ describe('SavedObjectsRepository', () => {
     const namespace = 'foo-namespace';
 
     const getSuccess = async (type, id, options) => {
-      const response = getMockGetResponse({ type, id, namespace: options?.namespace });
+      const response = getMockGetResponse({ type, id }, options?.namespace);
       client.get.mockResolvedValueOnce(
         elasticsearchClientMock.createSuccessTransportRequestPromise(response)
       );
@@ -2529,7 +2562,7 @@ describe('SavedObjectsRepository', () => {
       });
 
       it(`throws when type is multi-namespace and the document exists, but not in this namespace`, async () => {
-        const response = getMockGetResponse({ type: MULTI_NAMESPACE_TYPE, id, namespace });
+        const response = getMockGetResponse({ type: MULTI_NAMESPACE_TYPE, id }, namespace);
         client.get.mockResolvedValueOnce(
           elasticsearchClientMock.createSuccessTransportRequestPromise(response)
         );
@@ -2579,7 +2612,7 @@ describe('SavedObjectsRepository', () => {
     const incrementCounterSuccess = async (type, id, field, options) => {
       const isMultiNamespace = registry.isMultiNamespace(type);
       if (isMultiNamespace) {
-        const response = getMockGetResponse({ type, id, namespace: options?.namespace });
+        const response = getMockGetResponse({ type, id }, options?.namespace);
         client.get.mockResolvedValueOnce(
           elasticsearchClientMock.createSuccessTransportRequestPromise(response)
         );
@@ -2716,11 +2749,7 @@ describe('SavedObjectsRepository', () => {
       });
 
       it(`throws when there is a conflict with an existing multi-namespace saved object (get)`, async () => {
-        const response = getMockGetResponse({
-          type: MULTI_NAMESPACE_TYPE,
-          id,
-          namespace: 'bar-namespace',
-        });
+        const response = getMockGetResponse({ type: MULTI_NAMESPACE_TYPE, id }, 'bar-namespace');
         client.get.mockResolvedValueOnce(
           elasticsearchClientMock.createSuccessTransportRequestPromise(response)
         );
@@ -3152,7 +3181,7 @@ describe('SavedObjectsRepository', () => {
 
     const updateSuccess = async (type, id, attributes, options) => {
       if (registry.isMultiNamespace(type)) {
-        const mockGetResponse = getMockGetResponse({ type, id, namespace: options?.namespace });
+        const mockGetResponse = getMockGetResponse({ type, id }, options?.namespace);
         client.get.mockResolvedValueOnce(
           elasticsearchClientMock.createSuccessTransportRequestPromise(mockGetResponse)
         );
@@ -3349,7 +3378,7 @@ describe('SavedObjectsRepository', () => {
       });
 
       it(`throws when type is multi-namespace and the document exists, but not in this namespace`, async () => {
-        const response = getMockGetResponse({ type: MULTI_NAMESPACE_TYPE, id, namespace });
+        const response = getMockGetResponse({ type: MULTI_NAMESPACE_TYPE, id }, namespace);
         client.get.mockResolvedValueOnce(
           elasticsearchClientMock.createSuccessTransportRequestPromise(response)
         );

diff --git a/src/core/server/saved_objects/service/lib/repository.ts b/src/core/server/saved_objects/service/lib/repository.ts
@@ -63,7 +63,7 @@ import {
 } from '../../types';
 import { SavedObjectTypeRegistry } from '../../saved_objects_type_registry';
 import { validateConvertFilterToKueryNode } from './filter_utils';
-import { namespaceIdToString } from './namespace';
+import { namespaceIdToString, namespaceStringToId } from './namespace';
 
 // BEWARE: The SavedObjectClient depends on the implementation details of the SavedObjectsRepository
 // so any breaking changes to this repository are considered breaking changes to the SavedObjectsClient.
@@ -1131,7 +1131,9 @@ export class SavedObjectsRepository {
         };
       }
 
-      const { attributes, references, version } = object;
+      const { attributes, references, version, namespace: objectNamespace } = object;
+      // `objectNamespace` is a namespace string, while `namespace` is a namespace ID.
+      // The object namespace string, if defined, will supersede the operation's namespace ID.
 
       const documentToSave = {
         [type]: attributes,
@@ -1148,16 +1150,22 @@ export class SavedObjectsRepository {
           id,
           version,
           documentToSave,
+          objectNamespace,
           ...(requiresNamespacesCheck && { esRequestIndex: bulkGetRequestIndexCounter++ }),
         },
       };
     });
 
+    const getNamespaceId = (objectNamespace?: string) =>
+      objectNamespace !== undefined ? namespaceStringToId(objectNamespace) : namespace;
+    const getNamespaceString = (objectNamespace?: string) =>
+      objectNamespace ?? namespaceIdToString(namespace);
+
     const bulkGetDocs = expectedBulkGetResults
       .filter(isRight)
       .filter(({ value }) => value.esRequestIndex !== undefined)
-      .map(({ value: { type, id } }) => ({
-        _id: this._serializer.generateRawId(namespace, type, id),
+      .map(({ value: { type, id, objectNamespace } }) => ({
+        _id: this._serializer.generateRawId(getNamespaceId(objectNamespace), type, id),
         _index: this.getIndexForType(type),
         _source: ['type', 'namespaces'],
       }));
@@ -1182,14 +1190,25 @@ export class SavedObjectsRepository {
           return expectedBulkGetResult;
         }
 
-        const { esRequestIndex, id, type, version, documentToSave } = expectedBulkGetResult.value;
+        const {
+          esRequestIndex,
+          id,
+          type,
+          version,
+          documentToSave,
+          objectNamespace,
+        } = expectedBulkGetResult.value;
+
         let namespaces;
         let versionProperties;
         if (esRequestIndex !== undefined) {
           const indexFound = bulkGetResponse?.statusCode !== 404;
           const actualResult = indexFound ? bulkGetResponse?.body.docs[esRequestIndex] : undefined;
           const docFound = indexFound && actualResult.found === true;
-          if (!docFound || !this.rawDocExistsInNamespace(actualResult, namespace)) {
+          if (
+            !docFound ||
+            !this.rawDocExistsInNamespace(actualResult, getNamespaceId(objectNamespace))
+          ) {
             return {
               tag: 'Left' as 'Left',
               error: {
@@ -1205,7 +1224,7 @@ export class SavedObjectsRepository {
           versionProperties = getExpectedVersionProperties(version, actualResult);
         } else {
           if (this._registry.isSingleNamespace(type)) {
-            namespaces = [namespaceIdToString(namespace)];
+            namespaces = [getNamespaceString(objectNamespace)];
           }
           versionProperties = getExpectedVersionProperties(version);
         }
@@ -1221,7 +1240,7 @@ export class SavedObjectsRepository {
         bulkUpdateParams.push(
           {
             update: {
-              _id: this._serializer.generateRawId(namespace, type, id),
+              _id: this._serializer.generateRawId(getNamespaceId(objectNamespace), type, id),
               _index: this.getIndexForType(type),
               ...versionProperties,
             },

diff --git a/src/core/server/saved_objects/service/saved_objects_client.ts b/src/core/server/saved_objects/service/saved_objects_client.ts
@@ -75,6 +75,13 @@ export interface SavedObjectsBulkUpdateObject<T = unknown>
   type: string;
   /** {@inheritdoc SavedObjectAttributes} */
   attributes: Partial<T>;
+  /**
+   * Optional namespace string to use for this document. If this is defined, it will supersede the namespace ID that is in
+   * {@link SavedObjectsUpdateOptions}.
+   *
+   * Note: the default namespace's string representation is `'default'`, and its ID representation is `undefined`.
+   **/
+  namespace?: string;
 }
 
 /**

diff --git a/src/core/server/server.api.md b/src/core/server/server.api.md
@@ -2079,6 +2079,7 @@ export interface SavedObjectsBulkResponse<T = unknown> {
 export interface SavedObjectsBulkUpdateObject<T = unknown> extends Pick<SavedObjectsUpdateOptions, 'version' | 'references'> {
     attributes: Partial<T>;
     id: string;
+    namespace?: string;
     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
@@ -117,7 +117,11 @@ const expectSuccess = async (fn: Function, args: Record<string, any>) => {
   return result;
 };
 
-const expectPrivilegeCheck = async (fn: Function, args: Record<string, any>) => {
+const expectPrivilegeCheck = async (
+  fn: Function,
+  args: Record<string, any>,
+  namespacesOverride?: string[]
+) => {
   clientOpts.checkSavedObjectsPrivilegesAsCurrentUser.mockImplementation(
     getMockCheckPrivilegesFailure
   );
@@ -131,7 +135,7 @@ const expectPrivilegeCheck = async (fn: Function, args: Record<string, any>) =>
   expect(clientOpts.checkSavedObjectsPrivilegesAsCurrentUser).toHaveBeenCalledTimes(1);
   expect(clientOpts.checkSavedObjectsPrivilegesAsCurrentUser).toHaveBeenCalledWith(
     actions,
-    args.options?.namespace ?? args.options?.namespaces
+    namespacesOverride ?? args.options?.namespace ?? args.options?.namespaces
   );
 };
 
@@ -468,7 +472,18 @@ describe('#bulkUpdate', () => {
 
   test(`checks privileges for user, actions, and namespace`, async () => {
     const objects = [obj1, obj2];
-    await expectPrivilegeCheck(client.bulkUpdate, { objects, options });
+    const namespacesOverride = [options.namespace]; // the bulkCreate function checks privileges as an array
+    await expectPrivilegeCheck(client.bulkUpdate, { objects, options }, namespacesOverride);
+  });
+
+  test(`checks privileges for object namespaces if present`, async () => {
+    const objects = [
+      { ...obj1, namespace: 'foo-ns' },
+      { ...obj2, namespace: 'bar-ns' },
+    ];
+    const namespacesOverride = ['default', 'foo-ns', 'bar-ns'];
+    // use the default namespace for the options
+    await expectPrivilegeCheck(client.bulkUpdate, { objects, options: {} }, namespacesOverride);
   });
 
   test(`filters namespaces that the user doesn't have access to`, async () => {