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

[Security Solution] Allow users to edit required_fields field for custom rules #180682

Merged
merged 73 commits into from
May 17, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
73 commits
Select commit Hold shift + click to select a range
da17d7f
Add support for "required_fields" to create and edit rule endpoints
nikitaindik Mar 5, 2024
ce45a3a
WIP
nikitaindik Apr 12, 2024
ae926df
WIP
nikitaindik Apr 18, 2024
d7829db
Fix placeholders and validation
nikitaindik Apr 22, 2024
39fec26
Add more tests
nikitaindik Apr 22, 2024
d9006c4
Update i18n
nikitaindik Apr 23, 2024
194c5e6
Remove commented out code
nikitaindik Apr 23, 2024
316e404
Update Cypress E2E tests
nikitaindik Apr 23, 2024
954034e
Update API integration tests
nikitaindik Apr 23, 2024
cc6f6cd
Remove `related_integrations` from `PrebuiltRuleAsset` type
nikitaindik Apr 26, 2024
3c7eaa5
Fix UI issues, update tests
nikitaindik May 3, 2024
44f48d0
Remove required_fields from baseFields
nikitaindik May 4, 2024
a4294c4
Update i18n JSONs
nikitaindik May 4, 2024
f25362a
Merge API integration tests with Related Integerations
nikitaindik May 4, 2024
6c72992
Add a couple API integration tests to check defaulting req. fields value
nikitaindik May 4, 2024
1998ca3
Fix typos
nikitaindik May 4, 2024
8c1b79e
Fix a couple integration tests
nikitaindik May 5, 2024
b5f1c92
Update types
nikitaindik May 5, 2024
6a92698
Attempt to fix a bug with extracting indices from ESQL queries
nikitaindik May 6, 2024
835ca7e
Update comments
nikitaindik May 6, 2024
85dc485
Update paddings
nikitaindik May 6, 2024
e4f1270
Update comments in schemas
nikitaindik May 7, 2024
eb7e382
Fix missing "Related Integrations" label in rule creation preview
nikitaindik May 7, 2024
817bfe7
Extract render function into a component, move field logic into hooks…
nikitaindik May 7, 2024
b20f629
Use `ruleType` instead of `defineStepData.ruleType` (they are same va…
nikitaindik May 7, 2024
20fc01e
Fix a typo in comment
nikitaindik May 7, 2024
9c8ad08
Get rid of the hacky `isValidEsqlQuery` hook
nikitaindik May 8, 2024
0ee95a4
Update `useEsqlIndex` tests
nikitaindik May 8, 2024
d56fe2c
Add an explainer comment for `RuleToImport` type
nikitaindik May 8, 2024
c679627
Refactor `use_name_field`
nikitaindik May 8, 2024
09db2e6
Apply suggested refactoring to `useNameField`
nikitaindik May 8, 2024
f9470b2
Update `fieldsWithTypes` assignment in `RequiredFieldsList`
nikitaindik May 8, 2024
c2566d1
Fix a typo in schema definition comment
nikitaindik May 8, 2024
c5067d8
Omit the use of empty `initialState` in `RequiredFields` tests
nikitaindik May 8, 2024
aceed3c
Use testIds in form tests
nikitaindik May 8, 2024
6de4f44
Remove the unnecessary `type` from form field config
nikitaindik May 8, 2024
eb55a4e
Update `PrebuiltRuleAsset` type
nikitaindik May 9, 2024
a779cd8
Remove unnecessary `RequiredFieldWithOptionalEcs` type
nikitaindik May 9, 2024
7ed1573
Update doc link for Related Integrations
nikitaindik May 12, 2024
44690b3
Optimize UI perf by skipping re-renders when irrelevant fields change
nikitaindik May 12, 2024
9fbb3d8
Fix failing API integration test
nikitaindik May 12, 2024
23c78f2
Use a help popover instead of `helpText`
nikitaindik May 13, 2024
c01b461
Mock `useKibana` in tests to fix them
nikitaindik May 14, 2024
1ee24f1
Jest tests: Simplify waiting for `handleSubmit` to be called
nikitaindik May 14, 2024
3e7368d
Jest tests: Refactor `selectEuiComboBoxOption`
nikitaindik May 14, 2024
4edc2d2
Jest tests: Use a different testId in warning tests
nikitaindik May 14, 2024
4cc35ba
Refactor: Use `.some` instead of `.find` to compute `hasEmptyFieldName`
nikitaindik May 14, 2024
61a6475
Refactor: Move `makeValidateRequiredField` into a separate file
nikitaindik May 14, 2024
2a571e0
Refactor `pickTypeForName` and cover it with tests
nikitaindik May 14, 2024
5a36af8
Refactor: Warning creation
nikitaindik May 14, 2024
a8ae5c8
Refactor API integration test: "creates a rule with required_fields d…
nikitaindik May 14, 2024
941609e
Refactor API integration test: "should reset required fields field to…
nikitaindik May 14, 2024
6f875f2
Fix existing bug: display field type icons in Rule Creation and Rule …
nikitaindik May 14, 2024
0838d57
Refactor: wrap selected combobox items into an array just before pass…
nikitaindik May 14, 2024
f920a58
Commit an automatic tsconfig.json fix
nikitaindik May 14, 2024
6fefac8
Jest test: Fix incorrectly mocked URL
nikitaindik May 14, 2024
693eb32
Merge branch 'main' into editable-required-fields
nikitaindik May 15, 2024
ec9e049
Fix: Add missing icons
nikitaindik May 15, 2024
f506946
Merge branch 'main' into editable-required-fields
nikitaindik May 15, 2024
daf770f
Refactor: remove unnecessary optional chaining in `pickTypeForName`
nikitaindik May 15, 2024
92c7980
Fix a typo
nikitaindik May 15, 2024
dfae1a3
Refactor: getting `flattenedFieldNames` in `RequiredFieldsList`
nikitaindik May 15, 2024
af57ac5
Update UI copy
nikitaindik May 15, 2024
1b1221b
Merge branch 'main' into editable-required-fields
nikitaindik May 15, 2024
9489507
Merge branch 'main' into editable-required-fields
nikitaindik May 15, 2024
f1bfb0d
Merge remote-tracking branch 'upstream/main' into editable-required-f…
nikitaindik May 16, 2024
4af5bb8
Merge remote-tracking branch 'upstream/main' into editable-required-f…
nikitaindik May 17, 2024
48d6880
Update tooltip text
nikitaindik May 17, 2024
a824c2f
Refactor: remove unnecessary `fieldsWithTypes` computation
nikitaindik May 17, 2024
050be5f
Jest tests: Wrap into `I18nProvider` to remove warnings
nikitaindik May 17, 2024
936fa94
Jest tests: Add a warning assertion
nikitaindik May 17, 2024
3838882
Related Integrations: Allow to remove the last row to match Required …
nikitaindik May 17, 2024
d5988c5
Merge remote-tracking branch 'upstream/main' into editable-required-f…
nikitaindik May 17, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/kbn-doc-links/src/get_doc_links.ts
Original file line number Diff line number Diff line change
Expand Up @@ -479,6 +479,7 @@ export const getDocLinks = ({ kibanaBranch, buildFlavor }: GetDocLinkOptions): D
},
privileges: `${SECURITY_SOLUTION_DOCS}endpoint-management-req.html`,
manageDetectionRules: `${SECURITY_SOLUTION_DOCS}rules-ui-management.html`,
createDetectionRules: `${SECURITY_SOLUTION_DOCS}rules-ui-create.html`,
createEsqlRuleType: `${SECURITY_SOLUTION_DOCS}rules-ui-create.html#create-esql-rule`,
ruleUiAdvancedParams: `${SECURITY_SOLUTION_DOCS}rules-ui-create.html#rule-ui-advanced-params`,
entityAnalytics: {
Expand Down
1 change: 1 addition & 0 deletions packages/kbn-doc-links/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -354,6 +354,7 @@ export interface DocLinks {
};
readonly privileges: string;
readonly manageDetectionRules: string;
readonly createDetectionRules: string;
readonly createEsqlRuleType: string;
readonly ruleUiAdvancedParams: string;
readonly entityAnalytics: {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -303,13 +303,40 @@ export const TimestampOverride = z.string();
export type TimestampOverrideFallbackDisabled = z.infer<typeof TimestampOverrideFallbackDisabled>;
export const TimestampOverrideFallbackDisabled = z.boolean();

/**
* Describes an Elasticsearch field that is needed for the rule to function
*/
export type RequiredField = z.infer<typeof RequiredField>;
export const RequiredField = z.object({
/**
* Name of an Elasticsearch field
*/
name: NonEmptyString,
/**
* Type of the Elasticsearch field
*/
type: NonEmptyString,
/**
* Whether the field is an ECS field
*/
ecs: z.boolean(),
});

/**
* Input parameters to create a RequiredField. Does not include the `ecs` field, because `ecs` is calculated on the backend based on the field name and type.
*/
export type RequiredFieldInput = z.infer<typeof RequiredFieldInput>;
export const RequiredFieldInput = z.object({
/**
* Name of an Elasticsearch field
*/
name: NonEmptyString,
/**
* Type of an Elasticsearch field
*/
type: NonEmptyString,
});

export type RequiredFieldArray = z.infer<typeof RequiredFieldArray>;
export const RequiredFieldArray = z.array(RequiredField);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -315,18 +315,36 @@ components:

RequiredField:
type: object
description: Describes an Elasticsearch field that is needed for the rule to function
properties:
name:
$ref: '../../../model/primitives.schema.yaml#/components/schemas/NonEmptyString'
description: Name of an Elasticsearch field
type:
$ref: '../../../model/primitives.schema.yaml#/components/schemas/NonEmptyString'
description: Type of the Elasticsearch field
nikitaindik marked this conversation as resolved.
Show resolved Hide resolved
ecs:
type: boolean
description: Whether the field is an ECS field
required:
- name
- type
- ecs

RequiredFieldInput:
type: object
description: Input parameters to create a RequiredField. Does not include the `ecs` field, because `ecs` is calculated on the backend based on the field name and type.
properties:
name:
$ref: '../../../model/primitives.schema.yaml#/components/schemas/NonEmptyString'
description: Name of an Elasticsearch field
type:
$ref: '../../../model/primitives.schema.yaml#/components/schemas/NonEmptyString'
description: Type of an Elasticsearch field
required:
- name
- type

RequiredFieldArray:
type: array
items:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ import {
ThreatArray,
SetupGuide,
RelatedIntegrationArray,
RequiredFieldInput,
RuleObjectId,
RuleSignatureId,
IsRuleImmutable,
Expand Down Expand Up @@ -137,6 +138,7 @@ export const BaseDefaultableFields = z.object({
threat: ThreatArray.optional(),
setup: SetupGuide.optional(),
related_integrations: RelatedIntegrationArray.optional(),
required_fields: z.array(RequiredFieldInput).optional(),
});

export type BaseCreateProps = z.infer<typeof BaseCreateProps>;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -117,22 +117,32 @@ components:
author:
$ref: './common_attributes.schema.yaml#/components/schemas/RuleAuthorArray'

# False positive examples
false_positives:
$ref: './common_attributes.schema.yaml#/components/schemas/RuleFalsePositiveArray'

# Reference URLs
references:
$ref: './common_attributes.schema.yaml#/components/schemas/RuleReferenceArray'

# Max alerts per run
max_signals:
$ref: './common_attributes.schema.yaml#/components/schemas/MaxSignals'
threat:
$ref: './common_attributes.schema.yaml#/components/schemas/ThreatArray'
setup:
$ref: './common_attributes.schema.yaml#/components/schemas/SetupGuide'

# Related integrations
related_integrations:
$ref: './common_attributes.schema.yaml#/components/schemas/RelatedIntegrationArray'

# Required fields
required_fields:
type: array
items:
$ref: './common_attributes.schema.yaml#/components/schemas/RequiredFieldInput'

xcrzx marked this conversation as resolved.
Show resolved Hide resolved
BaseCreateProps:
x-inline: true
allOf:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import * as z from 'zod';
import {
BaseCreateProps,
ResponseFields,
RequiredFieldInput,
RuleSignatureId,
TypeSpecificCreateProps,
} from '../../model/rule_schema';
Expand All @@ -29,5 +30,13 @@ export const RuleToImport = BaseCreateProps.and(TypeSpecificCreateProps).and(
ResponseFields.partial().extend({
rule_id: RuleSignatureId,
immutable: z.literal(false).default(false),
/*
Overriding `required_fields` from ResponseFields because
in ResponseFields `required_fields` has the output type,
but for importing rules, we need to use the input type.
Otherwise importing rules without the "ecs" property in
`required_fields` will fail.
*/
required_fields: z.array(RequiredFieldInput).optional(),
})
);
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ import type { FieldHook } from '../../../../shared_imports';
import type { Integration, RelatedIntegration } from '../../../../../common/api/detection_engine';
import { useIntegrations } from '../../../../detections/components/rules/related_integrations/use_integrations';
import { IntegrationStatusBadge } from './integration_status_badge';
import { DEFAULT_RELATED_INTEGRATION } from './default_related_integration';
import * as i18n from './translations';

interface RelatedIntegrationItemFormProps {
Expand Down Expand Up @@ -95,16 +94,6 @@ export function RelatedIntegrationField({
);

const hasError = Boolean(packageErrorMessage) || Boolean(versionErrorMessage);
const isLastField = relatedIntegrations.length === 1;
const isLastEmptyField = isLastField && field.value.package === '';
const handleRemove = useCallback(() => {
if (isLastField) {
field.setValue(DEFAULT_RELATED_INTEGRATION);
return;
}

onRemove();
}, [onRemove, field, isLastField]);

return (
<EuiFormRow
Expand Down Expand Up @@ -145,8 +134,8 @@ export function RelatedIntegrationField({
<EuiFlexItem grow={false}>
<EuiButtonIcon
color="danger"
onClick={handleRemove}
isDisabled={!integrations || isLastEmptyField}
onClick={onRemove}
isDisabled={!integrations}
iconType="trash"
aria-label={i18n.REMOVE_RELATED_INTEGRATION_BUTTON_ARIA_LABEL}
data-test-subj="relatedIntegrationRemove"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ jest.mock('../../../../common/lib/kibana', () => ({
docLinks: {
links: {
securitySolution: {
ruleUiAdvancedParams: 'http://link-to-docs',
createDetectionRules: 'http://link-to-docs',
},
},
},
Expand Down Expand Up @@ -669,82 +669,6 @@ describe('RelatedIntegrations form part', () => {
});
});
});

describe('sticky last form row', () => {
it('does not remove the last item', async () => {
render(<TestForm />, { wrapper: createReactQueryWrapper() });

await addRelatedIntegrationRow();
await removeLastRelatedIntegrationRow();

expect(screen.getAllByTestId(RELATED_INTEGRATION_ROW)).toHaveLength(1);
});

it('disables remove button after clicking remove button on the last item', async () => {
render(<TestForm />, { wrapper: createReactQueryWrapper() });

await addRelatedIntegrationRow();
await removeLastRelatedIntegrationRow();

expect(screen.getByTestId(REMOVE_INTEGRATION_ROW_BUTTON_TEST_ID)).toBeDisabled();
});

it('clears selected integration when clicking remove the last form row button', async () => {
render(<TestForm />, { wrapper: createReactQueryWrapper() });

await addRelatedIntegrationRow();
await selectFirstEuiComboBoxOption({
comboBoxToggleButton: getLastByTestId(COMBO_BOX_TOGGLE_BUTTON_TEST_ID),
});
await removeLastRelatedIntegrationRow();

expect(screen.queryByTestId(COMBO_BOX_SELECTION_TEST_ID)).not.toBeInTheDocument();
});

it('submits an empty integration after clicking remove the last form row button', async () => {
const handleSubmit = jest.fn();

render(<TestForm onSubmit={handleSubmit} />, { wrapper: createReactQueryWrapper() });

await addRelatedIntegrationRow();
await selectFirstEuiComboBoxOption({
comboBoxToggleButton: getLastByTestId(COMBO_BOX_TOGGLE_BUTTON_TEST_ID),
});
await removeLastRelatedIntegrationRow();
await submitForm();
await waitFor(() => {
expect(handleSubmit).toHaveBeenCalled();
});

expect(handleSubmit).toHaveBeenCalledWith({
data: [{ package: '', version: '' }],
isValid: true,
});
});

it('submits an empty integration after previously saved integrations were removed', async () => {
const initialRelatedIntegrations: RelatedIntegration[] = [
{ package: 'package-a', version: '^1.2.3' },
];
const handleSubmit = jest.fn();

render(<TestForm initialState={initialRelatedIntegrations} onSubmit={handleSubmit} />, {
wrapper: createReactQueryWrapper(),
});

await waitForIntegrationsToBeLoaded();
await removeLastRelatedIntegrationRow();
await submitForm();
await waitFor(() => {
expect(handleSubmit).toHaveBeenCalled();
});

expect(handleSubmit).toHaveBeenCalledWith({
data: [{ package: '', version: '' }],
isValid: true,
});
});
});
});
});

Expand Down Expand Up @@ -778,11 +702,6 @@ function TestForm({ initialState, onSubmit }: TestFormProps): JSX.Element {
);
}

function getLastByTestId(testId: string): HTMLElement {
// getAllByTestId throws an error when there are no `testId` elements found
return screen.getAllByTestId(testId).at(-1)!;
}

function waitForIntegrationsToBeLoaded(): Promise<void> {
return waitForElementToBeRemoved(screen.queryAllByRole('progressbar'));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ export function RelatedIntegrationsHelpInfo(): JSX.Element {
defaultMessage="Choose the {integrationsDocLink} this rule depends on, and correct if necessary each integration’s version constraint in {semverLink} format. Only tilde, caret, and plain versions are supported, such as ~1.2.3, ^1.2.3, or 1.2.3."
values={{
integrationsDocLink: (
<EuiLink href={docLinks.links.securitySolution.ruleUiAdvancedParams} target="_blank">
<EuiLink href={docLinks.links.securitySolution.createDetectionRules} target="_blank">
<FormattedMessage
id="xpack.securitySolution.detectionEngine.ruleDescription.relatedIntegrations.integrationsLink"
defaultMessage="integrations"
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
/*
* 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.
*/

export { RequiredFields } from './required_fields';
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
/*
* 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 { RequiredFieldInput } from '../../../../../common/api/detection_engine/model/rule_schema/common_attributes.gen';
import type { ERROR_CODE, FormData, ValidationFunc } from '../../../../shared_imports';
import * as i18n from './translations';

export function makeValidateRequiredField(parentFieldPath: string) {
return function validateRequiredField(
...args: Parameters<ValidationFunc<FormData, string, RequiredFieldInput>>
): ReturnType<ValidationFunc<{}, ERROR_CODE>> | undefined {
const [{ value, path, form }] = args;

const formData = form.getFormData();
const parentFieldData: RequiredFieldInput[] = formData[parentFieldPath];

const isFieldNameUsedMoreThanOnce =
parentFieldData.filter((field) => field.name === value.name).length > 1;

if (isFieldNameUsedMoreThanOnce) {
return {
code: 'ERR_FIELD_FORMAT',
path: `${path}.name`,
message: i18n.FIELD_NAME_USED_MORE_THAN_ONCE(value.name),
};
}

/* Allow empty rows. They are going to be removed before submission. */
if (value.name.trim().length === 0 && value.type.trim().length === 0) {
return;
}

if (value.name.trim().length === 0) {
return {
code: 'ERR_FIELD_MISSING',
path: `${path}.name`,
message: i18n.FIELD_NAME_REQUIRED,
};
}

if (value.type.trim().length === 0) {
return {
code: 'ERR_FIELD_MISSING',
path: `${path}.type`,
message: i18n.FIELD_TYPE_REQUIRED,
};
}
};
}
Loading