Setter Approach commit 0

Signed-off-by: Ryan Liang <jiallian@amazon.com>

src/main/java/org/opensearch/security/auth/UserInjector.java

@@ -73,7 +73,7 @@ public InjectedUser(String name) {
         private Object writeReplace() throws ObjectStreamException {
             User user = new User(getName());
             user.addRoles(getRoles());
-            user.addSecurityRoles(getSecurityRoles());
+            user.setSecurityRoles(getSecurityRoles());
             user.setRequestedTenant(getRequestedTenant());
             user.addAttributes(getCustomAttributesMap());
             user.setInjected(true);

src/main/java/org/opensearch/security/auth/internal/InternalAuthenticationBackend.java

@@ -73,7 +73,7 @@ public boolean exists(User user) {
 
             final List<String> securityRoles = internalUsersModel.getSecurityRoles(user.getName());
             if (securityRoles != null) {
-                user.addSecurityRoles(securityRoles);
+                user.setSecurityRoles(securityRoles);
             }
 
             user.addAttributes(attributeMap);
@@ -140,7 +140,7 @@ public User authenticate(final AuthCredentials credentials) {
 
                 final List<String> securityRoles = internalUsersModel.getSecurityRoles(credentials.getUsername());
                 if (securityRoles != null) {
-                    user.addSecurityRoles(securityRoles);
+                    user.setSecurityRoles(securityRoles);
                 }
                 return user;
             } else {

src/main/java/org/opensearch/security/auth/internal/NoOpAuthenticationBackend.java

@@ -46,7 +46,9 @@ public String getType() {
 
     @Override
     public User authenticate(final AuthCredentials credentials) {
-        return new User(credentials.getUsername(), credentials.getBackendRoles(), credentials);
+        User user = new User(credentials.getUsername(), credentials.getBackendRoles(), credentials);
+        user.setSecurityRoles(credentials.getSecurityRoles());
+        return user;
     }
 
     @Override

src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java

@@ -262,7 +262,7 @@ public PrivilegesEvaluatorResponse evaluate(
 
         setUserInfoInThreadContext(user, mappedRoles);
         // Add the security roles for this user so that they can be used for DLS parameter substitution.
-        user.addSecurityRoles(mappedRoles);
+        user.setSecurityRoles(mappedRoles);
 
         final boolean isDebugEnabled = log.isDebugEnabled();
         if (isDebugEnabled) {

src/main/java/org/opensearch/security/support/Base64Helper.java

@@ -46,7 +46,6 @@
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
-import java.util.concurrent.atomic.AtomicReference;
 import java.util.regex.Pattern;
 
 import com.google.common.base.Preconditions;
@@ -146,7 +145,7 @@ private SafeObjectOutputStream(OutputStream out) throws IOException {
         @Override
         protected Object replaceObject(Object obj) throws IOException {
             Class<?> clazz = obj.getClass();
-            if (isSafeClass(clazz) || (AtomicReference.class.equals(clazz) && isSafeClass(((AtomicReference)obj).get().getClass()))) {
+            if (isSafeClass(clazz)) {
                 return obj;
             }
             throw new IOException("Unauthorized serialization attempt " + clazz.getName());
@@ -190,7 +189,7 @@ public SafeObjectInputStream(InputStream in) throws IOException {
         protected Class<?> resolveClass(ObjectStreamClass desc) throws IOException, ClassNotFoundException {
 
             Class<?> clazz = super.resolveClass(desc);
-            if (isSafeClass(clazz) || AtomicReference.class.equals(clazz)) {
+            if (isSafeClass(clazz)) {
                 return clazz;
             }
 

src/main/java/org/opensearch/security/user/AuthCredentials.java

@@ -48,6 +48,7 @@ public final class AuthCredentials {
     private final String username;
     private byte[] password;
     private Object nativeCredentials;
+    private final Set<String> securityRoles = new HashSet<String>();
     private final Set<String> backendRoles = new HashSet<String>();
     private boolean complete;
     private final byte[] internalPasswordHash;
@@ -203,6 +204,14 @@ public Set<String> getBackendRoles() {
         return new HashSet<String>(backendRoles);
     }
 
+    /**
+     *
+     * @return Defensive copy of the security roles this user is member of.
+     */
+    public Set<String> getSecurityRoles() {
+        return new HashSet<String>(securityRoles);
+    }
+
     public boolean isComplete() {
         return complete;
     }

src/main/java/org/opensearch/security/user/User.java

@@ -33,12 +33,8 @@
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.HashSet;
-import java.util.List;
 import java.util.Map;
 import java.util.Set;
-import java.util.concurrent.atomic.AtomicBoolean;
-import java.util.concurrent.atomic.AtomicReference;
-import java.util.stream.Collectors;
 
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Lists;
@@ -78,8 +74,7 @@ public class User implements Serializable, Writeable, CustomAttributesAware {
      * roles == backend_roles
      */
     private final Set<String> roles = Collections.synchronizedSet(new HashSet<String>());
-    private final AtomicReference<Set<String>> securityRoles = new AtomicReference<>(new HashSet<String>());
-    private String requestedTenant;
+    private final Set<String> securityRoles = Collections.synchronizedSet(new HashSet<String>());    private String requestedTenant;
     private Map<String, String> attributes = Collections.synchronizedMap(new HashMap<>());
     private boolean isInjected = false;
 
@@ -89,7 +84,7 @@ public User(final StreamInput in) throws IOException {
         roles.addAll(in.readList(StreamInput::readString));
         requestedTenant = in.readString();
         attributes = Collections.synchronizedMap(in.readMap(StreamInput::readString, StreamInput::readString));
-        securityRoles.get().addAll(in.readList(StreamInput::readString));
+        securityRoles.addAll(in.readList(StreamInput::readString));
     }
 
     /**
@@ -262,7 +257,7 @@ public void writeTo(StreamOutput out) throws IOException {
         out.writeStringCollection(new ArrayList<String>(roles));
         out.writeString(requestedTenant);
         out.writeMap(attributes, StreamOutput::writeString, StreamOutput::writeString);
-        out.writeStringCollection(securityRoles.get() ==null?Collections.emptyList():new ArrayList<String>(securityRoles.get()));
+        out.writeStringCollection(securityRoles ==null?Collections.emptyList():new ArrayList<String>(securityRoles));
     }
 
     /**
@@ -277,14 +272,16 @@ public synchronized final Map<String, String> getCustomAttributesMap() {
         return attributes;
     }
 
-    public final void addSecurityRoles(final Collection<String> securityRoles) {
-        if (securityRoles != null && this.securityRoles != null) {
-            List<String> filteredRoles = securityRoles.stream().filter(r -> r != null).collect(Collectors.toList());
-            this.securityRoles.get().addAll(filteredRoles);
+    public final void setSecurityRoles(final Collection<String> securityRoles) {
+        if(securityRoles != null) {
+            synchronized(this.securityRoles) {
+                this.securityRoles.clear();
+                this.securityRoles.addAll(securityRoles);
+            }
         }
     }
 
     public final Set<String> getSecurityRoles() {
-        return this.securityRoles.get() == null ? Collections.synchronizedSet(Collections.emptySet()) : ImmutableSet.copyOf(this.securityRoles.get());
+        return this.securityRoles == null ? Collections.synchronizedSet(Collections.emptySet()) : ImmutableSet.copyOf(this.securityRoles);
     }
 }