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] Changes coverage overview subtechnique display to base off active filters #170988

Merged
merged 5 commits into from
Nov 14, 2023

Conversation

dplumlee
Copy link
Contributor

@dplumlee dplumlee commented Nov 9, 2023

Fixes: #170945

Summary

Changes our sub-technique display logic to no longer just represent the enabled rules, but all rules that fall under the current page filters - similar to our tile coloring logic.

Screenshots

When only enabled filter is active

When enabled and disabled filters are active

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@dplumlee dplumlee added release_note:skip Skip the PR/issue when compiling release notes Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Feature:Rule Management Security Solution Detection Rule Management area Team:Detection Rule Management Security Detection Rule Management Team v8.12.0 labels Nov 9, 2023
@dplumlee dplumlee self-assigned this Nov 9, 2023
@dplumlee dplumlee marked this pull request as ready for review November 10, 2023 16:08
@dplumlee dplumlee requested a review from a team as a code owner November 10, 2023 16:08
@dplumlee dplumlee requested a review from banderror November 10, 2023 16:08
@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

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

@dplumlee
Copy link
Contributor Author

@elasticmachine merge upstream

@banderror banderror added bug Fixes for quality problems that affect the customer experience release_note:fix v8.11.2 impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. and removed release_note:skip Skip the PR/issue when compiling release notes labels Nov 13, 2023
@dplumlee
Copy link
Contributor Author

@elasticmachine merge upstream

1 similar comment
@dplumlee
Copy link
Contributor Author

@elasticmachine merge upstream

Copy link
Contributor

@banderror banderror left a comment

Choose a reason for hiding this comment

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

Tested the PR locally and checked the use case from the #170945 ticket. All worked as expected.

Reviewed the changes and left a few comments that shouldn't block the PR from merging. Thank you @dplumlee for the quick fix!

Comment on lines 61 to 64
...getMockCoverageOverviewMitreTechnique(),
subtechniques: [
getMockCoverageOverviewMitreSubTechnique(),
{ ...getMockCoverageOverviewMitreSubTechnique(), id: 'test-id' },
Copy link
Contributor

Choose a reason for hiding this comment

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

I think these tests need some refactoring later. The functions that create mocks are not parameterized and return implicit data. This means that we set the frontend-side CoverageOverview model in these tests implicitly, while the assertions are explicit. It's hard to read such tests because you always need to jump to the mock functions implementation and compare it with the assertions. Also, it makes the tests setup too rigid and we're probably missing a few tests cases because of that. For example, the mock data makes the same rule both enabled and disabled at the same time, which is 1) incorrect and 2) we need to test more than one rule mapped to the same technique through 2 or more tactics, and also test different combinations of these rules being enabled and disabled.

I'm not suggesting to address this in this PR, but this is something to work on later when we get back to working on the next milestone for the Coverage epic.

@dplumlee dplumlee force-pushed the coverage-overview-subtechnique-filter branch from 45f4584 to 05b4fbc Compare November 14, 2023 17:52
@dplumlee dplumlee enabled auto-merge (squash) November 14, 2023 17:53
@dplumlee dplumlee merged commit 87ec144 into elastic:main Nov 14, 2023
@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] Security Solution Cypress Tests #11 / Use Value list in exception entry Should use value list in exception entry, and validate deleting value list prompt Should use value list in exception entry, and validate deleting value list prompt

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
securitySolution 4669 4670 +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 13.0MB 13.0MB +163.0B

History

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

cc @dplumlee

@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
8.11

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

@dplumlee dplumlee deleted the coverage-overview-subtechnique-filter branch November 15, 2023 00:03
janmonschke pushed a commit to janmonschke/kibana that referenced this pull request Nov 23, 2023
…play to base off active filters (elastic#170988) (elastic#171230)

# Backport

This will backport the following commits from `main` to `8.11`:
- [[Security Solution] Changes coverage overview subtechnique display to
base off active filters
(elastic#170988)](elastic#170988)

<!--- Backport version: 8.9.7 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Davis
Plumlee","email":"[email protected]"},"sourceCommit":{"committedDate":"2023-11-14T19:04:09Z","message":"[Security
Solution] Changes coverage overview subtechnique display to base off
active filters
(elastic#170988)","sha":"87ec1440dcc1d25938795925237ca5194ee6551e","branchLabelMapping":{"^v8.12.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["bug","release_note:fix","impact:medium","Team:Detections
and Resp","Team: SecuritySolution","Feature:Rule
Management","Team:Detection Rule
Management","v8.12.0","v8.11.2"],"number":170988,"url":"https://github.com/elastic/kibana/pull/170988","mergeCommit":{"message":"[Security
Solution] Changes coverage overview subtechnique display to base off
active filters
(elastic#170988)","sha":"87ec1440dcc1d25938795925237ca5194ee6551e"}},"sourceBranch":"main","suggestedTargetBranches":["8.11"],"targetPullRequestStates":[{"branch":"main","label":"v8.12.0","labelRegex":"^v8.12.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/170988","number":170988,"mergeCommit":{"message":"[Security
Solution] Changes coverage overview subtechnique display to base off
active filters
(elastic#170988)","sha":"87ec1440dcc1d25938795925237ca5194ee6551e"}},{"branch":"8.11","label":"v8.11.2","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"}]}]
BACKPORT-->

Co-authored-by: Davis Plumlee <[email protected]>
@ghost
Copy link

ghost commented Dec 7, 2023

Hi @dplumlee

we have validated this issue and found the issue to be fixed on 8.11.2 ✔️ .

Now rule details are showing as per the current page filter set.

Build details:

Version: 8.11.2
Commit: 92746356b61c3e3ac62b6d7045727f8d737fa4b5
Build:68299

Observations:

  • Page Filter : Only Enabled : O/P : Only Enabled rules details are showing
test.mp4
  • Page Filter: Only Disabled : O/P : Only Disabled rules details are showing
only_disabled.mp4
  • Page Filter : Enabled + Disabled : O/P : Both Enabled and Disabled rules details are showing
both.mp4

Hence we are adding "QA:Validated" tag to it.

thanks !!

@ghost ghost added the QA:Validated Issue has been validated by QA label Dec 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fixes for quality problems that affect the customer experience Feature:Rule Management Security Solution Detection Rule Management area impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. QA:Validated Issue has been validated by QA release_note:fix Team:Detection Rule Management Security Detection Rule Management Team Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. v8.11.2 v8.12.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Security Solution] Coverage Overview page: incorrect number of MITRE subtechniques
5 participants