From 4236bb9a2ed6cd46682e6497325398f37d69fbbe Mon Sep 17 00:00:00 2001 From: Joe Portner <5295965+jportner@users.noreply.github.com> Date: Tue, 2 Nov 2021 17:06:29 -0400 Subject: [PATCH] Change saved object updateObjectsSpaces API to use deleteLegacyUrlAliases --- .../saved_objects/spaces/data.json | 88 +++++++++++++++++++ .../common/lib/saved_object_test_cases.ts | 8 ++ .../common/suites/delete.ts | 16 ++-- .../common/suites/update_objects_spaces.ts | 32 ++++++- .../apis/update_objects_spaces.ts | 4 + .../spaces_only/apis/update_objects_spaces.ts | 57 +++++++++++- 6 files changed, 191 insertions(+), 14 deletions(-) diff --git a/x-pack/test/spaces_api_integration/common/fixtures/es_archiver/saved_objects/spaces/data.json b/x-pack/test/spaces_api_integration/common/fixtures/es_archiver/saved_objects/spaces/data.json index c9b09456a9a492..c5dc147b45123c 100644 --- a/x-pack/test/spaces_api_integration/common/fixtures/es_archiver/saved_objects/spaces/data.json +++ b/x-pack/test/spaces_api_integration/common/fixtures/es_archiver/saved_objects/spaces/data.json @@ -582,6 +582,94 @@ } } +{ + "type": "doc", + "value": { + "id": "sharedtype:alias_delete_inclusive", + "index": ".kibana", + "source": { + "sharedtype": { + "title": "This is used to test that when an object is unshared from a space, inbound aliases for just those spaces are removed" + }, + "type": "sharedtype", + "namespaces": ["default", "space_1", "space_2"], + "updated_at": "2017-09-21T18:59:16.270Z" + }, + "type": "doc" + } +} + +{ + "type": "doc", + "value": { + "id": "legacy-url-alias:space_1:sharedtype:doesnt-matter", + "index": ".kibana", + "source": { + "type": "legacy-url-alias", + "updated_at": "2017-09-21T18:51:23.794Z", + "legacy-url-alias": { + "sourceId": "doesnt-matter", + "targetNamespace": "space_1", + "targetType": "sharedtype", + "targetId": "alias_delete_inclusive" + } + } + } +} + +{ + "type": "doc", + "value": { + "id": "legacy-url-alias:space_2:sharedtype:doesnt-matter", + "index": ".kibana", + "source": { + "type": "legacy-url-alias", + "updated_at": "2017-09-21T18:51:23.794Z", + "legacy-url-alias": { + "sourceId": "doesnt-matter", + "targetNamespace": "space_2", + "targetType": "sharedtype", + "targetId": "alias_delete_inclusive" + } + } + } +} + +{ + "type": "doc", + "value": { + "id": "sharedtype:alias_delete_exclusive", + "index": ".kibana", + "source": { + "sharedtype": { + "title": "This is used to test that when an object is unshared from all space, inbound aliases for all spaces are removed" + }, + "type": "sharedtype", + "namespaces": ["*"], + "updated_at": "2017-09-21T18:59:16.270Z" + }, + "type": "doc" + } +} + +{ + "type": "doc", + "value": { + "id": "legacy-url-alias:other_space:sharedtype:doesnt-matter", + "index": ".kibana", + "source": { + "type": "legacy-url-alias", + "updated_at": "2017-09-21T18:51:23.794Z", + "legacy-url-alias": { + "sourceId": "doesnt-matter", + "targetNamespace": "other_space", + "targetType": "sharedtype", + "targetId": "alias_delete_exclusive" + } + } + } +} + { "type": "doc", "value": { diff --git a/x-pack/test/spaces_api_integration/common/lib/saved_object_test_cases.ts b/x-pack/test/spaces_api_integration/common/lib/saved_object_test_cases.ts index ddbcf8f5f31c17..8d9af1170f288f 100644 --- a/x-pack/test/spaces_api_integration/common/lib/saved_object_test_cases.ts +++ b/x-pack/test/spaces_api_integration/common/lib/saved_object_test_cases.ts @@ -38,6 +38,14 @@ export const MULTI_NAMESPACE_SAVED_OBJECT_TEST_CASES = Object.freeze({ id: 'all_spaces', existingNamespaces: ['*'], // all current and future spaces }), + ALIAS_DELETE_INCLUSIVE: Object.freeze({ + id: 'alias_delete_inclusive', + existingNamespaces: ['default', 'space_1', 'space_2'], // each individual space + }), + ALIAS_DELETE_EXCLUSIVE: Object.freeze({ + id: 'alias_delete_exclusive', + existingNamespaces: ['*'], // all current and future spaces + }), DOES_NOT_EXIST: Object.freeze({ id: 'does_not_exist', existingNamespaces: [] as string[], diff --git a/x-pack/test/spaces_api_integration/common/suites/delete.ts b/x-pack/test/spaces_api_integration/common/suites/delete.ts index e0f222af707c54..ae8b73535c2c65 100644 --- a/x-pack/test/spaces_api_integration/common/suites/delete.ts +++ b/x-pack/test/spaces_api_integration/common/suites/delete.ts @@ -53,8 +53,8 @@ export function deleteTestSuiteFactory(es: Client, esArchiver: any, supertest: S // @ts-expect-error @elastic/elasticsearch doesn't defined `count.buckets`. const buckets = response.aggregations?.count.buckets; - // The test fixture contains three legacy URL aliases: - // (1) one for "space_1", (2) one for "space_2", and (3) one for "other_space", which is a non-existent space. + // The test fixture contains six legacy URL aliases: + // (1) two for "space_1", (2) two for "space_2", and (3) two for "other_space", which is a non-existent space. // Each test deletes "space_2", so the agg buckets should reflect that aliases (1) and (3) still exist afterwards. // Space 2 deleted, all others should exist @@ -75,26 +75,26 @@ export function deleteTestSuiteFactory(es: Client, esArchiver: any, supertest: S }, }, { - doc_count: 6, + doc_count: 7, key: 'space_1', countByType: { doc_count_error_upper_bound: 0, sum_other_doc_count: 0, buckets: [ { key: 'visualization', doc_count: 3 }, + { key: 'legacy-url-alias', doc_count: 2 }, // aliases (1) { key: 'dashboard', doc_count: 1 }, { key: 'index-pattern', doc_count: 1 }, - { key: 'legacy-url-alias', doc_count: 1 }, // alias (1) ], }, }, { - doc_count: 1, + doc_count: 2, key: 'other_space', countByType: { doc_count_error_upper_bound: 0, sum_other_doc_count: 0, - buckets: [{ key: 'legacy-url-alias', doc_count: 1 }], // alias (3) + buckets: [{ key: 'legacy-url-alias', doc_count: 2 }], // aliases (3) }, }, ]; @@ -110,8 +110,8 @@ export function deleteTestSuiteFactory(es: Client, esArchiver: any, supertest: S body: { query: { terms: { type: ['sharedtype'] } } }, }); const docs = multiNamespaceResponse.hits.hits; - // Just 12 results, since spaces_2_only, conflict_1_space_2 and conflict_2_space_2 got deleted. - expect(docs).length(12); + // Just 14 results, since spaces_2_only, conflict_1_space_2 and conflict_2_space_2 got deleted. + expect(docs).length(14); docs.forEach((doc) => () => { const containsSpace2 = doc?._source?.namespaces.includes('space_2'); expect(containsSpace2).to.eql(false); diff --git a/x-pack/test/spaces_api_integration/common/suites/update_objects_spaces.ts b/x-pack/test/spaces_api_integration/common/suites/update_objects_spaces.ts index ecd0d15b522e17..d2d35c303c512a 100644 --- a/x-pack/test/spaces_api_integration/common/suites/update_objects_spaces.ts +++ b/x-pack/test/spaces_api_integration/common/suites/update_objects_spaces.ts @@ -6,6 +6,8 @@ */ import expect from '@kbn/expect'; +import type { Client } from '@elastic/elasticsearch'; +import type { SearchTotalHits } from '@elastic/elasticsearch/lib/api/types'; import { without, uniq } from 'lodash'; import { SuperTest } from 'supertest'; import { @@ -35,6 +37,7 @@ export interface UpdateObjectsSpacesTestCase { objects: Array<{ id: string; existingNamespaces: string[]; + expectAliasDifference?: number; failure?: 400 | 404; }>; spacesToAdd: string[]; @@ -54,7 +57,11 @@ const getTestTitle = ({ objects, spacesToAdd, spacesToRemove }: UpdateObjectsSpa return `{objects: [${objStr}], spacesToAdd: [${addStr}], spacesToRemove: [${remStr}]}`; }; -export function updateObjectsSpacesTestSuiteFactory(esArchiver: any, supertest: SuperTest) { +export function updateObjectsSpacesTestSuiteFactory( + es: Client, + esArchiver: any, + supertest: SuperTest +) { const expectForbidden = expectResponses.forbiddenTypes('share_to_space'); const expectResponseBody = ( @@ -68,7 +75,9 @@ export function updateObjectsSpacesTestSuiteFactory(esArchiver: any, supertest: } else { const { objects, spacesToAdd, spacesToRemove } = testCase; const apiResponse = response.body as SavedObjectsUpdateObjectsSpacesResponse; - objects.forEach(({ id, existingNamespaces, failure }, i) => { + + for (let i = 0; i < objects.length; i++) { + const { id, existingNamespaces, expectAliasDifference, failure } = objects[i]; const object = apiResponse.objects[i]; if (failure === 404) { const error = SavedObjectsErrorHelpers.createGenericNotFoundError(TYPE, id); @@ -84,8 +93,25 @@ export function updateObjectsSpacesTestSuiteFactory(esArchiver: any, supertest: expect(result.type).to.eql(TYPE); expect(result.id).to.eql(id); expect(result.spaces.sort()).to.eql(expectedSpaces.sort()); + + if (expectAliasDifference !== undefined) { + // if we deleted an object that had an alias pointing to it, the alias should have been deleted as well + await es.indices.refresh({ index: '.kibana' }); // alias deletion uses refresh: false, so we need to manually refresh the index before searching + const searchResponse = await es.search({ + index: '.kibana', + body: { + size: 0, + query: { terms: { type: ['legacy-url-alias'] } }, + track_total_hits: true, + }, + }); + expect((searchResponse.hits.total as SearchTotalHits).value).to.eql( + // Six aliases exist in the test fixtures + 6 + expectAliasDifference + ); + } } - }); + } } }; const createTestDefinitions = ( diff --git a/x-pack/test/spaces_api_integration/security_and_spaces/apis/update_objects_spaces.ts b/x-pack/test/spaces_api_integration/security_and_spaces/apis/update_objects_spaces.ts index 36f50aa165e72d..c6a97337e6ad93 100644 --- a/x-pack/test/spaces_api_integration/security_and_spaces/apis/update_objects_spaces.ts +++ b/x-pack/test/spaces_api_integration/security_and_spaces/apis/update_objects_spaces.ts @@ -28,6 +28,8 @@ const { fail404 } = testCaseFailures; const createTestCases = (spaceId: string): UpdateObjectsSpacesTestCase[] => { const eachSpace = [DEFAULT_SPACE_ID, SPACE_1_ID, SPACE_2_ID]; + // Note: we intentionally exclude ALIAS_DELETION test cases because they are already covered in spaces_only test suite, and there is no + // authZ-specific logic that affects alias deletion, all of that happens at the Saved Objects Repository level. return [ // Test case to check adding and removing all spaces ("*") to a saved object { @@ -125,8 +127,10 @@ const calculateSingleSpaceAuthZ = (testCases: UpdateObjectsSpacesTestCase[], spa export default function ({ getService }: FtrProviderContext) { const supertest = getService('supertestWithoutAuth'); const esArchiver = getService('esArchiver'); + const es = getService('es'); const { addTests, createTestDefinitions } = updateObjectsSpacesTestSuiteFactory( + es, esArchiver, supertest ); diff --git a/x-pack/test/spaces_api_integration/spaces_only/apis/update_objects_spaces.ts b/x-pack/test/spaces_api_integration/spaces_only/apis/update_objects_spaces.ts index 865d5eca22cbd8..fc95f513f55195 100644 --- a/x-pack/test/spaces_api_integration/spaces_only/apis/update_objects_spaces.ts +++ b/x-pack/test/spaces_api_integration/spaces_only/apis/update_objects_spaces.ts @@ -11,6 +11,7 @@ import { getTestScenarios, } from '../../../saved_object_api_integration/common/lib/saved_object_test_utils'; import { MULTI_NAMESPACE_SAVED_OBJECT_TEST_CASES as CASES } from '../../common/lib/saved_object_test_cases'; +import type { UpdateObjectsSpacesTestCase } from '../../common/suites/update_objects_spaces'; import { updateObjectsSpacesTestSuiteFactory } from '../../common/suites/update_objects_spaces'; import { FtrProviderContext } from '../../common/ftr_provider_context'; @@ -51,7 +52,55 @@ const createSinglePartTestCases = (spaceId: string) => { const createMultiPartTestCases = () => { const nonExistentSpace = 'does_not_exist'; // space that doesn't exist const eachSpace = [DEFAULT_SPACE_ID, SPACE_1_ID, SPACE_2_ID]; - const group1 = [ + const group1: UpdateObjectsSpacesTestCase[] = [ + // These test cases ensure that aliases are deleted when objects are unshared. + // For simplicity these are done separately, before the others. + { + objects: [ + { + id: CASES.ALIAS_DELETE_INCLUSIVE.id, + existingNamespaces: eachSpace, + expectAliasDifference: -1, // one alias should have been deleted from space_2 + }, + ], + spacesToAdd: [], + spacesToRemove: [SPACE_2_ID], + }, + { + objects: [ + { + id: CASES.ALIAS_DELETE_INCLUSIVE.id, + existingNamespaces: [DEFAULT_SPACE_ID, SPACE_1_ID], + expectAliasDifference: -2, // one alias should have been deleted from space_1 + }, + ], + spacesToAdd: [], + spacesToRemove: [SPACE_1_ID], + }, + { + objects: [ + { + id: CASES.ALIAS_DELETE_INCLUSIVE.id, + existingNamespaces: [DEFAULT_SPACE_ID], + expectAliasDifference: -2, // no aliases can exist in the default space, so no aliases were deleted + }, + ], + spacesToAdd: [], + spacesToRemove: [DEFAULT_SPACE_ID], + }, + { + objects: [ + { + id: CASES.ALIAS_DELETE_EXCLUSIVE.id, + existingNamespaces: [SPACE_1_ID], + expectAliasDifference: -3, // one alias should have been deleted from other_space + }, + ], + spacesToAdd: [SPACE_1_ID], + spacesToRemove: ['*'], + }, + ]; + const group2 = [ // first, add this object to each space and remove it from nonExistentSpace // this will succeed even though the object already exists in the default space and it doesn't exist in nonExistentSpace { objects: [CASES.DEFAULT_ONLY], spacesToAdd: eachSpace, spacesToRemove: [nonExistentSpace] }, @@ -87,7 +136,7 @@ const createMultiPartTestCases = () => { spacesToRemove: [SPACE_1_ID], }, ]; - const group2 = [ + const group3 = [ // first, add this object to space_2 and remove it from space_1 { objects: [CASES.DEFAULT_AND_SPACE_1], @@ -111,15 +160,17 @@ const createMultiPartTestCases = () => { spacesToRemove: [], }, ]; - return [...group1, ...group2]; + return [...group1, ...group2, ...group3]; }; // eslint-disable-next-line import/no-default-export export default function ({ getService }: FtrProviderContext) { const supertest = getService('supertest'); const esArchiver = getService('esArchiver'); + const es = getService('es'); const { addTests, createTestDefinitions } = updateObjectsSpacesTestSuiteFactory( + es, esArchiver, supertest );