From 5f5ef30e9f9a0fc651afcea619285b220ffbb720 Mon Sep 17 00:00:00 2001 From: Christos Nasikas Date: Thu, 22 Dec 2022 11:41:31 +0200 Subject: [PATCH 01/20] Extend alerts client to update case ids --- .../server/alert_data_client/alerts_client.ts | 71 +++++++++++++++++-- 1 file changed, 65 insertions(+), 6 deletions(-) diff --git a/x-pack/plugins/rule_registry/server/alert_data_client/alerts_client.ts b/x-pack/plugins/rule_registry/server/alert_data_client/alerts_client.ts index 9aa0bbd7343cb..283da274f73e4 100644 --- a/x-pack/plugins/rule_registry/server/alert_data_client/alerts_client.ts +++ b/x-pack/plugins/rule_registry/server/alert_data_client/alerts_client.ts @@ -21,6 +21,7 @@ import { ALERT_STATUS_RECOVERED, ALERT_END, ALERT_STATUS_ACTIVE, + ALERT_CASE_IDS, } from '@kbn/rule-data-utils'; import { @@ -93,6 +94,12 @@ export interface BulkUpdateOptions { query: object | string | undefined | null; } +export interface BulkUpdateCasesOptions { + ids: string[]; + caseIds: string[]; + index: string; +} + interface GetAlertParams { id: string; index?: string; @@ -120,6 +127,9 @@ interface SingleSearchAfterAndAudit { lastSortIds?: Array | undefined; } +// TODO: Maybe move it to Cases or to @kbn/rule-data-utils +const MAX_CASES_PER_ALERT = 10; + /** * Provides apis to interact with alerts as data * ensures the request is authorized to perform read / write actions @@ -161,6 +171,12 @@ export class AlertsClient { : { [ALERT_WORKFLOW_STATUS]: status }; } + private getAlertCaseIdsFieldUpdate(source: ParsedTechnicalFields | undefined, caseIds: string[]) { + const uniqueCaseIds = new Set([...(source?.[ALERT_CASE_IDS] ?? []), ...caseIds]); + + return { [ALERT_CASE_IDS]: Array.from(uniqueCaseIds.values()) }; + } + /** * Accepts an array of ES documents and executes ensureAuthorized for the given operation * @param items @@ -323,14 +339,14 @@ export class AlertsClient { */ private async mgetAlertsAuditOperate({ ids, - status, indexName, operation, + fieldToUpdate, }: { ids: string[]; - status: STATUS_VALUES; indexName: string; operation: ReadOperations.Find | ReadOperations.Get | WriteOperations.Update; + fieldToUpdate: (source: ParsedTechnicalFields | undefined) => Record; }) { try { const mgetRes = await this.esClient.mget({ @@ -353,8 +369,6 @@ export class AlertsClient { } const bulkUpdateRequest = mgetRes.docs.flatMap((item) => { - // @ts-expect-error doesn't handle error branch in MGetResponse - const fieldToUpdate = this.getAlertStatusFieldUpdate(item?._source, status); return [ { update: { @@ -364,7 +378,8 @@ export class AlertsClient { }, { doc: { - ...fieldToUpdate, + // @ts-expect-error doesn't handle error branch in MGetResponse + ...fieldToUpdate(item?._source), }, }, ]; @@ -381,6 +396,30 @@ export class AlertsClient { } } + /** + * When an update by ids is requested, do a multi-get, ensure authz and audit alerts, then execute bulk update + * @param param0 + * @returns + */ + private async mgetAlertsAuditOperateStatus({ + ids, + status, + indexName, + operation, + }: { + ids: string[]; + status: STATUS_VALUES; + indexName: string; + operation: ReadOperations.Find | ReadOperations.Get | WriteOperations.Update; + }) { + return this.mgetAlertsAuditOperate({ + ids, + indexName, + operation, + fieldToUpdate: (source) => this.getAlertStatusFieldUpdate(source, status), + }); + } + private async buildEsQueryWithAuthz( query: object | string | null | undefined, id: string | null | undefined, @@ -676,7 +715,7 @@ export class AlertsClient { }: BulkUpdateOptions) { // rejects at the route level if more than 1000 id's are passed in if (ids != null) { - return this.mgetAlertsAuditOperate({ + return this.mgetAlertsAuditOperateStatus({ ids, status, indexName: index, @@ -726,6 +765,26 @@ export class AlertsClient { } } + public async bulkUpdateCases({ ids, index, caseIds }: BulkUpdateCasesOptions) { + /** + * A document may have already some case ids. The check below + * does not ensure thats. We need to also throw in case + * alert.caseIds + caseIds > MAX_CASES_PER_ALERT + * + * TODO: check for each alert if the validation applies + */ + if (caseIds.length > MAX_CASES_PER_ALERT) { + throw Boom.badRequest(`You cannot attach more than ${MAX_CASES_PER_ALERT} cases to an alert`); + } + + return this.mgetAlertsAuditOperate({ + ids, + indexName: index, + operation: WriteOperations.Update, + fieldToUpdate: (source) => this.getAlertCaseIdsFieldUpdate(source, caseIds), + }); + } + public async find({ query, aggs, From 799d2962719c8e2d2666ca65f4b9ef49d18e5892 Mon Sep 17 00:00:00 2001 From: Christos Nasikas Date: Wed, 14 Dec 2022 16:56:35 +0200 Subject: [PATCH 02/20] Pass the alertsClient to the cases client --- x-pack/plugins/cases/kibana.json | 3 ++- x-pack/plugins/cases/server/client/factory.ts | 7 ++++++- x-pack/plugins/cases/server/client/types.ts | 2 ++ x-pack/plugins/cases/server/plugin.ts | 3 +++ 4 files changed, 13 insertions(+), 2 deletions(-) diff --git a/x-pack/plugins/cases/kibana.json b/x-pack/plugins/cases/kibana.json index af7cdd7a99bed..1e9ef3fe89770 100644 --- a/x-pack/plugins/cases/kibana.json +++ b/x-pack/plugins/cases/kibana.json @@ -32,7 +32,8 @@ "management", "spaces", "security", - "notifications" + "notifications", + "ruleRegistry" ], "requiredBundles": [ "savedObjects" diff --git a/x-pack/plugins/cases/server/client/factory.ts b/x-pack/plugins/cases/server/client/factory.ts index 9816f8444e399..b5dfedff36a8c 100644 --- a/x-pack/plugins/cases/server/client/factory.ts +++ b/x-pack/plugins/cases/server/client/factory.ts @@ -25,6 +25,8 @@ import type { LensServerPluginSetup } from '@kbn/lens-plugin/server'; import type { SpacesPluginStart } from '@kbn/spaces-plugin/server'; import type { LicensingPluginStart } from '@kbn/licensing-plugin/server'; import type { NotificationsPluginStart } from '@kbn/notifications-plugin/server'; +import type { RuleRegistryPluginStartContract } from '@kbn/rule-registry-plugin/server'; + import { SAVED_OBJECT_TYPES } from '../../common/constants'; import { Authorization } from '../authorization/authorization'; import { @@ -53,10 +55,11 @@ interface CasesClientFactoryArgs { actionsPluginStart: ActionsPluginStart; licensingPluginStart: LicensingPluginStart; lensEmbeddableFactory: LensServerPluginSetup['lensEmbeddableFactory']; + notifications: NotificationsPluginStart; + ruleRegistry: RuleRegistryPluginStartContract; persistableStateAttachmentTypeRegistry: PersistableStateAttachmentTypeRegistry; externalReferenceAttachmentTypeRegistry: ExternalReferenceAttachmentTypeRegistry; publicBaseUrl?: IBasePath['publicBaseUrl']; - notifications: NotificationsPluginStart; } /** @@ -127,6 +130,7 @@ export class CasesClientFactory { }); const userInfo = await this.getUserInfo(request); + const alertsClient = await this.options.ruleRegistry.getRacClientWithRequest(request); return createCasesClient({ services, @@ -141,6 +145,7 @@ export class CasesClientFactory { securityStartPlugin: this.options.securityPluginStart, publicBaseUrl: this.options.publicBaseUrl, spaceId: this.options.spacesPluginStart.spacesService.getSpaceId(request), + alertsClient, }); } diff --git a/x-pack/plugins/cases/server/client/types.ts b/x-pack/plugins/cases/server/client/types.ts index 7fded9a5d1a45..0be9911d87c63 100644 --- a/x-pack/plugins/cases/server/client/types.ts +++ b/x-pack/plugins/cases/server/client/types.ts @@ -12,6 +12,7 @@ import type { LensServerPluginSetup } from '@kbn/lens-plugin/server'; import type { SecurityPluginStart } from '@kbn/security-plugin/server'; import type { IBasePath } from '@kbn/core-http-browser'; import type { KueryNode } from '@kbn/es-query'; +import type { AlertsClient } from '@kbn/rule-registry-plugin/server'; import type { CasesFindRequest, User } from '../../common/api'; import type { Authorization } from '../authorization/authorization'; import type { @@ -53,6 +54,7 @@ export interface CasesClientArgs { readonly externalReferenceAttachmentTypeRegistry: ExternalReferenceAttachmentTypeRegistry; readonly securityStartPlugin: SecurityPluginStart; readonly spaceId: string; + readonly alertsClient: AlertsClient; readonly publicBaseUrl?: IBasePath['publicBaseUrl']; } diff --git a/x-pack/plugins/cases/server/plugin.ts b/x-pack/plugins/cases/server/plugin.ts index ac26cfc7b81a0..508dad78a6e4a 100644 --- a/x-pack/plugins/cases/server/plugin.ts +++ b/x-pack/plugins/cases/server/plugin.ts @@ -32,6 +32,7 @@ import type { import type { UsageCollectionSetup } from '@kbn/usage-collection-plugin/server'; import type { LicensingPluginSetup, LicensingPluginStart } from '@kbn/licensing-plugin/server'; import type { NotificationsPluginStart } from '@kbn/notifications-plugin/server'; +import type { RuleRegistryPluginStartContract } from '@kbn/rule-registry-plugin/server'; import { APP_ID } from '../common/constants'; import { @@ -74,6 +75,7 @@ export interface PluginsStart { security: SecurityPluginStart; spaces: SpacesPluginStart; notifications: NotificationsPluginStart; + ruleRegistry: RuleRegistryPluginStartContract; } export class CasePlugin { @@ -202,6 +204,7 @@ export class CasePlugin { externalReferenceAttachmentTypeRegistry: this.externalReferenceAttachmentTypeRegistry, publicBaseUrl: core.http.basePath.publicBaseUrl, notifications: plugins.notifications, + ruleRegistry: plugins.ruleRegistry, }); const client = core.elasticsearch.client; From 235c5e56c8852829716ff46a281576ee80173468 Mon Sep 17 00:00:00 2001 From: Christos Nasikas Date: Thu, 22 Dec 2022 12:16:34 +0200 Subject: [PATCH 03/20] Update alerts schema when attaching alerts to a case --- .../cases/server/client/alerts/types.ts | 4 +- .../plugins/cases/server/client/cases/push.ts | 4 +- .../cases/server/client/cases/update.ts | 8 +-- .../common/models/case_with_comments.ts | 52 ++++++++++++++++--- x-pack/plugins/cases/server/common/utils.ts | 6 +-- .../cases/server/services/alerts/index.ts | 8 +-- .../server/alert_data_client/alerts_client.ts | 2 +- 7 files changed, 62 insertions(+), 22 deletions(-) diff --git a/x-pack/plugins/cases/server/client/alerts/types.ts b/x-pack/plugins/cases/server/client/alerts/types.ts index 23eef9850be24..485a00a10ee79 100644 --- a/x-pack/plugins/cases/server/client/alerts/types.ts +++ b/x-pack/plugins/cases/server/client/alerts/types.ts @@ -24,14 +24,14 @@ export type CasesClientGetAlertsResponse = Alert[]; /** * Defines the fields necessary to update an alert's status. */ -export interface UpdateAlertRequest { +export interface UpdateAlertStatusRequest { id: string; index: string; status: CaseStatuses; } export interface AlertUpdateStatus { - alerts: UpdateAlertRequest[]; + alerts: UpdateAlertStatusRequest[]; } export interface AlertGet { diff --git a/x-pack/plugins/cases/server/client/cases/push.ts b/x-pack/plugins/cases/server/client/cases/push.ts index 9f5e9adbb4815..71a187886de0b 100644 --- a/x-pack/plugins/cases/server/client/cases/push.ts +++ b/x-pack/plugins/cases/server/client/cases/push.ts @@ -31,7 +31,7 @@ import { CASE_COMMENT_SAVED_OBJECT } from '../../../common/constants'; import { createIncident, getDurationInSeconds } from './utils'; import { createCaseError } from '../../common/error'; import { - createAlertUpdateRequest, + createAlertUpdateStatusRequest, flattenCaseSavedObject, getAlertInfoFromComments, } from '../../common/utils'; @@ -68,7 +68,7 @@ const changeAlertsStatusToClose = async ( const alerts = alertAttachments.saved_objects .map((attachment) => - createAlertUpdateRequest({ + createAlertUpdateStatusRequest({ comment: attachment.attributes, status: CaseStatuses.closed, }) diff --git a/x-pack/plugins/cases/server/client/cases/update.ts b/x-pack/plugins/cases/server/client/cases/update.ts index 6a304bf12e3e6..2d1f072e18a66 100644 --- a/x-pack/plugins/cases/server/client/cases/update.ts +++ b/x-pack/plugins/cases/server/client/cases/update.ts @@ -51,11 +51,11 @@ import { arraysDifference, getCaseToUpdate } from '../utils'; import type { AlertService, CasesService } from '../../services'; import { createCaseError } from '../../common/error'; import { - createAlertUpdateRequest, + createAlertUpdateStatusRequest, flattenCaseSavedObject, isCommentRequestTypeAlert, } from '../../common/utils'; -import type { UpdateAlertRequest } from '../alerts/types'; +import type { UpdateAlertStatusRequest } from '../alerts/types'; import type { CasesClientArgs } from '..'; import type { OwnerEntity } from '../../authorization'; import { Operations } from '../../authorization'; @@ -238,14 +238,14 @@ async function updateAlerts({ // create an array of requests that indicate the id, index, and status to update an alert const alertsToUpdate = totalAlerts.saved_objects.reduce( - (acc: UpdateAlertRequest[], alertComment) => { + (acc: UpdateAlertStatusRequest[], alertComment) => { if (isCommentRequestTypeAlert(alertComment.attributes)) { const status = getSyncStatusForComment({ alertComment, casesToSyncToStatus, }); - acc.push(...createAlertUpdateRequest({ comment: alertComment.attributes, status })); + acc.push(...createAlertUpdateStatusRequest({ comment: alertComment.attributes, status })); } return acc; diff --git a/x-pack/plugins/cases/server/common/models/case_with_comments.ts b/x-pack/plugins/cases/server/common/models/case_with_comments.ts index 1cc40b48d51e6..998935b7c473e 100644 --- a/x-pack/plugins/cases/server/common/models/case_with_comments.ts +++ b/x-pack/plugins/cases/server/common/models/case_with_comments.ts @@ -12,6 +12,7 @@ import type { SavedObjectsUpdateOptions, SavedObjectsUpdateResponse, } from '@kbn/core/server'; +import pMap from 'p-map'; import type { CaseResponse, CommentAttributes, @@ -30,6 +31,7 @@ import { import { CASE_SAVED_OBJECT, MAX_ALERTS_PER_CASE, + MAX_CONCURRENT_SEARCHES, MAX_DOCS_PER_PAGE, } from '../../../common/constants'; import type { CasesClientArgs } from '../../client'; @@ -41,11 +43,13 @@ import { flattenCommentSavedObjects, transformNewComment, getOrUpdateLensReferences, - createAlertUpdateRequest, + createAlertUpdateStatusRequest, isCommentRequestTypeAlert, + getAlertInfoFromComments, } from '../utils'; type CaseCommentModelParams = Omit; + const ALERT_LIMIT_MSG = `Case has reached the maximum allowed number (${MAX_ALERTS_PER_CASE}) of attached alerts.`; /** @@ -310,18 +314,21 @@ export class CaseCommentModel { } private async handleAlertComments(attachments: CommentRequest[]) { - const alerts = attachments.filter( - (attachment) => - attachment.type === CommentType.alert && this.caseInfo.attributes.settings.syncAlerts + const alertAttachments = attachments.filter( + (attachment): attachment is CommentRequestAlertType => attachment.type === CommentType.alert ); - await this.updateAlertsStatus(alerts); + if (this.caseInfo.attributes.settings.syncAlerts) { + await this.updateAlertsStatus(alertAttachments); + } + + await this.updateAlertsSchemaWithCaseInfo(alertAttachments); } private async updateAlertsStatus(alerts: CommentRequest[]) { const alertsToUpdate = alerts .map((alert) => - createAlertUpdateRequest({ + createAlertUpdateStatusRequest({ comment: alert, status: this.caseInfo.attributes.status, }) @@ -331,6 +338,39 @@ export class CaseCommentModel { await this.params.services.alertsService.updateAlertsStatus(alertsToUpdate); } + private async updateAlertsSchemaWithCaseInfo(alertAttachments: CommentRequestAlertType[]) { + try { + const alerts = getAlertInfoFromComments(alertAttachments); + const alertsGroupedByIndex = new Map>(); + + for (const alert of alerts) { + const idsSet = alertsGroupedByIndex.get(alert.index) ?? new Set(); + idsSet.add(alert.id); + alertsGroupedByIndex.set(alert.index, idsSet); + } + + await pMap( + alertsGroupedByIndex.entries(), + async ([index, idsSet]) => + this.params.alertsClient.bulkUpdateCases({ + ids: Array.from(idsSet.values()), + index, + caseIds: [this.caseInfo.id], + }), + { + concurrency: MAX_CONCURRENT_SEARCHES, + } + ); + } catch (error) { + throw createCaseError({ + // TODO: better error message + message: `Failed to update alerts case schema: ${error}`, + error, + logger: this.params.logger, + }); + } + } + private async createCommentUserAction( comment: SavedObject, req: CommentRequest diff --git a/x-pack/plugins/cases/server/common/utils.ts b/x-pack/plugins/cases/server/common/utils.ts index 508b98775c619..2e8b0299ba5c5 100644 --- a/x-pack/plugins/cases/server/common/utils.ts +++ b/x-pack/plugins/cases/server/common/utils.ts @@ -48,7 +48,7 @@ import { ConnectorTypes, ExternalReferenceStorageType, } from '../../common/api'; -import type { UpdateAlertRequest } from '../client/alerts/types'; +import type { UpdateAlertStatusRequest } from '../client/alerts/types'; import { parseCommentString, getLensVisualizations, @@ -267,13 +267,13 @@ export const isCommentRequestTypeExternalReferenceSO = ( /** * Adds the ids and indices to a map of statuses */ -export function createAlertUpdateRequest({ +export function createAlertUpdateStatusRequest({ comment, status, }: { comment: CommentRequest; status: CaseStatuses; -}): UpdateAlertRequest[] { +}): UpdateAlertStatusRequest[] { return getAlertInfoFromComments([comment]).map((alert) => ({ ...alert, status })); } diff --git a/x-pack/plugins/cases/server/services/alerts/index.ts b/x-pack/plugins/cases/server/services/alerts/index.ts index 3a36d85fb0288..0eed62a2c2869 100644 --- a/x-pack/plugins/cases/server/services/alerts/index.ts +++ b/x-pack/plugins/cases/server/services/alerts/index.ts @@ -16,7 +16,7 @@ import { CaseStatuses } from '../../../common/api'; import { MAX_ALERTS_PER_CASE, MAX_CONCURRENT_SEARCHES } from '../../../common/constants'; import { createCaseError } from '../../common/error'; import type { AlertInfo } from '../../common/types'; -import type { UpdateAlertRequest } from '../../client/alerts/types'; +import type { UpdateAlertStatusRequest } from '../../client/alerts/types'; import type { AggregationBuilder, AggregationResponse } from '../../client/metrics/types'; export class AlertService { @@ -75,7 +75,7 @@ export class AlertService { }; } - public async updateAlertsStatus(alerts: UpdateAlertRequest[]) { + public async updateAlertsStatus(alerts: UpdateAlertStatusRequest[]) { try { const bucketedAlerts = this.bucketAlertsByIndexAndStatus(alerts); const indexBuckets = Array.from(bucketedAlerts.entries()); @@ -96,7 +96,7 @@ export class AlertService { } private bucketAlertsByIndexAndStatus( - alerts: UpdateAlertRequest[] + alerts: UpdateAlertStatusRequest[] ): Map> { return alerts.reduce>>( (acc, alert) => { @@ -130,7 +130,7 @@ export class AlertService { return isEmpty(alert.id) || isEmpty(alert.index); } - private translateStatus(alert: UpdateAlertRequest): STATUS_VALUES { + private translateStatus(alert: UpdateAlertStatusRequest): STATUS_VALUES { const translatedStatuses: Record = { [CaseStatuses.open]: 'open', [CaseStatuses['in-progress']]: 'acknowledged', diff --git a/x-pack/plugins/rule_registry/server/alert_data_client/alerts_client.ts b/x-pack/plugins/rule_registry/server/alert_data_client/alerts_client.ts index 283da274f73e4..9f7c87f6bffce 100644 --- a/x-pack/plugins/rule_registry/server/alert_data_client/alerts_client.ts +++ b/x-pack/plugins/rule_registry/server/alert_data_client/alerts_client.ts @@ -780,7 +780,7 @@ export class AlertsClient { return this.mgetAlertsAuditOperate({ ids, indexName: index, - operation: WriteOperations.Update, + operation: ReadOperations.Get, fieldToUpdate: (source) => this.getAlertCaseIdsFieldUpdate(source, caseIds), }); } From c466e46141df8185e5cabf5f5afd64b61ae47ce6 Mon Sep 17 00:00:00 2001 From: Christos Nasikas Date: Fri, 23 Dec 2022 13:01:02 +0200 Subject: [PATCH 04/20] Check total cases limit per alert --- .../alert_data_client/alerts_client.mock.ts | 1 + .../server/alert_data_client/alerts_client.ts | 39 +++++++++++++++---- 2 files changed, 32 insertions(+), 8 deletions(-) diff --git a/x-pack/plugins/rule_registry/server/alert_data_client/alerts_client.mock.ts b/x-pack/plugins/rule_registry/server/alert_data_client/alerts_client.mock.ts index e636af632464c..5ea5fa0e8b357 100644 --- a/x-pack/plugins/rule_registry/server/alert_data_client/alerts_client.mock.ts +++ b/x-pack/plugins/rule_registry/server/alert_data_client/alerts_client.mock.ts @@ -17,6 +17,7 @@ const createAlertsClientMock = () => { update: jest.fn(), getAuthorizedAlertsIndices: jest.fn(), bulkUpdate: jest.fn(), + bulkUpdateCases: jest.fn(), find: jest.fn(), getFeatureIdsByRegistrationContexts: jest.fn(), getBrowserFields: jest.fn(), diff --git a/x-pack/plugins/rule_registry/server/alert_data_client/alerts_client.ts b/x-pack/plugins/rule_registry/server/alert_data_client/alerts_client.ts index 9f7c87f6bffce..6914640ee5ae1 100644 --- a/x-pack/plugins/rule_registry/server/alert_data_client/alerts_client.ts +++ b/x-pack/plugins/rule_registry/server/alert_data_client/alerts_client.ts @@ -177,6 +177,14 @@ export class AlertsClient { return { [ALERT_CASE_IDS]: Array.from(uniqueCaseIds.values()) }; } + private validateTotalCasesPerAlert(source: ParsedTechnicalFields | undefined, caseIds: string[]) { + const currentCaseIds = source?.[ALERT_CASE_IDS] ?? []; + + if (currentCaseIds.length + caseIds.length > MAX_CASES_PER_ALERT) { + throw Boom.badRequest(`You cannot attach more than ${MAX_CASES_PER_ALERT} cases to an alert`); + } + } + /** * Accepts an array of ES documents and executes ensureAuthorized for the given operation * @param items @@ -342,11 +350,13 @@ export class AlertsClient { indexName, operation, fieldToUpdate, + validate, }: { ids: string[]; indexName: string; operation: ReadOperations.Find | ReadOperations.Get | WriteOperations.Update; fieldToUpdate: (source: ParsedTechnicalFields | undefined) => Record; + validate?: (source: ParsedTechnicalFields | undefined) => void; }) { try { const mgetRes = await this.esClient.mget({ @@ -368,8 +378,15 @@ export class AlertsClient { ); } - const bulkUpdateRequest = mgetRes.docs.flatMap((item) => { - return [ + const updateRequests = []; + + for (const item of mgetRes.docs) { + if (validate) { + // @ts-expect-error doesn't handle error branch in MGetResponse + validate(item?._source); + } + + updateRequests.push([ { update: { _index: item._index, @@ -382,8 +399,10 @@ export class AlertsClient { ...fieldToUpdate(item?._source), }, }, - ]; - }); + ]); + } + + const bulkUpdateRequest = updateRequests.flat(); const bulkUpdateResponse = await this.esClient.bulk({ refresh: 'wait_for', @@ -767,11 +786,14 @@ export class AlertsClient { public async bulkUpdateCases({ ids, index, caseIds }: BulkUpdateCasesOptions) { /** - * A document may have already some case ids. The check below - * does not ensure thats. We need to also throw in case - * alert.caseIds + caseIds > MAX_CASES_PER_ALERT + * First check in case the caseIds are more that the allowed limit. + * We do this check to avoid any mget calls or authorization checks + * + * The check below does not ensure that an alert may exceed the limit + * if we add the casedIds. We need to also throw in case + * alert.caseIds + caseIds > MAX_CASES_PER_ALERT. The validateTotalCasesPerAlert + * function ensures that. * - * TODO: check for each alert if the validation applies */ if (caseIds.length > MAX_CASES_PER_ALERT) { throw Boom.badRequest(`You cannot attach more than ${MAX_CASES_PER_ALERT} cases to an alert`); @@ -782,6 +804,7 @@ export class AlertsClient { indexName: index, operation: ReadOperations.Get, fieldToUpdate: (source) => this.getAlertCaseIdsFieldUpdate(source, caseIds), + validate: (source) => this.validateTotalCasesPerAlert(source, caseIds), }); } From 9796e971395df0a3ff4931cc80c21f4c84c22e55 Mon Sep 17 00:00:00 2001 From: Christos Nasikas Date: Fri, 23 Dec 2022 15:04:30 +0200 Subject: [PATCH 05/20] Add integration checks --- .../cases_api_integration/common/lib/utils.ts | 38 ++++++ .../tests/common/comments/post_comment.ts | 110 ++++++++++++------ .../internal/bulk_create_attachments.ts | 108 +++++++++++++---- 3 files changed, 200 insertions(+), 56 deletions(-) diff --git a/x-pack/test/cases_api_integration/common/lib/utils.ts b/x-pack/test/cases_api_integration/common/lib/utils.ts index 0fd34bddabbd6..7453fa49beb4a 100644 --- a/x-pack/test/cases_api_integration/common/lib/utils.ts +++ b/x-pack/test/cases_api_integration/common/lib/utils.ts @@ -60,12 +60,24 @@ import { ActionResult, FindActionResult } from '@kbn/actions-plugin/server/types import { ESCasesConfigureAttributes } from '@kbn/cases-plugin/server/services/configure/types'; import { ESCaseAttributes } from '@kbn/cases-plugin/server/services/cases/types'; import type { SavedObjectsRawDocSource } from '@kbn/core/server'; +import { ToolingLog } from '@kbn/tooling-log'; +import { DETECTION_ENGINE_QUERY_SIGNALS_URL } from '@kbn/security-solution-plugin/common/constants'; +import { DetectionAlert } from '@kbn/security-solution-plugin/common/detection_engine/schemas/alerts'; +import { RiskEnrichmentFields } from '@kbn/security-solution-plugin/server/lib/detection_engine/signals/enrichments/types'; import { User } from './authentication/types'; import { superUser } from './authentication/users'; import { getPostCaseRequest, postCaseReq } from './mock'; import { ObjectRemover as ActionsRemover } from '../../../alerting_api_integration/common/lib'; import { getServiceNowServer } from '../../../alerting_api_integration/common/fixtures/plugins/actions_simulators/server/plugin'; import { RecordingServiceNowSimulator } from '../../../alerting_api_integration/common/fixtures/plugins/actions_simulators/server/servicenow_simulation'; +import { + getRuleForSignalTesting, + createRule, + waitForRuleSuccessOrStatus, + waitForSignalsToBePresent, + getSignalsByIds, + getQuerySignalIds, +} from '../../../detection_engine_api_integration/utils'; function toArray(input: T | T[]): T[] { if (Array.isArray(input)) { @@ -1418,3 +1430,29 @@ export const getReferenceFromEsResponse = ( esResponse: TransportResult, unknown>, id: string ) => esResponse.body._source?.references?.find((r) => r.id === id); + +export const createSecuritySolutionAlerts = async ( + supertest: SuperTest.SuperTest, + log: ToolingLog +): Promise> => { + const rule = getRuleForSignalTesting(['auditbeat-*']); + const { id } = await createRule(supertest, log, rule); + await waitForRuleSuccessOrStatus(supertest, log, id); + await waitForSignalsToBePresent(supertest, log, 1, [id]); + const signals = await getSignalsByIds(supertest, log, [id]); + + return signals; +}; + +export const getSecuritySolutionAlerts = async ( + supertest: SuperTest.SuperTest, + alertIds: string[] +): Promise> => { + const { body: updatedAlert } = await supertest + .post(DETECTION_ENGINE_QUERY_SIGNALS_URL) + .set('kbn-xsrf', 'true') + .send(getQuerySignalIds(alertIds)) + .expect(200); + + return updatedAlert; +}; diff --git a/x-pack/test/cases_api_integration/security_and_spaces/tests/common/comments/post_comment.ts b/x-pack/test/cases_api_integration/security_and_spaces/tests/common/comments/post_comment.ts index 681f026246542..79c6c70e4f9c4 100644 --- a/x-pack/test/cases_api_integration/security_and_spaces/tests/common/comments/post_comment.ts +++ b/x-pack/test/cases_api_integration/security_and_spaces/tests/common/comments/post_comment.ts @@ -7,9 +7,8 @@ import { omit } from 'lodash/fp'; import expect from '@kbn/expect'; -import { ALERT_WORKFLOW_STATUS } from '@kbn/rule-data-utils'; +import { ALERT_CASE_IDS, ALERT_WORKFLOW_STATUS } from '@kbn/rule-data-utils'; -import { DETECTION_ENGINE_QUERY_SIGNALS_URL } from '@kbn/security-solution-plugin/common/constants'; import { CommentType, AttributesTypeUser, @@ -36,17 +35,13 @@ import { removeServerGeneratedPropertiesFromSavedObject, superUserSpace1Auth, updateCase, + createSecuritySolutionAlerts, + getSecuritySolutionAlerts, } from '../../../../common/lib/utils'; import { createSignalsIndex, deleteSignalsIndex, deleteAllAlerts, - getRuleForSignalTesting, - waitForRuleSuccessOrStatus, - waitForSignalsToBePresent, - getSignalsByIds, - createRule, - getQuerySignalIds, } from '../../../../../detection_engine_api_integration/utils'; import { globalRead, @@ -362,11 +357,35 @@ export default ({ getService }: FtrProviderContext): void => { await esArchiver.unload('x-pack/test/functional/es_archives/auditbeat/hosts'); }); + const createCommentAndRefreshIndex = async ( + caseId: string, + alertId: string, + alertIndex: string, + expectedHttpCode?: number + ) => { + await createComment({ + supertest, + caseId, + params: { + alertId, + index: alertIndex, + rule: { + id: 'id', + name: 'name', + }, + owner: 'securitySolutionFixture', + type: CommentType.alert, + }, + expectedHttpCode, + }); + + await es.indices.refresh({ index: alertIndex }); + }; + const bulkCreateAlertsAndVerifyAlertStatus = async ( syncAlerts: boolean, expectedAlertStatus: string ) => { - const rule = getRuleForSignalTesting(['auditbeat-*']); const postedCase = await createCase(supertest, { ...postCaseReq, settings: { syncAlerts }, @@ -385,38 +404,41 @@ export default ({ getService }: FtrProviderContext): void => { }, }); - const { id } = await createRule(supertest, log, rule); - await waitForRuleSuccessOrStatus(supertest, log, id); - await waitForSignalsToBePresent(supertest, log, 1, [id]); - const signals = await getSignalsByIds(supertest, log, [id]); + const signals = await createSecuritySolutionAlerts(supertest, log); const alert = signals.hits.hits[0]; expect(alert._source?.[ALERT_WORKFLOW_STATUS]).eql('open'); - await createComment({ - supertest, - caseId: postedCase.id, - params: { - alertId: alert._id, - index: alert._index, - rule: { - id: 'id', - name: 'name', - }, - owner: 'securitySolutionFixture', - type: CommentType.alert, - }, - }); + await createCommentAndRefreshIndex(postedCase.id, alert._id, alert._index); + + const updatedAlert = await getSecuritySolutionAlerts(supertest, [alert._id]); + + expect(updatedAlert.hits.hits[0]._source?.[ALERT_WORKFLOW_STATUS]).eql(expectedAlertStatus); + }; + + const bulkCreateAlertsAndVerifyCaseIdsInAlertSchema = async (totalCases: number) => { + const cases = await Promise.all( + [...Array(totalCases).keys()].map((index) => + createCase(supertest, { + ...postCaseReq, + settings: { syncAlerts: false }, + }) + ) + ); + + const signals = await createSecuritySolutionAlerts(supertest, log); + const alert = signals.hits.hits[0]; + + for (const theCase of cases) { + await createCommentAndRefreshIndex(theCase.id, alert._id, alert._index); + } - await es.indices.refresh({ index: alert._index }); + const updatedAlert = await getSecuritySolutionAlerts(supertest, [alert._id]); + const caseIds = cases.map((theCase) => theCase.id); - const { body: updatedAlert } = await supertest - .post(DETECTION_ENGINE_QUERY_SIGNALS_URL) - .set('kbn-xsrf', 'true') - .send(getQuerySignalIds([alert._id])) - .expect(200); + expect(updatedAlert.hits.hits[0]._source?.[ALERT_CASE_IDS]).eql(caseIds); - expect(updatedAlert.hits.hits[0]._source[ALERT_WORKFLOW_STATUS]).eql(expectedAlertStatus); + return updatedAlert; }; it('should change the status of the alert if sync alert is on', async () => { @@ -426,6 +448,26 @@ export default ({ getService }: FtrProviderContext): void => { it('should NOT change the status of the alert if sync alert is off', async () => { await bulkCreateAlertsAndVerifyAlertStatus(false, 'open'); }); + + it('should add the case ID to the alert schema', async () => { + await bulkCreateAlertsAndVerifyCaseIdsInAlertSchema(1); + }); + + it('should add multiple case ids to the alert schema', async () => { + await bulkCreateAlertsAndVerifyCaseIdsInAlertSchema(2); + }); + + it('should not add more than 10 cases to an alert', async () => { + const updatedAlert = await bulkCreateAlertsAndVerifyCaseIdsInAlertSchema(10); + const alert = updatedAlert.hits.hits[0]; + + const postedCase = await createCase(supertest, { + ...postCaseReq, + settings: { syncAlerts: false }, + }); + + createCommentAndRefreshIndex(postedCase.id, alert._id, alert._index, 400); + }); }); describe('alert format', () => { diff --git a/x-pack/test/cases_api_integration/security_and_spaces/tests/common/internal/bulk_create_attachments.ts b/x-pack/test/cases_api_integration/security_and_spaces/tests/common/internal/bulk_create_attachments.ts index f3c1b190260bc..4e3f88d042a2b 100644 --- a/x-pack/test/cases_api_integration/security_and_spaces/tests/common/internal/bulk_create_attachments.ts +++ b/x-pack/test/cases_api_integration/security_and_spaces/tests/common/internal/bulk_create_attachments.ts @@ -7,9 +7,8 @@ import { omit } from 'lodash/fp'; import expect from '@kbn/expect'; -import { ALERT_WORKFLOW_STATUS } from '@kbn/rule-data-utils'; +import { ALERT_CASE_IDS, ALERT_WORKFLOW_STATUS } from '@kbn/rule-data-utils'; -import { DETECTION_ENGINE_QUERY_SIGNALS_URL } from '@kbn/security-solution-plugin/common/constants'; import { BulkCreateCommentRequest, CaseResponse, @@ -35,17 +34,13 @@ import { createCaseAndBulkCreateAttachments, bulkCreateAttachments, updateCase, + createSecuritySolutionAlerts, + getSecuritySolutionAlerts, } from '../../../../common/lib/utils'; import { createSignalsIndex, deleteSignalsIndex, deleteAllAlerts, - getRuleForSignalTesting, - waitForRuleSuccessOrStatus, - waitForSignalsToBePresent, - getSignalsByIds, - createRule, - getQuerySignalIds, } from '../../../../../detection_engine_api_integration/utils'; import { globalRead, @@ -483,7 +478,6 @@ export default ({ getService }: FtrProviderContext): void => { syncAlerts: boolean, expectedAlertStatus: string ) => { - const rule = getRuleForSignalTesting(['auditbeat-*']); const postedCase = await createCase(supertest, { ...postCaseReq, settings: { syncAlerts }, @@ -502,10 +496,8 @@ export default ({ getService }: FtrProviderContext): void => { }, }); - const { id } = await createRule(supertest, log, rule); - await waitForRuleSuccessOrStatus(supertest, log, id); - await waitForSignalsToBePresent(supertest, log, 1, [id]); - const signals = await getSignalsByIds(supertest, log, [id]); + const signals = await createSecuritySolutionAlerts(supertest, log); + const attachments: CommentRequest[] = []; const indices: string[] = []; const ids: string[] = []; @@ -535,17 +527,53 @@ export default ({ getService }: FtrProviderContext): void => { await es.indices.refresh({ index: indices }); - const { body: updatedAlerts } = await supertest - .post(DETECTION_ENGINE_QUERY_SIGNALS_URL) - .set('kbn-xsrf', 'true') - .send(getQuerySignalIds(ids)) - .expect(200); + const updatedAlerts = await getSecuritySolutionAlerts(supertest, ids); + + updatedAlerts.hits.hits.forEach((alert) => { + expect(alert._source?.[ALERT_WORKFLOW_STATUS]).eql(expectedAlertStatus); + }); + }; - updatedAlerts.hits.hits.forEach( - (alert: { _source: { 'kibana.alert.workflow_status': string } }) => { - expect(alert._source[ALERT_WORKFLOW_STATUS]).eql(expectedAlertStatus); - } + const bulkCreateAlertsAndVerifyCaseIdsInAlertSchema = async (totalCases: number) => { + const cases = await Promise.all( + [...Array(totalCases).keys()].map((index) => + createCase(supertest, { + ...postCaseReq, + settings: { syncAlerts: false }, + }) + ) ); + + const signals = await createSecuritySolutionAlerts(supertest, log); + const alert = signals.hits.hits[0]; + + for (const theCase of cases) { + await bulkCreateAttachments({ + supertest, + caseId: theCase.id, + params: [ + { + alertId: alert._id, + index: alert._index, + rule: { + id: 'id', + name: 'name', + }, + owner: 'securitySolutionFixture', + type: CommentType.alert, + }, + ], + }); + } + + await es.indices.refresh({ index: alert._index }); + + const updatedAlert = await getSecuritySolutionAlerts(supertest, [alert._id]); + const caseIds = cases.map((theCase) => theCase.id); + + expect(updatedAlert.hits.hits[0]._source?.[ALERT_CASE_IDS]).eql(caseIds); + + return updatedAlert; }; it('should change the status of the alerts if sync alert is on', async () => { @@ -555,6 +583,42 @@ export default ({ getService }: FtrProviderContext): void => { it('should NOT change the status of the alert if sync alert is off', async () => { await bulkCreateAlertsAndVerifyAlertStatus(false, 'open'); }); + + it('should add the case ID to the alert schema', async () => { + await bulkCreateAlertsAndVerifyCaseIdsInAlertSchema(1); + }); + + it('should add multiple case ids to the alert schema', async () => { + await bulkCreateAlertsAndVerifyCaseIdsInAlertSchema(2); + }); + + it('should not add more than 10 cases to an alert', async () => { + const updatedAlert = await bulkCreateAlertsAndVerifyCaseIdsInAlertSchema(10); + const alert = updatedAlert.hits.hits[0]; + + const postedCase = await createCase(supertest, { + ...postCaseReq, + settings: { syncAlerts: false }, + }); + + await bulkCreateAttachments({ + supertest, + caseId: postedCase.id, + params: [ + { + alertId: alert._id, + index: alert._index, + rule: { + id: 'id', + name: 'name', + }, + owner: 'securitySolutionFixture', + type: CommentType.alert, + }, + ], + expectedHttpCode: 400, + }); + }); }); describe('alert format', () => { From 340b86f16faa43eb47c4c064cdaae9a6550f8030 Mon Sep 17 00:00:00 2001 From: Christos Nasikas Date: Fri, 23 Dec 2022 15:11:58 +0200 Subject: [PATCH 06/20] Fix types --- x-pack/plugins/cases/server/client/mocks.ts | 2 ++ x-pack/plugins/cases/server/client/types.ts | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/x-pack/plugins/cases/server/client/mocks.ts b/x-pack/plugins/cases/server/client/mocks.ts index 79e307e98f8f7..d0f0d16e70129 100644 --- a/x-pack/plugins/cases/server/client/mocks.ts +++ b/x-pack/plugins/cases/server/client/mocks.ts @@ -10,6 +10,7 @@ import { loggingSystemMock, savedObjectsClientMock } from '@kbn/core/server/mock import { securityMock } from '@kbn/security-plugin/server/mocks'; import { actionsClientMock } from '@kbn/actions-plugin/server/actions_client.mock'; +import { alertsClientMock } from '@kbn/rule-registry-plugin/server/alert_data_client/alerts_client.mock'; import { makeLensEmbeddableFactory } from '@kbn/lens-plugin/server/embeddable/make_lens_embeddable_factory'; import type { CasesClient } from '.'; import { createAuthorizationMock } from '../authorization/mock'; @@ -140,6 +141,7 @@ export const createCasesClientMockArgs = () => { logger: loggingSystemMock.createLogger(), unsecuredSavedObjectsClient: savedObjectsClientMock.create(), actionsClient: actionsClientMock.create(), + alertsClient: alertsClientMock.create(), user: { username: 'damaged_raccoon', email: 'damaged_raccoon@elastic.co', diff --git a/x-pack/plugins/cases/server/client/types.ts b/x-pack/plugins/cases/server/client/types.ts index 0be9911d87c63..10ac975c1cb70 100644 --- a/x-pack/plugins/cases/server/client/types.ts +++ b/x-pack/plugins/cases/server/client/types.ts @@ -54,7 +54,7 @@ export interface CasesClientArgs { readonly externalReferenceAttachmentTypeRegistry: ExternalReferenceAttachmentTypeRegistry; readonly securityStartPlugin: SecurityPluginStart; readonly spaceId: string; - readonly alertsClient: AlertsClient; + readonly alertsClient: PublicMethodsOf; readonly publicBaseUrl?: IBasePath['publicBaseUrl']; } From f9205c05104881a4b0748d53c489c897592526c6 Mon Sep 17 00:00:00 2001 From: Christos Nasikas Date: Wed, 11 Jan 2023 18:09:39 +0200 Subject: [PATCH 07/20] Add o11y tests --- .../common/lib/alerts.ts | 76 +++++ .../cases_api_integration/common/lib/utils.ts | 38 --- .../tests/common/comments/post_comment.ts | 287 ++++++++++------ .../internal/bulk_create_attachments.ts | 317 ++++++++++++------ 4 files changed, 474 insertions(+), 244 deletions(-) create mode 100644 x-pack/test/cases_api_integration/common/lib/alerts.ts diff --git a/x-pack/test/cases_api_integration/common/lib/alerts.ts b/x-pack/test/cases_api_integration/common/lib/alerts.ts new file mode 100644 index 0000000000000..f5584b61cb934 --- /dev/null +++ b/x-pack/test/cases_api_integration/common/lib/alerts.ts @@ -0,0 +1,76 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0; you may not use this file except in compliance with the Elastic License + * 2.0. + */ + +import type SuperTest from 'supertest'; +import type * as estypes from '@elastic/elasticsearch/lib/api/typesWithBodyKey'; +import { ToolingLog } from '@kbn/tooling-log'; +import { DETECTION_ENGINE_QUERY_SIGNALS_URL } from '@kbn/security-solution-plugin/common/constants'; +import { DetectionAlert } from '@kbn/security-solution-plugin/common/detection_engine/schemas/alerts'; +import { RiskEnrichmentFields } from '@kbn/security-solution-plugin/server/lib/detection_engine/signals/enrichments/types'; +import { + getRuleForSignalTesting, + createRule, + waitForRuleSuccessOrStatus, + waitForSignalsToBePresent, + getSignalsByIds, + getQuerySignalIds, +} from '../../../detection_engine_api_integration/utils'; +import { superUser } from './authentication/users'; +import { getSpaceUrlPrefix } from './utils'; +import { User } from './authentication/types'; + +export const createSecuritySolutionAlerts = async ( + supertest: SuperTest.SuperTest, + log: ToolingLog +): Promise> => { + const rule = getRuleForSignalTesting(['auditbeat-*']); + const { id } = await createRule(supertest, log, rule); + await waitForRuleSuccessOrStatus(supertest, log, id); + await waitForSignalsToBePresent(supertest, log, 1, [id]); + const signals = await getSignalsByIds(supertest, log, [id]); + + return signals; +}; + +export const getSecuritySolutionAlerts = async ( + supertest: SuperTest.SuperTest, + alertIds: string[] +): Promise> => { + const { body: updatedAlert } = await supertest + .post(DETECTION_ENGINE_QUERY_SIGNALS_URL) + .set('kbn-xsrf', 'true') + .send(getQuerySignalIds(alertIds)) + .expect(200); + + return updatedAlert; +}; + +interface AlertResponse { + 'kibana.alert.case_ids'?: string[]; +} + +export const getAlertById = async ({ + supertest, + id, + index, + expectedHttpCode = 200, + auth = { user: superUser, space: null }, +}: { + supertest: SuperTest.SuperTest; + id: string; + index: string; + expectedHttpCode?: number; + auth?: { user: User; space: string | null }; +}): Promise => { + const { body: alert } = await supertest + .get(`${getSpaceUrlPrefix(auth?.space)}/internal/rac/alerts?id=${id}&index=${index}`) + .auth(auth.user.username, auth.user.password) + .set('kbn-xsrf', 'true') + .expect(expectedHttpCode); + + return alert; +}; diff --git a/x-pack/test/cases_api_integration/common/lib/utils.ts b/x-pack/test/cases_api_integration/common/lib/utils.ts index 7453fa49beb4a..0fd34bddabbd6 100644 --- a/x-pack/test/cases_api_integration/common/lib/utils.ts +++ b/x-pack/test/cases_api_integration/common/lib/utils.ts @@ -60,24 +60,12 @@ import { ActionResult, FindActionResult } from '@kbn/actions-plugin/server/types import { ESCasesConfigureAttributes } from '@kbn/cases-plugin/server/services/configure/types'; import { ESCaseAttributes } from '@kbn/cases-plugin/server/services/cases/types'; import type { SavedObjectsRawDocSource } from '@kbn/core/server'; -import { ToolingLog } from '@kbn/tooling-log'; -import { DETECTION_ENGINE_QUERY_SIGNALS_URL } from '@kbn/security-solution-plugin/common/constants'; -import { DetectionAlert } from '@kbn/security-solution-plugin/common/detection_engine/schemas/alerts'; -import { RiskEnrichmentFields } from '@kbn/security-solution-plugin/server/lib/detection_engine/signals/enrichments/types'; import { User } from './authentication/types'; import { superUser } from './authentication/users'; import { getPostCaseRequest, postCaseReq } from './mock'; import { ObjectRemover as ActionsRemover } from '../../../alerting_api_integration/common/lib'; import { getServiceNowServer } from '../../../alerting_api_integration/common/fixtures/plugins/actions_simulators/server/plugin'; import { RecordingServiceNowSimulator } from '../../../alerting_api_integration/common/fixtures/plugins/actions_simulators/server/servicenow_simulation'; -import { - getRuleForSignalTesting, - createRule, - waitForRuleSuccessOrStatus, - waitForSignalsToBePresent, - getSignalsByIds, - getQuerySignalIds, -} from '../../../detection_engine_api_integration/utils'; function toArray(input: T | T[]): T[] { if (Array.isArray(input)) { @@ -1430,29 +1418,3 @@ export const getReferenceFromEsResponse = ( esResponse: TransportResult, unknown>, id: string ) => esResponse.body._source?.references?.find((r) => r.id === id); - -export const createSecuritySolutionAlerts = async ( - supertest: SuperTest.SuperTest, - log: ToolingLog -): Promise> => { - const rule = getRuleForSignalTesting(['auditbeat-*']); - const { id } = await createRule(supertest, log, rule); - await waitForRuleSuccessOrStatus(supertest, log, id); - await waitForSignalsToBePresent(supertest, log, 1, [id]); - const signals = await getSignalsByIds(supertest, log, [id]); - - return signals; -}; - -export const getSecuritySolutionAlerts = async ( - supertest: SuperTest.SuperTest, - alertIds: string[] -): Promise> => { - const { body: updatedAlert } = await supertest - .post(DETECTION_ENGINE_QUERY_SIGNALS_URL) - .set('kbn-xsrf', 'true') - .send(getQuerySignalIds(alertIds)) - .expect(200); - - return updatedAlert; -}; diff --git a/x-pack/test/cases_api_integration/security_and_spaces/tests/common/comments/post_comment.ts b/x-pack/test/cases_api_integration/security_and_spaces/tests/common/comments/post_comment.ts index 79c6c70e4f9c4..82b059417a5cb 100644 --- a/x-pack/test/cases_api_integration/security_and_spaces/tests/common/comments/post_comment.ts +++ b/x-pack/test/cases_api_integration/security_and_spaces/tests/common/comments/post_comment.ts @@ -35,8 +35,6 @@ import { removeServerGeneratedPropertiesFromSavedObject, superUserSpace1Auth, updateCase, - createSecuritySolutionAlerts, - getSecuritySolutionAlerts, } from '../../../../common/lib/utils'; import { createSignalsIndex, @@ -53,6 +51,11 @@ import { secOnlyRead, superUser, } from '../../../../common/lib/authentication/users'; +import { + getSecuritySolutionAlerts, + createSecuritySolutionAlerts, + getAlertById, +} from '../../../../common/lib/alerts'; // eslint-disable-next-line import/no-default-export export default ({ getService }: FtrProviderContext): void => { @@ -346,127 +349,219 @@ export default ({ getService }: FtrProviderContext): void => { }); describe('alerts', () => { - beforeEach(async () => { - await esArchiver.load('x-pack/test/functional/es_archives/auditbeat/hosts'); - await createSignalsIndex(supertest, log); - }); + describe('security_solution', () => { + beforeEach(async () => { + await esArchiver.load('x-pack/test/functional/es_archives/auditbeat/hosts'); + await createSignalsIndex(supertest, log); + }); - afterEach(async () => { - await deleteSignalsIndex(supertest, log); - await deleteAllAlerts(supertest, log); - await esArchiver.unload('x-pack/test/functional/es_archives/auditbeat/hosts'); - }); + afterEach(async () => { + await deleteSignalsIndex(supertest, log); + await deleteAllAlerts(supertest, log); + await esArchiver.unload('x-pack/test/functional/es_archives/auditbeat/hosts'); + }); - const createCommentAndRefreshIndex = async ( - caseId: string, - alertId: string, - alertIndex: string, - expectedHttpCode?: number - ) => { - await createComment({ - supertest, - caseId, - params: { - alertId, - index: alertIndex, - rule: { - id: 'id', - name: 'name', + const createCommentAndRefreshIndex = async ( + caseId: string, + alertId: string, + alertIndex: string, + expectedHttpCode?: number + ) => { + await createComment({ + supertest, + caseId, + params: { + alertId, + index: alertIndex, + rule: { + id: 'id', + name: 'name', + }, + owner: 'securitySolutionFixture', + type: CommentType.alert, }, - owner: 'securitySolutionFixture', - type: CommentType.alert, - }, - expectedHttpCode, + expectedHttpCode, + }); + + await es.indices.refresh({ index: alertIndex }); + }; + + const bulkCreateAlertsAndVerifyAlertStatus = async ( + syncAlerts: boolean, + expectedAlertStatus: string + ) => { + const postedCase = await createCase(supertest, { + ...postCaseReq, + settings: { syncAlerts }, + }); + + await updateCase({ + supertest, + params: { + cases: [ + { + id: postedCase.id, + version: postedCase.version, + status: CaseStatuses['in-progress'], + }, + ], + }, + }); + + const signals = await createSecuritySolutionAlerts(supertest, log); + + const alert = signals.hits.hits[0]; + expect(alert._source?.[ALERT_WORKFLOW_STATUS]).eql('open'); + + await createCommentAndRefreshIndex(postedCase.id, alert._id, alert._index); + + const updatedAlert = await getSecuritySolutionAlerts(supertest, [alert._id]); + + expect(updatedAlert.hits.hits[0]._source?.[ALERT_WORKFLOW_STATUS]).eql( + expectedAlertStatus + ); + }; + + const bulkCreateAlertsAndVerifyCaseIdsInAlertSchema = async (totalCases: number) => { + const cases = await Promise.all( + [...Array(totalCases).keys()].map((index) => + createCase(supertest, { + ...postCaseReq, + settings: { syncAlerts: false }, + }) + ) + ); + + const signals = await createSecuritySolutionAlerts(supertest, log); + const alert = signals.hits.hits[0]; + + for (const theCase of cases) { + await createCommentAndRefreshIndex(theCase.id, alert._id, alert._index); + } + + const updatedAlert = await getSecuritySolutionAlerts(supertest, [alert._id]); + const caseIds = cases.map((theCase) => theCase.id); + + expect(updatedAlert.hits.hits[0]._source?.[ALERT_CASE_IDS]).eql(caseIds); + + return updatedAlert; + }; + + it('should change the status of the alert if sync alert is on', async () => { + await bulkCreateAlertsAndVerifyAlertStatus(true, 'acknowledged'); }); - await es.indices.refresh({ index: alertIndex }); - }; + it('should NOT change the status of the alert if sync alert is off', async () => { + await bulkCreateAlertsAndVerifyAlertStatus(false, 'open'); + }); - const bulkCreateAlertsAndVerifyAlertStatus = async ( - syncAlerts: boolean, - expectedAlertStatus: string - ) => { - const postedCase = await createCase(supertest, { - ...postCaseReq, - settings: { syncAlerts }, + it('should add the case ID to the alert schema', async () => { + await bulkCreateAlertsAndVerifyCaseIdsInAlertSchema(1); }); - await updateCase({ - supertest, - params: { - cases: [ - { - id: postedCase.id, - version: postedCase.version, - status: CaseStatuses['in-progress'], - }, - ], - }, + it('should add multiple case ids to the alert schema', async () => { + await bulkCreateAlertsAndVerifyCaseIdsInAlertSchema(2); }); - const signals = await createSecuritySolutionAlerts(supertest, log); + it('should not add more than 10 cases to an alert', async () => { + const updatedAlert = await bulkCreateAlertsAndVerifyCaseIdsInAlertSchema(10); + const alert = updatedAlert.hits.hits[0]; - const alert = signals.hits.hits[0]; - expect(alert._source?.[ALERT_WORKFLOW_STATUS]).eql('open'); + const postedCase = await createCase(supertest, { + ...postCaseReq, + settings: { syncAlerts: false }, + }); - await createCommentAndRefreshIndex(postedCase.id, alert._id, alert._index); + createCommentAndRefreshIndex(postedCase.id, alert._id, alert._index, 400); + }); + }); - const updatedAlert = await getSecuritySolutionAlerts(supertest, [alert._id]); + describe('observability', () => { + const alertId = 'NoxgpHkBqbdrfX07MqXV'; + const ampIndex = '.alerts-observability.apm.alerts'; - expect(updatedAlert.hits.hits[0]._source?.[ALERT_WORKFLOW_STATUS]).eql(expectedAlertStatus); - }; + beforeEach(async () => { + await esArchiver.load('x-pack/test/functional/es_archives/rule_registry/alerts'); + }); - const bulkCreateAlertsAndVerifyCaseIdsInAlertSchema = async (totalCases: number) => { - const cases = await Promise.all( - [...Array(totalCases).keys()].map((index) => - createCase(supertest, { - ...postCaseReq, - settings: { syncAlerts: false }, - }) - ) - ); + afterEach(async () => { + await esArchiver.unload('x-pack/test/functional/es_archives/rule_registry/alerts'); + }); - const signals = await createSecuritySolutionAlerts(supertest, log); - const alert = signals.hits.hits[0]; + const bulkCreateAlertsAndVerifyCaseIdsInAlertSchema = async (totalCases: number) => { + const cases = await Promise.all( + [...Array(totalCases).keys()].map((index) => + createCase(supertest, { + ...postCaseReq, + owner: 'observabilityFixture', + settings: { syncAlerts: false }, + }) + ) + ); - for (const theCase of cases) { - await createCommentAndRefreshIndex(theCase.id, alert._id, alert._index); - } + for (const theCase of cases) { + await createComment({ + supertest, + caseId: theCase.id, + params: { + alertId, + index: ampIndex, + rule: { + id: 'id', + name: 'name', + }, + owner: 'observabilityFixture', + type: CommentType.alert, + }, + }); + } - const updatedAlert = await getSecuritySolutionAlerts(supertest, [alert._id]); - const caseIds = cases.map((theCase) => theCase.id); + const alert = await getAlertById({ + supertest, + id: alertId, + index: ampIndex, + auth: { user: superUser, space: 'space1' }, + }); - expect(updatedAlert.hits.hits[0]._source?.[ALERT_CASE_IDS]).eql(caseIds); + const caseIds = cases.map((theCase) => theCase.id); - return updatedAlert; - }; + expect(alert['kibana.alert.case_ids']).eql(caseIds); - it('should change the status of the alert if sync alert is on', async () => { - await bulkCreateAlertsAndVerifyAlertStatus(true, 'acknowledged'); - }); + return alert; + }; - it('should NOT change the status of the alert if sync alert is off', async () => { - await bulkCreateAlertsAndVerifyAlertStatus(false, 'open'); - }); + it('should add the case ID to the alert schema', async () => { + await bulkCreateAlertsAndVerifyCaseIdsInAlertSchema(1); + }); - it('should add the case ID to the alert schema', async () => { - await bulkCreateAlertsAndVerifyCaseIdsInAlertSchema(1); - }); + it('should add multiple case ids to the alert schema', async () => { + await bulkCreateAlertsAndVerifyCaseIdsInAlertSchema(2); + }); - it('should add multiple case ids to the alert schema', async () => { - await bulkCreateAlertsAndVerifyCaseIdsInAlertSchema(2); - }); + it('should not add more than 10 cases to an alert', async () => { + await bulkCreateAlertsAndVerifyCaseIdsInAlertSchema(10); - it('should not add more than 10 cases to an alert', async () => { - const updatedAlert = await bulkCreateAlertsAndVerifyCaseIdsInAlertSchema(10); - const alert = updatedAlert.hits.hits[0]; + const postedCase = await createCase(supertest, { + ...postCaseReq, + settings: { syncAlerts: false }, + }); - const postedCase = await createCase(supertest, { - ...postCaseReq, - settings: { syncAlerts: false }, + await createComment({ + supertest, + caseId: postedCase.id, + params: { + alertId, + index: ampIndex, + rule: { + id: 'id', + name: 'name', + }, + owner: 'observabilityFixture', + type: CommentType.alert, + }, + expectedHttpCode: 400, + }); }); - - createCommentAndRefreshIndex(postedCase.id, alert._id, alert._index, 400); }); }); diff --git a/x-pack/test/cases_api_integration/security_and_spaces/tests/common/internal/bulk_create_attachments.ts b/x-pack/test/cases_api_integration/security_and_spaces/tests/common/internal/bulk_create_attachments.ts index 4e3f88d042a2b..01d1d806b61f4 100644 --- a/x-pack/test/cases_api_integration/security_and_spaces/tests/common/internal/bulk_create_attachments.ts +++ b/x-pack/test/cases_api_integration/security_and_spaces/tests/common/internal/bulk_create_attachments.ts @@ -34,8 +34,6 @@ import { createCaseAndBulkCreateAttachments, bulkCreateAttachments, updateCase, - createSecuritySolutionAlerts, - getSecuritySolutionAlerts, } from '../../../../common/lib/utils'; import { createSignalsIndex, @@ -52,6 +50,11 @@ import { secOnlyRead, superUser, } from '../../../../common/lib/authentication/users'; +import { + getSecuritySolutionAlerts, + createSecuritySolutionAlerts, + getAlertById, +} from '../../../../common/lib/alerts'; // eslint-disable-next-line import/no-default-export export default ({ getService }: FtrProviderContext): void => { @@ -463,94 +466,148 @@ export default ({ getService }: FtrProviderContext): void => { }); describe('alerts', () => { - beforeEach(async () => { - await esArchiver.load('x-pack/test/functional/es_archives/auditbeat/hosts'); - await createSignalsIndex(supertest, log); - }); - - afterEach(async () => { - await deleteSignalsIndex(supertest, log); - await deleteAllAlerts(supertest, log); - await esArchiver.unload('x-pack/test/functional/es_archives/auditbeat/hosts'); - }); + describe('security_solution', () => { + beforeEach(async () => { + await esArchiver.load('x-pack/test/functional/es_archives/auditbeat/hosts'); + await createSignalsIndex(supertest, log); + }); - const bulkCreateAlertsAndVerifyAlertStatus = async ( - syncAlerts: boolean, - expectedAlertStatus: string - ) => { - const postedCase = await createCase(supertest, { - ...postCaseReq, - settings: { syncAlerts }, + afterEach(async () => { + await deleteSignalsIndex(supertest, log); + await deleteAllAlerts(supertest, log); + await esArchiver.unload('x-pack/test/functional/es_archives/auditbeat/hosts'); }); - await updateCase({ - supertest, - params: { - cases: [ - { - id: postedCase.id, - version: postedCase.version, - status: CaseStatuses['in-progress'], + const bulkCreateAlertsAndVerifyAlertStatus = async ( + syncAlerts: boolean, + expectedAlertStatus: string + ) => { + const postedCase = await createCase(supertest, { + ...postCaseReq, + settings: { syncAlerts }, + }); + + await updateCase({ + supertest, + params: { + cases: [ + { + id: postedCase.id, + version: postedCase.version, + status: CaseStatuses['in-progress'], + }, + ], + }, + }); + + const signals = await createSecuritySolutionAlerts(supertest, log); + + const attachments: CommentRequest[] = []; + const indices: string[] = []; + const ids: string[] = []; + + signals.hits.hits.forEach((alert) => { + expect(alert._source?.[ALERT_WORKFLOW_STATUS]).eql('open'); + attachments.push({ + alertId: alert._id, + index: alert._index, + rule: { + id: 'id', + name: 'name', }, - ], - }, - }); + owner: 'securitySolutionFixture', + type: CommentType.alert, + }); - const signals = await createSecuritySolutionAlerts(supertest, log); + indices.push(alert._index); + ids.push(alert._id); + }); - const attachments: CommentRequest[] = []; - const indices: string[] = []; - const ids: string[] = []; + await bulkCreateAttachments({ + supertest, + caseId: postedCase.id, + params: attachments, + }); - signals.hits.hits.forEach((alert) => { - expect(alert._source?.[ALERT_WORKFLOW_STATUS]).eql('open'); - attachments.push({ - alertId: alert._id, - index: alert._index, - rule: { - id: 'id', - name: 'name', - }, - owner: 'securitySolutionFixture', - type: CommentType.alert, + await es.indices.refresh({ index: indices }); + + const updatedAlerts = await getSecuritySolutionAlerts(supertest, ids); + + updatedAlerts.hits.hits.forEach((alert) => { + expect(alert._source?.[ALERT_WORKFLOW_STATUS]).eql(expectedAlertStatus); }); + }; - indices.push(alert._index); - ids.push(alert._id); - }); + const bulkCreateAlertsAndVerifyCaseIdsInAlertSchema = async (totalCases: number) => { + const cases = await Promise.all( + [...Array(totalCases).keys()].map((index) => + createCase(supertest, { + ...postCaseReq, + settings: { syncAlerts: false }, + }) + ) + ); - await bulkCreateAttachments({ - supertest, - caseId: postedCase.id, - params: attachments, + const signals = await createSecuritySolutionAlerts(supertest, log); + const alert = signals.hits.hits[0]; + + for (const theCase of cases) { + await bulkCreateAttachments({ + supertest, + caseId: theCase.id, + params: [ + { + alertId: alert._id, + index: alert._index, + rule: { + id: 'id', + name: 'name', + }, + owner: 'securitySolutionFixture', + type: CommentType.alert, + }, + ], + }); + } + + await es.indices.refresh({ index: alert._index }); + + const updatedAlert = await getSecuritySolutionAlerts(supertest, [alert._id]); + const caseIds = cases.map((theCase) => theCase.id); + + expect(updatedAlert.hits.hits[0]._source?.[ALERT_CASE_IDS]).eql(caseIds); + + return updatedAlert; + }; + + it('should change the status of the alerts if sync alert is on', async () => { + await bulkCreateAlertsAndVerifyAlertStatus(true, 'acknowledged'); }); - await es.indices.refresh({ index: indices }); + it('should NOT change the status of the alert if sync alert is off', async () => { + await bulkCreateAlertsAndVerifyAlertStatus(false, 'open'); + }); - const updatedAlerts = await getSecuritySolutionAlerts(supertest, ids); + it('should add the case ID to the alert schema', async () => { + await bulkCreateAlertsAndVerifyCaseIdsInAlertSchema(1); + }); - updatedAlerts.hits.hits.forEach((alert) => { - expect(alert._source?.[ALERT_WORKFLOW_STATUS]).eql(expectedAlertStatus); + it('should add multiple case ids to the alert schema', async () => { + await bulkCreateAlertsAndVerifyCaseIdsInAlertSchema(2); }); - }; - const bulkCreateAlertsAndVerifyCaseIdsInAlertSchema = async (totalCases: number) => { - const cases = await Promise.all( - [...Array(totalCases).keys()].map((index) => - createCase(supertest, { - ...postCaseReq, - settings: { syncAlerts: false }, - }) - ) - ); + it('should not add more than 10 cases to an alert', async () => { + const updatedAlert = await bulkCreateAlertsAndVerifyCaseIdsInAlertSchema(10); + const alert = updatedAlert.hits.hits[0]; - const signals = await createSecuritySolutionAlerts(supertest, log); - const alert = signals.hits.hits[0]; + const postedCase = await createCase(supertest, { + ...postCaseReq, + settings: { syncAlerts: false }, + }); - for (const theCase of cases) { await bulkCreateAttachments({ supertest, - caseId: theCase.id, + caseId: postedCase.id, params: [ { alertId: alert._id, @@ -563,60 +620,100 @@ export default ({ getService }: FtrProviderContext): void => { type: CommentType.alert, }, ], + expectedHttpCode: 400, }); - } + }); + }); - await es.indices.refresh({ index: alert._index }); + describe('observability', () => { + const alertId = 'NoxgpHkBqbdrfX07MqXV'; + const ampIndex = '.alerts-observability.apm.alerts'; - const updatedAlert = await getSecuritySolutionAlerts(supertest, [alert._id]); - const caseIds = cases.map((theCase) => theCase.id); + beforeEach(async () => { + await esArchiver.load('x-pack/test/functional/es_archives/rule_registry/alerts'); + }); - expect(updatedAlert.hits.hits[0]._source?.[ALERT_CASE_IDS]).eql(caseIds); + afterEach(async () => { + await esArchiver.unload('x-pack/test/functional/es_archives/rule_registry/alerts'); + }); - return updatedAlert; - }; + const bulkCreateAlertsAndVerifyCaseIdsInAlertSchema = async (totalCases: number) => { + const cases = await Promise.all( + [...Array(totalCases).keys()].map((index) => + createCase(supertest, { + ...postCaseReq, + owner: 'observabilityFixture', + settings: { syncAlerts: false }, + }) + ) + ); - it('should change the status of the alerts if sync alert is on', async () => { - await bulkCreateAlertsAndVerifyAlertStatus(true, 'acknowledged'); - }); + for (const theCase of cases) { + await bulkCreateAttachments({ + supertest, + caseId: theCase.id, + params: [ + { + alertId, + index: ampIndex, + rule: { + id: 'id', + name: 'name', + }, + owner: 'observabilityFixture', + type: CommentType.alert, + }, + ], + }); + } - it('should NOT change the status of the alert if sync alert is off', async () => { - await bulkCreateAlertsAndVerifyAlertStatus(false, 'open'); - }); + const alert = await getAlertById({ + supertest, + id: alertId, + index: ampIndex, + auth: { user: superUser, space: 'space1' }, + }); - it('should add the case ID to the alert schema', async () => { - await bulkCreateAlertsAndVerifyCaseIdsInAlertSchema(1); - }); + const caseIds = cases.map((theCase) => theCase.id); - it('should add multiple case ids to the alert schema', async () => { - await bulkCreateAlertsAndVerifyCaseIdsInAlertSchema(2); - }); + expect(alert['kibana.alert.case_ids']).eql(caseIds); - it('should not add more than 10 cases to an alert', async () => { - const updatedAlert = await bulkCreateAlertsAndVerifyCaseIdsInAlertSchema(10); - const alert = updatedAlert.hits.hits[0]; + return alert; + }; - const postedCase = await createCase(supertest, { - ...postCaseReq, - settings: { syncAlerts: false }, + it('should add the case ID to the alert schema', async () => { + await bulkCreateAlertsAndVerifyCaseIdsInAlertSchema(1); }); - await bulkCreateAttachments({ - supertest, - caseId: postedCase.id, - params: [ - { - alertId: alert._id, - index: alert._index, - rule: { - id: 'id', - name: 'name', + it('should add multiple case ids to the alert schema', async () => { + await bulkCreateAlertsAndVerifyCaseIdsInAlertSchema(2); + }); + + it('should not add more than 10 cases to an alert', async () => { + await bulkCreateAlertsAndVerifyCaseIdsInAlertSchema(10); + + const postedCase = await createCase(supertest, { + ...postCaseReq, + settings: { syncAlerts: false }, + }); + + await bulkCreateAttachments({ + supertest, + caseId: postedCase.id, + params: [ + { + alertId, + index: ampIndex, + rule: { + id: 'id', + name: 'name', + }, + owner: 'securitySolutionFixture', + type: CommentType.alert, }, - owner: 'securitySolutionFixture', - type: CommentType.alert, - }, - ], - expectedHttpCode: 400, + ], + expectedHttpCode: 400, + }); }); }); }); From 3ffe51996449c3aab4e90cd2b028c6475e291a82 Mon Sep 17 00:00:00 2001 From: Christos Nasikas Date: Wed, 11 Jan 2023 19:52:21 +0200 Subject: [PATCH 08/20] Add rbac tests --- .../common/lib/authentication/roles.ts | 39 ++++ .../common/lib/authentication/users.ts | 16 ++ .../tests/common/comments/post_comment.ts | 158 +++++++++++++-- .../internal/bulk_create_attachments.ts | 186 +++++++++++++++--- 4 files changed, 355 insertions(+), 44 deletions(-) diff --git a/x-pack/test/cases_api_integration/common/lib/authentication/roles.ts b/x-pack/test/cases_api_integration/common/lib/authentication/roles.ts index 3c63800c599c9..46c4fe58efadd 100644 --- a/x-pack/test/cases_api_integration/common/lib/authentication/roles.ts +++ b/x-pack/test/cases_api_integration/common/lib/authentication/roles.ts @@ -190,6 +190,24 @@ export const securitySolutionOnlyRead: Role = { }, }; +export const securitySolutionOnlyReadAlerts: Role = { + name: 'sec_only_read_alerts', + privileges: { + elasticsearch: { + indices: [], + }, + kibana: [ + { + feature: { + securitySolutionFixture: ['all'], + siem: ['read'], + }, + spaces: ['space1'], + }, + ], + }, +}; + export const observabilityOnlyAll: Role = { name: 'obs_only_all', privileges: { @@ -238,6 +256,25 @@ export const observabilityOnlyRead: Role = { }, }; +export const observabilityOnlyReadAlerts: Role = { + name: 'obs_only_read_alerts', + privileges: { + elasticsearch: { + indices: [], + }, + kibana: [ + { + feature: { + observabilityFixture: ['all'], + apm: ['read'], + logs: ['read'], + }, + spaces: ['space1'], + }, + ], + }, +}; + /** * These roles have access to all spaces. */ @@ -272,9 +309,11 @@ export const roles = [ globalRead, securitySolutionOnlyAll, securitySolutionOnlyRead, + securitySolutionOnlyReadAlerts, securitySolutionOnlyDelete, securitySolutionOnlyNoDelete, observabilityOnlyAll, observabilityOnlyRead, + observabilityOnlyReadAlerts, testDisabledPluginAll, ]; diff --git a/x-pack/test/cases_api_integration/common/lib/authentication/users.ts b/x-pack/test/cases_api_integration/common/lib/authentication/users.ts index 09f78cfb938be..48e060c926f48 100644 --- a/x-pack/test/cases_api_integration/common/lib/authentication/users.ts +++ b/x-pack/test/cases_api_integration/common/lib/authentication/users.ts @@ -17,6 +17,8 @@ import { testDisabledPluginAll, securitySolutionOnlyDelete, securitySolutionOnlyNoDelete, + observabilityOnlyReadAlerts, + securitySolutionOnlyReadAlerts, } from './roles'; import { User } from './types'; @@ -56,6 +58,12 @@ export const secOnlyRead: User = { roles: [securitySolutionOnlyRead.name], }; +export const secOnlyReadAlerts: User = { + username: 'sec_only_read_alerts', + password: 'sec_only_read_alerts', + roles: [securitySolutionOnlyReadAlerts.name], +}; + export const obsOnly: User = { username: 'obs_only', password: 'obs_only', @@ -68,6 +76,12 @@ export const obsOnlyRead: User = { roles: [observabilityOnlyRead.name], }; +export const obsOnlyReadAlerts: User = { + username: 'obs_only_read_alerts', + password: 'obs_only_read_alerts', + roles: [observabilityOnlyReadAlerts.name], +}; + export const obsSec: User = { username: 'obs_sec', password: 'obs_sec', @@ -112,10 +126,12 @@ export const users = [ superUser, secOnly, secOnlyRead, + secOnlyReadAlerts, secOnlyDelete, secOnlyNoDelete, obsOnly, obsOnlyRead, + obsOnlyReadAlerts, obsSec, obsSecRead, globalRead, diff --git a/x-pack/test/cases_api_integration/security_and_spaces/tests/common/comments/post_comment.ts b/x-pack/test/cases_api_integration/security_and_spaces/tests/common/comments/post_comment.ts index 82b059417a5cb..f50a27c9a48a2 100644 --- a/x-pack/test/cases_api_integration/security_and_spaces/tests/common/comments/post_comment.ts +++ b/x-pack/test/cases_api_integration/security_and_spaces/tests/common/comments/post_comment.ts @@ -50,16 +50,21 @@ import { secOnly, secOnlyRead, superUser, + obsOnlyReadAlerts, + obsSec, + secOnlyReadAlerts, } from '../../../../common/lib/authentication/users'; import { getSecuritySolutionAlerts, createSecuritySolutionAlerts, getAlertById, } from '../../../../common/lib/alerts'; +import { User } from '../../../../common/lib/authentication/types'; // eslint-disable-next-line import/no-default-export export default ({ getService }: FtrProviderContext): void => { const supertest = getService('supertest'); + const supertestWithoutAuth = getService('supertestWithoutAuth'); const esArchiver = getService('esArchiver'); const es = getService('es'); const log = getService('log'); @@ -361,14 +366,21 @@ export default ({ getService }: FtrProviderContext): void => { await esArchiver.unload('x-pack/test/functional/es_archives/auditbeat/hosts'); }); - const createCommentAndRefreshIndex = async ( - caseId: string, - alertId: string, - alertIndex: string, - expectedHttpCode?: number - ) => { + const createCommentAndRefreshIndex = async ({ + caseId, + alertId, + alertIndex, + expectedHttpCode = 200, + auth = { user: superUser, space: null }, + }: { + caseId: string; + alertId: string; + alertIndex: string; + expectedHttpCode?: number; + auth?: { user: User; space: string | null }; + }) => { await createComment({ - supertest, + supertest: supertestWithoutAuth, caseId, params: { alertId, @@ -381,6 +393,7 @@ export default ({ getService }: FtrProviderContext): void => { type: CommentType.alert, }, expectedHttpCode, + auth, }); await es.indices.refresh({ index: alertIndex }); @@ -413,7 +426,11 @@ export default ({ getService }: FtrProviderContext): void => { const alert = signals.hits.hits[0]; expect(alert._source?.[ALERT_WORKFLOW_STATUS]).eql('open'); - await createCommentAndRefreshIndex(postedCase.id, alert._id, alert._index); + await createCommentAndRefreshIndex({ + caseId: postedCase.id, + alertId: alert._id, + alertIndex: alert._index, + }); const updatedAlert = await getSecuritySolutionAlerts(supertest, [alert._id]); @@ -436,7 +453,11 @@ export default ({ getService }: FtrProviderContext): void => { const alert = signals.hits.hits[0]; for (const theCase of cases) { - await createCommentAndRefreshIndex(theCase.id, alert._id, alert._index); + await createCommentAndRefreshIndex({ + caseId: theCase.id, + alertId: alert._id, + alertIndex: alert._index, + }); } const updatedAlert = await getSecuritySolutionAlerts(supertest, [alert._id]); @@ -444,7 +465,7 @@ export default ({ getService }: FtrProviderContext): void => { expect(updatedAlert.hits.hits[0]._source?.[ALERT_CASE_IDS]).eql(caseIds); - return updatedAlert; + return { updatedAlert, cases }; }; it('should change the status of the alert if sync alert is on', async () => { @@ -464,7 +485,7 @@ export default ({ getService }: FtrProviderContext): void => { }); it('should not add more than 10 cases to an alert', async () => { - const updatedAlert = await bulkCreateAlertsAndVerifyCaseIdsInAlertSchema(10); + const { updatedAlert } = await bulkCreateAlertsAndVerifyCaseIdsInAlertSchema(10); const alert = updatedAlert.hits.hits[0]; const postedCase = await createCase(supertest, { @@ -472,7 +493,58 @@ export default ({ getService }: FtrProviderContext): void => { settings: { syncAlerts: false }, }); - createCommentAndRefreshIndex(postedCase.id, alert._id, alert._index, 400); + await createCommentAndRefreshIndex({ + caseId: postedCase.id, + alertId: alert._id, + alertIndex: alert._index, + expectedHttpCode: 400, + }); + }); + + it('should add the case ID to the alert schema when the user has read access only', async () => { + const postedCase = await createCase( + supertest, + { + ...postCaseReq, + settings: { syncAlerts: false }, + }, + 200, + { user: superUser, space: 'space1' } + ); + + const signals = await createSecuritySolutionAlerts(supertest, log); + const alert = signals.hits.hits[0]; + + await createCommentAndRefreshIndex({ + caseId: postedCase.id, + alertId: alert._id, + alertIndex: alert._index, + expectedHttpCode: 200, + auth: { user: secOnlyReadAlerts, space: 'space1' }, + }); + }); + + it('should NOT add the case ID to the alert schema when the user does NOT have access to the alert', async () => { + const postedCase = await createCase( + supertest, + { + ...postCaseReq, + settings: { syncAlerts: false }, + }, + 200, + { user: superUser, space: 'space1' } + ); + + const signals = await createSecuritySolutionAlerts(supertest, log); + const alert = signals.hits.hits[0]; + + await createCommentAndRefreshIndex({ + caseId: postedCase.id, + alertId: alert._id, + alertIndex: alert._index, + expectedHttpCode: 403, + auth: { user: obsSec, space: 'space1' }, + }); }); }); @@ -562,6 +634,66 @@ export default ({ getService }: FtrProviderContext): void => { expectedHttpCode: 400, }); }); + + it('should add the case ID to the alert schema when the user has read access only', async () => { + const postedCase = await createCase( + supertest, + { + ...postCaseReq, + owner: 'observabilityFixture', + settings: { syncAlerts: false }, + }, + 200, + { user: superUser, space: 'space1' } + ); + + await createComment({ + supertest: supertestWithoutAuth, + caseId: postedCase.id, + params: { + alertId, + index: ampIndex, + rule: { + id: 'id', + name: 'name', + }, + owner: 'observabilityFixture', + type: CommentType.alert, + }, + auth: { user: obsOnlyReadAlerts, space: 'space1' }, + expectedHttpCode: 200, + }); + }); + + it('should NOT add the case ID to the alert schema when the user does NOT have access to the alert', async () => { + const postedCase = await createCase( + supertest, + { + ...postCaseReq, + owner: 'observabilityFixture', + settings: { syncAlerts: false }, + }, + 200, + { user: superUser, space: 'space1' } + ); + + await createComment({ + supertest: supertestWithoutAuth, + caseId: postedCase.id, + params: { + alertId, + index: ampIndex, + rule: { + id: 'id', + name: 'name', + }, + owner: 'observabilityFixture', + type: CommentType.alert, + }, + auth: { user: obsSec, space: 'space1' }, + expectedHttpCode: 403, + }); + }); }); }); @@ -605,8 +737,6 @@ export default ({ getService }: FtrProviderContext): void => { }); describe('rbac', () => { - const supertestWithoutAuth = getService('supertestWithoutAuth'); - afterEach(async () => { await deleteAllCaseItems(es); }); diff --git a/x-pack/test/cases_api_integration/security_and_spaces/tests/common/internal/bulk_create_attachments.ts b/x-pack/test/cases_api_integration/security_and_spaces/tests/common/internal/bulk_create_attachments.ts index 01d1d806b61f4..124950edc894d 100644 --- a/x-pack/test/cases_api_integration/security_and_spaces/tests/common/internal/bulk_create_attachments.ts +++ b/x-pack/test/cases_api_integration/security_and_spaces/tests/common/internal/bulk_create_attachments.ts @@ -45,9 +45,12 @@ import { noKibanaPrivileges, obsOnly, obsOnlyRead, + obsOnlyReadAlerts, + obsSec, obsSecRead, secOnly, secOnlyRead, + secOnlyReadAlerts, superUser, } from '../../../../common/lib/authentication/users'; import { @@ -55,10 +58,12 @@ import { createSecuritySolutionAlerts, getAlertById, } from '../../../../common/lib/alerts'; +import { User } from '../../../../common/lib/authentication/types'; // eslint-disable-next-line import/no-default-export export default ({ getService }: FtrProviderContext): void => { const supertest = getService('supertest'); + const supertestWithoutAuth = getService('supertestWithoutAuth'); const esArchiver = getService('esArchiver'); const es = getService('es'); const log = getService('log'); @@ -478,6 +483,41 @@ export default ({ getService }: FtrProviderContext): void => { await esArchiver.unload('x-pack/test/functional/es_archives/auditbeat/hosts'); }); + const bulkCreateCommentAndRefreshIndex = async ({ + caseId, + alertId, + alertIndex, + expectedHttpCode = 200, + auth = { user: superUser, space: null }, + }: { + caseId: string; + alertId: string; + alertIndex: string; + expectedHttpCode?: number; + auth?: { user: User; space: string | null }; + }) => { + await bulkCreateAttachments({ + supertest: supertestWithoutAuth, + caseId, + params: [ + { + alertId, + index: alertIndex, + rule: { + id: 'id', + name: 'name', + }, + owner: 'securitySolutionFixture', + type: CommentType.alert, + }, + ], + expectedHttpCode, + auth, + }); + + await es.indices.refresh({ index: alertIndex }); + }; + const bulkCreateAlertsAndVerifyAlertStatus = async ( syncAlerts: boolean, expectedAlertStatus: string @@ -552,21 +592,10 @@ export default ({ getService }: FtrProviderContext): void => { const alert = signals.hits.hits[0]; for (const theCase of cases) { - await bulkCreateAttachments({ - supertest, + await bulkCreateCommentAndRefreshIndex({ caseId: theCase.id, - params: [ - { - alertId: alert._id, - index: alert._index, - rule: { - id: 'id', - name: 'name', - }, - owner: 'securitySolutionFixture', - type: CommentType.alert, - }, - ], + alertId: alert._id, + alertIndex: alert._index, }); } @@ -605,24 +634,59 @@ export default ({ getService }: FtrProviderContext): void => { settings: { syncAlerts: false }, }); - await bulkCreateAttachments({ - supertest, + await bulkCreateCommentAndRefreshIndex({ caseId: postedCase.id, - params: [ - { - alertId: alert._id, - index: alert._index, - rule: { - id: 'id', - name: 'name', - }, - owner: 'securitySolutionFixture', - type: CommentType.alert, - }, - ], + alertId: alert._id, + alertIndex: alert._index, expectedHttpCode: 400, }); }); + + it('should add the case ID to the alert schema when the user has read access only', async () => { + const postedCase = await createCase( + supertest, + { + ...postCaseReq, + settings: { syncAlerts: false }, + }, + 200, + { user: superUser, space: 'space1' } + ); + + const signals = await createSecuritySolutionAlerts(supertest, log); + const alert = signals.hits.hits[0]; + + await bulkCreateCommentAndRefreshIndex({ + caseId: postedCase.id, + alertId: alert._id, + alertIndex: alert._index, + expectedHttpCode: 200, + auth: { user: secOnlyReadAlerts, space: 'space1' }, + }); + }); + + it('should NOT add the case ID to the alert schema when the user does NOT have access to the alert', async () => { + const postedCase = await createCase( + supertest, + { + ...postCaseReq, + settings: { syncAlerts: false }, + }, + 200, + { user: superUser, space: 'space1' } + ); + + const signals = await createSecuritySolutionAlerts(supertest, log); + const alert = signals.hits.hits[0]; + + await bulkCreateCommentAndRefreshIndex({ + caseId: postedCase.id, + alertId: alert._id, + alertIndex: alert._index, + expectedHttpCode: 403, + auth: { user: obsSec, space: 'space1' }, + }); + }); }); describe('observability', () => { @@ -715,6 +779,70 @@ export default ({ getService }: FtrProviderContext): void => { expectedHttpCode: 400, }); }); + + it('should add the case ID to the alert schema when the user has read access only', async () => { + const postedCase = await createCase( + supertest, + { + ...postCaseReq, + owner: 'observabilityFixture', + settings: { syncAlerts: false }, + }, + 200, + { user: superUser, space: 'space1' } + ); + + await bulkCreateAttachments({ + supertest: supertestWithoutAuth, + caseId: postedCase.id, + params: [ + { + alertId, + index: ampIndex, + rule: { + id: 'id', + name: 'name', + }, + owner: 'observabilityFixture', + type: CommentType.alert, + }, + ], + auth: { user: obsOnlyReadAlerts, space: 'space1' }, + expectedHttpCode: 200, + }); + }); + + it('should NOT add the case ID to the alert schema when the user does NOT have access to the alert', async () => { + const postedCase = await createCase( + supertest, + { + ...postCaseReq, + owner: 'observabilityFixture', + settings: { syncAlerts: false }, + }, + 200, + { user: superUser, space: 'space1' } + ); + + await bulkCreateAttachments({ + supertest: supertestWithoutAuth, + caseId: postedCase.id, + params: [ + { + alertId, + index: ampIndex, + rule: { + id: 'id', + name: 'name', + }, + owner: 'observabilityFixture', + type: CommentType.alert, + }, + ], + auth: { user: obsSec, space: 'space1' }, + expectedHttpCode: 403, + }); + }); }); }); @@ -763,8 +891,6 @@ export default ({ getService }: FtrProviderContext): void => { }); describe('rbac', () => { - const supertestWithoutAuth = getService('supertestWithoutAuth'); - afterEach(async () => { await deleteAllCaseItems(es); }); From f99aad065744a1b3989e392980cf24aedface1eb Mon Sep 17 00:00:00 2001 From: Christos Nasikas Date: Thu, 12 Jan 2023 11:37:31 +0200 Subject: [PATCH 09/20] Address TODOs --- packages/kbn-rule-data-utils/index.ts | 1 + .../src/alerts_as_data_cases.ts | 9 ++++++++ .../common/models/case_with_comments.ts | 3 +-- .../server/alert_data_client/alerts_client.ts | 21 +++++++++---------- 4 files changed, 21 insertions(+), 13 deletions(-) create mode 100644 packages/kbn-rule-data-utils/src/alerts_as_data_cases.ts diff --git a/packages/kbn-rule-data-utils/index.ts b/packages/kbn-rule-data-utils/index.ts index 897e5609a8347..0341fbbd6e004 100644 --- a/packages/kbn-rule-data-utils/index.ts +++ b/packages/kbn-rule-data-utils/index.ts @@ -10,4 +10,5 @@ export * from './src/technical_field_names'; export * from './src/alerts_as_data_rbac'; export * from './src/alerts_as_data_severity'; export * from './src/alerts_as_data_status'; +export * from './src/alerts_as_data_cases'; export * from './src/routes/stack_rule_paths'; diff --git a/packages/kbn-rule-data-utils/src/alerts_as_data_cases.ts b/packages/kbn-rule-data-utils/src/alerts_as_data_cases.ts new file mode 100644 index 0000000000000..df8241699f77d --- /dev/null +++ b/packages/kbn-rule-data-utils/src/alerts_as_data_cases.ts @@ -0,0 +1,9 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0 and the Server Side Public License, v 1; you may not use this file except + * in compliance with, at your election, the Elastic License 2.0 or the Server + * Side Public License, v 1. + */ + +export const MAX_CASES_PER_ALERT = 10; diff --git a/x-pack/plugins/cases/server/common/models/case_with_comments.ts b/x-pack/plugins/cases/server/common/models/case_with_comments.ts index 998935b7c473e..ec9ac2538272f 100644 --- a/x-pack/plugins/cases/server/common/models/case_with_comments.ts +++ b/x-pack/plugins/cases/server/common/models/case_with_comments.ts @@ -363,8 +363,7 @@ export class CaseCommentModel { ); } catch (error) { throw createCaseError({ - // TODO: better error message - message: `Failed to update alerts case schema: ${error}`, + message: `Failed to add case info to alerts for caseId ${this.caseInfo.id}: ${error}`, error, logger: this.params.logger, }); diff --git a/x-pack/plugins/rule_registry/server/alert_data_client/alerts_client.ts b/x-pack/plugins/rule_registry/server/alert_data_client/alerts_client.ts index 6914640ee5ae1..768468ec0460d 100644 --- a/x-pack/plugins/rule_registry/server/alert_data_client/alerts_client.ts +++ b/x-pack/plugins/rule_registry/server/alert_data_client/alerts_client.ts @@ -22,6 +22,7 @@ import { ALERT_END, ALERT_STATUS_ACTIVE, ALERT_CASE_IDS, + MAX_CASES_PER_ALERT, } from '@kbn/rule-data-utils'; import { @@ -127,9 +128,6 @@ interface SingleSearchAfterAndAudit { lastSortIds?: Array | undefined; } -// TODO: Maybe move it to Cases or to @kbn/rule-data-utils -const MAX_CASES_PER_ALERT = 10; - /** * Provides apis to interact with alerts as data * ensures the request is authorized to perform read / write actions @@ -786,14 +784,10 @@ export class AlertsClient { public async bulkUpdateCases({ ids, index, caseIds }: BulkUpdateCasesOptions) { /** - * First check in case the caseIds are more that the allowed limit. - * We do this check to avoid any mget calls or authorization checks - * - * The check below does not ensure that an alert may exceed the limit - * if we add the casedIds. We need to also throw in case - * alert.caseIds + caseIds > MAX_CASES_PER_ALERT. The validateTotalCasesPerAlert - * function ensures that. - * + * We do this check to avoid any mget calls or authorization checks. + * The check below does not ensure that an alert may exceed the limit. + * We need to also throw in case alert.caseIds + caseIds > MAX_CASES_PER_ALERT. + * The validateTotalCasesPerAlert function ensures that. */ if (caseIds.length > MAX_CASES_PER_ALERT) { throw Boom.badRequest(`You cannot attach more than ${MAX_CASES_PER_ALERT} cases to an alert`); @@ -802,6 +796,11 @@ export class AlertsClient { return this.mgetAlertsAuditOperate({ ids, indexName: index, + /** + * A user with read access to an alert and write access to a case should be able to link + * the case to the alert (update the alert's data to include the case ids). + * For that reason, the operation is a read operation. + */ operation: ReadOperations.Get, fieldToUpdate: (source) => this.getAlertCaseIdsFieldUpdate(source, caseIds), validate: (source) => this.validateTotalCasesPerAlert(source, caseIds), From 994be500ccf5fbb59f03b00897ad3ea47a8d5daa Mon Sep 17 00:00:00 2001 From: Christos Nasikas Date: Thu, 12 Jan 2023 13:26:52 +0200 Subject: [PATCH 10/20] Integration tests: check for uniqueness --- .../tests/common/comments/post_comment.ts | 60 +++++++++++++++-- .../internal/bulk_create_attachments.ts | 66 ++++++++++++++++--- 2 files changed, 110 insertions(+), 16 deletions(-) diff --git a/x-pack/test/cases_api_integration/security_and_spaces/tests/common/comments/post_comment.ts b/x-pack/test/cases_api_integration/security_and_spaces/tests/common/comments/post_comment.ts index f50a27c9a48a2..64659884cd33d 100644 --- a/x-pack/test/cases_api_integration/security_and_spaces/tests/common/comments/post_comment.ts +++ b/x-pack/test/cases_api_integration/security_and_spaces/tests/common/comments/post_comment.ts @@ -484,6 +484,23 @@ export default ({ getService }: FtrProviderContext): void => { await bulkCreateAlertsAndVerifyCaseIdsInAlertSchema(2); }); + it('should remove cases with the same ID from the case_ids alerts field', async () => { + const { updatedAlert, cases } = await bulkCreateAlertsAndVerifyCaseIdsInAlertSchema(1); + const postedCase = cases[0]; + const alert = updatedAlert.hits.hits[0]; + + await createCommentAndRefreshIndex({ + caseId: postedCase.id, + alertId: alert._id, + alertIndex: alert._index, + }); + + const updatedAlertSecondTime = await getSecuritySolutionAlerts(supertest, [alert._id]); + expect(updatedAlertSecondTime.hits.hits[0]._source?.[ALERT_CASE_IDS]).eql([ + postedCase.id, + ]); + }); + it('should not add more than 10 cases to an alert', async () => { const { updatedAlert } = await bulkCreateAlertsAndVerifyCaseIdsInAlertSchema(10); const alert = updatedAlert.hits.hits[0]; @@ -550,7 +567,7 @@ export default ({ getService }: FtrProviderContext): void => { describe('observability', () => { const alertId = 'NoxgpHkBqbdrfX07MqXV'; - const ampIndex = '.alerts-observability.apm.alerts'; + const apmIndex = '.alerts-observability.apm.alerts'; beforeEach(async () => { await esArchiver.load('x-pack/test/functional/es_archives/rule_registry/alerts'); @@ -577,7 +594,7 @@ export default ({ getService }: FtrProviderContext): void => { caseId: theCase.id, params: { alertId, - index: ampIndex, + index: apmIndex, rule: { id: 'id', name: 'name', @@ -591,7 +608,7 @@ export default ({ getService }: FtrProviderContext): void => { const alert = await getAlertById({ supertest, id: alertId, - index: ampIndex, + index: apmIndex, auth: { user: superUser, space: 'space1' }, }); @@ -599,7 +616,7 @@ export default ({ getService }: FtrProviderContext): void => { expect(alert['kibana.alert.case_ids']).eql(caseIds); - return alert; + return { alert, cases }; }; it('should add the case ID to the alert schema', async () => { @@ -610,6 +627,35 @@ export default ({ getService }: FtrProviderContext): void => { await bulkCreateAlertsAndVerifyCaseIdsInAlertSchema(2); }); + it('should remove cases with the same ID from the case_ids alerts field', async () => { + const { cases } = await bulkCreateAlertsAndVerifyCaseIdsInAlertSchema(1); + const postedCase = cases[0]; + + await createComment({ + supertest, + caseId: postedCase.id, + params: { + alertId, + index: apmIndex, + rule: { + id: 'id', + name: 'name', + }, + owner: 'observabilityFixture', + type: CommentType.alert, + }, + }); + + const alert = await getAlertById({ + supertest, + id: alertId, + index: apmIndex, + auth: { user: superUser, space: 'space1' }, + }); + + expect(alert['kibana.alert.case_ids']).eql([postedCase.id]); + }); + it('should not add more than 10 cases to an alert', async () => { await bulkCreateAlertsAndVerifyCaseIdsInAlertSchema(10); @@ -623,7 +669,7 @@ export default ({ getService }: FtrProviderContext): void => { caseId: postedCase.id, params: { alertId, - index: ampIndex, + index: apmIndex, rule: { id: 'id', name: 'name', @@ -652,7 +698,7 @@ export default ({ getService }: FtrProviderContext): void => { caseId: postedCase.id, params: { alertId, - index: ampIndex, + index: apmIndex, rule: { id: 'id', name: 'name', @@ -682,7 +728,7 @@ export default ({ getService }: FtrProviderContext): void => { caseId: postedCase.id, params: { alertId, - index: ampIndex, + index: apmIndex, rule: { id: 'id', name: 'name', diff --git a/x-pack/test/cases_api_integration/security_and_spaces/tests/common/internal/bulk_create_attachments.ts b/x-pack/test/cases_api_integration/security_and_spaces/tests/common/internal/bulk_create_attachments.ts index 124950edc894d..a4448d4d1aeb4 100644 --- a/x-pack/test/cases_api_integration/security_and_spaces/tests/common/internal/bulk_create_attachments.ts +++ b/x-pack/test/cases_api_integration/security_and_spaces/tests/common/internal/bulk_create_attachments.ts @@ -606,7 +606,7 @@ export default ({ getService }: FtrProviderContext): void => { expect(updatedAlert.hits.hits[0]._source?.[ALERT_CASE_IDS]).eql(caseIds); - return updatedAlert; + return { updatedAlert, cases }; }; it('should change the status of the alerts if sync alert is on', async () => { @@ -625,8 +625,25 @@ export default ({ getService }: FtrProviderContext): void => { await bulkCreateAlertsAndVerifyCaseIdsInAlertSchema(2); }); + it('should remove cases with the same ID from the case_ids alerts field', async () => { + const { updatedAlert, cases } = await bulkCreateAlertsAndVerifyCaseIdsInAlertSchema(1); + const postedCase = cases[0]; + const alert = updatedAlert.hits.hits[0]; + + await bulkCreateCommentAndRefreshIndex({ + caseId: postedCase.id, + alertId: alert._id, + alertIndex: alert._index, + }); + + const updatedAlertSecondTime = await getSecuritySolutionAlerts(supertest, [alert._id]); + expect(updatedAlertSecondTime.hits.hits[0]._source?.[ALERT_CASE_IDS]).eql([ + postedCase.id, + ]); + }); + it('should not add more than 10 cases to an alert', async () => { - const updatedAlert = await bulkCreateAlertsAndVerifyCaseIdsInAlertSchema(10); + const { updatedAlert } = await bulkCreateAlertsAndVerifyCaseIdsInAlertSchema(10); const alert = updatedAlert.hits.hits[0]; const postedCase = await createCase(supertest, { @@ -691,7 +708,7 @@ export default ({ getService }: FtrProviderContext): void => { describe('observability', () => { const alertId = 'NoxgpHkBqbdrfX07MqXV'; - const ampIndex = '.alerts-observability.apm.alerts'; + const apmIndex = '.alerts-observability.apm.alerts'; beforeEach(async () => { await esArchiver.load('x-pack/test/functional/es_archives/rule_registry/alerts'); @@ -719,7 +736,7 @@ export default ({ getService }: FtrProviderContext): void => { params: [ { alertId, - index: ampIndex, + index: apmIndex, rule: { id: 'id', name: 'name', @@ -734,7 +751,7 @@ export default ({ getService }: FtrProviderContext): void => { const alert = await getAlertById({ supertest, id: alertId, - index: ampIndex, + index: apmIndex, auth: { user: superUser, space: 'space1' }, }); @@ -742,7 +759,7 @@ export default ({ getService }: FtrProviderContext): void => { expect(alert['kibana.alert.case_ids']).eql(caseIds); - return alert; + return { alert, cases }; }; it('should add the case ID to the alert schema', async () => { @@ -753,6 +770,37 @@ export default ({ getService }: FtrProviderContext): void => { await bulkCreateAlertsAndVerifyCaseIdsInAlertSchema(2); }); + it('should remove cases with the same ID from the case_ids alerts field', async () => { + const { cases } = await bulkCreateAlertsAndVerifyCaseIdsInAlertSchema(1); + const postedCase = cases[0]; + + await bulkCreateAttachments({ + supertest, + caseId: postedCase.id, + params: [ + { + alertId, + index: apmIndex, + rule: { + id: 'id', + name: 'name', + }, + owner: 'observabilityFixture', + type: CommentType.alert, + }, + ], + }); + + const alert = await getAlertById({ + supertest, + id: alertId, + index: apmIndex, + auth: { user: superUser, space: 'space1' }, + }); + + expect(alert['kibana.alert.case_ids']).eql([postedCase.id]); + }); + it('should not add more than 10 cases to an alert', async () => { await bulkCreateAlertsAndVerifyCaseIdsInAlertSchema(10); @@ -767,7 +815,7 @@ export default ({ getService }: FtrProviderContext): void => { params: [ { alertId, - index: ampIndex, + index: apmIndex, rule: { id: 'id', name: 'name', @@ -798,7 +846,7 @@ export default ({ getService }: FtrProviderContext): void => { params: [ { alertId, - index: ampIndex, + index: apmIndex, rule: { id: 'id', name: 'name', @@ -830,7 +878,7 @@ export default ({ getService }: FtrProviderContext): void => { params: [ { alertId, - index: ampIndex, + index: apmIndex, rule: { id: 'id', name: 'name', From 023ea28a247fd8576e2860e723e21a3218422e37 Mon Sep 17 00:00:00 2001 From: Christos Nasikas Date: Fri, 13 Jan 2023 17:28:34 +0200 Subject: [PATCH 11/20] PR feedback --- .../server/alert_data_client/alerts_client.ts | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/x-pack/plugins/rule_registry/server/alert_data_client/alerts_client.ts b/x-pack/plugins/rule_registry/server/alert_data_client/alerts_client.ts index 768468ec0460d..07fb58a9b4fa4 100644 --- a/x-pack/plugins/rule_registry/server/alert_data_client/alerts_client.ts +++ b/x-pack/plugins/rule_registry/server/alert_data_client/alerts_client.ts @@ -782,7 +782,20 @@ export class AlertsClient { } } + /** + * This function updates the case ids of multiple alerts per index. + * It is supposed to be used only by Cases. + * Cases implements its own RBAC. By using this function directly + * Cases RBAC is bypassed. + * Plugins that want to attach alerts to a case should use the + * cases client that does all the necessary cases RBAC checks + * before updating the alert with the case ids. + */ public async bulkUpdateCases({ ids, index, caseIds }: BulkUpdateCasesOptions) { + if (ids.length === 0) { + throw Boom.badRequest('You need to define at least one alert to update case ids'); + } + /** * We do this check to avoid any mget calls or authorization checks. * The check below does not ensure that an alert may exceed the limit. From 00f880cfaa49bdeeae5dc3ddafd19963d50d7743 Mon Sep 17 00:00:00 2001 From: Christos Nasikas Date: Mon, 16 Jan 2023 17:09:44 +0200 Subject: [PATCH 12/20] Change schema to do one mget call --- .../common/models/case_with_comments.ts | 26 ++---------- .../server/alert_data_client/alerts_client.ts | 40 +++++++++---------- 2 files changed, 22 insertions(+), 44 deletions(-) diff --git a/x-pack/plugins/cases/server/common/models/case_with_comments.ts b/x-pack/plugins/cases/server/common/models/case_with_comments.ts index ec9ac2538272f..864844e833806 100644 --- a/x-pack/plugins/cases/server/common/models/case_with_comments.ts +++ b/x-pack/plugins/cases/server/common/models/case_with_comments.ts @@ -12,7 +12,6 @@ import type { SavedObjectsUpdateOptions, SavedObjectsUpdateResponse, } from '@kbn/core/server'; -import pMap from 'p-map'; import type { CaseResponse, CommentAttributes, @@ -31,7 +30,6 @@ import { import { CASE_SAVED_OBJECT, MAX_ALERTS_PER_CASE, - MAX_CONCURRENT_SEARCHES, MAX_DOCS_PER_PAGE, } from '../../../common/constants'; import type { CasesClientArgs } from '../../client'; @@ -341,26 +339,10 @@ export class CaseCommentModel { private async updateAlertsSchemaWithCaseInfo(alertAttachments: CommentRequestAlertType[]) { try { const alerts = getAlertInfoFromComments(alertAttachments); - const alertsGroupedByIndex = new Map>(); - - for (const alert of alerts) { - const idsSet = alertsGroupedByIndex.get(alert.index) ?? new Set(); - idsSet.add(alert.id); - alertsGroupedByIndex.set(alert.index, idsSet); - } - - await pMap( - alertsGroupedByIndex.entries(), - async ([index, idsSet]) => - this.params.alertsClient.bulkUpdateCases({ - ids: Array.from(idsSet.values()), - index, - caseIds: [this.caseInfo.id], - }), - { - concurrency: MAX_CONCURRENT_SEARCHES, - } - ); + this.params.alertsClient.bulkUpdateCases({ + alerts, + caseIds: [this.caseInfo.id], + }); } catch (error) { throw createCaseError({ message: `Failed to add case info to alerts for caseId ${this.caseInfo.id}: ${error}`, diff --git a/x-pack/plugins/rule_registry/server/alert_data_client/alerts_client.ts b/x-pack/plugins/rule_registry/server/alert_data_client/alerts_client.ts index 07fb58a9b4fa4..35e1a3e1195b5 100644 --- a/x-pack/plugins/rule_registry/server/alert_data_client/alerts_client.ts +++ b/x-pack/plugins/rule_registry/server/alert_data_client/alerts_client.ts @@ -95,10 +95,14 @@ export interface BulkUpdateOptions { query: object | string | undefined | null; } +interface MgetAndAuditAlert { + id: string; + index: string; +} + export interface BulkUpdateCasesOptions { - ids: string[]; + alerts: MgetAndAuditAlert[]; caseIds: string[]; - index: string; } interface GetAlertParams { @@ -344,27 +348,23 @@ export class AlertsClient { * @returns */ private async mgetAlertsAuditOperate({ - ids, - indexName, + alerts, operation, fieldToUpdate, validate, }: { - ids: string[]; - indexName: string; + alerts: MgetAndAuditAlert[]; operation: ReadOperations.Find | ReadOperations.Get | WriteOperations.Update; fieldToUpdate: (source: ParsedTechnicalFields | undefined) => Record; validate?: (source: ParsedTechnicalFields | undefined) => void; }) { try { const mgetRes = await this.esClient.mget({ - index: indexName, - body: { - ids, - }, + docs: alerts.map(({ id, index }) => ({ _id: id, _index: index })), }); await this.ensureAllAuthorized(mgetRes.docs, operation); + const ids = mgetRes.docs.map(({ _id }) => _id); for (const id of ids) { this.auditLogger?.log( @@ -419,19 +419,16 @@ export class AlertsClient { * @returns */ private async mgetAlertsAuditOperateStatus({ - ids, + alerts, status, - indexName, operation, }: { - ids: string[]; + alerts: MgetAndAuditAlert[]; status: STATUS_VALUES; - indexName: string; operation: ReadOperations.Find | ReadOperations.Get | WriteOperations.Update; }) { return this.mgetAlertsAuditOperate({ - ids, - indexName, + alerts, operation, fieldToUpdate: (source) => this.getAlertStatusFieldUpdate(source, status), }); @@ -732,10 +729,10 @@ export class AlertsClient { }: BulkUpdateOptions) { // rejects at the route level if more than 1000 id's are passed in if (ids != null) { + const alerts = ids.map((id) => ({ id, index })); return this.mgetAlertsAuditOperateStatus({ - ids, + alerts, status, - indexName: index, operation: WriteOperations.Update, }); } else if (query != null) { @@ -791,8 +788,8 @@ export class AlertsClient { * cases client that does all the necessary cases RBAC checks * before updating the alert with the case ids. */ - public async bulkUpdateCases({ ids, index, caseIds }: BulkUpdateCasesOptions) { - if (ids.length === 0) { + public async bulkUpdateCases({ alerts, caseIds }: BulkUpdateCasesOptions) { + if (alerts.length === 0) { throw Boom.badRequest('You need to define at least one alert to update case ids'); } @@ -807,8 +804,7 @@ export class AlertsClient { } return this.mgetAlertsAuditOperate({ - ids, - indexName: index, + alerts, /** * A user with read access to an alert and write access to a case should be able to link * the case to the alert (update the alert's data to include the case ids). From 82c69bcf0d57bf1dd5f5a35e580d491a8269f711 Mon Sep 17 00:00:00 2001 From: Christos Nasikas Date: Mon, 16 Jan 2023 17:46:02 +0200 Subject: [PATCH 13/20] Move updating case ids to the alerts service --- .../cases/server/client/alerts/get.test.ts | 4 +++- .../cases/server/client/alerts/types.ts | 5 +++++ x-pack/plugins/cases/server/client/factory.ts | 15 +++++++++---- x-pack/plugins/cases/server/client/mocks.ts | 2 -- x-pack/plugins/cases/server/client/types.ts | 3 +-- .../common/models/case_with_comments.ts | 19 +++++----------- .../server/services/alerts/index.test.ts | 6 +++-- .../cases/server/services/alerts/index.ts | 22 +++++++++++++++++-- x-pack/plugins/cases/server/services/mocks.ts | 1 + 9 files changed, 51 insertions(+), 26 deletions(-) diff --git a/x-pack/plugins/cases/server/client/alerts/get.test.ts b/x-pack/plugins/cases/server/client/alerts/get.test.ts index 8659a190ef7a9..0d525319e11f8 100644 --- a/x-pack/plugins/cases/server/client/alerts/get.test.ts +++ b/x-pack/plugins/cases/server/client/alerts/get.test.ts @@ -6,6 +6,7 @@ */ import { elasticsearchServiceMock, loggingSystemMock } from '@kbn/core/server/mocks'; +import { alertsClientMock } from '@kbn/rule-registry-plugin/server/alert_data_client/alerts_client.mock'; import { AlertService } from '../../services'; import type { CasesClientArgs } from '../types'; import { getAlerts } from './get'; @@ -13,10 +14,11 @@ import { getAlerts } from './get'; describe('getAlerts', () => { const esClient = elasticsearchServiceMock.createElasticsearchClient(); const logger = loggingSystemMock.create().get('case'); + const alertsClient = alertsClientMock.create(); let alertsService: AlertService; beforeEach(async () => { - alertsService = new AlertService(esClient, logger); + alertsService = new AlertService(esClient, logger, alertsClient); jest.clearAllMocks(); }); diff --git a/x-pack/plugins/cases/server/client/alerts/types.ts b/x-pack/plugins/cases/server/client/alerts/types.ts index 485a00a10ee79..d5e6c2e8252e9 100644 --- a/x-pack/plugins/cases/server/client/alerts/types.ts +++ b/x-pack/plugins/cases/server/client/alerts/types.ts @@ -37,3 +37,8 @@ export interface AlertUpdateStatus { export interface AlertGet { alertsInfo: AlertInfo[]; } + +export interface UpdateAlertCasesRequest { + alerts: Array<{ id: string; index: string }>; + caseIds: string[]; +} diff --git a/x-pack/plugins/cases/server/client/factory.ts b/x-pack/plugins/cases/server/client/factory.ts index b5dfedff36a8c..caf82e57d872d 100644 --- a/x-pack/plugins/cases/server/client/factory.ts +++ b/x-pack/plugins/cases/server/client/factory.ts @@ -25,8 +25,12 @@ import type { LensServerPluginSetup } from '@kbn/lens-plugin/server'; import type { SpacesPluginStart } from '@kbn/spaces-plugin/server'; import type { LicensingPluginStart } from '@kbn/licensing-plugin/server'; import type { NotificationsPluginStart } from '@kbn/notifications-plugin/server'; -import type { RuleRegistryPluginStartContract } from '@kbn/rule-registry-plugin/server'; +import type { + AlertsClient, + RuleRegistryPluginStartContract, +} from '@kbn/rule-registry-plugin/server'; +import type { PublicMethodsOf } from '@kbn/utility-types'; import { SAVED_OBJECT_TYPES } from '../../common/constants'; import { Authorization } from '../authorization/authorization'; import { @@ -122,15 +126,17 @@ export class CasesClientFactory { excludedExtensions: [SECURITY_EXTENSION_ID], }); + const alertsClient = await this.options.ruleRegistry.getRacClientWithRequest(request); + const services = this.createServices({ unsecuredSavedObjectsClient, esClient: scopedClusterClient, request, auditLogger, + alertsClient, }); const userInfo = await this.getUserInfo(request); - const alertsClient = await this.options.ruleRegistry.getRacClientWithRequest(request); return createCasesClient({ services, @@ -145,7 +151,6 @@ export class CasesClientFactory { securityStartPlugin: this.options.securityPluginStart, publicBaseUrl: this.options.publicBaseUrl, spaceId: this.options.spacesPluginStart.spacesService.getSpaceId(request), - alertsClient, }); } @@ -160,11 +165,13 @@ export class CasesClientFactory { esClient, request, auditLogger, + alertsClient, }: { unsecuredSavedObjectsClient: SavedObjectsClientContract; esClient: ElasticsearchClient; request: KibanaRequest; auditLogger: AuditLogger; + alertsClient: PublicMethodsOf; }): CasesServices { this.validateInitialization(); @@ -198,7 +205,7 @@ export class CasesClientFactory { }); return { - alertsService: new AlertService(esClient, this.logger), + alertsService: new AlertService(esClient, this.logger, alertsClient), caseService, caseConfigureService: new CaseConfigureService(this.logger), connectorMappingsService: new ConnectorMappingsService(this.logger), diff --git a/x-pack/plugins/cases/server/client/mocks.ts b/x-pack/plugins/cases/server/client/mocks.ts index 5a9a6681cfb4f..3281cd7764240 100644 --- a/x-pack/plugins/cases/server/client/mocks.ts +++ b/x-pack/plugins/cases/server/client/mocks.ts @@ -10,7 +10,6 @@ import { loggingSystemMock, savedObjectsClientMock } from '@kbn/core/server/mock import { securityMock } from '@kbn/security-plugin/server/mocks'; import { actionsClientMock } from '@kbn/actions-plugin/server/actions_client.mock'; -import { alertsClientMock } from '@kbn/rule-registry-plugin/server/alert_data_client/alerts_client.mock'; import { makeLensEmbeddableFactory } from '@kbn/lens-plugin/server/embeddable/make_lens_embeddable_factory'; import type { CasesClient } from '.'; import { createAuthorizationMock } from '../authorization/mock'; @@ -142,7 +141,6 @@ export const createCasesClientMockArgs = () => { logger: loggingSystemMock.createLogger(), unsecuredSavedObjectsClient: savedObjectsClientMock.create(), actionsClient: actionsClientMock.create(), - alertsClient: alertsClientMock.create(), user: { username: 'damaged_raccoon', email: 'damaged_raccoon@elastic.co', diff --git a/x-pack/plugins/cases/server/client/types.ts b/x-pack/plugins/cases/server/client/types.ts index 10ac975c1cb70..3376fe9e99ecc 100644 --- a/x-pack/plugins/cases/server/client/types.ts +++ b/x-pack/plugins/cases/server/client/types.ts @@ -12,7 +12,6 @@ import type { LensServerPluginSetup } from '@kbn/lens-plugin/server'; import type { SecurityPluginStart } from '@kbn/security-plugin/server'; import type { IBasePath } from '@kbn/core-http-browser'; import type { KueryNode } from '@kbn/es-query'; -import type { AlertsClient } from '@kbn/rule-registry-plugin/server'; import type { CasesFindRequest, User } from '../../common/api'; import type { Authorization } from '../authorization/authorization'; import type { @@ -54,7 +53,7 @@ export interface CasesClientArgs { readonly externalReferenceAttachmentTypeRegistry: ExternalReferenceAttachmentTypeRegistry; readonly securityStartPlugin: SecurityPluginStart; readonly spaceId: string; - readonly alertsClient: PublicMethodsOf; + readonly publicBaseUrl?: IBasePath['publicBaseUrl']; } diff --git a/x-pack/plugins/cases/server/common/models/case_with_comments.ts b/x-pack/plugins/cases/server/common/models/case_with_comments.ts index 864844e833806..d2ac8fc45f5bc 100644 --- a/x-pack/plugins/cases/server/common/models/case_with_comments.ts +++ b/x-pack/plugins/cases/server/common/models/case_with_comments.ts @@ -337,19 +337,12 @@ export class CaseCommentModel { } private async updateAlertsSchemaWithCaseInfo(alertAttachments: CommentRequestAlertType[]) { - try { - const alerts = getAlertInfoFromComments(alertAttachments); - this.params.alertsClient.bulkUpdateCases({ - alerts, - caseIds: [this.caseInfo.id], - }); - } catch (error) { - throw createCaseError({ - message: `Failed to add case info to alerts for caseId ${this.caseInfo.id}: ${error}`, - error, - logger: this.params.logger, - }); - } + const alerts = getAlertInfoFromComments(alertAttachments); + + await this.params.services.alertsService.bulkUpdateCases({ + alerts, + caseIds: [this.caseInfo.id], + }); } private async createCommentUserAction( diff --git a/x-pack/plugins/cases/server/services/alerts/index.test.ts b/x-pack/plugins/cases/server/services/alerts/index.test.ts index 24eef0a88fcad..41fd65d508a6b 100644 --- a/x-pack/plugins/cases/server/services/alerts/index.test.ts +++ b/x-pack/plugins/cases/server/services/alerts/index.test.ts @@ -5,17 +5,19 @@ * 2.0. */ +import { elasticsearchServiceMock, loggingSystemMock } from '@kbn/core/server/mocks'; +import { alertsClientMock } from '@kbn/rule-registry-plugin/server/alert_data_client/alerts_client.mock'; import { CaseStatuses } from '../../../common/api'; import { AlertService } from '.'; -import { elasticsearchServiceMock, loggingSystemMock } from '@kbn/core/server/mocks'; describe('updateAlertsStatus', () => { const esClient = elasticsearchServiceMock.createElasticsearchClient(); const logger = loggingSystemMock.create().get('case'); + const alertsClient = alertsClientMock.create(); let alertService: AlertService; beforeEach(async () => { - alertService = new AlertService(esClient, logger); + alertService = new AlertService(esClient, logger, alertsClient); jest.clearAllMocks(); }); diff --git a/x-pack/plugins/cases/server/services/alerts/index.ts b/x-pack/plugins/cases/server/services/alerts/index.ts index 0eed62a2c2869..cd999065d5539 100644 --- a/x-pack/plugins/cases/server/services/alerts/index.ts +++ b/x-pack/plugins/cases/server/services/alerts/index.ts @@ -12,17 +12,20 @@ import type { ElasticsearchClient, Logger } from '@kbn/core/server'; import type { STATUS_VALUES } from '@kbn/rule-registry-plugin/common/technical_rule_data_field_names'; import { ALERT_WORKFLOW_STATUS } from '@kbn/rule-registry-plugin/common/technical_rule_data_field_names'; import type { MgetResponse } from '@elastic/elasticsearch/lib/api/types'; +import type { AlertsClient } from '@kbn/rule-registry-plugin/server'; +import type { PublicMethodsOf } from '@kbn/utility-types'; import { CaseStatuses } from '../../../common/api'; import { MAX_ALERTS_PER_CASE, MAX_CONCURRENT_SEARCHES } from '../../../common/constants'; import { createCaseError } from '../../common/error'; import type { AlertInfo } from '../../common/types'; -import type { UpdateAlertStatusRequest } from '../../client/alerts/types'; +import type { UpdateAlertCasesRequest, UpdateAlertStatusRequest } from '../../client/alerts/types'; import type { AggregationBuilder, AggregationResponse } from '../../client/metrics/types'; export class AlertService { constructor( private readonly scopedClusterClient: ElasticsearchClient, - private readonly logger: Logger + private readonly logger: Logger, + private readonly alertsClient: PublicMethodsOf ) {} public async executeAggregations({ @@ -201,6 +204,21 @@ export class AlertService { }); } } + + public async bulkUpdateCases({ alerts, caseIds }: UpdateAlertCasesRequest): Promise { + try { + await this.alertsClient.bulkUpdateCases({ + alerts, + caseIds, + }); + } catch (error) { + throw createCaseError({ + message: `Failed to add case info to alerts for caseIds ${caseIds}: ${error}`, + error, + logger: this.logger, + }); + } + } } interface TranslatedUpdateAlertRequest { diff --git a/x-pack/plugins/cases/server/services/mocks.ts b/x-pack/plugins/cases/server/services/mocks.ts index 125fbc7ec9650..0865045bff5bf 100644 --- a/x-pack/plugins/cases/server/services/mocks.ts +++ b/x-pack/plugins/cases/server/services/mocks.ts @@ -95,6 +95,7 @@ export const createAlertServiceMock = (): AlertServiceMock => { updateAlertsStatus: jest.fn(), getAlerts: jest.fn(), executeAggregations: jest.fn(), + bulkUpdateCases: jest.fn(), }; // the cast here is required because jest.Mocked tries to include private members and would throw an error From 5f48d8daf31277767a75c32739b521a7690c2f4f Mon Sep 17 00:00:00 2001 From: Christos Nasikas Date: Mon, 16 Jan 2023 18:09:40 +0200 Subject: [PATCH 14/20] Filter out empty alets --- .../server/services/alerts/index.test.ts | 28 +++++++++++++++++++ .../cases/server/services/alerts/index.ts | 4 ++- 2 files changed, 31 insertions(+), 1 deletion(-) diff --git a/x-pack/plugins/cases/server/services/alerts/index.test.ts b/x-pack/plugins/cases/server/services/alerts/index.test.ts index 41fd65d508a6b..72fc675f47b5f 100644 --- a/x-pack/plugins/cases/server/services/alerts/index.test.ts +++ b/x-pack/plugins/cases/server/services/alerts/index.test.ts @@ -338,4 +338,32 @@ describe('updateAlertsStatus', () => { expect(res).toBe(undefined); }); }); + + describe('bulkUpdateCases', () => { + const alerts = [ + { + id: 'c3869d546717e8c581add9cbf7d24578f34cd3e72cbc8d8b8e9a9330a899f70f', + index: '.internal.alerts-security.alerts-default-000001', + }, + ]; + const caseIds = ['test-case']; + + it('update case info', async () => { + await alertService.bulkUpdateCases({ alerts, caseIds }); + + expect(alertsClient.bulkUpdateCases).toBeCalledWith({ alerts, caseIds }); + }); + + it('filters out alerts with empty id', async () => { + await alertService.bulkUpdateCases({ alerts: [{ id: '', index: 'test-index' }], caseIds }); + + expect(alertsClient.bulkUpdateCases).toBeCalledWith({ alerts: [], caseIds }); + }); + + it('filters out alerts with empty index', async () => { + await alertService.bulkUpdateCases({ alerts: [{ id: 'test-id', index: '' }], caseIds }); + + expect(alertsClient.bulkUpdateCases).toBeCalledWith({ alerts: [], caseIds }); + }); + }); }); diff --git a/x-pack/plugins/cases/server/services/alerts/index.ts b/x-pack/plugins/cases/server/services/alerts/index.ts index cd999065d5539..07cf18749e4b1 100644 --- a/x-pack/plugins/cases/server/services/alerts/index.ts +++ b/x-pack/plugins/cases/server/services/alerts/index.ts @@ -207,8 +207,10 @@ export class AlertService { public async bulkUpdateCases({ alerts, caseIds }: UpdateAlertCasesRequest): Promise { try { + const nonEmptyAlerts = alerts.filter((alert) => !AlertService.isEmptyAlert(alert)); + await this.alertsClient.bulkUpdateCases({ - alerts, + alerts: nonEmptyAlerts, caseIds, }); } catch (error) { From bd6c48da507fba8ae67c286ee3f8f75be9b67687 Mon Sep 17 00:00:00 2001 From: Christos Nasikas Date: Tue, 17 Jan 2023 10:13:02 +0200 Subject: [PATCH 15/20] Do not call the alerts client if there are no alerts to update --- .../common/models/case_with_comments.ts | 4 +++- .../server/services/alerts/index.test.ts | 23 +++++++++++++++---- .../cases/server/services/alerts/index.ts | 4 ++++ 3 files changed, 26 insertions(+), 5 deletions(-) diff --git a/x-pack/plugins/cases/server/common/models/case_with_comments.ts b/x-pack/plugins/cases/server/common/models/case_with_comments.ts index d2ac8fc45f5bc..31e15031b3cf2 100644 --- a/x-pack/plugins/cases/server/common/models/case_with_comments.ts +++ b/x-pack/plugins/cases/server/common/models/case_with_comments.ts @@ -320,7 +320,9 @@ export class CaseCommentModel { await this.updateAlertsStatus(alertAttachments); } - await this.updateAlertsSchemaWithCaseInfo(alertAttachments); + if (alertAttachments.length > 0) { + await this.updateAlertsSchemaWithCaseInfo(alertAttachments); + } } private async updateAlertsStatus(alerts: CommentRequest[]) { diff --git a/x-pack/plugins/cases/server/services/alerts/index.test.ts b/x-pack/plugins/cases/server/services/alerts/index.test.ts index 72fc675f47b5f..6bc943ab3f0d5 100644 --- a/x-pack/plugins/cases/server/services/alerts/index.test.ts +++ b/x-pack/plugins/cases/server/services/alerts/index.test.ts @@ -355,15 +355,30 @@ describe('updateAlertsStatus', () => { }); it('filters out alerts with empty id', async () => { - await alertService.bulkUpdateCases({ alerts: [{ id: '', index: 'test-index' }], caseIds }); + await alertService.bulkUpdateCases({ + alerts: [{ id: '', index: 'test-index' }, ...alerts], + caseIds, + }); - expect(alertsClient.bulkUpdateCases).toBeCalledWith({ alerts: [], caseIds }); + expect(alertsClient.bulkUpdateCases).toBeCalledWith({ alerts, caseIds }); }); it('filters out alerts with empty index', async () => { - await alertService.bulkUpdateCases({ alerts: [{ id: 'test-id', index: '' }], caseIds }); + await alertService.bulkUpdateCases({ + alerts: [{ id: 'test-id', index: '' }, ...alerts], + caseIds, + }); + + expect(alertsClient.bulkUpdateCases).toBeCalledWith({ alerts, caseIds }); + }); + + it('does not call the alerts client with no alerts', async () => { + await alertService.bulkUpdateCases({ + alerts: [{ id: '', index: 'test-index' }], + caseIds, + }); - expect(alertsClient.bulkUpdateCases).toBeCalledWith({ alerts: [], caseIds }); + expect(alertsClient.bulkUpdateCases).not.toHaveBeenCalled(); }); }); }); diff --git a/x-pack/plugins/cases/server/services/alerts/index.ts b/x-pack/plugins/cases/server/services/alerts/index.ts index 07cf18749e4b1..ad089857e2846 100644 --- a/x-pack/plugins/cases/server/services/alerts/index.ts +++ b/x-pack/plugins/cases/server/services/alerts/index.ts @@ -209,6 +209,10 @@ export class AlertService { try { const nonEmptyAlerts = alerts.filter((alert) => !AlertService.isEmptyAlert(alert)); + if (nonEmptyAlerts.length <= 0) { + return; + } + await this.alertsClient.bulkUpdateCases({ alerts: nonEmptyAlerts, caseIds, From 841934538206936dcb1c0a728987b307d3ce85fa Mon Sep 17 00:00:00 2001 From: Christos Nasikas Date: Tue, 7 Feb 2023 17:25:42 +0200 Subject: [PATCH 16/20] Add ensureAllAlertsAuthorizedRead to the alerts client --- .../alert_data_client/alerts_client.mock.ts | 1 + .../server/alert_data_client/alerts_client.ts | 64 ++++++++++++++----- 2 files changed, 48 insertions(+), 17 deletions(-) diff --git a/x-pack/plugins/rule_registry/server/alert_data_client/alerts_client.mock.ts b/x-pack/plugins/rule_registry/server/alert_data_client/alerts_client.mock.ts index 5ea5fa0e8b357..74ea0225614b0 100644 --- a/x-pack/plugins/rule_registry/server/alert_data_client/alerts_client.mock.ts +++ b/x-pack/plugins/rule_registry/server/alert_data_client/alerts_client.mock.ts @@ -22,6 +22,7 @@ const createAlertsClientMock = () => { getFeatureIdsByRegistrationContexts: jest.fn(), getBrowserFields: jest.fn(), getAlertSummary: jest.fn(), + ensureAllAlertsAuthorizedRead: jest.fn(), }; return mocked; }; diff --git a/x-pack/plugins/rule_registry/server/alert_data_client/alerts_client.ts b/x-pack/plugins/rule_registry/server/alert_data_client/alerts_client.ts index 3071f08671830..198fcf75c0c71 100644 --- a/x-pack/plugins/rule_registry/server/alert_data_client/alerts_client.ts +++ b/x-pack/plugins/rule_registry/server/alert_data_client/alerts_client.ts @@ -359,22 +359,7 @@ export class AlertsClient { validate?: (source: ParsedTechnicalFields | undefined) => void; }) { try { - const mgetRes = await this.esClient.mget({ - docs: alerts.map(({ id, index }) => ({ _id: id, _index: index })), - }); - - await this.ensureAllAuthorized(mgetRes.docs, operation); - const ids = mgetRes.docs.map(({ _id }) => _id); - - for (const id of ids) { - this.auditLogger?.log( - alertAuditEvent({ - action: operationAlertAuditActionMap[operation], - id, - ...this.getOutcome(operation), - }) - ); - } + const mgetRes = await this.ensureAllAlertsAuthorized({ alerts, operation }); const updateRequests = []; @@ -545,6 +530,51 @@ export class AlertsClient { } } + /** + * Ensures that the user has access to the alerts + * for a given operation + */ + private async ensureAllAlertsAuthorized({ + alerts, + operation, + }: { + alerts: MgetAndAuditAlert[]; + operation: ReadOperations.Find | ReadOperations.Get | WriteOperations.Update; + }) { + try { + const mgetRes = await this.esClient.mget({ + docs: alerts.map(({ id, index }) => ({ _id: id, _index: index })), + }); + + await this.ensureAllAuthorized(mgetRes.docs, operation); + const ids = mgetRes.docs.map(({ _id }) => _id); + + for (const id of ids) { + this.auditLogger?.log( + alertAuditEvent({ + action: operationAlertAuditActionMap[operation], + id, + ...this.getOutcome(operation), + }) + ); + } + + return mgetRes; + } catch (exc) { + this.logger.error(`error in ensureAlertsAuthorized ${exc}`); + throw exc; + } + } + + public async ensureAllAlertsAuthorizedRead({ alerts }: { alerts: MgetAndAuditAlert[] }) { + try { + await this.ensureAllAlertsAuthorized({ alerts, operation: ReadOperations.Get }); + } catch (error) { + this.logger.error(`error authenticating alerts for read access: ${error}`); + throw error; + } + } + public async get({ id, index }: GetAlertParams) { try { // first search for the alert by id, then use the alert info to check if user has access to it @@ -935,7 +965,7 @@ export class AlertsClient { } } - async getBrowserFields({ + public async getBrowserFields({ indices, metaFields, allowNoIndex, From aafdf84411aef76abd56548e87b53f370690f1ac Mon Sep 17 00:00:00 2001 From: Christos Nasikas Date: Wed, 8 Feb 2023 12:25:38 +0200 Subject: [PATCH 17/20] Authorize alerts when attached to a case --- .../cases/server/client/alerts/types.ts | 2 +- .../common/models/case_with_comments.ts | 34 ++++++++----------- .../cases/server/services/alerts/index.ts | 26 +++++++++++++- 3 files changed, 41 insertions(+), 21 deletions(-) diff --git a/x-pack/plugins/cases/server/client/alerts/types.ts b/x-pack/plugins/cases/server/client/alerts/types.ts index d5e6c2e8252e9..ed2b6626427a9 100644 --- a/x-pack/plugins/cases/server/client/alerts/types.ts +++ b/x-pack/plugins/cases/server/client/alerts/types.ts @@ -39,6 +39,6 @@ export interface AlertGet { } export interface UpdateAlertCasesRequest { - alerts: Array<{ id: string; index: string }>; + alerts: AlertInfo[]; caseIds: string[]; } diff --git a/x-pack/plugins/cases/server/common/models/case_with_comments.ts b/x-pack/plugins/cases/server/common/models/case_with_comments.ts index 3a21fd3141ac6..cba3d461af45d 100644 --- a/x-pack/plugins/cases/server/common/models/case_with_comments.ts +++ b/x-pack/plugins/cases/server/common/models/case_with_comments.ts @@ -35,13 +35,12 @@ import { import type { CasesClientArgs } from '../../client'; import type { RefreshSetting } from '../../services/types'; import { createCaseError } from '../error'; -import type { CaseSavedObject } from '../types'; +import type { AlertInfo, CaseSavedObject } from '../types'; import { countAlertsForID, flattenCommentSavedObjects, transformNewComment, getOrUpdateLensReferences, - createAlertUpdateStatusRequest, isCommentRequestTypeAlert, getAlertInfoFromComments, } from '../utils'; @@ -312,31 +311,28 @@ export class CaseCommentModel { (attachment): attachment is CommentRequestAlertType => attachment.type === CommentType.alert ); - if (this.caseInfo.attributes.settings.syncAlerts) { - await this.updateAlertsStatus(alertAttachments); - } + const alerts = getAlertInfoFromComments(alertAttachments); + + if (alerts.length > 0) { + await this.params.services.alertsService.ensureAlertsAuthorized({ alerts }); + await this.updateAlertsSchemaWithCaseInfo(alerts); - if (alertAttachments.length > 0) { - await this.updateAlertsSchemaWithCaseInfo(alertAttachments); + if (this.caseInfo.attributes.settings.syncAlerts) { + await this.updateAlertsStatus(alerts); + } } } - private async updateAlertsStatus(alerts: CommentRequest[]) { - const alertsToUpdate = alerts - .map((alert) => - createAlertUpdateStatusRequest({ - comment: alert, - status: this.caseInfo.attributes.status, - }) - ) - .flat(); + private async updateAlertsStatus(alerts: AlertInfo[]) { + const alertsToUpdate = alerts.map((alert) => ({ + ...alert, + status: this.caseInfo.attributes.status, + })); await this.params.services.alertsService.updateAlertsStatus(alertsToUpdate); } - private async updateAlertsSchemaWithCaseInfo(alertAttachments: CommentRequestAlertType[]) { - const alerts = getAlertInfoFromComments(alertAttachments); - + private async updateAlertsSchemaWithCaseInfo(alerts: AlertInfo[]) { await this.params.services.alertsService.bulkUpdateCases({ alerts, caseIds: [this.caseInfo.id], diff --git a/x-pack/plugins/cases/server/services/alerts/index.ts b/x-pack/plugins/cases/server/services/alerts/index.ts index ad089857e2846..c79587aa57fb1 100644 --- a/x-pack/plugins/cases/server/services/alerts/index.ts +++ b/x-pack/plugins/cases/server/services/alerts/index.ts @@ -182,6 +182,10 @@ export class AlertService { ); } + private getNonEmptyAlerts(alerts: AlertInfo[]): AlertInfo[] { + return alerts.filter((alert) => !AlertService.isEmptyAlert(alert)); + } + public async getAlerts(alertsInfo: AlertInfo[]): Promise | undefined> { try { const docs = alertsInfo @@ -207,7 +211,7 @@ export class AlertService { public async bulkUpdateCases({ alerts, caseIds }: UpdateAlertCasesRequest): Promise { try { - const nonEmptyAlerts = alerts.filter((alert) => !AlertService.isEmptyAlert(alert)); + const nonEmptyAlerts = this.getNonEmptyAlerts(alerts); if (nonEmptyAlerts.length <= 0) { return; @@ -225,6 +229,26 @@ export class AlertService { }); } } + + public async ensureAlertsAuthorized({ alerts }: { alerts: AlertInfo[] }): Promise { + try { + const nonEmptyAlerts = this.getNonEmptyAlerts(alerts); + + if (nonEmptyAlerts.length <= 0) { + return; + } + + await this.alertsClient.ensureAllAlertsAuthorizedRead({ + alerts: nonEmptyAlerts, + }); + } catch (error) { + throw createCaseError({ + message: `Failed to authorize alerts: ${error}`, + error, + logger: this.logger, + }); + } + } } interface TranslatedUpdateAlertRequest { From a9e556203f39563a1c81dc8b032e602655f8781e Mon Sep 17 00:00:00 2001 From: Christos Nasikas Date: Wed, 8 Feb 2023 14:34:49 +0200 Subject: [PATCH 18/20] More tests --- x-pack/plugins/cases/server/services/mocks.ts | 1 + .../common/lib/authentication/roles.ts | 7 +- .../tests/common/comments/post_comment.ts | 68 +++++++-- .../internal/bulk_create_attachments.ts | 144 +++++++++++------- 4 files changed, 152 insertions(+), 68 deletions(-) diff --git a/x-pack/plugins/cases/server/services/mocks.ts b/x-pack/plugins/cases/server/services/mocks.ts index 8ef4c37a27200..59fdc9565fdd6 100644 --- a/x-pack/plugins/cases/server/services/mocks.ts +++ b/x-pack/plugins/cases/server/services/mocks.ts @@ -138,6 +138,7 @@ export const createAlertServiceMock = (): AlertServiceMock => { getAlerts: jest.fn(), executeAggregations: jest.fn(), bulkUpdateCases: jest.fn(), + ensureAlertsAuthorized: jest.fn(), }; // the cast here is required because jest.Mocked tries to include private members and would throw an error diff --git a/x-pack/test/cases_api_integration/common/lib/authentication/roles.ts b/x-pack/test/cases_api_integration/common/lib/authentication/roles.ts index 46c4fe58efadd..96262829f90dd 100644 --- a/x-pack/test/cases_api_integration/common/lib/authentication/roles.ts +++ b/x-pack/test/cases_api_integration/common/lib/authentication/roles.ts @@ -194,7 +194,12 @@ export const securitySolutionOnlyReadAlerts: Role = { name: 'sec_only_read_alerts', privileges: { elasticsearch: { - indices: [], + indices: [ + { + names: ['*'], + privileges: ['all'], + }, + ], }, kibana: [ { diff --git a/x-pack/test/cases_api_integration/security_and_spaces/tests/common/comments/post_comment.ts b/x-pack/test/cases_api_integration/security_and_spaces/tests/common/comments/post_comment.ts index 31269536ec009..cda903f2793ec 100644 --- a/x-pack/test/cases_api_integration/security_and_spaces/tests/common/comments/post_comment.ts +++ b/x-pack/test/cases_api_integration/security_and_spaces/tests/common/comments/post_comment.ts @@ -401,14 +401,28 @@ export default ({ getService }: FtrProviderContext): void => { await es.indices.refresh({ index: alertIndex }); }; - const bulkCreateAlertsAndVerifyAlertStatus = async ( - syncAlerts: boolean, - expectedAlertStatus: string - ) => { - const postedCase = await createCase(supertest, { - ...postCaseReq, - settings: { syncAlerts }, - }); + const bulkCreateAlertsAndVerifyAlertStatus = async ({ + syncAlerts, + expectedAlertStatus, + caseAuth, + attachmentExpectedHttpCode, + attachmentAuth, + }: { + syncAlerts: boolean; + expectedAlertStatus: string; + caseAuth?: { user: User; space: string | null }; + attachmentExpectedHttpCode?: number; + attachmentAuth?: { user: User; space: string | null }; + }) => { + const postedCase = await createCase( + supertest, + { + ...postCaseReq, + settings: { syncAlerts }, + }, + 200, + caseAuth + ); await updateCase({ supertest, @@ -421,6 +435,7 @@ export default ({ getService }: FtrProviderContext): void => { }, ], }, + auth: caseAuth, }); const signals = await createSecuritySolutionAlerts(supertest, log); @@ -432,6 +447,8 @@ export default ({ getService }: FtrProviderContext): void => { caseId: postedCase.id, alertId: alert._id, alertIndex: alert._index, + expectedHttpCode: attachmentExpectedHttpCode, + auth: attachmentAuth, }); const updatedAlert = await getSecuritySolutionAlerts(supertest, [alert._id]); @@ -471,11 +488,42 @@ export default ({ getService }: FtrProviderContext): void => { }; it('should change the status of the alert if sync alert is on', async () => { - await bulkCreateAlertsAndVerifyAlertStatus(true, 'acknowledged'); + await bulkCreateAlertsAndVerifyAlertStatus({ + syncAlerts: true, + expectedAlertStatus: 'acknowledged', + }); }); it('should NOT change the status of the alert if sync alert is off', async () => { - await bulkCreateAlertsAndVerifyAlertStatus(false, 'open'); + await bulkCreateAlertsAndVerifyAlertStatus({ + syncAlerts: false, + expectedAlertStatus: 'open', + }); + }); + + it('should change the status of the alert when the user has read access only', async () => { + await bulkCreateAlertsAndVerifyAlertStatus({ + syncAlerts: true, + expectedAlertStatus: 'acknowledged', + caseAuth: { + user: superUser, + space: 'space1', + }, + attachmentAuth: { user: secOnlyReadAlerts, space: 'space1' }, + }); + }); + + it('should NOT change the status of the alert when the user does NOT have access to the alert', async () => { + await bulkCreateAlertsAndVerifyAlertStatus({ + syncAlerts: true, + expectedAlertStatus: 'open', + caseAuth: { + user: superUser, + space: 'space1', + }, + attachmentExpectedHttpCode: 403, + attachmentAuth: { user: obsSec, space: 'space1' }, + }); }); it('should add the case ID to the alert schema', async () => { diff --git a/x-pack/test/cases_api_integration/security_and_spaces/tests/common/internal/bulk_create_attachments.ts b/x-pack/test/cases_api_integration/security_and_spaces/tests/common/internal/bulk_create_attachments.ts index ff7dc1b45ac85..d8502abd4e40a 100644 --- a/x-pack/test/cases_api_integration/security_and_spaces/tests/common/internal/bulk_create_attachments.ts +++ b/x-pack/test/cases_api_integration/security_and_spaces/tests/common/internal/bulk_create_attachments.ts @@ -13,7 +13,6 @@ import { BulkCreateCommentRequest, CaseResponse, CaseStatuses, - CommentRequest, CommentType, } from '@kbn/cases-plugin/common/api'; import { FtrProviderContext } from '../../../../common/ftr_provider_context'; @@ -485,49 +484,59 @@ export default ({ getService }: FtrProviderContext): void => { await esArchiver.unload('x-pack/test/functional/es_archives/auditbeat/hosts'); }); - const bulkCreateCommentAndRefreshIndex = async ({ + const bulkCreateAttachmentsAndRefreshIndex = async ({ caseId, - alertId, - alertIndex, + alerts, expectedHttpCode = 200, auth = { user: superUser, space: null }, }: { caseId: string; - alertId: string; - alertIndex: string; + alerts: Array<{ id: string; index: string }>; expectedHttpCode?: number; auth?: { user: User; space: string | null }; }) => { await bulkCreateAttachments({ supertest: supertestWithoutAuth, caseId, - params: [ - { - alertId, - index: alertIndex, - rule: { - id: 'id', - name: 'name', - }, - owner: 'securitySolutionFixture', - type: CommentType.alert, + params: alerts.map((alert) => ({ + alertId: alert.id, + index: alert.index, + rule: { + id: 'id', + name: 'name', }, - ], + owner: 'securitySolutionFixture', + type: CommentType.alert, + })), expectedHttpCode, auth, }); - await es.indices.refresh({ index: alertIndex }); + await es.indices.refresh({ index: alerts.map((alert) => alert.index) }); }; - const bulkCreateAlertsAndVerifyAlertStatus = async ( - syncAlerts: boolean, - expectedAlertStatus: string - ) => { - const postedCase = await createCase(supertest, { - ...postCaseReq, - settings: { syncAlerts }, - }); + const bulkCreateAlertsAndVerifyAlertStatus = async ({ + syncAlerts, + expectedAlertStatus, + caseAuth, + attachmentExpectedHttpCode, + attachmentAuth, + }: { + syncAlerts: boolean; + expectedAlertStatus: string; + caseAuth?: { user: User; space: string | null }; + attachmentExpectedHttpCode?: number; + attachmentAuth?: { user: User; space: string | null }; + }) => { + const postedCase = await createCase( + supertest, + { + ...postCaseReq, + settings: { syncAlerts }, + }, + 200, + caseAuth + ); await updateCase({ supertest, @@ -540,39 +549,34 @@ export default ({ getService }: FtrProviderContext): void => { }, ], }, + auth: caseAuth, }); const signals = await createSecuritySolutionAlerts(supertest, log); - const attachments: CommentRequest[] = []; + const alerts: Array<{ id: string; index: string }> = []; const indices: string[] = []; const ids: string[] = []; signals.hits.hits.forEach((alert) => { expect(alert._source?.[ALERT_WORKFLOW_STATUS]).eql('open'); - attachments.push({ - alertId: alert._id, + + alerts.push({ + id: alert._id, index: alert._index, - rule: { - id: 'id', - name: 'name', - }, - owner: 'securitySolutionFixture', - type: CommentType.alert, }); indices.push(alert._index); ids.push(alert._id); }); - await bulkCreateAttachments({ - supertest, + await bulkCreateAttachmentsAndRefreshIndex({ caseId: postedCase.id, - params: attachments, + alerts, + auth: attachmentAuth, + expectedHttpCode: attachmentExpectedHttpCode, }); - await es.indices.refresh({ index: indices }); - const updatedAlerts = await getSecuritySolutionAlerts(supertest, ids); updatedAlerts.hits.hits.forEach((alert) => { @@ -594,10 +598,9 @@ export default ({ getService }: FtrProviderContext): void => { const alert = signals.hits.hits[0]; for (const theCase of cases) { - await bulkCreateCommentAndRefreshIndex({ + await bulkCreateAttachmentsAndRefreshIndex({ caseId: theCase.id, - alertId: alert._id, - alertIndex: alert._index, + alerts: [{ id: alert._id, index: alert._index }], }); } @@ -612,11 +615,42 @@ export default ({ getService }: FtrProviderContext): void => { }; it('should change the status of the alerts if sync alert is on', async () => { - await bulkCreateAlertsAndVerifyAlertStatus(true, 'acknowledged'); + await bulkCreateAlertsAndVerifyAlertStatus({ + syncAlerts: true, + expectedAlertStatus: 'acknowledged', + }); }); it('should NOT change the status of the alert if sync alert is off', async () => { - await bulkCreateAlertsAndVerifyAlertStatus(false, 'open'); + await bulkCreateAlertsAndVerifyAlertStatus({ + syncAlerts: false, + expectedAlertStatus: 'open', + }); + }); + + it('should change the status of the alert when the user has read access only', async () => { + await bulkCreateAlertsAndVerifyAlertStatus({ + syncAlerts: true, + expectedAlertStatus: 'acknowledged', + caseAuth: { + user: superUser, + space: 'space1', + }, + attachmentAuth: { user: secOnlyReadAlerts, space: 'space1' }, + }); + }); + + it('should NOT change the status of the alert when the user does NOT have access to the alert', async () => { + await bulkCreateAlertsAndVerifyAlertStatus({ + syncAlerts: true, + expectedAlertStatus: 'open', + caseAuth: { + user: superUser, + space: 'space1', + }, + attachmentExpectedHttpCode: 403, + attachmentAuth: { user: obsSec, space: 'space1' }, + }); }); it('should add the case ID to the alert schema', async () => { @@ -632,10 +666,9 @@ export default ({ getService }: FtrProviderContext): void => { const postedCase = cases[0]; const alert = updatedAlert.hits.hits[0]; - await bulkCreateCommentAndRefreshIndex({ + await bulkCreateAttachmentsAndRefreshIndex({ caseId: postedCase.id, - alertId: alert._id, - alertIndex: alert._index, + alerts: [{ id: alert._id, index: alert._index }], }); const updatedAlertSecondTime = await getSecuritySolutionAlerts(supertest, [alert._id]); @@ -653,10 +686,9 @@ export default ({ getService }: FtrProviderContext): void => { settings: { syncAlerts: false }, }); - await bulkCreateCommentAndRefreshIndex({ + await bulkCreateAttachmentsAndRefreshIndex({ caseId: postedCase.id, - alertId: alert._id, - alertIndex: alert._index, + alerts: [{ id: alert._id, index: alert._index }], expectedHttpCode: 400, }); }); @@ -675,10 +707,9 @@ export default ({ getService }: FtrProviderContext): void => { const signals = await createSecuritySolutionAlerts(supertest, log); const alert = signals.hits.hits[0]; - await bulkCreateCommentAndRefreshIndex({ + await bulkCreateAttachmentsAndRefreshIndex({ caseId: postedCase.id, - alertId: alert._id, - alertIndex: alert._index, + alerts: [{ id: alert._id, index: alert._index }], expectedHttpCode: 200, auth: { user: secOnlyReadAlerts, space: 'space1' }, }); @@ -698,10 +729,9 @@ export default ({ getService }: FtrProviderContext): void => { const signals = await createSecuritySolutionAlerts(supertest, log); const alert = signals.hits.hits[0]; - await bulkCreateCommentAndRefreshIndex({ + await bulkCreateAttachmentsAndRefreshIndex({ caseId: postedCase.id, - alertId: alert._id, - alertIndex: alert._index, + alerts: [{ id: alert._id, index: alert._index }], expectedHttpCode: 403, auth: { user: obsSec, space: 'space1' }, }); From 7e5e8c081f69a3746fb92a2e03c0860ad00d1f21 Mon Sep 17 00:00:00 2001 From: Christos Nasikas Date: Thu, 9 Feb 2023 16:27:45 +0200 Subject: [PATCH 19/20] Fix conflicts --- x-pack/plugins/cases/kibana.jsonc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/x-pack/plugins/cases/kibana.jsonc b/x-pack/plugins/cases/kibana.jsonc index 0bda577c93623..5253600789b7b 100644 --- a/x-pack/plugins/cases/kibana.jsonc +++ b/x-pack/plugins/cases/kibana.jsonc @@ -25,7 +25,8 @@ "management", "spaces", "security", - "notifications" + "notifications", + "ruleRegistry" ], "optionalPlugins": [ "home", From ae05f46ea814a03a608dc42d0a73be7eb1fb2327 Mon Sep 17 00:00:00 2001 From: Christos Nasikas Date: Fri, 10 Feb 2023 17:01:59 +0200 Subject: [PATCH 20/20] PR feedback --- .../server/alert_data_client/alerts_client.ts | 2 +- .../common/lib/alerts.ts | 2 +- .../common/lib/authentication/roles.ts | 19 ++++++++ .../common/lib/authentication/users.ts | 8 ++++ .../tests/common/comments/post_comment.ts | 45 +++++++++++++++++-- .../internal/bulk_create_attachments.ts | 38 +++++++++++++++- 6 files changed, 107 insertions(+), 7 deletions(-) diff --git a/x-pack/plugins/rule_registry/server/alert_data_client/alerts_client.ts b/x-pack/plugins/rule_registry/server/alert_data_client/alerts_client.ts index 198fcf75c0c71..2ffe3451ac568 100644 --- a/x-pack/plugins/rule_registry/server/alert_data_client/alerts_client.ts +++ b/x-pack/plugins/rule_registry/server/alert_data_client/alerts_client.ts @@ -561,7 +561,7 @@ export class AlertsClient { return mgetRes; } catch (exc) { - this.logger.error(`error in ensureAlertsAuthorized ${exc}`); + this.logger.error(`error in ensureAllAlertsAuthorized ${exc}`); throw exc; } } diff --git a/x-pack/test/cases_api_integration/common/lib/alerts.ts b/x-pack/test/cases_api_integration/common/lib/alerts.ts index f5584b61cb934..5aadd7df90949 100644 --- a/x-pack/test/cases_api_integration/common/lib/alerts.ts +++ b/x-pack/test/cases_api_integration/common/lib/alerts.ts @@ -20,8 +20,8 @@ import { getQuerySignalIds, } from '../../../detection_engine_api_integration/utils'; import { superUser } from './authentication/users'; -import { getSpaceUrlPrefix } from './utils'; import { User } from './authentication/types'; +import { getSpaceUrlPrefix } from './api/helpers'; export const createSecuritySolutionAlerts = async ( supertest: SuperTest.SuperTest, diff --git a/x-pack/test/cases_api_integration/common/lib/authentication/roles.ts b/x-pack/test/cases_api_integration/common/lib/authentication/roles.ts index 96262829f90dd..eb11394a0cdf0 100644 --- a/x-pack/test/cases_api_integration/common/lib/authentication/roles.ts +++ b/x-pack/test/cases_api_integration/common/lib/authentication/roles.ts @@ -213,6 +213,24 @@ export const securitySolutionOnlyReadAlerts: Role = { }, }; +export const securitySolutionOnlyReadNoIndexAlerts: Role = { + name: 'sec_only_read_no_index_alerts', + privileges: { + elasticsearch: { + indices: [], + }, + kibana: [ + { + feature: { + securitySolutionFixture: ['all'], + siem: ['read'], + }, + spaces: ['space1'], + }, + ], + }, +}; + export const observabilityOnlyAll: Role = { name: 'obs_only_all', privileges: { @@ -321,4 +339,5 @@ export const roles = [ observabilityOnlyRead, observabilityOnlyReadAlerts, testDisabledPluginAll, + securitySolutionOnlyReadNoIndexAlerts, ]; diff --git a/x-pack/test/cases_api_integration/common/lib/authentication/users.ts b/x-pack/test/cases_api_integration/common/lib/authentication/users.ts index 48e060c926f48..8d997159cdb06 100644 --- a/x-pack/test/cases_api_integration/common/lib/authentication/users.ts +++ b/x-pack/test/cases_api_integration/common/lib/authentication/users.ts @@ -19,6 +19,7 @@ import { securitySolutionOnlyNoDelete, observabilityOnlyReadAlerts, securitySolutionOnlyReadAlerts, + securitySolutionOnlyReadNoIndexAlerts, } from './roles'; import { User } from './types'; @@ -64,6 +65,12 @@ export const secOnlyReadAlerts: User = { roles: [securitySolutionOnlyReadAlerts.name], }; +export const secSolutionOnlyReadNoIndexAlerts: User = { + username: 'sec_only_read_no_index_alerts', + password: 'sec_only_read_no_index_alerts', + roles: [securitySolutionOnlyReadNoIndexAlerts.name], +}; + export const obsOnly: User = { username: 'obs_only', password: 'obs_only', @@ -127,6 +134,7 @@ export const users = [ secOnly, secOnlyRead, secOnlyReadAlerts, + secSolutionOnlyReadNoIndexAlerts, secOnlyDelete, secOnlyNoDelete, obsOnly, diff --git a/x-pack/test/cases_api_integration/security_and_spaces/tests/common/comments/post_comment.ts b/x-pack/test/cases_api_integration/security_and_spaces/tests/common/comments/post_comment.ts index 635b30a383f22..c1512fbc4194c 100644 --- a/x-pack/test/cases_api_integration/security_and_spaces/tests/common/comments/post_comment.ts +++ b/x-pack/test/cases_api_integration/security_and_spaces/tests/common/comments/post_comment.ts @@ -53,6 +53,7 @@ import { obsOnlyReadAlerts, obsSec, secOnlyReadAlerts, + secSolutionOnlyReadNoIndexAlerts, } from '../../../../common/lib/authentication/users'; import { getSecuritySolutionAlerts, @@ -413,7 +414,7 @@ export default ({ getService }: FtrProviderContext): void => { attachmentAuth?: { user: User; space: string | null }; }) => { const postedCase = await createCase( - supertest, + supertestWithoutAuth, { ...postCaseReq, settings: { syncAlerts }, @@ -423,7 +424,7 @@ export default ({ getService }: FtrProviderContext): void => { ); await updateCase({ - supertest, + supertest: supertestWithoutAuth, params: { cases: [ { @@ -499,7 +500,7 @@ export default ({ getService }: FtrProviderContext): void => { }); }); - it('should change the status of the alert when the user has read access only', async () => { + it('should change the status of the alert when the user has write access to the indices and only read access to the siem solution', async () => { await bulkCreateAlertsAndVerifyAlertStatus({ syncAlerts: true, expectedAlertStatus: 'acknowledged', @@ -524,6 +525,19 @@ export default ({ getService }: FtrProviderContext): void => { }); }); + it('should NOT change the status of the alert when the user has read access to the kibana feature but no read access to the ES index', async () => { + await bulkCreateAlertsAndVerifyAlertStatus({ + syncAlerts: true, + expectedAlertStatus: 'open', + caseAuth: { + user: superUser, + space: 'space1', + }, + attachmentExpectedHttpCode: 500, + attachmentAuth: { user: secSolutionOnlyReadNoIndexAlerts, space: 'space1' }, + }); + }); + it('should add the case ID to the alert schema', async () => { await bulkCreateAlertsAndVerifyCaseIdsInAlertSchema(1); }); @@ -566,7 +580,7 @@ export default ({ getService }: FtrProviderContext): void => { }); }); - it('should add the case ID to the alert schema when the user has read access only', async () => { + it('should add the case ID to the alert schema when the user has write access to the indices and only read access to the siem solution', async () => { const postedCase = await createCase( supertest, { @@ -611,6 +625,29 @@ export default ({ getService }: FtrProviderContext): void => { auth: { user: obsSec, space: 'space1' }, }); }); + + it('should add the case ID to the alert schema when the user has read access to the kibana feature but no read access to the ES index', async () => { + const postedCase = await createCase( + supertest, + { + ...postCaseReq, + settings: { syncAlerts: false }, + }, + 200, + { user: superUser, space: 'space1' } + ); + + const signals = await createSecuritySolutionAlerts(supertest, log); + const alert = signals.hits.hits[0]; + + await createCommentAndRefreshIndex({ + caseId: postedCase.id, + alertId: alert._id, + alertIndex: alert._index, + expectedHttpCode: 200, + auth: { user: secSolutionOnlyReadNoIndexAlerts, space: 'space1' }, + }); + }); }); describe('observability', () => { diff --git a/x-pack/test/cases_api_integration/security_and_spaces/tests/common/internal/bulk_create_attachments.ts b/x-pack/test/cases_api_integration/security_and_spaces/tests/common/internal/bulk_create_attachments.ts index d346f1351d93b..460c56d08cf27 100644 --- a/x-pack/test/cases_api_integration/security_and_spaces/tests/common/internal/bulk_create_attachments.ts +++ b/x-pack/test/cases_api_integration/security_and_spaces/tests/common/internal/bulk_create_attachments.ts @@ -50,6 +50,7 @@ import { secOnly, secOnlyRead, secOnlyReadAlerts, + secSolutionOnlyReadNoIndexAlerts, superUser, } from '../../../../common/lib/authentication/users'; import { @@ -626,7 +627,7 @@ export default ({ getService }: FtrProviderContext): void => { }); }); - it('should change the status of the alert when the user has read access only', async () => { + it('should change the status of the alert when the user has write access to the indices and only read access to the siem solution', async () => { await bulkCreateAlertsAndVerifyAlertStatus({ syncAlerts: true, expectedAlertStatus: 'acknowledged', @@ -651,6 +652,19 @@ export default ({ getService }: FtrProviderContext): void => { }); }); + it('should NOT change the status of the alert when the user has read access to the kibana feature but no read access to the ES index', async () => { + await bulkCreateAlertsAndVerifyAlertStatus({ + syncAlerts: true, + expectedAlertStatus: 'open', + caseAuth: { + user: superUser, + space: 'space1', + }, + attachmentExpectedHttpCode: 500, + attachmentAuth: { user: secSolutionOnlyReadNoIndexAlerts, space: 'space1' }, + }); + }); + it('should add the case ID to the alert schema', async () => { await bulkCreateAlertsAndVerifyCaseIdsInAlertSchema(1); }); @@ -734,6 +748,28 @@ export default ({ getService }: FtrProviderContext): void => { auth: { user: obsSec, space: 'space1' }, }); }); + + it('should add the case ID to the alert schema when the user has read access to the kibana feature but no read access to the ES index', async () => { + const postedCase = await createCase( + supertest, + { + ...postCaseReq, + settings: { syncAlerts: false }, + }, + 200, + { user: superUser, space: 'space1' } + ); + + const signals = await createSecuritySolutionAlerts(supertest, log); + const alert = signals.hits.hits[0]; + + await bulkCreateAttachmentsAndRefreshIndex({ + caseId: postedCase.id, + alerts: [{ id: alert._id, index: alert._index }], + expectedHttpCode: 200, + auth: { user: secSolutionOnlyReadNoIndexAlerts, space: 'space1' }, + }); + }); }); describe('observability', () => {