Address code review comments

Signed-off-by: Craig Perkins <[email protected]>
opensearch-project · Dec 31, 2024 · fbd8882 · fbd8882
1 parent 657eb3f
commit fbd8882
Show file tree

Hide file tree

Showing 28 changed files with 147 additions and 159 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,21 +19,22 @@
 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;
 import org.opensearch.test.framework.cluster.TestRestClient;
 import org.opensearch.test.framework.cluster.TestRestClient.HttpResponse;
+import org.opensearch.test.framework.matcher.RestMatchers;
 
 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;
 
@@ -109,11 +110,11 @@ public void testPluginShouldNotBeAbleToIndexDocumentIntoSystemIndexRegisteredByO
         try (TestRestClient client = cluster.getRestClient(USER_ADMIN)) {
             HttpResponse response = client.put("try-create-and-index/" + SYSTEM_INDEX_2);
 
-            assertThat(response.getStatusCode(), equalTo(RestStatus.FORBIDDEN.getStatus()));
             assertThat(
-                response.getBody(),
-                containsString(
-                    "no permissions for [indices:admin/create] and User [name=plugin:org.opensearch.security.plugin.SystemIndexPlugin1"
+                response,
+                RestMatchers.isForbidden(
+                    "/error/root_cause/0/reason",
+                    "no permissions for [] and User [name=plugin:org.opensearch.security.plugin.SystemIndexPlugin1"
                 )
             );
         }
@@ -124,8 +125,7 @@ public void testPluginShouldBeAbleToCreateSystemIndexButUserShouldNotBeAbleToInd
         try (TestRestClient client = cluster.getRestClient(USER_ADMIN)) {
             HttpResponse response = client.put("try-create-and-index/" + SYSTEM_INDEX_1 + "?runAs=user");
 
-            assertThat(response.getStatusCode(), equalTo(RestStatus.FORBIDDEN.getStatus()));
-            assertThat(response.getBody(), containsString("no permissions for [indices:data/write/index] and User [name=admin"));
+            assertThat(response, RestMatchers.isForbidden("/error/root_cause/0/reason", "no permissions for [] and User [admin"));
         }
     }
 
@@ -134,11 +134,11 @@ public void testPluginShouldNotBeAbleToRunClusterActions() {
         try (TestRestClient client = cluster.getRestClient(USER_ADMIN)) {
             HttpResponse response = client.get("try-cluster-health/plugin");
 
-            assertThat(response.getStatusCode(), equalTo(RestStatus.FORBIDDEN.getStatus()));
             assertThat(
-                response.getBody(),
-                containsString(
-                    "no permissions for [cluster:monitor/health] and User [name=plugin:org.opensearch.security.plugin.SystemIndexPlugin1"
+                response,
+                RestMatchers.isForbidden(
+                    "/error/root_cause/0/reason",
+                    "no permissions for [] and User [name=plugin:org.opensearch.security.plugin.SystemIndexPlugin1"
                 )
             );
         }
@@ -182,9 +182,7 @@ public void testPluginShouldNotBeAbleToBulkIndexDocumentIntoMixOfSystemIndexWher
 
             assertThat(
                 response.getBody(),
-                containsString(
-                    "no permissions for [indices:data/write/bulk[s]] and User [name=plugin:org.opensearch.security.plugin.SystemIndexPlugin1"
-                )
+                containsString("no permissions for [] and User [name=plugin:org.opensearch.security.plugin.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;

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;
 
@@ -26,12 +26,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/integrationTest/java/org/opensearch/test/framework/matcher/RestMatchers.java b/src/integrationTest/java/org/opensearch/test/framework/matcher/RestMatchers.java
@@ -0,0 +1,68 @@
+/*
+ * Copyright OpenSearch Contributors
+ * 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.
+ *
+ */
+package org.opensearch.test.framework.matcher;
+
+import org.hamcrest.Description;
+import org.hamcrest.DiagnosingMatcher;
+
+import org.opensearch.test.framework.cluster.TestRestClient.HttpResponse;
+
+public class RestMatchers {
+
+    private RestMatchers() {}
+
+    public static DiagnosingMatcher<HttpResponse> isForbidden(String jsonPointer, String patternString) {
+        return new DiagnosingMatcher<HttpResponse>() {
+
+            @Override
+            public void describeTo(Description description) {
+                description.appendText("Response has status 403 Forbidden with a JSON response that has the value ")
+                    .appendValue(patternString)
+                    .appendText(" at ")
+                    .appendValue(jsonPointer);
+            }
+
+            @Override
+            protected boolean matches(Object item, Description mismatchDescription) {
+                if (!(item instanceof HttpResponse)) {
+                    mismatchDescription.appendValue(item).appendText(" is not a HttpResponse");
+                    return false;
+                }
+
+                HttpResponse response = (HttpResponse) item;
+
+                if (response.getStatusCode() != 403) {
+                    mismatchDescription.appendText("Status is not 403 Forbidden: ").appendText("\n").appendValue(item);
+                    return false;
+                }
+
+                try {
+                    String value = response.getTextFromJsonBody(jsonPointer);
+
+                    if (value == null) {
+                        mismatchDescription.appendText("Could not find value at " + jsonPointer).appendText("\n").appendValue(item);
+                        return false;
+                    }
+
+                    if (value.contains(patternString)) {
+                        return true;
+                    } else {
+                        mismatchDescription.appendText("Value at " + jsonPointer + " does not match pattern: " + patternString + "\n")
+                            .appendValue(item);
+                        return false;
+                    }
+                } catch (Exception e) {
+                    mismatchDescription.appendText("Parsing request body failed with " + e).appendText("\n").appendValue(item);
+                    return false;
+                }
+            }
+        };
+    }
+}
diff --git a/src/main/java/org/opensearch/security/auth/BackendRegistry.java b/src/main/java/org/opensearch/security/auth/BackendRegistry.java
@@ -391,16 +391,11 @@ public boolean authenticate(final SecurityRequestChannel request) {
 
         if (authenticated) {
             final User impersonatedUser = impersonate(request, authenticatedUser);
-            threadPool.getThreadContext()
-                .putTransient(ConfigConstants.OPENDISTRO_SECURITY_USER, impersonatedUser == null ? authenticatedUser : impersonatedUser);
-            UserSubject subject = new SecurityUser(threadPool, impersonatedUser == null ? authenticatedUser : impersonatedUser);
+            final User effectiveUser = impersonatedUser == null ? authenticatedUser : impersonatedUser;
+            threadPool.getThreadContext().putTransient(ConfigConstants.OPENDISTRO_SECURITY_USER, effectiveUser);
+            UserSubject subject = new SecurityUser(threadPool, effectiveUser);
             threadPool.getThreadContext().putPersistent(ConfigConstants.OPENDISTRO_SECURITY_AUTHENTICATED_USER, subject);
-            auditLog.logSucceededLogin(
-                (impersonatedUser == null ? authenticatedUser : impersonatedUser).getName(),
-                false,
-                authenticatedUser.getName(),
-                request
-            );
+            auditLog.logSucceededLogin(effectiveUser.getName(), false, authenticatedUser.getName(), request);
         } else {
             if (isDebugEnabled) {
                 log.debug("User still not authenticated after checking {} auth domains", restAuthDomains.size());

diff --git a/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluatorResponse.java b/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluatorResponse.java
@@ -47,10 +47,6 @@ public Set<String> getMissingPrivileges() {
         return new HashSet<String>(missingPrivileges);
     }
 
-    public boolean addMissingPrivileges(String action) {
-        return missingPrivileges.add(action);
-    }
-
     public Set<String> getMissingSecurityRoles() {
         return new HashSet<>(missingSecurityRoles);
     }

diff --git a/src/main/java/org/opensearch/security/privileges/SnapshotRestoreEvaluator.java b/src/main/java/org/opensearch/security/privileges/SnapshotRestoreEvaluator.java
@@ -109,7 +109,6 @@ public PrivilegesEvaluatorResponse evaluate(
             auditLog.logSecurityIndexAttempt(request, action, task);
             log.warn("{} for '{}' as source index is not allowed", action, securityIndex);
             presponse.allowed = false;
-            presponse.addMissingPrivileges(action);
             return presponse.markComplete();
         }
         return presponse;
-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 @@