Synchronize security roles to avoid ConcurrentModificationException

Signed-off-by: shikharj05 <8859327+shikharj05@users.noreply.github.com>
opensearch-project · Feb 4, 2025 · 7980c01 · 7980c01
1 parent cc44bf3
commit 7980c01
Show file tree

Hide file tree

Showing 2 changed files with 15 additions and 12 deletions.
diff --git a/src/main/java/org/opensearch/security/securityconf/ConfigModelV7.java b/src/main/java/org/opensearch/security/securityconf/ConfigModelV7.java
@@ -1222,7 +1222,10 @@ private Set<String> map(final User user, final TransportAddress caller) {
                 return Collections.emptySet();
             }
 
-            final Set<String> securityRoles = new HashSet<>(user.getSecurityRoles());
+            final Set<String> securityRoles = new HashSet<>();
+            synchronized (user.getSecurityRoles()) {
+                securityRoles.addAll(user.getSecurityRoles());
+            }
 
             if (rolesMappingResolution == ConfigConstants.RolesMappingResolution.BOTH
                 || rolesMappingResolution == ConfigConstants.RolesMappingResolution.BACKENDROLES_ONLY) {

diff --git a/src/main/java/org/opensearch/security/user/User.java b/src/main/java/org/opensearch/security/user/User.java
@@ -38,8 +38,6 @@
 
 import com.google.common.collect.Lists;
 
-import java.util.concurrent.ConcurrentHashMap;
-import java.util.concurrent.CopyOnWriteArraySet;
 import org.opensearch.core.common.io.stream.StreamInput;
 import org.opensearch.core.common.io.stream.StreamOutput;
 import org.opensearch.core.common.io.stream.Writeable;
@@ -74,10 +72,10 @@ public class User implements Serializable, Writeable, CustomAttributesAware {
     /**
      * roles == backend_roles
      */
-    private final Set<String> roles = new CopyOnWriteArraySet<>();
-    private final Set<String> securityRoles = new CopyOnWriteArraySet<>();
+    private final Set<String> roles = Collections.synchronizedSet(new HashSet<String>());
+    private final Set<String> securityRoles = Collections.synchronizedSet(new HashSet<String>());
     private String requestedTenant;
-    private final Map<String, String> attributes = new ConcurrentHashMap<>();
+    private Map<String, String> attributes = Collections.synchronizedMap(new HashMap<>());
     private boolean isInjected = false;
 
     public User(final StreamInput in) throws IOException {
@@ -88,10 +86,7 @@ public User(final StreamInput in) throws IOException {
         if (requestedTenant.isEmpty()) {
             requestedTenant = null;
         }
-        Map<String, String> readAttributes = in.readMap(StreamInput::readString, StreamInput::readString);
-        if (readAttributes != null) {
-            attributes.putAll(readAttributes);
-        }
+        attributes = Collections.synchronizedMap(in.readMap(StreamInput::readString, StreamInput::readString));
         securityRoles.addAll(in.readList(StreamInput::readString));
     }
 
@@ -274,7 +269,10 @@ public void writeTo(StreamOutput out) throws IOException {
      * @return A modifiable map with all the current custom attributes associated with this user
      */
     public synchronized final Map<String, String> getCustomAttributesMap() {
-        return Collections.unmodifiableMap(attributes);
+        if (attributes == null) {
+            attributes = Collections.synchronizedMap(new HashMap<>());
+        }
+        return attributes;
     }
 
     public final void addSecurityRoles(final Collection<String> securityRoles) {
@@ -284,7 +282,9 @@ public final void addSecurityRoles(final Collection<String> securityRoles) {
     }
 
     public final Set<String> getSecurityRoles() {
-        return Collections.unmodifiableSet(this.securityRoles);
+        return this.securityRoles == null
+            ? Collections.synchronizedSet(Collections.emptySet())
+            : Collections.unmodifiableSet(this.securityRoles);
     }
 
     /**