-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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 max_signals field for custom rules #179680
Merged
jpdjere
merged 62 commits into
elastic:main
from
dplumlee:max-signals-field-form-component
May 3, 2024
Merged
Changes from 58 commits
Commits
Show all changes
62 commits
Select commit
Hold shift + click to select a range
15db4e4
exposes alerting config setting and creates form compoenent
dplumlee af60e77
Merge remote-tracking branch 'upstream/main' into max-signals-field-f…
dplumlee bb5a1d2
changes user-facing language to max alerts
dplumlee 8015b84
adds tests
dplumlee 264cdb6
updates language
dplumlee 69dc936
adds param to one million mock constructors
dplumlee ca8b462
updates types
dplumlee d396a12
updates tests and mocks
dplumlee 07927d1
adds type
dplumlee 3522cb7
updates tests
dplumlee 1a1c121
Merge remote-tracking branch 'upstream/main' into max-signals-field-f…
dplumlee 0319c94
changes logic for max validations
dplumlee 1749631
updates tests
dplumlee 19ebcf0
adds warning state
dplumlee 813c807
reset config value
dplumlee 00e942a
Merge remote-tracking branch 'upstream/main' into max-signals-field-f…
dplumlee cd4eb47
adds max_signals rule execution logic
dplumlee 5ad1b65
updates tests and types
dplumlee ff4b232
adds cypress tests
dplumlee 2053068
Merge remote-tracking branch 'upstream/main' into max-signals-field-f…
dplumlee bec6bf7
updates test attributes
dplumlee aadde40
Merge remote-tracking branch 'upstream/main' into max-signals-field-f…
dplumlee 34588b2
updates warning design
dplumlee 61fbdc1
Merge remote-tracking branch 'upstream/main' into max-signals-field-f…
dplumlee 7aaa62a
Merge remote-tracking branch 'upstream/main' into max-signals-field-f…
dplumlee 3e749f6
updates language
dplumlee bd01d55
updates attribute
dplumlee aa2c565
Merge remote-tracking branch 'upstream/main' into max-signals-field-f…
dplumlee 975490e
Merge remote-tracking branch 'upstream/main' into max-signals-field-f…
dplumlee 25c05e3
addresses comments
dplumlee b47deff
Merge remote-tracking branch 'upstream/main' into max-signals-field-f…
dplumlee e709722
fixes execution logic
dplumlee 9886913
strips out no longer needed rulesClient param
dplumlee 89bd1e7
adds defaultable import export tests
dplumlee 304db6f
Merge remote-tracking branch 'upstream/main' into max-signals-field-f…
dplumlee db88782
addresses comments
dplumlee dd0a87e
updates tests
dplumlee bedbd7d
Merge remote-tracking branch 'upstream/main' into max-signals-field-f…
dplumlee f400f99
changes defaulting logic one last time
dplumlee 8add109
updates integration tests to match unified method
dplumlee e4e8af5
Merge remote-tracking branch 'upstream/main' into max-signals-field-f…
dplumlee 54b8041
fixes spelling mistakes
dplumlee ad64d19
addresses response ops changes
dplumlee 1cb02a2
fixes test
dplumlee 259d52e
Merge remote-tracking branch 'upstream/main' into max-signals-field-f…
dplumlee bcca901
addresses comments
dplumlee 346e10e
Merge remote-tracking branch 'upstream/main' into max-signals-field-f…
dplumlee ce2f06e
adds test
dplumlee 940f925
Merge remote-tracking branch 'upstream/main' into max-signals-field-f…
dplumlee 92d3ed2
Merge remote-tracking branch 'upstream/main' into max-signals-field-f…
dplumlee b1ce138
Merge remote-tracking branch 'upstream/main' into max-signals-field-f…
dplumlee 995462b
Merge remote-tracking branch 'upstream/main' into max-signals-field-f…
dplumlee 69f4b87
Merge remote-tracking branch 'upstream/main' into max-signals-field-f…
dplumlee dad1f8d
Merge remote-tracking branch 'upstream/main' into max-signals-field-f…
dplumlee 9244392
Merge remote-tracking branch 'upstream/main' into max-signals-field-f…
dplumlee 8d02476
Merge remote-tracking branch 'upstream/main' into max-signals-field-f…
dplumlee d0e0432
Merge remote-tracking branch 'upstream/main' into max-signals-field-f…
dplumlee 18a724f
Merge branch 'main' into max-signals-field-form-component
jpdjere 0b71b77
Fix handling of 0 in form
jpdjere 07d81e6
Added tests for form validation
jpdjere c850892
Remove empty line
jpdjere 3cbb10f
Merge branch 'main' into max-signals-field-form-component
jpdjere File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
99 changes: 99 additions & 0 deletions
99
...curity_solution/public/detection_engine/rule_creation_ui/components/max_signals/index.tsx
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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'; |
35 changes: 35 additions & 0 deletions
35
..._solution/public/detection_engine/rule_creation_ui/components/max_signals/translations.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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}.', | ||
} | ||
); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we use
exposeToBrowser
, then in theory we don't need this added to the triggers_actions_ui config route. ThoughexposeToBrowser
only makes it available to the browsers alerting plugin, not t_a_ui - though obviously we could make it accessible.But I don't think we need both, so we should pick one and not do the other. We don't need two ways to do the same thing ...
Or perhaps I missed something ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if I understand, the only
triggers_actions_ui
code that has been modified here was some test files to align with the mock alerting plugin. But to your larger point, I agree - theexposeToBrowser
has been implemented here and other plugins can use them if they so choose. That's how we're utilizing it in security solutionThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The way this is structured we are also changing the output of the HTTP endpoint
/internal/triggers_actions_ui/_config
- to return ALL the values underrun
,not just the alerts.max value -which is something we need to consider re: backwards compatibility, documentation, and security (should we be exposing these via API). Do we really need to change the output of this endpoint?
I believe it's also the case that the values returned by this endpoint are or can be calculated, so using the value from the config wouldn't work. Which is why we have this endpoint. Static values (I assume these are static) can just use the
exposeToBrowser
path.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the end, I would like to NOT change the output of the
_config
route (that's adding therun
props) - not so much because it's a security concern now, but feels like a slippery slope to having a problem later, if someone follows this pattern, and we do leak something we shouldn't.Feels like we need to remove
run
fromAlertingRulesConfig
, or change the_config
route in t_a_ui topick
the fields it should be returning from the bigger config object (the existing ones, but notrun
).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the second way is probably the more preferable method, just so we can use that
getConfig
function elsewhere in other plugins without changing the_config
route output. Right now we're using thegetConfig
function to compare on the server side in security solution as well which is why therun
props were added in the first place. If y'all are ok with exposing the config values underrun
to that internalgetConfig
method from the plugin setup object and modifying thetriggers_actions_ui
config route so that we're locked into the existing values, I can change that over.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have a problem exposting this at the plugin level - my concern is at the HTTP response level.
Filtering IN just the props we want returned from that
_config
endpoint would be perfect, as it means we won't have to worry about accidently leaking things later. So, in theory,x-pack/plugins/triggers_actions_ui/server/routes/config.test.ts
won't have any changes, but presumably it's pairconfig.ts
will.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok that all sounds good, I think we're on the same page 👍