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

[SIEM] Fixes UX issues around prebuilt ML Rules #62396

Merged
merged 7 commits into from
Apr 6, 2020

Conversation

spong
Copy link
Member

@spong spong commented Apr 3, 2020

Summary

This PR fixes a number of UX issues around the new prebuilt machine_learning rules when the user does not have the necessary permissions to manage the backing ML Job. Along with #62383, this ensures there is adequate information for the user determine if a rule is not working because the backing job is not running (and helping to prevent this from occurring). This also includes some requested copy changes, including:

  • Renames Anomaly Detection dropdown to ML job settings

  • Updates copy in ML job settings dropdown

  • Only shows ML job settings UI when on /detections/ routes

All Rules Changes

  • Disables the activate switch if user does not have permission to enable/disable jobs

  • Adds warning toast when attempting to activate via bulk actions (if user does not have permission to enable/disable jobs)

Rule Details Changes

  • Machine Learning job link now links to ML App with table filtered to the relevant job

  • Disables the activate switch if user does not have permission to enable/disable jobs

Create/Edit Rule Changes

  • If the job selected is not running, a warning will be displayed to remind the user to enable the job before running the rule. cc @benskelker @MikePaquette -- this okay copy here?

Resolves https://github.com/elastic/siem-team/issues/575
Resolves https://github.com/elastic/siem-team/issues/519

Checklist

Delete any items that are not applicable to this PR.

@spong spong added bug Fixes for quality problems that affect the customer experience Team:SIEM v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.7.0 v7.8.0 labels Apr 3, 2020
@spong spong requested a review from a team as a code owner April 3, 2020 03:42
@spong spong self-assigned this Apr 3, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/siem (Team:SIEM)

Copy link

@MikePaquette MikePaquette left a comment

Choose a reason for hiding this comment

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

great fix @spong, I've suggested a couple minor changes to the default message text to be more consistent with the ML job settings and rule activation.
image
and
image

@FrankHassanabad
Copy link
Contributor

FrankHassanabad commented Apr 3, 2020

You're fine if you want to stop users without privileges from turning off a ML rule I would assume, but the danger here is that if they run out of a trial license they can't turn it off.

But...They can still delete it. Up to you and UI/UX on what to do, just pointing out that the permissions are a bit funny that you can still delete it ;-) but you can't turn it off. Just mentioning.

Screen Shot 2020-04-03 at 1 00 46 PM

I would probably make it so they can disable it if it's already enabled and they have run out of their license if that's not too tricky.

Also, you can export and enable it and then import it and that will turn it on but very doubtful anyone would do that. Just mentioning that corner case.

@rylnd
Copy link
Contributor

rylnd commented Apr 3, 2020

Also, you can export and enable it and then import it and that will turn it on but very doubtful anyone would do that. Just mentioning that corner case.

@FrankHassanabad as of #61023 that shouldn't be possible; we prevent ML Rules from being imported if there's an insufficient license.

Edit: If the license is still valid, an ML non-admin could perform the export/import trick, and API users could also create/update/activate ML Rules irrespective of their ML capabilities. #62532

Copy link
Contributor

@rylnd rylnd left a comment

Choose a reason for hiding this comment

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

Couple of questions, but please use isMlRule instead of checking against our magic string.

Copy link
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.

Thanks for taking all the feedback and the extra energy on all the tests.

This is a great example of balancing between deadlines, real world code written by several people, tech debt, and taking in feedback.

All in a day's worth of work!

Everyone appreciates it.

LgTm :-)

@spong
Copy link
Member Author

spong commented Apr 6, 2020

@elasticmachine merge upstream

<EuiToolTip
position="top"
content={
rule?.type === 'machine_learning' && !hasMlPermissions
Copy link
Contributor

Choose a reason for hiding this comment

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

Could use isMlRule here but we can fix that next time we're here.

Copy link
Member Author

Choose a reason for hiding this comment

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

I had left that one and instead added the RuleDetails component to our list of tech debt since there is so much branching on null in that file. It's due for some TLC... 😅

Copy link
Contributor

@rylnd rylnd left a comment

Choose a reason for hiding this comment

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

Newest copy looks good, thanks!

@spong spong merged commit 0bdcda8 into elastic:master Apr 6, 2020
@spong spong deleted the ml-job-start-fixes branch April 6, 2020 19:44
spong added a commit to spong/kibana that referenced this pull request Apr 6, 2020
## Summary

This PR fixes a number of UX issues around the new prebuilt `machine_learning` rules when the user does not have the necessary permissions to manage the backing ML Job. Along with elastic#62383, this ensures there is adequate information for the user determine if a rule is not working because the backing job is not running (and helping to prevent this from occurring). This also includes some requested copy changes, including:

* Renames `Anomaly Detection`  dropdown to `ML job settings`
<p align="center">
  <img width="500" src="https://user-images.githubusercontent.com/2946766/78320279-57c5a880-7526-11ea-8350-647cbba263a4.png" />
</p>

* Updates copy in `ML job settings` dropdown
<p align="center">
  <img width="500" src="https://user-images.githubusercontent.com/2946766/78320473-cc98e280-7526-11ea-8871-e97661ff5f78.png" />
</p>

* Only shows `ML job settings` UI when on `/detections/` routes 
<p align="center">
  <img width="500" src="https://user-images.githubusercontent.com/2946766/78320401-922f4580-7526-11ea-9f97-0ec06526b273.png" />
</p>


### All Rules Changes

* Disables the `activate switch` if user does not have permission to enable/disable jobs
<p align="center">
  <img width="500" src="https://user-images.githubusercontent.com/2946766/78320892-d3742500-7527-11ea-90bb-91fd203480bd.png" />
</p>

* Adds warning toast when attempting to activate via bulk actions (if user does not have permission to enable/disable jobs)
<p align="center">
  <img width="300" src="https://user-images.githubusercontent.com/2946766/78321015-1a621a80-7528-11ea-8ab0-f9fef19240f7.png" />
</p>

### Rule Details Changes
* `Machine Learning job` link now links to ML App with table filtered to the relevant job

* Disables the `activate switch` if user does not have permission to enable/disable jobs
<p align="center">
  <img width="500" src="https://user-images.githubusercontent.com/2946766/78321277-c277e380-7528-11ea-99e9-034970a5054e.png" />
</p>

### Create/Edit Rule Changes

* If the job selected _is not running_, a warning will be displayed to remind the user to enable the job before running the rule. cc @benskelker @MikePaquette -- this okay copy here?
<p align="center">
  <img width="500" src="https://user-images.githubusercontent.com/2946766/78321498-63ff3500-7529-11ea-9b09-a87186cbe0ce.png" />
</p>

Resolves elastic/siem-team#575
Resolves elastic/siem-team#519

### Checklist

Delete any items that are not applicable to this PR.

- [X] 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/master/packages/kbn-i18n/README.md)
- [ ] [Documentation](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#writing-documentation) was added for features that require explanation or tutorials 
  - Scheduled time with @benskelker to update docs
- [X] [Unit or functional tests](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#cross-browser-compatibility) were updated or added to match the most common scenarios
spong added a commit to spong/kibana that referenced this pull request Apr 6, 2020
## Summary

This PR fixes a number of UX issues around the new prebuilt `machine_learning` rules when the user does not have the necessary permissions to manage the backing ML Job. Along with elastic#62383, this ensures there is adequate information for the user determine if a rule is not working because the backing job is not running (and helping to prevent this from occurring). This also includes some requested copy changes, including:

* Renames `Anomaly Detection`  dropdown to `ML job settings`
<p align="center">
  <img width="500" src="https://user-images.githubusercontent.com/2946766/78320279-57c5a880-7526-11ea-8350-647cbba263a4.png" />
</p>

* Updates copy in `ML job settings` dropdown
<p align="center">
  <img width="500" src="https://user-images.githubusercontent.com/2946766/78320473-cc98e280-7526-11ea-8871-e97661ff5f78.png" />
</p>

* Only shows `ML job settings` UI when on `/detections/` routes 
<p align="center">
  <img width="500" src="https://user-images.githubusercontent.com/2946766/78320401-922f4580-7526-11ea-9f97-0ec06526b273.png" />
</p>


### All Rules Changes

* Disables the `activate switch` if user does not have permission to enable/disable jobs
<p align="center">
  <img width="500" src="https://user-images.githubusercontent.com/2946766/78320892-d3742500-7527-11ea-90bb-91fd203480bd.png" />
</p>

* Adds warning toast when attempting to activate via bulk actions (if user does not have permission to enable/disable jobs)
<p align="center">
  <img width="300" src="https://user-images.githubusercontent.com/2946766/78321015-1a621a80-7528-11ea-8ab0-f9fef19240f7.png" />
</p>

### Rule Details Changes
* `Machine Learning job` link now links to ML App with table filtered to the relevant job

* Disables the `activate switch` if user does not have permission to enable/disable jobs
<p align="center">
  <img width="500" src="https://user-images.githubusercontent.com/2946766/78321277-c277e380-7528-11ea-99e9-034970a5054e.png" />
</p>

### Create/Edit Rule Changes

* If the job selected _is not running_, a warning will be displayed to remind the user to enable the job before running the rule. cc @benskelker @MikePaquette -- this okay copy here?
<p align="center">
  <img width="500" src="https://user-images.githubusercontent.com/2946766/78321498-63ff3500-7529-11ea-9b09-a87186cbe0ce.png" />
</p>

Resolves elastic/siem-team#575
Resolves elastic/siem-team#519

### Checklist

Delete any items that are not applicable to this PR.

- [X] 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/master/packages/kbn-i18n/README.md)
- [ ] [Documentation](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#writing-documentation) was added for features that require explanation or tutorials 
  - Scheduled time with @benskelker to update docs
- [X] [Unit or functional tests](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#cross-browser-compatibility) were updated or added to match the most common scenarios
spong added a commit that referenced this pull request Apr 6, 2020
## Summary

This PR fixes a number of UX issues around the new prebuilt `machine_learning` rules when the user does not have the necessary permissions to manage the backing ML Job. Along with #62383, this ensures there is adequate information for the user determine if a rule is not working because the backing job is not running (and helping to prevent this from occurring). This also includes some requested copy changes, including:

* Renames `Anomaly Detection`  dropdown to `ML job settings`
<p align="center">
  <img width="500" src="https://user-images.githubusercontent.com/2946766/78320279-57c5a880-7526-11ea-8350-647cbba263a4.png" />
</p>

* Updates copy in `ML job settings` dropdown
<p align="center">
  <img width="500" src="https://user-images.githubusercontent.com/2946766/78320473-cc98e280-7526-11ea-8871-e97661ff5f78.png" />
</p>

* Only shows `ML job settings` UI when on `/detections/` routes 
<p align="center">
  <img width="500" src="https://user-images.githubusercontent.com/2946766/78320401-922f4580-7526-11ea-9f97-0ec06526b273.png" />
</p>


### All Rules Changes

* Disables the `activate switch` if user does not have permission to enable/disable jobs
<p align="center">
  <img width="500" src="https://user-images.githubusercontent.com/2946766/78320892-d3742500-7527-11ea-90bb-91fd203480bd.png" />
</p>

* Adds warning toast when attempting to activate via bulk actions (if user does not have permission to enable/disable jobs)
<p align="center">
  <img width="300" src="https://user-images.githubusercontent.com/2946766/78321015-1a621a80-7528-11ea-8ab0-f9fef19240f7.png" />
</p>

### Rule Details Changes
* `Machine Learning job` link now links to ML App with table filtered to the relevant job

* Disables the `activate switch` if user does not have permission to enable/disable jobs
<p align="center">
  <img width="500" src="https://user-images.githubusercontent.com/2946766/78321277-c277e380-7528-11ea-99e9-034970a5054e.png" />
</p>

### Create/Edit Rule Changes

* If the job selected _is not running_, a warning will be displayed to remind the user to enable the job before running the rule. cc @benskelker @MikePaquette -- this okay copy here?
<p align="center">
  <img width="500" src="https://user-images.githubusercontent.com/2946766/78321498-63ff3500-7529-11ea-9b09-a87186cbe0ce.png" />
</p>

Resolves elastic/siem-team#575
Resolves elastic/siem-team#519

### Checklist

Delete any items that are not applicable to this PR.

- [X] 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/master/packages/kbn-i18n/README.md)
- [ ] [Documentation](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#writing-documentation) was added for features that require explanation or tutorials 
  - Scheduled time with @benskelker to update docs
- [X] [Unit or functional tests](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#cross-browser-compatibility) were updated or added to match the most common scenarios
spong added a commit that referenced this pull request Apr 6, 2020
## Summary

This PR fixes a number of UX issues around the new prebuilt `machine_learning` rules when the user does not have the necessary permissions to manage the backing ML Job. Along with #62383, this ensures there is adequate information for the user determine if a rule is not working because the backing job is not running (and helping to prevent this from occurring). This also includes some requested copy changes, including:

* Renames `Anomaly Detection`  dropdown to `ML job settings`
<p align="center">
  <img width="500" src="https://user-images.githubusercontent.com/2946766/78320279-57c5a880-7526-11ea-8350-647cbba263a4.png" />
</p>

* Updates copy in `ML job settings` dropdown
<p align="center">
  <img width="500" src="https://user-images.githubusercontent.com/2946766/78320473-cc98e280-7526-11ea-8871-e97661ff5f78.png" />
</p>

* Only shows `ML job settings` UI when on `/detections/` routes 
<p align="center">
  <img width="500" src="https://user-images.githubusercontent.com/2946766/78320401-922f4580-7526-11ea-9f97-0ec06526b273.png" />
</p>


### All Rules Changes

* Disables the `activate switch` if user does not have permission to enable/disable jobs
<p align="center">
  <img width="500" src="https://user-images.githubusercontent.com/2946766/78320892-d3742500-7527-11ea-90bb-91fd203480bd.png" />
</p>

* Adds warning toast when attempting to activate via bulk actions (if user does not have permission to enable/disable jobs)
<p align="center">
  <img width="300" src="https://user-images.githubusercontent.com/2946766/78321015-1a621a80-7528-11ea-8ab0-f9fef19240f7.png" />
</p>

### Rule Details Changes
* `Machine Learning job` link now links to ML App with table filtered to the relevant job

* Disables the `activate switch` if user does not have permission to enable/disable jobs
<p align="center">
  <img width="500" src="https://user-images.githubusercontent.com/2946766/78321277-c277e380-7528-11ea-99e9-034970a5054e.png" />
</p>

### Create/Edit Rule Changes

* If the job selected _is not running_, a warning will be displayed to remind the user to enable the job before running the rule. cc @benskelker @MikePaquette -- this okay copy here?
<p align="center">
  <img width="500" src="https://user-images.githubusercontent.com/2946766/78321498-63ff3500-7529-11ea-9b09-a87186cbe0ce.png" />
</p>

Resolves elastic/siem-team#575
Resolves elastic/siem-team#519

### Checklist

Delete any items that are not applicable to this PR.

- [X] 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/master/packages/kbn-i18n/README.md)
- [ ] [Documentation](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#writing-documentation) was added for features that require explanation or tutorials 
  - Scheduled time with @benskelker to update docs
- [X] [Unit or functional tests](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#cross-browser-compatibility) were updated or added to match the most common scenarios
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@rylnd rylnd mentioned this pull request Apr 9, 2020
11 tasks
@marrasherrier
Copy link
Contributor

From slack conversation:

Screen Recording 2020-04-14 at 9.31.41 AM.mov.zip

@spong:
The only thing I'm wondering about is with the anomalies table, and what logic we'll use to show the empty view. I think no jobs running + no data is probably the best bet. And of course if no permissions, whatever placeholder upsell/talk to your admin copy we want.

If there is anomaly data do show, will the button to enable jobs still be present (top right maybe?)

@marrasherrier
Copy link
Contributor

@spong what do you mean by "no jobs running + no data is probably the best bet?"

I will update designs to include these 3 use cases:

  • anamoly data present
  • no anamoly data (because no persmissions)
  • no anamoly data (because no jobs + no data)

@MindyRS MindyRS added the Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. label Sep 23, 2021
@elasticmachine
Copy link
Contributor

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

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 release_note:skip Skip the PR/issue when compiling release notes Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:SIEM v7.7.0 v7.8.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants