Address code review feedback

Signed-off-by: Craig Perkins <[email protected]>
opensearch-project · Sep 6, 2024 · 565fa20 · 565fa20
1 parent dc8868e
commit 565fa20
Show file tree

Hide file tree

Showing 3 changed files with 36 additions and 41 deletions.
diff --git a/src/integrationTest/java/org/opensearch/security/SystemIndexTests.java b/src/integrationTest/java/org/opensearch/security/SystemIndexTests.java
@@ -35,7 +35,6 @@
 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.support.ConfigConstants.SECURITY_SYSTEM_INDICES_PERMISSIONS_ENABLED_KEY;
 import static org.opensearch.test.framework.TestSecurityConfig.Role.ALL_ACCESS;
 import static org.opensearch.test.framework.TestSecurityConfig.User.USER_ADMIN;
 
@@ -50,13 +49,11 @@ public class SystemIndexTests {
         .anonymousAuth(false)
         .authc(AUTHC_DOMAIN)
         .users(USER_ADMIN)
-        .plugin(List.of(SystemIndexPlugin1.class, SystemIndexPlugin2.class))
+        .plugin(SystemIndexPlugin1.class, SystemIndexPlugin2.class)
         .nodeSettings(
             Map.of(
                 FeatureFlags.IDENTITY,
                 true,
-                SECURITY_SYSTEM_INDICES_PERMISSIONS_ENABLED_KEY,
-                true,
                 SECURITY_RESTAPI_ROLES_ENABLED,
                 List.of("user_" + USER_ADMIN.getName() + "__" + ALL_ACCESS.getName()),
                 SECURITY_SYSTEM_INDICES_ENABLED_KEY,
@@ -88,10 +85,15 @@ public void adminShouldNotBeAbleToDeleteSecurityIndex() {
 
             assertThat(response2.getStatusCode(), equalTo(RestStatus.OK.getStatus()));
 
-            // regular use cannot create system index when system index protection is enforced
+            // regular user can create system index
             HttpResponse response3 = client.put(".system-index1");
 
-            assertThat(response3.getStatusCode(), equalTo(RestStatus.FORBIDDEN.getStatus()));
+            assertThat(response3.getStatusCode(), equalTo(RestStatus.OK.getStatus()));
+
+            // regular user cannot delete system index
+            HttpResponse response4 = client.delete(".system-index1");
+
+            assertThat(response4.getStatusCode(), equalTo(RestStatus.FORBIDDEN.getStatus()));
         }
     }
 

diff --git a/src/integrationTest/java/org/opensearch/test/framework/cluster/LocalCluster.java b/src/integrationTest/java/org/opensearch/test/framework/cluster/LocalCluster.java
@@ -378,20 +378,12 @@ public Builder nodeSettings(Map<String, Object> settings) {
             return this;
         }
 
-        /**
-        * Adds additional plugin to the cluster
-        */
-        public Builder plugin(Class<? extends Plugin> plugin) {
-            this.plugins.add(plugin);
-
-            return this;
-        }
-
         /**
          * Adds additional plugins to the cluster
          */
-        public Builder plugin(List<Class<? extends Plugin>> plugins) {
-            this.plugins.addAll(plugins);
+        @SafeVarargs
+        public final Builder plugin(Class<? extends Plugin>... plugins) {
+            this.plugins.addAll(List.of(plugins));
 
             return this;
         }

diff --git a/src/main/java/org/opensearch/security/privileges/SystemIndexAccessEvaluator.java b/src/main/java/org/opensearch/security/privileges/SystemIndexAccessEvaluator.java
@@ -277,30 +277,6 @@ private void evaluateSystemIndicesAccess(
                 return;
             }
             boolean containsProtectedIndex = requestContainsAnyProtectedSystemIndices(requestedResolved);
-            if (user.isPluginUser()) {
-                Set<String> matchingSystemIndices = SystemIndexRegistry.matchesPluginSystemIndexPattern(
-                    user.getName(),
-                    requestedResolved.getAllIndices()
-                );
-                if (requestedResolved.getAllIndices().equals(matchingSystemIndices)) {
-                    // plugin is authorized to perform any actions on its own registered system indices
-                    presponse.allowed = true;
-                    presponse.markComplete();
-                } else {
-                    if (log.isInfoEnabled()) {
-                        log.warn(
-                            "Plugin {} can only perform {} on it's own registered System Indices. System indices from request that match plugin's registered system indices: {}",
-                            user.getName(),
-                            action,
-                            matchingSystemIndices
-                        );
-                    }
-                    presponse.allowed = false;
-                    presponse.addMissingPrivileges(action);
-                    presponse.markComplete();
-                }
-                return;
-            }
 
             if (containsProtectedIndex) {
                 auditLog.logSecurityIndexAttempt(request, action, task);
@@ -340,6 +316,31 @@ private void evaluateSystemIndicesAccess(
                 }
         }
 
+        if (user.isPluginUser()) {
+            Set<String> matchingSystemIndices = SystemIndexRegistry.matchesPluginSystemIndexPattern(
+                user.getName(),
+                requestedResolved.getAllIndices()
+            );
+            if (requestedResolved.getAllIndices().equals(matchingSystemIndices)) {
+                // plugin is authorized to perform any actions on its own registered system indices
+                presponse.allowed = true;
+                presponse.markComplete();
+            } else {
+                if (log.isInfoEnabled()) {
+                    log.info(
+                        "Plugin {} can only perform {} on it's own registered System Indices. System indices from request that match plugin's registered system indices: {}",
+                        user.getName(),
+                        action,
+                        matchingSystemIndices
+                    );
+                }
+                presponse.allowed = false;
+                presponse.addMissingPrivileges(action);
+                presponse.markComplete();
+            }
+            return;
+        }
+
         if (isActionAllowed(action)) {
             if (requestedResolved.isLocalAll()) {
                 if (filterSecurityIndex) {