Respond to PR feedback

Signed-off-by: Craig Perkins <[email protected]>

Author: cwperks

src/integrationTest/java/org/opensearch/security/SystemIndexTests.java src/integrationTest/java/org/opensearch/security/systemindex/SystemIndexTests.java

@@ -7,7 +7,7 @@
  * compatible open source license.
  *
  */
-package org.opensearch.security;
+package org.opensearch.security.systemindex;
 
 import java.util.List;
 import java.util.Map;
@@ -19,8 +19,8 @@
 import org.junit.runner.RunWith;
 
 import org.opensearch.core.rest.RestStatus;
-import org.opensearch.security.plugin.SystemIndexPlugin1;
-import org.opensearch.security.plugin.SystemIndexPlugin2;
+import org.opensearch.security.systemindex.sampleplugin.SystemIndexPlugin1;
+import org.opensearch.security.systemindex.sampleplugin.SystemIndexPlugin2;
 import org.opensearch.test.framework.TestSecurityConfig.AuthcDomain;
 import org.opensearch.test.framework.cluster.ClusterManager;
 import org.opensearch.test.framework.cluster.LocalCluster;
@@ -30,10 +30,10 @@
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.containsString;
 import static org.hamcrest.Matchers.equalTo;
-import static org.opensearch.security.plugin.SystemIndexPlugin1.SYSTEM_INDEX_1;
-import static org.opensearch.security.plugin.SystemIndexPlugin2.SYSTEM_INDEX_2;
 import static org.opensearch.security.support.ConfigConstants.SECURITY_RESTAPI_ROLES_ENABLED;
 import static org.opensearch.security.support.ConfigConstants.SECURITY_SYSTEM_INDICES_ENABLED_KEY;
+import static org.opensearch.security.systemindex.sampleplugin.SystemIndexPlugin1.SYSTEM_INDEX_1;
+import static org.opensearch.security.systemindex.sampleplugin.SystemIndexPlugin2.SYSTEM_INDEX_2;
 import static org.opensearch.test.framework.TestSecurityConfig.Role.ALL_ACCESS;
 import static org.opensearch.test.framework.TestSecurityConfig.User.USER_ADMIN;
 
@@ -111,7 +111,9 @@ public void testPluginShouldNotBeAbleToIndexDocumentIntoSystemIndexRegisteredByO
             assertThat(response.getStatusCode(), equalTo(RestStatus.FORBIDDEN.getStatus()));
             assertThat(
                 response.getBody(),
-                containsString("no permissions for [] and User [name=plugin:org.opensearch.security.plugin.SystemIndexPlugin1")
+                containsString(
+                    "no permissions for [] and User [name=plugin:org.opensearch.security.systemindex.sampleplugin.SystemIndexPlugin1"
+                )
             );
         }
     }
@@ -134,7 +136,9 @@ public void testPluginShouldNotBeAbleToRunClusterActions() {
             assertThat(response.getStatusCode(), equalTo(RestStatus.FORBIDDEN.getStatus()));
             assertThat(
                 response.getBody(),
-                containsString("no permissions for [] and User [name=plugin:org.opensearch.security.plugin.SystemIndexPlugin1")
+                containsString(
+                    "no permissions for [cluster:monitor/health] and User [name=plugin:org.opensearch.security.systemindex.sampleplugin.SystemIndexPlugin1"
+                )
             );
         }
     }
@@ -177,7 +181,9 @@ public void testPluginShouldNotBeAbleToBulkIndexDocumentIntoMixOfSystemIndexWher
 
             assertThat(
                 response.getBody(),
-                containsString("no permissions for [] and User [name=plugin:org.opensearch.security.plugin.SystemIndexPlugin1")
+                containsString(
+                    "no permissions for [] and User [name=plugin:org.opensearch.security.systemindex.sampleplugin.SystemIndexPlugin1"
+                )
             );
         }
     }

src/integrationTest/java/org/opensearch/security/plugin/IndexDocumentIntoSystemIndexAction.java src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/IndexDocumentIntoSystemIndexAction.java

@@ -8,7 +8,7 @@
  *
  */
 
-package org.opensearch.security.plugin;
+package org.opensearch.security.systemindex.sampleplugin;
 
 import org.opensearch.action.ActionType;
 

src/integrationTest/java/org/opensearch/security/plugin/IndexDocumentIntoSystemIndexRequest.java src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/IndexDocumentIntoSystemIndexRequest.java

@@ -8,7 +8,7 @@
  *
  */
 
-package org.opensearch.security.plugin;
+package org.opensearch.security.systemindex.sampleplugin;
 
 import java.io.IOException;
 

src/integrationTest/java/org/opensearch/security/plugin/IndexDocumentIntoSystemIndexResponse.java src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/IndexDocumentIntoSystemIndexResponse.java

@@ -8,7 +8,7 @@
  *
  */
 
-package org.opensearch.security.plugin;
+package org.opensearch.security.systemindex.sampleplugin;
 
 // CS-SUPPRESS-SINGLE: RegexpSingleline It is not possible to use phrase "cluster manager" instead of master here
 import java.io.IOException;

src/main/java/org/opensearch/security/identity/PluginContextSwitcher.java src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/PluginContextSwitcher.java

@@ -8,7 +8,7 @@
  * Modifications Copyright OpenSearch Contributors. See
  * GitHub history for details.
  */
-package org.opensearch.security.identity;
+package org.opensearch.security.systemindex.sampleplugin;
 
 import java.util.Objects;
 import java.util.concurrent.Callable;
@@ -21,11 +21,11 @@ public class PluginContextSwitcher {
     public PluginContextSwitcher() {}
 
     public void initialize(PluginSubject pluginSubject) {
+        Objects.requireNonNull(pluginSubject);
         this.pluginSubject = pluginSubject;
     }
 
     public <T> T runAs(Callable<T> callable) {
-        Objects.requireNonNull(pluginSubject);
         try {
             return pluginSubject.runAs(callable);
         } catch (Exception e) {

src/integrationTest/java/org/opensearch/security/plugin/RestBulkIndexDocumentIntoMixOfSystemIndexAction.java src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/RestBulkIndexDocumentIntoMixOfSystemIndexAction.java

@@ -8,7 +8,7 @@
  *
  */
 
-package org.opensearch.security.plugin;
+package org.opensearch.security.systemindex.sampleplugin;
 
 import java.util.List;
 
@@ -25,12 +25,11 @@
 import org.opensearch.rest.BytesRestResponse;
 import org.opensearch.rest.RestChannel;
 import org.opensearch.rest.RestRequest;
-import org.opensearch.security.identity.PluginContextSwitcher;
 
 import static java.util.Collections.singletonList;
 import static org.opensearch.rest.RestRequest.Method.PUT;
-import static org.opensearch.security.plugin.SystemIndexPlugin1.SYSTEM_INDEX_1;
-import static org.opensearch.security.plugin.SystemIndexPlugin2.SYSTEM_INDEX_2;
+import static org.opensearch.security.systemindex.sampleplugin.SystemIndexPlugin1.SYSTEM_INDEX_1;
+import static org.opensearch.security.systemindex.sampleplugin.SystemIndexPlugin2.SYSTEM_INDEX_2;
 
 public class RestBulkIndexDocumentIntoMixOfSystemIndexAction extends BaseRestHandler {
 

src/integrationTest/java/org/opensearch/security/plugin/RestBulkIndexDocumentIntoSystemIndexAction.java src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/RestBulkIndexDocumentIntoSystemIndexAction.java

@@ -8,7 +8,7 @@
  *
  */
 
-package org.opensearch.security.plugin;
+package org.opensearch.security.systemindex.sampleplugin;
 
 import java.util.List;
 
@@ -27,7 +27,6 @@
 import org.opensearch.rest.BytesRestResponse;
 import org.opensearch.rest.RestChannel;
 import org.opensearch.rest.RestRequest;
-import org.opensearch.security.identity.PluginContextSwitcher;
 
 import static java.util.Collections.singletonList;
 import static org.opensearch.rest.RestRequest.Method.PUT;

src/integrationTest/java/org/opensearch/security/plugin/RestIndexDocumentIntoSystemIndexAction.java src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/RestIndexDocumentIntoSystemIndexAction.java

@@ -8,7 +8,7 @@
  *
  */
 
-package org.opensearch.security.plugin;
+package org.opensearch.security.systemindex.sampleplugin;
 
 import java.util.List;
 

src/integrationTest/java/org/opensearch/security/plugin/RestRunClusterHealthAction.java src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/RestRunClusterHealthAction.java

@@ -8,7 +8,7 @@
  *
  */
 
-package org.opensearch.security.plugin;
+package org.opensearch.security.systemindex.sampleplugin;
 
 import java.util.List;
 
@@ -17,7 +17,6 @@
 import org.opensearch.rest.BaseRestHandler;
 import org.opensearch.rest.RestRequest;
 import org.opensearch.rest.action.RestToXContentListener;
-import org.opensearch.security.identity.PluginContextSwitcher;
 
 import static java.util.Collections.singletonList;
 import static org.opensearch.rest.RestRequest.Method.GET;

src/integrationTest/java/org/opensearch/security/plugin/RunClusterHealthAction.java src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/RunClusterHealthAction.java

@@ -8,7 +8,7 @@
  *
  */
 
-package org.opensearch.security.plugin;
+package org.opensearch.security.systemindex.sampleplugin;
 
 import org.opensearch.action.ActionType;
 

src/integrationTest/java/org/opensearch/security/plugin/RunClusterHealthRequest.java src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/RunClusterHealthRequest.java

@@ -8,7 +8,7 @@
  *
  */
 
-package org.opensearch.security.plugin;
+package org.opensearch.security.systemindex.sampleplugin;
 
 import java.io.IOException;
 

src/integrationTest/java/org/opensearch/security/plugin/RunClusterHealthResponse.java src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/RunClusterHealthResponse.java

@@ -8,7 +8,7 @@
  *
  */
 
-package org.opensearch.security.plugin;
+package org.opensearch.security.systemindex.sampleplugin;
 
 // CS-SUPPRESS-SINGLE: RegexpSingleline It is not possible to use phrase "cluster manager" instead of master here
 import java.io.IOException;

src/integrationTest/java/org/opensearch/security/plugin/SystemIndexPlugin1.java src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/SystemIndexPlugin1.java

@@ -8,7 +8,7 @@
  *
  */
 
-package org.opensearch.security.plugin;
+package org.opensearch.security.systemindex.sampleplugin;
 
 import java.util.Arrays;
 import java.util.Collection;
@@ -39,7 +39,6 @@
 import org.opensearch.rest.RestController;
 import org.opensearch.rest.RestHandler;
 import org.opensearch.script.ScriptService;
-import org.opensearch.security.identity.PluginContextSwitcher;
 import org.opensearch.threadpool.ThreadPool;
 import org.opensearch.watcher.ResourceWatcherService;
 

src/integrationTest/java/org/opensearch/security/plugin/SystemIndexPlugin2.java src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/SystemIndexPlugin2.java

@@ -8,7 +8,7 @@
  *
  */
 
-package org.opensearch.security.plugin;
+package org.opensearch.security.systemindex.sampleplugin;
 
 import java.util.Collection;
 import java.util.Collections;

src/integrationTest/java/org/opensearch/security/plugin/TransportIndexDocumentIntoSystemIndexAction.java src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/TransportIndexDocumentIntoSystemIndexAction.java

@@ -8,7 +8,7 @@
  *
  */
 
-package org.opensearch.security.plugin;
+package org.opensearch.security.systemindex.sampleplugin;
 
 import org.opensearch.action.admin.indices.create.CreateIndexRequest;
 import org.opensearch.action.index.IndexRequest;
@@ -21,7 +21,6 @@
 import org.opensearch.core.action.ActionListener;
 import org.opensearch.identity.IdentityService;
 import org.opensearch.identity.Subject;
-import org.opensearch.security.identity.PluginContextSwitcher;
 import org.opensearch.security.support.ConfigConstants;
 import org.opensearch.security.user.User;
 import org.opensearch.tasks.Task;

src/integrationTest/java/org/opensearch/security/plugin/TransportRunClusterHealthAction.java src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/TransportRunClusterHealthAction.java

@@ -8,7 +8,7 @@
  *
  */
 
-package org.opensearch.security.plugin;
+package org.opensearch.security.systemindex.sampleplugin;
 
 import org.opensearch.action.admin.cluster.health.ClusterHealthRequest;
 import org.opensearch.action.admin.cluster.health.ClusterHealthResponse;
@@ -19,7 +19,6 @@
 import org.opensearch.core.action.ActionListener;
 import org.opensearch.identity.IdentityService;
 import org.opensearch.identity.Subject;
-import org.opensearch.security.identity.PluginContextSwitcher;
 import org.opensearch.tasks.Task;
 import org.opensearch.threadpool.ThreadPool;
 import org.opensearch.transport.TransportService;

src/main/java/org/opensearch/security/auth/BackendRegistry.java

@@ -226,7 +226,7 @@ public boolean authenticate(final SecurityRequestChannel request) {
             // PKI authenticated REST call
             User superuser = new User(sslPrincipal);
             UserSubject subject = new SecurityUser(threadPool, superuser);
-            threadPool.getThreadContext().putPersistent(ConfigConstants.OPENDISTRO_SECURITY_AUTHENTICATED_USER, subject);
+            threadContext.putPersistent(ConfigConstants.OPENDISTRO_SECURITY_AUTHENTICATED_USER, subject);
             threadContext.putTransient(ConfigConstants.OPENDISTRO_SECURITY_USER, superuser);
             auditLog.logSucceededLogin(sslPrincipal, true, null, request);
             return true;

src/main/java/org/opensearch/security/privileges/SystemIndexAccessEvaluator.java

@@ -302,7 +302,7 @@ private void evaluateSystemIndicesAccess(
                 }
         }
 
-        if (user.isPluginUser()) {
+        if (user.isPluginUser() && !requestedResolved.isLocalAll()) {
             Set<String> matchingSystemIndices = SystemIndexRegistry.matchesPluginSystemIndexPattern(
                 user.getName().replace("plugin:", ""),
                 requestedResolved.getAllIndices()

src/test/java/org/opensearch/security/identity/ContextProvidingPluginSubjectTests.java

@@ -25,7 +25,6 @@
 import static org.hamcrest.Matchers.equalTo;
 import static org.opensearch.security.support.ConfigConstants.OPENDISTRO_SECURITY_USER;
 import static org.junit.Assert.assertNull;
-import static org.junit.Assert.assertThrows;
 
 public class ContextProvidingPluginSubjectTests {
     static class TestIdentityAwarePlugin extends Plugin implements IdentityAwarePlugin {
@@ -57,39 +56,4 @@ public void testSecurityUserSubjectRunAs() throws Exception {
 
         SecurityUserTests.terminate(threadPool);
     }
-
-    @Test
-    public void testPluginContextSwitcherRunAs() throws Exception {
-        final ThreadPool threadPool = new TestThreadPool(getClass().getName());
-
-        final Plugin testPlugin = new TestIdentityAwarePlugin();
-
-        final PluginContextSwitcher contextSwitcher = new PluginContextSwitcher();
-
-        final String pluginPrincipal = "plugin:" + testPlugin.getClass().getCanonicalName();
-
-        final User pluginUser = new User(pluginPrincipal);
-
-        ContextProvidingPluginSubject subject = new ContextProvidingPluginSubject(threadPool, Settings.EMPTY, testPlugin);
-
-        contextSwitcher.initialize(subject);
-
-        assertNull(threadPool.getThreadContext().getTransient(OPENDISTRO_SECURITY_USER));
-
-        subject.runAs(() -> {
-            assertThat(threadPool.getThreadContext().getTransient(OPENDISTRO_SECURITY_USER), equalTo(pluginUser));
-            return null;
-        });
-
-        assertNull(threadPool.getThreadContext().getTransient(OPENDISTRO_SECURITY_USER));
-
-        SecurityUserTests.terminate(threadPool);
-    }
-
-    @Test
-    public void testPluginContextSwitcherUninitializedRunAs() throws Exception {
-        final PluginContextSwitcher contextSwitcher = new PluginContextSwitcher();
-
-        assertThrows(NullPointerException.class, () -> contextSwitcher.runAs(() -> null));
-    }
 }