Skip to content

Conversation

@nibix
Copy link
Collaborator

@nibix nibix commented Nov 16, 2025

Description

Currently the class PrivilegesEvaluator has many more tasks than evaluating privileges: It also encapsulates configuration for role mapping, multi tenancy, it manages thread context user info data, etc.

Goal of this PR is to cut the class PrivilegesEvaluator into several modules with clearly dedicated scopes. Additionally, this PR introduces an interface for PrivilegesEvaluator which has the final goal to make privileges evaluator implementations pluggable (see #5399).

The main modules introduced by this PR are:

  • PrivilegesConfiguration: This now acts as an umbrella over all privileges-related configuration classes. It is centrally instantiated in the OpenSearchSecurityPlugin and exists only once per node runtime.
  • Interface PrivilegesEvaluator with implementation PrivilegesEvaluatorImpl and PrivilegesEvaluator.NotInitialized: This is reduced to the core privilege evaluation. PrivilegesEvaluatorImpl now contains the logic formerly located in PrivilegesEvaluator. Instances of PrivilegesEvaluator are managed by PrivilegesConfiguration, which can re-instantiate PrivilegeEvaluator on demand. Thus, PrivilegesEvaluatorImpl has no longer an unitialized state in case config has not been loaded (yet); the unitialized state is now represented by the class PrivilegesEvaluator.NotInitialized.
  • Interface RoleMapper and classes ConfigurableRoleMapper and InjectedRoleMapper. This encapsulates role mapping logic which was formerly sprayed around in PrivilegesEvaluator, SecurityFilter and ConfigModel. Instances of RoleMapper are managed by OpenSearchSecurityPlugin. They are not managed by PrivilegesConfiguration because role mapping is not really related to privileges.
  • ThreadContextUserInfo: Encapsulates logic related to writing a user info string into the thread context.
  • DashboardsMultiTenancyConfiguration: A lightweight container for configuration for OpenSearch Dashboards multi tenancy.
  • Category: Refactoring
  • Why these changes are required? Making PrivilegesEvaluator pluggable as preparation for Introduced explicit index resolution API #5399.
  • What is the old behavior before changes and new behavior after changes? No behavioral changes.

Testing

  • Existing int tests

Check List

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

@nibix nibix force-pushed the pluggable-privilege-evaluator branch from 157cff0 to ffcb697 Compare November 16, 2025 09:34
@codecov
Copy link

codecov bot commented Nov 16, 2025

Codecov Report

❌ Patch coverage is 85.98726% with 88 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.48%. Comparing base (f4e573f) to head (922e8f4).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...h/security/privileges/PrivilegesEvaluatorImpl.java 83.09% 27 Missing and 21 partials ⚠️
...ch/security/privileges/ConfigurableRoleMapper.java 86.31% 5 Missing and 8 partials ⚠️
...h/security/privileges/PrivilegesConfiguration.java 85.45% 5 Missing and 3 partials ⚠️
.../opensearch/security/support/HostResolverMode.java 42.85% 2 Missing and 2 partials ⚠️
...pensearch/security/user/ThreadContextUserInfo.java 87.87% 3 Missing and 1 partial ⚠️
...arch/security/configuration/ClusterInfoHolder.java 33.33% 1 Missing and 1 partial ⚠️
...rivileges/DashboardsMultiTenancyConfiguration.java 90.00% 1 Missing and 1 partial ⚠️
...earch/security/privileges/PrivilegesEvaluator.java 87.50% 1 Missing and 1 partial ⚠️
...opensearch/security/rest/DashboardsInfoAction.java 87.50% 1 Missing and 1 partial ⚠️
.../security/dlic/rest/api/PermissionsInfoAction.java 50.00% 1 Missing ⚠️
... and 2 more
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5791      +/-   ##
==========================================
+ Coverage   73.37%   73.48%   +0.11%     
==========================================
  Files         434      437       +3     
  Lines       26618    26716      +98     
  Branches     3963     3954       -9     
==========================================
+ Hits        19530    19633     +103     
- Misses       5199     5207       +8     
+ Partials     1889     1876      -13     
Files with missing lines Coverage Δ
.../opensearch/security/OpenSearchSecurityPlugin.java 85.38% <100.00%> (+0.17%) ⬆️
...va/org/opensearch/security/auth/RolesInjector.java 97.77% <100.00%> (+1.11%) ⬆️
...search/security/configuration/DlsFlsValveImpl.java 65.60% <ø> (ø)
...urity/configuration/PrivilegesInterceptorImpl.java 73.14% <100.00%> (+0.62%) ⬆️
...figuration/SecurityFlsDlsIndexSearcherWrapper.java 79.66% <100.00%> (ø)
...rity/configuration/SystemIndexSearcherWrapper.java 94.82% <100.00%> (+3.30%) ⬆️
...earch/security/dlic/rest/api/AccountApiAction.java 98.68% <100.00%> (+0.03%) ⬆️
...dlic/rest/api/RestApiAdminPrivilegesEvaluator.java 73.58% <100.00%> (ø)
...rity/dlic/rest/api/RestApiPrivilegesEvaluator.java 69.23% <100.00%> (ø)
...ecurity/dlic/rest/api/SecurityApiDependencies.java 94.11% <100.00%> (ø)
... and 23 more

... and 13 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@nibix nibix force-pushed the pluggable-privilege-evaluator branch from ffcb697 to 7154339 Compare November 16, 2025 12:06
@nibix nibix marked this pull request as ready for review November 16, 2025 12:07
Signed-off-by: Nils Bandener <[email protected]>
@nibix nibix force-pushed the pluggable-privilege-evaluator branch from 7154339 to 65375fd Compare November 16, 2025 12:41
Copy link
Collaborator

@shikharj05 shikharj05 left a comment

Choose a reason for hiding this comment

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

Thanks @nibix for the changes, this will help with #4702 as well. I think we should add a task(outside of this PR) to improve our documentation here.

shikharj05
shikharj05 previously approved these changes Nov 17, 2025
Signed-off-by: Nils Bandener <[email protected]>
@cwperks
Copy link
Member

cwperks commented Nov 17, 2025

Thank you for this PR @nibix ! The PrivilegesEvaluator class does have too much responsibility and this split up makes perfect sense to me.

@nibix
Copy link
Collaborator Author

nibix commented Nov 18, 2025

I forgot to apply one set of changes: As the role mapping has moved to RoleMapper, the class ConfigModel can now be deleted completely.

This is done in this commit:

ff01e72

shikharj05
shikharj05 previously approved these changes Nov 18, 2025
cwperks
cwperks previously approved these changes Nov 18, 2025
Signed-off-by: Nils Bandener <[email protected]>
@nibix
Copy link
Collaborator Author

nibix commented Nov 18, 2025

@cwperks @shikharj05 I had to apply a few minor fixes after rebasing to the current main branch. can you please take another look?

@nibix nibix merged commit a41bd03 into opensearch-project:main Nov 19, 2025
66 of 67 checks passed
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