From d0c0249017b2135682b14845ee026c4854fcd204 Mon Sep 17 00:00:00 2001 From: Gidi Meir Morris Date: Wed, 29 Jul 2020 16:33:50 +0100 Subject: [PATCH 01/41] use create to overwrite actions on update in action --- .../actions/server/actions_client.test.ts | 26 +++-- .../plugins/actions/server/actions_client.ts | 26 +++-- .../actions/server/saved_objects/index.ts | 1 + .../encrypted_saved_object_type_definition.ts | 2 + .../encrypted_saved_objects_service.mocks.ts | 4 + .../encrypted_saved_objects_service.test.ts | 15 +++ .../crypto/encrypted_saved_objects_service.ts | 9 ++ ...ypted_saved_objects_client_wrapper.test.ts | 110 ++++++++++++++++++ .../encrypted_saved_objects_client_wrapper.ts | 8 +- .../actions/builtin_action_types/webhook.ts | 71 +++++++++++ 10 files changed, 249 insertions(+), 23 deletions(-) diff --git a/x-pack/plugins/actions/server/actions_client.test.ts b/x-pack/plugins/actions/server/actions_client.test.ts index 90b989ac3b52eb..493031f379cc4b 100644 --- a/x-pack/plugins/actions/server/actions_client.test.ts +++ b/x-pack/plugins/actions/server/actions_client.test.ts @@ -893,7 +893,7 @@ describe('update()', () => { }, references: [], }); - unsecuredSavedObjectsClient.update.mockResolvedValueOnce({ + unsecuredSavedObjectsClient.create.mockResolvedValueOnce({ id: 'my-action', type: 'action', attributes: { @@ -946,7 +946,7 @@ describe('update()', () => { }, references: [], }); - unsecuredSavedObjectsClient.update.mockResolvedValueOnce({ + unsecuredSavedObjectsClient.create.mockResolvedValueOnce({ id: 'my-action', type: 'action', attributes: { @@ -972,17 +972,20 @@ describe('update()', () => { name: 'my name', config: {}, }); - expect(unsecuredSavedObjectsClient.update).toHaveBeenCalledTimes(1); - expect(unsecuredSavedObjectsClient.update.mock.calls[0]).toMatchInlineSnapshot(` + expect(unsecuredSavedObjectsClient.create).toHaveBeenCalledTimes(1); + expect(unsecuredSavedObjectsClient.create.mock.calls[0]).toMatchInlineSnapshot(` Array [ "action", - "my-action", Object { "actionTypeId": "my-action-type", "config": Object {}, "name": "my name", "secrets": Object {}, }, + Object { + "id": "my-action", + "overwrite": true, + }, ] `); expect(unsecuredSavedObjectsClient.get).toHaveBeenCalledTimes(1); @@ -1043,7 +1046,7 @@ describe('update()', () => { }, references: [], }); - unsecuredSavedObjectsClient.update.mockResolvedValueOnce({ + unsecuredSavedObjectsClient.create.mockResolvedValueOnce({ id: 'my-action', type: 'action', attributes: { @@ -1081,11 +1084,10 @@ describe('update()', () => { c: true, }, }); - expect(unsecuredSavedObjectsClient.update).toHaveBeenCalledTimes(1); - expect(unsecuredSavedObjectsClient.update.mock.calls[0]).toMatchInlineSnapshot(` + expect(unsecuredSavedObjectsClient.create).toHaveBeenCalledTimes(1); + expect(unsecuredSavedObjectsClient.create.mock.calls[0]).toMatchInlineSnapshot(` Array [ "action", - "my-action", Object { "actionTypeId": "my-action-type", "config": Object { @@ -1096,6 +1098,10 @@ describe('update()', () => { "name": "my name", "secrets": Object {}, }, + Object { + "id": "my-action", + "overwrite": true, + }, ] `); }); @@ -1118,7 +1124,7 @@ describe('update()', () => { }, references: [], }); - unsecuredSavedObjectsClient.update.mockResolvedValueOnce({ + unsecuredSavedObjectsClient.create.mockResolvedValueOnce({ id: 'my-action', type: 'action', attributes: { diff --git a/x-pack/plugins/actions/server/actions_client.ts b/x-pack/plugins/actions/server/actions_client.ts index 6744a8d1116234..a09ffe373fb519 100644 --- a/x-pack/plugins/actions/server/actions_client.ts +++ b/x-pack/plugins/actions/server/actions_client.ts @@ -159,18 +159,26 @@ export class ActionsClient { this.actionTypeRegistry.ensureActionTypeEnabled(actionTypeId); - const result = await this.unsecuredSavedObjectsClient.update('action', id, { - actionTypeId, - name, - config: validatedActionTypeConfig as SavedObjectAttributes, - secrets: validatedActionTypeSecrets as SavedObjectAttributes, - }); + // SOs client does partial updates - we want to overwrite the whole object, so we'll use create with overwrite + const result = await this.unsecuredSavedObjectsClient.create( + 'action', + { + actionTypeId, + name, + config: validatedActionTypeConfig as SavedObjectAttributes, + secrets: validatedActionTypeSecrets as SavedObjectAttributes, + }, + { + id, + overwrite: true, + } + ); return { id, - actionTypeId: result.attributes.actionTypeId as string, - name: result.attributes.name as string, - config: result.attributes.config as Record, + actionTypeId: result.attributes.actionTypeId, + name: result.attributes.name, + config: result.attributes.config, isPreconfigured: false, }; } diff --git a/x-pack/plugins/actions/server/saved_objects/index.ts b/x-pack/plugins/actions/server/saved_objects/index.ts index 54f186acc1ba57..353c01c1e57945 100644 --- a/x-pack/plugins/actions/server/saved_objects/index.ts +++ b/x-pack/plugins/actions/server/saved_objects/index.ts @@ -30,6 +30,7 @@ export function setupSavedObjects( type: ACTION_SAVED_OBJECT_TYPE, attributesToEncrypt: new Set(['secrets']), attributesToExcludeFromAAD: new Set(['name']), + allowsExplicitIDs: true, }); savedObjects.registerType({ diff --git a/x-pack/plugins/encrypted_saved_objects/server/crypto/encrypted_saved_object_type_definition.ts b/x-pack/plugins/encrypted_saved_objects/server/crypto/encrypted_saved_object_type_definition.ts index 849a2888b6e1a5..cd57faed89d0ea 100644 --- a/x-pack/plugins/encrypted_saved_objects/server/crypto/encrypted_saved_object_type_definition.ts +++ b/x-pack/plugins/encrypted_saved_objects/server/crypto/encrypted_saved_object_type_definition.ts @@ -13,6 +13,7 @@ import { EncryptedSavedObjectTypeRegistration } from './encrypted_saved_objects_ */ export class EncryptedSavedObjectAttributesDefinition { public readonly attributesToEncrypt: ReadonlySet; + public readonly allowsExplicitIDs: Readonly; private readonly attributesToExcludeFromAAD: ReadonlySet | undefined; private readonly attributesToStrip: ReadonlySet; @@ -34,6 +35,7 @@ export class EncryptedSavedObjectAttributesDefinition { this.attributesToEncrypt = attributesToEncrypt; this.attributesToStrip = attributesToStrip; this.attributesToExcludeFromAAD = typeRegistration.attributesToExcludeFromAAD; + this.allowsExplicitIDs = typeRegistration.allowsExplicitIDs ?? false; } /** diff --git a/x-pack/plugins/encrypted_saved_objects/server/crypto/encrypted_saved_objects_service.mocks.ts b/x-pack/plugins/encrypted_saved_objects/server/crypto/encrypted_saved_objects_service.mocks.ts index c692d8698771fe..fdc8e0e4ba4bb5 100644 --- a/x-pack/plugins/encrypted_saved_objects/server/crypto/encrypted_saved_objects_service.mocks.ts +++ b/x-pack/plugins/encrypted_saved_objects/server/crypto/encrypted_saved_objects_service.mocks.ts @@ -13,6 +13,7 @@ import { function createEncryptedSavedObjectsServiceMock() { return ({ isRegistered: jest.fn(), + allowsExplicitIDs: jest.fn(), stripOrDecryptAttributes: jest.fn(), encryptAttributes: jest.fn(), decryptAttributes: jest.fn(), @@ -49,6 +50,9 @@ export const encryptedSavedObjectsServiceMock = { return clonedAttrs; } + mock.allowsExplicitIDs.mockImplementation( + (type) => registrations.find((r) => r.type === type)?.allowsExplicitIDs ?? false + ); mock.isRegistered.mockImplementation( (type) => registrations.findIndex((r) => r.type === type) >= 0 ); diff --git a/x-pack/plugins/encrypted_saved_objects/server/crypto/encrypted_saved_objects_service.test.ts b/x-pack/plugins/encrypted_saved_objects/server/crypto/encrypted_saved_objects_service.test.ts index 42d2e2ffd15163..8230e09414e01d 100644 --- a/x-pack/plugins/encrypted_saved_objects/server/crypto/encrypted_saved_objects_service.test.ts +++ b/x-pack/plugins/encrypted_saved_objects/server/crypto/encrypted_saved_objects_service.test.ts @@ -85,6 +85,21 @@ describe('#isRegistered', () => { }); }); +describe('#allowsExplicitIDs', () => { + it('correctly returns allowsExplicitIDs when the specified type is registered', () => { + service.registerType({ + type: 'allowsExplicitIDs-type-1', + attributesToEncrypt: new Set(['attr-1']), + allowsExplicitIDs: true, + }); + expect(service.allowsExplicitIDs('allowsExplicitIDs-type-1')).toBe(true); + }); + + it('correctly defaults to false when the specified type is not registered', () => { + expect(service.allowsExplicitIDs('unknown-type-1')).toBe(false); + }); +}); + describe('#stripOrDecryptAttributes', () => { it('does not strip attributes from unknown types', async () => { const attributes = { attrOne: 'one', attrTwo: 'two', attrThree: 'three' }; diff --git a/x-pack/plugins/encrypted_saved_objects/server/crypto/encrypted_saved_objects_service.ts b/x-pack/plugins/encrypted_saved_objects/server/crypto/encrypted_saved_objects_service.ts index 99361107047c27..0c9339085b898f 100644 --- a/x-pack/plugins/encrypted_saved_objects/server/crypto/encrypted_saved_objects_service.ts +++ b/x-pack/plugins/encrypted_saved_objects/server/crypto/encrypted_saved_objects_service.ts @@ -31,6 +31,7 @@ export interface EncryptedSavedObjectTypeRegistration { readonly type: string; readonly attributesToEncrypt: ReadonlySet; readonly attributesToExcludeFromAAD?: ReadonlySet; + readonly allowsExplicitIDs?: boolean; } /** @@ -121,6 +122,14 @@ export class EncryptedSavedObjectsService { return this.typeDefinitions.has(type); } + /** + * Returns whether the type allows explciit IDs when creating a saved object + * resolves to `false` if the type is not registered + */ + public allowsExplicitIDs(type: string) { + return this.typeDefinitions.get(type)?.allowsExplicitIDs ?? false; + } + /** * Takes saved object attributes for the specified type and, depending on the type definition, * either decrypts or strips encrypted attributes (e.g. in case AAD or encryption key has changed diff --git a/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.test.ts b/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.test.ts index 5d4ea5a6370e4a..70a5f06bf56a39 100644 --- a/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.test.ts +++ b/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.test.ts @@ -30,6 +30,14 @@ beforeEach(() => { { key: 'attrNotSoSecret', dangerouslyExposeValue: true }, ]), }, + { + type: 'known-type-with-allowed-explicit-ids', + allowsExplicitIDs: true, + attributesToEncrypt: new Set([ + 'attrSecret', + { key: 'attrNotSoSecret', dangerouslyExposeValue: true }, + ]), + }, ]); wrapper = new EncryptedSavedObjectsClientWrapper({ @@ -121,6 +129,48 @@ describe('#create', () => { ); }); + it('allows explicit IDs when specified type allows it', async () => { + const attributes = { + attrOne: 'one', + attrSecret: 'secret', + attrNotSoSecret: 'not-so-secret', + attrThree: 'three', + }; + const options = { overwrite: true, id: 'custom-uuid' }; + const mockedResponse = { + id: 'custom-uuid', + type: 'known-type-with-allowed-explicit-ids', + attributes: { + attrOne: 'one', + attrSecret: '*secret*', + attrNotSoSecret: '*not-so-secret*', + attrThree: 'three', + }, + references: [], + }; + + mockBaseClient.create.mockResolvedValue(mockedResponse); + + expect( + await wrapper.create('known-type-with-allowed-explicit-ids', attributes, options) + ).toEqual({ + ...mockedResponse, + attributes: { attrOne: 'one', attrNotSoSecret: 'not-so-secret', attrThree: 'three' }, + }); + + expect(mockBaseClient.create).toHaveBeenCalledTimes(1); + expect(mockBaseClient.create).toHaveBeenCalledWith( + 'known-type-with-allowed-explicit-ids', + { + attrOne: 'one', + attrSecret: '*secret*', + attrNotSoSecret: '*not-so-secret*', + attrThree: 'three', + }, + { id: 'custom-uuid', overwrite: true } + ); + }); + describe('namespace', () => { const doTest = async (namespace: string, expectNamespaceInDescriptor: boolean) => { const attributes = { attrOne: 'one', attrSecret: 'secret', attrThree: 'three' }; @@ -234,6 +284,66 @@ describe('#bulkCreate', () => { ); }); + it('allows explicit IDs when specified type allows it', async () => { + const attributes = { attrOne: 'one', attrSecret: 'secret', attrThree: 'three' }; + const options = { namespace: 'some-namespace' }; + const mockedResponse = { + saved_objects: [ + { + id: 'uuid-v4-id', + type: 'known-type-with-allowed-explicit-ids', + attributes, + references: [], + }, + { + id: 'custom-uuid', + type: 'known-type-with-allowed-explicit-ids', + attributes, + references: [], + }, + { + id: 'some-id', + type: 'unknown-type', + attributes, + references: [], + }, + ], + }; + + mockBaseClient.bulkCreate.mockResolvedValue(mockedResponse); + + const bulkCreateParams = [ + { type: 'known-type-with-allowed-explicit-ids', attributes }, + { id: 'custom-uuid', type: 'known-type-with-allowed-explicit-ids', attributes }, + { id: 'some-id', type: 'unknown-type', attributes }, + ]; + + await expect(wrapper.bulkCreate(bulkCreateParams, options)).resolves.toEqual({ + saved_objects: [ + { ...mockedResponse.saved_objects[0], attributes: { attrOne: 'one', attrThree: 'three' } }, + { ...mockedResponse.saved_objects[1], attributes: { attrOne: 'one', attrThree: 'three' } }, + mockedResponse.saved_objects[2], + ], + }); + + expect(mockBaseClient.bulkCreate).toHaveBeenCalledTimes(1); + expect(mockBaseClient.bulkCreate).toHaveBeenCalledWith( + [ + { + ...bulkCreateParams[0], + id: 'uuid-v4-id', + attributes: { attrOne: 'one', attrSecret: '*secret*', attrThree: 'three' }, + }, + { + ...bulkCreateParams[1], + attributes: { attrOne: 'one', attrSecret: '*secret*', attrThree: 'three' }, + }, + bulkCreateParams[2], + ], + options + ); + }); + it('fails if ID is specified for registered type', async () => { const attributes = { attrOne: 'one', attrSecret: 'secret', attrThree: 'three' }; diff --git a/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.ts b/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.ts index 3246457179f68e..46e4fd9c2dc055 100644 --- a/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.ts +++ b/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.ts @@ -60,13 +60,13 @@ export class EncryptedSavedObjectsClientWrapper implements SavedObjectsClientCon // Saved objects with encrypted attributes should have IDs that are hard to guess especially // since IDs are part of the AAD used during encryption, that's why we control them within this // wrapper and don't allow consumers to specify their own IDs directly. - if (options.id) { + if (options.id && !this.options.service.allowsExplicitIDs(type)) { throw new Error( 'Predefined IDs are not allowed for saved objects with encrypted attributes.' ); } - const id = generateID(); + const id = options.id ?? generateID(); const namespace = getDescriptorNamespace( this.options.baseTypeRegistry, type, @@ -103,13 +103,13 @@ export class EncryptedSavedObjectsClientWrapper implements SavedObjectsClientCon // Saved objects with encrypted attributes should have IDs that are hard to guess especially // since IDs are part of the AAD used during encryption, that's why we control them within this // wrapper and don't allow consumers to specify their own IDs directly. - if (object.id) { + if (object.id && !this.options.service.allowsExplicitIDs(object.type)) { throw new Error( 'Predefined IDs are not allowed for saved objects with encrypted attributes.' ); } - const id = generateID(); + const id = object.id ?? generateID(); const namespace = getDescriptorNamespace( this.options.baseTypeRegistry, object.type, diff --git a/x-pack/test/alerting_api_integration/security_and_spaces/tests/actions/builtin_action_types/webhook.ts b/x-pack/test/alerting_api_integration/security_and_spaces/tests/actions/builtin_action_types/webhook.ts index 7eba753d7e98ba..ed5f4ca2014fc9 100644 --- a/x-pack/test/alerting_api_integration/security_and_spaces/tests/actions/builtin_action_types/webhook.ts +++ b/x-pack/test/alerting_api_integration/security_and_spaces/tests/actions/builtin_action_types/webhook.ts @@ -116,6 +116,77 @@ export default function webhookTest({ getService }: FtrProviderContext) { }); }); + it('should remove headers when a webhook is updated', async () => { + const { body: createdAction } = await supertest + .post('/api/actions/action') + .set('kbn-xsrf', 'test') + .send({ + name: 'A generic Webhook action', + actionTypeId: '.webhook', + secrets: { + user: 'username', + password: 'mypassphrase', + }, + config: { + url: webhookSimulatorURL, + headers: { + someHeader: '123', + }, + }, + }) + .expect(200); + + expect(createdAction).to.eql({ + id: createdAction.id, + isPreconfigured: false, + name: 'A generic Webhook action', + actionTypeId: '.webhook', + config: { + ...defaultValues, + url: webhookSimulatorURL, + headers: { + someHeader: '123', + }, + }, + }); + + await supertest + .put(`/api/actions/action/${createdAction.id}`) + .set('kbn-xsrf', 'foo') + .send({ + name: 'A generic Webhook action', + secrets: { + user: 'username', + password: 'mypassphrase', + }, + config: { + url: webhookSimulatorURL, + headers: { + someOtherHeader: '456', + }, + }, + }) + .expect(200); + + const { body: fetchedAction } = await supertest + .get(`/api/actions/action/${createdAction.id}`) + .expect(200); + + expect(fetchedAction).to.eql({ + id: fetchedAction.id, + isPreconfigured: false, + name: 'A generic Webhook action', + actionTypeId: '.webhook', + config: { + ...defaultValues, + url: webhookSimulatorURL, + headers: { + someOtherHeader: '456', + }, + }, + }); + }); + it('should send authentication to the webhook target', async () => { const webhookActionId = await createWebhookAction(webhookSimulatorURL); const { body: result } = await supertest From b0e5938805e77cf2e46771ee13c9c062eb5c1713 Mon Sep 17 00:00:00 2001 From: Gidi Meir Morris Date: Thu, 30 Jul 2020 13:08:07 +0100 Subject: [PATCH 02/41] Revert "use create to overwrite actions on update in action" This reverts commit d0c0249017b2135682b14845ee026c4854fcd204. --- .../actions/server/actions_client.test.ts | 26 ++--- .../plugins/actions/server/actions_client.ts | 26 ++--- .../actions/server/saved_objects/index.ts | 1 - .../encrypted_saved_object_type_definition.ts | 2 - .../encrypted_saved_objects_service.mocks.ts | 4 - .../encrypted_saved_objects_service.test.ts | 15 --- .../crypto/encrypted_saved_objects_service.ts | 9 -- ...ypted_saved_objects_client_wrapper.test.ts | 110 ------------------ .../encrypted_saved_objects_client_wrapper.ts | 8 +- .../actions/builtin_action_types/webhook.ts | 71 ----------- 10 files changed, 23 insertions(+), 249 deletions(-) diff --git a/x-pack/plugins/actions/server/actions_client.test.ts b/x-pack/plugins/actions/server/actions_client.test.ts index 493031f379cc4b..90b989ac3b52eb 100644 --- a/x-pack/plugins/actions/server/actions_client.test.ts +++ b/x-pack/plugins/actions/server/actions_client.test.ts @@ -893,7 +893,7 @@ describe('update()', () => { }, references: [], }); - unsecuredSavedObjectsClient.create.mockResolvedValueOnce({ + unsecuredSavedObjectsClient.update.mockResolvedValueOnce({ id: 'my-action', type: 'action', attributes: { @@ -946,7 +946,7 @@ describe('update()', () => { }, references: [], }); - unsecuredSavedObjectsClient.create.mockResolvedValueOnce({ + unsecuredSavedObjectsClient.update.mockResolvedValueOnce({ id: 'my-action', type: 'action', attributes: { @@ -972,20 +972,17 @@ describe('update()', () => { name: 'my name', config: {}, }); - expect(unsecuredSavedObjectsClient.create).toHaveBeenCalledTimes(1); - expect(unsecuredSavedObjectsClient.create.mock.calls[0]).toMatchInlineSnapshot(` + expect(unsecuredSavedObjectsClient.update).toHaveBeenCalledTimes(1); + expect(unsecuredSavedObjectsClient.update.mock.calls[0]).toMatchInlineSnapshot(` Array [ "action", + "my-action", Object { "actionTypeId": "my-action-type", "config": Object {}, "name": "my name", "secrets": Object {}, }, - Object { - "id": "my-action", - "overwrite": true, - }, ] `); expect(unsecuredSavedObjectsClient.get).toHaveBeenCalledTimes(1); @@ -1046,7 +1043,7 @@ describe('update()', () => { }, references: [], }); - unsecuredSavedObjectsClient.create.mockResolvedValueOnce({ + unsecuredSavedObjectsClient.update.mockResolvedValueOnce({ id: 'my-action', type: 'action', attributes: { @@ -1084,10 +1081,11 @@ describe('update()', () => { c: true, }, }); - expect(unsecuredSavedObjectsClient.create).toHaveBeenCalledTimes(1); - expect(unsecuredSavedObjectsClient.create.mock.calls[0]).toMatchInlineSnapshot(` + expect(unsecuredSavedObjectsClient.update).toHaveBeenCalledTimes(1); + expect(unsecuredSavedObjectsClient.update.mock.calls[0]).toMatchInlineSnapshot(` Array [ "action", + "my-action", Object { "actionTypeId": "my-action-type", "config": Object { @@ -1098,10 +1096,6 @@ describe('update()', () => { "name": "my name", "secrets": Object {}, }, - Object { - "id": "my-action", - "overwrite": true, - }, ] `); }); @@ -1124,7 +1118,7 @@ describe('update()', () => { }, references: [], }); - unsecuredSavedObjectsClient.create.mockResolvedValueOnce({ + unsecuredSavedObjectsClient.update.mockResolvedValueOnce({ id: 'my-action', type: 'action', attributes: { diff --git a/x-pack/plugins/actions/server/actions_client.ts b/x-pack/plugins/actions/server/actions_client.ts index a09ffe373fb519..6744a8d1116234 100644 --- a/x-pack/plugins/actions/server/actions_client.ts +++ b/x-pack/plugins/actions/server/actions_client.ts @@ -159,26 +159,18 @@ export class ActionsClient { this.actionTypeRegistry.ensureActionTypeEnabled(actionTypeId); - // SOs client does partial updates - we want to overwrite the whole object, so we'll use create with overwrite - const result = await this.unsecuredSavedObjectsClient.create( - 'action', - { - actionTypeId, - name, - config: validatedActionTypeConfig as SavedObjectAttributes, - secrets: validatedActionTypeSecrets as SavedObjectAttributes, - }, - { - id, - overwrite: true, - } - ); + const result = await this.unsecuredSavedObjectsClient.update('action', id, { + actionTypeId, + name, + config: validatedActionTypeConfig as SavedObjectAttributes, + secrets: validatedActionTypeSecrets as SavedObjectAttributes, + }); return { id, - actionTypeId: result.attributes.actionTypeId, - name: result.attributes.name, - config: result.attributes.config, + actionTypeId: result.attributes.actionTypeId as string, + name: result.attributes.name as string, + config: result.attributes.config as Record, isPreconfigured: false, }; } diff --git a/x-pack/plugins/actions/server/saved_objects/index.ts b/x-pack/plugins/actions/server/saved_objects/index.ts index 353c01c1e57945..54f186acc1ba57 100644 --- a/x-pack/plugins/actions/server/saved_objects/index.ts +++ b/x-pack/plugins/actions/server/saved_objects/index.ts @@ -30,7 +30,6 @@ export function setupSavedObjects( type: ACTION_SAVED_OBJECT_TYPE, attributesToEncrypt: new Set(['secrets']), attributesToExcludeFromAAD: new Set(['name']), - allowsExplicitIDs: true, }); savedObjects.registerType({ diff --git a/x-pack/plugins/encrypted_saved_objects/server/crypto/encrypted_saved_object_type_definition.ts b/x-pack/plugins/encrypted_saved_objects/server/crypto/encrypted_saved_object_type_definition.ts index cd57faed89d0ea..849a2888b6e1a5 100644 --- a/x-pack/plugins/encrypted_saved_objects/server/crypto/encrypted_saved_object_type_definition.ts +++ b/x-pack/plugins/encrypted_saved_objects/server/crypto/encrypted_saved_object_type_definition.ts @@ -13,7 +13,6 @@ import { EncryptedSavedObjectTypeRegistration } from './encrypted_saved_objects_ */ export class EncryptedSavedObjectAttributesDefinition { public readonly attributesToEncrypt: ReadonlySet; - public readonly allowsExplicitIDs: Readonly; private readonly attributesToExcludeFromAAD: ReadonlySet | undefined; private readonly attributesToStrip: ReadonlySet; @@ -35,7 +34,6 @@ export class EncryptedSavedObjectAttributesDefinition { this.attributesToEncrypt = attributesToEncrypt; this.attributesToStrip = attributesToStrip; this.attributesToExcludeFromAAD = typeRegistration.attributesToExcludeFromAAD; - this.allowsExplicitIDs = typeRegistration.allowsExplicitIDs ?? false; } /** diff --git a/x-pack/plugins/encrypted_saved_objects/server/crypto/encrypted_saved_objects_service.mocks.ts b/x-pack/plugins/encrypted_saved_objects/server/crypto/encrypted_saved_objects_service.mocks.ts index fdc8e0e4ba4bb5..c692d8698771fe 100644 --- a/x-pack/plugins/encrypted_saved_objects/server/crypto/encrypted_saved_objects_service.mocks.ts +++ b/x-pack/plugins/encrypted_saved_objects/server/crypto/encrypted_saved_objects_service.mocks.ts @@ -13,7 +13,6 @@ import { function createEncryptedSavedObjectsServiceMock() { return ({ isRegistered: jest.fn(), - allowsExplicitIDs: jest.fn(), stripOrDecryptAttributes: jest.fn(), encryptAttributes: jest.fn(), decryptAttributes: jest.fn(), @@ -50,9 +49,6 @@ export const encryptedSavedObjectsServiceMock = { return clonedAttrs; } - mock.allowsExplicitIDs.mockImplementation( - (type) => registrations.find((r) => r.type === type)?.allowsExplicitIDs ?? false - ); mock.isRegistered.mockImplementation( (type) => registrations.findIndex((r) => r.type === type) >= 0 ); diff --git a/x-pack/plugins/encrypted_saved_objects/server/crypto/encrypted_saved_objects_service.test.ts b/x-pack/plugins/encrypted_saved_objects/server/crypto/encrypted_saved_objects_service.test.ts index 8230e09414e01d..42d2e2ffd15163 100644 --- a/x-pack/plugins/encrypted_saved_objects/server/crypto/encrypted_saved_objects_service.test.ts +++ b/x-pack/plugins/encrypted_saved_objects/server/crypto/encrypted_saved_objects_service.test.ts @@ -85,21 +85,6 @@ describe('#isRegistered', () => { }); }); -describe('#allowsExplicitIDs', () => { - it('correctly returns allowsExplicitIDs when the specified type is registered', () => { - service.registerType({ - type: 'allowsExplicitIDs-type-1', - attributesToEncrypt: new Set(['attr-1']), - allowsExplicitIDs: true, - }); - expect(service.allowsExplicitIDs('allowsExplicitIDs-type-1')).toBe(true); - }); - - it('correctly defaults to false when the specified type is not registered', () => { - expect(service.allowsExplicitIDs('unknown-type-1')).toBe(false); - }); -}); - describe('#stripOrDecryptAttributes', () => { it('does not strip attributes from unknown types', async () => { const attributes = { attrOne: 'one', attrTwo: 'two', attrThree: 'three' }; diff --git a/x-pack/plugins/encrypted_saved_objects/server/crypto/encrypted_saved_objects_service.ts b/x-pack/plugins/encrypted_saved_objects/server/crypto/encrypted_saved_objects_service.ts index 0c9339085b898f..99361107047c27 100644 --- a/x-pack/plugins/encrypted_saved_objects/server/crypto/encrypted_saved_objects_service.ts +++ b/x-pack/plugins/encrypted_saved_objects/server/crypto/encrypted_saved_objects_service.ts @@ -31,7 +31,6 @@ export interface EncryptedSavedObjectTypeRegistration { readonly type: string; readonly attributesToEncrypt: ReadonlySet; readonly attributesToExcludeFromAAD?: ReadonlySet; - readonly allowsExplicitIDs?: boolean; } /** @@ -122,14 +121,6 @@ export class EncryptedSavedObjectsService { return this.typeDefinitions.has(type); } - /** - * Returns whether the type allows explciit IDs when creating a saved object - * resolves to `false` if the type is not registered - */ - public allowsExplicitIDs(type: string) { - return this.typeDefinitions.get(type)?.allowsExplicitIDs ?? false; - } - /** * Takes saved object attributes for the specified type and, depending on the type definition, * either decrypts or strips encrypted attributes (e.g. in case AAD or encryption key has changed diff --git a/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.test.ts b/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.test.ts index 70a5f06bf56a39..5d4ea5a6370e4a 100644 --- a/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.test.ts +++ b/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.test.ts @@ -30,14 +30,6 @@ beforeEach(() => { { key: 'attrNotSoSecret', dangerouslyExposeValue: true }, ]), }, - { - type: 'known-type-with-allowed-explicit-ids', - allowsExplicitIDs: true, - attributesToEncrypt: new Set([ - 'attrSecret', - { key: 'attrNotSoSecret', dangerouslyExposeValue: true }, - ]), - }, ]); wrapper = new EncryptedSavedObjectsClientWrapper({ @@ -129,48 +121,6 @@ describe('#create', () => { ); }); - it('allows explicit IDs when specified type allows it', async () => { - const attributes = { - attrOne: 'one', - attrSecret: 'secret', - attrNotSoSecret: 'not-so-secret', - attrThree: 'three', - }; - const options = { overwrite: true, id: 'custom-uuid' }; - const mockedResponse = { - id: 'custom-uuid', - type: 'known-type-with-allowed-explicit-ids', - attributes: { - attrOne: 'one', - attrSecret: '*secret*', - attrNotSoSecret: '*not-so-secret*', - attrThree: 'three', - }, - references: [], - }; - - mockBaseClient.create.mockResolvedValue(mockedResponse); - - expect( - await wrapper.create('known-type-with-allowed-explicit-ids', attributes, options) - ).toEqual({ - ...mockedResponse, - attributes: { attrOne: 'one', attrNotSoSecret: 'not-so-secret', attrThree: 'three' }, - }); - - expect(mockBaseClient.create).toHaveBeenCalledTimes(1); - expect(mockBaseClient.create).toHaveBeenCalledWith( - 'known-type-with-allowed-explicit-ids', - { - attrOne: 'one', - attrSecret: '*secret*', - attrNotSoSecret: '*not-so-secret*', - attrThree: 'three', - }, - { id: 'custom-uuid', overwrite: true } - ); - }); - describe('namespace', () => { const doTest = async (namespace: string, expectNamespaceInDescriptor: boolean) => { const attributes = { attrOne: 'one', attrSecret: 'secret', attrThree: 'three' }; @@ -284,66 +234,6 @@ describe('#bulkCreate', () => { ); }); - it('allows explicit IDs when specified type allows it', async () => { - const attributes = { attrOne: 'one', attrSecret: 'secret', attrThree: 'three' }; - const options = { namespace: 'some-namespace' }; - const mockedResponse = { - saved_objects: [ - { - id: 'uuid-v4-id', - type: 'known-type-with-allowed-explicit-ids', - attributes, - references: [], - }, - { - id: 'custom-uuid', - type: 'known-type-with-allowed-explicit-ids', - attributes, - references: [], - }, - { - id: 'some-id', - type: 'unknown-type', - attributes, - references: [], - }, - ], - }; - - mockBaseClient.bulkCreate.mockResolvedValue(mockedResponse); - - const bulkCreateParams = [ - { type: 'known-type-with-allowed-explicit-ids', attributes }, - { id: 'custom-uuid', type: 'known-type-with-allowed-explicit-ids', attributes }, - { id: 'some-id', type: 'unknown-type', attributes }, - ]; - - await expect(wrapper.bulkCreate(bulkCreateParams, options)).resolves.toEqual({ - saved_objects: [ - { ...mockedResponse.saved_objects[0], attributes: { attrOne: 'one', attrThree: 'three' } }, - { ...mockedResponse.saved_objects[1], attributes: { attrOne: 'one', attrThree: 'three' } }, - mockedResponse.saved_objects[2], - ], - }); - - expect(mockBaseClient.bulkCreate).toHaveBeenCalledTimes(1); - expect(mockBaseClient.bulkCreate).toHaveBeenCalledWith( - [ - { - ...bulkCreateParams[0], - id: 'uuid-v4-id', - attributes: { attrOne: 'one', attrSecret: '*secret*', attrThree: 'three' }, - }, - { - ...bulkCreateParams[1], - attributes: { attrOne: 'one', attrSecret: '*secret*', attrThree: 'three' }, - }, - bulkCreateParams[2], - ], - options - ); - }); - it('fails if ID is specified for registered type', async () => { const attributes = { attrOne: 'one', attrSecret: 'secret', attrThree: 'three' }; diff --git a/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.ts b/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.ts index 46e4fd9c2dc055..3246457179f68e 100644 --- a/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.ts +++ b/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.ts @@ -60,13 +60,13 @@ export class EncryptedSavedObjectsClientWrapper implements SavedObjectsClientCon // Saved objects with encrypted attributes should have IDs that are hard to guess especially // since IDs are part of the AAD used during encryption, that's why we control them within this // wrapper and don't allow consumers to specify their own IDs directly. - if (options.id && !this.options.service.allowsExplicitIDs(type)) { + if (options.id) { throw new Error( 'Predefined IDs are not allowed for saved objects with encrypted attributes.' ); } - const id = options.id ?? generateID(); + const id = generateID(); const namespace = getDescriptorNamespace( this.options.baseTypeRegistry, type, @@ -103,13 +103,13 @@ export class EncryptedSavedObjectsClientWrapper implements SavedObjectsClientCon // Saved objects with encrypted attributes should have IDs that are hard to guess especially // since IDs are part of the AAD used during encryption, that's why we control them within this // wrapper and don't allow consumers to specify their own IDs directly. - if (object.id && !this.options.service.allowsExplicitIDs(object.type)) { + if (object.id) { throw new Error( 'Predefined IDs are not allowed for saved objects with encrypted attributes.' ); } - const id = object.id ?? generateID(); + const id = generateID(); const namespace = getDescriptorNamespace( this.options.baseTypeRegistry, object.type, diff --git a/x-pack/test/alerting_api_integration/security_and_spaces/tests/actions/builtin_action_types/webhook.ts b/x-pack/test/alerting_api_integration/security_and_spaces/tests/actions/builtin_action_types/webhook.ts index ed5f4ca2014fc9..7eba753d7e98ba 100644 --- a/x-pack/test/alerting_api_integration/security_and_spaces/tests/actions/builtin_action_types/webhook.ts +++ b/x-pack/test/alerting_api_integration/security_and_spaces/tests/actions/builtin_action_types/webhook.ts @@ -116,77 +116,6 @@ export default function webhookTest({ getService }: FtrProviderContext) { }); }); - it('should remove headers when a webhook is updated', async () => { - const { body: createdAction } = await supertest - .post('/api/actions/action') - .set('kbn-xsrf', 'test') - .send({ - name: 'A generic Webhook action', - actionTypeId: '.webhook', - secrets: { - user: 'username', - password: 'mypassphrase', - }, - config: { - url: webhookSimulatorURL, - headers: { - someHeader: '123', - }, - }, - }) - .expect(200); - - expect(createdAction).to.eql({ - id: createdAction.id, - isPreconfigured: false, - name: 'A generic Webhook action', - actionTypeId: '.webhook', - config: { - ...defaultValues, - url: webhookSimulatorURL, - headers: { - someHeader: '123', - }, - }, - }); - - await supertest - .put(`/api/actions/action/${createdAction.id}`) - .set('kbn-xsrf', 'foo') - .send({ - name: 'A generic Webhook action', - secrets: { - user: 'username', - password: 'mypassphrase', - }, - config: { - url: webhookSimulatorURL, - headers: { - someOtherHeader: '456', - }, - }, - }) - .expect(200); - - const { body: fetchedAction } = await supertest - .get(`/api/actions/action/${createdAction.id}`) - .expect(200); - - expect(fetchedAction).to.eql({ - id: fetchedAction.id, - isPreconfigured: false, - name: 'A generic Webhook action', - actionTypeId: '.webhook', - config: { - ...defaultValues, - url: webhookSimulatorURL, - headers: { - someOtherHeader: '456', - }, - }, - }); - }); - it('should send authentication to the webhook target', async () => { const webhookActionId = await createWebhookAction(webhookSimulatorURL); const { body: result } = await supertest From 72546d01555c4203b38816a016109c978d4ba31a Mon Sep 17 00:00:00 2001 From: Gidi Meir Morris Date: Thu, 30 Jul 2020 14:41:53 +0100 Subject: [PATCH 03/41] changed ESO to overwrite on update --- ...ypted_saved_objects_client_wrapper.test.ts | 29 ++++---- .../encrypted_saved_objects_client_wrapper.ts | 21 ++++-- .../actions/builtin_action_types/webhook.ts | 71 +++++++++++++++++++ 3 files changed, 99 insertions(+), 22 deletions(-) diff --git a/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.test.ts b/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.test.ts index 5d4ea5a6370e4a..a874f3713c2c6c 100644 --- a/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.test.ts +++ b/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.test.ts @@ -1294,7 +1294,7 @@ describe('#update', () => { references: [], }; - mockBaseClient.update.mockResolvedValue(mockedResponse); + mockBaseClient.create.mockResolvedValue(mockedResponse); await expect(wrapper.update('known-type', 'some-id', attributes, options)).resolves.toEqual({ ...mockedResponse, @@ -1313,17 +1313,17 @@ describe('#update', () => { { user: mockAuthenticatedUser() } ); - expect(mockBaseClient.update).toHaveBeenCalledTimes(1); - expect(mockBaseClient.update).toHaveBeenCalledWith( + expect(mockBaseClient.create).toHaveBeenCalledTimes(1); + expect(mockBaseClient.create).toHaveBeenCalledWith( 'known-type', - 'some-id', + { attrOne: 'one', attrSecret: '*secret*', attrNotSoSecret: '*not-so-secret*', attrThree: 'three', }, - options + { id: 'some-id', overwrite: true, ...options } ); }); @@ -1333,7 +1333,7 @@ describe('#update', () => { const options = { version: 'some-version', namespace }; const mockedResponse = { id: 'some-id', type: 'known-type', attributes, references: [] }; - mockBaseClient.update.mockResolvedValue(mockedResponse); + mockBaseClient.create.mockResolvedValue(mockedResponse); await expect(wrapper.update('known-type', 'some-id', attributes, options)).resolves.toEqual({ ...mockedResponse, @@ -1351,12 +1351,12 @@ describe('#update', () => { { user: mockAuthenticatedUser() } ); - expect(mockBaseClient.update).toHaveBeenCalledTimes(1); - expect(mockBaseClient.update).toHaveBeenCalledWith( + expect(mockBaseClient.create).toHaveBeenCalledTimes(1); + expect(mockBaseClient.create).toHaveBeenCalledWith( 'known-type', - 'some-id', + { attrOne: 'one', attrSecret: '*secret*', attrThree: 'three' }, - options + { id: 'some-id', overwrite: true, ...options } ); }; @@ -1372,7 +1372,7 @@ describe('#update', () => { it('fails if base client fails', async () => { const failureReason = new Error('Something bad happened...'); - mockBaseClient.update.mockRejectedValue(failureReason); + mockBaseClient.create.mockRejectedValue(failureReason); await expect( wrapper.update('known-type', 'some-id', { @@ -1382,12 +1382,11 @@ describe('#update', () => { }) ).rejects.toThrowError(failureReason); - expect(mockBaseClient.update).toHaveBeenCalledTimes(1); - expect(mockBaseClient.update).toHaveBeenCalledWith( + expect(mockBaseClient.create).toHaveBeenCalledTimes(1); + expect(mockBaseClient.create).toHaveBeenCalledWith( 'known-type', - 'some-id', { attrOne: 'one', attrSecret: '*secret*', attrThree: 'three' }, - undefined + { id: 'some-id', overwrite: true } ); }); }); diff --git a/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.ts b/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.ts index 3246457179f68e..6696ef00b8c55b 100644 --- a/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.ts +++ b/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.ts @@ -200,7 +200,7 @@ export class EncryptedSavedObjectsClientWrapper implements SavedObjectsClientCon public async update( type: string, id: string, - attributes: Partial, + attributes: T, options?: SavedObjectsUpdateOptions ) { if (!this.options.service.isRegistered(type)) { @@ -212,13 +212,20 @@ export class EncryptedSavedObjectsClientWrapper implements SavedObjectsClientCon options?.namespace ); return this.handleEncryptedAttributesInResponse( - await this.options.baseClient.update( + await this.options.baseClient.create( type, - id, - await this.options.service.encryptAttributes({ type, id, namespace }, attributes, { - user: this.options.getCurrentUser(), - }), - options + (await this.options.service.encryptAttributes( + { type, id, namespace }, + attributes as Record, + { + user: this.options.getCurrentUser(), + } + )) as T, + { + id, + overwrite: true, + ...options, + } ), attributes, namespace diff --git a/x-pack/test/alerting_api_integration/security_and_spaces/tests/actions/builtin_action_types/webhook.ts b/x-pack/test/alerting_api_integration/security_and_spaces/tests/actions/builtin_action_types/webhook.ts index 7eba753d7e98ba..ed5f4ca2014fc9 100644 --- a/x-pack/test/alerting_api_integration/security_and_spaces/tests/actions/builtin_action_types/webhook.ts +++ b/x-pack/test/alerting_api_integration/security_and_spaces/tests/actions/builtin_action_types/webhook.ts @@ -116,6 +116,77 @@ export default function webhookTest({ getService }: FtrProviderContext) { }); }); + it('should remove headers when a webhook is updated', async () => { + const { body: createdAction } = await supertest + .post('/api/actions/action') + .set('kbn-xsrf', 'test') + .send({ + name: 'A generic Webhook action', + actionTypeId: '.webhook', + secrets: { + user: 'username', + password: 'mypassphrase', + }, + config: { + url: webhookSimulatorURL, + headers: { + someHeader: '123', + }, + }, + }) + .expect(200); + + expect(createdAction).to.eql({ + id: createdAction.id, + isPreconfigured: false, + name: 'A generic Webhook action', + actionTypeId: '.webhook', + config: { + ...defaultValues, + url: webhookSimulatorURL, + headers: { + someHeader: '123', + }, + }, + }); + + await supertest + .put(`/api/actions/action/${createdAction.id}`) + .set('kbn-xsrf', 'foo') + .send({ + name: 'A generic Webhook action', + secrets: { + user: 'username', + password: 'mypassphrase', + }, + config: { + url: webhookSimulatorURL, + headers: { + someOtherHeader: '456', + }, + }, + }) + .expect(200); + + const { body: fetchedAction } = await supertest + .get(`/api/actions/action/${createdAction.id}`) + .expect(200); + + expect(fetchedAction).to.eql({ + id: fetchedAction.id, + isPreconfigured: false, + name: 'A generic Webhook action', + actionTypeId: '.webhook', + config: { + ...defaultValues, + url: webhookSimulatorURL, + headers: { + someOtherHeader: '456', + }, + }, + }); + }); + it('should send authentication to the webhook target', async () => { const webhookActionId = await createWebhookAction(webhookSimulatorURL); const { body: result } = await supertest From e4280f2bb6fed76bc807f9852aa94dce78900e20 Mon Sep 17 00:00:00 2001 From: Gidi Meir Morris Date: Thu, 30 Jul 2020 17:42:10 +0100 Subject: [PATCH 04/41] added e2e test for new ESO update --- .../api_consumer_plugin/server/index.ts | 29 ++++++++ .../tests/encrypted_saved_objects_api.ts | 68 +++++++++++++++++-- 2 files changed, 90 insertions(+), 7 deletions(-) diff --git a/x-pack/test/encrypted_saved_objects_api_integration/fixtures/api_consumer_plugin/server/index.ts b/x-pack/test/encrypted_saved_objects_api_integration/fixtures/api_consumer_plugin/server/index.ts index 87bed7f4160191..0c40b795b5cc91 100644 --- a/x-pack/test/encrypted_saved_objects_api_integration/fixtures/api_consumer_plugin/server/index.ts +++ b/x-pack/test/encrypted_saved_objects_api_integration/fixtures/api_consumer_plugin/server/index.ts @@ -62,6 +62,10 @@ export const plugin: PluginInitializer = publicPropertyExcludedFromAAD: { type: 'keyword' }, publicPropertyStoredEncrypted: { type: 'binary' }, privateProperty: { type: 'binary' }, + publicDeepProperty: { + enabled: false, + type: 'object', + }, }, }), }); @@ -112,6 +116,31 @@ export const plugin: PluginInitializer = } ); + for (const route of ['saved_objects', 'hidden_saved_objects']) { + router.put( + { + path: `/api/${route}/update-with-partial/{type}/{id}`, + validate: { params: (value) => ({ value }), body: (value) => ({ value }) }, + }, + async (context, request, response) => { + const [{ savedObjects }] = await core.getStartServices(); + const { type, id } = request.params; + const { attributes } = request.body as { attributes: any }; + + return response.ok({ + body: await savedObjects + .getScopedClient(request, { + includedHiddenTypes: [HIDDEN_SAVED_OBJECT_WITH_SECRET_TYPE], + excludedWrappers: ['encryptedSavedObjects'], + }) + .update(type, id, attributes, { + refresh: true, + }), + }); + } + ); + } + registerHiddenSORoutes(router, core, deps, [HIDDEN_SAVED_OBJECT_WITH_SECRET_TYPE]); }, start() {}, diff --git a/x-pack/test/encrypted_saved_objects_api_integration/tests/encrypted_saved_objects_api.ts b/x-pack/test/encrypted_saved_objects_api_integration/tests/encrypted_saved_objects_api.ts index 8bdc1715bf487b..7c13cd646cb352 100644 --- a/x-pack/test/encrypted_saved_objects_api_integration/tests/encrypted_saved_objects_api.ts +++ b/x-pack/test/encrypted_saved_objects_api_integration/tests/encrypted_saved_objects_api.ts @@ -41,6 +41,7 @@ export default function ({ getService }: FtrProviderContext) { publicPropertyStoredEncrypted: string; privateProperty: string; publicPropertyExcludedFromAAD: string; + publicDeepProperty: Record; }; let savedObject: SavedObject; @@ -50,6 +51,7 @@ export default function ({ getService }: FtrProviderContext) { publicPropertyStoredEncrypted: randomness.string(), privateProperty: randomness.string(), publicPropertyExcludedFromAAD: randomness.string(), + publicDeepProperty: { [randomness.string()]: randomness.string() }, }; const { body } = await supertest @@ -66,6 +68,7 @@ export default function ({ getService }: FtrProviderContext) { publicProperty: savedObjectOriginalAttributes.publicProperty, publicPropertyExcludedFromAAD: savedObjectOriginalAttributes.publicPropertyExcludedFromAAD, publicPropertyStoredEncrypted: savedObjectOriginalAttributes.publicPropertyStoredEncrypted, + publicDeepProperty: savedObjectOriginalAttributes.publicDeepProperty, }); const rawAttributes = await getRawSavedObjectAttributes(savedObject); @@ -73,6 +76,9 @@ export default function ({ getService }: FtrProviderContext) { expect(rawAttributes.publicPropertyExcludedFromAAD).to.be( savedObjectOriginalAttributes.publicPropertyExcludedFromAAD ); + expect(rawAttributes.publicDeepProperty).to.eql( + savedObjectOriginalAttributes.publicDeepProperty + ); expect(rawAttributes.publicPropertyStoredEncrypted).to.not.be.empty(); expect(rawAttributes.publicPropertyStoredEncrypted).to.not.be( @@ -208,6 +214,7 @@ export default function ({ getService }: FtrProviderContext) { publicProperty: savedObjectOriginalAttributes.publicProperty, publicPropertyExcludedFromAAD: savedObjectOriginalAttributes.publicPropertyExcludedFromAAD, publicPropertyStoredEncrypted: savedObjectOriginalAttributes.publicPropertyStoredEncrypted, + publicDeepProperty: savedObjectOriginalAttributes.publicDeepProperty, }); expect(response.error).to.be(undefined); }); @@ -217,9 +224,13 @@ export default function ({ getService }: FtrProviderContext) { // encrypted attributes. const updatedPublicProperty = randomness.string(); await supertest - .put(`${getURLAPIBaseURL()}${encryptedSavedObjectType}/${savedObject.id}`) + .put( + `${getURLAPIBaseURL()}update-with-partial/${encryptedSavedObjectType}/${savedObject.id}` + ) .set('kbn-xsrf', 'xxx') - .send({ attributes: { publicProperty: updatedPublicProperty } }) + .send({ + attributes: { publicProperty: updatedPublicProperty }, + }) .expect(200); const { body: response } = await supertest @@ -229,6 +240,7 @@ export default function ({ getService }: FtrProviderContext) { expect(response.attributes).to.eql({ publicProperty: updatedPublicProperty, publicPropertyExcludedFromAAD: savedObjectOriginalAttributes.publicPropertyExcludedFromAAD, + publicDeepProperty: savedObjectOriginalAttributes.publicDeepProperty, }); expect(response.error).to.eql({ message: 'Unable to decrypt attribute "publicPropertyStoredEncrypted"', @@ -248,6 +260,7 @@ export default function ({ getService }: FtrProviderContext) { publicProperty: savedObjectOriginalAttributes.publicProperty, publicPropertyExcludedFromAAD: savedObjectOriginalAttributes.publicPropertyExcludedFromAAD, publicPropertyStoredEncrypted: savedObjectOriginalAttributes.publicPropertyStoredEncrypted, + publicDeepProperty: savedObjectOriginalAttributes.publicDeepProperty, }); expect(savedObjects[0].error).to.be(undefined); }); @@ -257,7 +270,9 @@ export default function ({ getService }: FtrProviderContext) { // encrypted attributes. const updatedPublicProperty = randomness.string(); await supertest - .put(`${getURLAPIBaseURL()}${encryptedSavedObjectType}/${savedObject.id}`) + .put( + `${getURLAPIBaseURL()}update-with-partial/${encryptedSavedObjectType}/${savedObject.id}` + ) .set('kbn-xsrf', 'xxx') .send({ attributes: { publicProperty: updatedPublicProperty } }) .expect(200); @@ -273,6 +288,7 @@ export default function ({ getService }: FtrProviderContext) { expect(savedObjects[0].attributes).to.eql({ publicProperty: updatedPublicProperty, publicPropertyExcludedFromAAD: savedObjectOriginalAttributes.publicPropertyExcludedFromAAD, + publicDeepProperty: savedObjectOriginalAttributes.publicDeepProperty, }); expect(savedObjects[0].error).to.eql({ message: 'Unable to decrypt attribute "publicPropertyStoredEncrypted"', @@ -294,6 +310,7 @@ export default function ({ getService }: FtrProviderContext) { publicProperty: savedObjectOriginalAttributes.publicProperty, publicPropertyExcludedFromAAD: savedObjectOriginalAttributes.publicPropertyExcludedFromAAD, publicPropertyStoredEncrypted: savedObjectOriginalAttributes.publicPropertyStoredEncrypted, + publicDeepProperty: savedObjectOriginalAttributes.publicDeepProperty, }); expect(savedObjects[0].error).to.be(undefined); }); @@ -303,9 +320,13 @@ export default function ({ getService }: FtrProviderContext) { // encrypted attributes. const updatedPublicProperty = randomness.string(); await supertest - .put(`${getURLAPIBaseURL()}${encryptedSavedObjectType}/${savedObject.id}`) + .put( + `${getURLAPIBaseURL()}update-with-partial/${encryptedSavedObjectType}/${savedObject.id}` + ) .set('kbn-xsrf', 'xxx') - .send({ attributes: { publicProperty: updatedPublicProperty } }) + .send({ + attributes: { publicProperty: updatedPublicProperty }, + }) .expect(200); const { @@ -321,6 +342,7 @@ export default function ({ getService }: FtrProviderContext) { expect(savedObjects[0].attributes).to.eql({ publicProperty: updatedPublicProperty, publicPropertyExcludedFromAAD: savedObjectOriginalAttributes.publicPropertyExcludedFromAAD, + publicDeepProperty: savedObjectOriginalAttributes.publicDeepProperty, }); expect(savedObjects[0].error).to.eql({ message: 'Unable to decrypt attribute "publicPropertyStoredEncrypted"', @@ -333,6 +355,7 @@ export default function ({ getService }: FtrProviderContext) { publicPropertyExcludedFromAAD: randomness.string(), publicPropertyStoredEncrypted: randomness.string(), privateProperty: randomness.string(), + publicDeepProperty: { [randomness.string()]: randomness.string() }, }; const { body: response } = await supertest @@ -345,6 +368,7 @@ export default function ({ getService }: FtrProviderContext) { publicProperty: updatedAttributes.publicProperty, publicPropertyExcludedFromAAD: updatedAttributes.publicPropertyExcludedFromAAD, publicPropertyStoredEncrypted: updatedAttributes.publicPropertyStoredEncrypted, + publicDeepProperty: updatedAttributes.publicDeepProperty, }); const rawAttributes = await getRawSavedObjectAttributes(savedObject); @@ -361,6 +385,32 @@ export default function ({ getService }: FtrProviderContext) { expect(rawAttributes.privateProperty).to.not.be(updatedAttributes.privateProperty); }); + it('#update overwrites objects in their entirety', async () => { + const updatedAttributes = { + publicProperty: randomness.string(), + publicPropertyExcludedFromAAD: randomness.string(), + publicPropertyStoredEncrypted: randomness.string(), + privateProperty: randomness.string(), + publicDeepProperty: { [randomness.string()]: randomness.string() }, + }; + + await supertest + .put(`${getURLAPIBaseURL()}${encryptedSavedObjectType}/${savedObject.id}`) + .set('kbn-xsrf', 'xxx') + .send({ attributes: updatedAttributes }) + .expect(200); + + const { body: decryptedResponse } = await supertest + .get( + `${getURLAPIBaseURL()}get-decrypted-as-internal-user/${encryptedSavedObjectType}/${ + savedObject.id + }` + ) + .expect(200); + + expect(decryptedResponse.attributes).to.eql(updatedAttributes); + }); + it('#getDecryptedAsInternalUser decrypts and returns all attributes', async () => { const { body: decryptedResponse } = await supertest .get( @@ -377,7 +427,9 @@ export default function ({ getService }: FtrProviderContext) { const updatedAttributes = { publicPropertyExcludedFromAAD: randomness.string() }; const { body: response } = await supertest - .put(`${getURLAPIBaseURL()}${encryptedSavedObjectType}/${savedObject.id}`) + .put( + `${getURLAPIBaseURL()}update-with-partial/${encryptedSavedObjectType}/${savedObject.id}` + ) .set('kbn-xsrf', 'xxx') .send({ attributes: updatedAttributes }) .expect(200); @@ -404,7 +456,9 @@ export default function ({ getService }: FtrProviderContext) { const updatedAttributes = { publicProperty: randomness.string() }; const { body: response } = await supertest - .put(`${getURLAPIBaseURL()}${encryptedSavedObjectType}/${savedObject.id}`) + .put( + `${getURLAPIBaseURL()}update-with-partial/${encryptedSavedObjectType}/${savedObject.id}` + ) .set('kbn-xsrf', 'xxx') .send({ attributes: updatedAttributes }) .expect(200); From 797781fadaf43f9c7c486d8df934ed2c1812cced Mon Sep 17 00:00:00 2001 From: Gidi Meir Morris Date: Wed, 5 Aug 2020 12:04:52 +0100 Subject: [PATCH 05/41] added `version` to SavedObjects create api --- .../saved_objects/service/lib/repository.test.js | 10 ++++++++++ .../server/saved_objects/service/lib/repository.ts | 2 ++ .../saved_objects/service/saved_objects_client.ts | 5 +++++ 3 files changed, 17 insertions(+) diff --git a/src/core/server/saved_objects/service/lib/repository.test.js b/src/core/server/saved_objects/service/lib/repository.test.js index 4a9fceb9bf3578..5632b5deaa816e 100644 --- a/src/core/server/saved_objects/service/lib/repository.test.js +++ b/src/core/server/saved_objects/service/lib/repository.test.js @@ -1516,6 +1516,16 @@ describe('SavedObjectsRepository', () => { expect(client.index).toHaveBeenCalled(); }); + it(`should use the ES index with version of ID and version are defined and overwrite=true`, async () => { + await createSuccess(type, attributes, { id, overwrite: true, version: mockVersion }); + expect(client.index).toHaveBeenCalled(); + + expect(client.index.mock.calls[0][0]).toMatchObject({ + if_seq_no: mockVersionProps._seq_no, + if_primary_term: mockVersionProps._primary_term, + }); + }); + it(`should use the ES create action if ID is defined and overwrite=false`, async () => { await createSuccess(type, attributes, { id }); expect(client.create).toHaveBeenCalled(); diff --git a/src/core/server/saved_objects/service/lib/repository.ts b/src/core/server/saved_objects/service/lib/repository.ts index 8b7b1d62c1b7de..0756ab1e50b4ca 100644 --- a/src/core/server/saved_objects/service/lib/repository.ts +++ b/src/core/server/saved_objects/service/lib/repository.ts @@ -220,6 +220,7 @@ export class SavedObjectsRepository { overwrite = false, references = [], refresh = DEFAULT_REFRESH_SETTING, + version, } = options; if (!this._allowedTypes.includes(type)) { @@ -259,6 +260,7 @@ export class SavedObjectsRepository { index: this.getIndexForType(type), refresh, body: raw._source, + ...(overwrite && version ? decodeRequestVersion(version) : {}), }; const { body } = diff --git a/src/core/server/saved_objects/service/saved_objects_client.ts b/src/core/server/saved_objects/service/saved_objects_client.ts index e15a92c92772f3..f5a0455ccad5b7 100644 --- a/src/core/server/saved_objects/service/saved_objects_client.ts +++ b/src/core/server/saved_objects/service/saved_objects_client.ts @@ -37,6 +37,11 @@ export interface SavedObjectsCreateOptions extends SavedObjectsBaseOptions { id?: string; /** Overwrite existing documents (defaults to false) */ overwrite?: boolean; + /** + * An opaque version number which changes on each successful write operation. + * Can be used in conjunction with `overwrite` for implementing optimistic concurrency control. + **/ + version?: string; /** {@inheritDoc SavedObjectsMigrationVersion} */ migrationVersion?: SavedObjectsMigrationVersion; references?: SavedObjectReference[]; From 1b5d8b216bab8fa1803263b8919751af2b195a3f Mon Sep 17 00:00:00 2001 From: Gidi Meir Morris Date: Wed, 5 Aug 2020 12:05:35 +0100 Subject: [PATCH 06/41] use version in ESO updates and updated bulkUpdate to use bulkCreate --- .../plugins/actions/server/actions_client.ts | 21 +++- x-pack/plugins/alerts/server/alerts_client.ts | 116 ++++++++++++------ ...ypted_saved_objects_client_wrapper.test.ts | 32 ++--- .../encrypted_saved_objects_client_wrapper.ts | 7 +- 4 files changed, 113 insertions(+), 63 deletions(-) diff --git a/x-pack/plugins/actions/server/actions_client.ts b/x-pack/plugins/actions/server/actions_client.ts index 6744a8d1116234..1bfe881cdfdeed 100644 --- a/x-pack/plugins/actions/server/actions_client.ts +++ b/x-pack/plugins/actions/server/actions_client.ts @@ -159,12 +159,21 @@ export class ActionsClient { this.actionTypeRegistry.ensureActionTypeEnabled(actionTypeId); - const result = await this.unsecuredSavedObjectsClient.update('action', id, { - actionTypeId, - name, - config: validatedActionTypeConfig as SavedObjectAttributes, - secrets: validatedActionTypeSecrets as SavedObjectAttributes, - }); + const result = await this.unsecuredSavedObjectsClient.update( + 'action', + id, + { + ...existingObject.attributes, + actionTypeId, + name, + config: validatedActionTypeConfig as SavedObjectAttributes, + secrets: validatedActionTypeSecrets as SavedObjectAttributes, + }, + { + references: existingObject.references, + version: existingObject.version, + } + ); return { id, diff --git a/x-pack/plugins/alerts/server/alerts_client.ts b/x-pack/plugins/alerts/server/alerts_client.ts index eec60f924bf384..4aeeb324850ff1 100644 --- a/x-pack/plugins/alerts/server/alerts_client.ts +++ b/x-pack/plugins/alerts/server/alerts_client.ts @@ -230,10 +230,19 @@ export class AlertsClient { } throw e; } - await this.unsecuredSavedObjectsClient.update('alert', createdAlert.id, { - scheduledTaskId: scheduledTask.id, - }); - createdAlert.attributes.scheduledTaskId = scheduledTask.id; + await this.unsecuredSavedObjectsClient.update( + 'alert', + createdAlert.id, + { + ...createdAlert.attributes, + scheduledTaskId: scheduledTask.id, + }, + { + references: createdAlert.references, + version: createdAlert.version, + } + ); + createdAlert.attributes.acscheduledTaskId = scheduledTask.id; } return this.getAlertFromRaw( createdAlert.id, @@ -458,6 +467,7 @@ export class AlertsClient { public async updateApiKey({ id }: { id: string }) { let apiKeyToInvalidate: string | null = null; let attributes: RawAlert; + let references: SavedObjectReference[]; let version: string | undefined; try { @@ -467,6 +477,7 @@ export class AlertsClient { apiKeyToInvalidate = decryptedAlert.attributes.apiKey; attributes = decryptedAlert.attributes; version = decryptedAlert.version; + references = decryptedAlert.references; } catch (e) { // We'll skip invalidating the API key since we failed to load the decrypted saved object this.logger.error( @@ -476,6 +487,7 @@ export class AlertsClient { const alert = await this.unsecuredSavedObjectsClient.get('alert', id); attributes = alert.attributes; version = alert.version; + references = alert.references; } await this.authorization.ensureAuthorized( attributes.alertTypeId, @@ -488,7 +500,7 @@ export class AlertsClient { } const username = await this.getUserName(); - await this.unsecuredSavedObjectsClient.update( + await this.unsecuredSavedObjectsClient.update( 'alert', id, { @@ -499,7 +511,7 @@ export class AlertsClient { ), updatedBy: username, }, - { version } + { version, references } ); if (apiKeyToInvalidate) { @@ -527,6 +539,7 @@ export class AlertsClient { let apiKeyToInvalidate: string | null = null; let attributes: RawAlert; let version: string | undefined; + let references: SavedObjectReference[]; try { const decryptedAlert = await this.encryptedSavedObjectsClient.getDecryptedAsInternalUser< @@ -535,6 +548,7 @@ export class AlertsClient { apiKeyToInvalidate = decryptedAlert.attributes.apiKey; attributes = decryptedAlert.attributes; version = decryptedAlert.version; + references = decryptedAlert.references; } catch (e) { // We'll skip invalidating the API key since we failed to load the decrypted saved object this.logger.error( @@ -544,6 +558,7 @@ export class AlertsClient { const alert = await this.unsecuredSavedObjectsClient.get('alert', id); attributes = alert.attributes; version = alert.version; + references = alert.references; } await this.authorization.ensureAuthorized( @@ -558,7 +573,7 @@ export class AlertsClient { if (attributes.enabled === false) { const username = await this.getUserName(); - await this.unsecuredSavedObjectsClient.update( + const updatedAlert = await this.unsecuredSavedObjectsClient.update( 'alert', id, { @@ -572,12 +587,18 @@ export class AlertsClient { ), updatedBy: username, }, - { version } + { version, references } ); const scheduledTask = await this.scheduleAlert(id, attributes.alertTypeId); - await this.unsecuredSavedObjectsClient.update('alert', id, { - scheduledTaskId: scheduledTask.id, - }); + await this.unsecuredSavedObjectsClient.update( + 'alert', + id, + { + ...updatedAlert.attributes, + scheduledTaskId: scheduledTask.id, + }, + { references: updatedAlert.references } + ); if (apiKeyToInvalidate) { await this.invalidateApiKey({ apiKey: apiKeyToInvalidate }); } @@ -588,6 +609,7 @@ export class AlertsClient { let apiKeyToInvalidate: string | null = null; let attributes: RawAlert; let version: string | undefined; + let references: SavedObjectReference[]; try { const decryptedAlert = await this.encryptedSavedObjectsClient.getDecryptedAsInternalUser< @@ -596,6 +618,7 @@ export class AlertsClient { apiKeyToInvalidate = decryptedAlert.attributes.apiKey; attributes = decryptedAlert.attributes; version = decryptedAlert.version; + references = decryptedAlert.references; } catch (e) { // We'll skip invalidating the API key since we failed to load the decrypted saved object this.logger.error( @@ -605,6 +628,7 @@ export class AlertsClient { const alert = await this.unsecuredSavedObjectsClient.get('alert', id); attributes = alert.attributes; version = alert.version; + references = alert.references; } await this.authorization.ensureAuthorized( @@ -614,18 +638,17 @@ export class AlertsClient { ); if (attributes.enabled === true) { - await this.unsecuredSavedObjectsClient.update( + await this.unsecuredSavedObjectsClient.update( 'alert', id, { ...attributes, enabled: false, - scheduledTaskId: null, apiKey: null, apiKeyOwner: null, updatedBy: await this.getUserName(), }, - { version } + { version, references } ); await Promise.all([ @@ -638,7 +661,9 @@ export class AlertsClient { } public async muteAll({ id }: { id: string }) { - const { attributes } = await this.unsecuredSavedObjectsClient.get('alert', id); + const { attributes, references, version } = await this.unsecuredSavedObjectsClient.get< + RawAlert + >('alert', id); await this.authorization.ensureAuthorized( attributes.alertTypeId, attributes.consumer, @@ -649,15 +674,23 @@ export class AlertsClient { await this.actionsAuthorization.ensureAuthorized('execute'); } - await this.unsecuredSavedObjectsClient.update('alert', id, { - muteAll: true, - mutedInstanceIds: [], - updatedBy: await this.getUserName(), - }); + await this.unsecuredSavedObjectsClient.update( + 'alert', + id, + { + ...attributes, + muteAll: true, + mutedInstanceIds: [], + updatedBy: await this.getUserName(), + }, + { references, version } + ); } public async unmuteAll({ id }: { id: string }) { - const { attributes } = await this.unsecuredSavedObjectsClient.get('alert', id); + const { attributes, references, version } = await this.unsecuredSavedObjectsClient.get< + RawAlert + >('alert', id); await this.authorization.ensureAuthorized( attributes.alertTypeId, attributes.consumer, @@ -668,18 +701,23 @@ export class AlertsClient { await this.actionsAuthorization.ensureAuthorized('execute'); } - await this.unsecuredSavedObjectsClient.update('alert', id, { - muteAll: false, - mutedInstanceIds: [], - updatedBy: await this.getUserName(), - }); + await this.unsecuredSavedObjectsClient.update( + 'alert', + id, + { + ...attributes, + muteAll: false, + mutedInstanceIds: [], + updatedBy: await this.getUserName(), + }, + { references, version } + ); } public async muteInstance({ alertId, alertInstanceId }: MuteOptions) { - const { attributes, version } = await this.unsecuredSavedObjectsClient.get( - 'alert', - alertId - ); + const { attributes, references, version } = await this.unsecuredSavedObjectsClient.get< + RawAlert + >('alert', alertId); await this.authorization.ensureAuthorized( attributes.alertTypeId, @@ -694,14 +732,15 @@ export class AlertsClient { const mutedInstanceIds = attributes.mutedInstanceIds || []; if (!attributes.muteAll && !mutedInstanceIds.includes(alertInstanceId)) { mutedInstanceIds.push(alertInstanceId); - await this.unsecuredSavedObjectsClient.update( + await this.unsecuredSavedObjectsClient.update( 'alert', alertId, { + ...attributes, mutedInstanceIds, updatedBy: await this.getUserName(), }, - { version } + { version, references } ); } } @@ -713,10 +752,9 @@ export class AlertsClient { alertId: string; alertInstanceId: string; }) { - const { attributes, version } = await this.unsecuredSavedObjectsClient.get( - 'alert', - alertId - ); + const { attributes, references, version } = await this.unsecuredSavedObjectsClient.get< + RawAlert + >('alert', alertId); await this.authorization.ensureAuthorized( attributes.alertTypeId, attributes.consumer, @@ -728,15 +766,15 @@ export class AlertsClient { const mutedInstanceIds = attributes.mutedInstanceIds || []; if (!attributes.muteAll && mutedInstanceIds.includes(alertInstanceId)) { - await this.unsecuredSavedObjectsClient.update( + await this.unsecuredSavedObjectsClient.update( 'alert', alertId, { + ...attributes, updatedBy: await this.getUserName(), - mutedInstanceIds: mutedInstanceIds.filter((id: string) => id !== alertInstanceId), }, - { version } + { version, references } ); } } diff --git a/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.test.ts b/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.test.ts index a874f3713c2c6c..fb99ce6b2f4cf0 100644 --- a/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.test.ts +++ b/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.test.ts @@ -409,7 +409,7 @@ describe('#bulkUpdate', () => { saved_objects: [{ id: 'some-id', type: 'unknown-type', attributes, references: [] }], }; - mockBaseClient.bulkUpdate.mockResolvedValue(mockedResponse); + mockBaseClient.bulkCreate.mockResolvedValue(mockedResponse); await expect( wrapper.bulkUpdate( @@ -418,10 +418,10 @@ describe('#bulkUpdate', () => { ) ).resolves.toEqual(mockedResponse); - expect(mockBaseClient.bulkUpdate).toHaveBeenCalledTimes(1); - expect(mockBaseClient.bulkUpdate).toHaveBeenCalledWith( + expect(mockBaseClient.bulkCreate).toHaveBeenCalledTimes(1); + expect(mockBaseClient.bulkCreate).toHaveBeenCalledWith( [{ type: 'unknown-type', id: 'some-id', attributes, version: 'some-version' }], - {} + { overwrite: true } ); }); @@ -461,7 +461,7 @@ describe('#bulkUpdate', () => { })), }; - mockBaseClient.bulkUpdate.mockResolvedValue(mockedResponse); + mockBaseClient.bulkCreate.mockResolvedValue(mockedResponse); await expect( wrapper.bulkUpdate( @@ -513,8 +513,8 @@ describe('#bulkUpdate', () => { { user: mockAuthenticatedUser() } ); - expect(mockBaseClient.bulkUpdate).toHaveBeenCalledTimes(1); - expect(mockBaseClient.bulkUpdate).toHaveBeenCalledWith( + expect(mockBaseClient.bulkCreate).toHaveBeenCalledTimes(1); + expect(mockBaseClient.bulkCreate).toHaveBeenCalledWith( [ { id: 'some-id', @@ -537,7 +537,7 @@ describe('#bulkUpdate', () => { }, }, ], - {} + { overwrite: true } ); }); @@ -557,7 +557,7 @@ describe('#bulkUpdate', () => { ]; const options = { namespace }; - mockBaseClient.bulkUpdate.mockResolvedValue({ + mockBaseClient.bulkCreate.mockResolvedValue({ saved_objects: docs.map((doc) => ({ ...doc, references: undefined })), }); @@ -587,8 +587,8 @@ describe('#bulkUpdate', () => { { user: mockAuthenticatedUser() } ); - expect(mockBaseClient.bulkUpdate).toHaveBeenCalledTimes(1); - expect(mockBaseClient.bulkUpdate).toHaveBeenCalledWith( + expect(mockBaseClient.bulkCreate).toHaveBeenCalledTimes(1); + expect(mockBaseClient.bulkCreate).toHaveBeenCalledWith( [ { id: 'some-id', @@ -603,7 +603,7 @@ describe('#bulkUpdate', () => { references: undefined, }, ], - options + { ...options, overwrite: true } ); }; @@ -621,7 +621,7 @@ describe('#bulkUpdate', () => { const attributes = { attrOne: 'one', attrSecret: 'secret', attrThree: 'three' }; const failureReason = new Error('Something bad happened...'); - mockBaseClient.bulkUpdate.mockRejectedValue(failureReason); + mockBaseClient.bulkCreate.mockRejectedValue(failureReason); await expect( wrapper.bulkUpdate( @@ -630,10 +630,10 @@ describe('#bulkUpdate', () => { ) ).rejects.toThrowError(failureReason); - expect(mockBaseClient.bulkUpdate).toHaveBeenCalledTimes(1); - expect(mockBaseClient.bulkUpdate).toHaveBeenCalledWith( + expect(mockBaseClient.bulkCreate).toHaveBeenCalledTimes(1); + expect(mockBaseClient.bulkCreate).toHaveBeenCalledWith( [{ type: 'unknown-type', id: 'some-id', attributes, version: 'some-version' }], - {} + { overwrite: true } ); }); }); diff --git a/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.ts b/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.ts index 6696ef00b8c55b..388cd1b3882b42 100644 --- a/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.ts +++ b/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.ts @@ -163,7 +163,10 @@ export class EncryptedSavedObjectsClientWrapper implements SavedObjectsClientCon ); return await this.handleEncryptedAttributesInBulkResponse( - await this.options.baseClient.bulkUpdate(encryptedObjects, options), + await this.options.baseClient.bulkCreate(encryptedObjects, { + ...options, + overwrite: true, + }), objects ); } @@ -222,9 +225,9 @@ export class EncryptedSavedObjectsClientWrapper implements SavedObjectsClientCon } )) as T, { + ...options, id, overwrite: true, - ...options, } ), attributes, From a5b4efb287567a0acdf3a59c6f93c32789a1cb93 Mon Sep 17 00:00:00 2001 From: Gidi Meir Morris Date: Wed, 5 Aug 2020 16:23:58 +0100 Subject: [PATCH 07/41] updated core docs --- ...-plugin-core-server.savedobjectscreateoptions.md | 1 + ...core-server.savedobjectscreateoptions.version.md | 13 +++++++++++++ src/core/server/server.api.md | 1 + 3 files changed, 15 insertions(+) create mode 100644 docs/development/core/server/kibana-plugin-core-server.savedobjectscreateoptions.version.md diff --git a/docs/development/core/server/kibana-plugin-core-server.savedobjectscreateoptions.md b/docs/development/core/server/kibana-plugin-core-server.savedobjectscreateoptions.md index 5e9433c5c9196a..c5201efd0608d6 100644 --- a/docs/development/core/server/kibana-plugin-core-server.savedobjectscreateoptions.md +++ b/docs/development/core/server/kibana-plugin-core-server.savedobjectscreateoptions.md @@ -20,4 +20,5 @@ export interface SavedObjectsCreateOptions extends SavedObjectsBaseOptions | [overwrite](./kibana-plugin-core-server.savedobjectscreateoptions.overwrite.md) | boolean | Overwrite existing documents (defaults to false) | | [references](./kibana-plugin-core-server.savedobjectscreateoptions.references.md) | SavedObjectReference[] | | | [refresh](./kibana-plugin-core-server.savedobjectscreateoptions.refresh.md) | MutatingOperationRefreshSetting | The Elasticsearch Refresh setting for this operation | +| [version](./kibana-plugin-core-server.savedobjectscreateoptions.version.md) | string | An opaque version number which changes on each successful write operation. Can be used in conjunction with overwrite for implementing optimistic concurrency control. | diff --git a/docs/development/core/server/kibana-plugin-core-server.savedobjectscreateoptions.version.md b/docs/development/core/server/kibana-plugin-core-server.savedobjectscreateoptions.version.md new file mode 100644 index 00000000000000..51da57064abb9e --- /dev/null +++ b/docs/development/core/server/kibana-plugin-core-server.savedobjectscreateoptions.version.md @@ -0,0 +1,13 @@ + + +[Home](./index.md) > [kibana-plugin-core-server](./kibana-plugin-core-server.md) > [SavedObjectsCreateOptions](./kibana-plugin-core-server.savedobjectscreateoptions.md) > [version](./kibana-plugin-core-server.savedobjectscreateoptions.version.md) + +## SavedObjectsCreateOptions.version property + +An opaque version number which changes on each successful write operation. Can be used in conjunction with `overwrite` for implementing optimistic concurrency control. + +Signature: + +```typescript +version?: string; +``` diff --git a/src/core/server/server.api.md b/src/core/server/server.api.md index 21ef66230f6981..429c22dff8788d 100644 --- a/src/core/server/server.api.md +++ b/src/core/server/server.api.md @@ -2173,6 +2173,7 @@ export interface SavedObjectsCreateOptions extends SavedObjectsBaseOptions { // (undocumented) references?: SavedObjectReference[]; refresh?: MutatingOperationRefreshSetting; + version?: string; } // @public (undocumented) From d5c0038aaccf11cfabeb84f3491b349df6b58652 Mon Sep 17 00:00:00 2001 From: Gidi Meir Morris Date: Thu, 6 Aug 2020 16:33:11 +0100 Subject: [PATCH 08/41] corrected alert updates in client --- .../alerts/server/alerts_client.test.ts | 166 +++++++++++++++--- x-pack/plugins/alerts/server/alerts_client.ts | 99 +++++++---- ...ypted_saved_objects_client_wrapper.test.ts | 2 +- .../fixtures/plugins/alerts/server/routes.ts | 9 +- .../tests/alerting/update.ts | 3 +- 5 files changed, 208 insertions(+), 71 deletions(-) diff --git a/x-pack/plugins/alerts/server/alerts_client.test.ts b/x-pack/plugins/alerts/server/alerts_client.test.ts index c25e040ad09ce8..ed24c3a3124b81 100644 --- a/x-pack/plugins/alerts/server/alerts_client.test.ts +++ b/x-pack/plugins/alerts/server/alerts_client.test.ts @@ -17,6 +17,7 @@ import { encryptedSavedObjectsMock } from '../../encrypted_saved_objects/server/ import { actionsClientMock, actionsAuthorizationMock } from '../../actions/server/mocks'; import { AlertsAuthorization } from './authorization/alerts_authorization'; import { ActionsAuthorization } from '../../actions/server'; +import { pick } from 'lodash'; const taskManager = taskManagerMock.start(); const alertTypeRegistry = alertTypeRegistryMock.create(); @@ -397,14 +398,44 @@ describe('create()', () => { ] `); expect(unsecuredSavedObjectsClient.update).toHaveBeenCalledTimes(1); - expect(unsecuredSavedObjectsClient.update.mock.calls[0]).toHaveLength(3); + expect(unsecuredSavedObjectsClient.update.mock.calls[0]).toHaveLength(4); expect(unsecuredSavedObjectsClient.update.mock.calls[0][0]).toEqual('alert'); expect(unsecuredSavedObjectsClient.update.mock.calls[0][1]).toEqual('1'); expect(unsecuredSavedObjectsClient.update.mock.calls[0][2]).toMatchInlineSnapshot(` - Object { - "scheduledTaskId": "task-123", - } - `); + Object { + "actions": Array [ + Object { + "actionRef": "action_0", + "actionTypeId": "test", + "group": "default", + "params": Object { + "foo": true, + }, + }, + ], + "alertTypeId": "123", + "createdAt": "2019-02-12T21:01:22.479Z", + "params": Object { + "bar": true, + }, + "schedule": Object { + "interval": "10s", + }, + "scheduledTaskId": "task-123", + } + `); + expect(unsecuredSavedObjectsClient.update.mock.calls[0][3]).toMatchInlineSnapshot(` + Object { + "references": Array [ + Object { + "id": "1", + "name": "action_0", + "type": "action", + }, + ], + "version": undefined, + } + `); }); test('creates an alert with multiple actions', async () => { @@ -1075,6 +1106,16 @@ describe('enable()', () => { alertsClientParams.createAPIKey.mockResolvedValue({ apiKeysEnabled: false, }); + unsecuredSavedObjectsClient.update.mockResolvedValueOnce({ + ...existingAlert, + attributes: { + ...existingAlert.attributes, + enabled: true, + apiKey: null, + apiKeyOwner: null, + updatedBy: 'elastic', + }, + }); taskManager.schedule.mockResolvedValue({ id: 'task-123', scheduledAt: new Date(), @@ -1133,6 +1174,17 @@ describe('enable()', () => { }); test('enables an alert', async () => { + unsecuredSavedObjectsClient.update.mockResolvedValueOnce({ + ...existingAlert, + attributes: { + ...existingAlert.attributes, + enabled: true, + apiKey: null, + apiKeyOwner: null, + updatedBy: 'elastic', + }, + }); + await alertsClient.enable({ id: '1' }); expect(unsecuredSavedObjectsClient.get).not.toHaveBeenCalled(); expect(encryptedSavedObjects.getDecryptedAsInternalUser).toHaveBeenCalledWith('alert', '1', { @@ -1165,6 +1217,7 @@ describe('enable()', () => { }, { version: '123', + references: existingAlert.references, } ); expect(taskManager.schedule).toHaveBeenCalledWith({ @@ -1180,9 +1233,22 @@ describe('enable()', () => { }, scope: ['alerting'], }); - expect(unsecuredSavedObjectsClient.update).toHaveBeenCalledWith('alert', '1', { - scheduledTaskId: 'task-123', - }); + expect(unsecuredSavedObjectsClient.update).toHaveBeenCalledWith( + 'alert', + '1', + { + ...existingAlert.attributes, + enabled: true, + apiKey: null, + apiKeyOwner: null, + updatedBy: 'elastic', + scheduledTaskId: 'task-123', + }, + { + version: '123', + references: existingAlert.references, + } + ); }); test('invalidates API key if ever one existed prior to updating', async () => { @@ -1249,6 +1315,7 @@ describe('enable()', () => { ], }, { + references: [], version: '123', } ); @@ -1278,6 +1345,7 @@ describe('enable()', () => { }); test('throws error when failing to update the first time', async () => { + unsecuredSavedObjectsClient.update.mockReset(); unsecuredSavedObjectsClient.update.mockRejectedValueOnce(new Error('Fail to update')); await expect(alertsClient.enable({ id: '1' })).rejects.toThrowErrorMatchingInlineSnapshot( @@ -1290,6 +1358,7 @@ describe('enable()', () => { }); test('throws error when failing to update the second time', async () => { + unsecuredSavedObjectsClient.update.mockReset(); unsecuredSavedObjectsClient.update.mockResolvedValueOnce({ ...existingAlert, attributes: { @@ -1395,10 +1464,7 @@ describe('disable()', () => { consumer: 'myApp', schedule: { interval: '10s' }, alertTypeId: 'myType', - apiKey: null, - apiKeyOwner: null, enabled: false, - scheduledTaskId: null, updatedBy: 'elastic', actions: [ { @@ -1413,6 +1479,7 @@ describe('disable()', () => { ], }, { + references: [], version: '123', } ); @@ -1435,10 +1502,7 @@ describe('disable()', () => { consumer: 'myApp', schedule: { interval: '10s' }, alertTypeId: 'myType', - apiKey: null, - apiKeyOwner: null, enabled: false, - scheduledTaskId: null, updatedBy: 'elastic', actions: [ { @@ -1454,6 +1518,7 @@ describe('disable()', () => { }, { version: '123', + references: [], } ); expect(taskManager.remove).toHaveBeenCalledWith('task-123'); @@ -1545,11 +1610,27 @@ describe('muteAll()', () => { }); await alertsClient.muteAll({ id: '1' }); - expect(unsecuredSavedObjectsClient.update).toHaveBeenCalledWith('alert', '1', { - muteAll: true, - mutedInstanceIds: [], - updatedBy: 'elastic', - }); + expect(unsecuredSavedObjectsClient.update).toHaveBeenCalledWith( + 'alert', + '1', + { + actions: [ + { + group: 'default', + id: '1', + actionTypeId: '1', + actionRef: '1', + params: { + foo: true, + }, + }, + ], + muteAll: true, + mutedInstanceIds: [], + updatedBy: 'elastic', + }, + { references: [] } + ); }); describe('authorization', () => { @@ -1630,11 +1711,27 @@ describe('unmuteAll()', () => { }); await alertsClient.unmuteAll({ id: '1' }); - expect(unsecuredSavedObjectsClient.update).toHaveBeenCalledWith('alert', '1', { - muteAll: false, - mutedInstanceIds: [], - updatedBy: 'elastic', - }); + expect(unsecuredSavedObjectsClient.update).toHaveBeenCalledWith( + 'alert', + '1', + { + actions: [ + { + group: 'default', + id: '1', + actionTypeId: '1', + actionRef: '1', + params: { + foo: true, + }, + }, + ], + muteAll: false, + mutedInstanceIds: [], + updatedBy: 'elastic', + }, + { references: [] } + ); }); describe('authorization', () => { @@ -1714,10 +1811,15 @@ describe('muteInstance()', () => { 'alert', '1', { + actions: [], + schedule: { interval: '10s' }, + alertTypeId: '2', + enabled: true, + scheduledTaskId: 'task-123', mutedInstanceIds: ['2'], updatedBy: 'elastic', }, - { version: '123' } + { version: '123', references: [] } ); }); @@ -1847,10 +1949,18 @@ describe('unmuteInstance()', () => { 'alert', '1', { + actions: [], + schedule: { interval: '10s' }, + alertTypeId: '2', + enabled: true, + scheduledTaskId: 'task-123', mutedInstanceIds: [], updatedBy: 'elastic', }, - { version: '123' } + { + version: '123', + references: [], + } ); }); @@ -3811,7 +3921,7 @@ describe('updateApiKey()', () => { }, ], }, - { version: '123' } + pick(existingAlert, 'version', 'references') ); expect(alertsClientParams.invalidateAPIKey).toHaveBeenCalledWith({ id: '123' }); }); @@ -3847,7 +3957,7 @@ describe('updateApiKey()', () => { }, ], }, - { version: '123' } + pick(existingAlert, 'version', 'references') ); expect(alertsClientParams.invalidateAPIKey).not.toHaveBeenCalled(); }); diff --git a/x-pack/plugins/alerts/server/alerts_client.ts b/x-pack/plugins/alerts/server/alerts_client.ts index 4aeeb324850ff1..fe1c562198e5de 100644 --- a/x-pack/plugins/alerts/server/alerts_client.ts +++ b/x-pack/plugins/alerts/server/alerts_client.ts @@ -5,7 +5,7 @@ */ import Boom from 'boom'; -import { omit, isEqual, map, uniq, pick, truncate } from 'lodash'; +import { omit, omitBy, isUndefined, isEqual, map, uniq, pick, truncate } from 'lodash'; import { i18n } from '@kbn/i18n'; import { Logger, @@ -234,15 +234,15 @@ export class AlertsClient { 'alert', createdAlert.id, { + ...rawAlert, ...createdAlert.attributes, scheduledTaskId: scheduledTask.id, }, { - references: createdAlert.references, - version: createdAlert.version, + references, } ); - createdAlert.attributes.acscheduledTaskId = scheduledTask.id; + createdAlert.attributes.scheduledTaskId = scheduledTask.id; } return this.getAlertFromRaw( createdAlert.id, @@ -511,7 +511,7 @@ export class AlertsClient { ), updatedBy: username, }, - { version, references } + omitBy({ version, references }, isUndefined) ); if (apiKeyToInvalidate) { @@ -573,31 +573,30 @@ export class AlertsClient { if (attributes.enabled === false) { const username = await this.getUserName(); + const updatedAttributes = { + ...attributes, + enabled: true, + ...this.apiKeyAsAlertAttributes( + await this.createAPIKey(this.generateAPIKeyName(attributes.alertTypeId, attributes.name)), + username + ), + updatedBy: username, + }; const updatedAlert = await this.unsecuredSavedObjectsClient.update( 'alert', id, - { - ...attributes, - enabled: true, - ...this.apiKeyAsAlertAttributes( - await this.createAPIKey( - this.generateAPIKeyName(attributes.alertTypeId, attributes.name) - ), - username - ), - updatedBy: username, - }, - { version, references } + updatedAttributes, + omitBy({ version, references }, isUndefined) ); const scheduledTask = await this.scheduleAlert(id, attributes.alertTypeId); await this.unsecuredSavedObjectsClient.update( 'alert', id, { - ...updatedAlert.attributes, + ...updatedAttributes, scheduledTaskId: scheduledTask.id, }, - { references: updatedAlert.references } + omitBy({ version: updatedAlert.version, references: updatedAlert.references }, isUndefined) ); if (apiKeyToInvalidate) { await this.invalidateApiKey({ apiKey: apiKeyToInvalidate }); @@ -642,13 +641,11 @@ export class AlertsClient { 'alert', id, { - ...attributes, + ...omit(attributes, 'scheduledTaskId', 'apiKey', 'apiKeyOwner'), enabled: false, - apiKey: null, - apiKeyOwner: null, updatedBy: await this.getUserName(), }, - { version, references } + omitBy({ version, references }, isUndefined) ); await Promise.all([ @@ -661,9 +658,13 @@ export class AlertsClient { } public async muteAll({ id }: { id: string }) { - const { attributes, references, version } = await this.unsecuredSavedObjectsClient.get< - RawAlert - >('alert', id); + const { + attributes, + references, + version, + } = await this.encryptedSavedObjectsClient.getDecryptedAsInternalUser('alert', id, { + namespace: this.namespace, + }); await this.authorization.ensureAuthorized( attributes.alertTypeId, attributes.consumer, @@ -683,14 +684,18 @@ export class AlertsClient { mutedInstanceIds: [], updatedBy: await this.getUserName(), }, - { references, version } + omitBy({ version, references }, isUndefined) ); } public async unmuteAll({ id }: { id: string }) { - const { attributes, references, version } = await this.unsecuredSavedObjectsClient.get< - RawAlert - >('alert', id); + const { + attributes, + references, + version, + } = await this.encryptedSavedObjectsClient.getDecryptedAsInternalUser('alert', id, { + namespace: this.namespace, + }); await this.authorization.ensureAuthorized( attributes.alertTypeId, attributes.consumer, @@ -710,14 +715,22 @@ export class AlertsClient { mutedInstanceIds: [], updatedBy: await this.getUserName(), }, - { references, version } + omitBy({ version, references }, isUndefined) ); } public async muteInstance({ alertId, alertInstanceId }: MuteOptions) { - const { attributes, references, version } = await this.unsecuredSavedObjectsClient.get< - RawAlert - >('alert', alertId); + const { + attributes, + references, + version, + } = await this.encryptedSavedObjectsClient.getDecryptedAsInternalUser( + 'alert', + alertId, + { + namespace: this.namespace, + } + ); await this.authorization.ensureAuthorized( attributes.alertTypeId, @@ -740,7 +753,7 @@ export class AlertsClient { mutedInstanceIds, updatedBy: await this.getUserName(), }, - { version, references } + omitBy({ version, references }, isUndefined) ); } } @@ -752,9 +765,17 @@ export class AlertsClient { alertId: string; alertInstanceId: string; }) { - const { attributes, references, version } = await this.unsecuredSavedObjectsClient.get< - RawAlert - >('alert', alertId); + const { + attributes, + references, + version, + } = await this.encryptedSavedObjectsClient.getDecryptedAsInternalUser( + 'alert', + alertId, + { + namespace: this.namespace, + } + ); await this.authorization.ensureAuthorized( attributes.alertTypeId, attributes.consumer, @@ -774,7 +795,7 @@ export class AlertsClient { updatedBy: await this.getUserName(), mutedInstanceIds: mutedInstanceIds.filter((id: string) => id !== alertInstanceId), }, - { version, references } + omitBy({ version, references }, isUndefined) ); } } diff --git a/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.test.ts b/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.test.ts index fb99ce6b2f4cf0..8e198719c76322 100644 --- a/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.test.ts +++ b/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.test.ts @@ -457,7 +457,7 @@ describe('#bulkUpdate', () => { attrSecret: `*${doc.attributes.attrSecret}*`, attrNotSoSecret: `*${doc.attributes.attrNotSoSecret}*`, }, - references: undefined, + references: [], })), }; diff --git a/x-pack/test/alerting_api_integration/common/fixtures/plugins/alerts/server/routes.ts b/x-pack/test/alerting_api_integration/common/fixtures/plugins/alerts/server/routes.ts index cc2be05554747d..735dd1f2fc5891 100644 --- a/x-pack/test/alerting_api_integration/common/fixtures/plugins/alerts/server/routes.ts +++ b/x-pack/test/alerting_api_integration/common/fixtures/plugins/alerts/server/routes.ts @@ -11,6 +11,7 @@ import { IKibanaResponse, } from 'kibana/server'; import { schema } from '@kbn/config-schema'; +import { RawAlert } from '../../../../../../../plugins/alerts/server/types'; export function defineRoutes(core: CoreSetup) { const router = core.http.createRouter(); @@ -50,7 +51,13 @@ export function defineRoutes(core: CoreSetup) { const savedObjectsWithAlerts = await savedObjects.getScopedClient(req, { includedHiddenTypes: ['alert'], }); - const result = await savedObjectsWithAlerts.update(type, id, attributes, options); + const savedAlert = await savedObjectsWithAlerts.get(type, id); + const result = await savedObjectsWithAlerts.update( + type, + id, + { ...savedAlert.attributes, ...attributes }, + options + ); return res.ok({ body: result }); } ); diff --git a/x-pack/test/alerting_api_integration/security_and_spaces/tests/alerting/update.ts b/x-pack/test/alerting_api_integration/security_and_spaces/tests/alerting/update.ts index cac6355409ac93..ab3a92d0b3f706 100644 --- a/x-pack/test/alerting_api_integration/security_and_spaces/tests/alerting/update.ts +++ b/x-pack/test/alerting_api_integration/security_and_spaces/tests/alerting/update.ts @@ -31,8 +31,7 @@ export default function createUpdateTests({ getService }: FtrProviderContext) { .then((response: SupertestResponse) => response.body); } - // FLAKY: https://github.com/elastic/kibana/issues/72803 - describe.skip('update', () => { + describe('update', () => { const objectRemover = new ObjectRemover(supertest); after(() => objectRemover.removeAll()); From 06d8c91c85348aca6bec5ff0034c6c83b645633c Mon Sep 17 00:00:00 2001 From: Gidi Meir Morris Date: Tue, 11 Aug 2020 11:49:48 +0100 Subject: [PATCH 09/41] fixed references --- .../encrypted_saved_objects_client_wrapper.test.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.test.ts b/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.test.ts index 8e198719c76322..b5bf815dd79811 100644 --- a/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.test.ts +++ b/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.test.ts @@ -478,6 +478,7 @@ describe('#bulkUpdate', () => { attrNotSoSecret: 'not-so-secret', attrThree: 'three', }, + references: [], }, { id: 'some-id-2', @@ -487,6 +488,7 @@ describe('#bulkUpdate', () => { attrNotSoSecret: 'not-so-secret 2', attrThree: 'three 2', }, + references: [], }, ], }); @@ -558,7 +560,7 @@ describe('#bulkUpdate', () => { const options = { namespace }; mockBaseClient.bulkCreate.mockResolvedValue({ - saved_objects: docs.map((doc) => ({ ...doc, references: undefined })), + saved_objects: docs.map((doc) => ({ ...doc, references: [] })), }); await expect(wrapper.bulkUpdate(docs, options)).resolves.toEqual({ @@ -571,7 +573,7 @@ describe('#bulkUpdate', () => { attrThree: 'three', }, version: 'some-version', - references: undefined, + references: [], }, ], }); From 4fc6ce0c24f6949a3c039fe155424f9a621dccee Mon Sep 17 00:00:00 2001 From: Gidi Meir Morris Date: Tue, 11 Aug 2020 16:15:00 +0100 Subject: [PATCH 10/41] handle mixed bulkUpdates in ESO --- ...ypted_saved_objects_client_wrapper.test.ts | 170 +++++++++++++++++- .../encrypted_saved_objects_client_wrapper.ts | 64 ++++++- 2 files changed, 218 insertions(+), 16 deletions(-) diff --git a/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.test.ts b/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.test.ts index b5bf815dd79811..3c0f2f1f8d2c82 100644 --- a/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.test.ts +++ b/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.test.ts @@ -409,7 +409,7 @@ describe('#bulkUpdate', () => { saved_objects: [{ id: 'some-id', type: 'unknown-type', attributes, references: [] }], }; - mockBaseClient.bulkCreate.mockResolvedValue(mockedResponse); + mockBaseClient.bulkUpdate.mockResolvedValue(mockedResponse); await expect( wrapper.bulkUpdate( @@ -418,10 +418,10 @@ describe('#bulkUpdate', () => { ) ).resolves.toEqual(mockedResponse); - expect(mockBaseClient.bulkCreate).toHaveBeenCalledTimes(1); - expect(mockBaseClient.bulkCreate).toHaveBeenCalledWith( + expect(mockBaseClient.bulkUpdate).toHaveBeenCalledTimes(1); + expect(mockBaseClient.bulkUpdate).toHaveBeenCalledWith( [{ type: 'unknown-type', id: 'some-id', attributes, version: 'some-version' }], - { overwrite: true } + {} ); }); @@ -543,6 +543,160 @@ describe('#bulkUpdate', () => { ); }); + it('handles mixed encrypted and non encrypted objects', async () => { + const encryptedDocs = [ + { + id: 'some-encrypted-id', + type: 'known-type', + attributes: { + attrOne: 'one', + attrSecret: 'secret', + attrNotSoSecret: 'not-so-secret', + attrThree: 'three', + }, + }, + { + id: 'some-encrypted-id-2', + type: 'known-type', + attributes: { + attrOne: 'one 2', + attrSecret: 'secret 2', + attrNotSoSecret: 'not-so-secret 2', + attrThree: 'three 2', + }, + }, + ]; + + mockBaseClient.bulkCreate.mockResolvedValue({ + saved_objects: encryptedDocs.map((doc) => ({ + ...doc, + attributes: { + ...doc.attributes, + attrSecret: `*${doc.attributes.attrSecret}*`, + attrNotSoSecret: `*${doc.attributes.attrNotSoSecret}*`, + }, + references: [], + })), + }); + + const nonEncyptedDoc = { + id: 'some-id', + type: 'unknown-type', + attributes: { attrOne: 'one', attrSecret: 'secret', attrThree: 'three' }, + references: [], + }; + mockBaseClient.bulkUpdate.mockResolvedValue({ + saved_objects: [nonEncyptedDoc], + }); + + await expect( + wrapper.bulkUpdate( + [ + { ...encryptedDocs[0] }, + { + type: 'unknown-type', + id: 'some-id', + attributes: nonEncyptedDoc.attributes, + version: 'some-version', + }, + { ...encryptedDocs[1] }, + ], + {} + ) + ).resolves.toEqual({ + saved_objects: [ + { + id: 'some-encrypted-id', + type: 'known-type', + attributes: { + attrOne: 'one', + attrNotSoSecret: 'not-so-secret', + attrThree: 'three', + }, + references: [], + }, + { + id: 'some-id', + type: 'unknown-type', + attributes: nonEncyptedDoc.attributes, + references: [], + }, + { + id: 'some-encrypted-id-2', + type: 'known-type', + attributes: { + attrOne: 'one 2', + attrNotSoSecret: 'not-so-secret 2', + attrThree: 'three 2', + }, + references: [], + }, + ], + }); + + expect(encryptedSavedObjectsServiceMockInstance.encryptAttributes).toHaveBeenCalledTimes(2); + expect(encryptedSavedObjectsServiceMockInstance.encryptAttributes).toHaveBeenCalledWith( + { type: 'known-type', id: 'some-encrypted-id' }, + { + attrOne: 'one', + attrSecret: 'secret', + attrNotSoSecret: 'not-so-secret', + attrThree: 'three', + }, + { user: mockAuthenticatedUser() } + ); + expect(encryptedSavedObjectsServiceMockInstance.encryptAttributes).toHaveBeenCalledWith( + { type: 'known-type', id: 'some-encrypted-id-2' }, + { + attrOne: 'one 2', + attrSecret: 'secret 2', + attrNotSoSecret: 'not-so-secret 2', + attrThree: 'three 2', + }, + { user: mockAuthenticatedUser() } + ); + + expect(mockBaseClient.bulkCreate).toHaveBeenCalledTimes(1); + expect(mockBaseClient.bulkCreate).toHaveBeenCalledWith( + [ + { + id: 'some-encrypted-id', + type: 'known-type', + attributes: { + attrOne: 'one', + attrSecret: '*secret*', + attrNotSoSecret: '*not-so-secret*', + attrThree: 'three', + }, + }, + { + id: 'some-encrypted-id-2', + type: 'known-type', + attributes: { + attrOne: 'one 2', + attrSecret: '*secret 2*', + attrNotSoSecret: '*not-so-secret 2*', + attrThree: 'three 2', + }, + }, + ], + { overwrite: true } + ); + + expect(mockBaseClient.bulkUpdate).toHaveBeenCalledTimes(1); + expect(mockBaseClient.bulkUpdate).toHaveBeenCalledWith( + [ + { + type: 'unknown-type', + id: 'some-id', + attributes: nonEncyptedDoc.attributes, + version: 'some-version', + }, + ], + {} + ); + }); + describe('namespace', () => { const doTest = async (namespace: string, expectNamespaceInDescriptor: boolean) => { const docs = [ @@ -623,7 +777,7 @@ describe('#bulkUpdate', () => { const attributes = { attrOne: 'one', attrSecret: 'secret', attrThree: 'three' }; const failureReason = new Error('Something bad happened...'); - mockBaseClient.bulkCreate.mockRejectedValue(failureReason); + mockBaseClient.bulkUpdate.mockRejectedValue(failureReason); await expect( wrapper.bulkUpdate( @@ -632,10 +786,10 @@ describe('#bulkUpdate', () => { ) ).rejects.toThrowError(failureReason); - expect(mockBaseClient.bulkCreate).toHaveBeenCalledTimes(1); - expect(mockBaseClient.bulkCreate).toHaveBeenCalledWith( + expect(mockBaseClient.bulkUpdate).toHaveBeenCalledTimes(1); + expect(mockBaseClient.bulkUpdate).toHaveBeenCalledWith( [{ type: 'unknown-type', id: 'some-id', attributes, version: 'some-version' }], - { overwrite: true } + {} ); }); }); diff --git a/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.ts b/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.ts index 388cd1b3882b42..2a65084a466422 100644 --- a/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.ts +++ b/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.ts @@ -137,15 +137,29 @@ export class EncryptedSavedObjectsClientWrapper implements SavedObjectsClientCon objects: Array>, options?: SavedObjectsBaseOptions ) { + const { nonEncryptedObjects, encryptableObjects, idsInOrder } = objects.reduce<{ + nonEncryptedObjects: Array>; + encryptableObjects: Array>; + idsInOrder: string[]; + }>( + (partition, object) => { + if (this.options.service.isRegistered(object.type)) { + partition.encryptableObjects.push(object); + } else { + partition.nonEncryptedObjects.push(object); + } + partition.idsInOrder.push(object.id); + return partition; + }, + { nonEncryptedObjects: [], encryptableObjects: [], idsInOrder: [] } + ); + // We encrypt attributes for every object in parallel and that can potentially exhaust libuv or // NodeJS thread pool. If it turns out to be a problem, we can consider switching to the // sequential processing. const encryptedObjects = await Promise.all( - objects.map(async (object) => { + encryptableObjects.map(async (object) => { const { type, id, attributes } = object; - if (!this.options.service.isRegistered(type)) { - return object; - } const namespace = getDescriptorNamespace( this.options.baseTypeRegistry, type, @@ -163,10 +177,22 @@ export class EncryptedSavedObjectsClientWrapper implements SavedObjectsClientCon ); return await this.handleEncryptedAttributesInBulkResponse( - await this.options.baseClient.bulkCreate(encryptedObjects, { - ...options, - overwrite: true, - }), + { + saved_objects: this.mergeBulkResponsesInOrder( + idsInOrder, + encryptedObjects.length + ? ( + await this.options.baseClient.bulkCreate(encryptedObjects, { + ...options, + overwrite: true, + }) + ).saved_objects + : [], + nonEncryptedObjects.length + ? (await this.options.baseClient.bulkUpdate(nonEncryptedObjects, options)).saved_objects + : [] + ), + }, objects ); } @@ -312,4 +338,26 @@ export class EncryptedSavedObjectsClientWrapper implements SavedObjectsClientCon return response; } + + /** + * merges two bulkResponses into one maintaining the order as specified in `idsInOrder`. + * pushes any response with without a clear index to the bottom maintaining its relative position + * @param idsInOrder array of string object IDs whose order dictates the final merged response + * @param responses array of responses to merge + */ + private mergeBulkResponsesInOrder< + T, + R extends + | SavedObjectsBulkResponse + | SavedObjectsFindResponse + | SavedObjectsBulkUpdateResponse + >(idsInOrder: string[], ...responses: Array): R['saved_objects'] { + const ResponsesById = new Map(); + for (const response of responses) { + for (const object of response) { + ResponsesById.set(object.id, object); + } + } + return idsInOrder.map((id) => ResponsesById.get(id)); + } } From 283d3ace7a0b5aaf434c536528725694a1701fc1 Mon Sep 17 00:00:00 2001 From: Gidi Meir Morris Date: Tue, 11 Aug 2020 18:19:29 +0100 Subject: [PATCH 11/41] merge returned docs in bulkUpdate in a slightly cleaner manner --- .../encrypted_saved_objects_client_wrapper.ts | 92 +++++++++++-------- 1 file changed, 55 insertions(+), 37 deletions(-) diff --git a/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.ts b/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.ts index 2a65084a466422..8573d34ea0f21f 100644 --- a/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.ts +++ b/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.ts @@ -137,21 +137,33 @@ export class EncryptedSavedObjectsClientWrapper implements SavedObjectsClientCon objects: Array>, options?: SavedObjectsBaseOptions ) { - const { nonEncryptedObjects, encryptableObjects, idsInOrder } = objects.reduce<{ + const { + nonEncryptedObjects, + nonEncryptedObjectsIndicies, + encryptableObjects, + encryptableObjectsIndicies, + } = objects.reduce<{ nonEncryptedObjects: Array>; encryptableObjects: Array>; - idsInOrder: string[]; + nonEncryptedObjectsIndicies: number[]; + encryptableObjectsIndicies: number[]; }>( - (partition, object) => { + (partition, object, index) => { if (this.options.service.isRegistered(object.type)) { partition.encryptableObjects.push(object); + partition.encryptableObjectsIndicies.push(index); } else { partition.nonEncryptedObjects.push(object); + partition.nonEncryptedObjectsIndicies.push(index); } - partition.idsInOrder.push(object.id); return partition; }, - { nonEncryptedObjects: [], encryptableObjects: [], idsInOrder: [] } + { + nonEncryptedObjects: [], + encryptableObjects: [], + nonEncryptedObjectsIndicies: [], + encryptableObjectsIndicies: [], + } ); // We encrypt attributes for every object in parallel and that can potentially exhaust libuv or @@ -178,20 +190,36 @@ export class EncryptedSavedObjectsClientWrapper implements SavedObjectsClientCon return await this.handleEncryptedAttributesInBulkResponse( { - saved_objects: this.mergeBulkResponsesInOrder( - idsInOrder, - encryptedObjects.length - ? ( - await this.options.baseClient.bulkCreate(encryptedObjects, { - ...options, - overwrite: true, - }) - ).saved_objects - : [], - nonEncryptedObjects.length - ? (await this.options.baseClient.bulkUpdate(nonEncryptedObjects, options)).saved_objects - : [] - ), + saved_objects: [ + // match up each object in the response with + // the index it had in the original input. + // if the object didn't have an index then assign + // the max possible index so it'll get pushed to the bottom + ...this.zip( + encryptedObjects.length + ? ( + await this.options.baseClient.bulkCreate(encryptedObjects, { + ...options, + overwrite: true, + }) + ).saved_objects + : [], + encryptableObjectsIndicies, + Number.MAX_VALUE + ), + ...this.zip( + nonEncryptedObjects.length + ? (await this.options.baseClient.bulkUpdate(nonEncryptedObjects, options)) + .saved_objects + : [], + nonEncryptedObjectsIndicies, + Number.MAX_VALUE + ), + ] + // sort by the index in the original input + .sort(([, leftIndex], [, rightIndex]) => leftIndex - rightIndex) + // drop the index now that we've sorted + .map(([object]) => object), }, objects ); @@ -340,24 +368,14 @@ export class EncryptedSavedObjectsClientWrapper implements SavedObjectsClientCon } /** - * merges two bulkResponses into one maintaining the order as specified in `idsInOrder`. - * pushes any response with without a clear index to the bottom maintaining its relative position - * @param idsInOrder array of string object IDs whose order dictates the final merged response - * @param responses array of responses to merge + * Zip two arrays together so each cell contains a tupple of two item, one from each array in that index. + * All items from the left array will be retained, and only as many items as needed will be pulled from right + * @param left array of items of any type + * @param right array of items of any type + * @param defaultZipValue default value to use when pulling from the left array and + * a corresponding item is missing in the right array */ - private mergeBulkResponsesInOrder< - T, - R extends - | SavedObjectsBulkResponse - | SavedObjectsFindResponse - | SavedObjectsBulkUpdateResponse - >(idsInOrder: string[], ...responses: Array): R['saved_objects'] { - const ResponsesById = new Map(); - for (const response of responses) { - for (const object of response) { - ResponsesById.set(object.id, object); - } - } - return idsInOrder.map((id) => ResponsesById.get(id)); + private zip(left: T[], right: G[], defaultZipValue: G) { + return left.map<[T, G]>((leftItem, index) => [leftItem, right[index] ?? defaultZipValue]); } } From e8748e97c2b1a7b352a98395e659fee59ab313da Mon Sep 17 00:00:00 2001 From: Gidi Meir Morris Date: Wed, 12 Aug 2020 12:28:58 +0100 Subject: [PATCH 12/41] fixed unit tesst and usage of ESO in actions client --- .../plugins/actions/server/actions_client.ts | 20 ++-- .../alerts/server/alerts_client.test.ts | 93 ++++++++++++------- x-pack/plugins/alerts/server/alerts_client.ts | 5 +- 3 files changed, 77 insertions(+), 41 deletions(-) diff --git a/x-pack/plugins/actions/server/actions_client.ts b/x-pack/plugins/actions/server/actions_client.ts index 26a13ed83cc283..b62ca500edccaf 100644 --- a/x-pack/plugins/actions/server/actions_client.ts +++ b/x-pack/plugins/actions/server/actions_client.ts @@ -13,6 +13,7 @@ import { } from 'src/core/server'; import { i18n } from '@kbn/i18n'; +import { omitBy, isUndefined } from 'lodash'; import { ActionTypeRegistry } from './action_type_registry'; import { validateConfig, validateSecrets, ActionExecutorContract } from './lib'; import { @@ -150,8 +151,10 @@ export class ActionsClient { 'update' ); } - const existingObject = await this.unsecuredSavedObjectsClient.get('action', id); - const { actionTypeId } = existingObject.attributes; + const { attributes, references, version } = await this.unsecuredSavedObjectsClient.get< + RawAction + >('action', id); + const { actionTypeId } = attributes; const { name, config, secrets } = action; const actionType = this.actionTypeRegistry.get(actionTypeId); const validatedActionTypeConfig = validateConfig(actionType, config); @@ -163,16 +166,19 @@ export class ActionsClient { 'action', id, { - ...existingObject.attributes, + ...attributes, actionTypeId, name, config: validatedActionTypeConfig as SavedObjectAttributes, secrets: validatedActionTypeSecrets as SavedObjectAttributes, }, - { - references: existingObject.references, - version: existingObject.version, - } + omitBy( + { + references, + version, + }, + isUndefined + ) ); return { diff --git a/x-pack/plugins/alerts/server/alerts_client.test.ts b/x-pack/plugins/alerts/server/alerts_client.test.ts index ed24c3a3124b81..20ec62b25175e4 100644 --- a/x-pack/plugins/alerts/server/alerts_client.test.ts +++ b/x-pack/plugins/alerts/server/alerts_client.test.ts @@ -244,27 +244,33 @@ describe('create()', () => { test('creates an alert', async () => { const data = getMockData(); + const createdAttributes = { + ...data, + alertTypeId: '123', + schedule: { interval: '10s' }, + params: { + bar: true, + }, + createdAt: '2019-02-12T21:01:22.479Z', + createdBy: 'elastic', + updatedBy: 'elastic', + muteAll: false, + mutedInstanceIds: [], + actions: [ + { + group: 'default', + actionRef: 'action_0', + actionTypeId: 'test', + params: { + foo: true, + }, + }, + ], + }; unsecuredSavedObjectsClient.create.mockResolvedValueOnce({ id: '1', type: 'alert', - attributes: { - alertTypeId: '123', - schedule: { interval: '10s' }, - params: { - bar: true, - }, - createdAt: '2019-02-12T21:01:22.479Z', - actions: [ - { - group: 'default', - actionRef: 'action_0', - actionTypeId: 'test', - params: { - foo: true, - }, - }, - ], - }, + attributes: createdAttributes, references: [ { name: 'action_0', @@ -290,7 +296,7 @@ describe('create()', () => { id: '1', type: 'alert', attributes: { - actions: [], + ...createdAttributes, scheduledTaskId: 'task-123', }, references: [ @@ -316,8 +322,14 @@ describe('create()', () => { }, ], "alertTypeId": "123", + "consumer": "bar", "createdAt": 2019-02-12T21:01:22.479Z, + "createdBy": "elastic", + "enabled": true, "id": "1", + "muteAll": false, + "mutedInstanceIds": Array [], + "name": "abc", "params": Object { "bar": true, }, @@ -325,7 +337,12 @@ describe('create()', () => { "interval": "10s", }, "scheduledTaskId": "task-123", + "tags": Array [ + "foo", + ], + "throttle": null, "updatedAt": 2019-02-12T21:01:22.479Z, + "updatedBy": "elastic", } `); expect(unsecuredSavedObjectsClient.create).toHaveBeenCalledTimes(1); @@ -414,7 +431,15 @@ describe('create()', () => { }, ], "alertTypeId": "123", + "apiKey": null, + "apiKeyOwner": null, + "consumer": "bar", "createdAt": "2019-02-12T21:01:22.479Z", + "createdBy": "elastic", + "enabled": true, + "muteAll": false, + "mutedInstanceIds": Array [], + "name": "abc", "params": Object { "bar": true, }, @@ -422,6 +447,11 @@ describe('create()', () => { "interval": "10s", }, "scheduledTaskId": "task-123", + "tags": Array [ + "foo", + ], + "throttle": null, + "updatedBy": "elastic", } `); expect(unsecuredSavedObjectsClient.update.mock.calls[0][3]).toMatchInlineSnapshot(` @@ -433,7 +463,6 @@ describe('create()', () => { "type": "action", }, ], - "version": undefined, } `); }); @@ -1589,7 +1618,7 @@ describe('disable()', () => { describe('muteAll()', () => { test('mutes an alert', async () => { const alertsClient = new AlertsClient(alertsClientParams); - unsecuredSavedObjectsClient.get.mockResolvedValueOnce({ + encryptedSavedObjects.getDecryptedAsInternalUser.mockResolvedValueOnce({ id: '1', type: 'alert', attributes: { @@ -1635,7 +1664,7 @@ describe('muteAll()', () => { describe('authorization', () => { beforeEach(() => { - unsecuredSavedObjectsClient.get.mockResolvedValueOnce({ + encryptedSavedObjects.getDecryptedAsInternalUser.mockResolvedValueOnce({ id: '1', type: 'alert', attributes: { @@ -1690,7 +1719,7 @@ describe('muteAll()', () => { describe('unmuteAll()', () => { test('unmutes an alert', async () => { const alertsClient = new AlertsClient(alertsClientParams); - unsecuredSavedObjectsClient.get.mockResolvedValueOnce({ + encryptedSavedObjects.getDecryptedAsInternalUser.mockResolvedValueOnce({ id: '1', type: 'alert', attributes: { @@ -1736,7 +1765,7 @@ describe('unmuteAll()', () => { describe('authorization', () => { beforeEach(() => { - unsecuredSavedObjectsClient.get.mockResolvedValueOnce({ + encryptedSavedObjects.getDecryptedAsInternalUser.mockResolvedValueOnce({ id: '1', type: 'alert', attributes: { @@ -1791,7 +1820,7 @@ describe('unmuteAll()', () => { describe('muteInstance()', () => { test('mutes an alert instance', async () => { const alertsClient = new AlertsClient(alertsClientParams); - unsecuredSavedObjectsClient.get.mockResolvedValueOnce({ + encryptedSavedObjects.getDecryptedAsInternalUser.mockResolvedValueOnce({ id: '1', type: 'alert', attributes: { @@ -1825,7 +1854,7 @@ describe('muteInstance()', () => { test('skips muting when alert instance already muted', async () => { const alertsClient = new AlertsClient(alertsClientParams); - unsecuredSavedObjectsClient.get.mockResolvedValueOnce({ + encryptedSavedObjects.getDecryptedAsInternalUser.mockResolvedValueOnce({ id: '1', type: 'alert', attributes: { @@ -1845,7 +1874,7 @@ describe('muteInstance()', () => { test('skips muting when alert is muted', async () => { const alertsClient = new AlertsClient(alertsClientParams); - unsecuredSavedObjectsClient.get.mockResolvedValueOnce({ + encryptedSavedObjects.getDecryptedAsInternalUser.mockResolvedValueOnce({ id: '1', type: 'alert', attributes: { @@ -1866,7 +1895,7 @@ describe('muteInstance()', () => { describe('authorization', () => { beforeEach(() => { - unsecuredSavedObjectsClient.get.mockResolvedValueOnce({ + encryptedSavedObjects.getDecryptedAsInternalUser.mockResolvedValueOnce({ id: '1', type: 'alert', attributes: { @@ -1929,7 +1958,7 @@ describe('muteInstance()', () => { describe('unmuteInstance()', () => { test('unmutes an alert instance', async () => { const alertsClient = new AlertsClient(alertsClientParams); - unsecuredSavedObjectsClient.get.mockResolvedValueOnce({ + encryptedSavedObjects.getDecryptedAsInternalUser.mockResolvedValueOnce({ id: '1', type: 'alert', attributes: { @@ -1966,7 +1995,7 @@ describe('unmuteInstance()', () => { test('skips unmuting when alert instance not muted', async () => { const alertsClient = new AlertsClient(alertsClientParams); - unsecuredSavedObjectsClient.get.mockResolvedValueOnce({ + encryptedSavedObjects.getDecryptedAsInternalUser.mockResolvedValueOnce({ id: '1', type: 'alert', attributes: { @@ -1986,7 +2015,7 @@ describe('unmuteInstance()', () => { test('skips unmuting when alert is muted', async () => { const alertsClient = new AlertsClient(alertsClientParams); - unsecuredSavedObjectsClient.get.mockResolvedValueOnce({ + encryptedSavedObjects.getDecryptedAsInternalUser.mockResolvedValueOnce({ id: '1', type: 'alert', attributes: { @@ -2007,7 +2036,7 @@ describe('unmuteInstance()', () => { describe('authorization', () => { beforeEach(() => { - unsecuredSavedObjectsClient.get.mockResolvedValueOnce({ + encryptedSavedObjects.getDecryptedAsInternalUser.mockResolvedValueOnce({ id: '1', type: 'alert', attributes: { diff --git a/x-pack/plugins/alerts/server/alerts_client.ts b/x-pack/plugins/alerts/server/alerts_client.ts index 29d0cc328b93e3..0cb049c4de425f 100644 --- a/x-pack/plugins/alerts/server/alerts_client.ts +++ b/x-pack/plugins/alerts/server/alerts_client.ts @@ -199,9 +199,10 @@ export class AlertsClient { this.validateActions(alertType, data.actions); const { references, actions } = await this.denormalizeActions(data.actions); + const encryptedAttributes = this.apiKeyAsAlertAttributes(createdAPIKey, username); const rawAlert: RawAlert = { ...data, - ...this.apiKeyAsAlertAttributes(createdAPIKey, username), + ...encryptedAttributes, actions, createdBy: username, updatedBy: username, @@ -234,8 +235,8 @@ export class AlertsClient { 'alert', createdAlert.id, { - ...rawAlert, ...createdAlert.attributes, + ...encryptedAttributes, scheduledTaskId: scheduledTask.id, }, { From c4829bf3719ee4264cf1d3fa8827d1cb441d2a20 Mon Sep 17 00:00:00 2001 From: Gidi Meir Morris Date: Wed, 12 Aug 2020 16:00:12 +0100 Subject: [PATCH 13/41] fixed snapshot --- x-pack/plugins/actions/server/actions_client.test.ts | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/x-pack/plugins/actions/server/actions_client.test.ts b/x-pack/plugins/actions/server/actions_client.test.ts index 16a5a59882dd6c..24b41bc197fc1e 100644 --- a/x-pack/plugins/actions/server/actions_client.test.ts +++ b/x-pack/plugins/actions/server/actions_client.test.ts @@ -983,6 +983,9 @@ describe('update()', () => { "name": "my name", "secrets": Object {}, }, + Object { + "references": Array [], + }, ] `); expect(unsecuredSavedObjectsClient.get).toHaveBeenCalledTimes(1); @@ -1096,6 +1099,9 @@ describe('update()', () => { "name": "my name", "secrets": Object {}, }, + Object { + "references": Array [], + }, ] `); }); From 489b2d118db4210345983eaf17b77cedb3529ee0 Mon Sep 17 00:00:00 2001 From: Gidi Meir Morris Date: Thu, 13 Aug 2020 12:36:02 +0100 Subject: [PATCH 14/41] prevent partial updates at typing level in alerts client --- x-pack/plugins/alerts/server/alerts_client.ts | 17 ++++---- .../alerts/server/saved_objects/index.ts | 2 + ..._objects_client_without_partial_updates.ts | 42 +++++++++++++++++++ 3 files changed, 52 insertions(+), 9 deletions(-) create mode 100644 x-pack/plugins/alerts/server/saved_objects/saved_objects_client_without_partial_updates.ts diff --git a/x-pack/plugins/alerts/server/alerts_client.ts b/x-pack/plugins/alerts/server/alerts_client.ts index 0cb049c4de425f..cd4507195857f6 100644 --- a/x-pack/plugins/alerts/server/alerts_client.ts +++ b/x-pack/plugins/alerts/server/alerts_client.ts @@ -7,12 +7,7 @@ import Boom from 'boom'; import { omit, omitBy, isUndefined, isEqual, map, uniq, pick, truncate } from 'lodash'; import { i18n } from '@kbn/i18n'; -import { - Logger, - SavedObjectsClientContract, - SavedObjectReference, - SavedObject, -} from 'src/core/server'; +import { Logger, SavedObjectReference, SavedObject } from 'src/core/server'; import { ActionsClient, ActionsAuthorization } from '../../actions/server'; import { Alert, @@ -41,6 +36,7 @@ import { WriteOperations, ReadOperations, } from './authorization/alerts_authorization'; +import { SavedObjectsClientWithoutPartialUpdates } from './saved_objects'; export interface RegistryAlertTypeWithAuth extends RegistryAlertType { authorizedConsumers: string[]; @@ -56,7 +52,7 @@ export type InvalidateAPIKeyResult = export interface ConstructorOptions { logger: Logger; taskManager: TaskManagerStartContract; - unsecuredSavedObjectsClient: SavedObjectsClientContract; + unsecuredSavedObjectsClient: SavedObjectsClientWithoutPartialUpdates; authorization: AlertsAuthorization; actionsAuthorization: ActionsAuthorization; alertTypeRegistry: AlertTypeRegistry; @@ -138,7 +134,7 @@ export class AlertsClient { private readonly spaceId?: string; private readonly namespace?: string; private readonly taskManager: TaskManagerStartContract; - private readonly unsecuredSavedObjectsClient: SavedObjectsClientContract; + private readonly unsecuredSavedObjectsClient: SavedObjectsClientWithoutPartialUpdates; private readonly authorization: AlertsAuthorization; private readonly alertTypeRegistry: AlertTypeRegistry; private readonly createAPIKey: (name: string) => Promise; @@ -650,7 +646,10 @@ export class AlertsClient { 'alert', id, { - ...omit(attributes, 'scheduledTaskId', 'apiKey', 'apiKeyOwner'), + ...attributes, + scheduledTaskId: undefined, + apiKey: null, + apiKeyOwner: null, enabled: false, updatedBy: await this.getUserName(), }, diff --git a/x-pack/plugins/alerts/server/saved_objects/index.ts b/x-pack/plugins/alerts/server/saved_objects/index.ts index c98d9bcbd9ae55..32f14a666cfb63 100644 --- a/x-pack/plugins/alerts/server/saved_objects/index.ts +++ b/x-pack/plugins/alerts/server/saved_objects/index.ts @@ -8,6 +8,8 @@ import { SavedObjectsServiceSetup } from 'kibana/server'; import mappings from './mappings.json'; import { EncryptedSavedObjectsPluginSetup } from '../../../encrypted_saved_objects/server'; +export { SavedObjectsClientWithoutPartialUpdates } from './saved_objects_client_without_partial_updates'; + export function setupSavedObjects( savedObjects: SavedObjectsServiceSetup, encryptedSavedObjects: EncryptedSavedObjectsPluginSetup diff --git a/x-pack/plugins/alerts/server/saved_objects/saved_objects_client_without_partial_updates.ts b/x-pack/plugins/alerts/server/saved_objects/saved_objects_client_without_partial_updates.ts new file mode 100644 index 00000000000000..8d603942419d13 --- /dev/null +++ b/x-pack/plugins/alerts/server/saved_objects/saved_objects_client_without_partial_updates.ts @@ -0,0 +1,42 @@ +/* + * 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 { + SavedObjectsClientContract, + SavedObjectsUpdateOptions, + SavedObjectsUpdateResponse, + SavedObjectsBulkUpdateOptions, + SavedObjectsBulkUpdateObject, + SavedObjectsBulkUpdateResponse, +} from 'kibana/server'; + +export interface SavedObjectsBulkUpdateObjectWithoutPartialUpdates + extends SavedObjectsBulkUpdateObject { + attributes: T; +} + +export interface SavedObjectsBulkUpdateResponseWithoutPartialUpdates + extends SavedObjectsBulkUpdateResponse { + saved_objects: Array>; +} + +export interface SavedObjectsUpdateResponseWithoutPartialUpdates + extends SavedObjectsUpdateResponse { + attributes: T; +} + +export interface SavedObjectsClientWithoutPartialUpdates extends SavedObjectsClientContract { + update( + type: string, + id: string, + attributes: T, + options: SavedObjectsUpdateOptions + ): Promise>; + + bulkUpdate( + objects: Array>, + options?: SavedObjectsBulkUpdateOptions + ): Promise>; +} From 79256e04345f5b39e0ab99abba42700afd3023b5 Mon Sep 17 00:00:00 2001 From: Gidi Meir Morris Date: Thu, 13 Aug 2020 13:47:49 +0100 Subject: [PATCH 15/41] fixed typing issue in update --- x-pack/plugins/alerts/server/alerts_client.ts | 19 +++++++++------- x-pack/plugins/alerts/server/types.ts | 22 +++++++++++++++++-- 2 files changed, 31 insertions(+), 10 deletions(-) diff --git a/x-pack/plugins/alerts/server/alerts_client.ts b/x-pack/plugins/alerts/server/alerts_client.ts index cd4507195857f6..0c0c2eaa990070 100644 --- a/x-pack/plugins/alerts/server/alerts_client.ts +++ b/x-pack/plugins/alerts/server/alerts_client.ts @@ -19,6 +19,7 @@ import { IntervalSchedule, SanitizedAlert, AlertTaskState, + AlertUpdateRequiredFields, } from './types'; import { validateAlertTypeParams } from './lib'; import { @@ -324,7 +325,7 @@ export class AlertsClient { public async delete({ id }: { id: string }) { let taskIdToRemove: string | undefined; - let apiKeyToInvalidate: string | null = null; + let apiKeyToInvalidate: string | null | undefined = null; let attributes: RawAlert; try { @@ -470,7 +471,7 @@ export class AlertsClient { } public async updateApiKey({ id }: { id: string }) { - let apiKeyToInvalidate: string | null = null; + let apiKeyToInvalidate: string | null | undefined = null; let attributes: RawAlert; let references: SavedObjectReference[]; let version: string | undefined; @@ -541,7 +542,7 @@ export class AlertsClient { } public async enable({ id }: { id: string }) { - let apiKeyToInvalidate: string | null = null; + let apiKeyToInvalidate: string | null | undefined = null; let attributes: RawAlert; let version: string | undefined; let references: SavedObjectReference[]; @@ -610,7 +611,7 @@ export class AlertsClient { } public async disable({ id }: { id: string }) { - let apiKeyToInvalidate: string | null = null; + let apiKeyToInvalidate: string | null | undefined = null; let attributes: RawAlert; let version: string | undefined; let references: SavedObjectReference[]; @@ -646,10 +647,12 @@ export class AlertsClient { 'alert', id, { - ...attributes, - scheduledTaskId: undefined, - apiKey: null, - apiKeyOwner: null, + ...(omit( + attributes, + 'scheduledTaskId', + 'apiKey', + 'apiKeyOwner' + ) as AlertUpdateRequiredFields), enabled: false, updatedBy: await this.getUserName(), }, diff --git a/x-pack/plugins/alerts/server/types.ts b/x-pack/plugins/alerts/server/types.ts index 71ab35f7f434b6..3af5eca3002db1 100644 --- a/x-pack/plugins/alerts/server/types.ts +++ b/x-pack/plugins/alerts/server/types.ts @@ -109,13 +109,31 @@ export interface RawAlert extends SavedObjectAttributes { createdBy: string | null; updatedBy: string | null; createdAt: string; - apiKey: string | null; - apiKeyOwner: string | null; + apiKey?: string | null; + apiKeyOwner?: string | null; throttle: string | null; muteAll: boolean; mutedInstanceIds: string[]; } +export type AlertUpdateRequiredFields = Pick< + RawAlert, + | 'enabled' + | 'name' + | 'tags' + | 'alertTypeId' + | 'consumer' + | 'schedule' + | 'actions' + | 'params' + | 'createdBy' + | 'updatedBy' + | 'createdAt' + | 'throttle' + | 'muteAll' + | 'mutedInstanceIds' +>; + export type AlertInfoParams = Pick< RawAlert, | 'params' From a88eba66d2114b52a60269961ea9a36c09e815a7 Mon Sep 17 00:00:00 2001 From: Gidi Meir Morris Date: Thu, 13 Aug 2020 15:37:53 +0100 Subject: [PATCH 16/41] fixed typing broken by making apiKey an optional field --- .../server/task_runner/create_execution_handler.ts | 6 +++--- .../plugins/alerts/server/task_runner/task_runner.ts | 12 ++++++++---- 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/x-pack/plugins/alerts/server/task_runner/create_execution_handler.ts b/x-pack/plugins/alerts/server/task_runner/create_execution_handler.ts index c21d81779e5e0d..b2104ff1a1417b 100644 --- a/x-pack/plugins/alerts/server/task_runner/create_execution_handler.ts +++ b/x-pack/plugins/alerts/server/task_runner/create_execution_handler.ts @@ -5,7 +5,7 @@ */ import { map } from 'lodash'; -import { AlertAction, State, Context, AlertType, AlertParams } from '../types'; +import { AlertAction, State, Context, AlertType, AlertParams, RawAlert } from '../types'; import { Logger, KibanaRequest } from '../../../../../src/core/server'; import { transformActionParams } from './transform_action_params'; import { PluginStartContract as ActionsPluginStartContract } from '../../../actions/server'; @@ -19,7 +19,7 @@ interface CreateExecutionHandlerOptions { actionsPlugin: ActionsPluginStartContract; actions: AlertAction[]; spaceId: string; - apiKey: string | null; + apiKey: RawAlert['apiKey']; alertType: AlertType; logger: Logger; eventLogger: IEventLogger; @@ -90,7 +90,7 @@ export function createExecutionHandler({ id: action.id, params: action.params, spaceId, - apiKey, + apiKey: apiKey ?? null, }); const namespace = spaceId === 'default' ? {} : { namespace: spaceId }; diff --git a/x-pack/plugins/alerts/server/task_runner/task_runner.ts b/x-pack/plugins/alerts/server/task_runner/task_runner.ts index 04fea58f250a33..e17ae5a3854ef4 100644 --- a/x-pack/plugins/alerts/server/task_runner/task_runner.ts +++ b/x-pack/plugins/alerts/server/task_runner/task_runner.ts @@ -73,7 +73,7 @@ export class TaskRunner { return apiKey; } - private getFakeKibanaRequest(spaceId: string, apiKey: string | null) { + private getFakeKibanaRequest(spaceId: string, apiKey: RawAlert['apiKey']) { const requestHeaders: Record = {}; if (apiKey) { @@ -98,7 +98,7 @@ export class TaskRunner { private getServicesWithSpaceLevelPermissions( spaceId: string, - apiKey: string | null + apiKey: RawAlert['apiKey'] ): [Services, PublicMethodsOf] { const request = this.getFakeKibanaRequest(spaceId, apiKey); return [this.context.getServices(request), this.context.getAlertsClientWithRequest(request)]; @@ -109,7 +109,7 @@ export class TaskRunner { alertName: string, tags: string[] | undefined, spaceId: string, - apiKey: string | null, + apiKey: RawAlert['apiKey'], actions: Alert['actions'], alertParams: RawAlert['params'] ) { @@ -250,7 +250,11 @@ export class TaskRunner { }; } - async validateAndExecuteAlert(services: Services, apiKey: string | null, alert: SanitizedAlert) { + async validateAndExecuteAlert( + services: Services, + apiKey: RawAlert['apiKey'], + alert: SanitizedAlert + ) { const { params: { alertId, spaceId }, } = this.taskInstance; From 6a2f3ca7a7f3e3ca00b1aa0fcd1be22763cc0acc Mon Sep 17 00:00:00 2001 From: Gidi Meir Morris Date: Fri, 14 Aug 2020 09:34:32 +0100 Subject: [PATCH 17/41] skip endpoint test to ensur eeverything else passes --- x-pack/test/security_solution_endpoint/apps/endpoint/index.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/test/security_solution_endpoint/apps/endpoint/index.ts b/x-pack/test/security_solution_endpoint/apps/endpoint/index.ts index ad1980cd7218b8..2d235c4f2ce28a 100644 --- a/x-pack/test/security_solution_endpoint/apps/endpoint/index.ts +++ b/x-pack/test/security_solution_endpoint/apps/endpoint/index.ts @@ -13,7 +13,7 @@ import { export default function (providerContext: FtrProviderContext) { const { loadTestFile, getService } = providerContext; - describe('endpoint', function () { + describe.skip('endpoint', function () { this.tags('ciGroup7'); const ingestManager = getService('ingestManager'); const log = getService('log'); From 15605492ba10005e5572bba057f1d1d078413eca Mon Sep 17 00:00:00 2001 From: Gidi Meir Morris Date: Fri, 14 Aug 2020 09:35:02 +0100 Subject: [PATCH 18/41] Revert "skip endpoint test to ensur eeverything else passes" This reverts commit 6a2f3ca7a7f3e3ca00b1aa0fcd1be22763cc0acc. --- x-pack/test/security_solution_endpoint/apps/endpoint/index.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/test/security_solution_endpoint/apps/endpoint/index.ts b/x-pack/test/security_solution_endpoint/apps/endpoint/index.ts index 2d235c4f2ce28a..ad1980cd7218b8 100644 --- a/x-pack/test/security_solution_endpoint/apps/endpoint/index.ts +++ b/x-pack/test/security_solution_endpoint/apps/endpoint/index.ts @@ -13,7 +13,7 @@ import { export default function (providerContext: FtrProviderContext) { const { loadTestFile, getService } = providerContext; - describe.skip('endpoint', function () { + describe('endpoint', function () { this.tags('ciGroup7'); const ingestManager = getService('ingestManager'); const log = getService('log'); From 640c0b2efbac00a0b849590cbed69ae12f768592 Mon Sep 17 00:00:00 2001 From: Gidi Meir Morris Date: Fri, 14 Aug 2020 16:01:31 +0100 Subject: [PATCH 19/41] revert change in ESO and prohibit partial updates in alert and action clients --- .../actions/server/actions_client.test.ts | 22 +- .../plugins/actions/server/actions_client.ts | 11 +- .../actions/server/create_execute_function.ts | 10 +- .../actions/server/saved_objects/index.ts | 2 + .../saved_objects_client_without_updates.ts | 21 ++ x-pack/plugins/alerts/server/alerts_client.ts | 120 +++++-- .../alerts/server/saved_objects/index.ts | 2 +- ..._objects_client_without_partial_updates.ts | 42 --- .../saved_objects_client_without_updates.ts | 21 ++ ...ypted_saved_objects_client_wrapper.test.ts | 328 ++++++++---------- .../encrypted_saved_objects_client_wrapper.ts | 110 +----- .../api_consumer_plugin/server/index.ts | 29 -- .../tests/encrypted_saved_objects_api.ts | 68 +--- 13 files changed, 330 insertions(+), 456 deletions(-) create mode 100644 x-pack/plugins/actions/server/saved_objects/saved_objects_client_without_updates.ts delete mode 100644 x-pack/plugins/alerts/server/saved_objects/saved_objects_client_without_partial_updates.ts create mode 100644 x-pack/plugins/alerts/server/saved_objects/saved_objects_client_without_updates.ts diff --git a/x-pack/plugins/actions/server/actions_client.test.ts b/x-pack/plugins/actions/server/actions_client.test.ts index 24b41bc197fc1e..c8235157e8c246 100644 --- a/x-pack/plugins/actions/server/actions_client.test.ts +++ b/x-pack/plugins/actions/server/actions_client.test.ts @@ -893,7 +893,7 @@ describe('update()', () => { }, references: [], }); - unsecuredSavedObjectsClient.update.mockResolvedValueOnce({ + unsecuredSavedObjectsClient.create.mockResolvedValueOnce({ id: 'my-action', type: 'action', attributes: { @@ -946,7 +946,7 @@ describe('update()', () => { }, references: [], }); - unsecuredSavedObjectsClient.update.mockResolvedValueOnce({ + unsecuredSavedObjectsClient.create.mockResolvedValueOnce({ id: 'my-action', type: 'action', attributes: { @@ -972,11 +972,10 @@ describe('update()', () => { name: 'my name', config: {}, }); - expect(unsecuredSavedObjectsClient.update).toHaveBeenCalledTimes(1); - expect(unsecuredSavedObjectsClient.update.mock.calls[0]).toMatchInlineSnapshot(` + expect(unsecuredSavedObjectsClient.create).toHaveBeenCalledTimes(1); + expect(unsecuredSavedObjectsClient.create.mock.calls[0]).toMatchInlineSnapshot(` Array [ "action", - "my-action", Object { "actionTypeId": "my-action-type", "config": Object {}, @@ -984,6 +983,8 @@ describe('update()', () => { "secrets": Object {}, }, Object { + "id": "my-action", + "overwrite": true, "references": Array [], }, ] @@ -1046,7 +1047,7 @@ describe('update()', () => { }, references: [], }); - unsecuredSavedObjectsClient.update.mockResolvedValueOnce({ + unsecuredSavedObjectsClient.create.mockResolvedValueOnce({ id: 'my-action', type: 'action', attributes: { @@ -1084,11 +1085,10 @@ describe('update()', () => { c: true, }, }); - expect(unsecuredSavedObjectsClient.update).toHaveBeenCalledTimes(1); - expect(unsecuredSavedObjectsClient.update.mock.calls[0]).toMatchInlineSnapshot(` + expect(unsecuredSavedObjectsClient.create).toHaveBeenCalledTimes(1); + expect(unsecuredSavedObjectsClient.create.mock.calls[0]).toMatchInlineSnapshot(` Array [ "action", - "my-action", Object { "actionTypeId": "my-action-type", "config": Object { @@ -1100,6 +1100,8 @@ describe('update()', () => { "secrets": Object {}, }, Object { + "id": "my-action", + "overwrite": true, "references": Array [], }, ] @@ -1124,7 +1126,7 @@ describe('update()', () => { }, references: [], }); - unsecuredSavedObjectsClient.update.mockResolvedValueOnce({ + unsecuredSavedObjectsClient.create.mockResolvedValueOnce({ id: 'my-action', type: 'action', attributes: { diff --git a/x-pack/plugins/actions/server/actions_client.ts b/x-pack/plugins/actions/server/actions_client.ts index b62ca500edccaf..79f14002be0c00 100644 --- a/x-pack/plugins/actions/server/actions_client.ts +++ b/x-pack/plugins/actions/server/actions_client.ts @@ -6,7 +6,6 @@ import Boom from 'boom'; import { ILegacyScopedClusterClient, - SavedObjectsClientContract, SavedObjectAttributes, SavedObject, KibanaRequest, @@ -31,6 +30,7 @@ import { } from './create_execute_function'; import { ActionsAuthorization } from './authorization/actions_authorization'; import { ActionType } from '../common'; +import { SavedObjectsClientWithoutUpdates } from './saved_objects'; // We are assuming there won't be many actions. This is why we will load // all the actions in advance and assume the total count to not go over 10000. @@ -55,7 +55,7 @@ interface ConstructorOptions { defaultKibanaIndex: string; scopedClusterClient: ILegacyScopedClusterClient; actionTypeRegistry: ActionTypeRegistry; - unsecuredSavedObjectsClient: SavedObjectsClientContract; + unsecuredSavedObjectsClient: SavedObjectsClientWithoutUpdates; preconfiguredActions: PreConfiguredAction[]; actionExecutor: ActionExecutorContract; executionEnqueuer: ExecutionEnqueuer; @@ -71,7 +71,7 @@ interface UpdateOptions { export class ActionsClient { private readonly defaultKibanaIndex: string; private readonly scopedClusterClient: ILegacyScopedClusterClient; - private readonly unsecuredSavedObjectsClient: SavedObjectsClientContract; + private readonly unsecuredSavedObjectsClient: SavedObjectsClientWithoutUpdates; private readonly actionTypeRegistry: ActionTypeRegistry; private readonly preconfiguredActions: PreConfiguredAction[]; private readonly actionExecutor: ActionExecutorContract; @@ -162,9 +162,8 @@ export class ActionsClient { this.actionTypeRegistry.ensureActionTypeEnabled(actionTypeId); - const result = await this.unsecuredSavedObjectsClient.update( + const result = await this.unsecuredSavedObjectsClient.create( 'action', - id, { ...attributes, actionTypeId, @@ -174,6 +173,8 @@ export class ActionsClient { }, omitBy( { + id, + overwrite: true, references, version, }, diff --git a/x-pack/plugins/actions/server/create_execute_function.ts b/x-pack/plugins/actions/server/create_execute_function.ts index 85052eef93e051..048d5a401769d8 100644 --- a/x-pack/plugins/actions/server/create_execute_function.ts +++ b/x-pack/plugins/actions/server/create_execute_function.ts @@ -4,10 +4,12 @@ * you may not use this file except in compliance with the Elastic License. */ -import { SavedObjectsClientContract } from '../../../../src/core/server'; import { TaskManagerStartContract } from '../../task_manager/server'; import { RawAction, ActionTypeRegistryContract, PreConfiguredAction } from './types'; -import { ACTION_TASK_PARAMS_SAVED_OBJECT_TYPE } from './saved_objects'; +import { + ACTION_TASK_PARAMS_SAVED_OBJECT_TYPE, + SavedObjectsClientWithoutUpdates, +} from './saved_objects'; interface CreateExecuteFunctionOptions { taskManager: TaskManagerStartContract; @@ -24,7 +26,7 @@ export interface ExecuteOptions { } export type ExecutionEnqueuer = ( - savedObjectsClient: SavedObjectsClientContract, + savedObjectsClient: SavedObjectsClientWithoutUpdates, options: ExecuteOptions ) => Promise; @@ -35,7 +37,7 @@ export function createExecutionEnqueuerFunction({ preconfiguredActions, }: CreateExecuteFunctionOptions) { return async function execute( - savedObjectsClient: SavedObjectsClientContract, + savedObjectsClient: SavedObjectsClientWithoutUpdates, { id, params, spaceId, apiKey }: ExecuteOptions ) { if (isESOUsingEphemeralEncryptionKey === true) { diff --git a/x-pack/plugins/actions/server/saved_objects/index.ts b/x-pack/plugins/actions/server/saved_objects/index.ts index 54f186acc1ba57..312a7264d986cd 100644 --- a/x-pack/plugins/actions/server/saved_objects/index.ts +++ b/x-pack/plugins/actions/server/saved_objects/index.ts @@ -8,6 +8,8 @@ import { SavedObjectsServiceSetup } from 'kibana/server'; import mappings from './mappings.json'; import { EncryptedSavedObjectsPluginSetup } from '../../../encrypted_saved_objects/server'; +export { SavedObjectsClientWithoutUpdates } from './saved_objects_client_without_updates'; + export const ACTION_SAVED_OBJECT_TYPE = 'action'; export const ACTION_TASK_PARAMS_SAVED_OBJECT_TYPE = 'action_task_params'; diff --git a/x-pack/plugins/actions/server/saved_objects/saved_objects_client_without_updates.ts b/x-pack/plugins/actions/server/saved_objects/saved_objects_client_without_updates.ts new file mode 100644 index 00000000000000..10ac773cb1f77a --- /dev/null +++ b/x-pack/plugins/actions/server/saved_objects/saved_objects_client_without_updates.ts @@ -0,0 +1,21 @@ +/* + * 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 { SavedObjectsClientContract, SavedObjectsCreateOptions, SavedObject } from 'kibana/server'; + +type AlertSavedObjectsCreateOptions = Omit; +type AlertSavedObjectsUpdateOptions = Omit & + Pick, 'id' | 'overwrite'>; + +export type SavedObjectsClientWithoutUpdates = Omit< + SavedObjectsClientContract, + 'create' | 'update' | 'bulkUpdate' +> & { + create( + type: string, + attributes: T, + options?: AlertSavedObjectsCreateOptions | AlertSavedObjectsUpdateOptions + ): Promise>; +}; diff --git a/x-pack/plugins/alerts/server/alerts_client.ts b/x-pack/plugins/alerts/server/alerts_client.ts index 0c0c2eaa990070..7734d089322595 100644 --- a/x-pack/plugins/alerts/server/alerts_client.ts +++ b/x-pack/plugins/alerts/server/alerts_client.ts @@ -37,7 +37,7 @@ import { WriteOperations, ReadOperations, } from './authorization/alerts_authorization'; -import { SavedObjectsClientWithoutPartialUpdates } from './saved_objects'; +import { SavedObjectsClientWithoutUpdates } from './saved_objects'; export interface RegistryAlertTypeWithAuth extends RegistryAlertType { authorizedConsumers: string[]; @@ -53,7 +53,7 @@ export type InvalidateAPIKeyResult = export interface ConstructorOptions { logger: Logger; taskManager: TaskManagerStartContract; - unsecuredSavedObjectsClient: SavedObjectsClientWithoutPartialUpdates; + unsecuredSavedObjectsClient: SavedObjectsClientWithoutUpdates; authorization: AlertsAuthorization; actionsAuthorization: ActionsAuthorization; alertTypeRegistry: AlertTypeRegistry; @@ -135,7 +135,7 @@ export class AlertsClient { private readonly spaceId?: string; private readonly namespace?: string; private readonly taskManager: TaskManagerStartContract; - private readonly unsecuredSavedObjectsClient: SavedObjectsClientWithoutPartialUpdates; + private readonly unsecuredSavedObjectsClient: SavedObjectsClientWithoutUpdates; private readonly authorization: AlertsAuthorization; private readonly alertTypeRegistry: AlertTypeRegistry; private readonly createAPIKey: (name: string) => Promise; @@ -228,15 +228,16 @@ export class AlertsClient { } throw e; } - await this.unsecuredSavedObjectsClient.update( + await this.unsecuredSavedObjectsClient.create( 'alert', - createdAlert.id, { ...createdAlert.attributes, ...encryptedAttributes, scheduledTaskId: scheduledTask.id, }, { + id: createdAlert.id, + overwrite: true, references, } ); @@ -430,9 +431,8 @@ export class AlertsClient { : null; const apiKeyAttributes = this.apiKeyAsAlertAttributes(createdAPIKey, username); - const updatedObject = await this.unsecuredSavedObjectsClient.update( + const updatedObject = await this.unsecuredSavedObjectsClient.create( 'alert', - id, { ...attributes, ...data, @@ -442,6 +442,8 @@ export class AlertsClient { updatedBy: username, }, { + id, + overwrite: true, version, references, } @@ -506,9 +508,8 @@ export class AlertsClient { } const username = await this.getUserName(); - await this.unsecuredSavedObjectsClient.update( + await this.unsecuredSavedObjectsClient.create( 'alert', - id, { ...attributes, ...this.apiKeyAsAlertAttributes( @@ -517,7 +518,15 @@ export class AlertsClient { ), updatedBy: username, }, - omitBy({ version, references }, isUndefined) + omitBy( + { + id, + overwrite: true, + version, + references, + }, + isUndefined + ) ); if (apiKeyToInvalidate) { @@ -588,21 +597,35 @@ export class AlertsClient { ), updatedBy: username, }; - const updatedAlert = await this.unsecuredSavedObjectsClient.update( + const updatedAlert = await this.unsecuredSavedObjectsClient.create( 'alert', - id, updatedAttributes, - omitBy({ version, references }, isUndefined) + omitBy( + { + id, + overwrite: true, + version, + references, + }, + isUndefined + ) ); const scheduledTask = await this.scheduleAlert(id, attributes.alertTypeId); - await this.unsecuredSavedObjectsClient.update( + await this.unsecuredSavedObjectsClient.create( 'alert', - id, { ...updatedAttributes, scheduledTaskId: scheduledTask.id, }, - omitBy({ version: updatedAlert.version, references: updatedAlert.references }, isUndefined) + omitBy( + { + id, + overwrite: true, + version: updatedAlert.version, + references: updatedAlert.references, + }, + isUndefined + ) ); if (apiKeyToInvalidate) { await this.invalidateApiKey({ apiKey: apiKeyToInvalidate }); @@ -643,9 +666,8 @@ export class AlertsClient { ); if (attributes.enabled === true) { - await this.unsecuredSavedObjectsClient.update( + await this.unsecuredSavedObjectsClient.create( 'alert', - id, { ...(omit( attributes, @@ -656,7 +678,15 @@ export class AlertsClient { enabled: false, updatedBy: await this.getUserName(), }, - omitBy({ version, references }, isUndefined) + omitBy( + { + id, + overwrite: true, + version, + references, + }, + isUndefined + ) ); await Promise.all([ @@ -686,16 +716,23 @@ export class AlertsClient { await this.actionsAuthorization.ensureAuthorized('execute'); } - await this.unsecuredSavedObjectsClient.update( + await this.unsecuredSavedObjectsClient.create( 'alert', - id, { ...attributes, muteAll: true, mutedInstanceIds: [], updatedBy: await this.getUserName(), }, - omitBy({ version, references }, isUndefined) + omitBy( + { + id, + overwrite: true, + version, + references, + }, + isUndefined + ) ); } @@ -717,16 +754,23 @@ export class AlertsClient { await this.actionsAuthorization.ensureAuthorized('execute'); } - await this.unsecuredSavedObjectsClient.update( + await this.unsecuredSavedObjectsClient.create( 'alert', - id, { ...attributes, muteAll: false, mutedInstanceIds: [], updatedBy: await this.getUserName(), }, - omitBy({ version, references }, isUndefined) + omitBy( + { + id, + overwrite: true, + version, + references, + }, + isUndefined + ) ); } @@ -756,15 +800,22 @@ export class AlertsClient { const mutedInstanceIds = attributes.mutedInstanceIds || []; if (!attributes.muteAll && !mutedInstanceIds.includes(alertInstanceId)) { mutedInstanceIds.push(alertInstanceId); - await this.unsecuredSavedObjectsClient.update( + await this.unsecuredSavedObjectsClient.create( 'alert', - alertId, { ...attributes, mutedInstanceIds, updatedBy: await this.getUserName(), }, - omitBy({ version, references }, isUndefined) + omitBy( + { + id: alertId, + overwrite: true, + version, + references, + }, + isUndefined + ) ); } } @@ -798,15 +849,22 @@ export class AlertsClient { const mutedInstanceIds = attributes.mutedInstanceIds || []; if (!attributes.muteAll && mutedInstanceIds.includes(alertInstanceId)) { - await this.unsecuredSavedObjectsClient.update( + await this.unsecuredSavedObjectsClient.create( 'alert', - alertId, { ...attributes, updatedBy: await this.getUserName(), mutedInstanceIds: mutedInstanceIds.filter((id: string) => id !== alertInstanceId), }, - omitBy({ version, references }, isUndefined) + omitBy( + { + id: alertId, + overwrite: true, + version, + references, + }, + isUndefined + ) ); } } diff --git a/x-pack/plugins/alerts/server/saved_objects/index.ts b/x-pack/plugins/alerts/server/saved_objects/index.ts index 32f14a666cfb63..652b98d6b0c763 100644 --- a/x-pack/plugins/alerts/server/saved_objects/index.ts +++ b/x-pack/plugins/alerts/server/saved_objects/index.ts @@ -8,7 +8,7 @@ import { SavedObjectsServiceSetup } from 'kibana/server'; import mappings from './mappings.json'; import { EncryptedSavedObjectsPluginSetup } from '../../../encrypted_saved_objects/server'; -export { SavedObjectsClientWithoutPartialUpdates } from './saved_objects_client_without_partial_updates'; +export { SavedObjectsClientWithoutUpdates } from './saved_objects_client_without_updates'; export function setupSavedObjects( savedObjects: SavedObjectsServiceSetup, diff --git a/x-pack/plugins/alerts/server/saved_objects/saved_objects_client_without_partial_updates.ts b/x-pack/plugins/alerts/server/saved_objects/saved_objects_client_without_partial_updates.ts deleted file mode 100644 index 8d603942419d13..00000000000000 --- a/x-pack/plugins/alerts/server/saved_objects/saved_objects_client_without_partial_updates.ts +++ /dev/null @@ -1,42 +0,0 @@ -/* - * 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 { - SavedObjectsClientContract, - SavedObjectsUpdateOptions, - SavedObjectsUpdateResponse, - SavedObjectsBulkUpdateOptions, - SavedObjectsBulkUpdateObject, - SavedObjectsBulkUpdateResponse, -} from 'kibana/server'; - -export interface SavedObjectsBulkUpdateObjectWithoutPartialUpdates - extends SavedObjectsBulkUpdateObject { - attributes: T; -} - -export interface SavedObjectsBulkUpdateResponseWithoutPartialUpdates - extends SavedObjectsBulkUpdateResponse { - saved_objects: Array>; -} - -export interface SavedObjectsUpdateResponseWithoutPartialUpdates - extends SavedObjectsUpdateResponse { - attributes: T; -} - -export interface SavedObjectsClientWithoutPartialUpdates extends SavedObjectsClientContract { - update( - type: string, - id: string, - attributes: T, - options: SavedObjectsUpdateOptions - ): Promise>; - - bulkUpdate( - objects: Array>, - options?: SavedObjectsBulkUpdateOptions - ): Promise>; -} diff --git a/x-pack/plugins/alerts/server/saved_objects/saved_objects_client_without_updates.ts b/x-pack/plugins/alerts/server/saved_objects/saved_objects_client_without_updates.ts new file mode 100644 index 00000000000000..0c3ecf7e3c78e3 --- /dev/null +++ b/x-pack/plugins/alerts/server/saved_objects/saved_objects_client_without_updates.ts @@ -0,0 +1,21 @@ +/* + * 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 { SavedObjectsClientContract, SavedObjectsCreateOptions } from 'kibana/server'; + +type AlertSavedObjectsCreateOptions = Omit; +type AlertSavedObjectsUpdateOptions = Omit & + Pick, 'id' | 'overwrite'>; + +export type SavedObjectsClientWithoutUpdates = Omit< + SavedObjectsClientContract, + 'create' | 'update' | 'bulkUpdate' +> & { + create( + type: string, + attributes: T, + options?: AlertSavedObjectsCreateOptions | AlertSavedObjectsUpdateOptions + ): Promise>; +}; diff --git a/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.test.ts b/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.test.ts index 3c0f2f1f8d2c82..b704aaaba03433 100644 --- a/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.test.ts +++ b/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.test.ts @@ -69,6 +69,58 @@ describe('#create', () => { expect(mockBaseClient.create).not.toHaveBeenCalled(); }); + it('allows a specified ID when overwriting an existing object', async () => { + const attributes = { + attrOne: 'one', + attrSecret: 'secret', + attrNotSoSecret: 'not-so-secret', + attrThree: 'three', + }; + const options = { id: 'predefined-uuid', overwrite: true }; + const mockedResponse = { + id: 'predefined-uuid', + type: 'known-type', + attributes: { + attrOne: 'one', + attrSecret: '*secret*', + attrNotSoSecret: '*not-so-secret*', + attrThree: 'three', + }, + references: [], + }; + + mockBaseClient.create.mockResolvedValue(mockedResponse); + + expect(await wrapper.create('known-type', attributes, options)).toEqual({ + ...mockedResponse, + attributes: { attrOne: 'one', attrNotSoSecret: 'not-so-secret', attrThree: 'three' }, + }); + + expect(encryptedSavedObjectsServiceMockInstance.encryptAttributes).toHaveBeenCalledTimes(1); + expect(encryptedSavedObjectsServiceMockInstance.encryptAttributes).toHaveBeenCalledWith( + { type: 'known-type', id: 'predefined-uuid' }, + { + attrOne: 'one', + attrSecret: 'secret', + attrNotSoSecret: 'not-so-secret', + attrThree: 'three', + }, + { user: mockAuthenticatedUser() } + ); + + expect(mockBaseClient.create).toHaveBeenCalledTimes(1); + expect(mockBaseClient.create).toHaveBeenCalledWith( + 'known-type', + { + attrOne: 'one', + attrSecret: '*secret*', + attrNotSoSecret: '*not-so-secret*', + attrThree: 'three', + }, + { id: 'predefined-uuid', overwrite: true } + ); + }); + it('generates ID, encrypts attributes and strips them from response except for ones with `dangerouslyExposeValue` set to `true`', async () => { const attributes = { attrOne: 'one', @@ -249,6 +301,77 @@ describe('#bulkCreate', () => { expect(mockBaseClient.bulkCreate).not.toHaveBeenCalled(); }); + it('allows a specified ID when overwriting an existing object', async () => { + const attributes = { + attrOne: 'one', + attrSecret: 'secret', + attrNotSoSecret: 'not-so-secret', + attrThree: 'three', + }; + const mockedResponse = { + saved_objects: [ + { + id: 'predefined-uuid', + type: 'known-type', + attributes: { ...attributes, attrSecret: '*secret*', attrNotSoSecret: '*not-so-secret*' }, + references: [], + }, + { + id: 'some-id', + type: 'unknown-type', + attributes, + references: [], + }, + ], + }; + + mockBaseClient.bulkCreate.mockResolvedValue(mockedResponse); + + const bulkCreateParams = [ + { id: 'predefined-uuid', type: 'known-type', attributes }, + { type: 'unknown-type', attributes }, + ]; + + await expect(wrapper.bulkCreate(bulkCreateParams, { overwrite: true })).resolves.toEqual({ + saved_objects: [ + { + ...mockedResponse.saved_objects[0], + attributes: { attrOne: 'one', attrNotSoSecret: 'not-so-secret', attrThree: 'three' }, + }, + mockedResponse.saved_objects[1], + ], + }); + + expect(encryptedSavedObjectsServiceMockInstance.encryptAttributes).toHaveBeenCalledTimes(1); + expect(encryptedSavedObjectsServiceMockInstance.encryptAttributes).toHaveBeenCalledWith( + { type: 'known-type', id: 'predefined-uuid' }, + { + attrOne: 'one', + attrSecret: 'secret', + attrNotSoSecret: 'not-so-secret', + attrThree: 'three', + }, + { user: mockAuthenticatedUser() } + ); + + expect(mockBaseClient.bulkCreate).toHaveBeenCalledTimes(1); + expect(mockBaseClient.bulkCreate).toHaveBeenCalledWith( + [ + { + ...bulkCreateParams[0], + attributes: { + attrOne: 'one', + attrSecret: '*secret*', + attrNotSoSecret: '*not-so-secret*', + attrThree: 'three', + }, + }, + bulkCreateParams[1], + ], + { overwrite: true } + ); + }); + it('generates ID, encrypts attributes and strips them from response except for ones with `dangerouslyExposeValue` set to `true`', async () => { const attributes = { attrOne: 'one', @@ -457,11 +580,11 @@ describe('#bulkUpdate', () => { attrSecret: `*${doc.attributes.attrSecret}*`, attrNotSoSecret: `*${doc.attributes.attrNotSoSecret}*`, }, - references: [], + references: undefined, })), }; - mockBaseClient.bulkCreate.mockResolvedValue(mockedResponse); + mockBaseClient.bulkUpdate.mockResolvedValue(mockedResponse); await expect( wrapper.bulkUpdate( @@ -478,7 +601,6 @@ describe('#bulkUpdate', () => { attrNotSoSecret: 'not-so-secret', attrThree: 'three', }, - references: [], }, { id: 'some-id-2', @@ -488,7 +610,6 @@ describe('#bulkUpdate', () => { attrNotSoSecret: 'not-so-secret 2', attrThree: 'three 2', }, - references: [], }, ], }); @@ -515,8 +636,8 @@ describe('#bulkUpdate', () => { { user: mockAuthenticatedUser() } ); - expect(mockBaseClient.bulkCreate).toHaveBeenCalledTimes(1); - expect(mockBaseClient.bulkCreate).toHaveBeenCalledWith( + expect(mockBaseClient.bulkUpdate).toHaveBeenCalledTimes(1); + expect(mockBaseClient.bulkUpdate).toHaveBeenCalledWith( [ { id: 'some-id', @@ -539,160 +660,6 @@ describe('#bulkUpdate', () => { }, }, ], - { overwrite: true } - ); - }); - - it('handles mixed encrypted and non encrypted objects', async () => { - const encryptedDocs = [ - { - id: 'some-encrypted-id', - type: 'known-type', - attributes: { - attrOne: 'one', - attrSecret: 'secret', - attrNotSoSecret: 'not-so-secret', - attrThree: 'three', - }, - }, - { - id: 'some-encrypted-id-2', - type: 'known-type', - attributes: { - attrOne: 'one 2', - attrSecret: 'secret 2', - attrNotSoSecret: 'not-so-secret 2', - attrThree: 'three 2', - }, - }, - ]; - - mockBaseClient.bulkCreate.mockResolvedValue({ - saved_objects: encryptedDocs.map((doc) => ({ - ...doc, - attributes: { - ...doc.attributes, - attrSecret: `*${doc.attributes.attrSecret}*`, - attrNotSoSecret: `*${doc.attributes.attrNotSoSecret}*`, - }, - references: [], - })), - }); - - const nonEncyptedDoc = { - id: 'some-id', - type: 'unknown-type', - attributes: { attrOne: 'one', attrSecret: 'secret', attrThree: 'three' }, - references: [], - }; - mockBaseClient.bulkUpdate.mockResolvedValue({ - saved_objects: [nonEncyptedDoc], - }); - - await expect( - wrapper.bulkUpdate( - [ - { ...encryptedDocs[0] }, - { - type: 'unknown-type', - id: 'some-id', - attributes: nonEncyptedDoc.attributes, - version: 'some-version', - }, - { ...encryptedDocs[1] }, - ], - {} - ) - ).resolves.toEqual({ - saved_objects: [ - { - id: 'some-encrypted-id', - type: 'known-type', - attributes: { - attrOne: 'one', - attrNotSoSecret: 'not-so-secret', - attrThree: 'three', - }, - references: [], - }, - { - id: 'some-id', - type: 'unknown-type', - attributes: nonEncyptedDoc.attributes, - references: [], - }, - { - id: 'some-encrypted-id-2', - type: 'known-type', - attributes: { - attrOne: 'one 2', - attrNotSoSecret: 'not-so-secret 2', - attrThree: 'three 2', - }, - references: [], - }, - ], - }); - - expect(encryptedSavedObjectsServiceMockInstance.encryptAttributes).toHaveBeenCalledTimes(2); - expect(encryptedSavedObjectsServiceMockInstance.encryptAttributes).toHaveBeenCalledWith( - { type: 'known-type', id: 'some-encrypted-id' }, - { - attrOne: 'one', - attrSecret: 'secret', - attrNotSoSecret: 'not-so-secret', - attrThree: 'three', - }, - { user: mockAuthenticatedUser() } - ); - expect(encryptedSavedObjectsServiceMockInstance.encryptAttributes).toHaveBeenCalledWith( - { type: 'known-type', id: 'some-encrypted-id-2' }, - { - attrOne: 'one 2', - attrSecret: 'secret 2', - attrNotSoSecret: 'not-so-secret 2', - attrThree: 'three 2', - }, - { user: mockAuthenticatedUser() } - ); - - expect(mockBaseClient.bulkCreate).toHaveBeenCalledTimes(1); - expect(mockBaseClient.bulkCreate).toHaveBeenCalledWith( - [ - { - id: 'some-encrypted-id', - type: 'known-type', - attributes: { - attrOne: 'one', - attrSecret: '*secret*', - attrNotSoSecret: '*not-so-secret*', - attrThree: 'three', - }, - }, - { - id: 'some-encrypted-id-2', - type: 'known-type', - attributes: { - attrOne: 'one 2', - attrSecret: '*secret 2*', - attrNotSoSecret: '*not-so-secret 2*', - attrThree: 'three 2', - }, - }, - ], - { overwrite: true } - ); - - expect(mockBaseClient.bulkUpdate).toHaveBeenCalledTimes(1); - expect(mockBaseClient.bulkUpdate).toHaveBeenCalledWith( - [ - { - type: 'unknown-type', - id: 'some-id', - attributes: nonEncyptedDoc.attributes, - version: 'some-version', - }, - ], {} ); }); @@ -713,8 +680,8 @@ describe('#bulkUpdate', () => { ]; const options = { namespace }; - mockBaseClient.bulkCreate.mockResolvedValue({ - saved_objects: docs.map((doc) => ({ ...doc, references: [] })), + mockBaseClient.bulkUpdate.mockResolvedValue({ + saved_objects: docs.map((doc) => ({ ...doc, references: undefined })), }); await expect(wrapper.bulkUpdate(docs, options)).resolves.toEqual({ @@ -727,7 +694,7 @@ describe('#bulkUpdate', () => { attrThree: 'three', }, version: 'some-version', - references: [], + references: undefined, }, ], }); @@ -743,8 +710,8 @@ describe('#bulkUpdate', () => { { user: mockAuthenticatedUser() } ); - expect(mockBaseClient.bulkCreate).toHaveBeenCalledTimes(1); - expect(mockBaseClient.bulkCreate).toHaveBeenCalledWith( + expect(mockBaseClient.bulkUpdate).toHaveBeenCalledTimes(1); + expect(mockBaseClient.bulkUpdate).toHaveBeenCalledWith( [ { id: 'some-id', @@ -759,7 +726,7 @@ describe('#bulkUpdate', () => { references: undefined, }, ], - { ...options, overwrite: true } + options ); }; @@ -1450,7 +1417,7 @@ describe('#update', () => { references: [], }; - mockBaseClient.create.mockResolvedValue(mockedResponse); + mockBaseClient.update.mockResolvedValue(mockedResponse); await expect(wrapper.update('known-type', 'some-id', attributes, options)).resolves.toEqual({ ...mockedResponse, @@ -1469,17 +1436,17 @@ describe('#update', () => { { user: mockAuthenticatedUser() } ); - expect(mockBaseClient.create).toHaveBeenCalledTimes(1); - expect(mockBaseClient.create).toHaveBeenCalledWith( + expect(mockBaseClient.update).toHaveBeenCalledTimes(1); + expect(mockBaseClient.update).toHaveBeenCalledWith( 'known-type', - + 'some-id', { attrOne: 'one', attrSecret: '*secret*', attrNotSoSecret: '*not-so-secret*', attrThree: 'three', }, - { id: 'some-id', overwrite: true, ...options } + options ); }); @@ -1489,7 +1456,7 @@ describe('#update', () => { const options = { version: 'some-version', namespace }; const mockedResponse = { id: 'some-id', type: 'known-type', attributes, references: [] }; - mockBaseClient.create.mockResolvedValue(mockedResponse); + mockBaseClient.update.mockResolvedValue(mockedResponse); await expect(wrapper.update('known-type', 'some-id', attributes, options)).resolves.toEqual({ ...mockedResponse, @@ -1507,12 +1474,12 @@ describe('#update', () => { { user: mockAuthenticatedUser() } ); - expect(mockBaseClient.create).toHaveBeenCalledTimes(1); - expect(mockBaseClient.create).toHaveBeenCalledWith( + expect(mockBaseClient.update).toHaveBeenCalledTimes(1); + expect(mockBaseClient.update).toHaveBeenCalledWith( 'known-type', - + 'some-id', { attrOne: 'one', attrSecret: '*secret*', attrThree: 'three' }, - { id: 'some-id', overwrite: true, ...options } + options ); }; @@ -1528,7 +1495,7 @@ describe('#update', () => { it('fails if base client fails', async () => { const failureReason = new Error('Something bad happened...'); - mockBaseClient.create.mockRejectedValue(failureReason); + mockBaseClient.update.mockRejectedValue(failureReason); await expect( wrapper.update('known-type', 'some-id', { @@ -1538,11 +1505,12 @@ describe('#update', () => { }) ).rejects.toThrowError(failureReason); - expect(mockBaseClient.create).toHaveBeenCalledTimes(1); - expect(mockBaseClient.create).toHaveBeenCalledWith( + expect(mockBaseClient.update).toHaveBeenCalledTimes(1); + expect(mockBaseClient.update).toHaveBeenCalledWith( 'known-type', + 'some-id', { attrOne: 'one', attrSecret: '*secret*', attrThree: 'three' }, - { id: 'some-id', overwrite: true } + undefined ); }); }); diff --git a/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.ts b/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.ts index 8573d34ea0f21f..ea4b0ea0a96e64 100644 --- a/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.ts +++ b/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.ts @@ -60,13 +60,13 @@ export class EncryptedSavedObjectsClientWrapper implements SavedObjectsClientCon // Saved objects with encrypted attributes should have IDs that are hard to guess especially // since IDs are part of the AAD used during encryption, that's why we control them within this // wrapper and don't allow consumers to specify their own IDs directly. - if (options.id) { + if (options.id && !options.overwrite) { throw new Error( 'Predefined IDs are not allowed for saved objects with encrypted attributes.' ); } - const id = generateID(); + const id = options.id ?? generateID(); const namespace = getDescriptorNamespace( this.options.baseTypeRegistry, type, @@ -89,7 +89,7 @@ export class EncryptedSavedObjectsClientWrapper implements SavedObjectsClientCon public async bulkCreate( objects: Array>, - options?: SavedObjectsBaseOptions + options?: SavedObjectsBaseOptions & Pick ) { // We encrypt attributes for every object in parallel and that can potentially exhaust libuv or // NodeJS thread pool. If it turns out to be a problem, we can consider switching to the @@ -103,13 +103,13 @@ export class EncryptedSavedObjectsClientWrapper implements SavedObjectsClientCon // Saved objects with encrypted attributes should have IDs that are hard to guess especially // since IDs are part of the AAD used during encryption, that's why we control them within this // wrapper and don't allow consumers to specify their own IDs directly. - if (object.id) { + if (object.id && !options?.overwrite) { throw new Error( 'Predefined IDs are not allowed for saved objects with encrypted attributes.' ); } - const id = generateID(); + const id = object?.id ?? generateID(); const namespace = getDescriptorNamespace( this.options.baseTypeRegistry, object.type, @@ -137,41 +137,15 @@ export class EncryptedSavedObjectsClientWrapper implements SavedObjectsClientCon objects: Array>, options?: SavedObjectsBaseOptions ) { - const { - nonEncryptedObjects, - nonEncryptedObjectsIndicies, - encryptableObjects, - encryptableObjectsIndicies, - } = objects.reduce<{ - nonEncryptedObjects: Array>; - encryptableObjects: Array>; - nonEncryptedObjectsIndicies: number[]; - encryptableObjectsIndicies: number[]; - }>( - (partition, object, index) => { - if (this.options.service.isRegistered(object.type)) { - partition.encryptableObjects.push(object); - partition.encryptableObjectsIndicies.push(index); - } else { - partition.nonEncryptedObjects.push(object); - partition.nonEncryptedObjectsIndicies.push(index); - } - return partition; - }, - { - nonEncryptedObjects: [], - encryptableObjects: [], - nonEncryptedObjectsIndicies: [], - encryptableObjectsIndicies: [], - } - ); - // We encrypt attributes for every object in parallel and that can potentially exhaust libuv or // NodeJS thread pool. If it turns out to be a problem, we can consider switching to the // sequential processing. const encryptedObjects = await Promise.all( - encryptableObjects.map(async (object) => { + objects.map(async (object) => { const { type, id, attributes } = object; + if (!this.options.service.isRegistered(type)) { + return object; + } const namespace = getDescriptorNamespace( this.options.baseTypeRegistry, type, @@ -189,38 +163,7 @@ export class EncryptedSavedObjectsClientWrapper implements SavedObjectsClientCon ); return await this.handleEncryptedAttributesInBulkResponse( - { - saved_objects: [ - // match up each object in the response with - // the index it had in the original input. - // if the object didn't have an index then assign - // the max possible index so it'll get pushed to the bottom - ...this.zip( - encryptedObjects.length - ? ( - await this.options.baseClient.bulkCreate(encryptedObjects, { - ...options, - overwrite: true, - }) - ).saved_objects - : [], - encryptableObjectsIndicies, - Number.MAX_VALUE - ), - ...this.zip( - nonEncryptedObjects.length - ? (await this.options.baseClient.bulkUpdate(nonEncryptedObjects, options)) - .saved_objects - : [], - nonEncryptedObjectsIndicies, - Number.MAX_VALUE - ), - ] - // sort by the index in the original input - .sort(([, leftIndex], [, rightIndex]) => leftIndex - rightIndex) - // drop the index now that we've sorted - .map(([object]) => object), - }, + await this.options.baseClient.bulkUpdate(encryptedObjects, options), objects ); } @@ -257,7 +200,7 @@ export class EncryptedSavedObjectsClientWrapper implements SavedObjectsClientCon public async update( type: string, id: string, - attributes: T, + attributes: Partial, options?: SavedObjectsUpdateOptions ) { if (!this.options.service.isRegistered(type)) { @@ -269,20 +212,13 @@ export class EncryptedSavedObjectsClientWrapper implements SavedObjectsClientCon options?.namespace ); return this.handleEncryptedAttributesInResponse( - await this.options.baseClient.create( + await this.options.baseClient.update( type, - (await this.options.service.encryptAttributes( - { type, id, namespace }, - attributes as Record, - { - user: this.options.getCurrentUser(), - } - )) as T, - { - ...options, - id, - overwrite: true, - } + id, + await this.options.service.encryptAttributes({ type, id, namespace }, attributes, { + user: this.options.getCurrentUser(), + }), + options ), attributes, namespace @@ -366,16 +302,4 @@ export class EncryptedSavedObjectsClientWrapper implements SavedObjectsClientCon return response; } - - /** - * Zip two arrays together so each cell contains a tupple of two item, one from each array in that index. - * All items from the left array will be retained, and only as many items as needed will be pulled from right - * @param left array of items of any type - * @param right array of items of any type - * @param defaultZipValue default value to use when pulling from the left array and - * a corresponding item is missing in the right array - */ - private zip(left: T[], right: G[], defaultZipValue: G) { - return left.map<[T, G]>((leftItem, index) => [leftItem, right[index] ?? defaultZipValue]); - } } diff --git a/x-pack/test/encrypted_saved_objects_api_integration/fixtures/api_consumer_plugin/server/index.ts b/x-pack/test/encrypted_saved_objects_api_integration/fixtures/api_consumer_plugin/server/index.ts index 0c40b795b5cc91..87bed7f4160191 100644 --- a/x-pack/test/encrypted_saved_objects_api_integration/fixtures/api_consumer_plugin/server/index.ts +++ b/x-pack/test/encrypted_saved_objects_api_integration/fixtures/api_consumer_plugin/server/index.ts @@ -62,10 +62,6 @@ export const plugin: PluginInitializer = publicPropertyExcludedFromAAD: { type: 'keyword' }, publicPropertyStoredEncrypted: { type: 'binary' }, privateProperty: { type: 'binary' }, - publicDeepProperty: { - enabled: false, - type: 'object', - }, }, }), }); @@ -116,31 +112,6 @@ export const plugin: PluginInitializer = } ); - for (const route of ['saved_objects', 'hidden_saved_objects']) { - router.put( - { - path: `/api/${route}/update-with-partial/{type}/{id}`, - validate: { params: (value) => ({ value }), body: (value) => ({ value }) }, - }, - async (context, request, response) => { - const [{ savedObjects }] = await core.getStartServices(); - const { type, id } = request.params; - const { attributes } = request.body as { attributes: any }; - - return response.ok({ - body: await savedObjects - .getScopedClient(request, { - includedHiddenTypes: [HIDDEN_SAVED_OBJECT_WITH_SECRET_TYPE], - excludedWrappers: ['encryptedSavedObjects'], - }) - .update(type, id, attributes, { - refresh: true, - }), - }); - } - ); - } - registerHiddenSORoutes(router, core, deps, [HIDDEN_SAVED_OBJECT_WITH_SECRET_TYPE]); }, start() {}, diff --git a/x-pack/test/encrypted_saved_objects_api_integration/tests/encrypted_saved_objects_api.ts b/x-pack/test/encrypted_saved_objects_api_integration/tests/encrypted_saved_objects_api.ts index 7c13cd646cb352..8bdc1715bf487b 100644 --- a/x-pack/test/encrypted_saved_objects_api_integration/tests/encrypted_saved_objects_api.ts +++ b/x-pack/test/encrypted_saved_objects_api_integration/tests/encrypted_saved_objects_api.ts @@ -41,7 +41,6 @@ export default function ({ getService }: FtrProviderContext) { publicPropertyStoredEncrypted: string; privateProperty: string; publicPropertyExcludedFromAAD: string; - publicDeepProperty: Record; }; let savedObject: SavedObject; @@ -51,7 +50,6 @@ export default function ({ getService }: FtrProviderContext) { publicPropertyStoredEncrypted: randomness.string(), privateProperty: randomness.string(), publicPropertyExcludedFromAAD: randomness.string(), - publicDeepProperty: { [randomness.string()]: randomness.string() }, }; const { body } = await supertest @@ -68,7 +66,6 @@ export default function ({ getService }: FtrProviderContext) { publicProperty: savedObjectOriginalAttributes.publicProperty, publicPropertyExcludedFromAAD: savedObjectOriginalAttributes.publicPropertyExcludedFromAAD, publicPropertyStoredEncrypted: savedObjectOriginalAttributes.publicPropertyStoredEncrypted, - publicDeepProperty: savedObjectOriginalAttributes.publicDeepProperty, }); const rawAttributes = await getRawSavedObjectAttributes(savedObject); @@ -76,9 +73,6 @@ export default function ({ getService }: FtrProviderContext) { expect(rawAttributes.publicPropertyExcludedFromAAD).to.be( savedObjectOriginalAttributes.publicPropertyExcludedFromAAD ); - expect(rawAttributes.publicDeepProperty).to.eql( - savedObjectOriginalAttributes.publicDeepProperty - ); expect(rawAttributes.publicPropertyStoredEncrypted).to.not.be.empty(); expect(rawAttributes.publicPropertyStoredEncrypted).to.not.be( @@ -214,7 +208,6 @@ export default function ({ getService }: FtrProviderContext) { publicProperty: savedObjectOriginalAttributes.publicProperty, publicPropertyExcludedFromAAD: savedObjectOriginalAttributes.publicPropertyExcludedFromAAD, publicPropertyStoredEncrypted: savedObjectOriginalAttributes.publicPropertyStoredEncrypted, - publicDeepProperty: savedObjectOriginalAttributes.publicDeepProperty, }); expect(response.error).to.be(undefined); }); @@ -224,13 +217,9 @@ export default function ({ getService }: FtrProviderContext) { // encrypted attributes. const updatedPublicProperty = randomness.string(); await supertest - .put( - `${getURLAPIBaseURL()}update-with-partial/${encryptedSavedObjectType}/${savedObject.id}` - ) + .put(`${getURLAPIBaseURL()}${encryptedSavedObjectType}/${savedObject.id}`) .set('kbn-xsrf', 'xxx') - .send({ - attributes: { publicProperty: updatedPublicProperty }, - }) + .send({ attributes: { publicProperty: updatedPublicProperty } }) .expect(200); const { body: response } = await supertest @@ -240,7 +229,6 @@ export default function ({ getService }: FtrProviderContext) { expect(response.attributes).to.eql({ publicProperty: updatedPublicProperty, publicPropertyExcludedFromAAD: savedObjectOriginalAttributes.publicPropertyExcludedFromAAD, - publicDeepProperty: savedObjectOriginalAttributes.publicDeepProperty, }); expect(response.error).to.eql({ message: 'Unable to decrypt attribute "publicPropertyStoredEncrypted"', @@ -260,7 +248,6 @@ export default function ({ getService }: FtrProviderContext) { publicProperty: savedObjectOriginalAttributes.publicProperty, publicPropertyExcludedFromAAD: savedObjectOriginalAttributes.publicPropertyExcludedFromAAD, publicPropertyStoredEncrypted: savedObjectOriginalAttributes.publicPropertyStoredEncrypted, - publicDeepProperty: savedObjectOriginalAttributes.publicDeepProperty, }); expect(savedObjects[0].error).to.be(undefined); }); @@ -270,9 +257,7 @@ export default function ({ getService }: FtrProviderContext) { // encrypted attributes. const updatedPublicProperty = randomness.string(); await supertest - .put( - `${getURLAPIBaseURL()}update-with-partial/${encryptedSavedObjectType}/${savedObject.id}` - ) + .put(`${getURLAPIBaseURL()}${encryptedSavedObjectType}/${savedObject.id}`) .set('kbn-xsrf', 'xxx') .send({ attributes: { publicProperty: updatedPublicProperty } }) .expect(200); @@ -288,7 +273,6 @@ export default function ({ getService }: FtrProviderContext) { expect(savedObjects[0].attributes).to.eql({ publicProperty: updatedPublicProperty, publicPropertyExcludedFromAAD: savedObjectOriginalAttributes.publicPropertyExcludedFromAAD, - publicDeepProperty: savedObjectOriginalAttributes.publicDeepProperty, }); expect(savedObjects[0].error).to.eql({ message: 'Unable to decrypt attribute "publicPropertyStoredEncrypted"', @@ -310,7 +294,6 @@ export default function ({ getService }: FtrProviderContext) { publicProperty: savedObjectOriginalAttributes.publicProperty, publicPropertyExcludedFromAAD: savedObjectOriginalAttributes.publicPropertyExcludedFromAAD, publicPropertyStoredEncrypted: savedObjectOriginalAttributes.publicPropertyStoredEncrypted, - publicDeepProperty: savedObjectOriginalAttributes.publicDeepProperty, }); expect(savedObjects[0].error).to.be(undefined); }); @@ -320,13 +303,9 @@ export default function ({ getService }: FtrProviderContext) { // encrypted attributes. const updatedPublicProperty = randomness.string(); await supertest - .put( - `${getURLAPIBaseURL()}update-with-partial/${encryptedSavedObjectType}/${savedObject.id}` - ) + .put(`${getURLAPIBaseURL()}${encryptedSavedObjectType}/${savedObject.id}`) .set('kbn-xsrf', 'xxx') - .send({ - attributes: { publicProperty: updatedPublicProperty }, - }) + .send({ attributes: { publicProperty: updatedPublicProperty } }) .expect(200); const { @@ -342,7 +321,6 @@ export default function ({ getService }: FtrProviderContext) { expect(savedObjects[0].attributes).to.eql({ publicProperty: updatedPublicProperty, publicPropertyExcludedFromAAD: savedObjectOriginalAttributes.publicPropertyExcludedFromAAD, - publicDeepProperty: savedObjectOriginalAttributes.publicDeepProperty, }); expect(savedObjects[0].error).to.eql({ message: 'Unable to decrypt attribute "publicPropertyStoredEncrypted"', @@ -355,7 +333,6 @@ export default function ({ getService }: FtrProviderContext) { publicPropertyExcludedFromAAD: randomness.string(), publicPropertyStoredEncrypted: randomness.string(), privateProperty: randomness.string(), - publicDeepProperty: { [randomness.string()]: randomness.string() }, }; const { body: response } = await supertest @@ -368,7 +345,6 @@ export default function ({ getService }: FtrProviderContext) { publicProperty: updatedAttributes.publicProperty, publicPropertyExcludedFromAAD: updatedAttributes.publicPropertyExcludedFromAAD, publicPropertyStoredEncrypted: updatedAttributes.publicPropertyStoredEncrypted, - publicDeepProperty: updatedAttributes.publicDeepProperty, }); const rawAttributes = await getRawSavedObjectAttributes(savedObject); @@ -385,32 +361,6 @@ export default function ({ getService }: FtrProviderContext) { expect(rawAttributes.privateProperty).to.not.be(updatedAttributes.privateProperty); }); - it('#update overwrites objects in their entirety', async () => { - const updatedAttributes = { - publicProperty: randomness.string(), - publicPropertyExcludedFromAAD: randomness.string(), - publicPropertyStoredEncrypted: randomness.string(), - privateProperty: randomness.string(), - publicDeepProperty: { [randomness.string()]: randomness.string() }, - }; - - await supertest - .put(`${getURLAPIBaseURL()}${encryptedSavedObjectType}/${savedObject.id}`) - .set('kbn-xsrf', 'xxx') - .send({ attributes: updatedAttributes }) - .expect(200); - - const { body: decryptedResponse } = await supertest - .get( - `${getURLAPIBaseURL()}get-decrypted-as-internal-user/${encryptedSavedObjectType}/${ - savedObject.id - }` - ) - .expect(200); - - expect(decryptedResponse.attributes).to.eql(updatedAttributes); - }); - it('#getDecryptedAsInternalUser decrypts and returns all attributes', async () => { const { body: decryptedResponse } = await supertest .get( @@ -427,9 +377,7 @@ export default function ({ getService }: FtrProviderContext) { const updatedAttributes = { publicPropertyExcludedFromAAD: randomness.string() }; const { body: response } = await supertest - .put( - `${getURLAPIBaseURL()}update-with-partial/${encryptedSavedObjectType}/${savedObject.id}` - ) + .put(`${getURLAPIBaseURL()}${encryptedSavedObjectType}/${savedObject.id}`) .set('kbn-xsrf', 'xxx') .send({ attributes: updatedAttributes }) .expect(200); @@ -456,9 +404,7 @@ export default function ({ getService }: FtrProviderContext) { const updatedAttributes = { publicProperty: randomness.string() }; const { body: response } = await supertest - .put( - `${getURLAPIBaseURL()}update-with-partial/${encryptedSavedObjectType}/${savedObject.id}` - ) + .put(`${getURLAPIBaseURL()}${encryptedSavedObjectType}/${savedObject.id}`) .set('kbn-xsrf', 'xxx') .send({ attributes: updatedAttributes }) .expect(200); From 3dfce39389fb6fa533da7c4b7596a37eb36f0cfe Mon Sep 17 00:00:00 2001 From: Gidi Meir Morris Date: Fri, 14 Aug 2020 18:14:09 +0100 Subject: [PATCH 20/41] fixed import --- .../saved_objects/saved_objects_client_without_updates.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/plugins/alerts/server/saved_objects/saved_objects_client_without_updates.ts b/x-pack/plugins/alerts/server/saved_objects/saved_objects_client_without_updates.ts index 0c3ecf7e3c78e3..10ac773cb1f77a 100644 --- a/x-pack/plugins/alerts/server/saved_objects/saved_objects_client_without_updates.ts +++ b/x-pack/plugins/alerts/server/saved_objects/saved_objects_client_without_updates.ts @@ -3,7 +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. */ -import { SavedObjectsClientContract, SavedObjectsCreateOptions } from 'kibana/server'; +import { SavedObjectsClientContract, SavedObjectsCreateOptions, SavedObject } from 'kibana/server'; type AlertSavedObjectsCreateOptions = Omit; type AlertSavedObjectsUpdateOptions = Omit & From 692894e995b35a85e977285f26d4f6ccceaec293 Mon Sep 17 00:00:00 2001 From: Gidi Meir Morris Date: Mon, 17 Aug 2020 10:13:16 +0100 Subject: [PATCH 21/41] fixed alerts clients unit tests --- .../alerts/server/alerts_client.test.ts | 243 ++++++++++-------- 1 file changed, 135 insertions(+), 108 deletions(-) diff --git a/x-pack/plugins/alerts/server/alerts_client.test.ts b/x-pack/plugins/alerts/server/alerts_client.test.ts index 44b872e68874d6..b2eb8731b1a781 100644 --- a/x-pack/plugins/alerts/server/alerts_client.test.ts +++ b/x-pack/plugins/alerts/server/alerts_client.test.ts @@ -217,7 +217,7 @@ describe('create()', () => { params: {}, ownerId: null, }); - unsecuredSavedObjectsClient.update.mockResolvedValueOnce({ + unsecuredSavedObjectsClient.create.mockResolvedValueOnce({ id: '1', type: 'alert', attributes: { @@ -315,7 +315,7 @@ describe('create()', () => { params: {}, ownerId: null, }); - unsecuredSavedObjectsClient.update.mockResolvedValueOnce({ + unsecuredSavedObjectsClient.create.mockResolvedValueOnce({ id: '1', type: 'alert', attributes: { @@ -368,7 +368,7 @@ describe('create()', () => { "updatedBy": "elastic", } `); - expect(unsecuredSavedObjectsClient.create).toHaveBeenCalledTimes(1); + expect(unsecuredSavedObjectsClient.create).toHaveBeenCalledTimes(2); expect(unsecuredSavedObjectsClient.create.mock.calls[0]).toHaveLength(3); expect(unsecuredSavedObjectsClient.create.mock.calls[0][0]).toEqual('alert'); expect(unsecuredSavedObjectsClient.create.mock.calls[0][1]).toMatchInlineSnapshot(` @@ -437,11 +437,9 @@ describe('create()', () => { }, ] `); - expect(unsecuredSavedObjectsClient.update).toHaveBeenCalledTimes(1); - expect(unsecuredSavedObjectsClient.update.mock.calls[0]).toHaveLength(4); - expect(unsecuredSavedObjectsClient.update.mock.calls[0][0]).toEqual('alert'); - expect(unsecuredSavedObjectsClient.update.mock.calls[0][1]).toEqual('1'); - expect(unsecuredSavedObjectsClient.update.mock.calls[0][2]).toMatchInlineSnapshot(` + expect(unsecuredSavedObjectsClient.create.mock.calls[1]).toHaveLength(3); + expect(unsecuredSavedObjectsClient.create.mock.calls[1][0]).toEqual('alert'); + expect(unsecuredSavedObjectsClient.create.mock.calls[1][1]).toMatchInlineSnapshot(` Object { "actions": Array [ Object { @@ -477,8 +475,10 @@ describe('create()', () => { "updatedBy": "elastic", } `); - expect(unsecuredSavedObjectsClient.update.mock.calls[0][3]).toMatchInlineSnapshot(` + expect(unsecuredSavedObjectsClient.create.mock.calls[1][2]).toMatchInlineSnapshot(` Object { + "id": "1", + "overwrite": true, "references": Array [ Object { "id": "1", @@ -584,7 +584,7 @@ describe('create()', () => { params: {}, ownerId: null, }); - unsecuredSavedObjectsClient.update.mockResolvedValueOnce({ + unsecuredSavedObjectsClient.create.mockResolvedValueOnce({ id: '1', type: 'alert', attributes: { @@ -954,7 +954,7 @@ describe('create()', () => { params: {}, ownerId: null, }); - unsecuredSavedObjectsClient.update.mockResolvedValueOnce({ + unsecuredSavedObjectsClient.create.mockResolvedValueOnce({ id: '1', type: 'alert', attributes: { @@ -1067,7 +1067,7 @@ describe('create()', () => { params: {}, ownerId: null, }); - unsecuredSavedObjectsClient.update.mockResolvedValueOnce({ + unsecuredSavedObjectsClient.create.mockResolvedValueOnce({ id: '1', type: 'alert', attributes: { @@ -1158,7 +1158,7 @@ describe('enable()', () => { alertsClientParams.createAPIKey.mockResolvedValue({ apiKeysEnabled: false, }); - unsecuredSavedObjectsClient.update.mockResolvedValueOnce({ + unsecuredSavedObjectsClient.create.mockResolvedValueOnce({ ...existingAlert, attributes: { ...existingAlert.attributes, @@ -1226,7 +1226,7 @@ describe('enable()', () => { }); test('enables an alert', async () => { - unsecuredSavedObjectsClient.update.mockResolvedValueOnce({ + unsecuredSavedObjectsClient.create.mockResolvedValueOnce({ ...existingAlert, attributes: { ...existingAlert.attributes, @@ -1244,9 +1244,8 @@ describe('enable()', () => { }); expect(alertsClientParams.invalidateAPIKey).not.toHaveBeenCalled(); expect(alertsClientParams.createAPIKey).toHaveBeenCalled(); - expect(unsecuredSavedObjectsClient.update).toHaveBeenCalledWith( + expect(unsecuredSavedObjectsClient.create).toHaveBeenCalledWith( 'alert', - '1', { schedule: { interval: '10s' }, alertTypeId: 'myType', @@ -1268,6 +1267,8 @@ describe('enable()', () => { ], }, { + id: '1', + overwrite: true, version: '123', references: existingAlert.references, } @@ -1285,9 +1286,8 @@ describe('enable()', () => { }, scope: ['alerting'], }); - expect(unsecuredSavedObjectsClient.update).toHaveBeenCalledWith( + expect(unsecuredSavedObjectsClient.create).toHaveBeenCalledWith( 'alert', - '1', { ...existingAlert.attributes, enabled: true, @@ -1297,6 +1297,8 @@ describe('enable()', () => { scheduledTaskId: 'task-123', }, { + id: '1', + overwrite: true, version: '123', references: existingAlert.references, } @@ -1332,7 +1334,7 @@ describe('enable()', () => { await alertsClient.enable({ id: '1' }); expect(alertsClientParams.getUserName).not.toHaveBeenCalled(); expect(alertsClientParams.createAPIKey).not.toHaveBeenCalled(); - expect(unsecuredSavedObjectsClient.update).not.toHaveBeenCalled(); + expect(unsecuredSavedObjectsClient.create).not.toHaveBeenCalled(); expect(taskManager.schedule).not.toHaveBeenCalled(); }); @@ -1343,9 +1345,8 @@ describe('enable()', () => { }); await alertsClient.enable({ id: '1' }); - expect(unsecuredSavedObjectsClient.update).toHaveBeenCalledWith( + expect(unsecuredSavedObjectsClient.create).toHaveBeenCalledWith( 'alert', - '1', { schedule: { interval: '10s' }, alertTypeId: 'myType', @@ -1367,6 +1368,8 @@ describe('enable()', () => { ], }, { + id: '1', + overwrite: true, references: [], version: '123', } @@ -1392,33 +1395,33 @@ describe('enable()', () => { ); expect(alertsClientParams.getUserName).not.toHaveBeenCalled(); expect(alertsClientParams.createAPIKey).not.toHaveBeenCalled(); - expect(unsecuredSavedObjectsClient.update).not.toHaveBeenCalled(); + expect(unsecuredSavedObjectsClient.create).not.toHaveBeenCalled(); expect(taskManager.schedule).not.toHaveBeenCalled(); }); test('throws error when failing to update the first time', async () => { - unsecuredSavedObjectsClient.update.mockReset(); - unsecuredSavedObjectsClient.update.mockRejectedValueOnce(new Error('Fail to update')); + unsecuredSavedObjectsClient.create.mockReset(); + unsecuredSavedObjectsClient.create.mockRejectedValueOnce(new Error('Fail to update')); await expect(alertsClient.enable({ id: '1' })).rejects.toThrowErrorMatchingInlineSnapshot( `"Fail to update"` ); expect(alertsClientParams.getUserName).toHaveBeenCalled(); expect(alertsClientParams.createAPIKey).toHaveBeenCalled(); - expect(unsecuredSavedObjectsClient.update).toHaveBeenCalledTimes(1); + expect(unsecuredSavedObjectsClient.create).toHaveBeenCalledTimes(1); expect(taskManager.schedule).not.toHaveBeenCalled(); }); test('throws error when failing to update the second time', async () => { - unsecuredSavedObjectsClient.update.mockReset(); - unsecuredSavedObjectsClient.update.mockResolvedValueOnce({ + unsecuredSavedObjectsClient.create.mockReset(); + unsecuredSavedObjectsClient.create.mockResolvedValueOnce({ ...existingAlert, attributes: { ...existingAlert.attributes, enabled: true, }, }); - unsecuredSavedObjectsClient.update.mockRejectedValueOnce( + unsecuredSavedObjectsClient.create.mockRejectedValueOnce( new Error('Fail to update second time') ); @@ -1427,7 +1430,7 @@ describe('enable()', () => { ); expect(alertsClientParams.getUserName).toHaveBeenCalled(); expect(alertsClientParams.createAPIKey).toHaveBeenCalled(); - expect(unsecuredSavedObjectsClient.update).toHaveBeenCalledTimes(2); + expect(unsecuredSavedObjectsClient.create).toHaveBeenCalledTimes(2); expect(taskManager.schedule).toHaveBeenCalled(); }); @@ -1439,7 +1442,7 @@ describe('enable()', () => { ); expect(alertsClientParams.getUserName).toHaveBeenCalled(); expect(alertsClientParams.createAPIKey).toHaveBeenCalled(); - expect(unsecuredSavedObjectsClient.update).toHaveBeenCalled(); + expect(unsecuredSavedObjectsClient.create).toHaveBeenCalled(); }); }); @@ -1475,6 +1478,8 @@ describe('disable()', () => { ...existingAlert.attributes, apiKey: Buffer.from('123:abc').toString('base64'), }, + version: '123', + references: [], }; beforeEach(() => { @@ -1509,9 +1514,8 @@ describe('disable()', () => { expect(encryptedSavedObjects.getDecryptedAsInternalUser).toHaveBeenCalledWith('alert', '1', { namespace: 'default', }); - expect(unsecuredSavedObjectsClient.update).toHaveBeenCalledWith( + expect(unsecuredSavedObjectsClient.create).toHaveBeenCalledWith( 'alert', - '1', { consumer: 'myApp', schedule: { interval: '10s' }, @@ -1531,6 +1535,8 @@ describe('disable()', () => { ], }, { + id: '1', + overwrite: true, references: [], version: '123', } @@ -1547,9 +1553,8 @@ describe('disable()', () => { expect(encryptedSavedObjects.getDecryptedAsInternalUser).toHaveBeenCalledWith('alert', '1', { namespace: 'default', }); - expect(unsecuredSavedObjectsClient.update).toHaveBeenCalledWith( + expect(unsecuredSavedObjectsClient.create).toHaveBeenCalledWith( 'alert', - '1', { consumer: 'myApp', schedule: { interval: '10s' }, @@ -1569,6 +1574,8 @@ describe('disable()', () => { ], }, { + id: '1', + overwrite: true, version: '123', references: [], } @@ -1588,7 +1595,7 @@ describe('disable()', () => { }); await alertsClient.disable({ id: '1' }); - expect(unsecuredSavedObjectsClient.update).not.toHaveBeenCalled(); + expect(unsecuredSavedObjectsClient.create).not.toHaveBeenCalled(); expect(taskManager.remove).not.toHaveBeenCalled(); expect(alertsClientParams.invalidateAPIKey).not.toHaveBeenCalled(); }); @@ -1604,7 +1611,7 @@ describe('disable()', () => { encryptedSavedObjects.getDecryptedAsInternalUser.mockRejectedValueOnce(new Error('Fail')); await alertsClient.disable({ id: '1' }); - expect(unsecuredSavedObjectsClient.update).toHaveBeenCalled(); + expect(unsecuredSavedObjectsClient.create).toHaveBeenCalled(); expect(taskManager.remove).toHaveBeenCalled(); expect(alertsClientParams.invalidateAPIKey).not.toHaveBeenCalled(); expect(alertsClientParams.logger.error).toHaveBeenCalledWith( @@ -1613,7 +1620,7 @@ describe('disable()', () => { }); test('throws when unsecuredSavedObjectsClient update fails', async () => { - unsecuredSavedObjectsClient.update.mockRejectedValueOnce(new Error('Failed to update')); + unsecuredSavedObjectsClient.create.mockRejectedValueOnce(new Error('Failed to update')); await expect(alertsClient.disable({ id: '1' })).rejects.toThrowErrorMatchingInlineSnapshot( `"Failed to update"` @@ -1662,9 +1669,8 @@ describe('muteAll()', () => { }); await alertsClient.muteAll({ id: '1' }); - expect(unsecuredSavedObjectsClient.update).toHaveBeenCalledWith( + expect(unsecuredSavedObjectsClient.create).toHaveBeenCalledWith( 'alert', - '1', { actions: [ { @@ -1681,7 +1687,11 @@ describe('muteAll()', () => { mutedInstanceIds: [], updatedBy: 'elastic', }, - { references: [] } + { + id: '1', + overwrite: true, + references: [], + } ); }); @@ -1760,12 +1770,12 @@ describe('unmuteAll()', () => { muteAll: true, }, references: [], + version: '123', }); await alertsClient.unmuteAll({ id: '1' }); - expect(unsecuredSavedObjectsClient.update).toHaveBeenCalledWith( + expect(unsecuredSavedObjectsClient.create).toHaveBeenCalledWith( 'alert', - '1', { actions: [ { @@ -1782,7 +1792,12 @@ describe('unmuteAll()', () => { mutedInstanceIds: [], updatedBy: 'elastic', }, - { references: [] } + { + id: '1', + overwrite: true, + version: '123', + references: [], + } ); }); @@ -1859,9 +1874,8 @@ describe('muteInstance()', () => { }); await alertsClient.muteInstance({ alertId: '1', alertInstanceId: '2' }); - expect(unsecuredSavedObjectsClient.update).toHaveBeenCalledWith( + expect(unsecuredSavedObjectsClient.create).toHaveBeenCalledWith( 'alert', - '1', { actions: [], schedule: { interval: '10s' }, @@ -1871,7 +1885,12 @@ describe('muteInstance()', () => { mutedInstanceIds: ['2'], updatedBy: 'elastic', }, - { version: '123', references: [] } + { + id: '1', + overwrite: true, + version: '123', + references: [], + } ); }); @@ -1892,7 +1911,7 @@ describe('muteInstance()', () => { }); await alertsClient.muteInstance({ alertId: '1', alertInstanceId: '2' }); - expect(unsecuredSavedObjectsClient.update).not.toHaveBeenCalled(); + expect(unsecuredSavedObjectsClient.create).not.toHaveBeenCalled(); }); test('skips muting when alert is muted', async () => { @@ -1913,7 +1932,7 @@ describe('muteInstance()', () => { }); await alertsClient.muteInstance({ alertId: '1', alertInstanceId: '2' }); - expect(unsecuredSavedObjectsClient.update).not.toHaveBeenCalled(); + expect(unsecuredSavedObjectsClient.create).not.toHaveBeenCalled(); }); describe('authorization', () => { @@ -1997,9 +2016,8 @@ describe('unmuteInstance()', () => { }); await alertsClient.unmuteInstance({ alertId: '1', alertInstanceId: '2' }); - expect(unsecuredSavedObjectsClient.update).toHaveBeenCalledWith( + expect(unsecuredSavedObjectsClient.create).toHaveBeenCalledWith( 'alert', - '1', { actions: [], schedule: { interval: '10s' }, @@ -2010,6 +2028,8 @@ describe('unmuteInstance()', () => { updatedBy: 'elastic', }, { + id: '1', + overwrite: true, version: '123', references: [], } @@ -2033,7 +2053,7 @@ describe('unmuteInstance()', () => { }); await alertsClient.unmuteInstance({ alertId: '1', alertInstanceId: '2' }); - expect(unsecuredSavedObjectsClient.update).not.toHaveBeenCalled(); + expect(unsecuredSavedObjectsClient.create).not.toHaveBeenCalled(); }); test('skips unmuting when alert is muted', async () => { @@ -2054,7 +2074,7 @@ describe('unmuteInstance()', () => { }); await alertsClient.unmuteInstance({ alertId: '1', alertInstanceId: '2' }); - expect(unsecuredSavedObjectsClient.update).not.toHaveBeenCalled(); + expect(unsecuredSavedObjectsClient.create).not.toHaveBeenCalled(); }); describe('authorization', () => { @@ -3076,7 +3096,7 @@ describe('update()', () => { }); test('updates given parameters', async () => { - unsecuredSavedObjectsClient.update.mockResolvedValueOnce({ + unsecuredSavedObjectsClient.create.mockResolvedValueOnce({ id: '1', type: 'alert', attributes: { @@ -3213,11 +3233,10 @@ describe('update()', () => { namespace: 'default', }); expect(unsecuredSavedObjectsClient.get).not.toHaveBeenCalled(); - expect(unsecuredSavedObjectsClient.update).toHaveBeenCalledTimes(1); - expect(unsecuredSavedObjectsClient.update.mock.calls[0]).toHaveLength(4); - expect(unsecuredSavedObjectsClient.update.mock.calls[0][0]).toEqual('alert'); - expect(unsecuredSavedObjectsClient.update.mock.calls[0][1]).toEqual('1'); - expect(unsecuredSavedObjectsClient.update.mock.calls[0][2]).toMatchInlineSnapshot(` + expect(unsecuredSavedObjectsClient.create).toHaveBeenCalledTimes(1); + expect(unsecuredSavedObjectsClient.create.mock.calls[0]).toHaveLength(3); + expect(unsecuredSavedObjectsClient.create.mock.calls[0][0]).toEqual('alert'); + expect(unsecuredSavedObjectsClient.create.mock.calls[0][1]).toMatchInlineSnapshot(` Object { "actions": Array [ Object { @@ -3265,8 +3284,10 @@ describe('update()', () => { "updatedBy": "elastic", } `); - expect(unsecuredSavedObjectsClient.update.mock.calls[0][3]).toMatchInlineSnapshot(` + expect(unsecuredSavedObjectsClient.create.mock.calls[0][2]).toMatchInlineSnapshot(` Object { + "id": "1", + "overwrite": true, "references": Array [ Object { "id": "1", @@ -3307,7 +3328,7 @@ describe('update()', () => { apiKeysEnabled: true, result: { id: '123', name: '123', api_key: 'abc' }, }); - unsecuredSavedObjectsClient.update.mockResolvedValueOnce({ + unsecuredSavedObjectsClient.create.mockResolvedValueOnce({ id: '1', type: 'alert', attributes: { @@ -3386,11 +3407,10 @@ describe('update()', () => { "updatedAt": 2019-02-12T21:01:22.479Z, } `); - expect(unsecuredSavedObjectsClient.update).toHaveBeenCalledTimes(1); - expect(unsecuredSavedObjectsClient.update.mock.calls[0]).toHaveLength(4); - expect(unsecuredSavedObjectsClient.update.mock.calls[0][0]).toEqual('alert'); - expect(unsecuredSavedObjectsClient.update.mock.calls[0][1]).toEqual('1'); - expect(unsecuredSavedObjectsClient.update.mock.calls[0][2]).toMatchInlineSnapshot(` + expect(unsecuredSavedObjectsClient.create).toHaveBeenCalledTimes(1); + expect(unsecuredSavedObjectsClient.create.mock.calls[0]).toHaveLength(3); + expect(unsecuredSavedObjectsClient.create.mock.calls[0][0]).toEqual('alert'); + expect(unsecuredSavedObjectsClient.create.mock.calls[0][1]).toMatchInlineSnapshot(` Object { "actions": Array [ Object { @@ -3422,18 +3442,20 @@ describe('update()', () => { "updatedBy": "elastic", } `); - expect(unsecuredSavedObjectsClient.update.mock.calls[0][3]).toMatchInlineSnapshot(` - Object { - "references": Array [ - Object { - "id": "1", - "name": "action_0", - "type": "action", - }, - ], - "version": "123", - } - `); + expect(unsecuredSavedObjectsClient.create.mock.calls[0][2]).toMatchInlineSnapshot(` + Object { + "id": "1", + "overwrite": true, + "references": Array [ + Object { + "id": "1", + "name": "action_0", + "type": "action", + }, + ], + "version": "123", + } + `); }); it(`doesn't call the createAPIKey function when alert is disabled`, async () => { @@ -3457,7 +3479,7 @@ describe('update()', () => { }, ], }); - unsecuredSavedObjectsClient.update.mockResolvedValueOnce({ + unsecuredSavedObjectsClient.create.mockResolvedValueOnce({ id: '1', type: 'alert', attributes: { @@ -3537,11 +3559,10 @@ describe('update()', () => { "updatedAt": 2019-02-12T21:01:22.479Z, } `); - expect(unsecuredSavedObjectsClient.update).toHaveBeenCalledTimes(1); - expect(unsecuredSavedObjectsClient.update.mock.calls[0]).toHaveLength(4); - expect(unsecuredSavedObjectsClient.update.mock.calls[0][0]).toEqual('alert'); - expect(unsecuredSavedObjectsClient.update.mock.calls[0][1]).toEqual('1'); - expect(unsecuredSavedObjectsClient.update.mock.calls[0][2]).toMatchInlineSnapshot(` + expect(unsecuredSavedObjectsClient.create).toHaveBeenCalledTimes(1); + expect(unsecuredSavedObjectsClient.create.mock.calls[0]).toHaveLength(3); + expect(unsecuredSavedObjectsClient.create.mock.calls[0][0]).toEqual('alert'); + expect(unsecuredSavedObjectsClient.create.mock.calls[0][1]).toMatchInlineSnapshot(` Object { "actions": Array [ Object { @@ -3573,18 +3594,20 @@ describe('update()', () => { "updatedBy": "elastic", } `); - expect(unsecuredSavedObjectsClient.update.mock.calls[0][3]).toMatchInlineSnapshot(` - Object { - "references": Array [ - Object { - "id": "1", - "name": "action_0", - "type": "action", - }, - ], - "version": "123", - } - `); + expect(unsecuredSavedObjectsClient.create.mock.calls[0][2]).toMatchInlineSnapshot(` + Object { + "id": "1", + "overwrite": true, + "references": Array [ + Object { + "id": "1", + "name": "action_0", + "type": "action", + }, + ], + "version": "123", + } + `); }); it('should validate params', async () => { @@ -3643,7 +3666,7 @@ describe('update()', () => { }, ], }); - unsecuredSavedObjectsClient.update.mockResolvedValueOnce({ + unsecuredSavedObjectsClient.create.mockResolvedValueOnce({ id: '1', type: 'alert', attributes: { @@ -3722,7 +3745,7 @@ describe('update()', () => { }, ], }); - unsecuredSavedObjectsClient.update.mockResolvedValueOnce({ + unsecuredSavedObjectsClient.create.mockResolvedValueOnce({ id: '1', type: 'alert', attributes: { @@ -3876,7 +3899,7 @@ describe('update()', () => { params: {}, ownerId: null, }); - unsecuredSavedObjectsClient.update.mockResolvedValueOnce({ + unsecuredSavedObjectsClient.create.mockResolvedValueOnce({ id: alertId, type: 'alert', attributes: { @@ -4048,7 +4071,7 @@ describe('update()', () => { describe('authorization', () => { beforeEach(() => { - unsecuredSavedObjectsClient.update.mockResolvedValueOnce({ + unsecuredSavedObjectsClient.create.mockResolvedValueOnce({ id: '1', type: 'alert', attributes: { @@ -4163,9 +4186,8 @@ describe('updateApiKey()', () => { expect(encryptedSavedObjects.getDecryptedAsInternalUser).toHaveBeenCalledWith('alert', '1', { namespace: 'default', }); - expect(unsecuredSavedObjectsClient.update).toHaveBeenCalledWith( + expect(unsecuredSavedObjectsClient.create).toHaveBeenCalledWith( 'alert', - '1', { schedule: { interval: '10s' }, alertTypeId: 'myType', @@ -4186,7 +4208,10 @@ describe('updateApiKey()', () => { }, ], }, - pick(existingAlert, 'version', 'references') + { + overwrite: true, + ...pick(existingAlert, 'id', 'version', 'references'), + } ); expect(alertsClientParams.invalidateAPIKey).toHaveBeenCalledWith({ id: '123' }); }); @@ -4199,9 +4224,8 @@ describe('updateApiKey()', () => { expect(encryptedSavedObjects.getDecryptedAsInternalUser).toHaveBeenCalledWith('alert', '1', { namespace: 'default', }); - expect(unsecuredSavedObjectsClient.update).toHaveBeenCalledWith( + expect(unsecuredSavedObjectsClient.create).toHaveBeenCalledWith( 'alert', - '1', { schedule: { interval: '10s' }, alertTypeId: 'myType', @@ -4222,7 +4246,10 @@ describe('updateApiKey()', () => { }, ], }, - pick(existingAlert, 'version', 'references') + { + overwrite: true, + ...pick(existingAlert, 'id', 'version', 'references'), + } ); expect(alertsClientParams.invalidateAPIKey).not.toHaveBeenCalled(); }); @@ -4234,7 +4261,7 @@ describe('updateApiKey()', () => { expect(alertsClientParams.logger.error).toHaveBeenCalledWith( 'Failed to invalidate API Key: Fail' ); - expect(unsecuredSavedObjectsClient.update).toHaveBeenCalled(); + expect(unsecuredSavedObjectsClient.create).toHaveBeenCalled(); }); test('swallows error when getting decrypted object throws', async () => { @@ -4244,12 +4271,12 @@ describe('updateApiKey()', () => { expect(alertsClientParams.logger.error).toHaveBeenCalledWith( 'updateApiKey(): Failed to load API key to invalidate on alert 1: Fail' ); - expect(unsecuredSavedObjectsClient.update).toHaveBeenCalled(); + expect(unsecuredSavedObjectsClient.create).toHaveBeenCalled(); expect(alertsClientParams.invalidateAPIKey).not.toHaveBeenCalled(); }); test('throws when unsecuredSavedObjectsClient update fails', async () => { - unsecuredSavedObjectsClient.update.mockRejectedValueOnce(new Error('Fail')); + unsecuredSavedObjectsClient.create.mockRejectedValueOnce(new Error('Fail')); await expect(alertsClient.updateApiKey({ id: '1' })).rejects.toThrowErrorMatchingInlineSnapshot( `"Fail"` From 13c4ec45dcfc98df821379a94783a69db5b8aed9 Mon Sep 17 00:00:00 2001 From: Gidi Meir Morris Date: Mon, 17 Aug 2020 12:20:46 +0100 Subject: [PATCH 22/41] added version to saved object creation --- ...-plugin-core-server.savedobjectscreateoptions.md | 1 + ...core-server.savedobjectscreateoptions.version.md | 13 +++++++++++++ .../saved_objects/service/lib/repository.test.js | 10 ++++++++++ .../server/saved_objects/service/lib/repository.ts | 2 ++ .../saved_objects/service/saved_objects_client.ts | 5 +++++ src/core/server/server.api.md | 1 + 6 files changed, 32 insertions(+) create mode 100644 docs/development/core/server/kibana-plugin-core-server.savedobjectscreateoptions.version.md diff --git a/docs/development/core/server/kibana-plugin-core-server.savedobjectscreateoptions.md b/docs/development/core/server/kibana-plugin-core-server.savedobjectscreateoptions.md index 5e9433c5c9196a..c5201efd0608d6 100644 --- a/docs/development/core/server/kibana-plugin-core-server.savedobjectscreateoptions.md +++ b/docs/development/core/server/kibana-plugin-core-server.savedobjectscreateoptions.md @@ -20,4 +20,5 @@ export interface SavedObjectsCreateOptions extends SavedObjectsBaseOptions | [overwrite](./kibana-plugin-core-server.savedobjectscreateoptions.overwrite.md) | boolean | Overwrite existing documents (defaults to false) | | [references](./kibana-plugin-core-server.savedobjectscreateoptions.references.md) | SavedObjectReference[] | | | [refresh](./kibana-plugin-core-server.savedobjectscreateoptions.refresh.md) | MutatingOperationRefreshSetting | The Elasticsearch Refresh setting for this operation | +| [version](./kibana-plugin-core-server.savedobjectscreateoptions.version.md) | string | An opaque version number which changes on each successful write operation. Can be used in conjunction with overwrite for implementing optimistic concurrency control. | diff --git a/docs/development/core/server/kibana-plugin-core-server.savedobjectscreateoptions.version.md b/docs/development/core/server/kibana-plugin-core-server.savedobjectscreateoptions.version.md new file mode 100644 index 00000000000000..51da57064abb9e --- /dev/null +++ b/docs/development/core/server/kibana-plugin-core-server.savedobjectscreateoptions.version.md @@ -0,0 +1,13 @@ + + +[Home](./index.md) > [kibana-plugin-core-server](./kibana-plugin-core-server.md) > [SavedObjectsCreateOptions](./kibana-plugin-core-server.savedobjectscreateoptions.md) > [version](./kibana-plugin-core-server.savedobjectscreateoptions.version.md) + +## SavedObjectsCreateOptions.version property + +An opaque version number which changes on each successful write operation. Can be used in conjunction with `overwrite` for implementing optimistic concurrency control. + +Signature: + +```typescript +version?: string; +``` diff --git a/src/core/server/saved_objects/service/lib/repository.test.js b/src/core/server/saved_objects/service/lib/repository.test.js index 4a9fceb9bf3578..5632b5deaa816e 100644 --- a/src/core/server/saved_objects/service/lib/repository.test.js +++ b/src/core/server/saved_objects/service/lib/repository.test.js @@ -1516,6 +1516,16 @@ describe('SavedObjectsRepository', () => { expect(client.index).toHaveBeenCalled(); }); + it(`should use the ES index with version of ID and version are defined and overwrite=true`, async () => { + await createSuccess(type, attributes, { id, overwrite: true, version: mockVersion }); + expect(client.index).toHaveBeenCalled(); + + expect(client.index.mock.calls[0][0]).toMatchObject({ + if_seq_no: mockVersionProps._seq_no, + if_primary_term: mockVersionProps._primary_term, + }); + }); + it(`should use the ES create action if ID is defined and overwrite=false`, async () => { await createSuccess(type, attributes, { id }); expect(client.create).toHaveBeenCalled(); diff --git a/src/core/server/saved_objects/service/lib/repository.ts b/src/core/server/saved_objects/service/lib/repository.ts index d7e1ecba0370be..226caecccf9314 100644 --- a/src/core/server/saved_objects/service/lib/repository.ts +++ b/src/core/server/saved_objects/service/lib/repository.ts @@ -220,6 +220,7 @@ export class SavedObjectsRepository { overwrite = false, references = [], refresh = DEFAULT_REFRESH_SETTING, + version, } = options; if (!this._allowedTypes.includes(type)) { @@ -259,6 +260,7 @@ export class SavedObjectsRepository { index: this.getIndexForType(type), refresh, body: raw._source, + ...(overwrite && version ? decodeRequestVersion(version) : {}), }; const { body } = diff --git a/src/core/server/saved_objects/service/saved_objects_client.ts b/src/core/server/saved_objects/service/saved_objects_client.ts index e15a92c92772f3..f5a0455ccad5b7 100644 --- a/src/core/server/saved_objects/service/saved_objects_client.ts +++ b/src/core/server/saved_objects/service/saved_objects_client.ts @@ -37,6 +37,11 @@ export interface SavedObjectsCreateOptions extends SavedObjectsBaseOptions { id?: string; /** Overwrite existing documents (defaults to false) */ overwrite?: boolean; + /** + * An opaque version number which changes on each successful write operation. + * Can be used in conjunction with `overwrite` for implementing optimistic concurrency control. + **/ + version?: string; /** {@inheritDoc SavedObjectsMigrationVersion} */ migrationVersion?: SavedObjectsMigrationVersion; references?: SavedObjectReference[]; diff --git a/src/core/server/server.api.md b/src/core/server/server.api.md index 21ef66230f6981..429c22dff8788d 100644 --- a/src/core/server/server.api.md +++ b/src/core/server/server.api.md @@ -2173,6 +2173,7 @@ export interface SavedObjectsCreateOptions extends SavedObjectsBaseOptions { // (undocumented) references?: SavedObjectReference[]; refresh?: MutatingOperationRefreshSetting; + version?: string; } // @public (undocumented) From d4820e1fd5d3073b23cc007b32ed5bd14a59c470 Mon Sep 17 00:00:00 2001 From: Gidi Meir Morris Date: Mon, 17 Aug 2020 16:08:47 +0100 Subject: [PATCH 23/41] added version to saved object bulk creation --- .../service/lib/repository.test.js | 33 +++++++++++++++++-- .../saved_objects/service/lib/repository.ts | 10 +++++- .../service/saved_objects_client.ts | 1 + 3 files changed, 41 insertions(+), 3 deletions(-) diff --git a/src/core/server/saved_objects/service/lib/repository.test.js b/src/core/server/saved_objects/service/lib/repository.test.js index 5632b5deaa816e..c461fbacc51cae 100644 --- a/src/core/server/saved_objects/service/lib/repository.test.js +++ b/src/core/server/saved_objects/service/lib/repository.test.js @@ -464,8 +464,16 @@ describe('SavedObjectsRepository', () => { { method, _index = expect.any(String), getId = () => expect.any(String) } ) => { const body = []; - for (const { type, id } of objects) { - body.push({ [method]: { _index, _id: getId(type, id) } }); + for (const { type, id, if_primary_term: ifPrimaryTerm, if_seq_no: ifSeqNo } of objects) { + body.push({ + [method]: { + _index, + _id: getId(type, id), + ...(ifPrimaryTerm && ifSeqNo + ? { if_primary_term: expect.any(Number), if_seq_no: expect.any(Number) } + : {}), + }, + }); body.push(expect.any(Object)); } expect(client.bulk).toHaveBeenCalledWith( @@ -525,6 +533,27 @@ describe('SavedObjectsRepository', () => { expectClientCallArgsAction([obj1, obj2], { method: 'index' }); }); + it(`should use the ES index method with version if ID and version are defined and overwrite=true`, async () => { + await bulkCreateSuccess( + [ + { + ...obj1, + version: mockVersion, + }, + obj2, + ], + { overwrite: true } + ); + + const obj1WithSeq = { + ...obj1, + if_seq_no: mockVersionProps._seq_no, + if_primary_term: mockVersionProps._primary_term, + }; + + expectClientCallArgsAction([obj1WithSeq, obj2], { method: 'index' }); + }); + it(`should use the ES create method if ID is defined and overwrite=false`, async () => { await bulkCreateSuccess([obj1, obj2]); expectClientCallArgsAction([obj1, obj2], { method: 'create' }); diff --git a/src/core/server/saved_objects/service/lib/repository.ts b/src/core/server/saved_objects/service/lib/repository.ts index 226caecccf9314..9f6db446ea195e 100644 --- a/src/core/server/saved_objects/service/lib/repository.ts +++ b/src/core/server/saved_objects/service/lib/repository.ts @@ -347,7 +347,12 @@ export class SavedObjectsRepository { let savedObjectNamespace; let savedObjectNamespaces; - const { esRequestIndex, object, method } = expectedBulkGetResult.value; + let versionProperties; + const { + esRequestIndex, + object: { version, ...object }, + method, + } = expectedBulkGetResult.value; if (esRequestIndex !== undefined) { const indexFound = bulkGetResponse?.statusCode !== 404; const actualResult = indexFound ? bulkGetResponse?.body.docs[esRequestIndex] : undefined; @@ -364,12 +369,14 @@ export class SavedObjectsRepository { }; } savedObjectNamespaces = getSavedObjectNamespaces(namespace, docFound && actualResult); + versionProperties = getExpectedVersionProperties(version, actualResult); } else { if (this._registry.isSingleNamespace(object.type)) { savedObjectNamespace = namespace; } else if (this._registry.isMultiNamespace(object.type)) { savedObjectNamespaces = getSavedObjectNamespaces(namespace); } + versionProperties = getExpectedVersionProperties(version); } const expectedResult = { @@ -394,6 +401,7 @@ export class SavedObjectsRepository { [method]: { _id: expectedResult.rawMigratedDoc._id, _index: this.getIndexForType(object.type), + ...(overwrite && versionProperties), }, }, expectedResult.rawMigratedDoc._source diff --git a/src/core/server/saved_objects/service/saved_objects_client.ts b/src/core/server/saved_objects/service/saved_objects_client.ts index f5a0455ccad5b7..6a9f4f5143e841 100644 --- a/src/core/server/saved_objects/service/saved_objects_client.ts +++ b/src/core/server/saved_objects/service/saved_objects_client.ts @@ -57,6 +57,7 @@ export interface SavedObjectsBulkCreateObject { id?: string; type: string; attributes: T; + version?: string; references?: SavedObjectReference[]; /** {@inheritDoc SavedObjectsMigrationVersion} */ migrationVersion?: SavedObjectsMigrationVersion; From 814b98ddc9dbb931c7c2fe7080c8610bdeec949e Mon Sep 17 00:00:00 2001 From: Gidi Meir Morris Date: Mon, 17 Aug 2020 18:59:29 +0100 Subject: [PATCH 24/41] added missing core docs --- ...plugin-core-server.savedobjectsbulkcreateobject.md | 1 + ...ore-server.savedobjectsbulkcreateobject.version.md | 11 +++++++++++ src/core/server/server.api.md | 2 ++ .../server/services/epm/kibana/assets/install.ts | 4 +++- 4 files changed, 17 insertions(+), 1 deletion(-) create mode 100644 docs/development/core/server/kibana-plugin-core-server.savedobjectsbulkcreateobject.version.md diff --git a/docs/development/core/server/kibana-plugin-core-server.savedobjectsbulkcreateobject.md b/docs/development/core/server/kibana-plugin-core-server.savedobjectsbulkcreateobject.md index 5a9ca36ba56f43..5ccad134248f64 100644 --- a/docs/development/core/server/kibana-plugin-core-server.savedobjectsbulkcreateobject.md +++ b/docs/development/core/server/kibana-plugin-core-server.savedobjectsbulkcreateobject.md @@ -20,4 +20,5 @@ export interface SavedObjectsBulkCreateObject | [migrationVersion](./kibana-plugin-core-server.savedobjectsbulkcreateobject.migrationversion.md) | SavedObjectsMigrationVersion | Information about the migrations that have been applied to this SavedObject. When Kibana starts up, KibanaMigrator detects outdated documents and migrates them based on this value. For each migration that has been applied, the plugin's name is used as a key and the latest migration version as the value. | | [references](./kibana-plugin-core-server.savedobjectsbulkcreateobject.references.md) | SavedObjectReference[] | | | [type](./kibana-plugin-core-server.savedobjectsbulkcreateobject.type.md) | string | | +| [version](./kibana-plugin-core-server.savedobjectsbulkcreateobject.version.md) | string | | diff --git a/docs/development/core/server/kibana-plugin-core-server.savedobjectsbulkcreateobject.version.md b/docs/development/core/server/kibana-plugin-core-server.savedobjectsbulkcreateobject.version.md new file mode 100644 index 00000000000000..ca2a38693d0365 --- /dev/null +++ b/docs/development/core/server/kibana-plugin-core-server.savedobjectsbulkcreateobject.version.md @@ -0,0 +1,11 @@ + + +[Home](./index.md) > [kibana-plugin-core-server](./kibana-plugin-core-server.md) > [SavedObjectsBulkCreateObject](./kibana-plugin-core-server.savedobjectsbulkcreateobject.md) > [version](./kibana-plugin-core-server.savedobjectsbulkcreateobject.version.md) + +## SavedObjectsBulkCreateObject.version property + +Signature: + +```typescript +version?: string; +``` diff --git a/src/core/server/server.api.md b/src/core/server/server.api.md index 429c22dff8788d..7734dacee87c7b 100644 --- a/src/core/server/server.api.md +++ b/src/core/server/server.api.md @@ -2039,6 +2039,8 @@ export interface SavedObjectsBulkCreateObject { references?: SavedObjectReference[]; // (undocumented) type: string; + // (undocumented) + version?: string; } // @public (undocumented) diff --git a/x-pack/plugins/ingest_manager/server/services/epm/kibana/assets/install.ts b/x-pack/plugins/ingest_manager/server/services/epm/kibana/assets/install.ts index 5741764164b839..84892d2027847d 100644 --- a/x-pack/plugins/ingest_manager/server/services/epm/kibana/assets/install.ts +++ b/x-pack/plugins/ingest_manager/server/services/epm/kibana/assets/install.ts @@ -14,7 +14,9 @@ import * as Registry from '../../registry'; import { AssetType, KibanaAssetType, AssetReference } from '../../../../types'; import { savedObjectTypes } from '../../packages'; -type SavedObjectToBe = Required & { type: AssetType }; +type SavedObjectToBe = Required> & { + type: AssetType; +}; export type ArchiveAsset = Pick< SavedObject, 'id' | 'attributes' | 'migrationVersion' | 'references' From 7bd360cca060e1007455434bbc88ec6f511d45e8 Mon Sep 17 00:00:00 2001 From: Gidi Meir Morris Date: Tue, 18 Aug 2020 11:06:03 +0100 Subject: [PATCH 25/41] Revert "added missing core docs" This reverts commit 814b98ddc9dbb931c7c2fe7080c8610bdeec949e. --- ...plugin-core-server.savedobjectsbulkcreateobject.md | 1 - ...ore-server.savedobjectsbulkcreateobject.version.md | 11 ----------- src/core/server/server.api.md | 2 -- .../server/services/epm/kibana/assets/install.ts | 4 +--- 4 files changed, 1 insertion(+), 17 deletions(-) delete mode 100644 docs/development/core/server/kibana-plugin-core-server.savedobjectsbulkcreateobject.version.md diff --git a/docs/development/core/server/kibana-plugin-core-server.savedobjectsbulkcreateobject.md b/docs/development/core/server/kibana-plugin-core-server.savedobjectsbulkcreateobject.md index 5ccad134248f64..5a9ca36ba56f43 100644 --- a/docs/development/core/server/kibana-plugin-core-server.savedobjectsbulkcreateobject.md +++ b/docs/development/core/server/kibana-plugin-core-server.savedobjectsbulkcreateobject.md @@ -20,5 +20,4 @@ export interface SavedObjectsBulkCreateObject | [migrationVersion](./kibana-plugin-core-server.savedobjectsbulkcreateobject.migrationversion.md) | SavedObjectsMigrationVersion | Information about the migrations that have been applied to this SavedObject. When Kibana starts up, KibanaMigrator detects outdated documents and migrates them based on this value. For each migration that has been applied, the plugin's name is used as a key and the latest migration version as the value. | | [references](./kibana-plugin-core-server.savedobjectsbulkcreateobject.references.md) | SavedObjectReference[] | | | [type](./kibana-plugin-core-server.savedobjectsbulkcreateobject.type.md) | string | | -| [version](./kibana-plugin-core-server.savedobjectsbulkcreateobject.version.md) | string | | diff --git a/docs/development/core/server/kibana-plugin-core-server.savedobjectsbulkcreateobject.version.md b/docs/development/core/server/kibana-plugin-core-server.savedobjectsbulkcreateobject.version.md deleted file mode 100644 index ca2a38693d0365..00000000000000 --- a/docs/development/core/server/kibana-plugin-core-server.savedobjectsbulkcreateobject.version.md +++ /dev/null @@ -1,11 +0,0 @@ - - -[Home](./index.md) > [kibana-plugin-core-server](./kibana-plugin-core-server.md) > [SavedObjectsBulkCreateObject](./kibana-plugin-core-server.savedobjectsbulkcreateobject.md) > [version](./kibana-plugin-core-server.savedobjectsbulkcreateobject.version.md) - -## SavedObjectsBulkCreateObject.version property - -Signature: - -```typescript -version?: string; -``` diff --git a/src/core/server/server.api.md b/src/core/server/server.api.md index d7f1e463a07e41..c7ba03f4da2ba0 100644 --- a/src/core/server/server.api.md +++ b/src/core/server/server.api.md @@ -2041,8 +2041,6 @@ export interface SavedObjectsBulkCreateObject { references?: SavedObjectReference[]; // (undocumented) type: string; - // (undocumented) - version?: string; } // @public (undocumented) diff --git a/x-pack/plugins/ingest_manager/server/services/epm/kibana/assets/install.ts b/x-pack/plugins/ingest_manager/server/services/epm/kibana/assets/install.ts index 84892d2027847d..5741764164b839 100644 --- a/x-pack/plugins/ingest_manager/server/services/epm/kibana/assets/install.ts +++ b/x-pack/plugins/ingest_manager/server/services/epm/kibana/assets/install.ts @@ -14,9 +14,7 @@ import * as Registry from '../../registry'; import { AssetType, KibanaAssetType, AssetReference } from '../../../../types'; import { savedObjectTypes } from '../../packages'; -type SavedObjectToBe = Required> & { - type: AssetType; -}; +type SavedObjectToBe = Required & { type: AssetType }; export type ArchiveAsset = Pick< SavedObject, 'id' | 'attributes' | 'migrationVersion' | 'references' From caa1834b3d732ba9983f83cfad113700a6f411c6 Mon Sep 17 00:00:00 2001 From: Gidi Meir Morris Date: Tue, 18 Aug 2020 11:06:15 +0100 Subject: [PATCH 26/41] Revert "added version to saved object bulk creation" This reverts commit d4820e1fd5d3073b23cc007b32ed5bd14a59c470. --- .../service/lib/repository.test.js | 33 ++----------------- .../saved_objects/service/lib/repository.ts | 10 +----- .../service/saved_objects_client.ts | 1 - 3 files changed, 3 insertions(+), 41 deletions(-) diff --git a/src/core/server/saved_objects/service/lib/repository.test.js b/src/core/server/saved_objects/service/lib/repository.test.js index c461fbacc51cae..5632b5deaa816e 100644 --- a/src/core/server/saved_objects/service/lib/repository.test.js +++ b/src/core/server/saved_objects/service/lib/repository.test.js @@ -464,16 +464,8 @@ describe('SavedObjectsRepository', () => { { method, _index = expect.any(String), getId = () => expect.any(String) } ) => { const body = []; - for (const { type, id, if_primary_term: ifPrimaryTerm, if_seq_no: ifSeqNo } of objects) { - body.push({ - [method]: { - _index, - _id: getId(type, id), - ...(ifPrimaryTerm && ifSeqNo - ? { if_primary_term: expect.any(Number), if_seq_no: expect.any(Number) } - : {}), - }, - }); + for (const { type, id } of objects) { + body.push({ [method]: { _index, _id: getId(type, id) } }); body.push(expect.any(Object)); } expect(client.bulk).toHaveBeenCalledWith( @@ -533,27 +525,6 @@ describe('SavedObjectsRepository', () => { expectClientCallArgsAction([obj1, obj2], { method: 'index' }); }); - it(`should use the ES index method with version if ID and version are defined and overwrite=true`, async () => { - await bulkCreateSuccess( - [ - { - ...obj1, - version: mockVersion, - }, - obj2, - ], - { overwrite: true } - ); - - const obj1WithSeq = { - ...obj1, - if_seq_no: mockVersionProps._seq_no, - if_primary_term: mockVersionProps._primary_term, - }; - - expectClientCallArgsAction([obj1WithSeq, obj2], { method: 'index' }); - }); - it(`should use the ES create method if ID is defined and overwrite=false`, async () => { await bulkCreateSuccess([obj1, obj2]); expectClientCallArgsAction([obj1, obj2], { method: 'create' }); diff --git a/src/core/server/saved_objects/service/lib/repository.ts b/src/core/server/saved_objects/service/lib/repository.ts index 9f6db446ea195e..226caecccf9314 100644 --- a/src/core/server/saved_objects/service/lib/repository.ts +++ b/src/core/server/saved_objects/service/lib/repository.ts @@ -347,12 +347,7 @@ export class SavedObjectsRepository { let savedObjectNamespace; let savedObjectNamespaces; - let versionProperties; - const { - esRequestIndex, - object: { version, ...object }, - method, - } = expectedBulkGetResult.value; + const { esRequestIndex, object, method } = expectedBulkGetResult.value; if (esRequestIndex !== undefined) { const indexFound = bulkGetResponse?.statusCode !== 404; const actualResult = indexFound ? bulkGetResponse?.body.docs[esRequestIndex] : undefined; @@ -369,14 +364,12 @@ export class SavedObjectsRepository { }; } savedObjectNamespaces = getSavedObjectNamespaces(namespace, docFound && actualResult); - versionProperties = getExpectedVersionProperties(version, actualResult); } else { if (this._registry.isSingleNamespace(object.type)) { savedObjectNamespace = namespace; } else if (this._registry.isMultiNamespace(object.type)) { savedObjectNamespaces = getSavedObjectNamespaces(namespace); } - versionProperties = getExpectedVersionProperties(version); } const expectedResult = { @@ -401,7 +394,6 @@ export class SavedObjectsRepository { [method]: { _id: expectedResult.rawMigratedDoc._id, _index: this.getIndexForType(object.type), - ...(overwrite && versionProperties), }, }, expectedResult.rawMigratedDoc._source diff --git a/src/core/server/saved_objects/service/saved_objects_client.ts b/src/core/server/saved_objects/service/saved_objects_client.ts index 6a9f4f5143e841..f5a0455ccad5b7 100644 --- a/src/core/server/saved_objects/service/saved_objects_client.ts +++ b/src/core/server/saved_objects/service/saved_objects_client.ts @@ -57,7 +57,6 @@ export interface SavedObjectsBulkCreateObject { id?: string; type: string; attributes: T; - version?: string; references?: SavedObjectReference[]; /** {@inheritDoc SavedObjectsMigrationVersion} */ migrationVersion?: SavedObjectsMigrationVersion; From 8f3dfc86623728a37860aca5021916c2e8be8acd Mon Sep 17 00:00:00 2001 From: Gidi Meir Morris Date: Tue, 18 Aug 2020 15:39:39 +0100 Subject: [PATCH 27/41] Revert "Revert "added version to saved object bulk creation"" This reverts commit caa1834b3d732ba9983f83cfad113700a6f411c6. --- .../service/lib/repository.test.js | 33 +++++++++++++++++-- .../saved_objects/service/lib/repository.ts | 10 +++++- .../service/saved_objects_client.ts | 1 + 3 files changed, 41 insertions(+), 3 deletions(-) diff --git a/src/core/server/saved_objects/service/lib/repository.test.js b/src/core/server/saved_objects/service/lib/repository.test.js index 5632b5deaa816e..c461fbacc51cae 100644 --- a/src/core/server/saved_objects/service/lib/repository.test.js +++ b/src/core/server/saved_objects/service/lib/repository.test.js @@ -464,8 +464,16 @@ describe('SavedObjectsRepository', () => { { method, _index = expect.any(String), getId = () => expect.any(String) } ) => { const body = []; - for (const { type, id } of objects) { - body.push({ [method]: { _index, _id: getId(type, id) } }); + for (const { type, id, if_primary_term: ifPrimaryTerm, if_seq_no: ifSeqNo } of objects) { + body.push({ + [method]: { + _index, + _id: getId(type, id), + ...(ifPrimaryTerm && ifSeqNo + ? { if_primary_term: expect.any(Number), if_seq_no: expect.any(Number) } + : {}), + }, + }); body.push(expect.any(Object)); } expect(client.bulk).toHaveBeenCalledWith( @@ -525,6 +533,27 @@ describe('SavedObjectsRepository', () => { expectClientCallArgsAction([obj1, obj2], { method: 'index' }); }); + it(`should use the ES index method with version if ID and version are defined and overwrite=true`, async () => { + await bulkCreateSuccess( + [ + { + ...obj1, + version: mockVersion, + }, + obj2, + ], + { overwrite: true } + ); + + const obj1WithSeq = { + ...obj1, + if_seq_no: mockVersionProps._seq_no, + if_primary_term: mockVersionProps._primary_term, + }; + + expectClientCallArgsAction([obj1WithSeq, obj2], { method: 'index' }); + }); + it(`should use the ES create method if ID is defined and overwrite=false`, async () => { await bulkCreateSuccess([obj1, obj2]); expectClientCallArgsAction([obj1, obj2], { method: 'create' }); diff --git a/src/core/server/saved_objects/service/lib/repository.ts b/src/core/server/saved_objects/service/lib/repository.ts index 226caecccf9314..9f6db446ea195e 100644 --- a/src/core/server/saved_objects/service/lib/repository.ts +++ b/src/core/server/saved_objects/service/lib/repository.ts @@ -347,7 +347,12 @@ export class SavedObjectsRepository { let savedObjectNamespace; let savedObjectNamespaces; - const { esRequestIndex, object, method } = expectedBulkGetResult.value; + let versionProperties; + const { + esRequestIndex, + object: { version, ...object }, + method, + } = expectedBulkGetResult.value; if (esRequestIndex !== undefined) { const indexFound = bulkGetResponse?.statusCode !== 404; const actualResult = indexFound ? bulkGetResponse?.body.docs[esRequestIndex] : undefined; @@ -364,12 +369,14 @@ export class SavedObjectsRepository { }; } savedObjectNamespaces = getSavedObjectNamespaces(namespace, docFound && actualResult); + versionProperties = getExpectedVersionProperties(version, actualResult); } else { if (this._registry.isSingleNamespace(object.type)) { savedObjectNamespace = namespace; } else if (this._registry.isMultiNamespace(object.type)) { savedObjectNamespaces = getSavedObjectNamespaces(namespace); } + versionProperties = getExpectedVersionProperties(version); } const expectedResult = { @@ -394,6 +401,7 @@ export class SavedObjectsRepository { [method]: { _id: expectedResult.rawMigratedDoc._id, _index: this.getIndexForType(object.type), + ...(overwrite && versionProperties), }, }, expectedResult.rawMigratedDoc._source diff --git a/src/core/server/saved_objects/service/saved_objects_client.ts b/src/core/server/saved_objects/service/saved_objects_client.ts index f5a0455ccad5b7..6a9f4f5143e841 100644 --- a/src/core/server/saved_objects/service/saved_objects_client.ts +++ b/src/core/server/saved_objects/service/saved_objects_client.ts @@ -57,6 +57,7 @@ export interface SavedObjectsBulkCreateObject { id?: string; type: string; attributes: T; + version?: string; references?: SavedObjectReference[]; /** {@inheritDoc SavedObjectsMigrationVersion} */ migrationVersion?: SavedObjectsMigrationVersion; From c4e8c59302c43ad5a9106fd07226d37f53900d11 Mon Sep 17 00:00:00 2001 From: Gidi Meir Morris Date: Tue, 18 Aug 2020 15:39:56 +0100 Subject: [PATCH 28/41] Revert "Revert "added missing core docs"" This reverts commit 7bd360cca060e1007455434bbc88ec6f511d45e8. --- ...plugin-core-server.savedobjectsbulkcreateobject.md | 1 + ...ore-server.savedobjectsbulkcreateobject.version.md | 11 +++++++++++ src/core/server/server.api.md | 2 ++ .../server/services/epm/kibana/assets/install.ts | 4 +++- 4 files changed, 17 insertions(+), 1 deletion(-) create mode 100644 docs/development/core/server/kibana-plugin-core-server.savedobjectsbulkcreateobject.version.md diff --git a/docs/development/core/server/kibana-plugin-core-server.savedobjectsbulkcreateobject.md b/docs/development/core/server/kibana-plugin-core-server.savedobjectsbulkcreateobject.md index 5a9ca36ba56f43..5ccad134248f64 100644 --- a/docs/development/core/server/kibana-plugin-core-server.savedobjectsbulkcreateobject.md +++ b/docs/development/core/server/kibana-plugin-core-server.savedobjectsbulkcreateobject.md @@ -20,4 +20,5 @@ export interface SavedObjectsBulkCreateObject | [migrationVersion](./kibana-plugin-core-server.savedobjectsbulkcreateobject.migrationversion.md) | SavedObjectsMigrationVersion | Information about the migrations that have been applied to this SavedObject. When Kibana starts up, KibanaMigrator detects outdated documents and migrates them based on this value. For each migration that has been applied, the plugin's name is used as a key and the latest migration version as the value. | | [references](./kibana-plugin-core-server.savedobjectsbulkcreateobject.references.md) | SavedObjectReference[] | | | [type](./kibana-plugin-core-server.savedobjectsbulkcreateobject.type.md) | string | | +| [version](./kibana-plugin-core-server.savedobjectsbulkcreateobject.version.md) | string | | diff --git a/docs/development/core/server/kibana-plugin-core-server.savedobjectsbulkcreateobject.version.md b/docs/development/core/server/kibana-plugin-core-server.savedobjectsbulkcreateobject.version.md new file mode 100644 index 00000000000000..ca2a38693d0365 --- /dev/null +++ b/docs/development/core/server/kibana-plugin-core-server.savedobjectsbulkcreateobject.version.md @@ -0,0 +1,11 @@ + + +[Home](./index.md) > [kibana-plugin-core-server](./kibana-plugin-core-server.md) > [SavedObjectsBulkCreateObject](./kibana-plugin-core-server.savedobjectsbulkcreateobject.md) > [version](./kibana-plugin-core-server.savedobjectsbulkcreateobject.version.md) + +## SavedObjectsBulkCreateObject.version property + +Signature: + +```typescript +version?: string; +``` diff --git a/src/core/server/server.api.md b/src/core/server/server.api.md index c7ba03f4da2ba0..d7f1e463a07e41 100644 --- a/src/core/server/server.api.md +++ b/src/core/server/server.api.md @@ -2041,6 +2041,8 @@ export interface SavedObjectsBulkCreateObject { references?: SavedObjectReference[]; // (undocumented) type: string; + // (undocumented) + version?: string; } // @public (undocumented) diff --git a/x-pack/plugins/ingest_manager/server/services/epm/kibana/assets/install.ts b/x-pack/plugins/ingest_manager/server/services/epm/kibana/assets/install.ts index 5741764164b839..84892d2027847d 100644 --- a/x-pack/plugins/ingest_manager/server/services/epm/kibana/assets/install.ts +++ b/x-pack/plugins/ingest_manager/server/services/epm/kibana/assets/install.ts @@ -14,7 +14,9 @@ import * as Registry from '../../registry'; import { AssetType, KibanaAssetType, AssetReference } from '../../../../types'; import { savedObjectTypes } from '../../packages'; -type SavedObjectToBe = Required & { type: AssetType }; +type SavedObjectToBe = Required> & { + type: AssetType; +}; export type ArchiveAsset = Pick< SavedObject, 'id' | 'attributes' | 'migrationVersion' | 'references' From c391446410b769746c6d4a62cb81d538d3533835 Mon Sep 17 00:00:00 2001 From: Gidi Meir Morris Date: Tue, 18 Aug 2020 18:04:45 +0100 Subject: [PATCH 29/41] omit version from SO import/export --- .../saved_objects/import/import_saved_objects.ts | 7 ++++++- .../saved_objects/import/resolve_import_errors.ts | 12 ++++++++---- 2 files changed, 14 insertions(+), 5 deletions(-) diff --git a/src/core/server/saved_objects/import/import_saved_objects.ts b/src/core/server/saved_objects/import/import_saved_objects.ts index 6065e03fb16283..4956491a79aa91 100644 --- a/src/core/server/saved_objects/import/import_saved_objects.ts +++ b/src/core/server/saved_objects/import/import_saved_objects.ts @@ -25,6 +25,7 @@ import { SavedObjectsImportOptions, } from './types'; import { validateReferences } from './validate_references'; +import { SavedObject } from '../types'; /** * Import saved objects from given stream. See the {@link SavedObjectsImportOptions | options} for more @@ -67,7 +68,7 @@ export async function importSavedObjectsFromStream({ } // Create objects in bulk - const bulkCreateResult = await savedObjectsClient.bulkCreate(filteredObjects, { + const bulkCreateResult = await savedObjectsClient.bulkCreate(omitVersion(filteredObjects), { overwrite, namespace, }); @@ -82,3 +83,7 @@ export async function importSavedObjectsFromStream({ ...(errorAccumulator.length ? { errors: errorAccumulator } : {}), }; } + +export function omitVersion(objects: SavedObject[]): SavedObject[] { + return objects.map(({ version, ...object }) => object); +} diff --git a/src/core/server/saved_objects/import/resolve_import_errors.ts b/src/core/server/saved_objects/import/resolve_import_errors.ts index a5175aa0805980..dce044a31a577a 100644 --- a/src/core/server/saved_objects/import/resolve_import_errors.ts +++ b/src/core/server/saved_objects/import/resolve_import_errors.ts @@ -26,6 +26,7 @@ import { SavedObjectsResolveImportErrorsOptions, } from './types'; import { validateReferences } from './validate_references'; +import { omitVersion } from './import_saved_objects'; /** * Resolve and return saved object import errors. @@ -91,7 +92,7 @@ export async function resolveSavedObjectsImportErrors({ // Bulk create in two batches, overwrites and non-overwrites const { objectsToOverwrite, objectsToNotOverwrite } = splitOverwrites(filteredObjects, retries); if (objectsToOverwrite.length) { - const bulkCreateResult = await savedObjectsClient.bulkCreate(objectsToOverwrite, { + const bulkCreateResult = await savedObjectsClient.bulkCreate(omitVersion(objectsToOverwrite), { overwrite: true, namespace, }); @@ -102,9 +103,12 @@ export async function resolveSavedObjectsImportErrors({ successCount += bulkCreateResult.saved_objects.filter((obj) => !obj.error).length; } if (objectsToNotOverwrite.length) { - const bulkCreateResult = await savedObjectsClient.bulkCreate(objectsToNotOverwrite, { - namespace, - }); + const bulkCreateResult = await savedObjectsClient.bulkCreate( + omitVersion(objectsToNotOverwrite), + { + namespace, + } + ); errorAccumulator = [ ...errorAccumulator, ...extractErrors(bulkCreateResult.saved_objects, objectsToNotOverwrite), From a0fc3845e4db856d230dba4276e9dac618dfff87 Mon Sep 17 00:00:00 2001 From: Gidi Meir Morris Date: Tue, 18 Aug 2020 18:28:02 +0100 Subject: [PATCH 30/41] remove version when loading sample data --- src/plugins/home/server/services/sample_data/routes/install.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/plugins/home/server/services/sample_data/routes/install.ts b/src/plugins/home/server/services/sample_data/routes/install.ts index 2d1a53fbb09dc8..b94456682afcc9 100644 --- a/src/plugins/home/server/services/sample_data/routes/install.ts +++ b/src/plugins/home/server/services/sample_data/routes/install.ts @@ -154,7 +154,7 @@ export function createInstallRoute( let createResults; try { createResults = await context.core.savedObjects.client.bulkCreate( - sampleDataset.savedObjects, + sampleDataset.savedObjects.map(({ version, ...savedObject }) => savedObject), { overwrite: true } ); } catch (err) { From c58689a724f6fd10fc7bb24971cd667193fb5d83 Mon Sep 17 00:00:00 2001 From: Gidi Meir Morris Date: Fri, 21 Aug 2020 17:55:57 +0100 Subject: [PATCH 31/41] only allow id in eso when theres a version --- .../encrypted_saved_objects_client_wrapper.test.ts | 6 +++--- .../saved_objects/encrypted_saved_objects_client_wrapper.ts | 6 ++++-- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.test.ts b/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.test.ts index b704aaaba03433..ebe61a3e953924 100644 --- a/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.test.ts +++ b/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.test.ts @@ -76,7 +76,7 @@ describe('#create', () => { attrNotSoSecret: 'not-so-secret', attrThree: 'three', }; - const options = { id: 'predefined-uuid', overwrite: true }; + const options = { id: 'predefined-uuid', overwrite: true, version: 'some-version' }; const mockedResponse = { id: 'predefined-uuid', type: 'known-type', @@ -117,7 +117,7 @@ describe('#create', () => { attrNotSoSecret: '*not-so-secret*', attrThree: 'three', }, - { id: 'predefined-uuid', overwrite: true } + { id: 'predefined-uuid', overwrite: true, version: 'some-version' } ); }); @@ -328,7 +328,7 @@ describe('#bulkCreate', () => { mockBaseClient.bulkCreate.mockResolvedValue(mockedResponse); const bulkCreateParams = [ - { id: 'predefined-uuid', type: 'known-type', attributes }, + { id: 'predefined-uuid', type: 'known-type', attributes, version: 'some-version' }, { type: 'unknown-type', attributes }, ]; diff --git a/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.ts b/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.ts index ea4b0ea0a96e64..81acc8de7454c8 100644 --- a/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.ts +++ b/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.ts @@ -60,7 +60,8 @@ export class EncryptedSavedObjectsClientWrapper implements SavedObjectsClientCon // Saved objects with encrypted attributes should have IDs that are hard to guess especially // since IDs are part of the AAD used during encryption, that's why we control them within this // wrapper and don't allow consumers to specify their own IDs directly. - if (options.id && !options.overwrite) { + const canSpecifyID = options.overwrite && options.version; + if (options.id && !canSpecifyID) { throw new Error( 'Predefined IDs are not allowed for saved objects with encrypted attributes.' ); @@ -103,7 +104,8 @@ export class EncryptedSavedObjectsClientWrapper implements SavedObjectsClientCon // Saved objects with encrypted attributes should have IDs that are hard to guess especially // since IDs are part of the AAD used during encryption, that's why we control them within this // wrapper and don't allow consumers to specify their own IDs directly. - if (object.id && !options?.overwrite) { + const canSpecifyID = options?.overwrite && object.version; + if (object.id && !canSpecifyID) { throw new Error( 'Predefined IDs are not allowed for saved objects with encrypted attributes.' ); From 96689fa50a0b8a2d51698be4b799ca8ad60dbcf8 Mon Sep 17 00:00:00 2001 From: Gidi Meir Morris Date: Fri, 21 Aug 2020 19:11:57 +0100 Subject: [PATCH 32/41] added comment about allowing id --- .../saved_objects/encrypted_saved_objects_client_wrapper.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.ts b/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.ts index 81acc8de7454c8..6f6abd6bd41d8c 100644 --- a/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.ts +++ b/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.ts @@ -60,6 +60,10 @@ export class EncryptedSavedObjectsClientWrapper implements SavedObjectsClientCon // Saved objects with encrypted attributes should have IDs that are hard to guess especially // since IDs are part of the AAD used during encryption, that's why we control them within this // wrapper and don't allow consumers to specify their own IDs directly. + + // only allow a specified ID if we're overwriting an existing ESO with a Version + // this helps us ensure that the document really was previously created using ESO + // and not being used to get around the specified ID limitation const canSpecifyID = options.overwrite && options.version; if (options.id && !canSpecifyID) { throw new Error( @@ -111,7 +115,7 @@ export class EncryptedSavedObjectsClientWrapper implements SavedObjectsClientCon ); } - const id = object?.id ?? generateID(); + const id = object.id ?? generateID(); const namespace = getDescriptorNamespace( this.options.baseTypeRegistry, object.type, From 1f641c36ca376f04c85335a3ac1a23b8a4bca832 Mon Sep 17 00:00:00 2001 From: Gidi Meir Morris Date: Wed, 2 Sep 2020 08:36:45 +0100 Subject: [PATCH 33/41] added comment about type --- .../saved_objects/saved_objects_client_without_updates.ts | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/x-pack/plugins/actions/server/saved_objects/saved_objects_client_without_updates.ts b/x-pack/plugins/actions/server/saved_objects/saved_objects_client_without_updates.ts index 10ac773cb1f77a..b53561e5bbf152 100644 --- a/x-pack/plugins/actions/server/saved_objects/saved_objects_client_without_updates.ts +++ b/x-pack/plugins/actions/server/saved_objects/saved_objects_client_without_updates.ts @@ -9,6 +9,11 @@ type AlertSavedObjectsCreateOptions = Omit & Pick, 'id' | 'overwrite'>; +/** + * This type ensures that: + * 1. We never accidentally call `update` + * 2. We always specify `overwrite` when call `create` _with_ an `id`. + */ export type SavedObjectsClientWithoutUpdates = Omit< SavedObjectsClientContract, 'create' | 'update' | 'bulkUpdate' From c527d9e31f495c026757bdcf0311b6a4418e9ca5 Mon Sep 17 00:00:00 2001 From: Gidi Meir Morris Date: Tue, 15 Sep 2020 11:26:42 +0100 Subject: [PATCH 34/41] revert removal of partial updates from so client in alerts client --- .../actions/server/create_execute_function.ts | 10 +++---- .../actions/server/saved_objects/index.ts | 2 -- .../saved_objects_client_without_updates.ts | 26 ------------------- x-pack/plugins/alerts/server/alerts_client.ts | 6 ++--- .../alerts/server/saved_objects/index.ts | 2 -- .../saved_objects_client_without_updates.ts | 21 --------------- .../encrypted_saved_objects_client_wrapper.ts | 2 +- 7 files changed, 7 insertions(+), 62 deletions(-) delete mode 100644 x-pack/plugins/actions/server/saved_objects/saved_objects_client_without_updates.ts delete mode 100644 x-pack/plugins/alerts/server/saved_objects/saved_objects_client_without_updates.ts diff --git a/x-pack/plugins/actions/server/create_execute_function.ts b/x-pack/plugins/actions/server/create_execute_function.ts index 048d5a401769d8..85052eef93e051 100644 --- a/x-pack/plugins/actions/server/create_execute_function.ts +++ b/x-pack/plugins/actions/server/create_execute_function.ts @@ -4,12 +4,10 @@ * you may not use this file except in compliance with the Elastic License. */ +import { SavedObjectsClientContract } from '../../../../src/core/server'; import { TaskManagerStartContract } from '../../task_manager/server'; import { RawAction, ActionTypeRegistryContract, PreConfiguredAction } from './types'; -import { - ACTION_TASK_PARAMS_SAVED_OBJECT_TYPE, - SavedObjectsClientWithoutUpdates, -} from './saved_objects'; +import { ACTION_TASK_PARAMS_SAVED_OBJECT_TYPE } from './saved_objects'; interface CreateExecuteFunctionOptions { taskManager: TaskManagerStartContract; @@ -26,7 +24,7 @@ export interface ExecuteOptions { } export type ExecutionEnqueuer = ( - savedObjectsClient: SavedObjectsClientWithoutUpdates, + savedObjectsClient: SavedObjectsClientContract, options: ExecuteOptions ) => Promise; @@ -37,7 +35,7 @@ export function createExecutionEnqueuerFunction({ preconfiguredActions, }: CreateExecuteFunctionOptions) { return async function execute( - savedObjectsClient: SavedObjectsClientWithoutUpdates, + savedObjectsClient: SavedObjectsClientContract, { id, params, spaceId, apiKey }: ExecuteOptions ) { if (isESOUsingEphemeralEncryptionKey === true) { diff --git a/x-pack/plugins/actions/server/saved_objects/index.ts b/x-pack/plugins/actions/server/saved_objects/index.ts index 312a7264d986cd..54f186acc1ba57 100644 --- a/x-pack/plugins/actions/server/saved_objects/index.ts +++ b/x-pack/plugins/actions/server/saved_objects/index.ts @@ -8,8 +8,6 @@ import { SavedObjectsServiceSetup } from 'kibana/server'; import mappings from './mappings.json'; import { EncryptedSavedObjectsPluginSetup } from '../../../encrypted_saved_objects/server'; -export { SavedObjectsClientWithoutUpdates } from './saved_objects_client_without_updates'; - export const ACTION_SAVED_OBJECT_TYPE = 'action'; export const ACTION_TASK_PARAMS_SAVED_OBJECT_TYPE = 'action_task_params'; diff --git a/x-pack/plugins/actions/server/saved_objects/saved_objects_client_without_updates.ts b/x-pack/plugins/actions/server/saved_objects/saved_objects_client_without_updates.ts deleted file mode 100644 index b53561e5bbf152..00000000000000 --- a/x-pack/plugins/actions/server/saved_objects/saved_objects_client_without_updates.ts +++ /dev/null @@ -1,26 +0,0 @@ -/* - * 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 { SavedObjectsClientContract, SavedObjectsCreateOptions, SavedObject } from 'kibana/server'; - -type AlertSavedObjectsCreateOptions = Omit; -type AlertSavedObjectsUpdateOptions = Omit & - Pick, 'id' | 'overwrite'>; - -/** - * This type ensures that: - * 1. We never accidentally call `update` - * 2. We always specify `overwrite` when call `create` _with_ an `id`. - */ -export type SavedObjectsClientWithoutUpdates = Omit< - SavedObjectsClientContract, - 'create' | 'update' | 'bulkUpdate' -> & { - create( - type: string, - attributes: T, - options?: AlertSavedObjectsCreateOptions | AlertSavedObjectsUpdateOptions - ): Promise>; -}; diff --git a/x-pack/plugins/alerts/server/alerts_client.ts b/x-pack/plugins/alerts/server/alerts_client.ts index b312a4fbcfec54..016f176e1b4ab6 100644 --- a/x-pack/plugins/alerts/server/alerts_client.ts +++ b/x-pack/plugins/alerts/server/alerts_client.ts @@ -26,7 +26,6 @@ import { SanitizedAlert, AlertTaskState, AlertUpdateRequiredFields, - AlertStatus, AlertInstanceSummary, } from './types'; import { validateAlertTypeParams } from './lib'; @@ -40,7 +39,6 @@ import { TaskManagerStartContract } from '../../task_manager/server'; import { taskInstanceToAlertTaskInstance } from './task_runner/alert_task_instance'; import { deleteTaskIfItExists } from './lib/delete_task_if_it_exists'; import { RegistryAlertType } from './alert_type_registry'; -import { SavedObjectsClientWithoutUpdates } from './saved_objects'; import { AlertsAuthorization, WriteOperations, ReadOperations, and } from './authorization'; import { IEventLogClient } from '../../../plugins/event_log/server'; import { parseIsoOrRelativeDate } from './lib/iso_or_relative_date'; @@ -62,7 +60,7 @@ export type InvalidateAPIKeyResult = export interface ConstructorOptions { logger: Logger; taskManager: TaskManagerStartContract; - unsecuredSavedObjectsClient: SavedObjectsClientWithoutUpdates; + unsecuredSavedObjectsClient: SavedObjectsClientContract; authorization: AlertsAuthorization; actionsAuthorization: ActionsAuthorization; alertTypeRegistry: AlertTypeRegistry; @@ -150,7 +148,7 @@ export class AlertsClient { private readonly spaceId?: string; private readonly namespace?: string; private readonly taskManager: TaskManagerStartContract; - private readonly unsecuredSavedObjectsClient: SavedObjectsClientWithoutUpdates; + private readonly unsecuredSavedObjectsClient: SavedObjectsClientContract; private readonly authorization: AlertsAuthorization; private readonly alertTypeRegistry: AlertTypeRegistry; private readonly createAPIKey: (name: string) => Promise; diff --git a/x-pack/plugins/alerts/server/saved_objects/index.ts b/x-pack/plugins/alerts/server/saved_objects/index.ts index 559874ee50327e..06ce8d673e6b71 100644 --- a/x-pack/plugins/alerts/server/saved_objects/index.ts +++ b/x-pack/plugins/alerts/server/saved_objects/index.ts @@ -9,8 +9,6 @@ import mappings from './mappings.json'; import { getMigrations } from './migrations'; import { EncryptedSavedObjectsPluginSetup } from '../../../encrypted_saved_objects/server'; -export { SavedObjectsClientWithoutUpdates } from './saved_objects_client_without_updates'; - export function setupSavedObjects( savedObjects: SavedObjectsServiceSetup, encryptedSavedObjects: EncryptedSavedObjectsPluginSetup diff --git a/x-pack/plugins/alerts/server/saved_objects/saved_objects_client_without_updates.ts b/x-pack/plugins/alerts/server/saved_objects/saved_objects_client_without_updates.ts deleted file mode 100644 index 10ac773cb1f77a..00000000000000 --- a/x-pack/plugins/alerts/server/saved_objects/saved_objects_client_without_updates.ts +++ /dev/null @@ -1,21 +0,0 @@ -/* - * 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 { SavedObjectsClientContract, SavedObjectsCreateOptions, SavedObject } from 'kibana/server'; - -type AlertSavedObjectsCreateOptions = Omit; -type AlertSavedObjectsUpdateOptions = Omit & - Pick, 'id' | 'overwrite'>; - -export type SavedObjectsClientWithoutUpdates = Omit< - SavedObjectsClientContract, - 'create' | 'update' | 'bulkUpdate' -> & { - create( - type: string, - attributes: T, - options?: AlertSavedObjectsCreateOptions | AlertSavedObjectsUpdateOptions - ): Promise>; -}; diff --git a/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.ts b/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.ts index 3a2137936d7427..eef389186d6708 100644 --- a/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.ts +++ b/x-pack/plugins/encrypted_saved_objects/server/saved_objects/encrypted_saved_objects_client_wrapper.ts @@ -115,7 +115,7 @@ export class EncryptedSavedObjectsClientWrapper implements SavedObjectsClientCon // Saved objects with encrypted attributes should have IDs that are hard to guess especially // since IDs are part of the AAD used during encryption, that's why we control them within this - // wrapper and don't allow consumers to specify their own IDs directly. + // wrapper and don't allow consumers to specify their own IDs directly unless overwriting the original document. const canSpecifyID = options?.overwrite && object.version; if (object.id && !canSpecifyID) { throw new Error( From 1434bbe1473a3070a4dd6c65cf77d288d98abcfb Mon Sep 17 00:00:00 2001 From: Gidi Meir Morris Date: Tue, 15 Sep 2020 12:05:54 +0100 Subject: [PATCH 35/41] removed potential clashes with OCC changes --- x-pack/plugins/alerts/server/alerts_client.ts | 16 +++------------- 1 file changed, 3 insertions(+), 13 deletions(-) diff --git a/x-pack/plugins/alerts/server/alerts_client.ts b/x-pack/plugins/alerts/server/alerts_client.ts index 016f176e1b4ab6..4f374371dd880d 100644 --- a/x-pack/plugins/alerts/server/alerts_client.ts +++ b/x-pack/plugins/alerts/server/alerts_client.ts @@ -244,19 +244,9 @@ export class AlertsClient { } throw e; } - await this.unsecuredSavedObjectsClient.create( - 'alert', - { - ...createdAlert.attributes, - ...encryptedAttributes, - scheduledTaskId: scheduledTask.id, - }, - { - id: createdAlert.id, - overwrite: true, - references, - } - ); + await this.unsecuredSavedObjectsClient.update('alert', createdAlert.id, { + scheduledTaskId: scheduledTask.id, + }); createdAlert.attributes.scheduledTaskId = scheduledTask.id; } return this.getAlertFromRaw( From 24723259f61e24705cf846a3b6c1e79e0728142a Mon Sep 17 00:00:00 2001 From: Gidi Meir Morris Date: Tue, 15 Sep 2020 17:47:30 +0100 Subject: [PATCH 36/41] fixed unit tests --- .../alerts/server/alerts_client.test.ts | 56 +++---------------- x-pack/plugins/alerts/server/alerts_client.ts | 2 +- 2 files changed, 8 insertions(+), 50 deletions(-) diff --git a/x-pack/plugins/alerts/server/alerts_client.test.ts b/x-pack/plugins/alerts/server/alerts_client.test.ts index 78088ef864edbb..4a674174ec02ec 100644 --- a/x-pack/plugins/alerts/server/alerts_client.test.ts +++ b/x-pack/plugins/alerts/server/alerts_client.test.ts @@ -370,7 +370,7 @@ describe('create()', () => { "updatedBy": "elastic", } `); - expect(unsecuredSavedObjectsClient.create).toHaveBeenCalledTimes(2); + expect(unsecuredSavedObjectsClient.create).toHaveBeenCalledTimes(1); expect(unsecuredSavedObjectsClient.create.mock.calls[0]).toHaveLength(3); expect(unsecuredSavedObjectsClient.create.mock.calls[0][0]).toEqual('alert'); expect(unsecuredSavedObjectsClient.create.mock.calls[0][1]).toMatchInlineSnapshot(` @@ -439,55 +439,13 @@ describe('create()', () => { }, ] `); - expect(unsecuredSavedObjectsClient.create.mock.calls[1]).toHaveLength(3); - expect(unsecuredSavedObjectsClient.create.mock.calls[1][0]).toEqual('alert'); - expect(unsecuredSavedObjectsClient.create.mock.calls[1][1]).toMatchInlineSnapshot(` + expect(unsecuredSavedObjectsClient.update).toHaveBeenCalledTimes(1); + expect(unsecuredSavedObjectsClient.update.mock.calls[0]).toHaveLength(3); + expect(unsecuredSavedObjectsClient.update.mock.calls[0][0]).toEqual('alert'); + expect(unsecuredSavedObjectsClient.update.mock.calls[0][1]).toEqual('1'); + expect(unsecuredSavedObjectsClient.update.mock.calls[0][2]).toMatchInlineSnapshot(` Object { - "actions": Array [ - Object { - "actionRef": "action_0", - "actionTypeId": "test", - "group": "default", - "params": Object { - "foo": true, - }, - }, - ], - "alertTypeId": "123", - "apiKey": null, - "apiKeyOwner": null, - "consumer": "bar", - "createdAt": "2019-02-12T21:01:22.479Z", - "createdBy": "elastic", - "enabled": true, - "muteAll": false, - "mutedInstanceIds": Array [], - "name": "abc", - "params": Object { - "bar": true, - }, - "schedule": Object { - "interval": "10s", - }, "scheduledTaskId": "task-123", - "tags": Array [ - "foo", - ], - "throttle": null, - "updatedBy": "elastic", - } - `); - expect(unsecuredSavedObjectsClient.create.mock.calls[1][2]).toMatchInlineSnapshot(` - Object { - "id": "1", - "overwrite": true, - "references": Array [ - Object { - "id": "1", - "name": "action_0", - "type": "action", - }, - ], } `); }); @@ -3757,7 +3715,7 @@ describe('update()', () => { }, ], }); - unsecuredSavedObjectsClient.update.mockResolvedValueOnce({ + unsecuredSavedObjectsClient.create.mockResolvedValueOnce({ id: '1', type: 'alert', attributes: { diff --git a/x-pack/plugins/alerts/server/alerts_client.ts b/x-pack/plugins/alerts/server/alerts_client.ts index 4f374371dd880d..f4c0057f6d8f74 100644 --- a/x-pack/plugins/alerts/server/alerts_client.ts +++ b/x-pack/plugins/alerts/server/alerts_client.ts @@ -375,7 +375,7 @@ export class AlertsClient { } public async delete({ id }: { id: string }) { - let taskIdToRemove: string | undefined; + let taskIdToRemove: string | undefined | null; let apiKeyToInvalidate: string | null | undefined = null; let attributes: RawAlert; From 7780d0fa29b2f4dd0a18ea6fe205b387b264a663 Mon Sep 17 00:00:00 2001 From: Gidi Meir Morris Date: Wed, 16 Sep 2020 13:29:57 +0100 Subject: [PATCH 37/41] fixed emrge conflicts --- .../plugins/actions/server/actions_client.ts | 6 +- .../alerts/server/alerts_client.test.ts | 180 +++++------------- x-pack/plugins/alerts/server/alerts_client.ts | 105 ++++------ 3 files changed, 88 insertions(+), 203 deletions(-) diff --git a/x-pack/plugins/actions/server/actions_client.ts b/x-pack/plugins/actions/server/actions_client.ts index 2a63bf68615965..01015f9046654c 100644 --- a/x-pack/plugins/actions/server/actions_client.ts +++ b/x-pack/plugins/actions/server/actions_client.ts @@ -6,6 +6,7 @@ import Boom from 'boom'; import { ILegacyScopedClusterClient, + SavedObjectsClientContract, SavedObjectAttributes, SavedObject, KibanaRequest, @@ -30,7 +31,6 @@ import { } from './create_execute_function'; import { ActionsAuthorization } from './authorization/actions_authorization'; import { ActionType } from '../common'; -import { SavedObjectsClientWithoutUpdates } from './saved_objects'; import { shouldLegacyRbacApplyBySource } from './authorization/should_legacy_rbac_apply_by_source'; // We are assuming there won't be many actions. This is why we will load @@ -56,7 +56,7 @@ interface ConstructorOptions { defaultKibanaIndex: string; scopedClusterClient: ILegacyScopedClusterClient; actionTypeRegistry: ActionTypeRegistry; - unsecuredSavedObjectsClient: SavedObjectsClientWithoutUpdates; + unsecuredSavedObjectsClient: SavedObjectsClientContract; preconfiguredActions: PreConfiguredAction[]; actionExecutor: ActionExecutorContract; executionEnqueuer: ExecutionEnqueuer; @@ -72,7 +72,7 @@ interface UpdateOptions { export class ActionsClient { private readonly defaultKibanaIndex: string; private readonly scopedClusterClient: ILegacyScopedClusterClient; - private readonly unsecuredSavedObjectsClient: SavedObjectsClientWithoutUpdates; + private readonly unsecuredSavedObjectsClient: SavedObjectsClientContract; private readonly actionTypeRegistry: ActionTypeRegistry; private readonly preconfiguredActions: PreConfiguredAction[]; private readonly actionExecutor: ActionExecutorContract; diff --git a/x-pack/plugins/alerts/server/alerts_client.test.ts b/x-pack/plugins/alerts/server/alerts_client.test.ts index 2c58134a716265..d54a95edc8bb92 100644 --- a/x-pack/plugins/alerts/server/alerts_client.test.ts +++ b/x-pack/plugins/alerts/server/alerts_client.test.ts @@ -19,7 +19,6 @@ import { encryptedSavedObjectsMock } from '../../encrypted_saved_objects/server/ import { actionsClientMock, actionsAuthorizationMock } from '../../actions/server/mocks'; import { AlertsAuthorization } from './authorization/alerts_authorization'; import { ActionsAuthorization } from '../../actions/server'; -import { pick } from 'lodash'; import { eventLogClientMock } from '../../event_log/server/mocks'; import { QueryEventsBySavedObjectResult } from '../../event_log/server'; import { SavedObject } from 'kibana/server'; @@ -1279,8 +1278,9 @@ describe('enable()', () => { }); expect(alertsClientParams.invalidateAPIKey).not.toHaveBeenCalled(); expect(alertsClientParams.createAPIKey).toHaveBeenCalled(); - expect(unsecuredSavedObjectsClient.create).toHaveBeenCalledWith( + expect(unsecuredSavedObjectsClient.update).toHaveBeenCalledWith( 'alert', + '1', { schedule: { interval: '10s' }, alertTypeId: 'myType', @@ -1305,10 +1305,7 @@ describe('enable()', () => { ], }, { - id: '1', - overwrite: true, version: '123', - references: existingAlert.references, } ); expect(taskManager.schedule).toHaveBeenCalledWith({ @@ -1324,23 +1321,9 @@ describe('enable()', () => { }, scope: ['alerting'], }); - expect(unsecuredSavedObjectsClient.create).toHaveBeenCalledWith( - 'alert', - { - ...existingAlert.attributes, - enabled: true, - apiKey: null, - apiKeyOwner: null, - updatedBy: 'elastic', - scheduledTaskId: 'task-123', - }, - { - id: '1', - overwrite: true, - version: '123', - references: existingAlert.references, - } - ); + expect(unsecuredSavedObjectsClient.update).toHaveBeenCalledWith('alert', '1', { + scheduledTaskId: 'task-123', + }); }); test('invalidates API key if ever one existed prior to updating', async () => { @@ -1383,8 +1366,9 @@ describe('enable()', () => { }); await alertsClient.enable({ id: '1' }); - expect(unsecuredSavedObjectsClient.create).toHaveBeenCalledWith( + expect(unsecuredSavedObjectsClient.update).toHaveBeenCalledWith( 'alert', + '1', { schedule: { interval: '10s' }, alertTypeId: 'myType', @@ -1409,9 +1393,6 @@ describe('enable()', () => { ], }, { - id: '1', - overwrite: true, - references: [], version: '123', } ); @@ -1436,33 +1417,33 @@ describe('enable()', () => { ); expect(alertsClientParams.getUserName).not.toHaveBeenCalled(); expect(alertsClientParams.createAPIKey).not.toHaveBeenCalled(); - expect(unsecuredSavedObjectsClient.create).not.toHaveBeenCalled(); + expect(unsecuredSavedObjectsClient.update).not.toHaveBeenCalled(); expect(taskManager.schedule).not.toHaveBeenCalled(); }); test('throws error when failing to update the first time', async () => { - unsecuredSavedObjectsClient.create.mockReset(); - unsecuredSavedObjectsClient.create.mockRejectedValueOnce(new Error('Fail to update')); + unsecuredSavedObjectsClient.update.mockReset(); + unsecuredSavedObjectsClient.update.mockRejectedValueOnce(new Error('Fail to update')); await expect(alertsClient.enable({ id: '1' })).rejects.toThrowErrorMatchingInlineSnapshot( `"Fail to update"` ); expect(alertsClientParams.getUserName).toHaveBeenCalled(); expect(alertsClientParams.createAPIKey).toHaveBeenCalled(); - expect(unsecuredSavedObjectsClient.create).toHaveBeenCalledTimes(1); + expect(unsecuredSavedObjectsClient.update).toHaveBeenCalledTimes(1); expect(taskManager.schedule).not.toHaveBeenCalled(); }); test('throws error when failing to update the second time', async () => { - unsecuredSavedObjectsClient.create.mockReset(); - unsecuredSavedObjectsClient.create.mockResolvedValueOnce({ + unsecuredSavedObjectsClient.update.mockReset(); + unsecuredSavedObjectsClient.update.mockResolvedValueOnce({ ...existingAlert, attributes: { ...existingAlert.attributes, enabled: true, }, }); - unsecuredSavedObjectsClient.create.mockRejectedValueOnce( + unsecuredSavedObjectsClient.update.mockRejectedValueOnce( new Error('Fail to update second time') ); @@ -1471,7 +1452,7 @@ describe('enable()', () => { ); expect(alertsClientParams.getUserName).toHaveBeenCalled(); expect(alertsClientParams.createAPIKey).toHaveBeenCalled(); - expect(unsecuredSavedObjectsClient.create).toHaveBeenCalledTimes(2); + expect(unsecuredSavedObjectsClient.update).toHaveBeenCalledTimes(2); expect(taskManager.schedule).toHaveBeenCalled(); }); @@ -1483,7 +1464,7 @@ describe('enable()', () => { ); expect(alertsClientParams.getUserName).toHaveBeenCalled(); expect(alertsClientParams.createAPIKey).toHaveBeenCalled(); - expect(unsecuredSavedObjectsClient.create).toHaveBeenCalled(); + expect(unsecuredSavedObjectsClient.update).toHaveBeenCalled(); }); }); @@ -1555,8 +1536,9 @@ describe('disable()', () => { expect(encryptedSavedObjects.getDecryptedAsInternalUser).toHaveBeenCalledWith('alert', '1', { namespace: 'default', }); - expect(unsecuredSavedObjectsClient.create).toHaveBeenCalledWith( + expect(unsecuredSavedObjectsClient.update).toHaveBeenCalledWith( 'alert', + '1', { consumer: 'myApp', schedule: { interval: '10s' }, @@ -1566,6 +1548,8 @@ describe('disable()', () => { versionApiKeyLastmodified: kibanaVersion, }, scheduledTaskId: null, + apiKey: null, + apiKeyOwner: null, updatedBy: 'elastic', actions: [ { @@ -1580,9 +1564,6 @@ describe('disable()', () => { ], }, { - id: '1', - overwrite: true, - references: [], version: '123', } ); @@ -1598,8 +1579,9 @@ describe('disable()', () => { expect(encryptedSavedObjects.getDecryptedAsInternalUser).toHaveBeenCalledWith('alert', '1', { namespace: 'default', }); - expect(unsecuredSavedObjectsClient.create).toHaveBeenCalledWith( + expect(unsecuredSavedObjectsClient.update).toHaveBeenCalledWith( 'alert', + '1', { consumer: 'myApp', schedule: { interval: '10s' }, @@ -1609,6 +1591,8 @@ describe('disable()', () => { versionApiKeyLastmodified: kibanaVersion, }, scheduledTaskId: null, + apiKey: null, + apiKeyOwner: null, updatedBy: 'elastic', actions: [ { @@ -1623,10 +1607,7 @@ describe('disable()', () => { ], }, { - id: '1', - overwrite: true, version: '123', - references: [], } ); expect(taskManager.remove).toHaveBeenCalledWith('task-123'); @@ -1644,7 +1625,7 @@ describe('disable()', () => { }); await alertsClient.disable({ id: '1' }); - expect(unsecuredSavedObjectsClient.create).not.toHaveBeenCalled(); + expect(unsecuredSavedObjectsClient.update).not.toHaveBeenCalled(); expect(taskManager.remove).not.toHaveBeenCalled(); expect(alertsClientParams.invalidateAPIKey).not.toHaveBeenCalled(); }); @@ -1660,7 +1641,7 @@ describe('disable()', () => { encryptedSavedObjects.getDecryptedAsInternalUser.mockRejectedValueOnce(new Error('Fail')); await alertsClient.disable({ id: '1' }); - expect(unsecuredSavedObjectsClient.create).toHaveBeenCalled(); + expect(unsecuredSavedObjectsClient.update).toHaveBeenCalled(); expect(taskManager.remove).toHaveBeenCalled(); expect(alertsClientParams.invalidateAPIKey).not.toHaveBeenCalled(); expect(alertsClientParams.logger.error).toHaveBeenCalledWith( @@ -1669,7 +1650,7 @@ describe('disable()', () => { }); test('throws when unsecuredSavedObjectsClient update fails', async () => { - unsecuredSavedObjectsClient.create.mockRejectedValueOnce(new Error('Failed to update')); + unsecuredSavedObjectsClient.update.mockRejectedValueOnce(new Error('Failed to update')); await expect(alertsClient.disable({ id: '1' })).rejects.toThrowErrorMatchingInlineSnapshot( `"Failed to update"` @@ -1718,30 +1699,11 @@ describe('muteAll()', () => { }); await alertsClient.muteAll({ id: '1' }); - expect(unsecuredSavedObjectsClient.create).toHaveBeenCalledWith( - 'alert', - { - actions: [ - { - group: 'default', - id: '1', - actionTypeId: '1', - actionRef: '1', - params: { - foo: true, - }, - }, - ], - muteAll: true, - mutedInstanceIds: [], - updatedBy: 'elastic', - }, - { - id: '1', - overwrite: true, - references: [], - } - ); + expect(unsecuredSavedObjectsClient.update).toHaveBeenCalledWith('alert', '1', { + muteAll: true, + mutedInstanceIds: [], + updatedBy: 'elastic', + }); }); describe('authorization', () => { @@ -1823,31 +1785,11 @@ describe('unmuteAll()', () => { }); await alertsClient.unmuteAll({ id: '1' }); - expect(unsecuredSavedObjectsClient.create).toHaveBeenCalledWith( - 'alert', - { - actions: [ - { - group: 'default', - id: '1', - actionTypeId: '1', - actionRef: '1', - params: { - foo: true, - }, - }, - ], - muteAll: false, - mutedInstanceIds: [], - updatedBy: 'elastic', - }, - { - id: '1', - overwrite: true, - version: '123', - references: [], - } - ); + expect(unsecuredSavedObjectsClient.update).toHaveBeenCalledWith('alert', '1', { + muteAll: false, + mutedInstanceIds: [], + updatedBy: 'elastic', + }); }); describe('authorization', () => { @@ -1923,22 +1865,15 @@ describe('muteInstance()', () => { }); await alertsClient.muteInstance({ alertId: '1', alertInstanceId: '2' }); - expect(unsecuredSavedObjectsClient.create).toHaveBeenCalledWith( + expect(unsecuredSavedObjectsClient.update).toHaveBeenCalledWith( 'alert', + '1', { - actions: [], - schedule: { interval: '10s' }, - alertTypeId: '2', - enabled: true, - scheduledTaskId: 'task-123', mutedInstanceIds: ['2'], updatedBy: 'elastic', }, { - id: '1', - overwrite: true, version: '123', - references: [], } ); }); @@ -2065,23 +2000,14 @@ describe('unmuteInstance()', () => { }); await alertsClient.unmuteInstance({ alertId: '1', alertInstanceId: '2' }); - expect(unsecuredSavedObjectsClient.create).toHaveBeenCalledWith( + expect(unsecuredSavedObjectsClient.update).toHaveBeenCalledWith( 'alert', + '1', { - actions: [], - schedule: { interval: '10s' }, - alertTypeId: '2', - enabled: true, - scheduledTaskId: 'task-123', mutedInstanceIds: [], updatedBy: 'elastic', }, - { - id: '1', - overwrite: true, - version: '123', - references: [], - } + { version: '123' } ); }); @@ -4328,8 +4254,9 @@ describe('updateApiKey()', () => { expect(encryptedSavedObjects.getDecryptedAsInternalUser).toHaveBeenCalledWith('alert', '1', { namespace: 'default', }); - expect(unsecuredSavedObjectsClient.create).toHaveBeenCalledWith( + expect(unsecuredSavedObjectsClient.update).toHaveBeenCalledWith( 'alert', + '1', { schedule: { interval: '10s' }, alertTypeId: 'myType', @@ -4353,10 +4280,7 @@ describe('updateApiKey()', () => { versionApiKeyLastmodified: kibanaVersion, }, }, - { - overwrite: true, - ...pick(existingAlert, 'id', 'version', 'references'), - } + { version: '123' } ); expect(alertsClientParams.invalidateAPIKey).toHaveBeenCalledWith({ id: '123' }); }); @@ -4369,8 +4293,9 @@ describe('updateApiKey()', () => { expect(encryptedSavedObjects.getDecryptedAsInternalUser).toHaveBeenCalledWith('alert', '1', { namespace: 'default', }); - expect(unsecuredSavedObjectsClient.create).toHaveBeenCalledWith( + expect(unsecuredSavedObjectsClient.update).toHaveBeenCalledWith( 'alert', + '1', { schedule: { interval: '10s' }, alertTypeId: 'myType', @@ -4394,10 +4319,7 @@ describe('updateApiKey()', () => { versionApiKeyLastmodified: kibanaVersion, }, }, - { - overwrite: true, - ...pick(existingAlert, 'id', 'version', 'references'), - } + { version: '123' } ); expect(alertsClientParams.invalidateAPIKey).not.toHaveBeenCalled(); }); @@ -4409,7 +4331,7 @@ describe('updateApiKey()', () => { expect(alertsClientParams.logger.error).toHaveBeenCalledWith( 'Failed to invalidate API Key: Fail' ); - expect(unsecuredSavedObjectsClient.create).toHaveBeenCalled(); + expect(unsecuredSavedObjectsClient.update).toHaveBeenCalled(); }); test('swallows error when getting decrypted object throws', async () => { @@ -4419,12 +4341,12 @@ describe('updateApiKey()', () => { expect(alertsClientParams.logger.error).toHaveBeenCalledWith( 'updateApiKey(): Failed to load API key to invalidate on alert 1: Fail' ); - expect(unsecuredSavedObjectsClient.create).toHaveBeenCalled(); + expect(unsecuredSavedObjectsClient.update).toHaveBeenCalled(); expect(alertsClientParams.invalidateAPIKey).not.toHaveBeenCalled(); }); test('throws when unsecuredSavedObjectsClient update fails', async () => { - unsecuredSavedObjectsClient.create.mockRejectedValueOnce(new Error('Fail')); + unsecuredSavedObjectsClient.update.mockRejectedValueOnce(new Error('Fail')); await expect(alertsClient.updateApiKey({ id: '1' })).rejects.toThrowErrorMatchingInlineSnapshot( `"Fail"` diff --git a/x-pack/plugins/alerts/server/alerts_client.ts b/x-pack/plugins/alerts/server/alerts_client.ts index 71ca28606a922d..585d616deef81c 100644 --- a/x-pack/plugins/alerts/server/alerts_client.ts +++ b/x-pack/plugins/alerts/server/alerts_client.ts @@ -5,7 +5,7 @@ */ import Boom from 'boom'; -import { omit, omitBy, isUndefined, isEqual, map, uniq, pick, truncate, trim } from 'lodash'; +import { omit, isEqual, map, uniq, pick, truncate, trim } from 'lodash'; import { i18n } from '@kbn/i18n'; import { Logger, @@ -534,7 +534,6 @@ export class AlertsClient { public async updateApiKey({ id }: { id: string }) { let apiKeyToInvalidate: string | null | undefined = null; let attributes: RawAlert; - let references: SavedObjectReference[]; let version: string | undefined; try { @@ -544,7 +543,6 @@ export class AlertsClient { apiKeyToInvalidate = decryptedAlert.attributes.apiKey; attributes = decryptedAlert.attributes; version = decryptedAlert.version; - references = decryptedAlert.references; } catch (e) { // We'll skip invalidating the API key since we failed to load the decrypted saved object this.logger.error( @@ -554,7 +552,6 @@ export class AlertsClient { const alert = await this.unsecuredSavedObjectsClient.get('alert', id); attributes = alert.attributes; version = alert.version; - references = alert.references; } await this.authorization.ensureAuthorized( attributes.alertTypeId, @@ -567,8 +564,9 @@ export class AlertsClient { } const username = await this.getUserName(); - await this.unsecuredSavedObjectsClient.create( + await this.unsecuredSavedObjectsClient.update( 'alert', + id, this.updateMeta({ ...attributes, ...this.apiKeyAsAlertAttributes( @@ -577,15 +575,7 @@ export class AlertsClient { ), updatedBy: username, }), - omitBy( - { - id, - overwrite: true, - version, - references, - }, - isUndefined - ) + { version } ); if (apiKeyToInvalidate) { @@ -610,10 +600,9 @@ export class AlertsClient { } public async enable({ id }: { id: string }) { - let apiKeyToInvalidate: string | null | undefined = null; + let apiKeyToInvalidate: string | undefined | null = null; let attributes: RawAlert; let version: string | undefined; - let references: SavedObjectReference[]; try { const decryptedAlert = await this.encryptedSavedObjectsClient.getDecryptedAsInternalUser< @@ -622,7 +611,6 @@ export class AlertsClient { apiKeyToInvalidate = decryptedAlert.attributes.apiKey; attributes = decryptedAlert.attributes; version = decryptedAlert.version; - references = decryptedAlert.references; } catch (e) { // We'll skip invalidating the API key since we failed to load the decrypted saved object this.logger.error( @@ -632,7 +620,6 @@ export class AlertsClient { const alert = await this.unsecuredSavedObjectsClient.get('alert', id); attributes = alert.attributes; version = alert.version; - references = alert.references; } await this.authorization.ensureAuthorized( @@ -647,27 +634,21 @@ export class AlertsClient { if (attributes.enabled === false) { const username = await this.getUserName(); - const updatedAttributes = { - ...attributes, - enabled: true, - ...this.apiKeyAsAlertAttributes( - await this.createAPIKey(this.generateAPIKeyName(attributes.alertTypeId, attributes.name)), - username - ), - updatedBy: username, - }; - const updatedAlert = await this.unsecuredSavedObjectsClient.create( + await this.unsecuredSavedObjectsClient.update( 'alert', - updatedAttributes, - omitBy( - { - id, - overwrite: true, - version, - references, - }, - isUndefined - ) + id, + this.updateMeta({ + ...attributes, + enabled: true, + ...this.apiKeyAsAlertAttributes( + await this.createAPIKey( + this.generateAPIKeyName(attributes.alertTypeId, attributes.name) + ), + username + ), + updatedBy: username, + }), + { version } ); const scheduledTask = await this.scheduleAlert(id, attributes.alertTypeId); await this.unsecuredSavedObjectsClient.update('alert', id, { @@ -683,7 +664,6 @@ export class AlertsClient { let apiKeyToInvalidate: string | null | undefined = null; let attributes: RawAlert; let version: string | undefined; - let references: SavedObjectReference[]; try { const decryptedAlert = await this.encryptedSavedObjectsClient.getDecryptedAsInternalUser< @@ -692,7 +672,6 @@ export class AlertsClient { apiKeyToInvalidate = decryptedAlert.attributes.apiKey; attributes = decryptedAlert.attributes; version = decryptedAlert.version; - references = decryptedAlert.references; } catch (e) { // We'll skip invalidating the API key since we failed to load the decrypted saved object this.logger.error( @@ -702,7 +681,6 @@ export class AlertsClient { const alert = await this.unsecuredSavedObjectsClient.get('alert', id); attributes = alert.attributes; version = alert.version; - references = alert.references; } await this.authorization.ensureAuthorized( @@ -712,27 +690,18 @@ export class AlertsClient { ); if (attributes.enabled === true) { - await this.unsecuredSavedObjectsClient.create( + await this.unsecuredSavedObjectsClient.update( 'alert', + id, this.updateMeta({ - ...(omit( - attributes, - 'scheduledTaskId', - 'apiKey', - 'apiKeyOwner' - ) as AlertUpdateRequiredFields), + ...attributes, enabled: false, + scheduledTaskId: null, + apiKey: null, + apiKeyOwner: null, updatedBy: await this.getUserName(), }), - omitBy( - { - id, - overwrite: true, - version, - references, - }, - isUndefined - ) + { version } ); await Promise.all([ @@ -745,11 +714,9 @@ export class AlertsClient { } public async muteAll({ id }: { id: string }) { - const { - attributes, - references, - version, - } = await this.encryptedSavedObjectsClient.getDecryptedAsInternalUser('alert', id, { + const { attributes } = await this.encryptedSavedObjectsClient.getDecryptedAsInternalUser< + RawAlert + >('alert', id, { namespace: this.namespace, }); await this.authorization.ensureAuthorized( @@ -774,11 +741,9 @@ export class AlertsClient { } public async unmuteAll({ id }: { id: string }) { - const { - attributes, - references, - version, - } = await this.encryptedSavedObjectsClient.getDecryptedAsInternalUser('alert', id, { + const { attributes } = await this.encryptedSavedObjectsClient.getDecryptedAsInternalUser< + RawAlert + >('alert', id, { namespace: this.namespace, }); await this.authorization.ensureAuthorized( @@ -805,7 +770,6 @@ export class AlertsClient { public async muteInstance({ alertId, alertInstanceId }: MuteOptions) { const { attributes, - references, version, } = await this.encryptedSavedObjectsClient.getDecryptedAsInternalUser( 'alert', @@ -828,7 +792,7 @@ export class AlertsClient { const mutedInstanceIds = attributes.mutedInstanceIds || []; if (!attributes.muteAll && !mutedInstanceIds.includes(alertInstanceId)) { mutedInstanceIds.push(alertInstanceId); - await this.unsecuredSavedObjectsClient.create( + await this.unsecuredSavedObjectsClient.update( 'alert', alertId, this.updateMeta({ @@ -849,7 +813,6 @@ export class AlertsClient { }) { const { attributes, - references, version, } = await this.encryptedSavedObjectsClient.getDecryptedAsInternalUser( 'alert', @@ -869,7 +832,7 @@ export class AlertsClient { const mutedInstanceIds = attributes.mutedInstanceIds || []; if (!attributes.muteAll && mutedInstanceIds.includes(alertInstanceId)) { - await this.unsecuredSavedObjectsClient.create( + await this.unsecuredSavedObjectsClient.update( 'alert', alertId, this.updateMeta({ From 0e797f569619dc0f31cdf9475d4a753dcaa97350 Mon Sep 17 00:00:00 2001 From: Gidi Meir Morris Date: Wed, 16 Sep 2020 13:51:49 +0100 Subject: [PATCH 38/41] removed unused type --- x-pack/plugins/alerts/server/alerts_client.ts | 1 - x-pack/plugins/alerts/server/types.ts | 18 ------------------ 2 files changed, 19 deletions(-) diff --git a/x-pack/plugins/alerts/server/alerts_client.ts b/x-pack/plugins/alerts/server/alerts_client.ts index 585d616deef81c..bf9754a8100604 100644 --- a/x-pack/plugins/alerts/server/alerts_client.ts +++ b/x-pack/plugins/alerts/server/alerts_client.ts @@ -26,7 +26,6 @@ import { IntervalSchedule, SanitizedAlert, AlertTaskState, - AlertUpdateRequiredFields, AlertInstanceSummary, } from './types'; import { validateAlertTypeParams } from './lib'; diff --git a/x-pack/plugins/alerts/server/types.ts b/x-pack/plugins/alerts/server/types.ts index 3397fcd36d0dbd..08ef1cca5eb339 100644 --- a/x-pack/plugins/alerts/server/types.ts +++ b/x-pack/plugins/alerts/server/types.ts @@ -138,24 +138,6 @@ export interface RawAlert extends SavedObjectAttributes { meta?: AlertMeta; } -export type AlertUpdateRequiredFields = Pick< - RawAlert, - | 'enabled' - | 'name' - | 'tags' - | 'alertTypeId' - | 'consumer' - | 'schedule' - | 'actions' - | 'params' - | 'createdBy' - | 'updatedBy' - | 'createdAt' - | 'throttle' - | 'muteAll' - | 'mutedInstanceIds' ->; - export type AlertInfoParams = Pick< RawAlert, | 'params' From fec5cdd9d916f37b2ce6b76ad34a0ce3ed207b88 Mon Sep 17 00:00:00 2001 From: Gidi Meir Morris Date: Wed, 16 Sep 2020 17:01:32 +0100 Subject: [PATCH 39/41] removed decryption from operations that dont need it --- .../alerts/server/alerts_client.test.ts | 24 +++++++------- x-pack/plugins/alerts/server/alerts_client.ts | 33 ++++--------------- 2 files changed, 19 insertions(+), 38 deletions(-) diff --git a/x-pack/plugins/alerts/server/alerts_client.test.ts b/x-pack/plugins/alerts/server/alerts_client.test.ts index d54a95edc8bb92..a6cffb02848159 100644 --- a/x-pack/plugins/alerts/server/alerts_client.test.ts +++ b/x-pack/plugins/alerts/server/alerts_client.test.ts @@ -1678,7 +1678,7 @@ describe('disable()', () => { describe('muteAll()', () => { test('mutes an alert', async () => { const alertsClient = new AlertsClient(alertsClientParams); - encryptedSavedObjects.getDecryptedAsInternalUser.mockResolvedValueOnce({ + unsecuredSavedObjectsClient.get.mockResolvedValueOnce({ id: '1', type: 'alert', attributes: { @@ -1708,7 +1708,7 @@ describe('muteAll()', () => { describe('authorization', () => { beforeEach(() => { - encryptedSavedObjects.getDecryptedAsInternalUser.mockResolvedValueOnce({ + unsecuredSavedObjectsClient.get.mockResolvedValueOnce({ id: '1', type: 'alert', attributes: { @@ -1763,7 +1763,7 @@ describe('muteAll()', () => { describe('unmuteAll()', () => { test('unmutes an alert', async () => { const alertsClient = new AlertsClient(alertsClientParams); - encryptedSavedObjects.getDecryptedAsInternalUser.mockResolvedValueOnce({ + unsecuredSavedObjectsClient.get.mockResolvedValueOnce({ id: '1', type: 'alert', attributes: { @@ -1794,7 +1794,7 @@ describe('unmuteAll()', () => { describe('authorization', () => { beforeEach(() => { - encryptedSavedObjects.getDecryptedAsInternalUser.mockResolvedValueOnce({ + unsecuredSavedObjectsClient.get.mockResolvedValueOnce({ id: '1', type: 'alert', attributes: { @@ -1849,7 +1849,7 @@ describe('unmuteAll()', () => { describe('muteInstance()', () => { test('mutes an alert instance', async () => { const alertsClient = new AlertsClient(alertsClientParams); - encryptedSavedObjects.getDecryptedAsInternalUser.mockResolvedValueOnce({ + unsecuredSavedObjectsClient.get.mockResolvedValueOnce({ id: '1', type: 'alert', attributes: { @@ -1880,7 +1880,7 @@ describe('muteInstance()', () => { test('skips muting when alert instance already muted', async () => { const alertsClient = new AlertsClient(alertsClientParams); - encryptedSavedObjects.getDecryptedAsInternalUser.mockResolvedValueOnce({ + unsecuredSavedObjectsClient.get.mockResolvedValueOnce({ id: '1', type: 'alert', attributes: { @@ -1900,7 +1900,7 @@ describe('muteInstance()', () => { test('skips muting when alert is muted', async () => { const alertsClient = new AlertsClient(alertsClientParams); - encryptedSavedObjects.getDecryptedAsInternalUser.mockResolvedValueOnce({ + unsecuredSavedObjectsClient.get.mockResolvedValueOnce({ id: '1', type: 'alert', attributes: { @@ -1921,7 +1921,7 @@ describe('muteInstance()', () => { describe('authorization', () => { beforeEach(() => { - encryptedSavedObjects.getDecryptedAsInternalUser.mockResolvedValueOnce({ + unsecuredSavedObjectsClient.get.mockResolvedValueOnce({ id: '1', type: 'alert', attributes: { @@ -1984,7 +1984,7 @@ describe('muteInstance()', () => { describe('unmuteInstance()', () => { test('unmutes an alert instance', async () => { const alertsClient = new AlertsClient(alertsClientParams); - encryptedSavedObjects.getDecryptedAsInternalUser.mockResolvedValueOnce({ + unsecuredSavedObjectsClient.get.mockResolvedValueOnce({ id: '1', type: 'alert', attributes: { @@ -2013,7 +2013,7 @@ describe('unmuteInstance()', () => { test('skips unmuting when alert instance not muted', async () => { const alertsClient = new AlertsClient(alertsClientParams); - encryptedSavedObjects.getDecryptedAsInternalUser.mockResolvedValueOnce({ + unsecuredSavedObjectsClient.get.mockResolvedValueOnce({ id: '1', type: 'alert', attributes: { @@ -2033,7 +2033,7 @@ describe('unmuteInstance()', () => { test('skips unmuting when alert is muted', async () => { const alertsClient = new AlertsClient(alertsClientParams); - encryptedSavedObjects.getDecryptedAsInternalUser.mockResolvedValueOnce({ + unsecuredSavedObjectsClient.get.mockResolvedValueOnce({ id: '1', type: 'alert', attributes: { @@ -2054,7 +2054,7 @@ describe('unmuteInstance()', () => { describe('authorization', () => { beforeEach(() => { - encryptedSavedObjects.getDecryptedAsInternalUser.mockResolvedValueOnce({ + unsecuredSavedObjectsClient.get.mockResolvedValueOnce({ id: '1', type: 'alert', attributes: { diff --git a/x-pack/plugins/alerts/server/alerts_client.ts b/x-pack/plugins/alerts/server/alerts_client.ts index bf9754a8100604..f73e69510c1627 100644 --- a/x-pack/plugins/alerts/server/alerts_client.ts +++ b/x-pack/plugins/alerts/server/alerts_client.ts @@ -713,11 +713,7 @@ export class AlertsClient { } public async muteAll({ id }: { id: string }) { - const { attributes } = await this.encryptedSavedObjectsClient.getDecryptedAsInternalUser< - RawAlert - >('alert', id, { - namespace: this.namespace, - }); + const { attributes } = await this.unsecuredSavedObjectsClient.get('alert', id); await this.authorization.ensureAuthorized( attributes.alertTypeId, attributes.consumer, @@ -740,11 +736,7 @@ export class AlertsClient { } public async unmuteAll({ id }: { id: string }) { - const { attributes } = await this.encryptedSavedObjectsClient.getDecryptedAsInternalUser< - RawAlert - >('alert', id, { - namespace: this.namespace, - }); + const { attributes } = await this.unsecuredSavedObjectsClient.get('alert', id); await this.authorization.ensureAuthorized( attributes.alertTypeId, attributes.consumer, @@ -767,15 +759,9 @@ export class AlertsClient { } public async muteInstance({ alertId, alertInstanceId }: MuteOptions) { - const { - attributes, - version, - } = await this.encryptedSavedObjectsClient.getDecryptedAsInternalUser( + const { attributes, version } = await this.unsecuredSavedObjectsClient.get( 'alert', - alertId, - { - namespace: this.namespace, - } + alertId ); await this.authorization.ensureAuthorized( @@ -810,16 +796,11 @@ export class AlertsClient { alertId: string; alertInstanceId: string; }) { - const { - attributes, - version, - } = await this.encryptedSavedObjectsClient.getDecryptedAsInternalUser( + const { attributes, version } = await this.unsecuredSavedObjectsClient.get( 'alert', - alertId, - { - namespace: this.namespace, - } + alertId ); + await this.authorization.ensureAuthorized( attributes.alertTypeId, attributes.consumer, From e32e091526ccc274c2ac5a7770855fe826115a0f Mon Sep 17 00:00:00 2001 From: Gidi Meir Morris Date: Wed, 16 Sep 2020 17:12:15 +0100 Subject: [PATCH 40/41] reduced unneeded changes --- x-pack/plugins/alerts/server/alerts_client.ts | 9 ++++----- x-pack/plugins/alerts/server/types.ts | 4 ++-- 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/x-pack/plugins/alerts/server/alerts_client.ts b/x-pack/plugins/alerts/server/alerts_client.ts index f73e69510c1627..b63fe3cf29e489 100644 --- a/x-pack/plugins/alerts/server/alerts_client.ts +++ b/x-pack/plugins/alerts/server/alerts_client.ts @@ -216,10 +216,9 @@ export class AlertsClient { this.validateActions(alertType, data.actions); const { references, actions } = await this.denormalizeActions(data.actions); - const encryptedAttributes = this.apiKeyAsAlertAttributes(createdAPIKey, username); const rawAlert: RawAlert = { ...data, - ...encryptedAttributes, + ...this.apiKeyAsAlertAttributes(createdAPIKey, username), actions, createdBy: username, updatedBy: username, @@ -384,7 +383,7 @@ export class AlertsClient { public async delete({ id }: { id: string }) { let taskIdToRemove: string | undefined | null; - let apiKeyToInvalidate: string | null | undefined = null; + let apiKeyToInvalidate: string | null = null; let attributes: RawAlert; try { @@ -531,7 +530,7 @@ export class AlertsClient { } public async updateApiKey({ id }: { id: string }) { - let apiKeyToInvalidate: string | null | undefined = null; + let apiKeyToInvalidate: string | null = null; let attributes: RawAlert; let version: string | undefined; @@ -660,7 +659,7 @@ export class AlertsClient { } public async disable({ id }: { id: string }) { - let apiKeyToInvalidate: string | null | undefined = null; + let apiKeyToInvalidate: string | null = null; let attributes: RawAlert; let version: string | undefined; diff --git a/x-pack/plugins/alerts/server/types.ts b/x-pack/plugins/alerts/server/types.ts index 08ef1cca5eb339..8d568e8b7ecd1b 100644 --- a/x-pack/plugins/alerts/server/types.ts +++ b/x-pack/plugins/alerts/server/types.ts @@ -130,8 +130,8 @@ export interface RawAlert extends SavedObjectAttributes { createdBy: string | null; updatedBy: string | null; createdAt: string; - apiKey?: string | null; - apiKeyOwner?: string | null; + apiKey: string | null; + apiKeyOwner: string | null; throttle: string | null; muteAll: boolean; mutedInstanceIds: string[]; From 93807e615316ae91f907b6d0edd74dc963f1c865 Mon Sep 17 00:00:00 2001 From: Gidi Meir Morris Date: Wed, 16 Sep 2020 17:13:41 +0100 Subject: [PATCH 41/41] removed unneeded undefined --- x-pack/plugins/alerts/server/alerts_client.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/plugins/alerts/server/alerts_client.ts b/x-pack/plugins/alerts/server/alerts_client.ts index b63fe3cf29e489..671b1d6411d7fc 100644 --- a/x-pack/plugins/alerts/server/alerts_client.ts +++ b/x-pack/plugins/alerts/server/alerts_client.ts @@ -598,7 +598,7 @@ export class AlertsClient { } public async enable({ id }: { id: string }) { - let apiKeyToInvalidate: string | undefined | null = null; + let apiKeyToInvalidate: string | null = null; let attributes: RawAlert; let version: string | undefined;