[ML] Space permision checks for job deletion #83871
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jgowdyelastic
requested review from
peteharverson,
darnautov and
alvarezmelissa87
jgowdyelastic
added
Feature:Anomaly Detection
| } | ||
|
|
||
| if ( | ||
| mlCapabilities.canDeleteJob === false || |
There was a problem hiding this comment.
Is it possible that the user could have one permission and not the other? Or is job deletion a single permission across ML?
There was a problem hiding this comment.
at the moment no, we only have ML viewer or ML admin roles.
But just in case we ever allow fine grained capabilities i'll change this check to be more explicit.
There was a problem hiding this comment.
Changed in 5951246
| results[jobId] = { | ||
| canDelete: true, | ||
| canUnTag: false, | ||
| // GLOBAL JOB BOOL???? |
There was a problem hiding this comment.
Is this comment needed?
There was a problem hiding this comment.
it was a reminder that we might need information in this endpoint that the UI can use to inform the user why they can't untag or delete the job.
I'll remove the comment for now, but when the UI work is done we may need to add in flags like isGlobalJob
|
Code LGTM but having trouble testing it. Will update first thing monday. |
|
@elasticmachine merge upstream |
|
|
||
| export interface DeleteJobPermission { | ||
| canDelete: boolean; | ||
| canUnTag: boolean; |
There was a problem hiding this comment.
Looking for this word online, it seems untag is the generally accepted spelling, rather than un-tag. In which case, I think it makes sense to go with canUntag here.
| * | ||
| * @api {get} /api/ml/saved_objects/delete_job_check Check whether user can delete a job | ||
| * @apiName DeleteJobCheck | ||
| * @apiDescription Check the users ability to delete jobs. Returns whether they are able |
|
|
||
| // jobs with are in individual spaces can only be untagged | ||
| // from current space if the job is in more than 1 space | ||
| const canUnTag = namespaces.length > 1; |
There was a problem hiding this comment.
As above, should probably be canUntag.
darnautov
reviewed
Nov 23, 2020
| * @apiSchema (body) jobIdsSchema (params) jobTypeSchema | ||
| * | ||
| */ | ||
| router.post( |
There was a problem hiding this comment.
Should be DELETE instead
Suggested change
| router.post( | |
| router.delete( |
There was a problem hiding this comment.
it's not deleting anything. it's checking whether you can delete
There was a problem hiding this comment.
Sorry, I read it wrong! It'd be more straightforward to start the method with Check then :)
There was a problem hiding this comment.
yes, i can see how this could be confused.
we could change this to check_delete_job_ability or something like that?
@peteharverson
There was a problem hiding this comment.
Or perhaps can_delete_job or check_can_delete_job ?
Comment on lines
242
to
246
| path: '/api/ml/saved_objects/delete_job_check/{jobType}', | ||
| validate: { | ||
| params: jobTypeSchema, | ||
| body: jobIdsSchema, | ||
| }, |
There was a problem hiding this comment.
maybe it's worth putting everything in params, e.g. '/api/ml/saved_objects/delete_job_check/{jobType}/{jobId}'
There was a problem hiding this comment.
jobIds is an array of ids, for all other similar kibana endpoints we pass the ids as an array in the body (not including endpoints which are simple wrappers around es endpoints)
There was a problem hiding this comment.
ok, I recall some endpoints with comma-separated job ids
…elastic/kibana into delete-job-space-permission-check
|
@elasticmachine merge upstream |
…elastic/kibana into delete-job-space-permission-check
|
Pinging @elastic/ml-ui (:ml) |
|
@elasticmachine merge upstream |
jgowdyelastic
added a commit
that referenced
this pull request
Nov 24, 2020
* [ML] Space permision checks for job deletion * updating spaces dependency * updating endpoint comments * adding delete job capabilities check * small change based on review * improving permissions checks * renaming function and endpoint Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
rylnd
added a commit
to rylnd/kibana
that referenced
this pull request
Nov 24, 2020
* master: (41 commits) [Maps] fix code-owners (elastic#84265) [@kbn/utils] Clean target before build (elastic#84253) [code coverage] collect for oss integration tests (elastic#83907) [APM] Use `asTransactionRate` consistently everywhere (elastic#84213) Attempt to fix incremental build error (elastic#84152) Unskip "Copy dashboards to space" (elastic#84115) Remove expressions.legacy from README (elastic#79681) Expression: Add render mode and use it for canvas interactivity (elastic#83559) [deb/rpm] Move systemd service to /usr/lib/systemd/system (elastic#83571) [Security Solution][Resolver] Allow a configurable entity_id field (elastic#81679) [ML] Space permision checks for job deletion (elastic#83871) [build] Provide ARM build of RE2 (elastic#84163) TSVB should use "histogram:maxBars" and "histogram:barTarget" settings for auto instead of a default 100 buckets (elastic#83628) [Workplace Search] Initial rendering of Org Sources (elastic#84164) update geckodriver to 0.28 (elastic#84085) Fix timelion vis escapes single quotes (elastic#84196) [Security Solution] Fix incorrect time for dns histogram (elastic#83532) [DX] Bump TS version to v4.1 (elastic#83397) [Security Solution] Add endpoint policy revision number (elastic#83982) [Fleet] Integration Policies List view (elastic#83634) ...
4 tasks
💔 Build Failed
Failed CI StepsTest FailuresX-Pack API Integration Tests.x-pack/test/api_integration/apis/ml/results/get_anomalies_table_data·ts.apis Machine Learning ResultsService GetAnomaliesTableData should fetch anomalies table dataStandard OutStack TraceX-Pack API Integration Tests.x-pack/test/api_integration/apis/ml/results/get_anomalies_table_data·ts.apis Machine Learning ResultsService GetAnomaliesTableData should fetch anomalies table dataStandard OutStack TraceMetrics [docs]Async chunks
History
To update your PR or re-run it, just comment with: |
3 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Endpoint to check what delete job tasks the user is able to perform on a specified list of jobs.
Endpoint returns a list of jobs with
canDeleteandcanUnTagflags for each.This check will be performed in the UI when a user attempts to delete a job.
UI work is coming in a different PR.
API test will be added in a follow up PR.
Job is in individual spaces, user cannot see all of them
Job can only be untagged from the current space
Job is individual spaces, which the user can see all of
Job can be deleted or untagged from current space
Job is in * space, user cannot see all spaces
Job cannot be untagged from space or deleted.
Job is in * space, user can see all spaces
Delete job only, no option to untag
Spaces plugin is disabled
Delete job only, no option to untag
User does not have
canDeleteJoborcanDeleteDataFrameAnalyticscapabilitiesJob cannot be untagged from space or deleted.
Checklist
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