-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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] Add EQL query editable component with EQL options fields #199115
base: main
Are you sure you want to change the base?
Conversation
@@ -33,7 +33,6 @@ export const timelineDefaults: SubsetTimelineModel & | |||
description: '', | |||
eqlOptions: { | |||
eventCategoryField: 'event.category', | |||
tiebreakerField: '', |
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.
tiebreakerField
as an empty doesn't look correct for the EQL validator. It sends requests to /internal/search/eql
to validate the request. It returns an error when a tiebreaker field has an empty string.
Additionally it looks like EQL options weren't provided correctly before this PR. Timeline EQL validation requests were sent without EQL options.
Pinging @elastic/security-detections-response (Team:Detections and Resp) |
Pinging @elastic/security-solution (Team: SecuritySolution) |
Pinging @elastic/security-detection-rule-management (Team:Detection Rule Management) |
2291ad0
to
db856ba
Compare
Thanks for all the work here! Pulled down and tested in rule creation/edit flow. So far LGTM. Confirmed that logic around disabling suppression for sequence queries remains. cc @dhurley14 |
QueryLanguageEnum, | ||
} from '../../../../../../../../../common/api/detection_engine'; | ||
import { EqlQueryEditAdapter } from './eql_query_edit_adapter'; | ||
// import { |
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.
Commented out code. Can be removed.
eqlQuery: { | ||
validations: [ | ||
// { | ||
// validator: debounceAsync(eqlValidator, 300), |
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.
Commented out code. Can be removed.
); | ||
} | ||
|
||
const kqlQuerySchema = { |
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.
Since there are no validations we can probably remove the schema
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.
RuleFieldEditFormWrapper
's ruleFieldFormSchema
property is required. We still should pass an empty object. So removing it completely it's possible.
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.
Got it. Then let's rename it to eqlQuerySchema
.
326eeb0
to
64b371f
Compare
Hi @nkhristinin, thanks for testing the PR. Indeed
Could you elaborate on that? A screenshot or a video recording would be nice. |
Finally was able to see that in diff, thanks! |
disabled, | ||
onEqlOptionsChange, | ||
onValidityChange, | ||
onValiditingChange, |
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.
Looks like there's a typo. Did you mean onIsValidatingChange
?
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.
Thanks for noticing. I copies this prop name from the existing EqlQueryBar
component but it definitely makes sense to fix this typo.
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.
Thanks, @maximpn! I have manually tested the EQL field in both the flyout and the Rule Creation and Editing pages. I also re-tested the query bar validation with other rule types since it was affected by this PR.
I haven't found any additional issues except the one I mentioned in #199115 (comment). As we decided, it should be addressed separately after discussing it with the product.
also left a few minor comments about the code. Overall, the PR looks good to me now.
e77ca51
to
7408045
Compare
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.
The code LGTM and I did some smoke testing and everything seems to be working normally.
Though I'm a little worried about the changes made to Timeline. I don't have enough knowledge about how things work internally related to this tiebreakerField
(I don't even what it is or what it does).
I would feel more comfortable if someone with more experience (@michaelolo24 @kqualters-elastic @janmonschke) was having a second look at this PR before approving.
7408045
to
1156ef3
Compare
💚 Build Succeeded
Metrics [docs]Module Count
Async chunks
Unknown metric groupsReferences to deprecated APIs
History
cc @maximpn |
Partially addresses: #171520
Summary
This PR adds is built on top of #193828 and #196948 and adds an EQL Query editable component with EQL Options fields (
event_category_override
,timestamp_field
andtiebreaker_field
) for Three Way Diff tab's final edit side of the upgrade prebuilt rule workflow.Details
This PR make a set of changes to make existing EQL Query bar component easily reusable and type safe when used in forms. In particular the following was done
EqlQueryEdit
component withUseField
inside. It helps to make it type safe avoiding issues like passing invalid types toEqlQueryBar
.UseField
types component properties asRecord<string, any>
so potentially any refactoring can break some functionality. For example code in Timeline passesDataViewSpec
whereDataViewBase
is expected while these two types aren't fully compatible.EqlQueryEdit
. Passing field configuration toUseField
rewrites field configuration defined in from schema. It leads to cases when validation is defined in both form schema and as a field configuration forUseFields
. Additionally we can reduce reusing complexity by incapsulating absolutely required validation inEqlQueryEdit
component.tiebreakerField
was removed in Timelines.tiebreakerField
is part of EQL options used for EQL validation. EQL validation endpoint/internal/search/eql
returns an error when an empty string provided fortiebreakerField
. This problem didn't surface earlier since It looks like EQL options weren't provided correctly before this PR. Timeline EQL validation requests were sent without EQL options.How to test
The simplest way to test is via patching installed prebuilt rules via Rule Patch API. Please follow steps below
Potential Code Execution via Postgresql
with rule_id2a692072-d78d-42f3-a48a-775677d79c4e
Detection Rules (SIEM)
Page ->Rule Updates
-> click onPotential Code Execution via Postgresql
rule -> expandEQL Query
to see EQL Query -> pressEdit
buttonScreenshots