From 460f5e333631500b2d7984c212ab4d15f5ca4d8e Mon Sep 17 00:00:00 2001 From: Jeramy Soucy Date: Mon, 16 Oct 2023 12:18:19 -0400 Subject: [PATCH] Fixes conflict checks for sharing saved objects (#168655) Closes #168049 ## Summary This PR adjusts the KQL filters when collecting references to saved objects for the purpose of updating their spaces (i.e. sharing saved objects to spaces). An additional filter is added to specifically exclude ID -> ID matches - an ID match would mean the object has already been shared to the destination space and there is no conflict. Filters to match the shared object's ID or origin ID to the destination space's objects' origin ID, and to match the shared object's origin ID to the destination space's objects' IDs remain in place to properly check for conflicts with potential copies. ### Manual Testing - Create 2 spaces: A and B - Add a sample data set (e.g. flight) to space A - In Discover, create a saved query called "s1" with a filter pill that uses the sample data logs data view - Create another saved query called "s2" with a filter pill that uses the sample data logs data view - Go to `Stack Management->Saved` Objects and share the "s1" query to space B - Now share the "s2" query to space B. From the main branch you will see that there is a conflict that disallows sharing the second query. This is because it is also attempting to share the referenced data view, which is already in space B. However, this should not be a conflict - from this PR you will be able to successfully share both queries. ### Automated Testing - packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/utils/find_shared_origin_objects.test.ts - packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/internals/collect_multi_namespace_references.test.ts - x-pack/test/spaces_api_integration/common/suites/get_shareable_references.ts --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com> --- ...collect_multi_namespace_references.test.ts | 33 ++- .../collect_multi_namespace_references.ts | 6 +- .../utils/find_shared_origin_objects.test.ts | 224 +++++++++++++++++- .../apis/utils/find_shared_origin_objects.ts | 67 +++++- .../apis/collect_multinamespace_references.ts | 9 +- .../src/apis/index.ts | 1 + .../common/suites/get_shareable_references.ts | 16 +- 7 files changed, 325 insertions(+), 31 deletions(-) diff --git a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/internals/collect_multi_namespace_references.test.ts b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/internals/collect_multi_namespace_references.test.ts index 5450f0c739ce3..a82c18d42876b 100644 --- a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/internals/collect_multi_namespace_references.test.ts +++ b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/internals/collect_multi_namespace_references.test.ts @@ -411,11 +411,12 @@ describe('collectMultiNamespaceReferences', () => { expect(mockFindSharedOriginObjects).toHaveBeenCalledWith( expect.anything(), [ - { type: obj1.type, origin: obj1.id }, - { type: obj2.type, origin: obj2.originId }, // If the found object has an `originId`, that is used instead of the object's `id`. - { type: obj3.type, origin: obj3.id }, + { type: obj1.type, id: obj1.id }, + { type: obj2.type, id: obj2.id, origin: obj2.originId }, + { type: obj3.type, id: obj3.id }, ], - ALIAS_OR_SHARED_ORIGIN_SEARCH_PER_PAGE + ALIAS_OR_SHARED_ORIGIN_SEARCH_PER_PAGE, + undefined ); expect(result.objects).toEqual([ // Note: in a realistic scenario, `spacesWithMatchingOrigins` would be a superset of `spaces`. But for the purposes of this unit @@ -441,8 +442,9 @@ describe('collectMultiNamespaceReferences', () => { expect(mockFindSharedOriginObjects).toHaveBeenCalledTimes(1); expect(mockFindSharedOriginObjects).toHaveBeenCalledWith( expect.anything(), - [{ type: obj1.type, origin: obj1.id }], - ALIAS_OR_SHARED_ORIGIN_SEARCH_PER_PAGE + [{ type: obj1.type, id: obj1.id }], + ALIAS_OR_SHARED_ORIGIN_SEARCH_PER_PAGE, + undefined ); }); @@ -458,6 +460,25 @@ describe('collectMultiNamespaceReferences', () => { 'Failed to retrieve shared origin objects: Oh no!' ); }); + + it('passes options to findSharedOriginObjects', async () => { + const obj1 = { type: MULTI_NAMESPACE_OBJ_TYPE_1, id: 'id-1' }; + const obj2 = { type: MULTI_NAMESPACE_OBJ_TYPE_1, id: 'id-2' }; + const params = setup([obj1, obj2]); + mockMgetResults({ found: true }, { found: false }); // results for obj1 and obj2 + + await collectMultiNamespaceReferences({ + ...params, + options: { purpose: 'updateObjectsSpaces' }, + }); + expect(mockFindSharedOriginObjects).toHaveBeenCalledTimes(1); + expect(mockFindSharedOriginObjects).toHaveBeenCalledWith( + expect.anything(), + [{ type: obj1.type, id: obj1.id }], + ALIAS_OR_SHARED_ORIGIN_SEARCH_PER_PAGE, + 'updateObjectsSpaces' + ); + }); }); describe('with security enabled', () => { diff --git a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/internals/collect_multi_namespace_references.ts b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/internals/collect_multi_namespace_references.ts index 87254d1608515..ab11f76d9e967 100644 --- a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/internals/collect_multi_namespace_references.ts +++ b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/internals/collect_multi_namespace_references.ts @@ -108,12 +108,14 @@ export async function collectMultiNamespaceReferences( ); const objectOriginsToSearchFor = foundObjects.map(({ type, id, originId }) => ({ type, - origin: originId || id, + id, + origin: originId, })); const originsMap = await findSharedOriginObjects( createPointInTimeFinder, objectOriginsToSearchFor, - ALIAS_OR_SHARED_ORIGIN_SEARCH_PER_PAGE + ALIAS_OR_SHARED_ORIGIN_SEARCH_PER_PAGE, + options?.purpose ); const results = objectsWithContext.map((obj) => { const aliasesVal = aliasesMap.get(getObjectKey(obj)); diff --git a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/utils/find_shared_origin_objects.test.ts b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/utils/find_shared_origin_objects.test.ts index c9f90073da24f..1622d953d7a36 100644 --- a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/utils/find_shared_origin_objects.test.ts +++ b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/utils/find_shared_origin_objects.test.ts @@ -59,10 +59,10 @@ describe('findSharedOriginObjects', () => { }); } - const obj1 = { type: 'type-1', origin: 'id-1' }; - const obj2 = { type: 'type-2', origin: 'id-2' }; - const obj3 = { type: 'type-3', origin: 'id-3' }; - const obj4 = { type: 'type-4', origin: 'id-4' }; + const obj1 = { type: 'type-1', id: 'id-1', origin: 'origin-1' }; + const obj2 = { type: 'type-2', id: 'id-2', origin: 'origin-2' }; + const obj3 = { type: 'type-3', id: 'id-3', origin: 'origin-3' }; + const obj4 = { type: 'type-4', id: 'id-4', origin: 'origin-4' }; it('uses the PointInTimeFinder to search for legacy URL aliases', async () => { mockFindResults( @@ -156,4 +156,220 @@ describe('findSharedOriginObjects', () => { expect(pointInTimeFinder.find).toHaveBeenCalledTimes(1); expect(pointInTimeFinder.close).toHaveBeenCalledTimes(2); }); + + describe(`when options.purpose is 'updateObjectsSpaces'`, () => { + it('calls createPointInTimeFinder with filter to ignore direct ID matches', async () => { + const objects = [obj1, obj2, obj3]; + await findSharedOriginObjects(createPointInTimeFinder, objects, 999, 'updateObjectsSpaces'); + expect(createPointInTimeFinder).toHaveBeenCalledTimes(1); + expect(createPointInTimeFinder).toHaveBeenCalledWith( + expect.objectContaining({ + filter: expect.objectContaining({ + arguments: expect.arrayContaining([ + expect.objectContaining({ + arguments: expect.arrayContaining([ + expect.objectContaining({ + arguments: [ + { + arguments: [ + { + isQuoted: false, + type: 'literal', + value: 'type-1.id', + }, + { + isQuoted: false, + type: 'literal', + value: 'type-1:id-1', + }, + ], + function: 'is', + type: 'function', + }, + ], + function: 'not', + type: 'function', + }), + ]), + }), + expect.objectContaining({ + arguments: expect.arrayContaining([ + expect.objectContaining({ + arguments: [ + { + arguments: [ + { + isQuoted: false, + type: 'literal', + value: 'type-2.id', + }, + { + isQuoted: false, + type: 'literal', + value: 'type-2:id-2', + }, + ], + function: 'is', + type: 'function', + }, + ], + function: 'not', + type: 'function', + }), + ]), + }), + expect.objectContaining({ + arguments: expect.arrayContaining([ + expect.objectContaining({ + arguments: [ + { + arguments: [ + { + isQuoted: false, + type: 'literal', + value: 'type-3.id', + }, + { + isQuoted: false, + type: 'literal', + value: 'type-3:id-3', + }, + ], + function: 'is', + type: 'function', + }, + ], + function: 'not', + type: 'function', + }), + ]), + }), + ]), + }), + }), + undefined, + { disableExtensions: true } + ); + }); + + it('calls createPointInTimeFinder without redundant filter when object does not have an origin ID', async () => { + const objects = [obj1, { ...obj2, origin: undefined }, obj3]; + await findSharedOriginObjects(createPointInTimeFinder, objects, 999, 'updateObjectsSpaces'); + expect(createPointInTimeFinder).toHaveBeenCalledTimes(1); + expect(createPointInTimeFinder).toHaveBeenCalledWith( + expect.objectContaining({ + filter: expect.objectContaining({ + arguments: expect.arrayContaining([ + expect.objectContaining({ + arguments: expect.arrayContaining([ + expect.objectContaining({ + arguments: [ + { + arguments: [ + { + isQuoted: false, + type: 'literal', + value: 'type-1.id', + }, + { + isQuoted: false, + type: 'literal', + value: 'type-1:origin-1', + }, + ], + function: 'is', + type: 'function', + }, + { + arguments: [ + { + isQuoted: false, + type: 'literal', + value: 'type-1.originId', + }, + { + isQuoted: false, + type: 'literal', + value: 'origin-1', + }, + ], + function: 'is', + type: 'function', + }, + ], + function: 'or', + type: 'function', + }), + ]), + }), + expect.objectContaining({ + arguments: expect.arrayContaining([ + expect.objectContaining({ + arguments: [ + { + isQuoted: false, + type: 'literal', + value: 'type-2.originId', + }, + { + isQuoted: false, + type: 'literal', + value: 'id-2', + }, + ], + function: 'is', + type: 'function', + }), + ]), + }), + expect.objectContaining({ + arguments: expect.arrayContaining([ + expect.objectContaining({ + arguments: [ + { + arguments: [ + { + isQuoted: false, + type: 'literal', + value: 'type-3.id', + }, + { + isQuoted: false, + type: 'literal', + value: 'type-3:origin-3', + }, + ], + function: 'is', + type: 'function', + }, + { + arguments: [ + { + isQuoted: false, + type: 'literal', + value: 'type-3.originId', + }, + { + isQuoted: false, + type: 'literal', + value: 'origin-3', + }, + ], + function: 'is', + type: 'function', + }, + ], + function: 'or', + type: 'function', + }), + ]), + }), + ]), + }), + }), + undefined, + { disableExtensions: true } + ); + }); + }); }); diff --git a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/utils/find_shared_origin_objects.ts b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/utils/find_shared_origin_objects.ts index 8ed1f0a3c965a..bdf1a11375763 100644 --- a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/utils/find_shared_origin_objects.ts +++ b/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/apis/utils/find_shared_origin_objects.ts @@ -9,13 +9,22 @@ import * as esKuery from '@kbn/es-query'; import { ALL_NAMESPACES_STRING } from '@kbn/core-saved-objects-utils-server'; import { getObjectKey } from '@kbn/core-saved-objects-base-server-internal'; +import { SavedObjectsCollectMultiNamespaceReferencesPurpose } from '@kbn/core-saved-objects-api-server/src/apis'; +import { + KQL_FUNCTION_AND, + KQL_FUNCTION_IS, + KQL_FUNCTION_NOT, + KQL_FUNCTION_OR, +} from '@kbn/es-query/src/kuery/functions'; import type { CreatePointInTimeFinderFn } from '../../point_in_time_finder'; interface ObjectOrigin { /** The object's type. */ type: string; - /** The object's origin is its `originId` field, or its `id` field if that is unavailable. */ - origin: string; + /** The object's ID. */ + id: string; + /** The object's origin is its `originId` field */ + origin: string | undefined; } /** @@ -26,14 +35,15 @@ interface ObjectOrigin { export async function findSharedOriginObjects( createPointInTimeFinder: CreatePointInTimeFinderFn, objects: ObjectOrigin[], - perPage?: number + perPage?: number, + purpose?: SavedObjectsCollectMultiNamespaceReferencesPurpose ) { if (!objects.length) { return new Map>(); } const uniqueObjectTypes = objects.reduce((acc, { type }) => acc.add(type), new Set()); - const filter = createAliasKueryFilter(objects); + const filter = createOriginKueryFilter(objects, purpose); const finder = createPointInTimeFinder( { type: [...uniqueObjectTypes], @@ -80,17 +90,54 @@ export async function findSharedOriginObjects( return objectsMap; } -function createAliasKueryFilter(objects: Array<{ type: string; origin: string }>) { +function createOriginKueryFilter( + objects: ObjectOrigin[], + purpose?: SavedObjectsCollectMultiNamespaceReferencesPurpose +) { const { buildNode } = esKuery.nodeTypes.function; // Note: these nodes include '.attributes' for type-level fields because these are eventually passed to `validateConvertFilterToKueryNode`, which requires it const kueryNodes = objects - .reduce((acc, { type, origin }) => { + .reduce((acc, { type, id, origin }) => { // Escape Kuery values to prevent parsing errors and unintended behavior (object types/IDs can contain KQL special characters/operators) - const match1 = buildNode('is', `${type}.id`, esKuery.escapeKuery(`${type}:${origin}`)); // here we are looking for the raw document `_id` field, which has a `type:` prefix - const match2 = buildNode('is', `${type}.originId`, esKuery.escapeKuery(origin)); // here we are looking for the saved object's `originId` field, which does not have a `type:` prefix - acc.push([match1, match2]); + + // Look for objects with an ID that matches the origin or ID (has a `type:` prefix) + const idMatchesOrigin = buildNode( + KQL_FUNCTION_IS, + `${type}.id`, + esKuery.escapeKuery(`${type}:${origin || id}`) + ); + + // Look for objects with an `originId` that matches the origin or ID (does not have a `type:` prefix) + const originMatch = buildNode( + KQL_FUNCTION_IS, + `${type}.originId`, + esKuery.escapeKuery(origin || id) + ); + + // If we are updating an object's spaces (as opposed to copying)... + if (purpose === 'updateObjectsSpaces') { + // we never want to match on the raw document `_id` fields. + // If they are equal, this just means that the object already exists in that space and it's ok. + const notIdMatch = buildNode( + KQL_FUNCTION_NOT, + buildNode(KQL_FUNCTION_IS, `${type}.id`, esKuery.escapeKuery(`${type}:${id}`)) + ); + + // If this object has an origin ID, then we do still want to match if another object's ID matches the + // object's origin (idMatchesOrigin), or if another object's origin matches the object's origin (originMatch). + // But if this object does not have an origin ID, we can skip the idMatchesOrigin part altogether + // and just check if another object's origin ID matches this object's ID (originMatch). + // (maybe slightly more efficient?) + acc.push( + buildNode(KQL_FUNCTION_AND, [ + notIdMatch, + origin ? buildNode(KQL_FUNCTION_OR, [idMatchesOrigin, originMatch]) : originMatch, + ]) + ); + } else acc.push([idMatchesOrigin, originMatch]); // If we are copying, things are much simpler + return acc; }, []) .flat(); - return buildNode('or', kueryNodes); + return buildNode(KQL_FUNCTION_OR, kueryNodes); } diff --git a/packages/core/saved-objects/core-saved-objects-api-server/src/apis/collect_multinamespace_references.ts b/packages/core/saved-objects/core-saved-objects-api-server/src/apis/collect_multinamespace_references.ts index fcd0d079961bd..f13d2f7261329 100644 --- a/packages/core/saved-objects/core-saved-objects-api-server/src/apis/collect_multinamespace_references.ts +++ b/packages/core/saved-objects/core-saved-objects-api-server/src/apis/collect_multinamespace_references.ts @@ -24,6 +24,13 @@ export interface SavedObjectsCollectMultiNamespaceReferencesObject { type: string; } +/** + * Purpose for collecting references. + */ +export type SavedObjectsCollectMultiNamespaceReferencesPurpose = + | 'collectMultiNamespaceReferences' + | 'updateObjectsSpaces'; + /** * Options for collecting references. * @@ -32,7 +39,7 @@ export interface SavedObjectsCollectMultiNamespaceReferencesObject { export interface SavedObjectsCollectMultiNamespaceReferencesOptions extends SavedObjectsBaseOptions { /** Optional purpose used to determine filtering and authorization checks; default is 'collectMultiNamespaceReferences' */ - purpose?: 'collectMultiNamespaceReferences' | 'updateObjectsSpaces'; + purpose?: SavedObjectsCollectMultiNamespaceReferencesPurpose; } /** diff --git a/packages/core/saved-objects/core-saved-objects-api-server/src/apis/index.ts b/packages/core/saved-objects/core-saved-objects-api-server/src/apis/index.ts index ab3c3ca12f572..d343e1472b86d 100644 --- a/packages/core/saved-objects/core-saved-objects-api-server/src/apis/index.ts +++ b/packages/core/saved-objects/core-saved-objects-api-server/src/apis/index.ts @@ -35,6 +35,7 @@ export type { SavedObjectReferenceWithContext, SavedObjectsCollectMultiNamespaceReferencesResponse, SavedObjectsCollectMultiNamespaceReferencesOptions, + SavedObjectsCollectMultiNamespaceReferencesPurpose, } from './collect_multinamespace_references'; export type { SavedObjectsCreateOptions } from './create'; export type { diff --git a/x-pack/test/spaces_api_integration/common/suites/get_shareable_references.ts b/x-pack/test/spaces_api_integration/common/suites/get_shareable_references.ts index fb6c22a761f1e..bedc8a52409b6 100644 --- a/x-pack/test/spaces_api_integration/common/suites/get_shareable_references.ts +++ b/x-pack/test/spaces_api_integration/common/suites/get_shareable_references.ts @@ -51,7 +51,7 @@ export const EXPECTED_RESULTS: Record { ...TEST_CASE_OBJECTS.SHAREABLE_TYPE, spaces: [DEFAULT_SPACE_ID, SPACE_1_ID, SPACE_2_ID], - spacesWithMatchingOrigins: [DEFAULT_SPACE_ID, SPACE_1_ID, SPACE_2_ID], + // No matching origins because there are no copies of the object in another space (we no longer consider a raw ID match to be an origin match) inboundReferences: [{ type: 'sharedtype', id: CASES.DEFAULT_ONLY.id, name: 'refname' }], // only reflects inbound reference that exist in the default space }, { @@ -65,7 +65,7 @@ export const EXPECTED_RESULTS: Record type: 'sharedtype', id: CASES.DEFAULT_ONLY.id, spaces: [DEFAULT_SPACE_ID], - spacesWithMatchingOrigins: [DEFAULT_SPACE_ID], // The first test assertion for spacesWithMatchingOrigins is an object that doesn't have any matching origins in other spaces + // No matching origins because there are no copies of the object in another space (we no longer consider a raw ID match to be an origin match) inboundReferences: [{ ...TEST_CASE_OBJECTS.SHAREABLE_TYPE, name: 'refname' }], }, { @@ -86,7 +86,7 @@ export const EXPECTED_RESULTS: Record type: 'sharedtype', id: CASES.ALL_SPACES.id, spaces: ['*'], - spacesWithMatchingOrigins: ['*'], + // No matching origins because there are no copies of the object in another space (we no longer consider a raw ID match to be an origin match) inboundReferences: [{ ...TEST_CASE_OBJECTS.SHAREABLE_TYPE, name: 'refname' }], }, ], @@ -94,7 +94,7 @@ export const EXPECTED_RESULTS: Record { ...TEST_CASE_OBJECTS.SHAREABLE_TYPE, spaces: [DEFAULT_SPACE_ID, SPACE_1_ID, SPACE_2_ID], - spacesWithMatchingOrigins: [DEFAULT_SPACE_ID, SPACE_1_ID, SPACE_2_ID], + // No matching origins because there are no copies of the object in another space (we no longer consider a raw ID match to be an origin match) inboundReferences: [{ type: 'sharedtype', id: CASES.SPACE_1_ONLY.id, name: 'refname' }], // only reflects inbound reference that exist in space 1 }, { @@ -116,7 +116,7 @@ export const EXPECTED_RESULTS: Record id: CASES.SPACE_1_ONLY.id, spaces: [SPACE_1_ID], spacesWithMatchingAliases: [DEFAULT_SPACE_ID, SPACE_2_ID], // aliases with a matching targetType and sourceId exist in two other spaces - spacesWithMatchingOrigins: ['other_space', SPACE_1_ID], // The second test assertion for spacesWithMatchingOrigins is an object that has a matching origin in one other space + spacesWithMatchingOrigins: ['other_space'], // The second test assertion for spacesWithMatchingOrigins is an object that has a matching origin in one other space inboundReferences: [{ ...TEST_CASE_OBJECTS.SHAREABLE_TYPE, name: 'refname' }], }, { @@ -130,7 +130,7 @@ export const EXPECTED_RESULTS: Record type: 'sharedtype', id: CASES.ALL_SPACES.id, spaces: ['*'], - spacesWithMatchingOrigins: ['*'], + // No matching origins because there are no copies of the object in another space (we no longer consider a raw ID match to be an origin match) inboundReferences: [{ ...TEST_CASE_OBJECTS.SHAREABLE_TYPE, name: 'refname' }], }, ], @@ -138,7 +138,7 @@ export const EXPECTED_RESULTS: Record { ...TEST_CASE_OBJECTS.SHAREABLE_TYPE, spaces: [DEFAULT_SPACE_ID, SPACE_1_ID, SPACE_2_ID], - spacesWithMatchingOrigins: [DEFAULT_SPACE_ID, SPACE_1_ID, SPACE_2_ID], + // No matching origins because there are no copies of the object in another space (we no longer consider a raw ID match to be an origin match) inboundReferences: [{ type: 'sharedtype', id: CASES.SPACE_2_ONLY.id, name: 'refname' }], // only reflects inbound reference that exist in space 2 }, { @@ -173,7 +173,7 @@ export const EXPECTED_RESULTS: Record type: 'sharedtype', id: CASES.ALL_SPACES.id, spaces: ['*'], - spacesWithMatchingOrigins: ['*'], + // No matching origins because there are no copies of the object in another space (we no longer consider a raw ID match to be an origin match) inboundReferences: [{ ...TEST_CASE_OBJECTS.SHAREABLE_TYPE, name: 'refname' }], }, ],