[Security Solution] Disable ML rule's edit button link under basic license #143260

maximpn · 2022-10-13T08:45:49Z

Resolves: #139796

Summary

It disables ML rule's edit button link under the basic license.

Details

ML rules aren't available under the basic license but installable from the prebuilt rules. Having an active edit button makes the UX inconsistent. Disabling such a button under the basic license for ML rules improves UX though doesn't block a user from opening the rule editing page from the address bar.

Before:

Screen.Recording.2022-10-13.at.11.59.02.mov

After:

Screen.Recording.2022-10-13.at.11.56.25.mov

Checklist

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

elasticmachine · 2022-10-13T12:05:44Z

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

elasticmachine · 2022-10-13T12:05:46Z

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

xcrzx

Overall, the changes look good to me. Thanks for fixing this bug, @maximpn 👍
Added some minor suggestions to consider.

...k/plugins/security_solution/public/detections/pages/detection_engine/rules/details/index.tsx

maximpn · 2022-10-19T19:48:19Z

@elasticmachine merge upstream

maximpn · 2022-10-19T20:24:34Z

@elasticmachine merge upstream

xcrzx

Sorry, I had to add a couple more comments 🙂
Thanks for addressing my previous ones, BTW!

xcrzx · 2022-10-20T09:02:32Z

...plugins/security_solution/public/detections/pages/detection_engine/rules/all/use_columns.tsx

@@ -68,7 +68,7 @@ const useEnabledColumn = ({ hasPermissions }: ColumnsProps): TableColumn => {
      render: (_, rule: Rule) => (
        <EuiToolTip
          position="top"
-          content={getToolTipContent(rule, hasMlPermissions, hasActionsPrivileges)}
+          content={getToolTipContent(rule, hasMlPermissions, hasActionsPrivileges, true)}


Could we pass an actual canUserCRUD value here instead of the hardcoded true?

xcrzx · 2022-10-20T09:02:41Z

...plugins/security_solution/public/detections/components/rules/rule_actions_overflow/index.tsx

@@ -96,7 +96,7 @@ const RuleActionsOverflowComponent = ({
            >
              <EuiToolTip
                position="left"
-                content={getToolTipContent(rule, true, canDuplicateRuleWithActions)}
+                content={getToolTipContent(rule, true, canDuplicateRuleWithActions, true)}


Could we pass an actual canUserCRUD value here instead of the hardcoded true?

getToolTipContent() is used as a utility function here. It can be simplified though full privileges refactoring will be much better.

xcrzx · 2022-10-20T09:04:03Z

...etections/pages/detection_engine/rules/details/components/edit_rule_settings_button_link.tsx

+      ev.preventDefault();
+      navigateToApp(APP_UI_ID, {
+        deepLinkId: SecurityPageName.rules,
+        path: getEditRuleUrl(ruleId ?? ''),


Nit: ruleId is not nullable.

xcrzx · 2022-10-20T09:04:08Z

...etections/pages/detection_engine/rules/details/components/edit_rule_settings_button_link.tsx

+        iconType="controlsHorizontal"
+        isDisabled={disabled}
+        deepLinkId={SecurityPageName.rules}
+        path={getEditRuleUrl(ruleId ?? '')}


Nit: ruleId is not nullable.

xcrzx · 2022-10-20T09:11:20Z

x-pack/plugins/security_solution/public/common/utils/privileges/index.ts

 ): string | undefined => {
  if (rule == null) {
    return undefined;
  } else if (isMlRule(rule.type) && !hasMlPermissions) {
    return detectionI18n.ML_RULES_DISABLED_MESSAGE;
  } else if (!canEditRuleWithActions(rule, hasReadActionsPrivileges)) {
-    return i18n.EDIT_RULE_SETTINGS_TOOLTIP;
+    return i18n.LACK_OF_KIBANA_PRIVILEGES;
+  } else if (canUserCRUD !== null && !canUserCRUD) {


Let's use the userHasPermissions(canUserCRUD) helper here.

xcrzx · 2022-10-20T09:19:31Z

x-pack/plugins/security_solution/public/detections/pages/detection_engine/rules/translations.ts

@@ -484,13 +484,20 @@ export const EDIT_RULE_SETTINGS = i18n.translate(
  }
 );

-export const EDIT_RULE_SETTINGS_TOOLTIP = i18n.translate(
-  'xpack.securitySolution.detectionEngine.rules.allRules.actions.editRuleSettingsToolTip',
+export const LACK_OF_KIBANA_PRIVILEGES = i18n.translate(


Thanks for renaming this constant! For more clarity, I'd propose to name it more specific, like LACK_OF_KIBANA_ACTIONS_FEATURE_PRIVILEGES, as Kibana privileges is a broader term that includes many different features, including actions, security, etc.

sounds good, thanks

xcrzx · 2022-10-20T09:20:12Z

x-pack/plugins/security_solution/public/detections/pages/detection_engine/rules/translations.ts

  {
    defaultMessage: 'You do not have Kibana Actions privileges',
  }
 );

+export const LACK_OF_RULE_EDITING_PRIVILEGES = i18n.translate(


LACK_OF_KIBANA_SECURITY_FEATURE_PRIVILEGES

xcrzx · 2022-10-20T09:23:17Z

x-pack/plugins/security_solution/public/detections/pages/detection_engine/rules/translations.ts

+export const LACK_OF_RULE_EDITING_PRIVILEGES = i18n.translate(
+  'xpack.securitySolution.detectionEngine.rules.allRules.actions.lackOfRuleEditingPrivileges',
+  {
+    defaultMessage: 'You do not have rule editing privileges',


It would be better to rephrase this line like "You do not have Kibana Security privileges" for consistency with the documentation and the Kibana feature privileges UI.

sounds good, thanks

xcrzx

LGTM, thanks for addressing my comments 👍

…-fix'

maximpn · 2022-10-21T07:07:58Z

@elasticmachine merge upstream

kibana-ci · 2022-10-21T08:15:27Z

💚 Build Succeeded

Buildkite Build
Commit: 477f6cc

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`securitySolution`	3201	3202	+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`	10.0MB	10.0MB	+855.0B

History

💔 Build #81873 failed d54856d
💔 Build #81834 failed 527c76f
💔 Build #81815 failed 60db907
💚 Build #81596 succeeded 1746361
💔 Build #81581 failed c08eb5a

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

cc @maximpn

maximpn · 2022-10-21T08:49:57Z

@banderror I've created an issue #143812 to address your comment.

kibanamachine · 2022-10-21T08:54:10Z

💔 All backports failed

Status	Branch	Result
❌	8.5	Backport failed because of merge conflicts

Manual backport

To create the backport manually run:

node scripts/backport --pr 143260

Questions ?

Please refer to the Backport tool documentation

banderror · 2022-10-21T10:01:45Z

@maximpn you shouldn't backport it to 8.5, it's too late.

* main: (57 commits) [Files] Filepicker (elastic#143111) [Infrastructure UI] Replace Lens table with EUI table and own api (elastic#142871) [api-docs] Daily api_docs build (elastic#143829) [api-docs] Daily api_docs build (elastic#143825) [api-docs] Daily api_docs build (elastic#143823) [Security Solution] Restructuring folders of Detection Engine + refactoring Rule Management (elastic#142950) [Dev tools] Fix performance issue with autocomplete suggestions (elastic#143428) [Security Solution] Disable ML rule's edit button link under basic license (elastic#143260) [Lens] Use the language-documentation package for formula (elastic#143649) [api-docs] Daily api_docs build (elastic#143811) [Security Solution] Fix missing title on inspect pop-up (elastic#143601) fix incorrect filters being passed to events table causing duplicate entries in our inpsect tool request tab (elastic#143239) [Security Solution][Endpoint] `get-file` response action kibana download file API (elastic#143708) Rely on refresh context to update stats independently of overview cards. (elastic#143308) [RAM] Rule event log - Fix incorrect results when filtering by message and outcome simultaneously (elastic#143119) [ML] Display link to create data view from error cases in data frame analytics results pages (elastic#143596) Update links in README :) (elastic#143675) Add more tests for ml_inference_logic (elastic#143764) skip failing test suite (elastic#143717) [DOCS] Add assignees to case APIs (elastic#143610) ...

…cense (elastic#143260) **Resolves:** [elastic#139796](elastic#139796) ## Summary It disables ML rule's edit button link under the basic license. ## Details ML rules aren't available under the basic license but installable from the prebuilt rules. Having an active edit button makes the UX inconsistent. Disabling such a button under the basic license for ML rules improves UX though doesn't block a user from opening the rule editing page from the address bar. Before: https://user-images.githubusercontent.com/3775283/195552179-525f0423-3a62-4ab5-b1ef-0f5cafe2286e.mov After: https://user-images.githubusercontent.com/3775283/195551540-b95fabeb-4e50-4a26-ae42-1a72f53573dc.mov ### Checklist - [ ] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md) - [x] Any UI touched in this PR does not create any new axe failures (run axe in browser: [FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/), [Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US)) - [x] This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server)) - [x] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers)

maximpn · 2022-11-07T16:58:17Z

💚 All backports created successfully

Status	Branch	Result
✅	8.5

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

Questions ?

Please refer to the Backport tool documentation

…cense (elastic#143260) **Resolves:** [elastic#139796](elastic#139796) ## Summary It disables ML rule's edit button link under the basic license. ## Details ML rules aren't available under the basic license but installable from the prebuilt rules. Having an active edit button makes the UX inconsistent. Disabling such a button under the basic license for ML rules improves UX though doesn't block a user from opening the rule editing page from the address bar. Before: https://user-images.githubusercontent.com/3775283/195552179-525f0423-3a62-4ab5-b1ef-0f5cafe2286e.mov After: https://user-images.githubusercontent.com/3775283/195551540-b95fabeb-4e50-4a26-ae42-1a72f53573dc.mov ### Checklist - [ ] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md) - [x] Any UI touched in this PR does not create any new axe failures (run axe in browser: [FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/), [Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US)) - [x] This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server)) - [x] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers) (cherry picked from commit a670c7f) # Conflicts: # x-pack/plugins/security_solution/public/detections/pages/detection_engine/rules/details/index.tsx

…cense (#143260) (#144723) **Resolves:** [#139796](#139796) ## Summary It disables ML rule's edit button link under the basic license. ## Details ML rules aren't available under the basic license but installable from the prebuilt rules. Having an active edit button makes the UX inconsistent. Disabling such a button under the basic license for ML rules improves UX though doesn't block a user from opening the rule editing page from the address bar. Before: https://user-images.githubusercontent.com/3775283/195552179-525f0423-3a62-4ab5-b1ef-0f5cafe2286e.mov After: https://user-images.githubusercontent.com/3775283/195551540-b95fabeb-4e50-4a26-ae42-1a72f53573dc.mov ### Checklist - [ ] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md) - [x] Any UI touched in this PR does not create any new axe failures (run axe in browser: [FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/), [Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US)) - [x] This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server)) - [x] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers) (cherry picked from commit a670c7f) # Conflicts: # x-pack/plugins/security_solution/public/detections/pages/detection_engine/rules/details/index.tsx

maximpn self-assigned this Oct 13, 2022

maximpn marked this pull request as ready for review October 13, 2022 12:05

maximpn requested a review from a team as a code owner October 13, 2022 12:05

maximpn requested a review from xcrzx October 13, 2022 12:05

xcrzx reviewed Oct 18, 2022

View reviewed changes

maximpn force-pushed the disable-ml-rule-edit-button-for-basic-license branch from 8564c29 to d42c47c Compare October 19, 2022 12:48

maximpn requested a review from a team as a code owner October 19, 2022 13:25

maximpn added 4 commits October 19, 2022 16:29

Disable ML rule's edit button link under basic license

a8dcef7

extract edit rule button link functionality to a separate component

69bb2ca

fix eslint errors

8d21efb

show user lacks of CRUD privileges message when necessary

83b355e

maximpn force-pushed the disable-ml-rule-edit-button-for-basic-license branch from 9ba2931 to 83b355e Compare October 19, 2022 13:29

maximpn added 2 commits October 19, 2022 21:16

fix checks failures

8a9ab6d

fix rename the i18n key

7230f9f

maximpn force-pushed the disable-ml-rule-edit-button-for-basic-license branch from 0f94dcc to 7230f9f Compare October 19, 2022 19:37

remove unused import

70c3a51

maximpn force-pushed the disable-ml-rule-edit-button-for-basic-license branch from 598af98 to 70c3a51 Compare October 19, 2022 20:24

xcrzx reviewed Oct 20, 2022

View reviewed changes

maximpn added 5 commits October 20, 2022 17:52

rename i18n import

63ae9af

simplify duplicate rule restriction message

6311e32

get rid of unnecessary coalescing operators

c070045

rename a translation key

b0a39a3

rename a translation key

60db907

maximpn force-pushed the disable-ml-rule-edit-button-for-basic-license branch from 1746361 to 60db907 Compare October 20, 2022 15:14

xcrzx approved these changes Oct 20, 2022

View reviewed changes

update outdated imports

41403c4

maximpn force-pushed the disable-ml-rule-edit-button-for-basic-license branch from 527c76f to 41403c4 Compare October 20, 2022 17:13

[CI] Auto-commit changed files from 'node scripts/eslint --no-cache -…

d54856d

…-fix'

Merge branch 'main' into disable-ml-rule-edit-button-for-basic-license

477f6cc

maximpn mentioned this pull request Oct 21, 2022

[Security Solution] Write e2e tests for how the edit rule button and the enable/disable toggle work for ML rules under different licenses and user privileges #143812

Open

maximpn merged commit a670c7f into elastic:main Oct 21, 2022

banderror removed the backport:prev-minor Backport to the previous minor version (i.e. one version back from main) label Oct 21, 2022

kibanamachine added the backport:skip This commit does not require backporting label Oct 21, 2022

banderror mentioned this pull request Oct 21, 2022

[Security Solution] Edit rule button is enable to ML rule on basic license #139796

Closed

maximpn deleted the disable-ml-rule-edit-button-for-basic-license branch October 24, 2022 05:35

banderror mentioned this pull request Oct 25, 2022

[Security Solution] Support for running Cypress e2e tests under different licenses #143932

Open

maximpn mentioned this pull request Nov 7, 2022

[8.5] [Security Solution] Disable ML rule's edit button link under basic license (#143260) #144723

Merged

kibanamachine added the v8.5.1 label Nov 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solution] Disable ML rule's edit button link under basic license #143260

[Security Solution] Disable ML rule's edit button link under basic license #143260

maximpn commented Oct 13, 2022 •

edited by kibanamachine

Loading

elasticmachine commented Oct 13, 2022

elasticmachine commented Oct 13, 2022

xcrzx left a comment

maximpn commented Oct 19, 2022

maximpn commented Oct 19, 2022

xcrzx left a comment

xcrzx Oct 20, 2022

maximpn Oct 20, 2022

xcrzx Oct 20, 2022

maximpn Oct 20, 2022

xcrzx Oct 20, 2022

maximpn Oct 20, 2022

xcrzx Oct 20, 2022

maximpn Oct 20, 2022

xcrzx Oct 20, 2022

xcrzx Oct 20, 2022

maximpn Oct 20, 2022

xcrzx Oct 20, 2022

maximpn Oct 20, 2022

xcrzx Oct 20, 2022

maximpn Oct 20, 2022

xcrzx left a comment

maximpn commented Oct 21, 2022

kibana-ci commented Oct 21, 2022

maximpn commented Oct 21, 2022

kibanamachine commented Oct 21, 2022

banderror commented Oct 21, 2022

maximpn commented Nov 7, 2022

[Security Solution] Disable ML rule's edit button link under basic license #143260

[Security Solution] Disable ML rule's edit button link under basic license #143260

Conversation

maximpn commented Oct 13, 2022 • edited by kibanamachine Loading

Summary

Details

Checklist

elasticmachine commented Oct 13, 2022

elasticmachine commented Oct 13, 2022

xcrzx left a comment

Choose a reason for hiding this comment

maximpn commented Oct 19, 2022

maximpn commented Oct 19, 2022

xcrzx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xcrzx left a comment

Choose a reason for hiding this comment

maximpn commented Oct 21, 2022

kibana-ci commented Oct 21, 2022

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

History

maximpn commented Oct 21, 2022

kibanamachine commented Oct 21, 2022

💔 All backports failed

Manual backport

Questions ?

banderror commented Oct 21, 2022

maximpn commented Nov 7, 2022

💚 All backports created successfully

Questions ?

maximpn commented Oct 13, 2022 •

edited by kibanamachine

Loading