Fix IllegalArgumentException when resolved indices are empty

CHANGELOG.md

@@ -34,6 +34,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 - Updates DlsFlsValveImpl condition to return true if request is internal and not a protected resource request ([#5721](https://github.com/opensearch-project/security/pull/5721))
 - [Performance] Call AdminDns.isAdmin once per request ([#5752](https://github.com/opensearch-project/security/pull/5752))
 - Update operations on `.kibana` system index now work correctly with Dashboards multi tenancy enabled. ([#5778](https://github.com/opensearch-project/security/pull/5778))
+- Fix IllegalArgumentException when resolved indices are empty in PrivilegesEvaluator ([#5770](https://github.com/opensearch-project/security/pull/5797)
 
 ### Refactoring
 - [Resource Sharing] Make migrate api require default access level to be supplied and updates documentations + tests ([#5717](https://github.com/opensearch-project/security/pull/5717))

src/integrationTest/java/org/opensearch/security/privileges/actionlevel/RoleBasedActionPrivilegesTest.java

@@ -30,6 +30,7 @@
 import org.junit.runners.Suite;
 
 import org.opensearch.action.support.IndicesOptions;
+import org.opensearch.cluster.ClusterState;
 import org.opensearch.cluster.metadata.IndexAbstraction;
 import org.opensearch.cluster.metadata.IndexMetadata;
 import org.opensearch.cluster.metadata.Metadata;
@@ -1034,6 +1035,30 @@ public void statefulDisabled() throws Exception {
             subject.updateStatefulIndexPrivileges(metadata, 1);
             assertEquals(0, subject.getEstimatedStatefulIndexByteSize());
         }
+
+        /**
+         * Tests the behavior of hasIndexPrivilege when the resolved indices are empty.
+         * @throws Exception If failed.
+         */
+        @Test
+        public void hasIndexPrivilegeEmptyResolvedIndices() throws Exception {
+            SecurityDynamicConfiguration<RoleV7> roles = SecurityDynamicConfiguration.fromYaml(
+                "test_role:\n"
+                    + "  index_permissions:\n"
+                    + "  - index_patterns: ['test_index*']\n"
+                    + "    allowed_actions: ['indices:monitor/recovery']",
+                CType.ROLES
+            );
+
+            RoleBasedActionPrivileges subject = new RoleBasedActionPrivileges(roles, FlattenedActionGroups.EMPTY, Settings.EMPTY);
+
+            PrivilegesEvaluatorResponse result = subject.hasIndexPrivilege(
+                ctx().clusterState(ClusterState.EMPTY_STATE).roles("test_role").get(),
+                ImmutableSet.of("indices:monitor/recovery"),
+                IndexResolverReplacer.Resolved._LOCAL_ALL
+            );
+            assertThat(result, isAllowed());
+        }
     }
 
     /**

src/integrationTest/java/org/opensearch/security/privileges/int_tests/IndexAuthorizationWithClosedIndicesIntTests.java

@@ -0,0 +1,216 @@
+/*
+ * SPDX-License-Identifier: Apache-2.0
+ *
+ * The OpenSearch Contributors require contributions made to
+ * this file be licensed under the Apache-2.0 license or a
+ * compatible open source license.
+ *
+ * Modifications Copyright OpenSearch Contributors. See
+ * GitHub history for details.
+ */
+
+package org.opensearch.security.privileges.int_tests;
+
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.List;
+
+import com.carrotsearch.randomizedtesting.annotations.ParametersFactory;
+import com.carrotsearch.randomizedtesting.annotations.ThreadLeakScope;
+import com.google.common.collect.ImmutableList;
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+
+import org.opensearch.script.mustache.MustacheModulePlugin;
+import org.opensearch.test.framework.TestSecurityConfig;
+import org.opensearch.test.framework.cluster.LocalCluster;
+import org.opensearch.test.framework.cluster.TestRestClient;
+import org.opensearch.test.framework.data.TestIndex;
+
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.opensearch.test.framework.TestSecurityConfig.AuthcDomain.AUTHC_HTTPBASIC_INTERNAL;
+import static org.opensearch.test.framework.matcher.RestIndexMatchers.IndexMatcher;
+import static org.opensearch.test.framework.matcher.RestIndexMatchers.OnUserIndexMatcher.limitedToNone;
+import static org.opensearch.test.framework.matcher.RestMatchers.isForbidden;
+import static org.opensearch.test.framework.matcher.RestMatchers.isOk;
+
+/**
+ * Integration tests for index authorization when all indices are closed.
+ * Tests operations that should work even when getAllIndicesResolved() returns empty.
+ * This test suite verifies that operations like _cat/recovery can still be authorized
+ * correctly when all indices are closed, which results in an empty resolved indices set.
+ * <p>
+ * We need this dedicated test suite because closing indices triggers cluster state changes,
+ * affecting some threads and losing access to the RandomizedContext. Therefore, we isolate
+ * these tests to avoid interference with other tests.
+ */
+@RunWith(com.carrotsearch.randomizedtesting.RandomizedRunner.class)
+@ThreadLeakScope(ThreadLeakScope.Scope.NONE)
+public class IndexAuthorizationWithClosedIndicesIntTests {
+
+    private static final Logger log = LogManager.getLogger(IndexAuthorizationWithClosedIndicesIntTests.class);
+
+    // -------------------------------------------------------------------------------------------------------
+    // Test indices used by this test suite
+    // -------------------------------------------------------------------------------------------------------
+
+    static final TestIndex index_a1 = TestIndex.name("index_a1").documentCount(100).seed(1).build();
+    static final TestIndex index_a2 = TestIndex.name("index_a2").documentCount(110).seed(2).build();
+    static final TestIndex index_b1 = TestIndex.name("index_b1").documentCount(51).seed(3).build();
+    static final TestIndex index_b2 = TestIndex.name("index_b2").documentCount(52).seed(4).build();
+
+    /**
+     * This key identifies assertion reference data for index search/read permissions of individual users.
+     */
+    static final TestSecurityConfig.User.MetadataKey<IndexMatcher> READ = new TestSecurityConfig.User.MetadataKey<>(
+        "read",
+        IndexMatcher.class
+    );
+
+    /**
+     * A user with indices:monitor/recovery permission on all indices to verify that it succeeds in case all indices are closed.
+     */
+    static final TestSecurityConfig.User LIMITED_USER_RECOVERY = new TestSecurityConfig.User("limited_user_recovery").description(
+        "indices:monitor/recovery on *"
+    )
+        .roles(
+            new TestSecurityConfig.Role("r1").clusterPermissions("cluster_composite_ops_ro", "cluster_monitor")
+                .indexPermissions("indices:monitor/recovery")
+                .on("*")
+        )
+        .reference(READ, limitedToNone());
+
+    /**
+     * A user with indices:monitor/recovery permission on specific indices (not wildcard) to test empty resolved indices handling.
+     * This user is crucial for testing the empty resolved indices scenario because it doesn't have wildcard privileges,
+     * which means checkWildcardIndexPrivilegesOnWellKnownActions will return null, forcing execution to reach
+     * the CheckTable.create() call with empty allIndicesResolved.
+     */
+    static final TestSecurityConfig.User LIMITED_USER_RECOVERY_SPECIFIC = new TestSecurityConfig.User("limited_user_recovery_specific")
+        .description("indices:monitor/recovery on index_a*")
+        .roles(
+            new TestSecurityConfig.Role("r1").clusterPermissions("cluster_composite_ops_ro", "cluster_monitor")
+                .indexPermissions("indices:monitor/recovery")
+                .on("index_a*")
+        )
+        .reference(READ, limitedToNone());
+
+    /**
+     * A user with "*" privileges on "*"; as it is a regular user, they are still subject to system index
+     * restrictions and similar things.
+     */
+    static final TestSecurityConfig.User UNLIMITED_USER = new TestSecurityConfig.User("unlimited_user")//
+        .description("unlimited")
+        .roles(new TestSecurityConfig.Role("r1").clusterPermissions("*").indexPermissions("*").on("*"))
+        .reference(READ, limitedToNone());
+
+    /**
+     * The SUPER_UNLIMITED_USER authenticates with an admin cert, which will cause all access control code to be skipped.
+     * This serves as a base for comparison with the default behavior.
+     */
+    static final TestSecurityConfig.User SUPER_UNLIMITED_USER = new TestSecurityConfig.User("super_unlimited_user")//
+        .description("super unlimited (admin cert)")
+        .adminCertUser()
+        .reference(READ, limitedToNone());
+
+    /**
+     * A user with no index privileges to test that operations are properly denied.
+     */
+    static final TestSecurityConfig.User LIMITED_USER_NONE = new TestSecurityConfig.User("limited_user_none").description(
+        "no index privileges"
+    )
+        .roles(new TestSecurityConfig.Role("r2").clusterPermissions("cluster_composite_ops_ro", "cluster_monitor"))
+        .reference(READ, limitedToNone());
+
+    static final List<TestSecurityConfig.User> USERS = ImmutableList.of(
+        LIMITED_USER_NONE,
+        LIMITED_USER_RECOVERY,
+        LIMITED_USER_RECOVERY_SPECIFIC,
+        SUPER_UNLIMITED_USER,
+        UNLIMITED_USER
+    );
+
+    static LocalCluster.Builder clusterBuilder() {
+        return new LocalCluster.Builder().singleNode()
+            .authc(AUTHC_HTTPBASIC_INTERNAL)
+            .users(USERS)
+            .indices(index_a1, index_a2, index_b1, index_b2)
+            .plugin(MustacheModulePlugin.class);
+    }
+
+    private final TestSecurityConfig.User user;
+    private final LocalCluster cluster;
+    private final ClusterConfig clusterConfig;
+
+    @ParametersFactory(shuffle = false, argumentFormatting = "%1$s, %3$s")
+    public static Collection<Object[]> params() {
+        List<Object[]> result = new ArrayList<>();
+
+        for (ClusterConfig clusterConfig : ClusterConfig.values()) {
+            for (TestSecurityConfig.User user : USERS) {
+                result.add(new Object[] { clusterConfig, user, user.getDescription() });
+            }
+        }
+        return result;
+    }
+
+    public IndexAuthorizationWithClosedIndicesIntTests(ClusterConfig clusterConfig, TestSecurityConfig.User user, String description)
+        throws Exception {
+        this.user = user;
+        this.cluster = clusterConfig.cluster(IndexAuthorizationWithClosedIndicesIntTests::clusterBuilder);
+        this.clusterConfig = clusterConfig;
+    }
+
+    @Before
+    public void setup() {
+        // In order to test index authorization when all indices are closed, we close all indices, including hidden ones.
+        try (TestRestClient adminClient = cluster.getAdminCertRestClient()) {
+            TestRestClient.HttpResponse hiddenCloseResponse = adminClient.post("_all/_close?expand_wildcards=all");
+            assertThat(hiddenCloseResponse, isOk());
+        }
+    }
+
+    @After
+    public void teardown() {
+        try (TestRestClient adminClient = cluster.getAdminCertRestClient()) {
+            try {
+                adminClient.post("_all/_open?expand_wildcards=all");
+            } catch (Exception e) {
+                log.warn("Error reopening all indices during teardown", e);
+            }
+        }
+    }
+
+    /**
+     * Tests _cat/recovery operation succeeds when all indices are closed. This verifies that
+     * the empty resolved indices check in RuntimeOptimizedActionPrivileges works correctly.
+     */
+    @Test
+    public void cat_recovery_allIndicesClosed() throws Exception {
+        try (TestRestClient restClient = cluster.getRestClient(user)) {
+            TestRestClient.HttpResponse httpResponse = restClient.get("_cat/recovery");
+
+            if (user == UNLIMITED_USER || user == SUPER_UNLIMITED_USER) {
+                assertThat(httpResponse, isOk());
+            } else if (user == LIMITED_USER_RECOVERY) {
+                // This user has indices:monitor/recovery on wildcard "*".
+                // checkWildcardIndexPrivilegesOnWellKnownActions() == null should handle this.
+                assertThat(httpResponse, isOk());
+            } else if (user == LIMITED_USER_RECOVERY_SPECIFIC) {
+                // This user has indices:monitor/recovery on index_a* but not wildcard.
+                // When all indices are closed, getAllIndicesResolved() returns empty,
+                // then we must ensure that the empty resolved indices case is handled correctly.
+                assertThat(httpResponse, isOk());
+            } else if (user == LIMITED_USER_NONE) {
+                // This user has no permission at the same time as all indices are closed.
+                assertThat(httpResponse, isForbidden("/error/root_cause/0/reason", "no permissions for [indices:monitor/recovery]"));
+            } else {
+                assertThat(httpResponse, isForbidden("/error/root_cause/0/reason", "no permissions for [indices:monitor/recovery]"));
+            }
+        }
+    }
+}

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

@@ -98,12 +98,19 @@ public PrivilegesEvaluatorResponse hasIndexPrivilege(
             return PrivilegesEvaluatorResponse.ok();
         }
 
+        Set<String> allIndicesResolved = resolvedIndices.getAllIndicesResolved(
+            context.getClusterStateSupplier(),
+            context.getIndexNameExpressionResolver()
+        );
+
+        if (allIndicesResolved.isEmpty()) {
+            log.debug("No resolved indices; grant the request (user has index privileges for the action)");
+            return PrivilegesEvaluatorResponse.ok();
+        }
+
         // TODO one might want to consider to create a semantic wrapper for action in order to be better tell apart
         // what's the action and what's the index in the generic parameters of CheckTable.
-        CheckTable<String, String> checkTable = CheckTable.create(
-            resolvedIndices.getAllIndicesResolved(context.getClusterStateSupplier(), context.getIndexNameExpressionResolver()),
-            actions
-        );
+        CheckTable<String, String> checkTable = CheckTable.create(allIndicesResolved, actions);
 
         StatefulIndexPrivileges statefulIndex = this.currentStatefulIndexPrivileges();
         PrivilegesEvaluatorResponse resultFromStatefulIndex = null;