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

[Logs UI] Add missing ML capabilities checks #72606

Merged

Conversation

weltenwort
Copy link
Member

@weltenwort weltenwort commented Jul 21, 2020

Summary

This adds several missing Machine Learning capabilities checks to the UI to make sure the user doesn't run into downstream errors resulting from the lack of permissions. It also updates the messages of the permission prompt screens to refer to the new Kibana Machine Learning permissions instead of the old built-in roles.

closes #72416
closes #72590

Details

These buttons are now disabled without "All" Machine Learning permissions:

  • the "Recreate" button on the "Categories" result page
  • the "Enable" and "Recreate" buttons on the module list cards in the "Anomalies" flyout
  • the "Recreate" buttons in the job status callouts (for oudated jobs)

Previews

image

image

image

image

Testing

The changes affect both the "Anomalies" and the "Categories" tabs, except for the module setup list changes, which only exist in the former.

For testing the cartesian product of the permission scenarios

  • user has "All" ML permissions
  • user has "Read" ML permissions
  • user has no ML permissions

and the ML job presence scenarios

  • relevant ML jobs exist
  • relevant ML jobs don't exist

could be of particular interest.

@weltenwort weltenwort added bug Fixes for quality problems that affect the customer experience v8.0.0 Feature:Logs UI Logs UI feature Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services release_note:skip Skip the PR/issue when compiling release notes v7.9.0 labels Jul 21, 2020
@weltenwort weltenwort added this to the Logs UI 7.9 milestone Jul 21, 2020
@weltenwort weltenwort self-assigned this Jul 21, 2020
@apmmachine
Copy link
Contributor

apmmachine commented Jul 21, 2020

💚 Build Succeeded

Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: [Pull request #72606 updated]

  • Start Time: 2020-07-21T14:05:15.935+0000

  • Duration: 4 min 2 sec

@weltenwort weltenwort marked this pull request as ready for review July 21, 2020 15:57
@weltenwort weltenwort requested a review from a team as a code owner July 21, 2020 15:57
@elasticmachine
Copy link
Contributor

Pinging @elastic/logs-metrics-ui (Team:logs-metrics-ui)

@Kerry350 Kerry350 self-requested a review July 22, 2020 10:00
Copy link
Contributor

@Kerry350 Kerry350 left a comment

Choose a reason for hiding this comment

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

Code looks good 👌 Feel free to ignore, or push to a followup, my minor comments.

Testing the functionality now.

{children ?? (
<FormattedMessage
id="xpack.infra.logs.analysis.createJobButtonLabel"
defaultMessage="Create ML jobs"
Copy link
Contributor

Choose a reason for hiding this comment

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

For the default text here should it be job instead of jobs?

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 would say, it depends on whether we want to abstract away the fact that this module happens to only contain one job. 🤷


import { i18n } from '@kbn/i18n';

export const missingMlResultsPrivilegesTitle = 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.

Would there be any harm in rolling missingMlResultsPrivilegesTitle and missingMlSetupPrivilegesTitle into a singular translation as they use the same text?

Copy link
Member Author

Choose a reason for hiding this comment

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

good point, I didn't realize that when I factored them out 👍

Comment on lines -14 to +15
app: 'kibana',
hash: '/management/security/users',
app: 'management',
pathname: '/security/users',
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@Kerry350
Copy link
Contributor

Kerry350 commented Jul 22, 2020

@weltenwort Hmm, so I'm seeing some weird behaviour with the anomalies and categories tabs. It's really inconsistent, and I can't find the common thread that makes it happen, but I'll try to describe what I'm seeing. It could also be the case that I've misunderstood Kibana roles / privileges, but even if that was the case I'd expect one behaviour rather than multiple.

Context:

Against the dev-next shared cluster I've created a kerry_test_ml_all role (which is still on the cluster if that helps at all), that role is assigned to one user kerry_test_ml, and it's the only role the user has. The role assigns everything - full index privileges and space privileges - as far as I can tell there's nothing I could do to make this user more privileged.

Screenshot 2020-07-22 13 33 04

Screenshot 2020-07-22 13 33 11

I then log in to Kibana as that user. Within that single login, I see all of the following behaviours accessing the anomalies tab:

Screenshot 2020-07-22 13 10 53

Screenshot 2020-07-22 13 27 41
Screenshot 2020-07-22 13 27 54
Screenshot 2020-07-22 13 28 07
Screenshot 2020-07-22 13 28 26
Screenshot 2020-07-22 13 29 34

So there's a mixture of asking for elevated privileges, saying there's no data, and showing results.

What's really curious is the User not authorized for "/api/ml/jobs/jobs_summary" failures, these happen just hitting the ML APIs, without hitting our internal APIs at all. In the screenshot of the network panel it can be seen that they fail, and then succeed straight after, with a call for results also managing to succeed.

I don't understand the mixture of behaviours here?

@Kerry350
Copy link
Contributor

Logging in and out 3 or 4 times I was able to at least get consistency between my roles (I set up all, read, and none roles), still not sure how I managed to see such a mixture of behaviours in one session (also using a private window) 🤔

All works as expected, read works as expected, but I do see a difference for none between the two tabs:

ml-privileges

@weltenwort
Copy link
Member Author

weltenwort commented Jul 22, 2020

Thanks for checking all the permutations so thoroughly! I couldn't quite reproduce the "Anomalies" tab situation, though. 🤔 This is what it looks like for me for a user with "None" as the "Machine Learning" permissions:

image

Copy link
Contributor

@Kerry350 Kerry350 left a comment

Choose a reason for hiding this comment

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

(After talking on Slack / Zoom I had one part of my none role misconfigured).

Approving 🎉

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

async chunks size

id value diff baseline
infra 3.6MB +3.8KB 3.6MB

History

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

@weltenwort weltenwort merged commit ba55ca9 into elastic:master Jul 22, 2020
@weltenwort weltenwort deleted the logs-ui-add-flyout-ml-permissions-check branch July 22, 2020 16:32
weltenwort added a commit to weltenwort/kibana that referenced this pull request Jul 22, 2020
This adds several missing Machine Learning capabilities checks to the UI to make sure the user doesn't run into downstream errors resulting from the lack of permissions. It also updates the messages of the permission prompt screens to refer to the new Kibana Machine Learning permissions instead of the old built-in roles.
weltenwort added a commit that referenced this pull request Jul 23, 2020
Backports the following commits to 7.x:
 - [Logs UI] Add missing ML capabilities checks (#72606)
weltenwort added a commit that referenced this pull request Jul 23, 2020
Backports the following commits to 7.9:
 - [Logs UI] Add missing ML capabilities checks (#72606)
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:Logs UI Logs UI feature release_note:skip Skip the PR/issue when compiling release notes Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services v7.9.0 v8.0.0
Projects
None yet
5 participants