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] Disable ML rule's edit button link under basic license #143260

Merged

Conversation

maximpn
Copy link
Contributor

@maximpn maximpn commented Oct 13, 2022

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

@maximpn maximpn self-assigned this Oct 13, 2022
@maximpn maximpn added bug Fixes for quality problems that affect the customer experience Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:Detection Rule Management Security Detection Rule Management Team Feature:Rule Details Security Solution Detection Rule Details page release_note:fix backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) v8.6.0 labels Oct 13, 2022
@maximpn maximpn marked this pull request as ready for review October 13, 2022 12:05
@maximpn maximpn requested a review from a team as a code owner October 13, 2022 12:05
@maximpn maximpn requested a review from xcrzx October 13, 2022 12:05
@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

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

Copy link
Contributor

@xcrzx xcrzx left a comment

Choose a reason for hiding this comment

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

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

@maximpn maximpn force-pushed the disable-ml-rule-edit-button-for-basic-license branch from 8564c29 to d42c47c Compare October 19, 2022 12:48
@maximpn maximpn requested a review from a team as a code owner October 19, 2022 13:25
@maximpn maximpn force-pushed the disable-ml-rule-edit-button-for-basic-license branch from 9ba2931 to 83b355e Compare October 19, 2022 13:29
@maximpn maximpn force-pushed the disable-ml-rule-edit-button-for-basic-license branch from 0f94dcc to 7230f9f Compare October 19, 2022 19:37
@maximpn
Copy link
Contributor Author

maximpn commented Oct 19, 2022

@elasticmachine merge upstream

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

maximpn commented Oct 19, 2022

@elasticmachine merge upstream

Copy link
Contributor

@xcrzx xcrzx left a comment

Choose a reason for hiding this comment

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

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

@@ -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)}
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

Nit: ruleId is not nullable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks

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

Choose a reason for hiding this comment

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

Nit: ruleId is not nullable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks

): 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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

@@ -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(
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sounds good, thanks

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

export const LACK_OF_RULE_EDITING_PRIVILEGES = i18n.translate(
Copy link
Contributor

Choose a reason for hiding this comment

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

LACK_OF_KIBANA_SECURITY_FEATURE_PRIVILEGES

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks

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

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sounds good, thanks

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

@xcrzx xcrzx left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for addressing my comments 👍

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

maximpn commented Oct 21, 2022

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

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

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

cc @maximpn

@maximpn
Copy link
Contributor Author

maximpn commented Oct 21, 2022

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

@maximpn maximpn merged commit a670c7f into elastic:main Oct 21, 2022
@kibanamachine
Copy link
Contributor

💔 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 banderror removed the backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) label Oct 21, 2022
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Oct 21, 2022
@banderror
Copy link
Contributor

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

@maximpn maximpn deleted the disable-ml-rule-edit-button-for-basic-license branch October 24, 2022 05:35
jloleysens added a commit to jloleysens/kibana that referenced this pull request Oct 24, 2022
* 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)
  ...
@maximpn
Copy link
Contributor Author

maximpn commented Nov 7, 2022

💚 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

maximpn added a commit to maximpn/kibana that referenced this pull request Nov 7, 2022
…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
maximpn added a commit that referenced this pull request Nov 7, 2022
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting bug Fixes for quality problems that affect the customer experience Feature:Rule Details Security Solution Detection Rule Details page 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.5.1 v8.6.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants