Fix ESO migration decryption for converted saved object types

A bug in the algorithm neglected to differentiate when a converted
object may have had its ID changed (this happens when an object exists
in a non-default space and then it is converted).
This commit fixes the bug and adds several more unit tests and
integration tests to exercise different permutations of migrations.

x-pack/plugins/encrypted_saved_objects/server/create_migration.test.ts

@@ -170,10 +170,14 @@ describe('createMigration()', () => {
  describe('uses the object `namespaces` field to populate the descriptor when the migration context indicates this type is being converted', () => {
  const doTest = ({
  objectNamespace,
+ objectNamespaces,
  decryptDescriptorNamespace,
+ encryptDescriptorNamespace,
  }: {
  objectNamespace: string | undefined;
+ objectNamespaces: string[] | undefined;
  decryptDescriptorNamespace: string | undefined;
+ encryptDescriptorNamespace: string | undefined;
  }) => {
  const instantiateServiceWithLegacyType = jest.fn(() =>
  encryptedSavedObjectsServiceMock.create()
@@ -189,56 +193,82 @@ describe('createMigration()', () => {
  },
  (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' };
+
+ ['7.99.99', '8.0.0'].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, // test works with any version <= 8.0.0
+ convertToMultiNamespaceTypeVersion: '8.0.0',
+ })
+ );
+ expect(encryptionSavedObjectService.decryptAttributesSync).toHaveBeenNthCalledWith(
+ i + 1,
+ { id: '123', type: 'known-type-1', namespace: decryptDescriptorNamespace },
  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
- );
+ { convertToMultiNamespaceType: true, originId: 'some-origin-id' }
+ );
+ expect(encryptionSavedObjectService.encryptAttributesSync).toHaveBeenNthCalledWith(
+ i + 1,
+ { id: '123', type: 'known-type-1', namespace: encryptDescriptorNamespace },
+ 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 });
- });
-
- it('when the first namespace element is another string', () => {
- doTest({ objectNamespace: 'foo', decryptDescriptorNamespace: 'foo' });
+ [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({
+ objectNamespace,
+ objectNamespaces: undefined,
+ decryptDescriptorNamespace: objectNamespace,
+ encryptDescriptorNamespace: objectNamespace,
+ });
+ });
+
+ 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({
+ objectNamespace,
+ objectNamespaces: [],
+ decryptDescriptorNamespace: objectNamespace,
+ encryptDescriptorNamespace: objectNamespace,
+ });
+ });
+
+ it(`when namespaces is a non-empty array (default space) and namespace is ${namespaceDescription}`, () => {
+ doTest({
+ objectNamespace,
+ objectNamespaces: ['default', 'additional-spaces-are-ignored'],
+ decryptDescriptorNamespace: undefined,
+ encryptDescriptorNamespace: objectNamespace,
+ });
+ });
+
+ it(`when namespaces is a non-empty array (custom space) and namespace is ${namespaceDescription}`, () => {
+ doTest({
+ objectNamespace,
+ objectNamespaces: ['custom', 'additional-spaces-are-ignored'],
+ decryptDescriptorNamespace: 'custom',
+ encryptDescriptorNamespace: objectNamespace,
+ });
+ });
  });
  });
  });

x-pack/plugins/encrypted_saved_objects/server/create_migration.ts

@@ -5,6 +5,8 @@
  * 2.0.
  */
 
+import Semver from 'semver';
+
 import type {
  SavedObjectMigrationContext,
  SavedObjectMigrationFn,
@@ -65,19 +67,37 @@ export const getCreateMigration = (
  return encryptedDoc;
  }
 
+ // After it is converted, the object's old ID is stored in the `originId` field. In addition, objects that are imported a certain way
+ // may have this field set, but it would not be necessary to use this to decrypt saved object attributes.
+ const originId = encryptedDoc.originId;
+
+ // If an object is slated to be converted, it should be decrypted flexibly:
+ // * If this is an index migration:
+ // a. If there is one or more pending migration _before_ the conversion, the object will be decrypted and re-encrypted with its
+ // namespace in the descriptor. Then, after the conversion, the object will be decrypted with its namespace and old ID in the
+ // descriptor and re-encrypted with its new ID (without a namespace) in the descriptor.
+ // b. If there are no pending migrations before the conversion, then after the conversion the object will be decrypted with its
+ // namespace and old ID in the descriptor and re-encrypted with its new ID (without a namespace) in the descriptor.
+ // * If this is *not* an index migration, then it is a single document migration. In that case, the object will be decrypted and
+ // re-encrypted without a namespace in the descriptor.
+ // To account for these different scenarios, when this field is set, the ESO service will attempt several different permutations of
+ // the descriptor when decrypting the object.
+ const convertToMultiNamespaceType =
+ !!context.convertToMultiNamespaceTypeVersion &&
+ Semver.lte(context.migrationVersion, context.convertToMultiNamespaceTypeVersion);
+
  // If an object has been converted right before this migration function is called, it will no longer have a `namespace` field, but it
  // will have a `namespaces` field; in that case, the first/only element in that array should be used as the namespace in the descriptor
  // during decryption.
- const convertToMultiNamespaceType =
- context.convertToMultiNamespaceTypeVersion === context.migrationVersion;
- const decryptDescriptorNamespace = convertToMultiNamespaceType
- ? normalizeNamespace(encryptedDoc.namespaces?.[0]) // `namespaces` contains string values, but we need to normalize this to the namespace ID representation
- : encryptedDoc.namespace;
+ const decryptDescriptorNamespace =
+ convertToMultiNamespaceType && encryptedDoc.namespaces?.length
+ ? normalizeNamespace(encryptedDoc.namespaces[0]) // `namespaces` contains string values, but we need to normalize this to the namespace ID representation
+ : encryptedDoc.namespace;
 
  const { id, type } = encryptedDoc;
  // These descriptors might have a `namespace` that is undefined. That is expected for multi-namespace and namespace-agnostic types.
  const decryptDescriptor = { id, type, namespace: decryptDescriptorNamespace };
- const encryptDescriptor = { id, type, namespace: encryptedDoc.namespace };
+ const encryptDescriptor = { id, type, namespace: encryptedDoc.namespace }; // It would be preferable to rely on getDescriptorNamespace() here, but that requires the SO type registry which can only be retrieved from a promise, and this is not an async function
 
  // decrypt the attributes using the input type definition
  // then migrate the document
@@ -87,6 +107,7 @@ export const getCreateMigration = (
  mapAttributes(encryptedDoc, (inputAttributes) =>
  inputService.decryptAttributesSync<any>(decryptDescriptor, inputAttributes, {
  convertToMultiNamespaceType,
+ originId,
  })
  ),
  context