-
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][Detections] Improves indicator match Cypress tests #94913
[Security Solution][Detections] Improves indicator match Cypress tests #94913
Conversation
indicatorMapping: 'agent.id', | ||
indicatorIndexField: 'agent.threat', | ||
indicatorIndexPattern: ['filebeat-*'], | ||
indicatorMapping: 'myhash.mysha256', |
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 realize this isn't a new field, but the data structure we're using here conflicts with the terminology in the application: a mapping contains a "field" and a "value", which here correspond to `indicatorMapping" and "indicatorIndexField", respectively
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 assume that the disconnect between cypress' typings and the actual application typings is expected/desired, but I wanted to call this out as a potential source of confusion moving forward.
"type": "doc", | ||
"value": { | ||
"id": "_eZE7mwBOpWiDweStB_c", | ||
"index": "threat-data-001", |
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.
threat-data
is not very descriptive, especially when paired with threat_indicator
. Perhaps we could call this suspicious_source_events
or something like that?
|
||
export const JSON_CONTENT = '.ace_content'; | ||
|
||
export const JSON_VIEW_TAB = '#json-view'; |
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.
We should be able to add data-test-subj
s for these tabs, but it looks like it would have to go into the tab's contents.
"id": "_uZE6nwBOpWiDweSth_D", | ||
"index": "threat-indicator-0001", | ||
"id": "84cf452c1e0375c3d4412cb550bd1783358468a3b3b777da4829d72c7d6fb74f", | ||
"index": "filebeat-7.12.0-2021.03.10-000001", |
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 mappings for this index is currently 25k LOC; I think you can copy/paste/slightly modify the FTR mappings to reduce that file by two orders of magnitude 😉
@elasticmachine merge upstream |
d8da4a3
to
9e7968b
Compare
Pinging @elastic/security-solution (Team: SecuritySolution) |
Pinging @elastic/security-detections-response (Team:Detections and Resp) |
💚 Build SucceededMetrics [docs]Async chunks
History
To update your PR or re-run it, just comment with: |
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.
Mostly style questions here, nothing that should block this from getting merged since it's now green! 🍏 📗 💚
I would really like to try and keep the mappings as minimal as possible for these archives, but we can do that in a followup if need be; better to have the coverage now.
const expectedEnrichment = [ | ||
{ line: 4, text: ' "threat": {' }, |
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.
Nit: this is an array so the subsequent .forEach
reads a little odd.
const expectedEnrichment = [ | |
{ line: 4, text: ' "threat": {' }, | |
const expectedJsonViewRows = [ | |
{ index: 4, text: ' "threat": {' }, |
|
||
export const scrollJsonViewToBottom = () => { | ||
cy.get(JSON_CONTENT).click({ force: true }); | ||
cy.get(JSON_CONTENT).type('{pagedown}{pagedown}{pagedown}'); |
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.
Interesting! Are there no prebuilt scroll
helpers in cypress that would work here?
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.
There is but does not work with that concrete element (I don't know why)
} from '../screens/fields_browser'; | ||
import { KQL_SEARCH_BAR } from '../screens/hosts/main'; | ||
|
||
export const addsFields = (fields: string[]) => { |
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.
Nit: the tense on these helpers is a little strange; most of these use imperative tense ("add", "remove", "delete"), but several in this file use simple present tense ("adds"). Not the end of the world, just a discrepancy that gave me pause while reviewing.
@@ -92,6 +95,10 @@ export const TIMELINE_TEMPLATE_DETAILS = 'Timeline template'; | |||
|
|||
export const TIMESTAMP_OVERRIDE_DETAILS = 'Timestamp override'; | |||
|
|||
export const TIMELINE_FIELD = (field: string) => { |
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.
Do we have any standards/guidelines for when to use all caps vs when to use camel case, here? I had assumed that functions would use camelcase as in getDetails
below.
"fileset": { | ||
"name": "abusemalware" | ||
}, | ||
"threatintel": { |
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.
💯
"properties": { | ||
"@timestamp": { | ||
"type": "date" | ||
}, | ||
"agent": { | ||
"properties": { | ||
"build": { |
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.
Do we want to trim this down further to only the fields that we're using in the accompanying data.json
? I thought that was the consensus but maybe I misunderstood.
Thanks for the review @rylnd I'll merge it to have coverage as soon as possible and we can do all the mentioned changes in a future PR. |
elastic#94913) * updates the data used in the test * adds matches test * adds enrichment test * improves speed and adds missing files * fixes type check issue * adds 'data-test-subj' for the json view tab * refactor * fixes typecheck issue * updates tests with latest master changes Co-authored-by: Kibana Machine <[email protected]>
#94913) (#95449) * updates the data used in the test * adds matches test * adds enrichment test * improves speed and adds missing files * fixes type check issue * adds 'data-test-subj' for the json view tab * refactor * fixes typecheck issue * updates tests with latest master changes Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Kibana Machine <[email protected]>
Summary
In this PR we are extending the current Indicator Match rules cypress tests:
Pending to add a test to test the investigate on timeline as soon as the following bug is fixed: #95095