From fd624b1077d2493b985c94359e71d3c967739e0d Mon Sep 17 00:00:00 2001 From: Gidi Meir Morris Date: Fri, 18 Sep 2020 09:53:35 +0100 Subject: [PATCH] [Alerting & Actions] Overwrite SOs when updating instead of partially updating (#73688) This PR changes the Alerts & Actions clients to ensure they require full updates (rather than partial) to SOs and overwrites the entire document when making the update. This is to prevent the situation where nested objects get _merged_ instead of replaced when a user makes an `update`. We also enhanced the EncryptedSavedObjectsClient to allow specified `id`s when overwriting an existing object. --- .../actions/server/actions_client.test.ts | 28 ++- .../plugins/actions/server/actions_client.ts | 32 ++- .../alerts/server/alerts_client.test.ts | 212 +++++++++++------- x-pack/plugins/alerts/server/alerts_client.ts | 10 +- .../task_runner/create_execution_handler.ts | 5 +- .../alerts/server/task_runner/task_runner.ts | 12 +- ...ypted_saved_objects_client_wrapper.test.ts | 123 ++++++++++ .../encrypted_saved_objects_client_wrapper.ts | 18 +- .../fixtures/plugins/alerts/server/routes.ts | 8 +- .../actions/builtin_action_types/webhook.ts | 71 ++++++ 10 files changed, 402 insertions(+), 117 deletions(-) diff --git a/x-pack/plugins/actions/server/actions_client.test.ts b/x-pack/plugins/actions/server/actions_client.test.ts index 573fb0e1be5809..adef12454f2d52 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,21 @@ 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, + "references": Array [], + }, ] `); expect(unsecuredSavedObjectsClient.get).toHaveBeenCalledTimes(1); @@ -1043,7 +1047,7 @@ describe('update()', () => { }, references: [], }); - unsecuredSavedObjectsClient.update.mockResolvedValueOnce({ + unsecuredSavedObjectsClient.create.mockResolvedValueOnce({ id: 'my-action', type: 'action', attributes: { @@ -1081,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 { @@ -1096,6 +1099,11 @@ describe('update()', () => { "name": "my name", "secrets": Object {}, }, + Object { + "id": "my-action", + "overwrite": true, + "references": Array [], + }, ] `); }); @@ -1118,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 06c9555f3a18d0..01015f9046654c 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 { @@ -151,8 +152,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); @@ -160,12 +163,25 @@ 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.create( + 'action', + { + ...attributes, + actionTypeId, + name, + config: validatedActionTypeConfig as SavedObjectAttributes, + secrets: validatedActionTypeSecrets as SavedObjectAttributes, + }, + omitBy( + { + id, + overwrite: true, + references, + version, + }, + isUndefined + ) + ); return { id, diff --git a/x-pack/plugins/alerts/server/alerts_client.test.ts b/x-pack/plugins/alerts/server/alerts_client.test.ts index 4b5af942024c0c..a6cffb02848159 100644 --- a/x-pack/plugins/alerts/server/alerts_client.test.ts +++ b/x-pack/plugins/alerts/server/alerts_client.test.ts @@ -220,7 +220,7 @@ describe('create()', () => { params: {}, ownerId: null, }); - unsecuredSavedObjectsClient.update.mockResolvedValueOnce({ + unsecuredSavedObjectsClient.create.mockResolvedValueOnce({ id: '1', type: 'alert', attributes: { @@ -270,27 +270,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', @@ -312,11 +318,11 @@ describe('create()', () => { params: {}, ownerId: null, }); - unsecuredSavedObjectsClient.update.mockResolvedValueOnce({ + unsecuredSavedObjectsClient.create.mockResolvedValueOnce({ id: '1', type: 'alert', attributes: { - actions: [], + ...createdAttributes, scheduledTaskId: 'task-123', }, references: [ @@ -342,8 +348,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, }, @@ -351,7 +363,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); @@ -531,7 +548,7 @@ describe('create()', () => { params: {}, ownerId: null, }); - unsecuredSavedObjectsClient.update.mockResolvedValueOnce({ + unsecuredSavedObjectsClient.create.mockResolvedValueOnce({ id: '1', type: 'alert', attributes: { @@ -965,7 +982,7 @@ describe('create()', () => { params: {}, ownerId: null, }); - unsecuredSavedObjectsClient.update.mockResolvedValueOnce({ + unsecuredSavedObjectsClient.create.mockResolvedValueOnce({ id: '1', type: 'alert', attributes: { @@ -1081,7 +1098,7 @@ describe('create()', () => { params: {}, ownerId: null, }); - unsecuredSavedObjectsClient.update.mockResolvedValueOnce({ + unsecuredSavedObjectsClient.create.mockResolvedValueOnce({ id: '1', type: 'alert', attributes: { @@ -1175,6 +1192,16 @@ describe('enable()', () => { alertsClientParams.createAPIKey.mockResolvedValue({ apiKeysEnabled: false, }); + unsecuredSavedObjectsClient.create.mockResolvedValueOnce({ + ...existingAlert, + attributes: { + ...existingAlert.attributes, + enabled: true, + apiKey: null, + apiKeyOwner: null, + updatedBy: 'elastic', + }, + }); taskManager.schedule.mockResolvedValue({ id: 'task-123', scheduledAt: new Date(), @@ -1233,6 +1260,17 @@ describe('enable()', () => { }); test('enables an alert', async () => { + unsecuredSavedObjectsClient.create.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', { @@ -1317,7 +1355,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(); }); @@ -1384,6 +1422,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( @@ -1396,6 +1435,7 @@ describe('enable()', () => { }); test('throws error when failing to update the second time', async () => { + unsecuredSavedObjectsClient.update.mockReset(); unsecuredSavedObjectsClient.update.mockResolvedValueOnce({ ...existingAlert, attributes: { @@ -1460,6 +1500,8 @@ describe('disable()', () => { ...existingAlert.attributes, apiKey: Buffer.from('123:abc').toString('base64'), }, + version: '123', + references: [], }; beforeEach(() => { @@ -1501,13 +1543,13 @@ describe('disable()', () => { consumer: 'myApp', schedule: { interval: '10s' }, alertTypeId: 'myType', - apiKey: null, - apiKeyOwner: null, enabled: false, meta: { versionApiKeyLastmodified: kibanaVersion, }, scheduledTaskId: null, + apiKey: null, + apiKeyOwner: null, updatedBy: 'elastic', actions: [ { @@ -1544,13 +1586,13 @@ describe('disable()', () => { consumer: 'myApp', schedule: { interval: '10s' }, alertTypeId: 'myType', - apiKey: null, - apiKeyOwner: null, enabled: false, meta: { versionApiKeyLastmodified: kibanaVersion, }, scheduledTaskId: null, + apiKey: null, + apiKeyOwner: null, updatedBy: 'elastic', actions: [ { @@ -1739,6 +1781,7 @@ describe('unmuteAll()', () => { muteAll: true, }, references: [], + version: '123', }); await alertsClient.unmuteAll({ id: '1' }); @@ -1829,7 +1872,9 @@ describe('muteInstance()', () => { mutedInstanceIds: ['2'], updatedBy: 'elastic', }, - { version: '123' } + { + version: '123', + } ); }); @@ -1850,7 +1895,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 () => { @@ -1871,7 +1916,7 @@ describe('muteInstance()', () => { }); await alertsClient.muteInstance({ alertId: '1', alertInstanceId: '2' }); - expect(unsecuredSavedObjectsClient.update).not.toHaveBeenCalled(); + expect(unsecuredSavedObjectsClient.create).not.toHaveBeenCalled(); }); describe('authorization', () => { @@ -1983,7 +2028,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 () => { @@ -2004,7 +2049,7 @@ describe('unmuteInstance()', () => { }); await alertsClient.unmuteInstance({ alertId: '1', alertInstanceId: '2' }); - expect(unsecuredSavedObjectsClient.update).not.toHaveBeenCalled(); + expect(unsecuredSavedObjectsClient.create).not.toHaveBeenCalled(); }); describe('authorization', () => { @@ -3052,7 +3097,7 @@ describe('update()', () => { }); test('updates given parameters', async () => { - unsecuredSavedObjectsClient.update.mockResolvedValueOnce({ + unsecuredSavedObjectsClient.create.mockResolvedValueOnce({ id: '1', type: 'alert', attributes: { @@ -3189,11 +3234,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 { @@ -3244,8 +3288,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", @@ -3286,7 +3332,7 @@ describe('update()', () => { apiKeysEnabled: true, result: { id: '123', name: '123', api_key: 'abc' }, }); - unsecuredSavedObjectsClient.update.mockResolvedValueOnce({ + unsecuredSavedObjectsClient.create.mockResolvedValueOnce({ id: '1', type: 'alert', attributes: { @@ -3365,11 +3411,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 { @@ -3404,18 +3449,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 () => { @@ -3439,7 +3486,7 @@ describe('update()', () => { }, ], }); - unsecuredSavedObjectsClient.update.mockResolvedValueOnce({ + unsecuredSavedObjectsClient.create.mockResolvedValueOnce({ id: '1', type: 'alert', attributes: { @@ -3519,11 +3566,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 { @@ -3558,18 +3604,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 () => { @@ -3627,7 +3675,7 @@ describe('update()', () => { }, ], }); - unsecuredSavedObjectsClient.update.mockResolvedValueOnce({ + unsecuredSavedObjectsClient.create.mockResolvedValueOnce({ id: '1', type: 'alert', attributes: { @@ -3686,7 +3734,7 @@ describe('update()', () => { }, ], }); - unsecuredSavedObjectsClient.update.mockResolvedValueOnce({ + unsecuredSavedObjectsClient.create.mockResolvedValueOnce({ id: '1', type: 'alert', attributes: { @@ -3765,7 +3813,7 @@ describe('update()', () => { }, ], }); - unsecuredSavedObjectsClient.update.mockResolvedValueOnce({ + unsecuredSavedObjectsClient.create.mockResolvedValueOnce({ id: '1', type: 'alert', attributes: { @@ -3919,7 +3967,7 @@ describe('update()', () => { params: {}, ownerId: null, }); - unsecuredSavedObjectsClient.update.mockResolvedValueOnce({ + unsecuredSavedObjectsClient.create.mockResolvedValueOnce({ id: alertId, type: 'alert', attributes: { @@ -4091,7 +4139,7 @@ describe('update()', () => { describe('authorization', () => { beforeEach(() => { - 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 0a08ca848c73d7..671b1d6411d7fc 100644 --- a/x-pack/plugins/alerts/server/alerts_client.ts +++ b/x-pack/plugins/alerts/server/alerts_client.ts @@ -251,7 +251,7 @@ export class AlertsClient { } throw e; } - await this.unsecuredSavedObjectsClient.update('alert', createdAlert.id, { + await this.unsecuredSavedObjectsClient.update('alert', createdAlert.id, { scheduledTaskId: scheduledTask.id, }); createdAlert.attributes.scheduledTaskId = scheduledTask.id; @@ -488,9 +488,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, this.updateMeta({ ...attributes, ...data, @@ -500,6 +499,8 @@ export class AlertsClient { updatedBy: username, }), { + id, + overwrite: true, version, references, } @@ -798,6 +799,7 @@ export class AlertsClient { 'alert', alertId ); + await this.authorization.ensureAuthorized( attributes.alertTypeId, attributes.consumer, @@ -809,7 +811,7 @@ export class AlertsClient { const mutedInstanceIds = attributes.mutedInstanceIds || []; if (!attributes.muteAll && mutedInstanceIds.includes(alertInstanceId)) { - await this.unsecuredSavedObjectsClient.update( + await this.unsecuredSavedObjectsClient.update( 'alert', alertId, this.updateMeta({ 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 f873b0178ece99..aca447b6adedd7 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 @@ -19,6 +19,7 @@ import { AlertInstanceContext, AlertType, AlertTypeParams, + RawAlert, } from '../types'; interface CreateExecutionHandlerOptions { @@ -28,7 +29,7 @@ interface CreateExecutionHandlerOptions { actionsPlugin: ActionsPluginStartContract; actions: AlertAction[]; spaceId: string; - apiKey: string | null; + apiKey: RawAlert['apiKey']; alertType: AlertType; logger: Logger; eventLogger: IEventLogger; @@ -99,7 +100,7 @@ export function createExecutionHandler({ id: action.id, params: action.params, spaceId, - apiKey, + apiKey: apiKey ?? null, source: asSavedObjectExecutionSource({ id: alertId, type: 'alert', 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 4c16d23b485b5a..5be684eca4651a 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; 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 18834f55af0a50..e516ab1cb73b37 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 @@ -82,6 +82,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, version: 'some-version' }; + 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, version: 'some-version' } + ); + }); + it('generates ID, encrypts attributes and strips them from response except for ones with `dangerouslyExposeValue` set to `true`', async () => { const attributes = { attrOne: 'one', @@ -262,6 +314,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, version: 'some-version' }, + { 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', 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 0eeb9943b5be98..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 @@ -68,13 +68,18 @@ 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) { + + // 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( '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, @@ -97,7 +102,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 @@ -110,14 +115,15 @@ 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) { + // 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( '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/common/fixtures/plugins/alerts/server/routes.ts b/x-pack/test/alerting_api_integration/common/fixtures/plugins/alerts/server/routes.ts index 02d39bba42659b..bd8c72f57d8f20 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 @@ -138,7 +138,13 @@ export function defineRoutes( 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/actions/builtin_action_types/webhook.ts b/x-pack/test/alerting_api_integration/security_and_spaces/tests/actions/builtin_action_types/webhook.ts index d82d116396cd65..ef14dd9ec2eff7 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 @@ -141,6 +141,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, {}, kibanaURL); const { body: result } = await supertest