-
Notifications
You must be signed in to change notification settings - Fork 300
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix logic for determining if a role has no DLS/FLS/FieldMasking restrictions #5184
Conversation
…LS/FieldMasking restrictions Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
FYI @nibix |
src/main/java/org/opensearch/security/privileges/dlsfls/DocumentPrivileges.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - should we add a test case for this?
src/main/java/org/opensearch/security/privileges/dlsfls/AbstractRuleBasedPrivileges.java
Outdated
Show resolved
Hide resolved
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5184 +/- ##
==========================================
- Coverage 71.71% 71.69% -0.02%
==========================================
Files 337 337
Lines 22783 22786 +3
Branches 3604 3606 +2
==========================================
- Hits 16338 16336 -2
- Misses 4642 4644 +2
- Partials 1803 1806 +3
🚀 New features to boost your workflow:
|
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
src/main/java/org/opensearch/security/configuration/DlsFlsValveImpl.java
Show resolved
Hide resolved
src/main/java/org/opensearch/security/privileges/dlsfls/DocumentPrivileges.java
Show resolved
Hide resolved
@shikharj05 @nibix This PR is needed to unblock opensearch-3.0.0-alpha1 release. Do either of you have concerns with merging in the current state? |
Sure, let's create a separate issue to continue the conversation. |
Description
This PR fixes the logic for determining whether a role is unrestricted with regards to FLS/DLS/FieldMasking restrictions. A role is considered unrestricted if the respective
fls:
,dls:
, ormasked_fields:
portions are either an empty list or left out of the role definition altogether.The gist of the problem is that we bypass the FlsDlsValveImpl if a role has no restrictions, but we are not computing whether a role has no restrictions correctly because we are resolving to concrete indices for cluster actions and then applying the default of
{"match_none": {}}
to any resolvable indices such as.opendistro_security
.There was a bug seen in CCR tests w/ security: opensearch-project/cross-cluster-replication#1518
The issue can be reproduced by checking out the CCR repo and running:
Ensure that any local snapshots for core, security and common-utils are up to date.
For core and common-utils run
./gradlew publishToMavenLocal
For security run
./gradlew publishZipToMavenLocal
Bug fix
Issues Resolved
Resolves opensearch-project/cross-cluster-replication#1518
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.