Skip to content
This repository has been archived by the owner on Aug 2, 2022. It is now read-only.

change the backend role filtering to keep consistent with alerting pl… #383

Merged
merged 1 commit into from
Feb 5, 2021

Conversation

ylwu-amzn
Copy link
Contributor

@ylwu-amzn ylwu-amzn commented Feb 4, 2021

…ugin

Description of changes:

Now we have two different logics for AD document level access control
1.For search APIs we will add user role filter query. When user is null, will add user==null filter, if user is not null, but backend role is null or empty will add user != null and backend_roles == null or empty query. So for user who has no backend role, they can still see their detectors after enabling backend role filtering

2.For other APIs like get/update/start/stop, we will check if the detector’s user backend role can match current thread user’s backend role. If either detector’s user backend role or current thread user backend role is null, we will throw no permission exception. That means if user doesn’t configure backend role, after enabling backend role filtering, they will not see their detectors.

For alerting plugin, if user's backend role is empty, the user can't see any monitors. User has to configure backend roles before enabling alerting backend role filtering. We will change to the same way of alerting plugin (alerting plugin code link).

Test

  1. ./gradlew build
  2. ./gradlew integTest -PnumNodes=3

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@codecov
Copy link

codecov bot commented Feb 4, 2021

Codecov Report

Merging #383 (a1eb09b) into master (925a3cd) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #383      +/-   ##
============================================
- Coverage     79.16%   79.14%   -0.03%     
+ Complexity     2666     2663       -3     
============================================
  Files           247      247              
  Lines         11726    11717       -9     
  Branches       1009     1008       -1     
============================================
- Hits           9283     9273      -10     
+ Misses         1968     1967       -1     
- Partials        475      477       +2     
Flag Coverage Δ Complexity Δ
plugin 79.13% <100.00%> (-0.03%) 0.00 <4.00> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ Complexity Δ
...opendistroforelasticsearch/ad/util/ParseUtils.java 56.72% <100.00%> (-1.58%) 39.00 <4.00> (-1.00)
...on/opendistroforelasticsearch/ad/model/ADTask.java 96.56% <0.00%> (-1.57%) 66.00% <0.00%> (-2.00%)
...port/SearchAnomalyDetectorInfoTransportAction.java 63.63% <0.00%> (+9.09%) 4.00% <0.00%> (ø%)

NestedQueryBuilder nestedQueryBuilder = new NestedQueryBuilder(userFieldName, userRolesFilterQuery, ScoreMode.None);
boolQueryBuilder.must(nestedQueryBuilder);
}
List<String> backendRoles = user.getBackendRoles() != null ? user.getBackendRoles() : ImmutableList.of();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently AD plugin will parse user from thread context OPENDISTRO_SECURITY_USER_INFO_THREAD_CONTEXT, from the User.parse code, the backend role should be empty list is no backend roles. We recheck here in case we support other case or the user parse method changes in future.

Copy link
Contributor

@saratvemulapalli saratvemulapalli left a comment

Choose a reason for hiding this comment

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

The changes look good to me.
I understand the timeline is tight for the 1.13 but can we add integration tests in https://github.com/opendistro-for-elasticsearch/anomaly-detection/blob/master/src/test/java/com/amazon/opendistroforelasticsearch/ad/rest/SecureADRestIT.java for the search API as a fast follow up?

Test cases to cover:

  1. Search when backend role filter is enabled and user has no backend role
  2. Search when backend role filter is enabled and user has a backend role
  3. Search when backend role filter is disabled

Copy link
Contributor

@ohltyler ohltyler 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 changes

@ohltyler ohltyler added the enhancement New feature or request label Feb 5, 2021
@ylwu-amzn ylwu-amzn closed this Feb 5, 2021
@ylwu-amzn ylwu-amzn reopened this Feb 5, 2021
@ylwu-amzn ylwu-amzn merged commit e9d016c into opendistro-for-elasticsearch:master Feb 5, 2021
ohltyler added a commit that referenced this pull request Feb 5, 2021
@ylwu-amzn ylwu-amzn deleted the master branch February 8, 2021 22:17
ylwu-amzn pushed a commit that referenced this pull request Feb 24, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants