Respond to PR feedback

Signed-off-by: Craig Perkins <[email protected]>
cwperks · Dec 30, 2024 · 1f7417c · 1f7417c
1 parent fce023a
commit 1f7417c
Show file tree

Hide file tree

Showing 19 changed files with 34 additions and 70 deletions.
diff --git a/...opensearch/security/SystemIndexTests.java → ...ecurity/systemindex/SystemIndexTests.java b/...opensearch/security/SystemIndexTests.java → ...ecurity/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"
+                )
             );
         }
     }

diff --git a/...n/IndexDocumentIntoSystemIndexAction.java → ...n/IndexDocumentIntoSystemIndexAction.java b/...n/IndexDocumentIntoSystemIndexAction.java → ...n/IndexDocumentIntoSystemIndexAction.java
@@ -8,7 +8,7 @@
  *
  */
 
-package org.opensearch.security.plugin;
+package org.opensearch.security.systemindex.sampleplugin;
 
 import org.opensearch.action.ActionType;
 

diff --git a/.../IndexDocumentIntoSystemIndexRequest.java → .../IndexDocumentIntoSystemIndexRequest.java b/.../IndexDocumentIntoSystemIndexRequest.java → .../IndexDocumentIntoSystemIndexRequest.java
@@ -8,7 +8,7 @@
  *
  */
 
-package org.opensearch.security.plugin;
+package org.opensearch.security.systemindex.sampleplugin;
 
 import java.io.IOException;
 

diff --git a/...IndexDocumentIntoSystemIndexResponse.java → ...IndexDocumentIntoSystemIndexResponse.java b/...IndexDocumentIntoSystemIndexResponse.java → ...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;

diff --git a/...urity/identity/PluginContextSwitcher.java → ...x/sampleplugin/PluginContextSwitcher.java b/...urity/identity/PluginContextSwitcher.java → ...x/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) {

diff --git a/...exDocumentIntoMixOfSystemIndexAction.java → ...exDocumentIntoMixOfSystemIndexAction.java b/...exDocumentIntoMixOfSystemIndexAction.java → ...exDocumentIntoMixOfSystemIndexAction.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 {
 

diff --git a/...lkIndexDocumentIntoSystemIndexAction.java → ...lkIndexDocumentIntoSystemIndexAction.java b/...lkIndexDocumentIntoSystemIndexAction.java → ...lkIndexDocumentIntoSystemIndexAction.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;

diff --git a/...stIndexDocumentIntoSystemIndexAction.java → ...stIndexDocumentIntoSystemIndexAction.java b/...stIndexDocumentIntoSystemIndexAction.java → ...stIndexDocumentIntoSystemIndexAction.java
@@ -8,7 +8,7 @@
  *
  */
 
-package org.opensearch.security.plugin;
+package org.opensearch.security.systemindex.sampleplugin;
 
 import java.util.List;
 

diff --git a/...ty/plugin/RestRunClusterHealthAction.java → ...pleplugin/RestRunClusterHealthAction.java b/...ty/plugin/RestRunClusterHealthAction.java → ...pleplugin/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;

diff --git a/...curity/plugin/RunClusterHealthAction.java → .../sampleplugin/RunClusterHealthAction.java b/...curity/plugin/RunClusterHealthAction.java → .../sampleplugin/RunClusterHealthAction.java
@@ -8,7 +8,7 @@
  *
  */
 
-package org.opensearch.security.plugin;
+package org.opensearch.security.systemindex.sampleplugin;
 
 import org.opensearch.action.ActionType;
 

diff --git a/...urity/plugin/RunClusterHealthRequest.java → ...sampleplugin/RunClusterHealthRequest.java b/...urity/plugin/RunClusterHealthRequest.java → ...sampleplugin/RunClusterHealthRequest.java
@@ -8,7 +8,7 @@
  *
  */
 
-package org.opensearch.security.plugin;
+package org.opensearch.security.systemindex.sampleplugin;
 
 import java.io.IOException;
 

diff --git a/...rity/plugin/RunClusterHealthResponse.java → ...ampleplugin/RunClusterHealthResponse.java b/...rity/plugin/RunClusterHealthResponse.java → ...ampleplugin/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;

diff --git a/...h/security/plugin/SystemIndexPlugin1.java → ...ndex/sampleplugin/SystemIndexPlugin1.java b/...h/security/plugin/SystemIndexPlugin1.java → ...ndex/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;
 

diff --git a/...h/security/plugin/SystemIndexPlugin2.java → ...ndex/sampleplugin/SystemIndexPlugin2.java b/...h/security/plugin/SystemIndexPlugin2.java → ...ndex/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;

diff --git a/...rtIndexDocumentIntoSystemIndexAction.java → ...rtIndexDocumentIntoSystemIndexAction.java b/...rtIndexDocumentIntoSystemIndexAction.java → ...rtIndexDocumentIntoSystemIndexAction.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;

diff --git a/...ugin/TransportRunClusterHealthAction.java → ...ugin/TransportRunClusterHealthAction.java b/...ugin/TransportRunClusterHealthAction.java → ...ugin/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;

diff --git a/src/main/java/org/opensearch/security/auth/BackendRegistry.java b/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;

diff --git a/src/main/java/org/opensearch/security/privileges/SystemIndexAccessEvaluator.java b/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()

diff --git a/src/test/java/org/opensearch/security/identity/ContextProvidingPluginSubjectTests.java b/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));
-    }
 }
-Original file line number
+Diff line change
@@ Expand Up / @@ -8,7 +8,7 @@ @@
      *
      */
-    package org.opensearch.security.plugin;
+    package org.opensearch.security.systemindex.sampleplugin;
     import org.opensearch.action.ActionType;
@@ Expand Down @@