Skip hasExplicitIndexPrivilege check for plugin users accessing their own system indices

[llilyy]

Description

• Category

  • Bug fix

• Why these changes are required?

  • When isSystemIndexPermissionEnabled is set to true, plugin users are unable to access their own registered system indices. This happens because the hasExplicitIndexPrivilege check is performed before the plugin user's own system index access logic, causing the request to be rejected even though the plugin should have access to its own system indices.

  • This breaks the expected behavior where plugins can manage their own system indices without requiring explicit system_index permission.

• What is the old behavior before changes and new behavior after changes?

  • Old behavior:

    • When isSystemIndexPermissionEnabled = true, all system index access (including plugin's own system indices) goes through hasExplicitIndexPrivilege check
    • Plugin users are rejected with "no permissions" error when trying to access their own registered system indices
    • The plugin user special handling logic at the bottom of evaluateSystemIndicesAccess() is never reached because the request is already rejected

  • New behavior:

    • Plugin users can access their own registered system indices for non-cluster actions without requiring hasExplicitIndexPrivilege check
    • The isPluginIndexAction condition is added to skip the permission check when:
      • The user is a plugin user
      • The action is not a cluster-level action
      • The request only targets the plugin's own registered system indices
    • Cluster-level actions are still properly restricted for plugin users
    • A new helper method getMatchingPluginIndices() is added to calculate plugin's own system indices once and reuse it, avoiding code duplication

Issues Resolved

opensearch-project/job-scheduler#865

Testing

New Integration Tests Added:

• Created SystemIndexPermissionEnabledTests.java with isSystemIndexPermissionEnabled = true configuration
• Tests verify plugin users can access their own system indices when system index permission is enabled:
  • testPluginShouldBeAbleToIndexDocumentIntoItsSystemIndex
  • testPluginShouldBeAbleToBulkIndexDocumentIntoItsSystemIndex
  • testPluginShouldBeAbleSearchOnItsSystemIndex
  • testPluginShouldBeAbleGetOnItsSystemIndex
  • testPluginShouldBeAbleUpdateOnItsSystemIndex
• Tests verify plugin users are still properly restricted:
  • testPluginShouldNotBeAbleToIndexDocumentIntoSystemIndexRegisteredByOtherPlugin
  • testPluginShouldNotBeAbleToRunClusterActions
  • testPluginShouldNotBeAbleToBulkIndexDocumentIntoMixOfSystemIndexWhereAtLeastOneDoesNotBelongToPlugin

Existing Tests Verified:

• SystemIndexTests - All tests pass (no regression with isSystemIndexPermissionEnabled = false)
• SystemIndexAccessEvaluatorTest - All unit tests pass (no impact on regular user behavior)

Test Execution:

./gradlew :integrationTest --tests "org.opensearch.security.systemindex.SystemIndexTests" --tests "org.opensearch.security.systemindex.SystemIndexPermissionEnabledTests"
# BUILD SUCCESSFUL

./gradlew :test --tests "org.opensearch.security.privileges.SystemIndexAccessEvaluatorTest"
# BUILD SUCCESSFUL

Check List

• New functionality includes testing
• New functionality has been documented
• New Roles/Permissions have a corresponding security dashboards plugin PR
• API changes companion pull request created
• 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]

Thank you for this!

We have already tests for enabled system index privileges in https://github.com/opensearch-project/security/blob/main/src/integrationTest/java/org/opensearch/security/privileges/int_tests/IndexAuthorizationReadOnlyIntTests.java ... do you think it would be possible to move the new tests there? We are currently working on a major change on privilege evaluation in #5827 , which also includes system indices. With the tests being in IndexAuthorizationReadOnlyIntTests, it would be easy for us to have test coverage also for these cases.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73.71%. Comparing base (7f168b1) to head (cb5162b).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5858      +/-   ##
==========================================
+ Coverage   73.68%   73.71%   +0.03%     
==========================================
  Files         438      438              
  Lines       26649    26656       +7     
  Branches     3939     3943       +4     
==========================================
+ Hits        19635    19650      +15     
+ Misses       5141     5133       -8     
  Partials     1873     1873              

Files with missing lines	Coverage Δ
...ecurity/privileges/SystemIndexAccessEvaluator.java	77.77% <100.00%> (+1.13%)	⬆️

... and 9 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.

[llilyy]

Thanks for the review, @nibix!
I looked into it, and IndexAuthorizationReadOnlyIntTests uses @ParametersFactory to test various user combinations, but plugin user tests always run as the plugin user internally regardless of which user calls the REST endpoint. This doesn't fit well with the matrix structure.

I can either:

1. Move the tests to a separate class in the same directory while reusing the ClusterConfig enum
2. Merge them into IndexAuthorizationReadOnlyIntTests if you prefer

Which approach would you prefer?

[nibix]

@llilyy thanks for the explanation!

1. Move the tests to a separate class in the same directory while reusing the ClusterConfig enum

If you wouldn't mind, this seems to be the optimal approach, wdyt?

[cwperks]

Thank you @llilyy. The functional changes in this PR LGTM, reviewing the tests added now.

[cwperks]

Approving the PR, but have similar comment to @nibix . I don't think parameterized tests make sense here but it would make sense to have an abstract test suite with 2 concrete implementations with different cluster settings.

[llilyy]

Thanks! I will add tests this week.

[llilyy]

@nibix
I implemented the changes suggested by @cwperks and added the corresponding integration tests under
/opensearch-project/security/blob/main/src/integrationTest/java/org/opensearch/security/privileges/int_tests.
Please let me know if you have any feedback.

[nibix]

Apologies for replying to late, but:

@cwperks

I don't think parameterized tests make sense here but it would make sense to have an abstract test suite with 2 concrete implementations with different cluster settings.

To be honest, I do strongly believe that using super classes for test suites is a strong anti-pattern. Inheritance is meant for polymorphic usage of implementations, it is NOT meant for code-reuse.

This creates IMHO really difficult to understand test classes because actual test suites like

https://github.com/opensearch-project/security/blob/b8d7b72e4913f891fdb4a8b79495bd6b7c581384/src/integrationTest/java/org/opensearch/security/privileges/int_tests/PluginSystemIndexPermissionDisabledIntTests.java

do actually not contain any tests.

See also https://en.wikipedia.org/wiki/Composition_over_inheritance

[llilyy]

@nibix

This test structure is not using inheritance for ad-hoc code reuse. It defines an explicit privileges test contract that is executed across multiple cluster configurations.

The abstract base class expresses the complete set of privilege rules that must hold, while the concrete subclasses provide different environments in which the same contract is validated. These tests are actually executed polymorphically: the same test methods are run with different LocalCluster implementations, and behavior is dynamically bound via getCluster() at runtime.

In privilege-focused tests, duplicating the same privilege rules and expectations across multiple test classes is particularly error-prone. When privilege semantics change, duplicated logic is easy to update inconsistently, which can lead to missed cases, diverging behavior, or even false positives. Centralizing the privilege rules in a single abstract test suite intentionally enforces a single source of truth for the privileges contract, while cleanly separating environmental differences such as cluster configuration or feature flags.

From the perspective of privilege correctness and long-term test stability, this is a deliberate and justified design choice rather than an inheritance anti-pattern. To address the readability concern, I've added comprehensive Javadoc to the abstract base class and the concrete implementations explaining that the actual test cases are defined in the base class to enforce consistency across configurations.

[llilyy]

@cwperks @nibix
Hi there, thanks for maintaining this project.
I'm reaching out because this PR fixes a critical bug that specifically impacts our team's workflow in production. It has become a major blocker for our upcoming release schedule.
Due to the rapid changes in the base branch, I’ve been frequently rebasing to keep this PR up-to-date, but it’s becoming increasingly difficult to manage the manual conflicts. To prevent further integration issues and move forward with our release, could you please take a look at this when you have a moment?
Your help would be greatly appreciated!

[nibix]

@llilyy

given the current test implementation, we will have to re-implement the tests before we can merge #5827 in order to also have test coverage for the new privilege evaluation mode. This will then add friction to merging #5827.

Regarding this:

In privilege-focused tests, duplicating the same privilege rules and expectations across multiple test classes is particularly error-prone. When privilege semantics change, duplicated logic is easy to update inconsistently, which can lead to missed cases, diverging behavior, or even false positives. Centralizing the privilege rules in a single abstract test suite intentionally enforces a single source of truth for the privileges contract, while cleanly separating environmental differences such as cluster configuration or feature flags.

As far as I understand, the approach with a parameterized test using the ClusterConfig enum would provide you the same benefits. No code duplication would occur. Or do I get something wrong?

From the perspective of privilege correctness and long-term test stability, this is a deliberate and justified design choice rather than an inheritance anti-pattern

For this exact purpose, parameterized tests do exist.

[nibix]

As it seem to cause a lot of pain, approving...

Still, I would really appreciate it if the tests could be afterwards adapted to use the ClusterConfig enum. #5827 should also not wait much longer; and, actually, it will fix quite a lot of system index bugs.