Test fixes

Signed-off-by: Nils Bandener <nils.bandener@eliatra.com>

Author: nibix

src/integrationTest/java/org/opensearch/security/privileges/actionlevel/RoleBasedActionPrivilegesTest.java

@@ -931,7 +931,7 @@ public void hasIndexPrivilege_errors() throws Exception {
             assertTrue(
                 "Result mentions role_with_errors: " + result.getEvaluationExceptionInfo(),
                 result.getEvaluationExceptionInfo()
-                    .contains("Exceptions encountered during privilege evaluation:\n" + "Error while evaluating")
+                    .contains("Error while evaluating dynamic index pattern: /invalid_regex_with_attr${user.name}\\/")
             );
         }
 
@@ -1063,8 +1063,7 @@ public void hasExplicitIndexPrivilege_errors() throws Exception {
             assertTrue(result.hasEvaluationExceptions());
             assertTrue(
                 "Result mentions role_with_errors: " + result.getEvaluationExceptionInfo(),
-                result.getEvaluationExceptionInfo()
-                    .contains("Exceptions encountered during privilege evaluation:\n" + "Error while evaluating role role_with_errors")
+                result.getEvaluationExceptionInfo().contains("Error while evaluating role role_with_errors")
             );
         }
 

src/integrationTest/java/org/opensearch/test/framework/TestSecurityConfig.java

@@ -35,7 +35,6 @@
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.HashMap;
-import java.util.HashSet;
 import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
@@ -45,6 +44,7 @@
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
 
+import com.google.common.collect.Streams;
 import com.fasterxml.jackson.databind.JsonNode;
 import com.fasterxml.jackson.databind.ObjectMapper;
 import com.fasterxml.jackson.dataformat.yaml.YAMLFactory;
@@ -166,7 +166,9 @@ public TestSecurityConfig authz(AuthzDomain authzDomain) {
 
     public TestSecurityConfig user(User user) {
         this.internalUsers.put(user.name, user);
-        // The user's roles will be collected by aggregateRoles() when the configuration is written
+        for (Role role : user.roles) {
+            this.roles.put(role.name, role);
+        }
         return this;
     }
 
@@ -453,11 +455,8 @@ public static final class User implements UserCredentialsHolder, ToXContentObjec
         String name;
         private String password;
         List<Role> roles = new ArrayList<>();
+        List<Role> referencedRoles = new ArrayList<>();
         List<String> backendRoles = new ArrayList<>();
-        /**
-         * This will be initialized by aggregateRoles()
-         */
-        Set<String> roleNames;
         String requestedTenant;
         private Map<String, String> attributes = new HashMap<>();
         private Map<MetadataKey<?>, Object> matchers = new HashMap<>();
@@ -486,8 +485,31 @@ public User password(String password) {
             return this;
         }
 
+        /**
+         * Adds a user-specific role to this user. Internally, the role name will be scoped with the user name
+         * to avoid accidental collisions between roles of different users.
+         */
         public User roles(Role... roles) {
-            this.roles.addAll(Arrays.asList(roles));
+            // We scope the role names by user to keep tests free of potential side effects
+            String roleNamePrefix = "user_" + this.getName() + "__";
+
+            for (Role role : roles) {
+                Role copy = role.clone();
+                if (!copy.name.startsWith(roleNamePrefix)) {
+                    copy.name = roleNamePrefix + role.name;
+                }
+                this.roles.add(copy);
+            }
+
+            return this;
+        }
+
+        /**
+         * Adds references to roles which are already defined for the top-level SecurityTestConfig object.
+         * This allows tests to share roles between users.
+         */
+        public User referencedRoles(Role... roles) {
+            this.referencedRoles.addAll(Arrays.asList(roles));
             return this;
         }
 
@@ -534,10 +556,7 @@ public String getPassword() {
         }
 
         public Set<String> getRoleNames() {
-            if (roleNames == null) {
-                this.aggregateRoles();
-            }
-            return roleNames;
+            return Streams.concat(roles.stream(), referencedRoles.stream()).map(Role::getName).collect(Collectors.toSet());
         }
 
         public String getDescription() {
@@ -640,25 +659,6 @@ public MetadataKey(String name, Class<T> type) {
                 this.type = type;
             }
         }
-
-        void aggregateRoles() {
-            if (this.roleNames == null) {
-                this.roleNames = new HashSet<>();
-            }
-
-            for (Role role : this.roles) {
-                if (role.addedIndependentlyOfUser) {
-                    // This is a globally defined role, we just use this
-                    this.roleNames.add(role.name);
-                } else {
-                    // This is role that is locally defined for the user; let's scope the name
-                    if (!role.name.startsWith("user_" + this.name)) {
-                        role.name = "user_" + this.name + "__" + role.name;
-                    }
-                    this.roleNames.add(role.name);
-                }
-            }
-        }
     }
 
     public static class Role implements ToXContentObject {
@@ -1115,11 +1115,13 @@ public void initIndex(Client client) {
         client.admin().indices().create(new CreateIndexRequest(indexName).settings(settings)).actionGet();
 
         if (rawConfigurationDocuments == null) {
+            checkReferencedRoles();
+
             writeSingleEntryConfigToIndex(client, CType.CONFIG, config);
             if (auditConfiguration != null) {
                 writeSingleEntryConfigToIndex(client, CType.AUDIT, "config", auditConfiguration);
             }
-            writeConfigToIndex(client, CType.ROLES, aggregateRoles());
+            writeConfigToIndex(client, CType.ROLES, roles);
             writeConfigToIndex(client, CType.INTERNALUSERS, internalUsers);
             writeConfigToIndex(client, CType.ROLESMAPPING, rolesMapping);
             writeConfigToIndex(client, CType.ACTIONGROUPS, actionGroups);
@@ -1134,23 +1136,23 @@ public void initIndex(Client client) {
     }
 
     /**
-     * Merges the globally defined roles with the roles defined by user. Roles defined by user will be scoped
-     * so that user definitions cannot interfere with others.
+     * Does a sanity check on the user's referenced roles; these must actually match the globally defined roles.
      */
-    private Map<String, Role> aggregateRoles() {
-        Map<String, Role> result = new HashMap<>(this.roles);
-
+    private void checkReferencedRoles() {
         for (User user : this.internalUsers.values()) {
-            user.aggregateRoles();
-
-            for (Role role : user.roles) {
-                if (!role.addedIndependentlyOfUser) {
-                    result.put(role.name, role);
+            for (Role role : user.referencedRoles) {
+                if (this.roles.containsKey(role.name) && !this.roles.get(role.name).equals(role)) {
+                    throw new RuntimeException(
+                        "Referenced role '"
+                            + role.name
+                            + "' in user '"
+                            + user.name
+                            + "' does not match the definition in TestSecurityConfig: "
+                            + this.roles.get(role.name)
+                    );
                 }
             }
         }
-
-        return result;
     }
 
     public void updateInternalUsersConfiguration(Client client, List<User> users) {

src/main/java/org/opensearch/security/privileges/actionlevel/RuntimeOptimizedActionPrivileges.java

@@ -452,7 +452,7 @@ protected void checkPrivilegeWithIndexPatternOnWellKnownActions(
                         } catch (PrivilegesEvaluationException e) {
                             // We can ignore these errors, as this max leads to fewer privileges than available
                             log.error("Error while evaluating index pattern of {}. Ignoring entry", this, e);
-                            exceptions.add(new PrivilegesEvaluationException("Error while evaluating " + this, e));
+                            exceptions.add(new PrivilegesEvaluationException("Error while evaluating index pattern " + indexPattern, e));
                         }
                     }
                 }