Skip to content
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 min_doc_count handling when using Document Level Security #1714

Merged
merged 1 commit into from
Mar 29, 2022

Conversation

cliu123
Copy link
Member

@cliu123 cliu123 commented Mar 29, 2022

Signed-off-by: cliu123 lc12251109@gmail.com

Description

  • Category (Enhancement, New feature, Bug fix, Test fix, Refactoring, Maintenance, Documentation) Bug fix
  • Why these changes are required?
    Fixes an issue in Document Level Security (DLS) by improving how the min_doc_count parameter is handled. The previous behavior returns keys for aggregations with 0 matches when min_doc_count is passed as 0, regardless of whether there are actually 0 matches or matches are not visible due to DLS rules. With this fix, we throw an error message if min_doc_count is passed as 0 and DLS rules are present for that index.

Testing

UTs

Check List

  • 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.

@cliu123 cliu123 requested a review from a team March 29, 2022 19:34
@peternied
Copy link
Member

Looks like there is a build break, was this change cherry-picked directly the 1.3.1 branch?

@cliu123
Copy link
Member Author

cliu123 commented Mar 29, 2022

Looks like there is a build break, was this change cherry-picked directly the 1.3.1 branch?

No, it's not cherry-picked from 1.3 branch.

davidlago
davidlago previously approved these changes Mar 29, 2022
Signed-off-by: cliu123 <lc12251109@gmail.com>
@codecov-commenter
Copy link

codecov-commenter commented Mar 29, 2022

Codecov Report

Merging #1714 (0cc9027) into main (51e492c) will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##               main    #1714      +/-   ##
============================================
+ Coverage     62.86%   62.90%   +0.03%     
- Complexity     3262     3267       +5     
============================================
  Files           253      253              
  Lines         18089    18096       +7     
  Branches       3243     3246       +3     
============================================
+ Hits          11372    11383      +11     
+ Misses         5063     5059       -4     
  Partials       1654     1654              
Impacted Files Coverage Δ
...search/security/configuration/DlsFlsValveImpl.java 59.25% <100.00%> (+1.08%) ⬆️
.../dlic/auth/ldap2/LDAPConnectionFactoryFactory.java 56.48% <0.00%> (-1.53%) ⬇️
...ecurity/configuration/ConfigurationRepository.java 75.27% <0.00%> (+2.19%) ⬆️
...security/auditlog/sink/ExternalOpenSearchSink.java 61.72% <0.00%> (+2.46%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 51e492c...0cc9027. Read the comment docs.

@cliu123 cliu123 merged commit 7ced417 into opensearch-project:main Mar 29, 2022
wuychn pushed a commit to ochprince/security that referenced this pull request Mar 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants