[ML] Adding capabilities checks to shared functions #70069

jgowdyelastic · 2020-06-26T13:17:33Z

Adds capabilities checks to all shared ML functions.
Checks are the same as the kibana endpoint equivalents. e.g.
/api/ml/results/anomalies_table_data and getAnomaliesTableData both require canGetJobs.
All plugins calling our shared functions now need to supply a request object.

Note, due to limitations in alerting, the functions called by SIEM (jobsSummary and mlAnomalySearch) currently have their capabilities checks disabled.
Once alerting can supply a real request object, these checks can be reinstated.

Functional tests will be added in a follow up PR.

Note, also include some small refactoring to replace ILegacyScopedClusterClient with LegacyAPICaller

Unit or functional tests were updated or added to match the most common scenarios
This was checked for breaking API changes and was labeled appropriately

jgowdyelastic · 2020-06-29T08:18:02Z

@elasticmachine merge upstream

elasticmachine · 2020-06-29T13:33:15Z

Pinging @elastic/ml-ui (:ml)

elasticmachine · 2020-06-29T13:33:17Z

Pinging @elastic/apm-ui (Team:apm)

smith

APM changes look good.

rylnd

Security Solution changes LGTM; I was able to generate signals from anomalies via an ML Detections rule.

walterra

Looks good overall, just added a question about await without try/catch blocks and a types.

walterra · 2020-07-01T08:09:21Z

x-pack/plugins/ml/server/lib/capabilities/check_capabilities.ts

+export type HasMlCapabilities = (capabilities: MlCapabilitiesKey[]) => void;
+
+export function hasMlCapabilitiesProvider(resolveMlCapabilities: ResolveMlCapabilities) {
+  return (request: KibanaRequest) => {


Is there a way you could make use of the HasMlCapabilities type to enforce it? Looking at the code like this I'm unsure why the type HasMlCapabilities is a function without async and the return function in the provider uses async.

HasMlCapabilities should really return Promise<void> here. i'll update it.
I'll also add HasMlCapabilities as the return type of this returned function to make it more explicit.

x-pack/plugins/ml/server/shared_services/providers/anomaly_detectors.ts

x-pack/plugins/ml/server/lib/capabilities/check_capabilities.ts

x-pack/plugins/ml/server/lib/capabilities/check_capabilities.test.ts

weltenwort

Seems to work for the infra plugin where the checks are not disabled. 👍

weltenwort · 2020-07-01T10:06:48Z

x-pack/plugins/ml/server/lib/capabilities/check_capabilities.ts

+      }
+
+      if (capabilities.every((c) => mlCapabilities![c] === true) === false) {
+        throw Error('Insufficient privileges to access feature');


I was wondering if we could use a custom Error subclass here to enable the routes to translate it into a 403 instead of 500 error. 🤔

We're doing something like that in the infra plugin here:

kibana/x-pack/plugins/infra/server/lib/log_analysis/errors.ts

Lines 16 to 21 in e8cf08f

export class NoLogAnalysisMlJobError extends Error {

constructor(message?: string) {

super(message);

Object.setPrototypeOf(this, new.target.prototype);

}

}

Alternatively, a custom marker attribute on the Error subclass could also be used for identification.

What do you think?

yes, I like this idea.
added in fbac69f

awesome, thank you ❤️

jgowdyelastic · 2020-07-01T13:55:54Z

@elasticmachine merge upstream

…ions

kibanamachine · 2020-07-01T15:46:52Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 09bbe7f

Build metrics

✅ unchanged

History

💔 Build #57830 failed fbac69f
💔 Build #57797 failed 6bb87f3
💔 Build #57774 failed f81d37f
💚 Build #57456 succeeded dfacbb2
💚 Build #57188 succeeded 9ab59b5841a6da87a9857c6b9387a545d2201c64

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

darnautov

LGTM

* [ML] Adding capabilities checks to shared functions * small refactor * disabling capabilities checks for functions called by SIEM alerting * testing git * removing comment * using constant for ml app id * tiny type clean up * removing check in ml_capabilities * fixing types * removing capabilities checks from ml_capabilities endpoint * updating types * better error handling * improving capabilities check * adding custom errors Co-authored-by: Elastic Machine <[email protected]>

* [ML] Adding capabilities checks to shared functions * small refactor * disabling capabilities checks for functions called by SIEM alerting * testing git * removing comment * using constant for ml app id * tiny type clean up * removing check in ml_capabilities * fixing types * removing capabilities checks from ml_capabilities endpoint * updating types * better error handling * improving capabilities check * adding custom errors Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: Elastic Machine <[email protected]>

jgowdyelastic requested review from peteharverson, darnautov, Jeremy-Walters, alvarezmelissa87, rylnd and walterra and removed request for Jeremy-Walters and rylnd June 29, 2020 13:21

jgowdyelastic self-assigned this Jun 29, 2020

jgowdyelastic marked this pull request as ready for review June 29, 2020 13:33

jgowdyelastic requested review from a team as code owners June 29, 2020 13:33

botelastic bot added the Team:APM All issues that need APM UI Team support label Jun 29, 2020

jgowdyelastic mentioned this pull request Jun 29, 2020

[ML] Integration with Kibana Spaces #64172

Closed

26 tasks

jgowdyelastic removed the v7.8.0 label Jun 29, 2020

jgowdyelastic added 5 commits June 30, 2020 09:50

testing git

23dd34c

removing comment

14c3bb3

using constant for ml app id

9c4d702

tiny type clean up

7d87a9d

removing check in ml_capabilities

41b1d6e

jgowdyelastic force-pushed the adding-capabilities-checks-to-shared-functions branch from 9ab59b5 to 41b1d6e Compare June 30, 2020 10:36

jgowdyelastic added 2 commits June 30, 2020 11:36

fixing types

116892a

removing capabilities checks from ml_capabilities endpoint

dfacbb2

smith approved these changes Jun 30, 2020

View reviewed changes

jgowdyelastic requested a review from rylnd June 30, 2020 20:39

rylnd approved these changes Jun 30, 2020

View reviewed changes

walterra reviewed Jul 1, 2020

View reviewed changes

updating types

f81d37f

darnautov reviewed Jul 1, 2020

View reviewed changes

jgowdyelastic added 2 commits July 1, 2020 10:29

better error handling

51c08b0

improving capabilities check

6bb87f3

weltenwort approved these changes Jul 1, 2020

View reviewed changes

weltenwort mentioned this pull request Jul 1, 2020

[Logs UI] Return status code 403 instead of 500 for log analysis routes when missing ml capabilities #70414

Closed

adding custom errors

fbac69f

Merge branch 'master' into adding-capabilities-checks-to-shared-funct…

09bbe7f

…ions

darnautov approved these changes Jul 1, 2020

View reviewed changes

jgowdyelastic merged commit eafd2af into elastic:master Jul 1, 2020

jgowdyelastic deleted the adding-capabilities-checks-to-shared-functions branch July 1, 2020 16:45

jgowdyelastic mentioned this pull request Jul 1, 2020

[7.x] [ML] Adding capabilities checks to shared functions (#70069) #70479

Merged

Kerry350 mentioned this pull request Aug 6, 2020

[Logs UI] Return 403s rather than 500s for ML privilege errors #74506

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ML] Adding capabilities checks to shared functions #70069

[ML] Adding capabilities checks to shared functions #70069

jgowdyelastic commented Jun 26, 2020 •

edited

Loading

jgowdyelastic commented Jun 29, 2020

elasticmachine commented Jun 29, 2020

elasticmachine commented Jun 29, 2020

smith left a comment

rylnd left a comment

walterra left a comment

walterra Jul 1, 2020

jgowdyelastic Jul 1, 2020

weltenwort left a comment

weltenwort Jul 1, 2020

jgowdyelastic Jul 1, 2020

weltenwort Jul 1, 2020

jgowdyelastic commented Jul 1, 2020

kibanamachine commented Jul 1, 2020

darnautov left a comment

	export class NoLogAnalysisMlJobError extends Error {
	constructor(message?: string) {
	super(message);
	Object.setPrototypeOf(this, new.target.prototype);
	}
	}

[ML] Adding capabilities checks to shared functions #70069

[ML] Adding capabilities checks to shared functions #70069

Conversation

jgowdyelastic commented Jun 26, 2020 • edited Loading

jgowdyelastic commented Jun 29, 2020

elasticmachine commented Jun 29, 2020

elasticmachine commented Jun 29, 2020

smith left a comment

Choose a reason for hiding this comment

rylnd left a comment

Choose a reason for hiding this comment

walterra left a comment

Choose a reason for hiding this comment

walterra Jul 1, 2020

Choose a reason for hiding this comment

jgowdyelastic Jul 1, 2020

Choose a reason for hiding this comment

weltenwort left a comment

Choose a reason for hiding this comment

weltenwort Jul 1, 2020

Choose a reason for hiding this comment

jgowdyelastic Jul 1, 2020

Choose a reason for hiding this comment

weltenwort Jul 1, 2020

Choose a reason for hiding this comment

jgowdyelastic commented Jul 1, 2020

kibanamachine commented Jul 1, 2020

💚 Build Succeeded

Build metrics

History

darnautov left a comment

Choose a reason for hiding this comment

jgowdyelastic commented Jun 26, 2020 •

edited

Loading