Skip to content

Conversation

@MaciejMierzwa
Copy link
Contributor

@MaciejMierzwa MaciejMierzwa commented Nov 14, 2023

Description

Bug fix. Shrink, or resize operations weren't properly evaluated. More in the task: #2141

Issues Resolved

#2141

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

Testing

[Please provide details of testing done: unit testing, integration testing and manual testing]

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.

Signed-off-by: Maciej Mierzwa <[email protected]>
@codecov
Copy link

codecov bot commented Nov 15, 2023

Codecov Report

Merging #3716 (7a76d1d) into main (ee66199) will increase coverage by 1.32%.
Report is 1 commits behind head on main.
The diff coverage is 100.00%.

❗ Current head 7a76d1d differs from pull request most recent head 610ba7c. Consider uploading reports for the commit 610ba7c to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3716      +/-   ##
==========================================
+ Coverage   64.88%   66.21%   +1.32%     
==========================================
  Files         292      292              
  Lines       20776    20779       +3     
  Branches     3409     3410       +1     
==========================================
+ Hits        13481    13759     +278     
+ Misses       5606     5322     -284     
- Partials     1689     1698       +9     
Files Coverage Δ
...earch/security/resolver/IndexResolverReplacer.java 69.35% <100.00%> (+0.79%) ⬆️

... and 34 files with indirect coverage changes

Signed-off-by: Maciej Mierzwa <[email protected]>
@MaciejMierzwa
Copy link
Contributor Author

I cherry-picked changes made in this PR to not combine bug fix and test fix.

Copy link
Contributor

@stephen-crawford stephen-crawford left a comment

Choose a reason for hiding this comment

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

Hi @MaciejMierzwa, just one question but otherwise looks good.

Judging from this it looks like the issue was that the index replacer resolver was not filtering properly? Is that the change this implements which corrects the evaluation?

@MaciejMierzwa
Copy link
Contributor Author

Hi @MaciejMierzwa, just one question but otherwise looks good.

Judging from this it looks like the issue was that the index replacer resolver was not filtering properly? Is that the change this implements which corrects the evaluation?

Exactly. It wasn't even analyzing incoming requests. After going through that part of the code, in PriviledgesEvaluator it assumes the most restrictive access control -> As a result only user with wildcard access could perform shrink operation.

Copy link
Contributor

@stephen-crawford stephen-crawford left a comment

Choose a reason for hiding this comment

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

Looks good

@cwperks
Copy link
Member

cwperks commented Nov 22, 2023

Thank you @MaciejMierzwa . There was another case of this recently with SearchTemplateRequests too where a SearchTemplateRequest was falling through the cracks of IndexResolverReplacer and it required a user to have permissions to search all indices in order to use Search Template: opensearch-project/OpenSearch#9122

@cwperks cwperks merged commit 3c01fde into opensearch-project:main Nov 22, 2023
opensearch-trigger-bot bot pushed a commit that referenced this pull request Nov 22, 2023
### Description
Bug fix. Shrink, or resize operations weren't properly evaluated. More
in the task: #2141

### Issues Resolved
#2141

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

### Testing
[Please provide details of testing done: unit testing, integration
testing and manual testing]

### 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](https://github.com/opensearch-project/OpenSearch/blob/main/CONTRIBUTING.md#developer-certificate-of-origin).

---------

Signed-off-by: Maciej Mierzwa <[email protected]>
(cherry picked from commit 3c01fde)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
cwperks pushed a commit that referenced this pull request Nov 27, 2023
Backport 3c01fde from #3716.

Signed-off-by: Maciej Mierzwa <[email protected]>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
prabhask5 pushed a commit to prabhask5/opensearch-security that referenced this pull request Jan 11, 2024
### Description
Bug fix. Shrink, or resize operations weren't properly evaluated. More
in the task: opensearch-project#2141

### Issues Resolved
opensearch-project#2141

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

### Testing
[Please provide details of testing done: unit testing, integration
testing and manual testing]

### 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](https://github.com/opensearch-project/OpenSearch/blob/main/CONTRIBUTING.md#developer-certificate-of-origin).

---------

Signed-off-by: Maciej Mierzwa <[email protected]>
Signed-off-by: Prabhas Kurapati <[email protected]>
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.

3 participants