From cc44bf3f238df2d6358b65146bb4c830e377b70a Mon Sep 17 00:00:00 2001 From: shikharj05 <8859327+shikharj05@users.noreply.github.com> Date: Tue, 4 Feb 2025 12:25:01 +0530 Subject: [PATCH 1/2] Avoid ConcurrentModificationException for User class fields Signed-off-by: shikharj05 <8859327+shikharj05@users.noreply.github.com> --- .../org/opensearch/security/user/User.java | 22 +++++++++---------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/src/main/java/org/opensearch/security/user/User.java b/src/main/java/org/opensearch/security/user/User.java index 190729623d..5c8b9d2f5e 100644 --- a/src/main/java/org/opensearch/security/user/User.java +++ b/src/main/java/org/opensearch/security/user/User.java @@ -38,6 +38,8 @@ 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; @@ -72,10 +74,10 @@ public class User implements Serializable, Writeable, CustomAttributesAware { /** * roles == backend_roles */ - private final Set roles = Collections.synchronizedSet(new HashSet()); - private final Set securityRoles = Collections.synchronizedSet(new HashSet()); + private final Set roles = new CopyOnWriteArraySet<>(); + private final Set securityRoles = new CopyOnWriteArraySet<>(); private String requestedTenant; - private Map attributes = Collections.synchronizedMap(new HashMap<>()); + private final Map attributes = new ConcurrentHashMap<>(); private boolean isInjected = false; public User(final StreamInput in) throws IOException { @@ -86,7 +88,10 @@ public User(final StreamInput in) throws IOException { if (requestedTenant.isEmpty()) { requestedTenant = null; } - attributes = Collections.synchronizedMap(in.readMap(StreamInput::readString, StreamInput::readString)); + Map readAttributes = in.readMap(StreamInput::readString, StreamInput::readString); + if (readAttributes != null) { + attributes.putAll(readAttributes); + } securityRoles.addAll(in.readList(StreamInput::readString)); } @@ -269,10 +274,7 @@ 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 getCustomAttributesMap() { - if (attributes == null) { - attributes = Collections.synchronizedMap(new HashMap<>()); - } - return attributes; + return Collections.unmodifiableMap(attributes); } public final void addSecurityRoles(final Collection securityRoles) { @@ -282,9 +284,7 @@ public final void addSecurityRoles(final Collection securityRoles) { } public final Set getSecurityRoles() { - return this.securityRoles == null - ? Collections.synchronizedSet(Collections.emptySet()) - : Collections.unmodifiableSet(this.securityRoles); + return Collections.unmodifiableSet(this.securityRoles); } /** From 7980c019ac56214799f7370b7af7d6e991d8d5b1 Mon Sep 17 00:00:00 2001 From: shikharj05 <8859327+shikharj05@users.noreply.github.com> Date: Tue, 4 Feb 2025 14:07:08 +0530 Subject: [PATCH 2/2] Synchronize security roles to avoid ConcurrentModificationException Signed-off-by: shikharj05 <8859327+shikharj05@users.noreply.github.com> --- .../security/securityconf/ConfigModelV7.java | 5 ++++- .../org/opensearch/security/user/User.java | 22 +++++++++---------- 2 files changed, 15 insertions(+), 12 deletions(-) diff --git a/src/main/java/org/opensearch/security/securityconf/ConfigModelV7.java b/src/main/java/org/opensearch/security/securityconf/ConfigModelV7.java index 463afafa88..7f14e41912 100644 --- 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 map(final User user, final TransportAddress caller) { return Collections.emptySet(); } - final Set securityRoles = new HashSet<>(user.getSecurityRoles()); + final Set 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 index 5c8b9d2f5e..190729623d 100644 --- 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 roles = new CopyOnWriteArraySet<>(); - private final Set securityRoles = new CopyOnWriteArraySet<>(); + private final Set roles = Collections.synchronizedSet(new HashSet()); + private final Set securityRoles = Collections.synchronizedSet(new HashSet()); private String requestedTenant; - private final Map attributes = new ConcurrentHashMap<>(); + private Map 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 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 getCustomAttributesMap() { - return Collections.unmodifiableMap(attributes); + if (attributes == null) { + attributes = Collections.synchronizedMap(new HashMap<>()); + } + return attributes; } public final void addSecurityRoles(final Collection securityRoles) { @@ -284,7 +282,9 @@ public final void addSecurityRoles(final Collection securityRoles) { } public final Set getSecurityRoles() { - return Collections.unmodifiableSet(this.securityRoles); + return this.securityRoles == null + ? Collections.synchronizedSet(Collections.emptySet()) + : Collections.unmodifiableSet(this.securityRoles); } /**