Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Cloud Security] Alerts Preview Contextual Flyout #197102

Merged
40 changes: 40 additions & 0 deletions x-pack/packages/kbn-cloud-security-posture/common/utils/helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -62,3 +62,43 @@ export const buildEntityFlyoutPreviewQuery = (field: string, queryValue?: string
},
};
};

export const buildEntityAlertsQuery = (field: string, queryValue?: string, size?: number) => {
maxcold marked this conversation as resolved.
Show resolved Hide resolved
return {
size: size || 0,
_source: false,
fields: [
maxcold marked this conversation as resolved.
Show resolved Hide resolved
'kibana.alert.rule.uuid',
'signal.rule.name',
'signal.rule.severity',
'kibana.alert.reason',
],
query: {
bool: {
filter: [
{
bool: {
must: [],
filter: [
{
match_phrase: {
[field]: {
query: queryValue,
},
},
},
],
should: [],
must_not: [],
},
},
{
terms: {
'kibana.alert.workflow_status': ['open', 'acknowledged'],
maxcold marked this conversation as resolved.
Show resolved Hide resolved
},
},
],
},
},
};
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
/*
* 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 React from 'react';
import { render } from '@testing-library/react';
import { AlertsPreview } from './alerts_preview';
import { TestProviders } from '../../../common/mock/test_providers';
import { useExpandableFlyoutApi } from '@kbn/expandable-flyout';
import type { ParsedAlertsData } from '../../../overview/components/detection_response/alerts_by_status/types';

const mockAlertsData: ParsedAlertsData = {
closed: { total: 2, severities: [{ key: 'low', value: 1, label: 'Low' }] },
open: {
total: 3,
severities: [
{ key: 'low', value: 2, label: 'Low' },
{ key: 'medium', value: 1, label: 'Medium' },
],
},
acknowledged: {
total: 2,
severities: [
{ key: 'low', value: 1, label: 'Low' },
{ key: 'high', value: 1, label: 'High' },
],
},
};

jest.mock(
'../../../detections/components/alerts_kpis/alerts_summary_charts_panel/use_summary_chart_data'
);
jest.mock('@kbn/expandable-flyout');

describe('AlertsPreview', () => {
const mockOpenLeftPanel = jest.fn();

beforeEach(() => {
(useExpandableFlyoutApi as jest.Mock).mockReturnValue({ openLeftPanel: mockOpenLeftPanel });
});
afterEach(() => {
jest.clearAllMocks();
});

it('renders', () => {
const { getByTestId } = render(
<TestProviders>
<AlertsPreview alertsData={mockAlertsData} alertsCount={5} />
</TestProviders>
);

expect(getByTestId('securitySolutionFlyoutInsightsAlertsTitleText')).toBeInTheDocument();
maxcold marked this conversation as resolved.
Show resolved Hide resolved
});

it('renders correct alerts number', () => {
const { getByTestId } = render(
<TestProviders>
<AlertsPreview alertsData={mockAlertsData} alertsCount={5} />
</TestProviders>
);

expect(getByTestId('securitySolutionFlyoutInsightsAlertsCount').textContent).toEqual('5');
});

it('renders correct number distribution bar based on severity', () => {
const { queryAllByTestId } = render(
<TestProviders>
<AlertsPreview alertsData={mockAlertsData} alertsCount={5} />
</TestProviders>
);

expect(queryAllByTestId('undefined__part').length).toEqual(3);
maxcold marked this conversation as resolved.
Show resolved Hide resolved
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,118 @@
/*
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we shouldn't be the owners of this component, it has nothing to do with Cloud Security Posture. I understand that this is the outcome of this unclear ownership, but I would try to find a more suitable place for the alerts component. @PhilippeOberti wdyt about the code ownership? right now it's a bit like "who wrote the code owns it", but I don't think it is a good idea longer term

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah it's a bit tricky... While I agree that the logic isn't related to Cloud Security, if the component is only used within another component owned by Cloud Security it's difficult for other teams to know how to properly test it (where it is, how to generate data to have things show correctly...).
A few times I've had situation where I didn't even know that one of our components was used by another team and didn't test things when I made changes to it...

Also, if that component is only used in one place, it's hard to justify having us own it. What if we need to make changes to it? We would still need a way to have you guys review it to make sure we don't brake your functionality?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

on the other side, we are the ones that are supposed to know how this thing works, so I totally get your point of us owning it. I just feel like if we want to do this, we then need to have a good look at it before it's being merged to make sure we understand what it does, we should have a say on how it's being written and place it in a folder that makes sense

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, let's discuss it in the sync as proposed. Right now we have the situation that we've built misconfig, vuln and alert previews on the entity flyout and you folks built the same but on document flyout. For me the ideal situation would be to split the ownership by the business domain: we own everything related to cloud security (misconfiguration and vulnerability) and alerts should be owned by the team owning the business domain of alerts. Which team is the owner of the alerts logic in the Security Org is unclear to me tbh. But if we continue to own based on who wrote the code, we have the risk of things being broken (no necessarily technically but rather in business sense) without the owners of the domain logic realising that

* 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 React from 'react';
import { capitalize } from 'lodash';
import { css } from '@emotion/react';
import type { EuiThemeComputed } from '@elastic/eui';
import { EuiFlexGroup, EuiFlexItem, EuiSpacer, EuiText, EuiTitle, useEuiTheme } from '@elastic/eui';
import { FormattedMessage } from '@kbn/i18n-react';
import { DistributionBar } from '@kbn/security-solution-distribution-bar';
import { getAbbreviatedNumber } from '@kbn/cloud-security-posture-common';
import { ExpandablePanel } from '../../../flyout/shared/components/expandable_panel';
import { getSeverityColor } from '../../../detections/components/alerts_kpis/severity_level_panel/helpers';
import type {
AlertsByStatus,
ParsedAlertsData,
} from '../../../overview/components/detection_response/alerts_by_status/types';

const AlertsCount = ({
alertsTotal,
euiTheme,
}: {
alertsTotal: string | number;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this truly be a string? We can use only a number instead of a union.

euiTheme: EuiThemeComputed<{}>;
}) => {
return (
<EuiFlexItem>
<EuiFlexGroup direction="column" gutterSize="none">
<EuiFlexItem>
<EuiTitle size="s">
<h1 data-test-subj={'securitySolutionFlyoutInsightsAlertsCount'}>{alertsTotal}</h1>
</EuiTitle>
</EuiFlexItem>
<EuiFlexItem>
<EuiText
size="m"
css={css`
font-weight: ${euiTheme.font.weight.semiBold};
`}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
css={css`
font-weight: ${euiTheme.font.weight.semiBold};
`}
css={{
fontWeight: euiTheme.font.weight.semiBold,
}}
>

>
<FormattedMessage
id="xpack.securitySolution.flyout.right.insights.alerts.alertsCountDescription"
defaultMessage="Alerts"
/>
</EuiText>
</EuiFlexItem>
</EuiFlexGroup>
</EuiFlexItem>
);
};

export const AlertsPreview = ({
alertsData,
alertsCount,
isPreviewMode,
}: {
alertsData: ParsedAlertsData;
alertsCount: number;
isPreviewMode?: boolean;
}) => {
const { euiTheme } = useEuiTheme();

const severityMap = new Map<string, number>();

(['open', 'acknowledged'] as AlertsByStatus[]).forEach((status) => {
Copy link
Contributor

@maxcold maxcold Nov 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you don't need this piece of logic here as you filter out closed in the entity insight. So the component can be free of knowing about different statuses

alertsData?.[status]?.severities.forEach((severity) => {
if (severityMap.has(severity.key)) {
severityMap.set(severity.key, (severityMap?.get(severity.key) || 0) + severity.value);
} else {
severityMap.set(severity.key, severity.value);
}
});
});

const alertStats = Array.from(severityMap, ([key, count]) => ({
key: capitalize(key),
count,
color: getSeverityColor(key),
}));

return (
<ExpandablePanel
header={{
title: (
<EuiText
size="xs"
css={css`
font-weight: ${euiTheme.font.weight.semiBold};
`}
>
<FormattedMessage
id="xpack.securitySolution.flyout.right.insights.alerts.alertsTitle"
defaultMessage="Alerts"
/>
</EuiText>
),
}}
data-test-subj={'securitySolutionFlyoutInsightsAlerts'}
>
<EuiFlexGroup gutterSize="none">
<AlertsCount alertsTotal={getAbbreviatedNumber(alertsCount)} euiTheme={euiTheme} />
<EuiFlexItem grow={2}>
<EuiFlexGroup direction="column" gutterSize="none">
<EuiFlexItem />
<EuiFlexItem>
<EuiSpacer />
<DistributionBar stats={alertStats.reverse()} />
</EuiFlexItem>
</EuiFlexGroup>
</EuiFlexItem>
</EuiFlexGroup>
</ExpandablePanel>
);
};
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@

import { EuiAccordion, EuiHorizontalRule, EuiSpacer, EuiTitle, useEuiTheme } from '@elastic/eui';

import React from 'react';
import React, { useMemo } from 'react';
import { css } from '@emotion/react';
import { FormattedMessage } from '@kbn/i18n-react';
import { useMisconfigurationPreview } from '@kbn/cloud-security-posture/src/hooks/use_misconfiguration_preview';
Expand All @@ -16,6 +16,11 @@ import { useVulnerabilitiesPreview } from '@kbn/cloud-security-posture/src/hooks
import { hasVulnerabilitiesData } from '@kbn/cloud-security-posture';
import { MisconfigurationsPreview } from './misconfiguration/misconfiguration_preview';
import { VulnerabilitiesPreview } from './vulnerabilities/vulnerabilities_preview';
import { AlertsPreview } from './alerts/alerts_preview';
import { useGlobalTime } from '../../common/containers/use_global_time';
import { DETECTION_RESPONSE_ALERTS_BY_STATUS_ID } from '../../overview/components/detection_response/alerts_by_status/types';
import { useAlertsByStatus } from '../../overview/components/detection_response/alerts_by_status/use_alerts_by_status';
import { useSignalIndex } from '../../detections/containers/detection_engine/alerts/use_signal_index';

export const EntityInsight = <T,>({
name,
Expand Down Expand Up @@ -60,6 +65,39 @@ export const EntityInsight = <T,>({

const isVulnerabilitiesFindingForHost = hasVulnerabilitiesFindings && fieldName === 'host.name';

const { signalIndexName } = useSignalIndex();

const entityFilter = useMemo(() => ({ field: fieldName, value: name }), [fieldName, name]);

const { to, from } = useGlobalTime();

const { items: alertsData } = useAlertsByStatus({
entityFilter,
signalIndexName,
queryId: DETECTION_RESPONSE_ALERTS_BY_STATUS_ID,
to,
from,
});

const alertsOpenCount = alertsData?.open?.total || 0;

const alertsAcknowledgedCount = alertsData?.acknowledged?.total || 0;

const alertsCount = alertsOpenCount + alertsAcknowledgedCount;

if (alertsCount > 0) {
insightContent.push(
<>
<AlertsPreview
alertsData={alertsData}
maxcold marked this conversation as resolved.
Show resolved Hide resolved
alertsCount={alertsCount}
isPreviewMode={isPreviewMode}
/>
<EuiSpacer size="s" />
</>
);
}

if (hasMisconfigurationFindings)
insightContent.push(
<>
Expand All @@ -76,7 +114,8 @@ export const EntityInsight = <T,>({
);
return (
<>
{(hasMisconfigurationFindings ||
{(insightContent.length > 0 ||
hasMisconfigurationFindings ||
(isVulnerabilitiesFindingForHost && hasVulnerabilitiesFindings)) && (
<>
<EuiAccordion
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -149,8 +149,8 @@ export const VulnerabilitiesPreview = ({
callback: goToEntityInsightTab,
tooltip: (
<FormattedMessage
id="xpack.securitySolution.flyout.right.insights.misconfiguration.misconfigurationTooltip"
defaultMessage="Show all misconfiguration findings"
id="xpack.securitySolution.flyout.right.insights.vulnerabilities.vulnerabilitiesTooltip"
defaultMessage="Show all vulnerabilities findings"
/>
),
}
Expand Down