use kuery nodes instead of kuery query

gmmorris · Sep 9, 2020 · eb1ae15 · eb1ae15
1 parent 53a2c38
commit eb1ae15
Show file tree

Hide file tree

Showing 7 changed files with 82 additions and 135 deletions.
diff --git a/x-pack/plugins/alerts/server/alerts_client.test.ts b/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,
       });

diff --git a/x-pack/plugins/alerts/server/alerts_client.ts b/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',
     });

diff --git a/x-pack/plugins/alerts/server/authorization/alerts_authorization.test.ts b/x-pack/plugins/alerts/server/authorization/alerts_authorization.test.ts
@@ -9,12 +9,7 @@ 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';
@@ -607,9 +602,9 @@ describe('AlertsAuthorization', () => {
       alertTypeRegistry.list.mockReturnValue(setOfAlertTypes);
 
       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)))`
-        // )
+        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();

diff --git a/x-pack/plugins/alerts/server/authorization/alerts_authorization.ts b/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';
@@ -15,6 +15,7 @@ import { PluginStartContract as FeaturesPluginStart } from '../../../features/se
 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',
@@ -215,7 +216,7 @@ export class AlertsAuthorization {
   }
 
   public async getFindAuthorizationFilter(): Promise<{
-    filter?: string;
+    filter?: KueryNode;
     ensureAlertTypeIsAuthorized: (alertTypeId: string, consumer: string) => void;
     logSuccessfulAuthorization: () => void;
   }> {

diff --git a/x-pack/plugins/alerts/server/authorization/alerts_authorization_kuery.test.ts b/x-pack/plugins/alerts/server/authorization/alerts_authorization_kuery.test.ts
@@ -10,13 +10,7 @@ import {
 } from './alerts_authorization_kuery';
 
 describe('asFiltersByAlertTypeAndConsumer', () => {
-  // test('1', async () => {
-  //   expect(asFiltersByAlertTypeAndConsumer()).toEqual(
-  //     `((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)))`
-  //   );
-  // });
-
-  test('1', async () => {
+  test('constructs filter for single alert type with single authorized consumer', async () => {
     expect(
       asFiltersByAlertTypeAndConsumer(
         new Set([
@@ -27,128 +21,71 @@ describe('asFiltersByAlertTypeAndConsumer', () => {
             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(
-      `((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)))`
+      esKuery.fromKueryExpression(
+        `((alert.attributes.alertTypeId:myAppAlertType and alert.attributes.consumer:(myApp)))`
+      )
     );
   });
 
-  test('2', async () => {
+  test('constructs filter for single alert type with multiple authorized consumer', async () => {
     expect(
       asFiltersByAlertTypeAndConsumer(
         new Set([
-          {
-            actionGroups: [],
-            defaultActionGroupId: 'default',
-            id: 'myOtherAppAlertType',
-            name: 'myOtherAppAlertType',
-            producer: 'alerts',
-            authorizedConsumers: { myApp: { read: true, all: false } },
-          },
           {
             actionGroups: [],
             defaultActionGroupId: 'default',
             id: 'myAppAlertType',
             name: 'myAppAlertType',
             producer: 'myApp',
             authorizedConsumers: {
-              myApp: { read: true, all: false },
-              alerts: { read: true, all: false },
+              alerts: { read: true, all: true },
+              myApp: { read: true, all: true },
+              myOtherApp: { read: true, all: true },
             },
           },
         ])
       )
     ).toEqual(
-      `((alert.attributes.alertTypeId:myOtherAppAlertType and alert.attributes.consumer:(myApp)) or (alert.attributes.alertTypeId:myAppAlertType and alert.attributes.consumer:(myApp or alerts)))`
+      esKuery.fromKueryExpression(
+        `((alert.attributes.alertTypeId:myAppAlertType and alert.attributes.consumer:(alerts or myApp or myOtherApp)))`
+      )
     );
   });
 
-  test('3', async () => {
+  test('constructs filter for multiple alert types across authorized consumer', async () => {
     expect(
       asFiltersByAlertTypeAndConsumer(
         new Set([
-          {
-            actionGroups: [],
-            defaultActionGroupId: 'default',
-            id: 'myOtherAppAlertType',
-            name: 'myOtherAppAlertType',
-            producer: 'alerts',
-            authorizedConsumers: { myApp: { read: true, all: false } },
-          },
           {
             actionGroups: [],
             defaultActionGroupId: 'default',
             id: 'myAppAlertType',
             name: 'myAppAlertType',
             producer: 'myApp',
             authorizedConsumers: {
-              myApp: { read: true, all: false },
-              alerts: { read: true, all: false },
-              myOtherApp: { read: true, all: false },
+              alerts: { read: true, all: true },
+              myApp: { read: true, all: true },
+              myOtherApp: { read: true, all: true },
+              myAppWithSubFeature: { read: true, all: true },
             },
           },
-        ])
-      )
-    ).toEqual(
-      `((alert.attributes.alertTypeId:myOtherAppAlertType and alert.attributes.consumer:(myApp)) or (alert.attributes.alertTypeId:myAppAlertType and alert.attributes.consumer:(myApp or alerts or myOtherApp)))`
-    );
-  });
-
-  test('4', async () => {
-    expect(
-      asFiltersByAlertTypeAndConsumer(
-        new Set([
           {
             actionGroups: [],
             defaultActionGroupId: 'default',
             id: 'myOtherAppAlertType',
             name: 'myOtherAppAlertType',
             producer: 'alerts',
-            authorizedConsumers: { myApp: { read: true, all: false } },
-          },
-          {
-            actionGroups: [],
-            defaultActionGroupId: 'default',
-            id: 'myAppAlertType',
-            name: 'myAppAlertType',
-            producer: 'myApp',
             authorizedConsumers: {
-              myApp: { read: true, all: false },
-              alerts: { read: true, all: false },
-              myOtherApp: { read: true, all: false },
+              alerts: { read: true, all: true },
+              myApp: { read: true, all: true },
+              myOtherApp: { read: true, all: true },
+              myAppWithSubFeature: { read: true, all: true },
             },
           },
           {
@@ -158,15 +95,18 @@ describe('asFiltersByAlertTypeAndConsumer', () => {
             name: 'mySecondAppAlertType',
             producer: 'myApp',
             authorizedConsumers: {
-              myApp: { read: true, all: false },
-              alerts: { read: true, all: false },
-              myOtherApp: { read: true, all: false },
+              alerts: { read: true, all: true },
+              myApp: { read: true, all: true },
+              myOtherApp: { read: true, all: true },
+              myAppWithSubFeature: { read: true, all: true },
             },
           },
         ])
       )
     ).toEqual(
-      `((alert.attributes.alertTypeId:myOtherAppAlertType and alert.attributes.consumer:(myApp)) or (alert.attributes.alertTypeId:myAppAlertType and alert.attributes.consumer:(myApp or alerts or myOtherApp)) or (alert.attributes.alertTypeId:mySecondAppAlertType and alert.attributes.consumer:(myApp or alerts or myOtherApp)))`
+      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)))`
+      )
     );
   });
 });

diff --git a/x-pack/plugins/alerts/server/authorization/alerts_authorization_kuery.ts b/x-pack/plugins/alerts/server/authorization/alerts_authorization_kuery.ts
@@ -4,36 +4,40 @@
  * you may not use this file except in compliance with the Elastic License.
  */
 
-import Boom from 'boom';
-import { map, mapValues, remove, fromPairs, has } from 'lodash';
-import { KibanaRequest } from 'src/core/server';
-import { ALERTS_FEATURE_ID } from '../../common';
-import { AlertTypeRegistry } from '../types';
-import { SecurityPluginSetup } from '../../../security/server';
-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 { remove } from 'lodash';
+import { nodeTypes } from '../../../../../src/plugins/data/common';
+import { KueryNode } from '../../../../../src/plugins/data/server';
+import { RegistryAlertTypeWithAuth } from './alerts_authorization';
+
+export const is = (fieldName: string, value: string | KueryNode) =>
+  nodeTypes.function.buildNode('is', fieldName, value);
+
+export const or = ([first, ...args]: KueryNode[]): KueryNode =>
+  args.length ? nodeTypes.function.buildNode('or', [first, or(args)]) : first;
+
+export const and = ([first, ...args]: KueryNode[]): KueryNode =>
+  args.length ? nodeTypes.function.buildNode('and', [first, and(args)]) : first;
 
 export function asFiltersByAlertTypeAndConsumer(
   alertTypes: Set<RegistryAlertTypeWithAuth>
-): string[] {
-  return `(${Array.from(alertTypes)
-    .reduce<string[]>((filters, { id, authorizedConsumers }) => {
+): KueryNode {
+  return or(
+    Array.from(alertTypes).reduce<KueryNode[]>((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 ')}))`
+        and([
+          is(`alert.attributes.alertTypeId`, id),
+          or(
+            Object.keys(authorizedConsumers).map((consumer) => {
+              ensureFieldIsSafeForQuery('consumer', consumer);
+              return is(`alert.attributes.consumer`, consumer);
+            })
+          ),
+        ])
       );
       return filters;
     }, [])
-    .join(' or ')})`;
+  );
 }
 
 export function ensureFieldIsSafeForQuery(field: string, value: string): boolean {