[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 Make FLS, DLS and Field Masking Unit Tests more extensive by testing more document retrieval APIs #3139

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

Testing

Added new test cases.

Check List

• New functionality includes testing
• New functionality has been documented
• 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: #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 #2556 since it would not depend on the order of overlapping roles.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files

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

[prabhask5]

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

[DarshitChanpura]

Thank you for adding extensive tests. Left some brief nits. Waiting for reviews from other maintainers.

[shikharj05]

Thank you @prabhask5 for adding these tests!

[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 #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

[cwperks]

Thank you for the contribution @prabhask5 !

[opensearch-trigger-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.

[nibix]

@prabhask5 @cwperks Is this actually defined behavior that is tested here?

The docs just say about combining exclusions and inclusions:

You can achieve the same outcomes using inclusion or exclusion, so choose whichever makes sense for your use case. Mixing the two doesn’t make sense and is not supported.

https://opensearch.org/docs/latest/security/access-control/field-level-security/

[cwperks]

What happens when a user defines a role that is "invalid"? Is there at least a log entry that can be looked for that mentions that the role is "invalid"? There should be some communication to the user if they try to add inclusive and exclusive rules simultaneously.