Skip to content

Commit

Permalink
[Security Solution] Allow users to edit max_signals field for custom …
Browse files Browse the repository at this point in the history
…rules (elastic#179680)

**Resolves: elastic#173593
**Fixes: elastic#164234

## Summary

Adds a number component in the create and edit rule forms so that users
are able to customize the `max_signals` value for custom rules from the
UI. Also adds validations to the rule API's for invalid values being
passed in.

This PR also exposes the `xpack.alerting.rules.run.alerts.max` config
setting from the alerting framework to the frontend and backend so that
we can validate against it as it supersedes our own `max_signals` value.

[Flaky test run (internal)

](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/5601)

### Screenshots
**Form component**
<p align="center">
<img width="887" alt="Screenshot 2024-04-08 at 11 02 12 PM"
src="https://github.com/elastic/kibana/assets/56367316/58cd2f6d-61b6-4343-8025-ff867c050dd7">
</p>

**Details Page**
<p align="center">
<img width="595" alt="Screenshot 2024-04-08 at 11 04 04 PM"
src="https://github.com/elastic/kibana/assets/56367316/d2c61593-3d35-408e-b047-b4d1f68898f8">
</p>

**Error state**
<p align="center">
<img width="857" alt="Screenshot 2024-04-08 at 11 01 55 PM"
src="https://github.com/elastic/kibana/assets/56367316/86e64280-7b81-46f2-b223-fde8c20066c8">
</p>

**Warning state**
<p align="center">
<img width="601" alt="Screenshot 2024-04-16 at 3 20 00 PM"
src="https://github.com/elastic/kibana/assets/56367316/eab07d62-3d3e-4c85-8468-36c3e56c5a99">
</p>

### Checklist

Delete any items that are not applicable to this PR.

- [x] Any text added follows [EUI's writing
guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses
sentence case text and includes [i18n
support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)
- [x]
[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)
was added for features that require explanation or tutorials
- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
- [x] [Flaky Test
Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was
used on any tests changed
- [x] Any UI touched in this PR is usable by keyboard only (learn more
about [keyboard accessibility](https://webaim.org/techniques/keyboard/))


### For maintainers

- [ ] This was checked for breaking API changes and was [labeled
appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)

---------

Co-authored-by: Juan Pablo Djeredjian <jpdjeredjian@gmail.com>
  • Loading branch information
dplumlee and jpdjere committed May 3, 2024
1 parent 8575fc5 commit f841763
Show file tree
Hide file tree
Showing 54 changed files with 640 additions and 85 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -330,6 +330,7 @@ export default function ({ getService }: PluginFunctionalProviderContext) {
'xpack.stack_connectors.enableExperimental (array)',
'xpack.trigger_actions_ui.enableExperimental (array)',
'xpack.trigger_actions_ui.enableGeoTrackingThresholdAlert (boolean)',
'xpack.alerting.rules.run.alerts.max (number)',
'xpack.upgrade_assistant.featureSet.migrateSystemIndices (boolean)',
'xpack.upgrade_assistant.featureSet.mlSnapshots (boolean)',
'xpack.upgrade_assistant.featureSet.reindexCorrectiveActions (boolean)',
Expand Down
1 change: 1 addition & 0 deletions x-pack/plugins/alerting/public/mocks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ const createSetupContract = (): Setup => ({

const createStartContract = (): Start => ({
getNavigation: jest.fn(),
getMaxAlertsPerRun: jest.fn(),
});

export const alertingPluginMock = {
Expand Down
14 changes: 12 additions & 2 deletions x-pack/plugins/alerting/public/plugin.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
* 2.0.
*/

import { AlertingPublicPlugin } from './plugin';
import { AlertingPublicPlugin, AlertingUIConfig } from './plugin';
import { coreMock } from '@kbn/core/public/mocks';
import {
createManagementSectionMock,
Expand All @@ -17,7 +17,17 @@ jest.mock('./services/rule_api', () => ({
loadRuleType: jest.fn(),
}));

const mockInitializerContext = coreMock.createPluginInitializerContext();
const mockAlertingUIConfig: AlertingUIConfig = {
rules: {
run: {
alerts: {
max: 1000,
},
},
},
};

const mockInitializerContext = coreMock.createPluginInitializerContext(mockAlertingUIConfig);
const management = managementPluginMock.createSetupContract();
const mockSection = createManagementSectionMock();

Expand Down
21 changes: 20 additions & 1 deletion x-pack/plugins/alerting/public/plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ export interface PluginSetupContract {
}
export interface PluginStartContract {
getNavigation: (ruleId: Rule['id']) => Promise<string | undefined>;
getMaxAlertsPerRun: () => number;
}
export interface AlertingPluginSetup {
management: ManagementSetup;
Expand All @@ -69,13 +70,28 @@ export interface AlertingPluginStart {
data: DataPublicPluginStart;
}

export interface AlertingUIConfig {
rules: {
run: {
alerts: {
max: number;
};
};
};
}

export class AlertingPublicPlugin
implements
Plugin<PluginSetupContract, PluginStartContract, AlertingPluginSetup, AlertingPluginStart>
{
private alertNavigationRegistry?: AlertNavigationRegistry;
private config: AlertingUIConfig;
readonly maxAlertsPerRun: number;

constructor(private readonly initContext: PluginInitializerContext) {}
constructor(private readonly initContext: PluginInitializerContext) {
this.config = this.initContext.config.get<AlertingUIConfig>();
this.maxAlertsPerRun = this.config.rules.run.alerts.max;
}

public setup(core: CoreSetup, plugins: AlertingPluginSetup) {
this.alertNavigationRegistry = new AlertNavigationRegistry();
Expand Down Expand Up @@ -150,6 +166,9 @@ export class AlertingPublicPlugin
return rule.viewInAppRelativeUrl;
}
},
getMaxAlertsPerRun: () => {
return this.maxAlertsPerRun;
},
};
}
}
2 changes: 1 addition & 1 deletion x-pack/plugins/alerting/server/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ export type AlertingConfig = TypeOf<typeof configSchema>;
export type RulesConfig = TypeOf<typeof rulesSchema>;
export type AlertingRulesConfig = Pick<
AlertingConfig['rules'],
'minimumScheduleInterval' | 'maxScheduledPerMinute'
'minimumScheduleInterval' | 'maxScheduledPerMinute' | 'run'
> & {
isUsingSecurity: boolean;
};
Expand Down
8 changes: 5 additions & 3 deletions x-pack/plugins/alerting/server/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,7 @@
import type { PublicMethodsOf } from '@kbn/utility-types';
import { PluginConfigDescriptor, PluginInitializerContext } from '@kbn/core/server';
import { RulesClient as RulesClientClass } from './rules_client';
import { configSchema } from './config';
import { AlertsConfigType } from './types';
import { AlertingConfig, configSchema } from './config';

export type RulesClient = PublicMethodsOf<RulesClientClass>;

Expand Down Expand Up @@ -79,8 +78,11 @@ export const plugin = async (initContext: PluginInitializerContext) => {
return new AlertingPlugin(initContext);
};

export const config: PluginConfigDescriptor<AlertsConfigType> = {
export const config: PluginConfigDescriptor<AlertingConfig> = {
schema: configSchema,
exposeToBrowser: {
rules: { run: { alerts: { max: true } } },
},
deprecations: ({ renameFromRoot, deprecate }) => [
renameFromRoot('xpack.alerts.healthCheck', 'xpack.alerting.healthCheck', { level: 'warning' }),
renameFromRoot(
Expand Down
1 change: 1 addition & 0 deletions x-pack/plugins/alerting/server/plugin.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,7 @@ describe('Alerting Plugin', () => {
maxScheduledPerMinute: 10000,
isUsingSecurity: false,
minimumScheduleInterval: { value: '1m', enforce: false },
run: { alerts: { max: 1000 }, actions: { max: 1000 } },
});

expect(setupContract.frameworkAlerts.enabled()).toEqual(false);
Expand Down
2 changes: 1 addition & 1 deletion x-pack/plugins/alerting/server/plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -458,7 +458,7 @@ export class AlertingPlugin {
},
getConfig: () => {
return {
...pick(this.config.rules, ['minimumScheduleInterval', 'maxScheduledPerMinute']),
...pick(this.config.rules, ['minimumScheduleInterval', 'maxScheduledPerMinute', 'run']),
isUsingSecurity: this.licenseState ? !!this.licenseState.getIsSecurityEnabled() : false,
};
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,6 @@ components:
references:
$ref: './common_attributes.schema.yaml#/components/schemas/RuleReferenceArray'

# maxSignals not used in ML rules but probably should be used
max_signals:
$ref: './common_attributes.schema.yaml#/components/schemas/MaxSignals'
threat:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ import { fieldFormatsMock } from '@kbn/field-formats-plugin/common/mocks';
import { indexPatternFieldEditorPluginMock } from '@kbn/data-view-field-editor-plugin/public/mocks';
import { UpsellingService } from '@kbn/security-solution-upselling/service';
import { calculateBounds } from '@kbn/data-plugin/common';
import { alertingPluginMock } from '@kbn/alerting-plugin/public/mocks';

const mockUiSettings: Record<string, unknown> = {
[DEFAULT_TIME_RANGE]: { from: 'now-15m', to: 'now', mode: 'quick' },
Expand Down Expand Up @@ -128,6 +129,7 @@ export const createStartServicesMock = (
const cloud = cloudMock.createStart();
const mockSetHeaderActionMenu = jest.fn();
const mockTimelineFilterManager = createFilterManagerMock();
const alerting = alertingPluginMock.createStartContract();

/*
* Below mocks are needed by unified field list
Expand Down Expand Up @@ -250,6 +252,7 @@ export const createStartServicesMock = (
dataViewFieldEditor: indexPatternFieldEditorPluginMock.createStartContract(),
upselling: new UpsellingService(),
timelineFilterManager: mockTimelineFilterManager,
alerting,
} as unknown as StartServices;
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -263,7 +263,7 @@ describe('description_step', () => {
mockLicenseService
);

expect(result.length).toEqual(13);
expect(result.length).toEqual(14);
});
});

Expand Down Expand Up @@ -768,6 +768,33 @@ describe('description_step', () => {
});
});
});

describe('maxSignals', () => {
test('returns default "max signals" description', () => {
const result: ListItems[] = getDescriptionItem(
'maxSignals',
'Max alerts per run',
mockAboutStep,
mockFilterManager,
mockLicenseService
);

expect(result[0].title).toEqual('Max alerts per run');
expect(result[0].description).toEqual(100);
});

test('returns empty array when "value" is a undefined', () => {
const result: ListItems[] = getDescriptionItem(
'maxSignals',
'Max alerts per run',
{ ...mockAboutStep, maxSignals: undefined },
mockFilterManager,
mockLicenseService
);

expect(result.length).toEqual(0);
});
});
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -342,6 +342,9 @@ export const getDescriptionItem = (
return get('isBuildingBlock', data)
? [{ title: i18n.BUILDING_BLOCK_LABEL, description: i18n.BUILDING_BLOCK_DESCRIPTION }]
: [];
} else if (field === 'maxSignals') {
const value: number | undefined = get(field, data);
return value ? [{ title: label, description: value }] : [];
}

const description: string = get(field, data);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
/*
* 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, { useMemo, useCallback } from 'react';
import type { EuiFieldNumberProps } from '@elastic/eui';
import { EuiTextColor, EuiFormRow, EuiFieldNumber, EuiIcon } from '@elastic/eui';
import type { FieldHook } from '@kbn/es-ui-shared-plugin/static/forms/hook_form_lib';
import { css } from '@emotion/css';
import { DEFAULT_MAX_SIGNALS } from '../../../../../common/constants';
import * as i18n from './translations';
import { useKibana } from '../../../../common/lib/kibana';

interface MaxSignalsFieldProps {
dataTestSubj: string;
field: FieldHook<number | ''>;
idAria: string;
isDisabled: boolean;
placeholder?: string;
}

const MAX_SIGNALS_FIELD_WIDTH = 200;

export const MaxSignals: React.FC<MaxSignalsFieldProps> = ({
dataTestSubj,
field,
idAria,
isDisabled,
placeholder,
}): JSX.Element => {
const { setValue, value } = field;
const { alerting } = useKibana().services;
const maxAlertsPerRun = alerting.getMaxAlertsPerRun();

const [isInvalid, error] = useMemo(() => {
if (typeof value === 'number' && !isNaN(value) && value <= 0) {
return [true, i18n.GREATER_THAN_ERROR];
}
return [false];
}, [value]);

const hasWarning = useMemo(
() => typeof value === 'number' && !isNaN(value) && value > maxAlertsPerRun,
[maxAlertsPerRun, value]
);

const handleMaxSignalsChange: EuiFieldNumberProps['onChange'] = useCallback(
(e) => {
const maxSignalsValue = (e.target as HTMLInputElement).value;
// Has to handle an empty string as the field is optional
setValue(maxSignalsValue !== '' ? Number(maxSignalsValue.trim()) : '');
},
[setValue]
);

const helpText = useMemo(() => {
const textToRender = [];
if (hasWarning) {
textToRender.push(
<EuiTextColor color="warning">{i18n.LESS_THAN_WARNING(maxAlertsPerRun)}</EuiTextColor>
);
}
textToRender.push(i18n.MAX_SIGNALS_HELP_TEXT(DEFAULT_MAX_SIGNALS));
return textToRender;
}, [hasWarning, maxAlertsPerRun]);

return (
<EuiFormRow
css={css`
.euiFormControlLayout {
width: ${MAX_SIGNALS_FIELD_WIDTH}px;
}
`}
describedByIds={idAria ? [idAria] : undefined}
fullWidth
helpText={helpText}
label={field.label}
labelAppend={field.labelAppend}
isInvalid={isInvalid}
error={error}
>
<EuiFieldNumber
isInvalid={isInvalid}
value={value as EuiFieldNumberProps['value']}
onChange={handleMaxSignalsChange}
isLoading={field.isValidating}
placeholder={placeholder}
data-test-subj={dataTestSubj}
disabled={isDisabled}
append={hasWarning ? <EuiIcon size="s" type="warning" color="warning" /> : undefined}
/>
</EuiFormRow>
);
};

MaxSignals.displayName = 'MaxSignals';
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
/*
* 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 { i18n } from '@kbn/i18n';

export const GREATER_THAN_ERROR = i18n.translate(
'xpack.securitySolution.detectionEngine.createRule.stepAboutRule.maxAlertsFieldGreaterThanError',
{
defaultMessage: 'Max alerts must be greater than 0.',
}
);

export const LESS_THAN_WARNING = (maxNumber: number) =>
i18n.translate(
'xpack.securitySolution.detectionEngine.createRule.stepAboutRule.maxAlertsFieldLessThanWarning',
{
values: { maxNumber },
defaultMessage:
'Kibana only allows a maximum of {maxNumber} {maxNumber, plural, =1 {alert} other {alerts}} per rule run.',
}
);

export const MAX_SIGNALS_HELP_TEXT = (defaultNumber: number) =>
i18n.translate(
'xpack.securitySolution.detectionEngine.createRule.stepAboutRule.fieldMaxAlertsHelpText',
{
values: { defaultNumber },
defaultMessage:
'The maximum number of alerts the rule will create each time it runs. Default is {defaultNumber}.',
}
);
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
* 2.0.
*/

import { DEFAULT_MAX_SIGNALS } from '../../../../../common/constants';
import type { AboutStepRule } from '../../../../detections/pages/detection_engine/rules/types';
import { fillEmptySeverityMappings } from '../../../../detections/pages/detection_engine/rules/helpers';

Expand Down Expand Up @@ -33,5 +34,6 @@ export const stepAboutDefaultValue: AboutStepRule = {
timestampOverride: '',
threat: threatDefault,
note: '',
maxSignals: DEFAULT_MAX_SIGNALS,
setup: '',
};
Loading

0 comments on commit f841763

Please sign in to comment.