Fix ESO migration decryption for converted saved object types (#106897)

elastic · Jul 29, 2021 · 9742a52 · 9742a52
1 parent 744e5b2
commit 9742a52
Show file tree

Hide file tree

Showing 13 changed files with 733 additions and 174 deletions.
diff --git a/.../kibana-plugin-core-server.savedobjectmigrationcontext.issinglenamespacetype.md b/.../kibana-plugin-core-server.savedobjectmigrationcontext.issinglenamespacetype.md
@@ -0,0 +1,13 @@
+<!-- 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; [SavedObjectMigrationContext](./kibana-plugin-core-server.savedobjectmigrationcontext.md) &gt; [isSingleNamespaceType](./kibana-plugin-core-server.savedobjectmigrationcontext.issinglenamespacetype.md)
+
+## SavedObjectMigrationContext.isSingleNamespaceType property
+
+Whether this is a single-namespace type or not
+
+<b>Signature:</b>
+
+```typescript
+readonly isSingleNamespaceType: boolean;
+```
diff --git a/...evelopment/core/server/kibana-plugin-core-server.savedobjectmigrationcontext.md b/...evelopment/core/server/kibana-plugin-core-server.savedobjectmigrationcontext.md
@@ -17,6 +17,7 @@ export interface SavedObjectMigrationContext
 |  Property | Type | Description |
 |  --- | --- | --- |
 |  [convertToMultiNamespaceTypeVersion](./kibana-plugin-core-server.savedobjectmigrationcontext.converttomultinamespacetypeversion.md) | <code>string</code> | The version in which this object type is being converted to a multi-namespace type |
+|  [isSingleNamespaceType](./kibana-plugin-core-server.savedobjectmigrationcontext.issinglenamespacetype.md) | <code>boolean</code> | Whether this is a single-namespace type or not |
 |  [log](./kibana-plugin-core-server.savedobjectmigrationcontext.log.md) | <code>SavedObjectsMigrationLogger</code> | logger instance to be used by the migration handler |
 |  [migrationVersion](./kibana-plugin-core-server.savedobjectmigrationcontext.migrationversion.md) | <code>string</code> | The migration version that this migration function is defined for |
 
diff --git a/src/core/server/saved_objects/migrations/core/document_migrator.ts b/src/core/server/saved_objects/migrations/core/document_migrator.ts
@@ -667,6 +667,7 @@ function wrapWithTry(
     log: new MigrationLogger(log),
     migrationVersion: version,
     convertToMultiNamespaceTypeVersion: type.convertToMultiNamespaceTypeVersion,
+    isSingleNamespaceType: type.namespaceType === 'single',
   });
 
   return function tryTransformDoc(doc: SavedObjectUnsanitizedDoc) {

diff --git a/src/core/server/saved_objects/migrations/mocks.ts b/src/core/server/saved_objects/migrations/mocks.ts
@@ -24,14 +24,17 @@ export const createSavedObjectsMigrationLoggerMock = (): jest.Mocked<SavedObject
 const createContextMock = ({
   migrationVersion = '8.0.0',
   convertToMultiNamespaceTypeVersion,
+  isSingleNamespaceType = false,
 }: {
   migrationVersion?: string;
   convertToMultiNamespaceTypeVersion?: string;
+  isSingleNamespaceType?: boolean;
 } = {}): jest.Mocked<SavedObjectMigrationContext> => {
   const mock = {
     log: createSavedObjectsMigrationLoggerMock(),
     migrationVersion,
     convertToMultiNamespaceTypeVersion,
+    isSingleNamespaceType,
   };
   return mock;
 };

diff --git a/src/core/server/saved_objects/migrations/types.ts b/src/core/server/saved_objects/migrations/types.ts
@@ -65,6 +65,10 @@ export interface SavedObjectMigrationContext {
    * The version in which this object type is being converted to a multi-namespace type
    */
   readonly convertToMultiNamespaceTypeVersion?: string;
+  /**
+   * Whether this is a single-namespace type or not
+   */
+  readonly isSingleNamespaceType: boolean;
 }
 
 /**

diff --git a/src/core/server/server.api.md b/src/core/server/server.api.md
@@ -2268,6 +2268,7 @@ export interface SavedObjectExportBaseOptions {
 // @public
 export interface SavedObjectMigrationContext {
     readonly convertToMultiNamespaceTypeVersion?: string;
+    readonly isSingleNamespaceType: boolean;
     readonly log: SavedObjectsMigrationLogger;
     readonly migrationVersion: string;
 }

diff --git a/x-pack/plugins/encrypted_saved_objects/server/create_migration.test.ts b/x-pack/plugins/encrypted_saved_objects/server/create_migration.test.ts
@@ -16,8 +16,23 @@ afterEach(() => {
   jest.clearAllMocks();
 });
 
+interface ConversionTestOptions {
+  /** Migration version(s) to test */
+  migrationVersions: string[];
+  /** The `namespace` field of the object that is being migrated */
+  objectNamespace: string | undefined;
+  /** The `namespaces` field of the object that is being migrated */
+  objectNamespaces: string[] | undefined;
+  /** The expected namespace in the decrypt descriptor */
+  expectedDecryptDescriptorNamespace: string | undefined;
+  /** The expected namespace in the encrypt descriptor */
+  expectedEncryptDescriptorNamespace: string | undefined;
+  /** The expected value for the `isTypeBeingConverted` field in the decrypt params */
+  expectedIsTypeBeingConverted: boolean;
+}
+
 describe('createMigration()', () => {
-  const migrationContext = migrationMocks.createContext();
+  const migrationContext = migrationMocks.createContext({ isSingleNamespaceType: true });
   const inputType = { type: 'known-type-1', attributesToEncrypt: new Set(['firstAttr']) };
   const migrationType = {
     type: 'known-type-1',
@@ -100,7 +115,7 @@ describe('createMigration()', () => {
           namespace: 'namespace',
         },
         attributes,
-        { convertToMultiNamespaceType: false }
+        { isTypeBeingConverted: false }
       );
 
       expect(encryptionSavedObjectService.encryptAttributesSync).toHaveBeenCalledWith(
@@ -157,7 +172,7 @@ describe('createMigration()', () => {
           namespace: 'namespace',
         },
         attributes,
-        { convertToMultiNamespaceType: false }
+        { isTypeBeingConverted: false }
       );
 
       expect(migrationFunc).not.toHaveBeenCalled();
@@ -209,7 +224,7 @@ describe('createMigration()', () => {
           namespace: 'namespace',
         },
         attributes,
-        { convertToMultiNamespaceType: false }
+        { isTypeBeingConverted: false }
       );
 
       expect(encryptionSavedObjectService.stripOrDecryptAttributesSync).not.toHaveBeenCalled();
@@ -273,7 +288,7 @@ describe('createMigration()', () => {
           namespace: 'namespace',
         },
         attributes,
-        { convertToMultiNamespaceType: false }
+        { isTypeBeingConverted: false }
       );
 
       expect(migrationFunc).toHaveBeenCalled();
@@ -331,7 +346,7 @@ describe('createMigration()', () => {
           namespace: 'namespace',
         },
         attributes,
-        { convertToMultiNamespaceType: false }
+        { isTypeBeingConverted: false }
       );
 
       expect(migrationFunc).toHaveBeenCalled();
@@ -383,7 +398,7 @@ describe('createMigration()', () => {
           namespace: 'namespace',
         },
         attributes,
-        { convertToMultiNamespaceType: false }
+        { isTypeBeingConverted: false }
       );
 
       expect(migrationFunc).toHaveBeenCalled();
@@ -435,7 +450,7 @@ describe('createMigration()', () => {
           namespace: 'namespace',
         },
         attributes,
-        { convertToMultiNamespaceType: false }
+        { isTypeBeingConverted: false }
       );
 
       expect(migrationFunc).toHaveBeenCalled();
@@ -495,7 +510,7 @@ describe('createMigration()', () => {
           namespace: 'namespace',
         },
         attributes,
-        { convertToMultiNamespaceType: false }
+        { isTypeBeingConverted: false }
       );
 
       expect(migrationFunc).toHaveBeenCalled();
@@ -551,7 +566,7 @@ describe('createMigration()', () => {
           namespace: 'namespace',
         },
         attributes,
-        { convertToMultiNamespaceType: false }
+        { isTypeBeingConverted: false }
       );
 
       expect(encryptionSavedObjectService.encryptAttributesSync).toHaveBeenCalledWith(
@@ -564,14 +579,15 @@ describe('createMigration()', () => {
       );
     });
 
-    describe('uses the object `namespaces` field to populate the descriptor when the migration context indicates this type is being converted', () => {
+    describe('handles conversion to a multi-namespace type (convertToMultiNamespaceVersion migration context field)', () => {
       const doTest = ({
+        migrationVersions,
         objectNamespace,
-        decryptDescriptorNamespace,
-      }: {
-        objectNamespace: string | undefined;
-        decryptDescriptorNamespace: string | undefined;
-      }) => {
+        objectNamespaces,
+        expectedDecryptDescriptorNamespace,
+        expectedEncryptDescriptorNamespace,
+        expectedIsTypeBeingConverted,
+      }: ConversionTestOptions) => {
         const instantiateServiceWithLegacyType = jest.fn(() =>
           encryptedSavedObjectsServiceMock.create()
         );
@@ -586,56 +602,150 @@ describe('createMigration()', () => {
           },
           migration: (doc) => doc,
         });
-
-        const attributes = {
-          firstAttr: 'first_attr',
-        };
-
-        encryptionSavedObjectService.decryptAttributesSync.mockReturnValueOnce(attributes);
-        encryptionSavedObjectService.encryptAttributesSync.mockReturnValueOnce(attributes);
-
-        noopMigration(
-          {
-            id: '123',
-            type: 'known-type-1',
-            namespaces: objectNamespace ? [objectNamespace] : [],
+        const attributes = { firstAttr: 'first_attr' };
+
+        migrationVersions.forEach((migrationVersion, i) => {
+          encryptionSavedObjectService.decryptAttributesSync.mockReturnValueOnce(attributes);
+          encryptionSavedObjectService.encryptAttributesSync.mockReturnValueOnce(attributes);
+          noopMigration(
+            {
+              id: '123',
+              originId: 'some-origin-id',
+              type: 'known-type-1',
+              namespace: objectNamespace,
+              namespaces: objectNamespaces,
+              attributes,
+            },
+            migrationMocks.createContext({
+              migrationVersion, // Any migrationVersion <= convertToMultiNamespaceTypeVersion (8.0.0) indicates this type is being converted
+              convertToMultiNamespaceTypeVersion: '8.0.0',
+              isSingleNamespaceType: false, // in practice, this can never be true if convertToMultiNamespaceTypeVersion is defined
+            })
+          );
+          expect(encryptionSavedObjectService.decryptAttributesSync).toHaveBeenNthCalledWith(
+            i + 1,
+            { id: '123', type: 'known-type-1', namespace: expectedDecryptDescriptorNamespace },
             attributes,
-          },
-          migrationMocks.createContext({
-            migrationVersion: '8.0.0',
-            convertToMultiNamespaceTypeVersion: '8.0.0',
-          })
-        );
-
-        expect(encryptionSavedObjectService.decryptAttributesSync).toHaveBeenCalledWith(
-          {
-            id: '123',
-            type: 'known-type-1',
-            namespace: decryptDescriptorNamespace,
-          },
-          attributes,
-          { convertToMultiNamespaceType: true }
-        );
-
-        expect(encryptionSavedObjectService.encryptAttributesSync).toHaveBeenCalledWith(
-          {
-            id: '123',
-            type: 'known-type-1',
-          },
-          attributes
-        );
+            { isTypeBeingConverted: expectedIsTypeBeingConverted, originId: 'some-origin-id' } // decrypt parameters
+          );
+          expect(encryptionSavedObjectService.encryptAttributesSync).toHaveBeenNthCalledWith(
+            i + 1,
+            { id: '123', type: 'known-type-1', namespace: expectedEncryptDescriptorNamespace },
+            attributes
+          );
+        });
       };
 
-      it('when namespaces is an empty array', () => {
-        doTest({ objectNamespace: undefined, decryptDescriptorNamespace: undefined });
-      });
-
-      it('when the first namespace element is "default"', () => {
-        doTest({ objectNamespace: 'default', decryptDescriptorNamespace: undefined });
+      describe('when migrationVersion <= convertToMultiNamespaceVersion, it sets the descriptors and decrypt parameters appropriately', () => {
+        const migrationVersions = ['7.99.99', '8.0.0'];
+        [undefined, 'foo'].forEach((objectNamespace) => {
+          // In the test cases below, we test what will happen if an object has both `namespace` and `namespaces` fields. This will not happen
+          // in normal operation, as when Kibana converts an object from a single- to a multi-namespace type, it removes the `namespace` field
+          // and adds the `namespaces` field. The tests below are for completeness.
+          const namespaceDescription = objectNamespace ? 'defined' : undefined;
+
+          it(`when namespaces is undefined and namespace is ${namespaceDescription}`, () => {
+            doTest({
+              migrationVersions,
+              objectNamespace,
+              objectNamespaces: undefined,
+              expectedDecryptDescriptorNamespace: objectNamespace,
+              expectedEncryptDescriptorNamespace: undefined,
+              expectedIsTypeBeingConverted: true,
+            });
+          });
+
+          it(`when namespaces is an empty array and namespace is ${namespaceDescription}`, () => {
+            // The `namespaces` field should never be an empty array, but we test for it anyway. In this case, we fall back to attempting to
+            // decrypt with the `namespace` field; if that doesn't work, the ESO service will try again without it.
+            doTest({
+              migrationVersions,
+              objectNamespace,
+              objectNamespaces: [],
+              expectedDecryptDescriptorNamespace: objectNamespace,
+              expectedEncryptDescriptorNamespace: undefined,
+              expectedIsTypeBeingConverted: true,
+            });
+          });
+
+          it(`when namespaces is a non-empty array (default space) and namespace is ${namespaceDescription}`, () => {
+            doTest({
+              migrationVersions,
+              objectNamespace,
+              objectNamespaces: ['default', 'additional-spaces-are-ignored'],
+              expectedDecryptDescriptorNamespace: undefined,
+              expectedEncryptDescriptorNamespace: undefined,
+              expectedIsTypeBeingConverted: true,
+            });
+          });
+
+          it(`when namespaces is a non-empty array (custom space) and namespace is ${namespaceDescription}`, () => {
+            doTest({
+              migrationVersions,
+              objectNamespace,
+              objectNamespaces: ['custom', 'additional-spaces-are-ignored'],
+              expectedDecryptDescriptorNamespace: 'custom',
+              expectedEncryptDescriptorNamespace: undefined,
+              expectedIsTypeBeingConverted: true,
+            });
+          });
+        });
       });
 
-      it('when the first namespace element is another string', () => {
-        doTest({ objectNamespace: 'foo', decryptDescriptorNamespace: 'foo' });
+      describe('when migrationVersion > convertToMultiNamespaceVersion, it sets the descriptors and decrypt parameters appropriately', () => {
+        const migrationVersions = ['8.0.1'];
+        [undefined, 'foo'].forEach((objectNamespace) => {
+          // In the test cases below, we test what will happen if an object has both `namespace` and `namespaces` fields. This will not happen
+          // in normal operation, as when Kibana converts an object from a single- to a multi-namespace type, it removes the `namespace` field
+          // and adds the `namespaces` field. The tests below are for completeness.
+          const namespaceDescription = objectNamespace ? 'defined' : undefined;
+
+          it(`when namespaces is undefined and namespace is ${namespaceDescription}`, () => {
+            doTest({
+              migrationVersions,
+              objectNamespace,
+              objectNamespaces: undefined,
+              expectedDecryptDescriptorNamespace: undefined,
+              expectedEncryptDescriptorNamespace: undefined,
+              expectedIsTypeBeingConverted: false,
+            });
+          });
+
+          it(`when namespaces is an empty array and namespace is ${namespaceDescription}`, () => {
+            // The `namespaces` field should never be an empty array, but we test for it anyway. In this case, we fall back to attempting to
+            // decrypt with the `namespace` field; if that doesn't work, the ESO service will try again without it.
+            doTest({
+              migrationVersions,
+              objectNamespace,
+              objectNamespaces: [],
+              expectedDecryptDescriptorNamespace: undefined,
+              expectedEncryptDescriptorNamespace: undefined,
+              expectedIsTypeBeingConverted: false,
+            });
+          });
+
+          it(`when namespaces is a non-empty array (default space) and namespace is ${namespaceDescription}`, () => {
+            doTest({
+              migrationVersions,
+              objectNamespace,
+              objectNamespaces: ['default', 'additional-spaces-are-ignored'],
+              expectedDecryptDescriptorNamespace: undefined,
+              expectedEncryptDescriptorNamespace: undefined,
+              expectedIsTypeBeingConverted: false,
+            });
+          });
+
+          it(`when namespaces is a non-empty array (custom space) and namespace is ${namespaceDescription}`, () => {
+            doTest({
+              migrationVersions,
+              objectNamespace,
+              objectNamespaces: ['custom', 'additional-spaces-are-ignored'],
+              expectedDecryptDescriptorNamespace: undefined,
+              expectedEncryptDescriptorNamespace: undefined,
+              expectedIsTypeBeingConverted: false,
+            });
+          });
+        });
       });
     });
   });
@@ -754,7 +864,7 @@ describe('createMigration()', () => {
           firstAttr: '#####',
           nonEncryptedAttr: 'non encrypted',
         },
-        { convertToMultiNamespaceType: false }
+        { isTypeBeingConverted: false }
       );
 
       expect(serviceWithMigrationLegacyType.encryptAttributesSync).toHaveBeenCalledWith(