From 2b43dfd0ea0b3710ed08b5fa1520dfb42603db43 Mon Sep 17 00:00:00 2001 From: Mike Cote Date: Thu, 6 Feb 2020 16:08:52 -0500 Subject: [PATCH 1/3] Don't create API key for disabled alerts when calling create API --- .../alerting/server/alerts_client.test.ts | 181 ++++++++++++------ .../plugins/alerting/server/alerts_client.ts | 7 +- 2 files changed, 129 insertions(+), 59 deletions(-) diff --git a/x-pack/legacy/plugins/alerting/server/alerts_client.test.ts b/x-pack/legacy/plugins/alerting/server/alerts_client.test.ts index 56ccf08d6a44f..32e25c7129d53 100644 --- a/x-pack/legacy/plugins/alerting/server/alerts_client.test.ts +++ b/x-pack/legacy/plugins/alerting/server/alerts_client.test.ts @@ -79,15 +79,20 @@ function getMockData(overwrites: Record = {}) { } describe('create()', () => { - test('creates an alert', async () => { - const alertsClient = new AlertsClient(alertsClientParams); - const data = getMockData(); - alertTypeRegistry.get.mockReturnValueOnce({ + let alertsClient: AlertsClient; + + beforeEach(() => { + alertsClient = new AlertsClient(alertsClientParams); + alertTypeRegistry.get.mockReturnValue({ id: '123', name: 'Test', actionGroups: ['default'], async executor() {}, }); + }); + + test('creates an alert', async () => { + const data = getMockData(); savedObjectsClient.bulkGet.mockResolvedValueOnce({ saved_objects: [ { @@ -263,7 +268,6 @@ describe('create()', () => { }); test('creates an alert with multiple actions', async () => { - const alertsClient = new AlertsClient(alertsClientParams); const data = getMockData({ actions: [ { @@ -289,12 +293,6 @@ describe('create()', () => { }, ], }); - alertTypeRegistry.get.mockReturnValueOnce({ - id: '123', - name: 'Test', - actionGroups: ['default'], - async executor() {}, - }); savedObjectsClient.bulkGet.mockResolvedValueOnce({ saved_objects: [ { @@ -446,14 +444,7 @@ describe('create()', () => { }); test('creates a disabled alert', async () => { - const alertsClient = new AlertsClient(alertsClientParams); const data = getMockData({ enabled: false }); - alertTypeRegistry.get.mockReturnValueOnce({ - id: '123', - name: 'Test', - actionGroups: ['default'], - async executor() {}, - }); savedObjectsClient.bulkGet.mockResolvedValueOnce({ saved_objects: [ { @@ -527,9 +518,8 @@ describe('create()', () => { }); test('should validate params', async () => { - const alertsClient = new AlertsClient(alertsClientParams); const data = getMockData(); - alertTypeRegistry.get.mockReturnValueOnce({ + alertTypeRegistry.get.mockReturnValue({ id: '123', name: 'Test', actionGroups: [], @@ -547,14 +537,7 @@ describe('create()', () => { }); test('throws error if loading actions fails', async () => { - const alertsClient = new AlertsClient(alertsClientParams); const data = getMockData(); - alertTypeRegistry.get.mockReturnValueOnce({ - id: '123', - name: 'Test', - actionGroups: ['default'], - async executor() {}, - }); savedObjectsClient.bulkGet.mockRejectedValueOnce(new Error('Test Error')); await expect(alertsClient.create({ data })).rejects.toThrowErrorMatchingInlineSnapshot( `"Test Error"` @@ -564,14 +547,7 @@ describe('create()', () => { }); test('throws error if create saved object fails', async () => { - const alertsClient = new AlertsClient(alertsClientParams); const data = getMockData(); - alertTypeRegistry.get.mockReturnValueOnce({ - id: '123', - name: 'Test', - actionGroups: ['default'], - async executor() {}, - }); savedObjectsClient.bulkGet.mockResolvedValueOnce({ saved_objects: [ { @@ -592,14 +568,7 @@ describe('create()', () => { }); test('attempts to remove saved object if scheduling failed', async () => { - const alertsClient = new AlertsClient(alertsClientParams); const data = getMockData(); - alertTypeRegistry.get.mockReturnValueOnce({ - id: '123', - name: 'Test', - actionGroups: ['default'], - async executor() {}, - }); savedObjectsClient.bulkGet.mockResolvedValueOnce({ saved_objects: [ { @@ -655,14 +624,7 @@ describe('create()', () => { }); test('returns task manager error if cleanup fails, logs to console', async () => { - const alertsClient = new AlertsClient(alertsClientParams); const data = getMockData(); - alertTypeRegistry.get.mockReturnValueOnce({ - id: '123', - name: 'Test', - actionGroups: ['default'], - async executor() {}, - }); savedObjectsClient.bulkGet.mockResolvedValueOnce({ saved_objects: [ { @@ -714,7 +676,6 @@ describe('create()', () => { }); test('throws an error if alert type not registerd', async () => { - const alertsClient = new AlertsClient(alertsClientParams); const data = getMockData(); alertTypeRegistry.get.mockImplementation(() => { throw new Error('Invalid type'); @@ -725,14 +686,7 @@ describe('create()', () => { }); test('calls the API key function', async () => { - const alertsClient = new AlertsClient(alertsClientParams); const data = getMockData(); - alertTypeRegistry.get.mockReturnValueOnce({ - id: '123', - name: 'Test', - actionGroups: ['default'], - async executor() {}, - }); alertsClientParams.createAPIKey.mockResolvedValueOnce({ apiKeysEnabled: true, result: { id: '123', api_key: 'abc' }, @@ -845,6 +799,121 @@ describe('create()', () => { } ); }); + + test(`doesn't create API key for disabled alerts`, async () => { + const data = getMockData({ enabled: false }); + alertsClientParams.createAPIKey.mockResolvedValueOnce({ + apiKeysEnabled: true, + result: { id: '123', api_key: 'abc' }, + }); + savedObjectsClient.bulkGet.mockResolvedValueOnce({ + saved_objects: [ + { + id: '1', + type: 'action', + attributes: { + actionTypeId: 'test', + }, + references: [], + }, + ], + }); + savedObjectsClient.create.mockResolvedValueOnce({ + id: '1', + type: 'alert', + attributes: { + alertTypeId: '123', + schedule: { interval: '10s' }, + params: { + bar: true, + }, + actions: [ + { + group: 'default', + actionRef: 'action_0', + actionTypeId: 'test', + params: { + foo: true, + }, + }, + ], + }, + references: [ + { + name: 'action_0', + type: 'action', + id: '1', + }, + ], + }); + taskManager.schedule.mockResolvedValueOnce({ + id: 'task-123', + taskType: 'alerting:123', + scheduledAt: new Date(), + attempts: 1, + status: TaskStatus.Idle, + runAt: new Date(), + startedAt: null, + retryAt: null, + state: {}, + params: {}, + ownerId: null, + }); + savedObjectsClient.update.mockResolvedValueOnce({ + id: '1', + type: 'alert', + attributes: { + scheduledTaskId: 'task-123', + }, + references: [ + { + id: '1', + name: 'action_0', + type: 'action', + }, + ], + }); + await alertsClient.create({ data }); + + expect(alertsClientParams.createAPIKey).not.toHaveBeenCalled(); + expect(savedObjectsClient.create).toHaveBeenCalledWith( + 'alert', + { + actions: [ + { + actionRef: 'action_0', + group: 'default', + actionTypeId: 'test', + params: { foo: true }, + }, + ], + alertTypeId: '123', + consumer: 'bar', + name: 'abc', + params: { bar: true }, + apiKey: null, + apiKeyOwner: null, + createdBy: 'elastic', + createdAt: '2019-02-12T21:01:22.479Z', + updatedBy: 'elastic', + enabled: false, + schedule: { interval: '10s' }, + throttle: null, + muteAll: false, + mutedInstanceIds: [], + tags: ['foo'], + }, + { + references: [ + { + id: '1', + name: 'action_0', + type: 'action', + }, + ], + } + ); + }); }); describe('enable()', () => { diff --git a/x-pack/legacy/plugins/alerting/server/alerts_client.ts b/x-pack/legacy/plugins/alerting/server/alerts_client.ts index 40125f3067ee3..36192133f4f7d 100644 --- a/x-pack/legacy/plugins/alerting/server/alerts_client.ts +++ b/x-pack/legacy/plugins/alerting/server/alerts_client.ts @@ -151,13 +151,14 @@ export class AlertsClient { const alertType = this.alertTypeRegistry.get(data.alertTypeId); const validatedAlertTypeParams = validateAlertTypeParams(alertType, data.params); const username = await this.getUserName(); + const createdAPIKey = data.enabled ? await this.createAPIKey() : null; this.validateActions(alertType, data.actions); const { references, actions } = await this.denormalizeActions(data.actions); const rawAlert: RawAlert = { ...data, - ...this.apiKeyAsAlertAttributes(await this.createAPIKey(), username), + ...this.apiKeyAsAlertAttributes(createdAPIKey, username), actions, createdBy: username, updatedBy: username, @@ -317,10 +318,10 @@ export class AlertsClient { } private apiKeyAsAlertAttributes( - apiKey: CreateAPIKeyResult, + apiKey: CreateAPIKeyResult | null, username: string | null ): Pick { - return apiKey.apiKeysEnabled + return apiKey && apiKey.apiKeysEnabled ? { apiKeyOwner: username, apiKey: Buffer.from(`${apiKey.result.id}:${apiKey.result.api_key}`).toString('base64'), From cfb7dd626b9c6e5dc7725eb5ddb41bd6b11c7586 Mon Sep 17 00:00:00 2001 From: Mike Cote Date: Fri, 7 Feb 2020 09:02:19 -0500 Subject: [PATCH 2/3] Fix failing test --- .../security_and_spaces/tests/alerting/find.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/test/alerting_api_integration/security_and_spaces/tests/alerting/find.ts b/x-pack/test/alerting_api_integration/security_and_spaces/tests/alerting/find.ts index d99ab794cd28f..65ffa9ebe9dfa 100644 --- a/x-pack/test/alerting_api_integration/security_and_spaces/tests/alerting/find.ts +++ b/x-pack/test/alerting_api_integration/security_and_spaces/tests/alerting/find.ts @@ -158,7 +158,7 @@ export default function createFindTests({ getService }: FtrProviderContext) { createdBy: 'elastic', throttle: '1m', updatedBy: 'elastic', - apiKeyOwner: 'elastic', + apiKeyOwner: null, muteAll: false, mutedInstanceIds: [], createdAt: match.createdAt, From 31ead6d9fa2fb14d76749cbef05d0ebfec68b799 Mon Sep 17 00:00:00 2001 From: Mike Cote Date: Mon, 10 Feb 2020 08:35:14 -0500 Subject: [PATCH 3/3] Remove unused code in test --- x-pack/legacy/plugins/alerting/server/alerts_client.test.ts | 4 ---- 1 file changed, 4 deletions(-) diff --git a/x-pack/legacy/plugins/alerting/server/alerts_client.test.ts b/x-pack/legacy/plugins/alerting/server/alerts_client.test.ts index cfada5b4494e0..deb5f658721c4 100644 --- a/x-pack/legacy/plugins/alerting/server/alerts_client.test.ts +++ b/x-pack/legacy/plugins/alerting/server/alerts_client.test.ts @@ -802,10 +802,6 @@ describe('create()', () => { test(`doesn't create API key for disabled alerts`, async () => { const data = getMockData({ enabled: false }); - alertsClientParams.createAPIKey.mockResolvedValueOnce({ - apiKeysEnabled: true, - result: { id: '123', api_key: 'abc' }, - }); savedObjectsClient.bulkGet.mockResolvedValueOnce({ saved_objects: [ {