[Enhancement-3139] Make FLS, DLS and Field Masking Unit Tests more extensive by testing more document retrieval APIs

[prabhask5]

Description

Add new cases to DlsIntegrationTests.java, and FlsAndFieldMaskingTests.java to account for all permutations of cases.

• Category (Enhancement, New feature, Bug fix, Test fix, Refactoring, Maintenance, Documentation) Enhancement

• Why these changes are required? Previously these tests weren't comprehensive, therefore leading to uncatched bugs in deployment.

• What is the old behavior before changes and new behavior after changes? Tests are more comprehensive.

Issues Resolved

• Resolves #3139

Is this a backport? If so, please add backport PR # and/or commits # No

Testing

Added new test cases.

Check List

• [x] New functionality includes testing
• [x] New functionality has been documented
• [x] Commits are signed per the DCO using --signoff

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.

[prabhask5]

@peternied @scrawfor99 @cwperks @DarshitChanpura Ready for review!

Also related to @scrawfor99's comment on the last PR I made:

Hi @prabhask5, thanks for making those changes above. Looking at some of the definitions you have, it seems like the issue is because of the contradiction in some of the roles and the ordering of how the rules are applied. I can see where you tried to combine a role which can only see "titles" with a role which cannot see "titles." I think you thought that this would result in having a user which could not see any "titles" since that is more restrictive then the other role. I can definitely see where you are coming from...

Unfortunately, things don't quite work like that. When DLS/FLS is computed, it is done so in an additive way. Here are some related discussions around DLS/FLS that may be of use for moving forward: https://github.com/opensearch-project/security/issues/2556. Basically, we end up taking a union of the roles and this can lead to weird behavior when the ordering of the rules is swapped. In this case, I suspect the read title option is being applied after the read none option meaning the final result is that the user can get the titles. I would have to step through your tests to be sure but from what I remember this is my best guess. You can try reordering the rules or redefining them to be more consistent.

Is there consensus that this is the best way to deal with overlapping roles? In these test scenarios I have overlapping roles that cause users to both only see the title field in a document and see every other field but title in a document. With the current functionality the combination of these roles leads to not a union, but just following the less restrictive role completely, leading to the output having all fields except the title field. But in the case that both of these roles are applied to the same user, wouldn't the additive property of the roles make more sense for all fields of the document to be visible here? I can work on refactoring the role filtering implementation if there is support for this idea. Also I believe that this additive implementation might fix issues like https://github.com/opensearch-project/security/issues/2556 since it would not depend on the order of overlapping roles.

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 65.44%. Comparing base (8688d6b) to head (5a28412). Report is 8 commits behind head on main.

Additional details and impacted files

[](https://app.codecov.io/gh/opensearch-project/security/pull/4417?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=opensearch-project) ```diff @@ Coverage Diff @@ ## main #4417 +/- ## ========================================== + Coverage 65.42% 65.44% +0.01% ========================================== Files 310 312 +2 Lines 22013 22042 +29 Branches 3556 3559 +3 ========================================== + Hits 14402 14425 +23 - Misses 5841 5843 +2 - Partials 1770 1774 +4 ``` [see 16 files with indirect coverage changes](https://app.codecov.io/gh/opensearch-project/security/pull/4417/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=opensearch-project)

[prabhask5]

Also let me know how I can increase the code coverage more

[cwperks]

Is there consensus that this is the best way to deal with overlapping roles? In these test scenarios I have overlapping roles that cause users to both only see the title field in a document and see every other field but title in a document. With the current functionality the combination of these roles leads to not a union, but just following the less restrictive role completely, leading to the output having all fields except the title field. But in the case that both of these roles are applied to the same user, wouldn't the additive property of the roles make more sense for all fields of the document to be visible here? I can work on refactoring the role filtering implementation if there is support for this idea. Also I believe that this additive implementation might fix issues like https://github.com/opensearch-project/security/issues/2556 since it would not depend on the order of overlapping roles.

Good question @prabhask5! I think the functionality is more clear in DLS, but FLS is trickier because of implementation details on the Lucene level.

For DLS it is a Union:

Imagine UserA is mapped to role1 and role2

Also take an index called songs with these entries:

doc1, doc2, doc3, doc4, doc5

role1 would allow user to see doc1 and doc2

role2 would allow user to see doc4 and doc5

What should UserA be able to see? In the DLS implementation in the security plugin it would be doc1, doc2, doc4 and doc5 (all except for doc3) which is a union of the documents that can be seen from the user being mapped to role1 and role2

This is all documented here: https://opensearch.org/docs/latest/security/access-control/document-level-security/#dls-and-multiple-roles

The limitation on FLS is mentioned here: https://opensearch.org/docs/latest/security/access-control/field-level-security/#interaction-with-multiple-roles

[opensearch-trigger-bot[bot]]

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/security/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/security/backport-2.x
# Create a new branch
git switch --create backport/backport-4417-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 9caf5cbaa90c2ccbaf8b0bf00ee8bb1fe9326919
# Push it to GitHub
git push --set-upstream origin backport/backport-4417-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/security/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-4417-to-2.x.