[Security Solution] Coverage Overview follow-up #164613

dplumlee · 2023-08-23T17:14:44Z

Fixes: #164536
Fixes: #164816
Fixes: #164866

Summary

As well as some other housekeeping items like color and text changes for overall usability and design.

Now populates initial filter of enabled rules and all rule source types
Works in dark mode
Moves link to rules tab instead of dashboards
Changes tile coloring method

Screenshots

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios

For maintainers

This was checked for breaking API changes and was labeled appropriately

elasticmachine · 2023-08-24T14:21:01Z

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

elasticmachine · 2023-08-24T14:21:03Z

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

machadoum

threat-hunting-explore changs LGTM!

banderror · 2023-08-25T14:16:57Z

I started testing the PR, so far:

found a bug that seems to be unrelated to these changes: [Security Solution] Coverage Overview page: console errors when changing a filter #164846
opened a performance improvement ticket: [Security Solution] Coverage Overview page: optimize rendering performance #164853

Will be updating this comment with more findings during review and testing.

kibana-ci · 2023-08-25T14:47:09Z

💚 Build Succeeded

Buildkite Build
Commit: e76f800

Metrics [docs]

Async chunks

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

id	before	after	diff
`securitySolution`	15.7MB	15.7MB	+1.0KB

History

💛 Build #152986 was flaky 563331b1c692063eb47a8e77b2a11592692010b8
💔 Build #152877 failed c8879c4369aa4ab775291a4563388f3397702f12

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

cc @dplumlee

banderror

Reviewed the diff and tested the app locally. Can confirm that all the fixes work as expected:

The "enable rules" button is disabled for read-only users
The page now has the initial filter of enabled rules and all rule source types
It looks good in both dark and normal modes, the tile coloring corresponds to the design
The Nav menu link is now under the "Rules" menu and on the corresponding page, it's not under "Dashboards" anymore

The changes look good, I left a few minor comments but don't want to block the PR from being merged. Feel free to address them in a follow-up PR.

In a comment above I also left a couple of links to the tickets I opened while testing. We will chat if we should include them in the 8.11 scope.

Overall, great PR, thank you @dplumlee for all your efforts! I also took a brief look at the whole implementation and I think it's well done, so a separate thank you for that 👍

banderror · 2023-08-25T16:35:24Z

...ity_solution/public/detection_engine/rule_management_ui/pages/coverage_overview/constants.ts

 import {
  CoverageOverviewRuleActivity,
  CoverageOverviewRuleSource,
 } from '../../../../../common/api/detection_engine';
 import * as i18n from './translations';

-export const coverageOverviewPaletteColors = euiPalettePositive(5);
+export const coverageOverviewPaletteColors = ['#00BFB326', '#00BFB34D', '#00BFB399', '#00BFB3'];


Nit: why do we need this as a separate array and why it is exported? It looks like magic numbers in an array.

I think this would be sufficient:

/** * Rules count -> color map * * A corresponding color is applied if rules count >= a specific threshold */ export const coverageOverviewCardColorThresholds = [ { threshold: 10, color: '#00BFB3' }, { threshold: 7, color: '#00BFB399' }, { threshold: 3, color: '#00BFB34D' }, { threshold: 1, color: '#00BFB326' }, ];

banderror · 2023-08-25T16:38:24Z

.../detection_engine/rule_management_ui/pages/coverage_overview/coverage_overview_dashboard.tsx

+  const subtitle = (
+    <EuiText color="subdued" size="s">
+      <span>{i18n.CoverageOverviewDashboardInformation}</span>{' '}
+      <EuiLink
+        external={true}
+        href={'https://www.elastic.co/'} // TODO: change to actual docs link before release
+        rel="noopener noreferrer"
+        target="_blank"
+      >
+        {i18n.CoverageOverviewDashboardInformationLink}
+      </EuiLink>
+    </EuiText>
+  );


Let's extract this to a separate component and memoize it. It's a static component w/o parameters that should be rendered only once.

banderror · 2023-08-25T16:45:52Z

.../detection_engine/rule_management_ui/pages/coverage_overview/coverage_overview_dashboard.tsx

+      <span>{i18n.CoverageOverviewDashboardInformation}</span>{' '}
+      <EuiLink
+        external={true}
+        href={'https://www.elastic.co/'} // TODO: change to actual docs link before release


I think it's time to ask the @elastic/security-docs team more actively to provide the URL. Meanwhile, let's change the link to a more specific one, such as https://www.elastic.co/guide/en/security/current/index.html.

Also, let's render the link itself using x-pack/plugins/security_solution/public/common/components/links_to_docs/links_components.tsx - we'd need to add a new link component to this file.

Also, there's packages/kbn-doc-links/src/get_doc_links.ts but I'm not sure if we've ever used it.

banderror · 2023-08-25T17:04:19Z

...urity_solution/public/detection_engine/rule_management_ui/pages/coverage_overview/helpers.ts

+export const getTotalRuleCount = (
+  technique: CoverageOverviewMitreTechnique,
+  activity?: CoverageOverviewRuleActivity[]
+): number => {
+  if (!activity) {
+    return technique.enabledRules.length + technique.disabledRules.length;
+  }
+  let totalRuleCount = 0;
+  if (activity.includes(CoverageOverviewRuleActivity.Enabled)) {
+    totalRuleCount += technique.enabledRules.length;
+  }
+  if (activity.includes(CoverageOverviewRuleActivity.Disabled)) {
+    totalRuleCount += technique.disabledRules.length;
+  }
+  return totalRuleCount;
+};


This is actually our domain model, it's not just a helper. It's a primitive calculation on CoverageOverviewMitreTechnique, or you could say its "behavior". Since it's highly coupled with CoverageOverviewMitreTechnique, it should be placed closer to this type under x-pack/plugins/security_solution/public/detection_engine/rule_management/model/coverage_overview, or even to the same file.

banderror · 2023-08-25T17:22:45Z

..._solution/public/detection_engine/rule_management_ui/pages/coverage_overview/translations.ts

+export const CoverageOverviewDashboardInformationLink = i18n.translate(
+  'xpack.securitySolution.coverageOverviewDashboard.dashboardInformationLink',
+  {
+    defaultMessage: 'docs.',


Nit: We could reword the sentence so that we don't have to include punctuation in our localization dictionary.

dplumlee · 2023-08-25T17:56:28Z

@banderror these are good comments, since we're doing a follow-up anyways for the column order I can address them there just to get this merged today 👍

(cherry picked from commit 168e3dc)

kibanamachine · 2023-08-25T18:03:55Z

💚 All backports created successfully

Status	Branch	Result
✅	8.10

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

Questions ?

Please refer to the Backport tool documentation

…4885) # Backport This will backport the following commits from `main` to `8.10`: - [[Security Solution] Coverage Overview follow-up (#164613)](#164613)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  Co-authored-by: Davis Plumlee <[email protected]>

joepeeples · 2023-08-25T21:03:32Z

@dplumlee My UI copy suggestions (note the title has lowercase "coverage" for consistency with other pages in the Rules section of the app, which follow sentence case):

MITRE ATT&CK® coverage
Your current coverage of MITRE ATT&CK® tactics and techniques, based on installed rules. Click a cell to view and enable a technique’s rules. Rules must be mapped to the MITRE ATT&CK® framework to be displayed. Learn more (link).

For the doc link's target, I just created a stub page at https://www.elastic.co/guide/en/security/master/rules-coverage.html.

dplumlee self-assigned this Aug 23, 2023

banderror added bug Fixes for quality problems that affect the customer experience Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. labels Aug 23, 2023

dplumlee mentioned this pull request Aug 24, 2023

[DOCS] Document the new MITRE ATT&CK Coverage page elastic/security-docs#3797

Closed

dplumlee marked this pull request as ready for review August 24, 2023 14:20

dplumlee requested review from a team as code owners August 24, 2023 14:20

dplumlee requested review from spong and maximpn August 24, 2023 14:20

banderror removed the request for review from spong August 24, 2023 18:22

machadoum approved these changes Aug 25, 2023

View reviewed changes

dplumlee added 2 commits August 25, 2023 15:37

housekeeping items

d572023

removes dashboard link and fixes tests

e76f800

banderror force-pushed the coverage-overview-follow-up branch from 563331b to e76f800 Compare August 25, 2023 13:37

This was referenced Aug 25, 2023

[Security Solution]Mitre Att&CK Coverage Dashboard search bar filtering not working #164816

Closed

[Security Solution] Some MITRE ATT&CK cards cannot be read in dark mode #164866

Closed

banderror approved these changes Aug 25, 2023

View reviewed changes

dplumlee merged commit 168e3dc into elastic:main Aug 25, 2023

kibanamachine mentioned this pull request Aug 25, 2023

[8.10] [Security Solution] Coverage Overview follow-up (#164613) #164885

Merged

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Aug 25, 2023

[Security Solution] Coverage Overview follow-up (elastic#164613)

1b48064

(cherry picked from commit 168e3dc)

dplumlee deleted the coverage-overview-follow-up branch August 25, 2023 18:07

banderror mentioned this pull request Aug 25, 2023

[Security Solution] Enable all disabled button is displayed as enabled for a user with read authorization #164536

Closed

dplumlee mentioned this pull request Aug 28, 2023

[Security Solution] Coverage Overview follow-up 2 #164986

Merged

3 tasks

banderror added release_note:feature Makes this part of the condensed release notes and removed release_note:skip Skip the PR/issue when compiling release notes labels Sep 6, 2023

banderror mentioned this pull request Sep 6, 2023

8.10.0 Release notes elastic/security-docs#3781

Merged

joepeeples mentioned this pull request Sep 11, 2023

[UX Copy] Security Mitre Attack detection coverage page. elastic/security-docs#3495

Closed

This was referenced Oct 1, 2023

[8.10] fix(slo): Handle partial indicator url state (#167247) #167722

Merged

[8.10] backport SLO fixes #168231

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solution] Coverage Overview follow-up #164613

[Security Solution] Coverage Overview follow-up #164613

dplumlee commented Aug 23, 2023 •

edited by kibanamachine

Loading

elasticmachine commented Aug 24, 2023

elasticmachine commented Aug 24, 2023

machadoum left a comment

banderror commented Aug 25, 2023 •

edited

Loading

kibana-ci commented Aug 25, 2023

banderror left a comment

banderror Aug 25, 2023

banderror Aug 25, 2023

banderror Aug 25, 2023

banderror Aug 25, 2023

banderror Aug 25, 2023

dplumlee commented Aug 25, 2023

kibanamachine commented Aug 25, 2023

joepeeples commented Aug 25, 2023

[Security Solution] Coverage Overview follow-up #164613

[Security Solution] Coverage Overview follow-up #164613

Conversation

dplumlee commented Aug 23, 2023 • edited by kibanamachine Loading

Summary

Screenshots

Checklist

For maintainers

elasticmachine commented Aug 24, 2023

elasticmachine commented Aug 24, 2023

machadoum left a comment

Choose a reason for hiding this comment

banderror commented Aug 25, 2023 • edited Loading

kibana-ci commented Aug 25, 2023

💚 Build Succeeded

Metrics [docs]

Async chunks

History

banderror left a comment

Choose a reason for hiding this comment

banderror Aug 25, 2023

Choose a reason for hiding this comment

banderror Aug 25, 2023

Choose a reason for hiding this comment

banderror Aug 25, 2023

Choose a reason for hiding this comment

banderror Aug 25, 2023

Choose a reason for hiding this comment

banderror Aug 25, 2023

Choose a reason for hiding this comment

dplumlee commented Aug 25, 2023

kibanamachine commented Aug 25, 2023

💚 All backports created successfully

Questions ?

joepeeples commented Aug 25, 2023

dplumlee commented Aug 23, 2023 •

edited by kibanamachine

Loading

banderror commented Aug 25, 2023 •

edited

Loading