Change calls to isPluginUser and create InMemorySecurityRoles

Signed-off-by: Craig Perkins <[email protected]>
opensearch-project · Sep 20, 2024 · e5855ab · e5855ab
1 parent 23e62bf
commit e5855ab
Show file tree

Hide file tree

Showing 11 changed files with 63 additions and 67 deletions.
diff --git a/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java b/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java
@@ -90,10 +90,13 @@
 import org.opensearch.security.resolver.IndexResolverReplacer.Resolved;
 import org.opensearch.security.securityconf.ConfigModel;
 import org.opensearch.security.securityconf.DynamicConfigModel;
+import org.opensearch.security.securityconf.InMemorySecurityRoles;
+import org.opensearch.security.securityconf.InMemorySecurityRolesV7;
 import org.opensearch.security.securityconf.SecurityRoles;
 import org.opensearch.security.securityconf.impl.DashboardSignInOption;
 import org.opensearch.security.support.ConfigConstants;
 import org.opensearch.security.support.WildcardMatcher;
+import org.opensearch.security.user.PluginUser;
 import org.opensearch.security.user.User;
 import org.opensearch.tasks.Task;
 import org.opensearch.threadpool.ThreadPool;
@@ -143,7 +146,7 @@ public class PrivilegesEvaluator {
     private final PitPrivilegesEvaluator pitPrivilegesEvaluator;
     private DynamicConfigModel dcm;
     private final NamedXContentRegistry namedXContentRegistry;
-    private final Map<String, SecurityRoles> pluginRoles;
+    private final Map<String, InMemorySecurityRoles> pluginRoles;
 
     public PrivilegesEvaluator(
         final ClusterService clusterService,
@@ -197,9 +200,10 @@ public SecurityRoles getSecurityRoles(Set<String> roles) {
     }
 
     public SecurityRoles getSecurityRoleForPlugin(String pluginIdentifier) {
-        SecurityRoles pluginRole = pluginRoles.get(pluginIdentifier);
+        InMemorySecurityRoles pluginRole = pluginRoles.get(pluginIdentifier);
         if (pluginRole == null) {
-            pluginRole = configModel.getSecurityRoles().createSecurityRole(pluginIdentifier, Set.of(BulkAction.NAME), Map.of());
+            pluginRole = new InMemorySecurityRolesV7(1);
+            pluginRole.addSecurityRole(pluginIdentifier, Set.of(BulkAction.NAME), Map.of());
             pluginRoles.put(pluginIdentifier, pluginRole);
         }
         return pluginRole;
@@ -292,7 +296,7 @@ public PrivilegesEvaluatorResponse evaluate(PrivilegesEvaluationContext context)
         }
         presponse.resolvedSecurityRoles.addAll(mappedRoles);
         final SecurityRoles securityRoles;
-        if (user.isPluginUser()) {
+        if (user instanceof PluginUser) {
             securityRoles = getSecurityRoleForPlugin(user.getName());
         } else {
             securityRoles = getSecurityRoles(mappedRoles);

diff --git a/src/main/java/org/opensearch/security/privileges/SystemIndexAccessEvaluator.java b/src/main/java/org/opensearch/security/privileges/SystemIndexAccessEvaluator.java
@@ -48,6 +48,7 @@
 import org.opensearch.security.securityconf.SecurityRoles;
 import org.opensearch.security.support.ConfigConstants;
 import org.opensearch.security.support.WildcardMatcher;
+import org.opensearch.security.user.PluginUser;
 import org.opensearch.security.user.User;
 import org.opensearch.tasks.Task;
 
@@ -316,7 +317,7 @@ private void evaluateSystemIndicesAccess(
                 }
         }
 
-        if (user.isPluginUser()) {
+        if (user instanceof PluginUser) {
             Set<String> matchingSystemIndices = SystemIndexRegistry.matchesPluginSystemIndexPattern(
                 user.getName(),
                 requestedResolved.getAllIndices()

diff --git a/src/main/java/org/opensearch/security/securityconf/ConfigModelV6.java b/src/main/java/org/opensearch/security/securityconf/ConfigModelV6.java
@@ -359,25 +359,6 @@ public SecurityRoles filter(Set<String> keep) {
             return retVal;
         }
 
-        @Override
-        public SecurityRoles createSecurityRole(
-            String roleName,
-            Set<String> clusterPerms,
-            Map<String, Set<String>> indexPatternToAllowedActions
-        ) {
-            SecurityRole role = new SecurityRole(roleName);
-            role.addClusterPerms(clusterPerms);
-            for (Map.Entry<String, Set<String>> entry : indexPatternToAllowedActions.entrySet()) {
-                IndexPattern idxPattern = new IndexPattern(entry.getKey());
-                TypePerm perms = new TypePerm("");
-                perms.addPerms(entry.getValue());
-                idxPattern.addTypePerms(perms);
-            }
-            SecurityRoles roles = new SecurityRoles(1);
-            roles.addSecurityRole(role);
-            return roles;
-        }
-
         @Override
         public EvaluatedDlsFlsConfig getDlsFls(
             User user,

diff --git a/src/main/java/org/opensearch/security/securityconf/ConfigModelV7.java b/src/main/java/org/opensearch/security/securityconf/ConfigModelV7.java
@@ -219,11 +219,11 @@ public static class SecurityRoles implements org.opensearch.security.securitycon
 
         final Set<SecurityRole> roles;
 
-        private SecurityRoles(int roleCount) {
+        protected SecurityRoles(int roleCount) {
             roles = new HashSet<>(roleCount);
         }
 
-        private SecurityRoles addSecurityRole(SecurityRole securityRole) {
+        protected SecurityRoles addSecurityRole(SecurityRole securityRole) {
             if (securityRole != null) {
                 this.roles.add(securityRole);
             }
@@ -273,24 +273,6 @@ public SecurityRoles filter(Set<String> keep) {
             return retVal;
         }
 
-        @Override
-        public SecurityRoles createSecurityRole(
-            String roleName,
-            Set<String> clusterPerms,
-            Map<String, Set<String>> indexPatternToAllowedActions
-        ) {
-            Set<IndexPattern> ipatterns = new HashSet<>();
-            for (Map.Entry<String, Set<String>> entry : indexPatternToAllowedActions.entrySet()) {
-                IndexPattern idxPattern = new IndexPattern(entry.getKey());
-                idxPattern.addPerm(entry.getValue());
-                ipatterns.add(idxPattern);
-            }
-            SecurityRole role = new SecurityRole(roleName, ipatterns, WildcardMatcher.from(clusterPerms));
-            SecurityRoles roles = new SecurityRoles(1);
-            roles.addSecurityRole(role);
-            return roles;
-        }
-
         @Override
         public EvaluatedDlsFlsConfig getDlsFls(
             User user,
@@ -554,7 +536,7 @@ public SecurityRole build() {
             }
         }
 
-        private SecurityRole(String name, Set<IndexPattern> ipatterns, WildcardMatcher clusterPerms) {
+        SecurityRole(String name, Set<IndexPattern> ipatterns, WildcardMatcher clusterPerms) {
             this.name = Objects.requireNonNull(name);
             this.ipatterns = ipatterns;
             this.clusterPerms = clusterPerms;

diff --git a/src/main/java/org/opensearch/security/securityconf/InMemorySecurityRoles.java b/src/main/java/org/opensearch/security/securityconf/InMemorySecurityRoles.java
@@ -0,0 +1,20 @@
+/*
+ * 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.securityconf;
+
+import java.util.Map;
+import java.util.Set;
+
+public interface InMemorySecurityRoles extends SecurityRoles {
+
+    void addSecurityRole(String roleName, Set<String> clusterPerms, Map<String, Set<String>> indexPatternToAllowedActions);
+}
diff --git a/src/main/java/org/opensearch/security/securityconf/InMemorySecurityRolesV7.java b/src/main/java/org/opensearch/security/securityconf/InMemorySecurityRolesV7.java
@@ -0,0 +1,26 @@
+package org.opensearch.security.securityconf;
+
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
+
+import org.opensearch.security.support.WildcardMatcher;
+
+public class InMemorySecurityRolesV7 extends ConfigModelV7.SecurityRoles implements InMemorySecurityRoles {
+
+    public InMemorySecurityRolesV7(int roleCount) {
+        super(roleCount);
+    }
+
+    @Override
+    public void addSecurityRole(String roleName, Set<String> clusterPerms, Map<String, Set<String>> indexPatternToAllowedActions) {
+        Set<ConfigModelV7.IndexPattern> ipatterns = new HashSet<>();
+        for (Map.Entry<String, Set<String>> entry : indexPatternToAllowedActions.entrySet()) {
+            ConfigModelV7.IndexPattern idxPattern = new ConfigModelV7.IndexPattern(entry.getKey());
+            idxPattern.addPerm(entry.getValue());
+            ipatterns.add(idxPattern);
+        }
+        ConfigModelV7.SecurityRole role = new ConfigModelV7.SecurityRole(roleName, ipatterns, WildcardMatcher.from(clusterPerms));
+        roles.add(role);
+    }
+}
diff --git a/src/main/java/org/opensearch/security/securityconf/SecurityRoles.java b/src/main/java/org/opensearch/security/securityconf/SecurityRoles.java
@@ -27,7 +27,6 @@
 
 package org.opensearch.security.securityconf;
 
-import java.util.Map;
 import java.util.Set;
 
 import org.opensearch.cluster.metadata.IndexNameExpressionResolver;
@@ -98,7 +97,5 @@ Set<String> getAllPermittedIndicesForDashboards(
 
     SecurityRoles filter(Set<String> roles);
 
-    SecurityRoles createSecurityRole(String roleName, Set<String> clusterPerms, Map<String, Set<String>> indexPatternToAllowedActions);
-
     boolean isPermittedOnSystemIndex(String indexName);
 }
diff --git a/src/main/java/org/opensearch/security/transport/SecurityInterceptor.java b/src/main/java/org/opensearch/security/transport/SecurityInterceptor.java
@@ -64,6 +64,7 @@
 import org.opensearch.security.support.ConfigConstants;
 import org.opensearch.security.support.HeaderHelper;
 import org.opensearch.security.support.SerializationFormat;
+import org.opensearch.security.user.PluginUser;
 import org.opensearch.security.user.User;
 import org.opensearch.threadpool.ThreadPool;
 import org.opensearch.transport.Transport.Connection;
@@ -132,7 +133,7 @@ public <T extends TransportRequest> SecurityRequestHandler<T> getHandler(String
     private User determineUser(Connection connection) {
         User user0 = getThreadContext().getTransient(ConfigConstants.OPENDISTRO_SECURITY_USER);
         // pluginUser did not exist prior to 2.18.0
-        if (user0 != null && user0.isPluginUser() && connection.getVersion().before(Version.V_2_18_0)) {
+        if (user0 != null && user0 instanceof PluginUser && connection.getVersion().before(Version.V_2_18_0)) {
             user0 = null;
         }
         return user0;

diff --git a/src/main/java/org/opensearch/security/user/User.java b/src/main/java/org/opensearch/security/user/User.java
@@ -296,11 +296,4 @@ public boolean isServiceAccount() {
         Map<String, String> userAttributesMap = this.getCustomAttributesMap();
         return userAttributesMap != null && "true".equals(userAttributesMap.get("attr.internal.service"));
     }
-
-    /**
-     * @return true if this instance is of the type PluginUser
-     */
-    public boolean isPluginUser() {
-        return this instanceof PluginUser;
-    }
 }
diff --git a/src/test/java/org/opensearch/security/securityconf/SecurityRolesPermissionsTest.java b/src/test/java/org/opensearch/security/securityconf/SecurityRolesPermissionsTest.java
@@ -221,8 +221,8 @@ public void hasExplicitClusterPermissionPermissionForRestAdmin() {
 
     @Test
     public void testCreateSecurityRole() {
-        SecurityRoles securityRoles = configModel.getSecurityRoles()
-            .createSecurityRole("testRole", Set.of("cluster:monitor/health"), Map.of("*", Set.of("indices:data/read/search")));
+        InMemorySecurityRoles securityRoles = new InMemorySecurityRolesV7(1);
+        securityRoles.addSecurityRole("testRole", Set.of("cluster:monitor/health"), Map.of("*", Set.of("indices:data/read/search")));
         assertTrue(securityRoles.getRoleNames().contains("testRole"));
         assertTrue(securityRoles.hasExplicitClusterPermissionPermission("cluster:monitor/health"));
     }

diff --git a/src/test/java/org/opensearch/security/securityconf/SecurityRolesPermissionsV6Test.java b/src/test/java/org/opensearch/security/securityconf/SecurityRolesPermissionsV6Test.java
@@ -15,7 +15,6 @@
 import java.util.Arrays;
 import java.util.List;
 import java.util.Map;
-import java.util.Set;
 import java.util.TreeMap;
 
 import com.google.common.collect.ImmutableMap;
@@ -128,14 +127,6 @@ public void hasExplicitIndexPermission() {
         );
     }
 
-    @Test
-    public void testCreateSecurityRole() {
-        SecurityRoles securityRoles = configModel.getSecurityRoles()
-            .createSecurityRole("testRole", Set.of("cluster:monitor/health"), Map.of("*", Set.of("indices:data/read/search")));
-        assertTrue(securityRoles.getRoleNames().contains("testRole"));
-        assertTrue(securityRoles.hasExplicitClusterPermissionPermission("cluster:monitor/health"));
-    }
-
     @Test
     public void isPermittedOnSystemIndex() {
         final SecurityRoles securityRoleWithExplicitAccess = configModel.getSecurityRoles()