Skip to content

[Security Solution][Exceptions Table] - Fix exceptions table search by name#88701

Merged
yctercero merged 26 commits intoelastic:masterfrom
yctercero:exceptions_dejavu
Feb 11, 2021
Merged

[Security Solution][Exceptions Table] - Fix exceptions table search by name#88701
yctercero merged 26 commits intoelastic:masterfrom
yctercero:exceptions_dejavu

Conversation

@yctercero
Copy link
Copy Markdown
Contributor

@yctercero yctercero commented Jan 19, 2021

Summary

Addresses #88450

Issue

It seems that the exceptions list table search was not filtering out. Turns out when using a search like list_id:SOME_LIST_ID or created_by:SOMEONE or name:SOME_NAME works, but not when someone just types Example list name which should filter by list name.

So initially thought that there was an issue with the way we were parsing the search term. Thanks to help from @FrankHassanabad found that the reason search was not working as expected was because the exception list property name is mapped as a keyword - this means it does not get tokenized which is why one word searches were working but if the name included multiple words and was partial, it was not filtering properly.

Solution

Switched to using EUI's SearchBar component which does the parsing of the text for us and updated the mappings for exception list name to include text.

Searching by multiple fields (these are AND-ed)

Screen Shot 2021-01-20 at 4 41 55 PM

Searching multiple words (these are OR-ed)

Screen Shot 2021-01-20 at 4 42 26 PM

Exact match

Screen Shot 2021-01-20 at 4 57 11 PM

Checklist

},
name: {
type: 'keyword',
fields: {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: Left the property type as is because exception list type is either detections or endpoint and I believe we use the standard ES tokenizer which tokenizes based on word boundaries.

@yctercero yctercero marked this pull request as ready for review February 3, 2021 06:29
@yctercero yctercero requested review from a team as code owners February 3, 2021 06:29
@elasticmachine
Copy link
Copy Markdown
Contributor

Pinging @elastic/security-solution (Team: SecuritySolution)

@elasticmachine
Copy link
Copy Markdown
Contributor

Pinging @elastic/security-detections-response (Team:Detections and Resp)

@elasticmachine
Copy link
Copy Markdown
Contributor

Pinging @elastic/siem (Team:SIEM)

@spong spong requested a review from a team February 3, 2021 16:08
@yctercero
Copy link
Copy Markdown
Contributor Author

@elasticmachine merge upstream


return { [defaultSearchTerm]: searchValue };
} catch {
return { [defaultSearchTerm]: searchValue };
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When does this try catch blow up? Is it because it's a reduce without a default value going on here? ;-)

If so, I rarely use reduce without a default value to avoid the situation altogether. As a maintainer this looks highly suspect if I don't know when/why this try/catch is going to be hit without comments.

I'll look at the unit tests next to be like, "Is there a unit test that expresses the try/catch so I can see if it's needed and why?"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the catch clause I added because of the query.ast.getFieldClauses() throws an error if the user attempts to search a term that is not defined in the schema like not_a_property:something.

Copy link
Copy Markdown
Contributor Author

@yctercero yctercero Feb 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But oof good catch on the reduce sans default value there. I'll update that and add a test to express when the try/catch blows.

EDIT: it does have the default value - filterOptions

@yctercero yctercero requested a review from a team as a code owner February 9, 2021 20:25
@yctercero
Copy link
Copy Markdown
Contributor Author

Known test failures - those won't pass till we figure out changes needed to SO _find filter parsing.


cy.get(EXCEPTIONS_TABLE_SHOWING_LISTS).should('have.text', `Showing 1 list`);
});
});
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

Copy link
Copy Markdown
Contributor

@FrankHassanabad FrankHassanabad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gave it a re-look, everything is great! 👍

@kibanamachine
Copy link
Copy Markdown
Contributor

💛 Build succeeded, but was flaky


Test Failures

Kibana Pipeline / general / "before all" hook for "should contain notes".Timeline notes tab "before all" hook for "should contain notes"

Link to Jenkins

Stack Trace

Failed Tests Reporter:
  - Test has not failed recently on tracked branches

AssertionError: Timed out retrying after 60000ms: Expected to find element: `[data-test-subj="title-70e80c10-6cb8-11eb-a026-413436d190c0"]`, but never found it.

Because this error occurred during a `before all` hook we are skipping the remaining tests in the current suite: `Timeline notes tab`

Although you have test retries enabled, we do not retry tests when `before all` or `after all` hooks fail
    at Object.openTimelineById (http://localhost:6111/__cypress/tests?p=cypress/integration/timelines/notes_tab.spec.ts:16007:15)
    at Context.eval (http://localhost:6111/__cypress/tests?p=cypress/integration/timelines/notes_tab.spec.ts:15041:24)

Kibana Pipeline / general / "after all" hook for "should contain notes".Timeline notes tab "after all" hook for "should contain notes"

Link to Jenkins

Stack Trace

Failed Tests Reporter:
  - Test has not failed recently on tracked branches

CypressError: `cy.filter()` failed because it requires a DOM element.

The subject received was:

  > `undefined`

The previous command that ran was:

  > `cy.get()`

All 2 subject validations failed on this subject.

Because this error occurred during a `after all` hook we are skipping the remaining tests in the current suite: `Timeline notes tab`

Although you have test retries enabled, we do not retry tests when `before all` or `after all` hooks fail
    at ensureElement (http://elastic:changeme@localhost:6111/__cypress/runner/cypress_runner.js:161322:24)
    at validateType (http://elastic:changeme@localhost:6111/__cypress/runner/cypress_runner.js:161159:16)
    at Object.ensureSubjectByType (http://elastic:changeme@localhost:6111/__cypress/runner/cypress_runner.js:161195:9)
    at pushSubjectAndValidate (http://elastic:changeme@localhost:6111/__cypress/runner/cypress_runner.js:169888:15)
    at Context.<anonymous> (http://elastic:changeme@localhost:6111/__cypress/runner/cypress_runner.js:170225:18)
From Your Spec Code:
    at Object.closeTimeline (http://localhost:6111/__cypress/tests?p=cypress/integration/timelines/notes_tab.spec.ts:15960:43)
    at Context.eval (http://localhost:6111/__cypress/tests?p=cypress/integration/timelines/notes_tab.spec.ts:15051:20)

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
securitySolution 2187 2188 +1

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
securitySolution 7.5MB 7.5MB +2.5KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
lists 142.7KB 142.9KB +206.0B

Saved Objects .kibana field count

Every field in each saved object type adds overhead to Elasticsearch. Kibana needs to keep the total field count below Elasticsearch's default limit of 1000 fields. Only specify field mappings for the fields you wish to search on or query. See https://www.elastic.co/guide/en/kibana/master/development-plugin-saved-objects.html#_mappings

id before after diff
exception-list 39 41 +2
exception-list-agnostic 39 41 +2
total +4

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@yctercero yctercero removed the v7.11.1 label Feb 11, 2021
@yctercero yctercero merged commit 6e44496 into elastic:master Feb 11, 2021
yctercero added a commit to yctercero/kibana that referenced this pull request Feb 11, 2021
…y name (elastic#88701)

Addresses elastic#88450

Issue
Search was not working as expected was because the exception list property name is mapped as a keyword - this means it does not get tokenized which is why one word searches were working but if the name included multiple words and was partial, it was not filtering properly.
yctercero added a commit that referenced this pull request Feb 12, 2021
…y name (#88701) (#91255)

Addresses #88450

Issue
Search was not working as expected was because the exception list property name is mapped as a keyword - this means it does not get tokenized which is why one word searches were working but if the name included multiple words and was partial, it was not filtering properly.
@yctercero yctercero deleted the exceptions_dejavu branch October 13, 2021 06:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release_note:enhancement Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:SIEM v7.12.0 v8.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants