opensearch-project / security

🔐 Secure your cluster with TLS, numerous authentication backends, data masking, audit logging as well as role-based access control on indices, documents, and fields
https://opensearch.org/docs/latest/security-plugin/index/
Apache License 2.0
180 stars 264 forks source link

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

Closed nibix closed 2 weeks ago

nibix commented 3 weeks ago

Description

This code change is just in preparation for the change in #4380 as requested in https://github.com/opensearch-project/security/pull/4380#discussion_r1621325450 .

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.

Issues Resolved

Testing

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

Check List

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 commented 3 weeks ago

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

reta commented 3 weeks ago

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

Fixing it right away, thank you @nibix

nibix commented 3 weeks ago

@reta Thanks for the quick fix!

nibix commented 3 weeks ago

@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 commented 3 weeks ago

@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 commented 2 weeks ago

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

cwperks commented 2 weeks ago

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

nibix commented 2 weeks ago

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

codecov[bot] commented 2 weeks ago

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](https://app.codecov.io/gh/opensearch-project/security/pull/4432/graphs/tree.svg?width=650&height=150&src=pr&token=rBpySfQXMt&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=opensearch-project)](https://app.codecov.io/gh/opensearch-project/security/pull/4432?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 #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](https://app.codecov.io/gh/opensearch-project/security/pull/4432?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=opensearch-project) | Coverage Δ | | |---|---|---| | [...earch/security/privileges/PrivilegesEvaluator.java](https://app.codecov.io/gh/opensearch-project/security/pull/4432?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fopensearch%2Fsecurity%2Fprivileges%2FPrivilegesEvaluator.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=opensearch-project#diff-c3JjL21haW4vamF2YS9vcmcvb3BlbnNlYXJjaC9zZWN1cml0eS9wcml2aWxlZ2VzL1ByaXZpbGVnZXNFdmFsdWF0b3IuamF2YQ==) | `72.17% <50.00%> (+0.05%)` | :arrow_up: | | [...rity/privileges/ProtectedIndexAccessEvaluator.java](https://app.codecov.io/gh/opensearch-project/security/pull/4432?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fopensearch%2Fsecurity%2Fprivileges%2FProtectedIndexAccessEvaluator.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=opensearch-project#diff-c3JjL21haW4vamF2YS9vcmcvb3BlbnNlYXJjaC9zZWN1cml0eS9wcml2aWxlZ2VzL1Byb3RlY3RlZEluZGV4QWNjZXNzRXZhbHVhdG9yLmphdmE=) | `74.46% <66.66%> (+0.99%)` | :arrow_up: | ... and [1 file with indirect coverage changes](https://app.codecov.io/gh/opensearch-project/security/pull/4432/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=opensearch-project)