[Alerting] Improves performance of the authorization filter in AlertsClient.find by skipping KQL parsing

x-pack/plugins/alerts/server/alerts_client.test.ts

@@ -7,6 +7,8 @@ import uuid from 'uuid';
 import { schema } from '@kbn/config-schema';
 import { AlertsClient, CreateOptions, ConstructorOptions } from './alerts_client';
 import { savedObjectsClientMock, loggingSystemMock } from '../../../../src/core/server/mocks';
+import { nodeTypes } from '../../../../src/plugins/data/common';
+import { esKuery } from '../../../../src/plugins/data/server';
 import { taskManagerMock } from '../../task_manager/server/task_manager.mock';
 import { alertTypeRegistryMock } from './alert_type_registry.mock';
 import { alertsAuthorizationMock } from './authorization/alerts_authorization.mock';
@@ -2722,6 +2724,7 @@ describe('find()', () => {
       Array [
         Object {
           "fields": undefined,
+          "filter": undefined,
           "type": "alert",
         },
       ]
@@ -2730,9 +2733,11 @@ describe('find()', () => {
 
   describe('authorization', () => {
     test('ensures user is query filter types down to those the user is authorized to find', async () => {
+      const filter = esKuery.fromKueryExpression(
+        '((alert.attributes.alertTypeId:myType and alert.attributes.consumer:myApp) or (alert.attributes.alertTypeId:myOtherType and alert.attributes.consumer:myApp) or (alert.attributes.alertTypeId:myOtherType and alert.attributes.consumer:myOtherApp))'
+      );
       authorization.getFindAuthorizationFilter.mockResolvedValue({
-        filter:
-          '((alert.attributes.alertTypeId:myType and alert.attributes.consumer:myApp) or (alert.attributes.alertTypeId:myOtherType and alert.attributes.consumer:myApp) or (alert.attributes.alertTypeId:myOtherType and alert.attributes.consumer:myOtherApp))',
+        filter,
         ensureAlertTypeIsAuthorized() {},
         logSuccessfulAuthorization() {},
       });
@@ -2741,8 +2746,8 @@ describe('find()', () => {
       await alertsClient.find({ options: { filter: 'someTerm' } });
 
       const [options] = unsecuredSavedObjectsClient.find.mock.calls[0];
-      expect(options.filter).toMatchInlineSnapshot(
-        `"someTerm and ((alert.attributes.alertTypeId:myType and alert.attributes.consumer:myApp) or (alert.attributes.alertTypeId:myOtherType and alert.attributes.consumer:myApp) or (alert.attributes.alertTypeId:myOtherType and alert.attributes.consumer:myOtherApp))"`
+      expect(options.filter).toEqual(
+        nodeTypes.function.buildNode('and', [esKuery.fromKueryExpression('someTerm'), filter])
       );
       expect(authorization.getFindAuthorizationFilter).toHaveBeenCalledTimes(1);
     });
@@ -2759,7 +2764,6 @@ describe('find()', () => {
       const ensureAlertTypeIsAuthorized = jest.fn();
       const logSuccessfulAuthorization = jest.fn();
       authorization.getFindAuthorizationFilter.mockResolvedValue({
-        filter: '',
         ensureAlertTypeIsAuthorized,
         logSuccessfulAuthorization,
       });

x-pack/plugins/alerts/server/alerts_client.ts

@@ -13,6 +13,7 @@ import {
   SavedObjectReference,
   SavedObject,
 } from 'src/core/server';
+import { esKuery } from '../../../../src/plugins/data/server';
 import { ActionsClient, ActionsAuthorization } from '../../actions/server';
 import {
   Alert,
@@ -37,11 +38,7 @@ 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 {
-  AlertsAuthorization,
-  WriteOperations,
-  ReadOperations,
-} from './authorization/alerts_authorization';
+import { AlertsAuthorization, WriteOperations, ReadOperations, and } from './authorization';
 import { IEventLogClient } from '../../../plugins/event_log/server';
 import { parseIsoOrRelativeDate } from './lib/iso_or_relative_date';
 import { alertInstanceSummaryFromEventLog } from './lib/alert_instance_summary_from_event_log';
@@ -339,18 +336,17 @@ export class AlertsClient {
       logSuccessfulAuthorization,
     } = await this.authorization.getFindAuthorizationFilter();
 
-    if (authorizationFilter) {
-      options.filter = options.filter
-        ? `${options.filter} and ${authorizationFilter}`
-        : authorizationFilter;
-    }
     const {
       page,
       per_page: perPage,
       total,
       saved_objects: data,
     } = await this.unsecuredSavedObjectsClient.find<RawAlert>({
       ...options,
+      filter:
+        (authorizationFilter && options.filter
+          ? and([esKuery.fromKueryExpression(options.filter), authorizationFilter])
+          : authorizationFilter) ?? options.filter,
       fields: fields ? this.includeFieldsRequiredForAuthentication(fields) : fields,
       type: 'alert',
     });

x-pack/plugins/alerts/server/authorization/alerts_authorization.test.ts

@@ -6,14 +6,10 @@
 import { KibanaRequest } from 'kibana/server';
 import { alertTypeRegistryMock } from '../alert_type_registry.mock';
 import { securityMock } from '../../../../plugins/security/server/mocks';
+import { esKuery } from '../../../../../src/plugins/data/server';
 import { PluginStartContract as FeaturesStartContract, Feature } from '../../../features/server';
 import { featuresPluginMock } from '../../../features/server/mocks';
-import {
-  AlertsAuthorization,
-  ensureFieldIsSafeForQuery,
-  WriteOperations,
-  ReadOperations,
-} from './alerts_authorization';
+import { AlertsAuthorization, WriteOperations, ReadOperations } from './alerts_authorization';
 import { alertsAuthorizationAuditLoggerMock } from './audit_logger.mock';
 import { AlertsAuthorizationAuditLogger, AuthorizationResult } from './audit_logger';
 import uuid from 'uuid';
@@ -605,8 +601,10 @@ describe('AlertsAuthorization', () => {
       });
       alertTypeRegistry.list.mockReturnValue(setOfAlertTypes);
 
-      expect((await alertAuthorization.getFindAuthorizationFilter()).filter).toMatchInlineSnapshot(
-        `"((alert.attributes.alertTypeId:myAppAlertType and alert.attributes.consumer:(alerts or myApp or myOtherApp or myAppWithSubFeature)) or (alert.attributes.alertTypeId:myOtherAppAlertType and alert.attributes.consumer:(alerts or myApp or myOtherApp or myAppWithSubFeature)) or (alert.attributes.alertTypeId:mySecondAppAlertType and alert.attributes.consumer:(alerts or myApp or myOtherApp or myAppWithSubFeature)))"`
+      expect((await alertAuthorization.getFindAuthorizationFilter()).filter).toEqual(
+        esKuery.fromKueryExpression(
+          `((alert.attributes.alertTypeId:myAppAlertType and alert.attributes.consumer:(alerts or myApp or myOtherApp or myAppWithSubFeature)) or (alert.attributes.alertTypeId:myOtherAppAlertType and alert.attributes.consumer:(alerts or myApp or myOtherApp or myAppWithSubFeature)) or (alert.attributes.alertTypeId:mySecondAppAlertType and alert.attributes.consumer:(alerts or myApp or myOtherApp or myAppWithSubFeature)))`
+        )
       );
 
       expect(auditLogger.alertsAuthorizationSuccess).not.toHaveBeenCalled();
@@ -1221,36 +1219,4 @@ describe('AlertsAuthorization', () => {
             `);
     });
   });
-
-  describe('ensureFieldIsSafeForQuery', () => {
-    test('throws if field contains character that isnt safe in a KQL query', () => {
-      expect(() => ensureFieldIsSafeForQuery('id', 'alert-*')).toThrowError(
-        `expected id not to include invalid character: *`
-      );
-
-      expect(() => ensureFieldIsSafeForQuery('id', '<=""')).toThrowError(
-        `expected id not to include invalid character: <=`
-      );
-
-      expect(() => ensureFieldIsSafeForQuery('id', '>=""')).toThrowError(
-        `expected id not to include invalid character: >=`
-      );
-
-      expect(() => ensureFieldIsSafeForQuery('id', '1 or alertid:123')).toThrowError(
-        `expected id not to include whitespace and invalid character: :`
-      );
-
-      expect(() => ensureFieldIsSafeForQuery('id', ') or alertid:123')).toThrowError(
-        `expected id not to include whitespace and invalid characters: ), :`
-      );
-
-      expect(() => ensureFieldIsSafeForQuery('id', 'some space')).toThrowError(
-        `expected id not to include whitespace`
-      );
-    });
-
-    test('doesnt throws if field is safe as part of a KQL query', () => {
-      expect(() => ensureFieldIsSafeForQuery('id', '123-0456-678')).not.toThrow();
-    });
-  });
 });

x-pack/plugins/alerts/server/authorization/alerts_authorization.ts

@@ -5,7 +5,7 @@
  */
 
 import Boom from 'boom';
-import { map, mapValues, remove, fromPairs, has } from 'lodash';
+import { map, mapValues, fromPairs, has } from 'lodash';
 import { KibanaRequest } from 'src/core/server';
 import { ALERTS_FEATURE_ID } from '../../common';
 import { AlertTypeRegistry } from '../types';
@@ -14,6 +14,8 @@ import { RegistryAlertType } from '../alert_type_registry';
 import { PluginStartContract as FeaturesPluginStart } from '../../../features/server';
 import { AlertsAuthorizationAuditLogger, ScopeType } from './audit_logger';
 import { Space } from '../../../spaces/server';
+import { asFiltersByAlertTypeAndConsumer } from './alerts_authorization_kuery';
+import { KueryNode } from '../../../../../src/plugins/data/server';
 
 export enum ReadOperations {
   Get = 'get',
@@ -214,7 +216,7 @@ export class AlertsAuthorization {
   }
 
   public async getFindAuthorizationFilter(): Promise<{
-    filter?: string;
+    filter?: KueryNode;
     ensureAlertTypeIsAuthorized: (alertTypeId: string, consumer: string) => void;
     logSuccessfulAuthorization: () => void;
   }> {
@@ -243,7 +245,7 @@ export class AlertsAuthorization {
 
       const authorizedEntries: Map<string, Set<string>> = new Map();
       return {
-        filter: `(${this.asFiltersByAlertTypeAndConsumer(authorizedAlertTypes).join(' or ')})`,
+        filter: asFiltersByAlertTypeAndConsumer(authorizedAlertTypes),
         ensureAlertTypeIsAuthorized: (alertTypeId: string, consumer: string) => {
           if (!authorizedAlertTypeIdsToConsumers.has(`${alertTypeId}/${consumer}`)) {
             throw Boom.forbidden(
@@ -399,39 +401,6 @@ export class AlertsAuthorization {
       }))
     );
   }
-
-  private asFiltersByAlertTypeAndConsumer(alertTypes: Set<RegistryAlertTypeWithAuth>): string[] {
-    return Array.from(alertTypes).reduce<string[]>((filters, { id, authorizedConsumers }) => {
-      ensureFieldIsSafeForQuery('alertTypeId', id);
-      filters.push(
-        `(alert.attributes.alertTypeId:${id} and alert.attributes.consumer:(${Object.keys(
-          authorizedConsumers
-        )
-          .map((consumer) => {
-            ensureFieldIsSafeForQuery('alertTypeId', id);
-            return consumer;
-          })
-          .join(' or ')}))`
-      );
-      return filters;
-    }, []);
-  }
-}
-
-export function ensureFieldIsSafeForQuery(field: string, value: string): boolean {
-  const invalid = value.match(/([>=<\*:()]+|\s+)/g);
-  if (invalid) {
-    const whitespace = remove(invalid, (chars) => chars.trim().length === 0);
-    const errors = [];
-    if (whitespace.length) {
-      errors.push(`whitespace`);
-    }
-    if (invalid.length) {
-      errors.push(`invalid character${invalid.length > 1 ? `s` : ``}: ${invalid?.join(`, `)}`);
-    }
-    throw new Error(`expected ${field} not to include ${errors.join(' and ')}`);
-  }
-  return true;
 }
 
 function mergeHasPrivileges(left: HasPrivileges, right?: HasPrivileges): HasPrivileges {

x-pack/plugins/alerts/server/authorization/alerts_authorization_kuery.test.ts

@@ -0,0 +1,144 @@
+/*
+ * 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 { esKuery } from '../../../../../src/plugins/data/server';
+import {
+  asFiltersByAlertTypeAndConsumer,
+  ensureFieldIsSafeForQuery,
+} from './alerts_authorization_kuery';
+
+describe('asFiltersByAlertTypeAndConsumer', () => {
+  test('constructs filter for single alert type with single authorized consumer', async () => {
+    expect(
+      asFiltersByAlertTypeAndConsumer(
+        new Set([
+          {
+            actionGroups: [],
+            defaultActionGroupId: 'default',
+            id: 'myAppAlertType',
+            name: 'myAppAlertType',
+            producer: 'myApp',
+            authorizedConsumers: {
+              myApp: { read: true, all: true },
+            },
+          },
+        ])
+      )
+    ).toEqual(
+      esKuery.fromKueryExpression(
+        `((alert.attributes.alertTypeId:myAppAlertType and alert.attributes.consumer:(myApp)))`
+      )
+    );
+  });
+
+  test('constructs filter for single alert type with multiple authorized consumer', async () => {
+    expect(
+      asFiltersByAlertTypeAndConsumer(
+        new Set([
+          {
+            actionGroups: [],
+            defaultActionGroupId: 'default',
+            id: 'myAppAlertType',
+            name: 'myAppAlertType',
+            producer: 'myApp',
+            authorizedConsumers: {
+              alerts: { read: true, all: true },
+              myApp: { read: true, all: true },
+              myOtherApp: { read: true, all: true },
+            },
+          },
+        ])
+      )
+    ).toEqual(
+      esKuery.fromKueryExpression(
+        `((alert.attributes.alertTypeId:myAppAlertType and alert.attributes.consumer:(alerts or myApp or myOtherApp)))`
+      )
+    );
+  });
+
+  test('constructs filter for multiple alert types across authorized consumer', async () => {
+    expect(
+      asFiltersByAlertTypeAndConsumer(
+        new Set([
+          {
+            actionGroups: [],
+            defaultActionGroupId: 'default',
+            id: 'myAppAlertType',
+            name: 'myAppAlertType',
+            producer: 'myApp',
+            authorizedConsumers: {
+              alerts: { read: true, all: true },
+              myApp: { read: true, all: true },
+              myOtherApp: { read: true, all: true },
+              myAppWithSubFeature: { read: true, all: true },
+            },
+          },
+          {
+            actionGroups: [],
+            defaultActionGroupId: 'default',
+            id: 'myOtherAppAlertType',
+            name: 'myOtherAppAlertType',
+            producer: 'alerts',
+            authorizedConsumers: {
+              alerts: { read: true, all: true },
+              myApp: { read: true, all: true },
+              myOtherApp: { read: true, all: true },
+              myAppWithSubFeature: { read: true, all: true },
+            },
+          },
+          {
+            actionGroups: [],
+            defaultActionGroupId: 'default',
+            id: 'mySecondAppAlertType',
+            name: 'mySecondAppAlertType',
+            producer: 'myApp',
+            authorizedConsumers: {
+              alerts: { read: true, all: true },
+              myApp: { read: true, all: true },
+              myOtherApp: { read: true, all: true },
+              myAppWithSubFeature: { read: true, all: true },
+            },
+          },
+        ])
+      )
+    ).toEqual(
+      esKuery.fromKueryExpression(
+        `((alert.attributes.alertTypeId:myAppAlertType and alert.attributes.consumer:(alerts or myApp or myOtherApp or myAppWithSubFeature)) or (alert.attributes.alertTypeId:myOtherAppAlertType and alert.attributes.consumer:(alerts or myApp or myOtherApp or myAppWithSubFeature)) or (alert.attributes.alertTypeId:mySecondAppAlertType and alert.attributes.consumer:(alerts or myApp or myOtherApp or myAppWithSubFeature)))`
+      )
+    );
+  });
+});
+
+describe('ensureFieldIsSafeForQuery', () => {
+  test('throws if field contains character that isnt safe in a KQL query', () => {
+    expect(() => ensureFieldIsSafeForQuery('id', 'alert-*')).toThrowError(
+      `expected id not to include invalid character: *`
+    );
+
+    expect(() => ensureFieldIsSafeForQuery('id', '<=""')).toThrowError(
+      `expected id not to include invalid character: <=`
+    );
+
+    expect(() => ensureFieldIsSafeForQuery('id', '>=""')).toThrowError(
+      `expected id not to include invalid character: >=`
+    );
+
+    expect(() => ensureFieldIsSafeForQuery('id', '1 or alertid:123')).toThrowError(
+      `expected id not to include whitespace and invalid character: :`
+    );
+
+    expect(() => ensureFieldIsSafeForQuery('id', ') or alertid:123')).toThrowError(
+      `expected id not to include whitespace and invalid characters: ), :`
+    );
+
+    expect(() => ensureFieldIsSafeForQuery('id', 'some space')).toThrowError(
+      `expected id not to include whitespace`
+    );
+  });
+
+  test('doesnt throws if field is safe as part of a KQL query', () => {
+    expect(() => ensureFieldIsSafeForQuery('id', '123-0456-678')).not.toThrow();
+  });
+});