add user role filter for monitor on top of ad result #275
Conversation
Codecov Report
@@ Coverage Diff @@
## master #275 +/- ##
============================================
- Coverage 79.85% 79.71% -0.15%
- Complexity 194 196 +2
============================================
Files 150 151 +1
Lines 5132 5195 +63
Branches 661 672 +11
============================================
+ Hits 4098 4141 +43
- Misses 668 699 +31
+ Partials 366 355 -11
Continue to review full report at Codecov.
|
| @@ -323,7 +324,19 @@ class MonitorRunner( | |||
| XContentType.JSON.xContent().createParser(xContentRegistry, LoggingDeprecationHandler.INSTANCE, searchSource).use { | |||
| searchRequest.source(SearchSourceBuilder.fromXContent(it)) | |||
| } | |||
| val searchResponse: SearchResponse = client.suspendUntil { client.search(searchRequest, it) } | |||
There was a problem hiding this comment.
Roles inject happens here: https://github.com/opendistro-for-elasticsearch/alerting/blob/master/alerting/src/main/kotlin/com/amazon/opendistroforelasticsearch/alerting/MonitorRunner.kt#L227
This takes care of keeping context intact when co-routines switch.
There was a problem hiding this comment.
Can you think of implementing a separate collectInputResults for AD.
There was a problem hiding this comment.
This change breaks the functionality, needs rework.
There was a problem hiding this comment.
Thanks. I add a separate method to collect anomaly result input.
...tlin/com/amazon/opendistroforelasticsearch/alerting/transport/TransportIndexMonitorAction.kt
Show resolved
Hide resolved
...tlin/com/amazon/opendistroforelasticsearch/alerting/transport/TransportIndexMonitorAction.kt
Show resolved
Hide resolved
| val nestedQueryBuilder = NestedQueryBuilder(userFieldName, userRolesFilterQuery, ScoreMode.None) | ||
| boolQueryBuilder.must(nestedQueryBuilder) |
There was a problem hiding this comment.
These 2 lines are repeated in all of the different if else cases
There was a problem hiding this comment.
Not all branches. line 68 and line 63 looks like duplicate, but if I remove one line of them, I need to add another 2 lines:
Sample code
var mustQuery = null
if (...) {
mustQuery = userExistsNestedQueryBuilder
} else {
mustQuery = nestedQueryBuilder
}
boolQueryBuilder.must(mustQuery)
Prefer to keep as is.
There was a problem hiding this comment.
ah ok, didnt realize that. Thanks
| fun addUserRolesFilter(userRoles: List<String>, searchSourceBuilder: SearchSourceBuilder, fieldName: String) { | ||
| val filterBackendRoles = QueryBuilders.termsQuery(fieldName, userRoles) | ||
| val queryBuilder = searchSourceBuilder.query() as BoolQueryBuilder | ||
| searchSourceBuilder.query(queryBuilder.filter(filterBackendRoles)) | ||
| } |
There was a problem hiding this comment.
Why do u need to create a new function? If it is because of the function name, I would then rather rename.
There was a problem hiding this comment.
This change is from my previous commit, in that commit, I need this function. Will remove this one to make the change minimal .
| @@ -323,7 +324,19 @@ class MonitorRunner( | |||
| XContentType.JSON.xContent().createParser(xContentRegistry, LoggingDeprecationHandler.INSTANCE, searchSource).use { | |||
| searchRequest.source(SearchSourceBuilder.fromXContent(it)) | |||
| } | |||
| val searchResponse: SearchResponse = client.suspendUntil { client.search(searchRequest, it) } | |||
There was a problem hiding this comment.
Can you think of implementing a separate collectInputResults for AD.
| @@ -323,7 +324,19 @@ class MonitorRunner( | |||
| XContentType.JSON.xContent().createParser(xContentRegistry, LoggingDeprecationHandler.INSTANCE, searchSource).use { | |||
| searchRequest.source(SearchSourceBuilder.fromXContent(it)) | |||
| } | |||
| val searchResponse: SearchResponse = client.suspendUntil { client.search(searchRequest, it) } | |||
There was a problem hiding this comment.
This change breaks the functionality, needs rework.
* add user role filter for monitor on top of ad result * add separate method to collect anomaly result input * check if user have available detectors when create monitor;add unit test and integration test * set size 0 when checking detector existence * rollback changes to add user role filter
Issue #opendistro-for-elasticsearch/anomaly-detection#195
Description of changes:
AD result index will be system indices, so create AD monitor will fail as common users have no read permission on anomaly result index.
And AD and Alerting will provide document level access control with user backend role user. This PR adds detector backend role checking when create and run monitor.
Backend role checking logic for AD monitor creation:
1.If monitor's user is null, then check if there is detector with null user. For old monitor and detector, they have no user field. With this we can provide backward compatibility.
2.If monitor's user is not null, but has no backend roles, then will check if there is detector with empty backend role.
3.If monitor's user has backend roles, will check if there is detector which backend role has any of the monitor's user backend roles.
The same logic for AD monitor's schedule job run, just change detector existence checking to user role filter query.
Tradeoff
We have no way to verify if user has AD read permission in alerting plugin now. So we have to check user's backend role no matter the backend role filter open or not. If we don't add backend role checking when the filter setting disabled, that will be a security leak: if user has permission to create monitor, they can get any detector's results even they have no AD read permission. We make a balance on security and user experience and choose to prioritize security. In next phase, when secure rest client ready or security plugin provides permission check action, we will add AD read permission check and provide complete user experience.
Test
Add UT and IT cases for AD monitor creation and run.
Have tested by running
./gradlew :alerting:run.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.