From c2c5b66bb878e601519da54666f4dbc5ef56d6b6 Mon Sep 17 00:00:00 2001 From: Gidi Meir Morris Date: Mon, 6 Jan 2020 14:53:50 +0000 Subject: [PATCH] adds strict types to Alerting Client (#53821) (#54007) The AlertsClient API currently returns mixed inferred types instead of a clear strict type, making it harder to work with the client's type signatures. The root causes for this difficulty is that we have to support the SavedObjects API which allows partial updates of types, and the implementation of code that converts the SavedObject from a RawAlert to an Alert in a non type-strict manner. To address this we've added concrete types on the AlertsClient APIs, using Partial on update due to the SavedObjects API, and a strict Alert on the other APIs. --- .../plugins/alerting/server/alerts_client.ts | 88 +++++++++++-------- .../alerting/server/routes/create.test.ts | 18 +++- .../alerting/server/routes/get.test.ts | 11 +++ .../legacy/plugins/alerting/server/types.ts | 3 + .../routes/__mocks__/request_responses.ts | 2 - .../lib/detection_engine/rules/find_rules.ts | 9 +- .../lib/detection_engine/rules/types.ts | 5 +- 7 files changed, 90 insertions(+), 46 deletions(-) diff --git a/x-pack/legacy/plugins/alerting/server/alerts_client.ts b/x-pack/legacy/plugins/alerting/server/alerts_client.ts index fc0252c86fe50b..33a6b716e9b8a1 100644 --- a/x-pack/legacy/plugins/alerting/server/alerts_client.ts +++ b/x-pack/legacy/plugins/alerting/server/alerts_client.ts @@ -15,6 +15,7 @@ import { } from 'src/core/server'; import { Alert, + PartialAlert, RawAlert, AlertTypeRegistry, AlertAction, @@ -69,28 +70,26 @@ export interface FindOptions { }; } -interface FindResult { +export interface FindResult { page: number; perPage: number; total: number; - data: object[]; + data: Alert[]; } interface CreateOptions { - data: Pick< + data: Omit< Alert, - Exclude< - keyof Alert, - | 'createdBy' - | 'updatedBy' - | 'createdAt' - | 'updatedAt' - | 'apiKey' - | 'apiKeyOwner' - | 'muteAll' - | 'mutedInstanceIds' - | 'actions' - > + | 'id' + | 'createdBy' + | 'updatedBy' + | 'createdAt' + | 'updatedAt' + | 'apiKey' + | 'apiKeyOwner' + | 'muteAll' + | 'mutedInstanceIds' + | 'actions' > & { actions: NormalizedAlertAction[] }; options?: { migrationVersion?: Record; @@ -146,7 +145,7 @@ export class AlertsClient { this.encryptedSavedObjectsPlugin = encryptedSavedObjectsPlugin; } - public async create({ data, options }: CreateOptions) { + public async create({ data, options }: CreateOptions): Promise { // Throws an error if alert type isn't registered const alertType = this.alertTypeRegistry.get(data.alertTypeId); const validatedAlertTypeParams = validateAlertTypeParams(alertType, data.params); @@ -199,26 +198,29 @@ export class AlertsClient { ); } - public async get({ id }: { id: string }) { + public async get({ id }: { id: string }): Promise { const result = await this.savedObjectsClient.get('alert', id); return this.getAlertFromRaw(result.id, result.attributes, result.updated_at, result.references); } public async find({ options = {} }: FindOptions = {}): Promise { - const results = await this.savedObjectsClient.find({ + const { + page, + per_page: perPage, + total, + saved_objects: data, + } = await this.savedObjectsClient.find({ ...options, type: 'alert', }); - const data = results.saved_objects.map(result => - this.getAlertFromRaw(result.id, result.attributes, result.updated_at, result.references) - ); - return { - page: results.page, - perPage: results.per_page, - total: results.total, - data, + page, + perPage, + total, + data: data.map(({ id, attributes, updated_at, references }) => + this.getAlertFromRaw(id, attributes, updated_at, references) + ), }; } @@ -234,7 +236,7 @@ export class AlertsClient { return removeResult; } - public async update({ id, data }: UpdateOptions) { + public async update({ id, data }: UpdateOptions): Promise { const decryptedAlertSavedObject = await this.encryptedSavedObjectsPlugin.getDecryptedAsInternalUser< RawAlert >('alert', id, { namespace: this.namespace }); @@ -257,7 +259,7 @@ export class AlertsClient { private async updateAlert( { id, data }: UpdateOptions, { attributes, version }: SavedObject - ) { + ): Promise { const alertType = this.alertTypeRegistry.get(attributes.alertTypeId); // Validate @@ -287,7 +289,7 @@ export class AlertsClient { await this.invalidateApiKey({ apiKey: attributes.apiKey }); - return this.getAlertFromRaw( + return this.getPartialAlertFromRaw( id, updatedObject.attributes, updatedObject.updated_at, @@ -494,24 +496,34 @@ export class AlertsClient { } private getAlertFromRaw( + id: string, + rawAlert: RawAlert, + updatedAt: SavedObject['updated_at'], + references: SavedObjectReference[] | undefined + ): Alert { + // In order to support the partial update API of Saved Objects we have to support + // partial updates of an Alert, but when we receive an actual RawAlert, it is safe + // to cast the result to an Alert + return this.getPartialAlertFromRaw(id, rawAlert, updatedAt, references) as Alert; + } + + private getPartialAlertFromRaw( id: string, rawAlert: Partial, updatedAt: SavedObject['updated_at'], references: SavedObjectReference[] | undefined - ) { - if (!rawAlert.actions) { - return { - id, - ...rawAlert, - }; - } - const actions = this.injectReferencesIntoActions(rawAlert.actions, references || []); + ): PartialAlert { return { id, ...rawAlert, + // we currently only support the Interval Schedule type + // Once we support additional types, this type signature will likely change + schedule: rawAlert.schedule as IntervalSchedule, updatedAt: updatedAt ? new Date(updatedAt) : new Date(rawAlert.createdAt!), createdAt: new Date(rawAlert.createdAt!), - actions, + actions: rawAlert.actions + ? this.injectReferencesIntoActions(rawAlert.actions, references || []) + : [], }; } diff --git a/x-pack/legacy/plugins/alerting/server/routes/create.test.ts b/x-pack/legacy/plugins/alerting/server/routes/create.test.ts index 03b33b0bd40b0d..2a0ae78fd78b22 100644 --- a/x-pack/legacy/plugins/alerting/server/routes/create.test.ts +++ b/x-pack/legacy/plugins/alerting/server/routes/create.test.ts @@ -20,6 +20,7 @@ const mockedAlert = { params: { bar: true, }, + throttle: '30s', actions: [ { group: 'default', @@ -44,6 +45,13 @@ test('creates an alert with proper parameters', async () => { const updatedAt = new Date(); alertsClient.create.mockResolvedValueOnce({ ...mockedAlert, + enabled: true, + muteAll: false, + createdBy: '', + updatedBy: '', + apiKey: '', + apiKeyOwner: '', + mutedInstanceIds: [], createdAt, updatedAt, id: '123', @@ -71,8 +79,14 @@ test('creates an alert with proper parameters', async () => { }, ], "alertTypeId": "1", + "apiKey": "", + "apiKeyOwner": "", "consumer": "bar", + "createdBy": "", + "enabled": true, "id": "123", + "muteAll": false, + "mutedInstanceIds": Array [], "name": "abc", "params": Object { "bar": true, @@ -83,6 +97,8 @@ test('creates an alert with proper parameters', async () => { "tags": Array [ "foo", ], + "throttle": "30s", + "updatedBy": "", } `); expect(alertsClient.create).toHaveBeenCalledTimes(1); @@ -112,7 +128,7 @@ test('creates an alert with proper parameters', async () => { "tags": Array [ "foo", ], - "throttle": null, + "throttle": "30s", }, }, ] diff --git a/x-pack/legacy/plugins/alerting/server/routes/get.test.ts b/x-pack/legacy/plugins/alerting/server/routes/get.test.ts index 5b1bdc7f697086..320e9042d87c59 100644 --- a/x-pack/legacy/plugins/alerting/server/routes/get.test.ts +++ b/x-pack/legacy/plugins/alerting/server/routes/get.test.ts @@ -29,6 +29,17 @@ const mockedAlert = { }, }, ], + consumer: 'bar', + name: 'abc', + tags: ['foo'], + enabled: true, + muteAll: false, + createdBy: '', + updatedBy: '', + apiKey: '', + apiKeyOwner: '', + throttle: '30s', + mutedInstanceIds: [], }; beforeEach(() => jest.resetAllMocks()); diff --git a/x-pack/legacy/plugins/alerting/server/types.ts b/x-pack/legacy/plugins/alerting/server/types.ts index a2390bf93d005a..62dcf07abb7bdd 100644 --- a/x-pack/legacy/plugins/alerting/server/types.ts +++ b/x-pack/legacy/plugins/alerting/server/types.ts @@ -65,6 +65,7 @@ export interface IntervalSchedule extends SavedObjectAttributes { } export interface Alert { + id: string; enabled: boolean; name: string; tags: string[]; @@ -85,6 +86,8 @@ export interface Alert { mutedInstanceIds: string[]; } +export type PartialAlert = Pick & Partial>; + export interface RawAlert extends SavedObjectAttributes { enabled: boolean; name: string; diff --git a/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/__mocks__/request_responses.ts b/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/__mocks__/request_responses.ts index 2e16f209acfb17..edf196b96f5d02 100644 --- a/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/__mocks__/request_responses.ts +++ b/x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/__mocks__/request_responses.ts @@ -283,9 +283,7 @@ export const getResult = (): RuleAlertType => ({ ], riskScore: 50, maxSignals: 100, - size: 1, severity: 'high', - tags: [], to: 'now', type: 'query', threats: [ diff --git a/x-pack/legacy/plugins/siem/server/lib/detection_engine/rules/find_rules.ts b/x-pack/legacy/plugins/siem/server/lib/detection_engine/rules/find_rules.ts index c1058bd353e8ce..5f69082e3fc719 100644 --- a/x-pack/legacy/plugins/siem/server/lib/detection_engine/rules/find_rules.ts +++ b/x-pack/legacy/plugins/siem/server/lib/detection_engine/rules/find_rules.ts @@ -5,7 +5,7 @@ */ import { SIGNALS_ID } from '../../../../common/constants'; -import { FindRuleParams } from './types'; +import { FindRuleParams, RuleAlertType } from './types'; export const getFilter = (filter: string | null | undefined) => { if (filter == null) { @@ -33,5 +33,10 @@ export const findRules = async ({ sortOrder, sortField, }, - }); + }) as Promise<{ + page: number; + perPage: number; + total: number; + data: RuleAlertType[]; + }>; }; diff --git a/x-pack/legacy/plugins/siem/server/lib/detection_engine/rules/types.ts b/x-pack/legacy/plugins/siem/server/lib/detection_engine/rules/types.ts index b0578174e1f658..4f4c0da7127cd7 100644 --- a/x-pack/legacy/plugins/siem/server/lib/detection_engine/rules/types.ts +++ b/x-pack/legacy/plugins/siem/server/lib/detection_engine/rules/types.ts @@ -35,10 +35,9 @@ export interface BulkUpdateRulesRequest extends RequestFacade { payload: UpdateRuleAlertParamsRest[]; } -export type RuleAlertType = Alert & { - id: string; +export interface RuleAlertType extends Alert { params: RuleTypeParams; -}; +} export interface RulesRequest extends RequestFacade { payload: RuleAlertParamsRest;