Adding User support for Detector and DetectorJob #251
Conversation
Codecov Report
@@ Coverage Diff @@
## master #251 +/- ##
============================================
- Coverage 73.01% 72.85% -0.17%
- Complexity 1461 1465 +4
============================================
Files 164 164
Lines 6834 6867 +33
Branches 527 533 +6
============================================
+ Hits 4990 5003 +13
- Misses 1594 1613 +19
- Partials 250 251 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
saratvemulapalli
changed the title
| @@ -289,6 +308,9 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws | |||
| if (categoryFields != null) { | |||
| xContentBuilder.field(CATEGORY_FIELD, categoryFields.toArray()); | |||
| } | |||
| if (user != null) { | |||
There was a problem hiding this comment.
Add user field in detector index mapping https://github.com/opendistro-for-elasticsearch/anomaly-detection/blob/master/src/main/resources/mappings/anomaly-detectors.json ?
Same for detector job.
There was a problem hiding this comment.
Great point sure. Will add.
| @@ -131,7 +135,8 @@ public AnomalyDetector( | |||
| Map<String, Object> uiMetadata, | |||
| Integer schemaVersion, | |||
| Instant lastUpdateTime, | |||
| List<String> categoryFields | |||
| List<String> categoryFields, | |||
| User user | |||
There was a problem hiding this comment.
Here we just add User in each detector document. Security plugin will do authorization check for each request based on backend_roles or roles ? Can you share the related code?
There was a problem hiding this comment.
Good question, security plugin will only provide the authorization headers.
My next PR will address this. Basically we have to query and filter by roles or backend_roles.
Here is a code pointer in alerting which does that: opendistro-for-elasticsearch/alerting@44abca1#diff-39bec5249e2b91e055cdb30bb3edcd28ee868ab3676e98652547bcce28eed895R86
There was a problem hiding this comment.
Got it, that makes sense.
| output.writeBoolean(true); // user exists | ||
| user.writeTo(output); | ||
| } else { | ||
| output.writeBoolean(false); // user does not exist |
There was a problem hiding this comment.
For old detector, they have no user field, so they will be open to all users which has AD permission ?
For new detector, we will create default user with the creator's user role?
There was a problem hiding this comment.
Exactly, for existing detectors where User doesn't exist, the filter will not work and users who have all access to rest api's will be able to see those detectors.
This is to maintain backward compatibility.
*Issue #195 *
Description of changes:
Adding User support to store User information for each detector created and store user context in .opendistro-anomaly-detectors index.
Adding User support for AD Job and store user context in .opendistro-anomaly-detectors-jobs.
User role will be used to filter when AD Rest API's are invoked.
Background jobs will soon have injected role from the User context.
User information is always sent to elasticsearch when Opendistro security plugin is enabled.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.