Immutable user object

CHANGELOG.md

@@ -12,6 +12,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 ### Changed
 - Use extendedPlugins in integrationTest framework for sample resource plugin testing ([#5322](https://github.com/opensearch-project/security/pull/5322))
 
+- Performance improvements: Immutable user object ([#5212])
+
 ### Dependencies
 - Bump `guava_version` from 33.4.6-jre to 33.4.8-jre ([#5284](https://github.com/opensearch-project/security/pull/5284))
 - Bump `spring_version` from 6.2.5 to 6.2.6 ([#5283](https://github.com/opensearch-project/security/pull/5283))

src/integrationTest/java/org/opensearch/security/privileges/ActionPrivilegesTest.java

@@ -1071,8 +1071,7 @@ static SecurityDynamicConfiguration<RoleV7> createRoles(int numberOfRoles, int n
     }
 
     static PrivilegesEvaluationContext ctx(String... roles) {
-        User user = new User("test_user");
-        user.addAttributes(ImmutableMap.of("attrs.dept_no", "a11"));
+        User user = new User("test_user").withAttributes(ImmutableMap.of("attrs.dept_no", "a11"));
         return new PrivilegesEvaluationContext(
             user,
             ImmutableSet.copyOf(roles),
@@ -1086,8 +1085,7 @@ static PrivilegesEvaluationContext ctx(String... roles) {
     }
 
     static PrivilegesEvaluationContext ctxByUsername(String username) {
-        User user = new User(username);
-        user.addAttributes(ImmutableMap.of("attrs.dept_no", "a11"));
+        User user = new User(username).withAttributes(ImmutableMap.of("attrs.dept_no", "a11"));
         return new PrivilegesEvaluationContext(
             user,
             ImmutableSet.of(),

src/integrationTest/java/org/opensearch/security/privileges/IndexPatternTest.java

@@ -234,10 +234,7 @@ public void equals() {
     private static PrivilegesEvaluationContext ctx() {
         IndexNameExpressionResolver indexNameExpressionResolver = new IndexNameExpressionResolver(new ThreadContext(Settings.EMPTY));
         IndexResolverReplacer indexResolverReplacer = new IndexResolverReplacer(indexNameExpressionResolver, () -> CLUSTER_STATE, null);
-        User user = new User("test_user");
-        user.addAttributes(ImmutableMap.of("attrs.a11", "a11"));
-        user.addAttributes(ImmutableMap.of("attrs.year", "year"));
-
+        User user = new User("test_user").withAttributes(ImmutableMap.of("attrs.a11", "a11", "attrs.year", "year"));
         return new PrivilegesEvaluationContext(
             user,
             ImmutableSet.of(),

src/integrationTest/java/org/opensearch/security/privileges/dlsfls/DocumentPrivilegesTest.java

@@ -1191,9 +1191,7 @@ UserSpec attribute(String name, String value) {
         }
 
         User buildUser() {
-            User user = new User("test_user_" + description);
-            user.addAttributes(this.attributes);
-            return user;
+            return new User("test_user_" + description).withAttributes(this.attributes);
         }
 
         @Override

src/integrationTest/java/org/opensearch/security/user/UserFactoryTest.java

@@ -0,0 +1,65 @@
+/*
+ * 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.user;
+
+import java.util.Arrays;
+import java.util.Collection;
+
+import com.google.common.collect.ImmutableSet;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+
+import org.opensearch.OpenSearchException;
+import org.opensearch.common.settings.Settings;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
+
+@RunWith(Parameterized.class)
+public class UserFactoryTest {
+    UserFactory subject;
+
+    @Test
+    public void parse_successful() {
+        User source = new User("test_user").withRoles(ImmutableSet.of("a", "b"));
+        User target = subject.fromSerializedBase64(source.toSerializedBase64());
+
+        assertEquals(source, target);
+    }
+
+    @Test
+    public void parse_invalid() {
+        try {
+            User target = subject.fromSerializedBase64("invaliddata123");
+            fail("Should have failed; got " + target);
+        } catch (Exception e) {
+            assertTrue(
+                "Got invalid stream header " + e,
+                e instanceof OpenSearchException && e.getMessage().contains("invalid stream header")
+            );
+        }
+    }
+
+    public UserFactoryTest(UserFactory subject, String name) {
+        this.subject = subject;
+    }
+
+    @Parameterized.Parameters(name = "{1}")
+    public static Collection<Object[]> params() {
+        return Arrays.asList(
+            new Object[] { new UserFactory.Simple(), "Simple" },
+            new Object[] { new UserFactory.Caching(Settings.EMPTY), "Caching" }
+        );
+    }
+}

src/integrationTest/java/org/opensearch/security/user/UserTest.java

@@ -0,0 +1,138 @@
+/*
+ * 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.user;
+
+import java.util.Arrays;
+import java.util.Map;
+
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
+import org.junit.Test;
+
+import org.opensearch.security.support.Base64Helper;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertSame;
+
+public class UserTest {
+    @Test
+    public void serialization() {
+        User user = new User("serialization_test_user").withRoles(Arrays.asList("br1", "br2", "br3"))
+            .withSecurityRoles(Arrays.asList("sr1", "sr2"))
+            .withAttributes(ImmutableMap.of("a", "v_a", "b", "v_b"));
+
+        String serialized = Base64Helper.serializeObject(user);
+        User user2 = User.fromSerializedBase64(serialized);
+        assertEquals(user, user2);
+
+    }
+
+    @Test
+    public void deserializationFrom2_19() {
+        // The following base64 string was produced by the following code on OpenSearch 2.19
+        // User user = new User("serialization_test_user");
+        // user.addRoles(Arrays.asList("br1", "br2", "br3"));
+        // user.addSecurityRoles(Arrays.asList("sr1", "sr2"));
+        // user.addAttributes(ImmutableMap.of("a", "v_a", "b", "v_b"));
+        // println(Base64JDKHelper.serializeObject(user));
+        String serialized =
+            "rO0ABXNyACFvcmcub3BlbnNlYXJjaC5zZWN1cml0eS51c2VyLlVzZXKzqL2T65dH3AIABloACmlzSW5qZWN0ZWRMAAphdHRyaWJ1dGVzdAAPTGphdmEvdXRpbC9NYXA7TAAEbmFtZXQAEkxqYXZhL2xhbmcvU3RyaW5nO0wAD3JlcXVlc3RlZFRlbmFudHEAfgACTAAFcm9sZXN0AA9MamF2YS91dGlsL1NldDtMAA1zZWN1cml0eVJvbGVzcQB+AAN4cABzcgAlamF2YS51dGlsLkNvbGxlY3Rpb25zJFN5bmNocm9uaXplZE1hcBtz+QlLSzl7AwACTAABbXEAfgABTAAFbXV0ZXh0ABJMamF2YS9sYW5nL09iamVjdDt4cHNyABFqYXZhLnV0aWwuSGFzaE1hcAUH2sHDFmDRAwACRgAKbG9hZEZhY3RvckkACXRocmVzaG9sZHhwP0AAAAAAAAN3CAAAAAQAAAACdAABYXQAA3ZfYXQAAWJ0AAN2X2J4cQB+AAd4dAAXc2VyaWFsaXphdGlvbl90ZXN0X3VzZXJwc3IAJWphdmEudXRpbC5Db2xsZWN0aW9ucyRTeW5jaHJvbml6ZWRTZXQGw8J5Au7fPAIAAHhyACxqYXZhLnV0aWwuQ29sbGVjdGlvbnMkU3luY2hyb25pemVkQ29sbGVjdGlvbiph+E0JnJm1AwACTAABY3QAFkxqYXZhL3V0aWwvQ29sbGVjdGlvbjtMAAVtdXRleHEAfgAGeHBzcgARamF2YS51dGlsLkhhc2hTZXS6RIWVlri3NAMAAHhwdwwAAAAQP0AAAAAAAAN0AANicjF0AANicjN0AANicjJ4cQB+ABJ4c3EAfgAPc3EAfgATdwwAAAAQP0AAAAAAAAJ0AANzcjJ0AANzcjF4cQB+ABh4";
+
+        User user = User.fromSerializedBase64(serialized);
+        assertEquals(
+            new User("serialization_test_user").withRoles(Arrays.asList("br1", "br2", "br3"))
+                .withSecurityRoles(Arrays.asList("sr1", "sr2"))
+                .withAttributes(ImmutableMap.of("a", "v_a", "b", "v_b")),
+            user
+        );
+    }
+
+    @Test
+    public void deserializationLdapUserFrom2_19() {
+        // The following base64 string was produced by the following code on OpenSearch 2.19
+        // LdapUser user = new LdapUser("serialization_test_user",
+        // "original_user_name",
+        // new LdapEntry("cn=test,ou=people,o=TEST", new LdapAttribute("test_ldap_attr", "test_ldap_attr_value")),
+        // new AuthCredentials("test_user", "secret".getBytes(StandardCharsets.UTF_8)),
+        // 100,
+        // WildcardMatcher.ANY);
+        // user.addRoles(Arrays.asList("br1", "br2", "br3"));
+        // user.addSecurityRoles(Arrays.asList("sr1", "sr2"));
+        // user.addAttributes(ImmutableMap.of("a", "v_a", "b", "v_b"));
+        // println(Base64JDKHelper.serializeObject(user));
+        String serialized =
+            "rO0ABXNyACJjb20uYW1hem9uLmRsaWMuYXV0aC5sZGFwLkxkYXBVc2VyAAAAAAAAAAECAAFMABBvcmlnaW5hbFVzZXJuYW1ldAASTGphdmEvbGFuZy9TdHJpbmc7eHIAIW9yZy5vcGVuc2VhcmNoLnNlY3VyaXR5LnVzZXIuVXNlcrOovZPrl0fcAgAGWgAKaXNJbmplY3RlZEwACmF0dHJpYnV0ZXN0AA9MamF2YS91dGlsL01hcDtMAARuYW1lcQB+AAFMAA9yZXF1ZXN0ZWRUZW5hbnRxAH4AAUwABXJvbGVzdAAPTGphdmEvdXRpbC9TZXQ7TAANc2VjdXJpdHlSb2xlc3EAfgAEeHAAc3IAJWphdmEudXRpbC5Db2xsZWN0aW9ucyRTeW5jaHJvbml6ZWRNYXAbc/kJS0s5ewMAAkwAAW1xAH4AA0wABW11dGV4dAASTGphdmEvbGFuZy9PYmplY3Q7eHBzcgARamF2YS51dGlsLkhhc2hNYXAFB9rBwxZg0QMAAkYACmxvYWRGYWN0b3JJAAl0aHJlc2hvbGR4cD9AAAAAAAAGdwgAAAAIAAAABXQAB2xkYXAuZG50ABhjbj10ZXN0LG91PXBlb3BsZSxvPVRFU1R0AAFhdAADdl9hdAAYYXR0ci5sZGFwLnRlc3RfbGRhcF9hdHRydAAUdGVzdF9sZGFwX2F0dHJfdmFsdWV0AAFidAADdl9idAAWbGRhcC5vcmlnaW5hbC51c2VybmFtZXQAEm9yaWdpbmFsX3VzZXJfbmFtZXhxAH4ACHh0ABdzZXJpYWxpemF0aW9uX3Rlc3RfdXNlcnBzcgAlamF2YS51dGlsLkNvbGxlY3Rpb25zJFN5bmNocm9uaXplZFNldAbDwnkC7t88AgAAeHIALGphdmEudXRpbC5Db2xsZWN0aW9ucyRTeW5jaHJvbml6ZWRDb2xsZWN0aW9uKmH4TQmcmbUDAAJMAAFjdAAWTGphdmEvdXRpbC9Db2xsZWN0aW9uO0wABW11dGV4cQB+AAd4cHNyABFqYXZhLnV0aWwuSGFzaFNldLpEhZWWuLc0AwAAeHB3DAAAABA/QAAAAAAAA3QAA2JyMXQAA2JyM3QAA2JyMnhxAH4AGXhzcQB+ABZzcQB+ABp3DAAAABA/QAAAAAAAAnQAA3NyMnQAA3NyMXhxAH4AH3hxAH4AFA==";
+
+        User user = User.fromSerializedBase64(serialized);
+        assertEquals(
+            new User("serialization_test_user").withRoles(Arrays.asList("br1", "br2", "br3"))
+                .withSecurityRoles(Arrays.asList("sr1", "sr2"))
+                .withAttributes(ImmutableMap.of("a", "v_a", "b", "v_b")),
+            user
+        );
+    }
+
+    @Test
+    public void withRoles() {
+        User original = new User("test_user").withRoles("a");
+        User modified = original.withRoles("b");
+
+        assertEquals(ImmutableSet.of("a"), original.getRoles());
+        assertEquals(ImmutableSet.of("a", "b"), modified.getRoles());
+    }
+
+    @Test
+    public void withRoles_unmodified() {
+        User original = new User("test_user").withRoles("a");
+        User unmodified = original.withRoles(ImmutableSet.of());
+
+        assertSame(original, unmodified);
+    }
+
+    @Test
+    public void withAttributes() {
+        User original = new User("test_user").withAttributes(Map.of("a", "1"));
+        User modified = original.withAttributes(Map.of("b", "2"));
+
+        assertEquals(ImmutableMap.of("a", "1"), original.getCustomAttributesMap());
+        assertEquals(ImmutableMap.of("a", "1", "b", "2"), modified.getCustomAttributesMap());
+    }
+
+    @Test
+    public void withAttributes_unmodified() {
+        User original = new User("test_user").withAttributes(Map.of("a", "1"));
+        User unmodified = original.withAttributes(Map.of());
+
+        assertSame(original, unmodified);
+    }
+
+    @Test
+    public void withRequestedTenant() {
+        User original = new User("test_user").withRequestedTenant("a");
+        User modified = original.withRequestedTenant("b");
+
+        assertEquals("a", original.getRequestedTenant());
+        assertEquals("b", modified.getRequestedTenant());
+    }
+
+    @Test
+    public void withRequestedTenant_unmodified() {
+        User original = new User("test_user").withRequestedTenant("a");
+        User unmodified = original.withRequestedTenant("a");
+
+        assertSame(original, unmodified);
+    }
+
+    @Test(expected = IllegalArgumentException.class)
+    public void illegalName() {
+        new User("");
+    }
+}

src/main/java/com/amazon/dlic/auth/ldap/LdapUser.java

@@ -11,91 +11,42 @@
 
 package com.amazon.dlic.auth.ldap;
 
-import java.util.Collections;
-import java.util.HashMap;
-import java.util.Map;
-
-import org.opensearch.security.auth.ldap.util.Utils;
-import org.opensearch.security.support.WildcardMatcher;
-import org.opensearch.security.user.AuthCredentials;
-import org.opensearch.security.user.User;
-
-import org.ldaptive.LdapAttribute;
-import org.ldaptive.LdapEntry;
-
 /**
  * This class intentionally remains in the com.amazon.dlic.auth.ldap package
  * to maintain compatibility with serialization/deserialization in mixed cluster
  * environments (nodes running different versions). The class is serialized and
  * passed between nodes, and changing the package would break backward compatibility.
  *
- * Note: This class is planned to be replaced as part of making the User object
- * immutable in a future release or reconsidering java serialization.
+ * This class is only used for deserialization. During deserialization, the readResolve()
+ * method will automatically convert it to a org.opensearch.security.user.User user object.
+ * It will never be used for serialization, only the org.opensearch.security.user.User user object
+ * will be serialized. This is possible because the additional attributes of LdapUser were only
+ * needed during the auth/auth phase, where no inter-node communication is necessary. Afterwards,
+ * the user object is never used as LdapUser, but just as a plain User object.
+ *
+ * This class can be removed as soon as it is no longer possible that a mixed cluster can contain
+ * nodes which send serialized LdapUser objects. This will be the case for OpenSearch 4.0.
  *
  * @see https://github.com/opensearch-project/security/pull/5223
  */
-public class LdapUser extends User {
+public class LdapUser extends org.opensearch.security.user.serialized.User {
 
     private static final long serialVersionUID = 1L;
-    private final transient LdapEntry userEntry;
     private final String originalUsername;
 
-    public LdapUser(
-        final String name,
-        String originalUsername,
-        final LdapEntry userEntry,
-        final AuthCredentials credentials,
-        int customAttrMaxValueLen,
-        WildcardMatcher allowlistedCustomLdapAttrMatcher
-    ) {
-        super(name, null, credentials);
-        this.originalUsername = originalUsername;
-        this.userEntry = userEntry;
-        Map<String, String> attributes = getCustomAttributesMap();
-        attributes.putAll(extractLdapAttributes(originalUsername, userEntry, customAttrMaxValueLen, allowlistedCustomLdapAttrMatcher));
+    public LdapUser() {
+        this.originalUsername = null;
     }
 
     /**
-     * May return null because ldapEntry is transient
-     *
-     * @return ldapEntry or null if object was deserialized
+     * Converts this objects back to User, just after deserialization.
+     * <p>
+     * Note: We do not convert back to LdapUser, but just to User. The additional attributes of
+     * LdapUser were only needed during the auth/auth phase, where no inter-node communication
+     * is necessary. Afterwards, the user object is never used as LdapUser, but just as a plain User
+     * object.
      */
-    public LdapEntry getUserEntry() {
-        return userEntry;
-    }
-
-    public String getDn() {
-        return userEntry.getDn();
-    }
-
-    public String getOriginalUsername() {
-        return originalUsername;
-    }
-
-    public static Map<String, String> extractLdapAttributes(
-        String originalUsername,
-        final LdapEntry userEntry,
-        int customAttrMaxValueLen,
-        WildcardMatcher allowlistedCustomLdapAttrMatcher
-    ) {
-        Map<String, String> attributes = new HashMap<>();
-        attributes.put("ldap.original.username", originalUsername);
-        attributes.put("ldap.dn", userEntry.getDn());
-
-        if (customAttrMaxValueLen > 0) {
-            for (LdapAttribute attr : userEntry.getAttributes()) {
-                if (attr != null && !attr.isBinary() && !attr.getName().toLowerCase().contains("password")) {
-                    final String val = Utils.getSingleStringValue(attr);
-                    // only consider attributes which are not binary and where its value is not
-                    // longer than customAttrMaxValueLen characters
-                    if (val != null && val.length() > 0 && val.length() <= customAttrMaxValueLen) {
-                        if (allowlistedCustomLdapAttrMatcher.test(attr.getName())) {
-                            attributes.put("attr.ldap." + attr.getName(), val);
-                        }
-                    }
-                }
-            }
-        }
-        return Collections.unmodifiableMap(attributes);
+    protected Object readResolve() {
+        return super.readResolve();
     }
 }