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

[ML] Changing all calls to ML endpoints to use internal user #70487

Conversation

jgowdyelastic
Copy link
Member

@jgowdyelastic jgowdyelastic commented Jul 1, 2020

Changes all calls to ML elasticsearch endpoints so that they are called by kibana's internal user.
All requests to standard es functions, like search are unchanged and use the current user.

All shared function providers now need to supply the cluster client object rather than just the callWithCurrentUser function.

Functions which originally used ml.privilegeCheck to check ML privileges now use the same checks at the routing level.

Calls to these endpoints pass across the original users credentials as a header:
PUT /api/ml/data_frame/analytics/{analyticsId}
POST /api/ml/data_frame/_evaluate
PUT /api/ml/datafeeds/{datafeedId}
POST /api/ml/datafeeds/{datafeedId}/_update
GET /api/ml/datafeeds/{datafeedId}/_preview

When creating a user role, we now have the ability to manage ML capabilities in spaces through the UI:

image

I believe further refactoring to our shared functions could be done where we would no longer require the cluster client to be passed to us, we could instead scope our own client using the request object passed in on each call to the shared function providers. This work can be done in a follow up PR.

Checklist

Delete any items that are not applicable to this PR.

@jgowdyelastic
Copy link
Member Author

@elasticmachine merge upstream

elasticmachine and others added 2 commits July 2, 2020 10:24
…' of github.com:jgowdyelastic/kibana into changing-all-calls-to-ml-endpoints-to-use-internal-user
@jgowdyelastic
Copy link
Member Author

@elasticmachine merge upstream

@elasticmachine
Copy link
Contributor

merge conflict between base and head

@jgowdyelastic
Copy link
Member Author

@elasticmachine merge upstream

@jgowdyelastic
Copy link
Member Author

@elasticmachine merge upstream

@jgowdyelastic
Copy link
Member Author

@elasticmachine merge upstream

@elasticmachine
Copy link
Contributor

merge conflict between base and head

Comment on lines +76 to +77
window.location.href = '#/jobs';
return reject();
Copy link
Contributor

Choose a reason for hiding this comment

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

I presume there is no need to reject the promise when you perform a hard refresh. Would be better to use navigateToUrl instead,

@jgowdyelastic
Copy link
Member Author

@elasticmachine merge upstream

1 similar comment
@jgowdyelastic
Copy link
Member Author

@elasticmachine merge upstream

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.

Detections changes look good! As mentioned offline we'll need to update our telemetry once this goes in, but we look good there as well 👍

@jgowdyelastic
Copy link
Member Author

@elasticmachine merge upstream

@jgowdyelastic
Copy link
Member Author

retest

@jgowdyelastic
Copy link
Member Author

@elasticmachine merge upstream

@jgowdyelastic
Copy link
Member Author

@elasticmachine merge upstream

@legrego legrego self-requested a review July 14, 2020 12:31
catalogue: [PLUGIN_ID],
savedObject: {
all: [],
read: [],
Copy link
Member

Choose a reason for hiding this comment

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

As discussed offline, let's grant access to index patterns and saved searches to users of this feature:

Suggested change
read: [],
read: ['index-pattern', 'search'],

Copy link
Member Author

Choose a reason for hiding this comment

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

added in b150246

Copy link
Member

@legrego legrego 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 the edits, LGTM.

I opened #71647 to address the current requirement on the monitor cluster privilege. It would be great if we could solve for this in a followup shortly after merging

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

✅ unchanged

History

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

@jgowdyelastic jgowdyelastic merged commit a1e511a into elastic:master Jul 14, 2020
@jgowdyelastic jgowdyelastic deleted the changing-all-calls-to-ml-endpoints-to-use-internal-user branch July 14, 2020 14:49
jgowdyelastic added a commit to jgowdyelastic/kibana that referenced this pull request Jul 14, 2020
…#70487)

* [ML] Changing all calls to ML endpoints to use internal user

* updating alerting

* updating documentation

* [ML] Changing all calls to ML endpoints to use internal user

* updating alerting

* updating documentation

* fixing missed types

* adding authorization headers to endpoint calls

* correcting has privileges call

* updating security tests

* odd eslint error

* adding auth header to module setup

* fixing missing auth argument

* fixing delete DFA job permission checks

* removing debug test tag

* removing additional ml privilege checks

* adding authorization header to _evaluate

* updating alerting cluster client name

* code clean up

* changing authorizationHeader name

* updating alterting documentation

* fixing secondary credentials

* adding management links

* updating SIEM telemetry

* fixing merge conflicts

* granting access to index patterns

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jul 14, 2020
* master: (21 commits)
  [Maps] 7.9 design improvements (elastic#71563)
  [ML] Changing all calls to ML endpoints to use internal user (elastic#70487)
  [eventLog] prevent log writing when initialization fails (elastic#71339)
  [Observability] landing page always being displayed (elastic#71494)
  [IM] Address data stream copy feedback (elastic#71615)
  [Logs UI] Anomalies page dataset filtering (elastic#71110)
  [data.search.aggs] Remove `use_field_mapping` from top hits agg (elastic#71168)
  [ML] Anomaly swim lane embeddable navigation and filter actions (elastic#71082)
  Fixes typo in siem_cloudtrail job description (elastic#71569)
  Require granted API Keys to have a name (elastic#71623)
  Update  getUsageForCollection (elastic#71609)
  Only fetch saved elements once (elastic#71310)
  [SecuritySolution][Resolver] Adding siem index and guarding process ancestry (elastic#71570)
  [APM] Additional data telemetry changes (elastic#71112)
  [Visualize] Fix export table for table export links (elastic#71249)
  [Search] Server side search API (elastic#70446)
  use inclusive language (elastic#71607)
  [Security Solution] Hide timeline footer when Resolver is open (elastic#71516)
  [Index template wizard] Remove shadow and use border for components panels (elastic#71606)
  [ML] Kibana API endpoint for histogram chart data (elastic#70976)
  ...
jgowdyelastic added a commit that referenced this pull request Jul 14, 2020
…#71667)

* [ML] Changing all calls to ML endpoints to use internal user

* updating alerting

* updating documentation

* [ML] Changing all calls to ML endpoints to use internal user

* updating alerting

* updating documentation

* fixing missed types

* adding authorization headers to endpoint calls

* correcting has privileges call

* updating security tests

* odd eslint error

* adding auth header to module setup

* fixing missing auth argument

* fixing delete DFA job permission checks

* removing debug test tag

* removing additional ml privilege checks

* adding authorization header to _evaluate

* updating alerting cluster client name

* code clean up

* changing authorizationHeader name

* updating alterting documentation

* fixing secondary credentials

* adding management links

* updating SIEM telemetry

* fixing merge conflicts

* granting access to index patterns

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Security/Spaces Platform Security - Spaces feature :ml release_note:enhancement review Team:APM All issues that need APM UI Team support v7.9.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.