From eb1ae15fae5dab0ad670c521077a2f1b19ea2bf1 Mon Sep 17 00:00:00 2001 From: Gidi Meir Morris Date: Wed, 9 Sep 2020 13:15:41 +0100 Subject: [PATCH] use kuery nodes instead of kuery query --- .../alerts/server/alerts_client.test.ts | 14 ++- x-pack/plugins/alerts/server/alerts_client.ts | 16 +-- .../alerts_authorization.test.ts | 13 +- .../authorization/alerts_authorization.ts | 5 +- .../alerts_authorization_kuery.test.ts | 114 +++++------------- .../alerts_authorization_kuery.ts | 48 ++++---- .../alerts/server/authorization/index.ts | 7 ++ 7 files changed, 82 insertions(+), 135 deletions(-) create mode 100644 x-pack/plugins/alerts/server/authorization/index.ts diff --git a/x-pack/plugins/alerts/server/alerts_client.test.ts b/x-pack/plugins/alerts/server/alerts_client.test.ts index 801c2c8775361..b20e6c6be2ebf 100644 --- 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 index 0703a1e13937c..f8da17f4bf089 100644 --- 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,11 +336,6 @@ export class AlertsClient { logSuccessfulAuthorization, } = await this.authorization.getFindAuthorizationFilter(); - if (authorizationFilter) { - options.filter = options.filter - ? `${options.filter} and ${authorizationFilter}` - : authorizationFilter; - } const { page, per_page: perPage, @@ -351,6 +343,10 @@ export class AlertsClient { saved_objects: data, } = await this.unsecuredSavedObjectsClient.find({ ...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 index ed06c6889c064..d27a98a739f5e 100644 --- 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 index 6994ea44c58c0..1ced28295e38d 100644 --- 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 index 582d1ef432eb8..e4b9f8c54c38d 100644 --- 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,57 +21,22 @@ 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', @@ -85,29 +44,24 @@ describe('asFiltersByAlertTypeAndConsumer', () => { 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', @@ -115,40 +69,23 @@ describe('asFiltersByAlertTypeAndConsumer', () => { 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 index b9196ce5b2941..83d1dfd56909e 100644 --- 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 -): string[] { - return `(${Array.from(alertTypes) - .reduce((filters, { id, authorizedConsumers }) => { +): KueryNode { + return or( + Array.from(alertTypes).reduce((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 { diff --git a/x-pack/plugins/alerts/server/authorization/index.ts b/x-pack/plugins/alerts/server/authorization/index.ts new file mode 100644 index 0000000000000..66c7f0afd0122 --- /dev/null +++ b/x-pack/plugins/alerts/server/authorization/index.ts @@ -0,0 +1,7 @@ +/* + * 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. + */ +export * from './alerts_authorization'; +export * from './alerts_authorization_kuery';