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

Replaced uses of SecurityRoles by Set<String> mappedRoles where the SecurityRoles functionality is not needed #4432

Merged
merged 1 commit into from
Jun 13, 2024

Conversation

nibix
Copy link
Collaborator

@nibix nibix commented Jun 11, 2024

Description

This code change is just in preparation for the change in #4380 as requested in #4380 (comment) .

In the course of #4380 , the SecurityRoles class will be replaced by a new concept. This PR already replaces the usages of SecurityRoles where no methods are used on it except the getRoleNames() method. This method call can be easily replaced by the Set<String> mappedRoles, which is computed by PrivilegesEvaluator before the SecurityRoles instance is created. This change elliminated the coupling of the interfaces that consume that information to the SecurityRoles class, thereby enhancing code quality.

  • Category: Refactoring
  • Why these changes are required? The changes in Optimized Privilege Evaluation [DRAFT] #4380 will abolish the SecurityRoles class in the end.
  • What is the old behavior before changes and new behavior after changes? No behavioral changes.

Issues Resolved

Testing

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

Check List

  • New functionality includes testing - no new functionality
  • New functionality has been documented - no new functionality
  • 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.

@nibix
Copy link
Collaborator Author

nibix commented Jun 11, 2024

@peternied @cwperks @reta I am getting test failures on DelegatingRestHandlerTests - I guess this is a regression caused by the changes done in opensearch-project/OpenSearch#13772 to RestHandler. I guess this can be fixed by adding supportsStreaming() to DelegatingRestHandler.

@reta
Copy link
Collaborator

reta commented Jun 11, 2024

@peternied @cwperks @reta I am getting test failures on DelegatingRestHandlerTests

Fixing it right away, thank you @nibix

@reta reta mentioned this pull request Jun 11, 2024
3 tasks
@nibix
Copy link
Collaborator Author

nibix commented Jun 11, 2024

@reta Thanks for the quick fix!

@nibix
Copy link
Collaborator Author

nibix commented Jun 11, 2024

@peternied @reta I am getting now another CI failure where I am a bit lost. Maybe someone of you has an idea?

* Where:
Build file '/home/runner/work/security/security/build.gradle' line: 473

* What went wrong:
A problem occurred evaluating root project 'opensearch-security'.
> Cannot change resolution strategy of dependency configuration ':runtimeClasspath' after it has been resolved.

However, gradlew assemble on my own system runs fine.

@reta
Copy link
Collaborator

reta commented Jun 11, 2024

@peternied @reta I am getting now another CI failure where I am a bit lost. Maybe someone of you has an idea?

@nibix fix is coming shortly , very unfortunate you run into those, apologies for that

@nibix
Copy link
Collaborator Author

nibix commented Jun 12, 2024

I ran into more CI issues, a fix is here: #4446

@cwperks cwperks added the backport 2.x backport to 2.x branch label Jun 13, 2024
@cwperks
Copy link
Member

cwperks commented Jun 13, 2024

@nibix I'm not sure why some of the tests have not run. Can you rebase and push to see if it kicks off the CI checks? I'm not able to force start them.

…ecurityRoles functionality is not needed.

Signed-off-by: Nils Bandener <[email protected]>
@nibix
Copy link
Collaborator Author

nibix commented Jun 13, 2024

@cwperks Done, but we had again test failures in a windows job. Would you mind to restart this one again?

Copy link

codecov bot commented Jun 13, 2024

Codecov Report

Attention: Patch coverage is 60.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 65.43%. Comparing base (c1872b6) to head (3817416).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4432      +/-   ##
==========================================
- Coverage   65.44%   65.43%   -0.01%     
==========================================
  Files         312      312              
  Lines       22042    22037       -5     
  Branches     3559     3557       -2     
==========================================
- Hits        14425    14420       -5     
  Misses       5843     5843              
  Partials     1774     1774              
Files Coverage Δ
...earch/security/privileges/PrivilegesEvaluator.java 72.17% <50.00%> (+0.05%) ⬆️
...rity/privileges/ProtectedIndexAccessEvaluator.java 74.46% <66.66%> (+0.99%) ⬆️

... and 1 file with indirect coverage changes

@cwperks cwperks merged commit 681a944 into opensearch-project:main Jun 13, 2024
80 of 82 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Jun 13, 2024
…ecurityRoles functionality is not needed (#4432)

Signed-off-by: Nils Bandener <[email protected]>
(cherry picked from commit 681a944)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x backport to 2.x branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants