From 89b892ab6c9480d198defa2b0992403788e651e3 Mon Sep 17 00:00:00 2001 From: Larry Gregory Date: Mon, 17 Sep 2018 12:47:52 -0400 Subject: [PATCH 1/2] delete objects belonging to removed space --- .../lib/__snapshots__/repository.test.js.snap | 5 + .../saved_objects/service/lib/repository.js | 34 ++++- .../service/lib/repository.test.js | 66 ++++++++++ x-pack/plugins/spaces/index.ts | 4 +- .../confirm_delete_modal.test.tsx.snap | 120 ++++++++++++------ .../components/confirm_delete_modal.tsx | 93 ++++++++++---- .../spaces/server/lib/spaces_client.test.ts | 9 ++ .../spaces/server/lib/spaces_client.ts | 2 + .../api/__fixtures__/create_test_handler.ts | 1 + .../saved_objects/spaces/data.json | 53 ++++++++ .../saved_objects/spaces/mappings.json | 5 +- .../common/lib/create_users_and_roles.ts | 8 +- .../common/suites/delete.ts | 71 ++++++++++- .../security_and_spaces/apis/delete.ts | 3 +- .../security_and_spaces/apis/index.ts | 6 +- .../spaces_only/apis/delete.ts | 3 +- 16 files changed, 403 insertions(+), 80 deletions(-) create mode 100644 src/server/saved_objects/service/lib/__snapshots__/repository.test.js.snap diff --git a/src/server/saved_objects/service/lib/__snapshots__/repository.test.js.snap b/src/server/saved_objects/service/lib/__snapshots__/repository.test.js.snap new file mode 100644 index 0000000000000..609906c97d599 --- /dev/null +++ b/src/server/saved_objects/service/lib/__snapshots__/repository.test.js.snap @@ -0,0 +1,5 @@ +// Jest Snapshot v1, https://goo.gl/fbAQLP + +exports[`SavedObjectsRepository #deleteByNamespace requires namespace to be a string 1`] = `"namespace is required, and must be a string"`; + +exports[`SavedObjectsRepository #deleteByNamespace requires namespace to be defined 1`] = `"namespace is required, and must be a string"`; diff --git a/src/server/saved_objects/service/lib/repository.js b/src/server/saved_objects/service/lib/repository.js index 694a22041d61c..b01b893fc93b4 100644 --- a/src/server/saved_objects/service/lib/repository.js +++ b/src/server/saved_objects/service/lib/repository.js @@ -18,7 +18,7 @@ */ import { omit } from 'lodash'; -import { getRootType } from '../../../mappings'; +import { getRootType, getRootPropertiesObjects } from '../../../mappings'; import { getSearchDsl } from './search_dsl'; import { includedFields } from './included_fields'; import { decorateEsError } from './decorate_es_error'; @@ -245,6 +245,38 @@ export class SavedObjectsRepository { ); } + /** + * Deletes all objects from the provided namespace. + * + * @param {string} namespace + * @returns {promise} - { took, timed_out, total, deleted, batches, version_conflicts, noops, retries, failures } + */ + async deleteByNamespace(namespace) { + + if (!namespace || typeof namespace !== 'string') { + throw new TypeError(`namespace is required, and must be a string`); + } + + const allTypes = Object.keys(getRootPropertiesObjects(this._mappings)); + + const typesToDelete = allTypes.filter(type => !this._schema.isNamespaceAgnostic(type)); + + const esOptions = { + index: this._index, + ignore: [404], + refresh: 'wait_for', + body: { + conflicts: 'proceed', + ...getSearchDsl(this._mappings, this._schema, { + namespace, + type: typesToDelete, + }) + } + }; + + return await this._writeToCluster('deleteByQuery', esOptions); + } + /** * @param {object} [options={}] * @property {(string|Array)} [options.type] diff --git a/src/server/saved_objects/service/lib/repository.test.js b/src/server/saved_objects/service/lib/repository.test.js index c332db6f72d28..c0dbcc964d588 100644 --- a/src/server/saved_objects/service/lib/repository.test.js +++ b/src/server/saved_objects/service/lib/repository.test.js @@ -162,6 +162,21 @@ describe('SavedObjectsRepository', () => { } }; + const deleteByQueryResults = { + took: 27, + timed_out: false, + total: 23, + deleted: 23, + batches: 1, + version_conflicts: 0, + noops: 0, + retries: { bulk: 0, search: 0 }, + throttled_millis: 0, + requests_per_second: -1, + throttled_until_millis: 0, + failures: [] + }; + const mappings = { doc: { properties: { @@ -171,6 +186,20 @@ describe('SavedObjectsRepository', () => { type: 'keyword' } } + }, + 'dashboard': { + properties: { + otherField: { + type: 'keyword' + } + } + }, + 'globaltype': { + properties: { + yetAnotherField: { + type: 'keyword' + } + } } } } @@ -708,6 +737,43 @@ describe('SavedObjectsRepository', () => { }); }); + describe('#deleteByNamespace', () => { + it('requires namespace to be defined', async () => { + callAdminCluster.returns(deleteByQueryResults); + expect(savedObjectsRepository.deleteByNamespace()).rejects.toThrowErrorMatchingSnapshot(); + sinon.assert.notCalled(callAdminCluster); + sinon.assert.notCalled(onBeforeWrite); + }); + + it('requires namespace to be a string', async () => { + callAdminCluster.returns(deleteByQueryResults); + expect(savedObjectsRepository.deleteByNamespace(['namespace-1', 'namespace-2'])).rejects.toThrowErrorMatchingSnapshot(); + sinon.assert.notCalled(callAdminCluster); + sinon.assert.notCalled(onBeforeWrite); + }); + + it('constructs a deleteByQuery call using all types that are namespace aware', async () => { + callAdminCluster.returns(deleteByQueryResults); + const result = await savedObjectsRepository.deleteByNamespace('my-namespace'); + + expect(result).toEqual(deleteByQueryResults); + sinon.assert.calledOnce(callAdminCluster); + sinon.assert.calledOnce(onBeforeWrite); + + sinon.assert.calledWithExactly(getSearchDsl, mappings, schema, { + namespace: 'my-namespace', + type: ['index-pattern', 'dashboard'] + }); + + sinon.assert.calledWithExactly(callAdminCluster, 'deleteByQuery', { + body: { conflicts: 'proceed' }, + ignore: [404], + index: '.kibana-test', + refresh: 'wait_for' + }); + }); + }); + describe('#find', () => { it('waits until migrations are complete before proceeding', async () => { migrator.awaitMigration = sinon.spy(async () => sinon.assert.notCalled(callAdminCluster)); diff --git a/x-pack/plugins/spaces/index.ts b/x-pack/plugins/spaces/index.ts index 00813193dedb4..c7c791e61f493 100644 --- a/x-pack/plugins/spaces/index.ts +++ b/x-pack/plugins/spaces/index.ts @@ -126,7 +126,9 @@ export const spaces = (kibana: any) => callWithRequestRepository, server.config(), internalRepository, - request + request, + savedObjects.schema, + savedObjects.types ); }, }); diff --git a/x-pack/plugins/spaces/public/views/management/components/__snapshots__/confirm_delete_modal.test.tsx.snap b/x-pack/plugins/spaces/public/views/management/components/__snapshots__/confirm_delete_modal.test.tsx.snap index 7f00705b160fb..0d95afebebbf9 100644 --- a/x-pack/plugins/spaces/public/views/management/components/__snapshots__/confirm_delete_modal.test.tsx.snap +++ b/x-pack/plugins/spaces/public/views/management/components/__snapshots__/confirm_delete_modal.test.tsx.snap @@ -2,52 +2,92 @@ exports[`ConfirmDeleteModal renders as expected 1`] = ` - -

- Deleting a space permanently removes the space and all of its contents. You can't undo this action. -

- - - - + + + Delete space + 'My Space' + + + - You are about to delete your current space - - ( +

+ Deleting a space permanently removes the space and + - My Space + all of its contents - ) - - . You will be redirected to choose a different space if you continue. + . You can't undo this action. +

+ + + + + + You are about to delete your current space + + ( + + My Space + + ) + + . You will be redirected to choose a different space if you continue. + + - - + + + + Cancel + + + Delete space and all contents + + + `; diff --git a/x-pack/plugins/spaces/public/views/management/components/confirm_delete_modal.tsx b/x-pack/plugins/spaces/public/views/management/components/confirm_delete_modal.tsx index 28515ba71593d..97f2f75da8321 100644 --- a/x-pack/plugins/spaces/public/views/management/components/confirm_delete_modal.tsx +++ b/x-pack/plugins/spaces/public/views/management/components/confirm_delete_modal.tsx @@ -5,11 +5,18 @@ */ import { + EuiButton, + EuiButtonEmpty, EuiCallOut, // @ts-ignore EuiConfirmModal, EuiFieldText, EuiFormRow, + EuiModal, + EuiModalBody, + EuiModalFooter, + EuiModalHeader, + EuiModalHeaderTitle, EuiOverlayMask, EuiText, } from '@elastic/eui'; @@ -29,12 +36,14 @@ interface Props { interface State { confirmSpaceName: string; error: boolean | null; + deleteInProgress: boolean; } export class ConfirmDeleteModal extends Component { public state = { confirmSpaceName: '', error: null, + deleteInProgress: false, }; public render() { @@ -57,32 +66,59 @@ export class ConfirmDeleteModal extends Component { ); } + // This is largely the same as the built-in EuiConfirmModal component, but we needed the ability + // to disable the buttons since this could be a long-running operation + return ( - -

- Deleting a space permanently removes the space and all of its contents. You can't undo - this action. -

- - - - - - {warning} - + + + + Delete space {`'${space.name}'`} + + + + +

+ Deleting a space permanently removes the space and{' '} + all of its contents. You can't undo this action. +

+ + + + + + {warning} + + + + + Cancel + + + + Delete space and all contents + + + ); } @@ -105,7 +141,16 @@ export class ConfirmDeleteModal extends Component { const needsRedirect = isDeletingCurrentSpace(this.props.space, this.props.spacesNavState); const spacesManager = this.props.spacesManager; + this.setState({ + deleteInProgress: true, + }); + await this.props.onConfirm(); + + this.setState({ + deleteInProgress: false, + }); + if (needsRedirect) { spacesManager.redirectToSpaceSelector(); } diff --git a/x-pack/plugins/spaces/server/lib/spaces_client.test.ts b/x-pack/plugins/spaces/server/lib/spaces_client.test.ts index b28501795788d..eaf2e2bc5590c 100644 --- a/x-pack/plugins/spaces/server/lib/spaces_client.test.ts +++ b/x-pack/plugins/spaces/server/lib/spaces_client.test.ts @@ -1072,7 +1072,9 @@ describe('#delete', () => { const mockCallWithRequestRepository = { get: jest.fn().mockReturnValue(notReservedSavedObject), delete: jest.fn(), + deleteByNamespace: jest.fn(), }; + const request = Symbol(); const client = new SpacesClient( @@ -1088,6 +1090,7 @@ describe('#delete', () => { expect(mockCallWithRequestRepository.get).toHaveBeenCalledWith('space', id); expect(mockCallWithRequestRepository.delete).toHaveBeenCalledWith('space', id); + expect(mockCallWithRequestRepository.deleteByNamespace).toHaveBeenCalledWith(id); expect(mockAuditLogger.spacesAuthorizationFailure).toHaveBeenCalledTimes(0); expect(mockAuditLogger.spacesAuthorizationSuccess).toHaveBeenCalledTimes(0); }); @@ -1127,7 +1130,9 @@ describe('#delete', () => { const mockCallWithRequestRepository = { get: jest.fn().mockReturnValue(notReservedSavedObject), delete: jest.fn(), + deleteByNamespace: jest.fn(), }; + const request = Symbol(); const client = new SpacesClient( @@ -1144,6 +1149,7 @@ describe('#delete', () => { expect(mockAuthorization.mode.useRbacForRequest).toHaveBeenCalledWith(request); expect(mockCallWithRequestRepository.get).toHaveBeenCalledWith('space', id); expect(mockCallWithRequestRepository.delete).toHaveBeenCalledWith('space', id); + expect(mockCallWithRequestRepository.deleteByNamespace).toHaveBeenCalledWith(id); expect(mockAuditLogger.spacesAuthorizationFailure).toHaveBeenCalledTimes(0); expect(mockAuditLogger.spacesAuthorizationSuccess).toHaveBeenCalledTimes(0); }); @@ -1226,7 +1232,9 @@ describe('#delete', () => { const mockInternalRepository = { get: jest.fn().mockReturnValue(notReservedSavedObject), delete: jest.fn(), + deleteByNamespace: jest.fn(), }; + const request = Symbol(); const client = new SpacesClient( mockAuditLogger as any, @@ -1246,6 +1254,7 @@ describe('#delete', () => { ); expect(mockInternalRepository.get).toHaveBeenCalledWith('space', id); expect(mockInternalRepository.delete).toHaveBeenCalledWith('space', id); + expect(mockInternalRepository.deleteByNamespace).toHaveBeenCalledWith(id); expect(mockAuditLogger.spacesAuthorizationFailure).toHaveBeenCalledTimes(0); expect(mockAuditLogger.spacesAuthorizationSuccess).toHaveBeenCalledWith(username, 'delete'); }); diff --git a/x-pack/plugins/spaces/server/lib/spaces_client.ts b/x-pack/plugins/spaces/server/lib/spaces_client.ts index e96481e30753c..f4fcc88c7e6f0 100644 --- a/x-pack/plugins/spaces/server/lib/spaces_client.ts +++ b/x-pack/plugins/spaces/server/lib/spaces_client.ts @@ -156,6 +156,8 @@ export class SpacesClient { } await repository.delete('space', id); + + await repository.deleteByNamespace(id); } private useRbac(): boolean { diff --git a/x-pack/plugins/spaces/server/routes/api/__fixtures__/create_test_handler.ts b/x-pack/plugins/spaces/server/routes/api/__fixtures__/create_test_handler.ts index e4c9652b8024b..20579915c2393 100644 --- a/x-pack/plugins/spaces/server/routes/api/__fixtures__/create_test_handler.ts +++ b/x-pack/plugins/spaces/server/routes/api/__fixtures__/create_test_handler.ts @@ -118,6 +118,7 @@ export function createTestHandler(initApiFn: (server: any, preCheckLicenseImpl: delete: jest.fn((type: string, id: string) => { return {}; }), + deleteByNamespace: jest.fn(), }; server.savedObjects = { 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 383f7083ff070..d4b32254613ad 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 @@ -49,3 +49,56 @@ } } } + +{ + "type": "doc", + "value": { + "index": ".kibana", + "type": "doc", + "id": "space_2:dashboard:my_dashboard", + "source": { + "type": "dashboard", + "updated_at": "2017-09-21T18:49:16.270Z", + "namespace": "space_2", + "dashboard": { + "description": "Space 2", + "title": "This is the second test space" + } + } + } +} + +{ + "type": "doc", + "value": { + "index": ".kibana", + "type": "doc", + "id": "space_1:dashboard:my_dashboard", + "source": { + "type": "dashboard", + "updated_at": "2017-09-21T18:49:16.270Z", + "namespace": "space_1", + "dashboard": { + "description": "Space 1", + "title": "This is the second test space" + } + } + } +} + +{ + "type": "doc", + "value": { + "index": ".kibana", + "type": "doc", + "id": "dashboard:my_dashboard", + "source": { + "type": "dashboard", + "updated_at": "2017-09-21T18:49:16.270Z", + "dashboard": { + "description": "Default Space", + "title": "This is the default test space" + } + } + } +} \ No newline at end of file diff --git a/x-pack/test/spaces_api_integration/common/fixtures/es_archiver/saved_objects/spaces/mappings.json b/x-pack/test/spaces_api_integration/common/fixtures/es_archiver/saved_objects/spaces/mappings.json index d8d4c52985bf5..4c8ef2bdc8580 100644 --- a/x-pack/test/spaces_api_integration/common/fixtures/es_archiver/saved_objects/spaces/mappings.json +++ b/x-pack/test/spaces_api_integration/common/fixtures/es_archiver/saved_objects/spaces/mappings.json @@ -55,6 +55,9 @@ } } }, + "namespace": { + "type": "keyword" + }, "dashboard": { "properties": { "description": { @@ -305,4 +308,4 @@ }, "aliases": {} } -} +} \ No newline at end of file diff --git a/x-pack/test/spaces_api_integration/common/lib/create_users_and_roles.ts b/x-pack/test/spaces_api_integration/common/lib/create_users_and_roles.ts index 0017888c63567..7fbfca1057eab 100644 --- a/x-pack/test/spaces_api_integration/common/lib/create_users_and_roles.ts +++ b/x-pack/test/spaces_api_integration/common/lib/create_users_and_roles.ts @@ -11,7 +11,7 @@ export const createUsersAndRoles = async (es: any, supertest: SuperTest) => elasticsearch: { indices: [ { - names: ['.kibana'], + names: ['.kibana*'], privileges: ['manage', 'read', 'index', 'delete'], }, ], @@ -22,7 +22,7 @@ export const createUsersAndRoles = async (es: any, supertest: SuperTest) => elasticsearch: { indices: [ { - names: ['.kibana'], + names: ['.kibana*'], privileges: ['read', 'view_index_metadata'], }, ], @@ -33,7 +33,7 @@ export const createUsersAndRoles = async (es: any, supertest: SuperTest) => elasticsearch: { indices: [ { - names: ['.kibana'], + names: ['.kibana*'], privileges: ['manage', 'read', 'index', 'delete'], }, ], @@ -47,7 +47,7 @@ export const createUsersAndRoles = async (es: any, supertest: SuperTest) => elasticsearch: { indices: [ { - names: ['.kibana'], + names: ['.kibana*'], privileges: ['read', 'view_index_metadata'], }, ], 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 3376e73420fb2..698b09fc8cd5f 100644 --- a/x-pack/test/spaces_api_integration/common/suites/delete.ts +++ b/x-pack/test/spaces_api_integration/common/suites/delete.ts @@ -25,7 +25,7 @@ interface DeleteTestDefinition { tests: DeleteTests; } -export function deleteTestSuiteFactory(esArchiver: any, supertest: SuperTest) { +export function deleteTestSuiteFactory(es: any, esArchiver: any, supertest: SuperTest) { const createExpectLegacyForbidden = (username: string, action: string) => (resp: { [key: string]: any; }) => { @@ -40,8 +40,75 @@ export function deleteTestSuiteFactory(esArchiver: any, supertest: SuperTest { + const expectEmptyResult = async (resp: { [key: string]: any }) => { expect(resp.body).to.eql(''); + + // Query ES to ensure that we deleted everything we expected, and nothing we didn't + // Grouping first by namespace, then by saved object type + const response = await es.search({ + index: '.kibana', + body: { + size: 0, + aggs: { + count: { + terms: { + field: 'namespace', + missing: 'default', + size: 10, + }, + aggs: { + countByType: { + terms: { + field: 'type', + missing: 'UNKNOWN', + size: 10, + }, + }, + }, + }, + }, + }, + }); + + const buckets = response.aggregations.count.buckets; + + // Space 2 deleted, all others should exist + const expectedBuckets = [ + { + key: 'default', + doc_count: 3, + countByType: { + doc_count_error_upper_bound: 0, + sum_other_doc_count: 0, + buckets: [ + { + key: 'space', + doc_count: 2, + }, + { + key: 'dashboard', + doc_count: 1, + }, + ], + }, + }, + { + doc_count: 1, + key: 'space_1', + countByType: { + doc_count_error_upper_bound: 0, + sum_other_doc_count: 0, + buckets: [ + { + key: 'dashboard', + doc_count: 1, + }, + ], + }, + }, + ]; + + expect(buckets).to.eql(expectedBuckets); }; const expectNotFound = (resp: { [key: string]: any }) => { diff --git a/x-pack/test/spaces_api_integration/security_and_spaces/apis/delete.ts b/x-pack/test/spaces_api_integration/security_and_spaces/apis/delete.ts index 7d2d124f1a374..a0bea80f034e2 100644 --- a/x-pack/test/spaces_api_integration/security_and_spaces/apis/delete.ts +++ b/x-pack/test/spaces_api_integration/security_and_spaces/apis/delete.ts @@ -13,6 +13,7 @@ import { deleteTestSuiteFactory } from '../../common/suites/delete'; export default function deleteSpaceTestSuite({ getService }: TestInvoker) { const supertestWithoutAuth = getService('supertestWithoutAuth'); const esArchiver = getService('esArchiver'); + const es = getService('es'); const { deleteTest, @@ -21,7 +22,7 @@ export default function deleteSpaceTestSuite({ getService }: TestInvoker) { expectEmptyResult, expectNotFound, expectReservedSpaceResult, - } = deleteTestSuiteFactory(esArchiver, supertestWithoutAuth); + } = deleteTestSuiteFactory(es, esArchiver, supertestWithoutAuth); describe('delete', () => { [ diff --git a/x-pack/test/spaces_api_integration/security_and_spaces/apis/index.ts b/x-pack/test/spaces_api_integration/security_and_spaces/apis/index.ts index 044670a822ac2..65ab34e7ae8f8 100644 --- a/x-pack/test/spaces_api_integration/security_and_spaces/apis/index.ts +++ b/x-pack/test/spaces_api_integration/security_and_spaces/apis/index.ts @@ -3,11 +3,7 @@ * or more contributor license agreements. Licensed under the Elastic License; * you may not use this file except in compliance with the Elastic License. */ -/* - * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one - * or more contributor license agreements. Licensed under the Elastic License; - * you may not use this file except in compliance with the Elastic License. - */ + import { createUsersAndRoles } from '../../common/lib/create_users_and_roles'; import { TestInvoker } from '../../common/lib/types'; diff --git a/x-pack/test/spaces_api_integration/spaces_only/apis/delete.ts b/x-pack/test/spaces_api_integration/spaces_only/apis/delete.ts index a0902281f4c73..29045a21060e7 100644 --- a/x-pack/test/spaces_api_integration/spaces_only/apis/delete.ts +++ b/x-pack/test/spaces_api_integration/spaces_only/apis/delete.ts @@ -12,13 +12,14 @@ import { deleteTestSuiteFactory } from '../../common/suites/delete'; export default function deleteSpaceTestSuite({ getService }: TestInvoker) { const supertestWithoutAuth = getService('supertestWithoutAuth'); const esArchiver = getService('esArchiver'); + const es = getService('es'); const { deleteTest, expectEmptyResult, expectReservedSpaceResult, expectNotFound, - } = deleteTestSuiteFactory(esArchiver, supertestWithoutAuth); + } = deleteTestSuiteFactory(es, esArchiver, supertestWithoutAuth); describe('delete', () => { [ From 68973c1da7b8fab66d56b814262e4ea11b815f22 Mon Sep 17 00:00:00 2001 From: Larry Gregory Date: Mon, 1 Oct 2018 09:32:21 -0400 Subject: [PATCH 2/2] remove unused parameters --- x-pack/plugins/spaces/index.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/x-pack/plugins/spaces/index.ts b/x-pack/plugins/spaces/index.ts index c7c791e61f493..00813193dedb4 100644 --- a/x-pack/plugins/spaces/index.ts +++ b/x-pack/plugins/spaces/index.ts @@ -126,9 +126,7 @@ export const spaces = (kibana: any) => callWithRequestRepository, server.config(), internalRepository, - request, - savedObjects.schema, - savedObjects.types + request ); }, });