Fix IllegalArgumentException when resolved indices are empty

[Mmuzaf]

Description

[Describe what this change achieves]

• Category (Enhancement, New feature, Bug fix, Test fix, Refactoring, Maintenance, Documentation)
• Why these changes are required?
• What is the old behavior before changes and new behavior after changes?

Issues Resolved

Resolves #5794

Is this a backport? If so, please add backport PR # and/or commits #, and remove backport-failed label from the original PR.

Do these changes introduce new permission(s) to be displayed in the static dropdown on the front-end? If so, please open a draft PR in the security dashboards plugin and link the draft PR here

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

That looks quite good to me, thank you!

Just a few things:

• We should first target the main branch; we can then backport to 2.19
• You need to sign off your commit (this will then fix the DCO check). See https://github.com/opensearch-project/.github/blob/main/CONTRIBUTING.md
• It's not a blocker for me, but it might be worthwhile to also have an integration test for this (or at least a quick manual test), as the index resolution process actually has sometimes a few unexpected pecularities (we are working on fixes for these in Introduced explicit index resolution API #5399 ) Integration tests are in https://github.com/opensearch-project/security/blob/main/src/integrationTest/java/org/opensearch/security/privileges/int_tests/IndexAuthorizationReadOnlyIntTests.java (it is quite a complicated beast though, I can help if needed).

[Mmuzaf]

@nibix during the implementation of the integration test, I encountered another edge case: if there are no indices, and users do not have any indices priveleges for the action at all, such requests should be also forbidden. To check this edge case I've added - a new method hasAnyIndexPrivilegeForAction. Could you please take a look if this is correct?

You've also mentioned IndexAuthorizationReadOnlyIntTests.java to add a new ingegration test there. I've added new IndexAuthorizationWithClosedIndicesIntTests instead for the following reasons:

• we close all indices (including hidden ones) in order to perform a test on our specific case. In my opinion, this operation does not align with the read-only test as closing indices affects the whole IndexAuthorizationReadOnlyIntTests suite state;
• adding new users to the USERS for our specific case in IndexAuthorizationReadOnlyIntTests suite affects all the tests in this suite due to the whole suite is parametrized, I thinks it's better to avoid it in our case;
• we may want to expand test coverage for another issues when all indexes are closed;

wdyt?

[nibix]

@Mmuzaf

Thanks for the update! On Friday, I will be quite busy with other stuff, so I will only have time to take a closer look on Monday.

if there are no indices, and users do not have any indices priveleges for the action at all, such requests should be also forbidden.

We need to take a closer look at this:

• I believe that this behavior is the case also for all other index actions. If this is a fundamentally exposed behavior, we will likely have to mantain backwards compatiblity
• For remote index operations, such a behavior is actually required. It is even possible that there are coordinator clusters with no local indices at all. In such cases it is expected that no local privileges are required.

Of course, this is a discussable topic - at the moment, we are working on some fundamental behavior changes (see #3905, #5367 and #5399 ; I also need to write an updated RFC soon). Feel invited to discuss there.

[nibix]

@Mmuzaf

Sorry for the late reply, was just too busy :-(

That looks generally very good to me! Yet, I think we need to briefly reflect on this:

if there are no indices, and users do not have any indices priveleges for the action at all, such requests should be also forbidden

If I see this correctly, this would have the following effect:

• Doing a GET /a*,remote_cluster:b/_search would be allowed even if the local user has no local privileges for search if a* does not match any indices. That's due to the condition here:

  security/src/main/java/org/opensearch/security/privileges/actionlevel/RuntimeOptimizedActionPrivileges.java

  Line 94 in 91a7097

  if (!resolvedIndices.isLocalAll() && resolvedIndices.getAllIndices().isEmpty()) {

• Doing a GET /*,remote_cluster:b/_search would be then forbidden however as this goes through the new gate this.index.hasAnyIndexPrivilegeForAction(context, actions)

This is a kind of inconsistent behavior which we should avoid IMHO (even though it might be a case that seldomly happens in the real word).

For now, I would propose not to do the hasAnyIndexPrivilegeForAction() check to stay in sync with the existing behavior.

At the moment, we are busy on revising the index authorization behavior, see #5814. We could use that chance and implement the behavior change in that go. Feel invited to have a look there and comment :)

@cwperks wdyt?