Skip to content

Commit 64c3709

Browse files
nibixnibix
authored
Immutable user object (#5212)
Signed-off-by: Nils Bandener <nils.bandener@eliatra.com>
1 parent 9d054c2 commit 64c3709

File tree

56 files changed

+1569
-1338
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

56 files changed

+1569
-1338
lines changed

‎CHANGELOG.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
1313
- Use extendedPlugins in integrationTest framework for sample resource plugin testing ([#5322](https://github.com/opensearch-project/security/pull/5322))
1414
- Refactor ResourcePermissions to refer to action groups as access levels ([#5335](https://github.com/opensearch-project/security/pull/5335))
1515

16+
- Performance improvements: Immutable user object ([#5212])
17+
1618
### Dependencies
1719
- Bump `guava_version` from 33.4.6-jre to 33.4.8-jre ([#5284](https://github.com/opensearch-project/security/pull/5284))
1820
- Bump `spring_version` from 6.2.5 to 6.2.7 ([#5283](https://github.com/opensearch-project/security/pull/5283), [#5345](https://github.com/opensearch-project/security/pull/5345))

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

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1071,8 +1071,7 @@ static SecurityDynamicConfiguration<RoleV7> createRoles(int numberOfRoles, int n
10711071
}
10721072

10731073
static PrivilegesEvaluationContext ctx(String... roles) {
1074-
User user = new User("test_user");
1075-
user.addAttributes(ImmutableMap.of("attrs.dept_no", "a11"));
1074+
User user = new User("test_user").withAttributes(ImmutableMap.of("attrs.dept_no", "a11"));
10761075
return new PrivilegesEvaluationContext(
10771076
user,
10781077
ImmutableSet.copyOf(roles),
@@ -1086,8 +1085,7 @@ static PrivilegesEvaluationContext ctx(String... roles) {
10861085
}
10871086

10881087
static PrivilegesEvaluationContext ctxByUsername(String username) {
1089-
User user = new User(username);
1090-
user.addAttributes(ImmutableMap.of("attrs.dept_no", "a11"));
1088+
User user = new User(username).withAttributes(ImmutableMap.of("attrs.dept_no", "a11"));
10911089
return new PrivilegesEvaluationContext(
10921090
user,
10931091
ImmutableSet.of(),

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

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -234,10 +234,7 @@ public void equals() {
234234
private static PrivilegesEvaluationContext ctx() {
235235
IndexNameExpressionResolver indexNameExpressionResolver = new IndexNameExpressionResolver(new ThreadContext(Settings.EMPTY));
236236
IndexResolverReplacer indexResolverReplacer = new IndexResolverReplacer(indexNameExpressionResolver, () -> CLUSTER_STATE, null);
237-
User user = new User("test_user");
238-
user.addAttributes(ImmutableMap.of("attrs.a11", "a11"));
239-
user.addAttributes(ImmutableMap.of("attrs.year", "year"));
240-
237+
User user = new User("test_user").withAttributes(ImmutableMap.of("attrs.a11", "a11", "attrs.year", "year"));
241238
return new PrivilegesEvaluationContext(
242239
user,
243240
ImmutableSet.of(),

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

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1191,9 +1191,7 @@ UserSpec attribute(String name, String value) {
11911191
}
11921192

11931193
User buildUser() {
1194-
User user = new User("test_user_" + description);
1195-
user.addAttributes(this.attributes);
1196-
return user;
1194+
return new User("test_user_" + description).withAttributes(this.attributes);
11971195
}
11981196

11991197
@Override

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

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,65 @@
1+
/*
2+
* SPDX-License-Identifier: Apache-2.0
3+
*
4+
* The OpenSearch Contributors require contributions made to
5+
* this file be licensed under the Apache-2.0 license or a
6+
* compatible open source license.
7+
*
8+
* Modifications Copyright OpenSearch Contributors. See
9+
* GitHub history for details.
10+
*/
11+
12+
package org.opensearch.security.user;
13+
14+
import java.util.Arrays;
15+
import java.util.Collection;
16+
17+
import com.google.common.collect.ImmutableSet;
18+
import org.junit.Test;
19+
import org.junit.runner.RunWith;
20+
import org.junit.runners.Parameterized;
21+
22+
import org.opensearch.OpenSearchException;
23+
import org.opensearch.common.settings.Settings;
24+
25+
import static org.junit.Assert.assertEquals;
26+
import static org.junit.Assert.assertTrue;
27+
import static org.junit.Assert.fail;
28+
29+
@RunWith(Parameterized.class)
30+
public class UserFactoryTest {
31+
UserFactory subject;
32+
33+
@Test
34+
public void parse_successful() {
35+
User source = new User("test_user").withRoles(ImmutableSet.of("a", "b"));
36+
User target = subject.fromSerializedBase64(source.toSerializedBase64());
37+
38+
assertEquals(source, target);
39+
}
40+
41+
@Test
42+
public void parse_invalid() {
43+
try {
44+
User target = subject.fromSerializedBase64("invaliddata123");
45+
fail("Should have failed; got " + target);
46+
} catch (Exception e) {
47+
assertTrue(
48+
"Got invalid stream header " + e,
49+
e instanceof OpenSearchException && e.getMessage().contains("invalid stream header")
50+
);
51+
}
52+
}
53+
54+
public UserFactoryTest(UserFactory subject, String name) {
55+
this.subject = subject;
56+
}
57+
58+
@Parameterized.Parameters(name = "{1}")
59+
public static Collection<Object[]> params() {
60+
return Arrays.asList(
61+
new Object[] { new UserFactory.Simple(), "Simple" },
62+
new Object[] { new UserFactory.Caching(Settings.EMPTY), "Caching" }
63+
);
64+
}
65+
}

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

Lines changed: 138 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,138 @@
1+
/*
2+
* SPDX-License-Identifier: Apache-2.0
3+
*
4+
* The OpenSearch Contributors require contributions made to
5+
* this file be licensed under the Apache-2.0 license or a
6+
* compatible open source license.
7+
*
8+
* Modifications Copyright OpenSearch Contributors. See
9+
* GitHub history for details.
10+
*/
11+
package org.opensearch.security.user;
12+
13+
import java.util.Arrays;
14+
import java.util.Map;
15+
16+
import com.google.common.collect.ImmutableMap;
17+
import com.google.common.collect.ImmutableSet;
18+
import org.junit.Test;
19+
20+
import org.opensearch.security.support.Base64Helper;
21+
22+
import static org.junit.Assert.assertEquals;
23+
import static org.junit.Assert.assertSame;
24+
25+
public class UserTest {
26+
@Test
27+
public void serialization() {
28+
User user = new User("serialization_test_user").withRoles(Arrays.asList("br1", "br2", "br3"))
29+
.withSecurityRoles(Arrays.asList("sr1", "sr2"))
30+
.withAttributes(ImmutableMap.of("a", "v_a", "b", "v_b"));
31+
32+
String serialized = Base64Helper.serializeObject(user);
33+
User user2 = User.fromSerializedBase64(serialized);
34+
assertEquals(user, user2);
35+
36+
}
37+
38+
@Test
39+
public void deserializationFrom2_19() {
40+
// The following base64 string was produced by the following code on OpenSearch 2.19
41+
// User user = new User("serialization_test_user");
42+
// user.addRoles(Arrays.asList("br1", "br2", "br3"));
43+
// user.addSecurityRoles(Arrays.asList("sr1", "sr2"));
44+
// user.addAttributes(ImmutableMap.of("a", "v_a", "b", "v_b"));
45+
// println(Base64JDKHelper.serializeObject(user));
46+
String serialized =
47+
"rO0ABXNyACFvcmcub3BlbnNlYXJjaC5zZWN1cml0eS51c2VyLlVzZXKzqL2T65dH3AIABloACmlzSW5qZWN0ZWRMAAphdHRyaWJ1dGVzdAAPTGphdmEvdXRpbC9NYXA7TAAEbmFtZXQAEkxqYXZhL2xhbmcvU3RyaW5nO0wAD3JlcXVlc3RlZFRlbmFudHEAfgACTAAFcm9sZXN0AA9MamF2YS91dGlsL1NldDtMAA1zZWN1cml0eVJvbGVzcQB+AAN4cABzcgAlamF2YS51dGlsLkNvbGxlY3Rpb25zJFN5bmNocm9uaXplZE1hcBtz+QlLSzl7AwACTAABbXEAfgABTAAFbXV0ZXh0ABJMamF2YS9sYW5nL09iamVjdDt4cHNyABFqYXZhLnV0aWwuSGFzaE1hcAUH2sHDFmDRAwACRgAKbG9hZEZhY3RvckkACXRocmVzaG9sZHhwP0AAAAAAAAN3CAAAAAQAAAACdAABYXQAA3ZfYXQAAWJ0AAN2X2J4cQB+AAd4dAAXc2VyaWFsaXphdGlvbl90ZXN0X3VzZXJwc3IAJWphdmEudXRpbC5Db2xsZWN0aW9ucyRTeW5jaHJvbml6ZWRTZXQGw8J5Au7fPAIAAHhyACxqYXZhLnV0aWwuQ29sbGVjdGlvbnMkU3luY2hyb25pemVkQ29sbGVjdGlvbiph+E0JnJm1AwACTAABY3QAFkxqYXZhL3V0aWwvQ29sbGVjdGlvbjtMAAVtdXRleHEAfgAGeHBzcgARamF2YS51dGlsLkhhc2hTZXS6RIWVlri3NAMAAHhwdwwAAAAQP0AAAAAAAAN0AANicjF0AANicjN0AANicjJ4cQB+ABJ4c3EAfgAPc3EAfgATdwwAAAAQP0AAAAAAAAJ0AANzcjJ0AANzcjF4cQB+ABh4";
48+
49+
User user = User.fromSerializedBase64(serialized);
50+
assertEquals(
51+
new User("serialization_test_user").withRoles(Arrays.asList("br1", "br2", "br3"))
52+
.withSecurityRoles(Arrays.asList("sr1", "sr2"))
53+
.withAttributes(ImmutableMap.of("a", "v_a", "b", "v_b")),
54+
user
55+
);
56+
}
57+
58+
@Test
59+
public void deserializationLdapUserFrom2_19() {
60+
// The following base64 string was produced by the following code on OpenSearch 2.19
61+
// LdapUser user = new LdapUser("serialization_test_user",
62+
// "original_user_name",
63+
// new LdapEntry("cn=test,ou=people,o=TEST", new LdapAttribute("test_ldap_attr", "test_ldap_attr_value")),
64+
// new AuthCredentials("test_user", "secret".getBytes(StandardCharsets.UTF_8)),
65+
// 100,
66+
// WildcardMatcher.ANY);
67+
// user.addRoles(Arrays.asList("br1", "br2", "br3"));
68+
// user.addSecurityRoles(Arrays.asList("sr1", "sr2"));
69+
// user.addAttributes(ImmutableMap.of("a", "v_a", "b", "v_b"));
70+
// println(Base64JDKHelper.serializeObject(user));
71+
String serialized =
72+
"rO0ABXNyACJjb20uYW1hem9uLmRsaWMuYXV0aC5sZGFwLkxkYXBVc2VyAAAAAAAAAAECAAFMABBvcmlnaW5hbFVzZXJuYW1ldAASTGphdmEvbGFuZy9TdHJpbmc7eHIAIW9yZy5vcGVuc2VhcmNoLnNlY3VyaXR5LnVzZXIuVXNlcrOovZPrl0fcAgAGWgAKaXNJbmplY3RlZEwACmF0dHJpYnV0ZXN0AA9MamF2YS91dGlsL01hcDtMAARuYW1lcQB+AAFMAA9yZXF1ZXN0ZWRUZW5hbnRxAH4AAUwABXJvbGVzdAAPTGphdmEvdXRpbC9TZXQ7TAANc2VjdXJpdHlSb2xlc3EAfgAEeHAAc3IAJWphdmEudXRpbC5Db2xsZWN0aW9ucyRTeW5jaHJvbml6ZWRNYXAbc/kJS0s5ewMAAkwAAW1xAH4AA0wABW11dGV4dAASTGphdmEvbGFuZy9PYmplY3Q7eHBzcgARamF2YS51dGlsLkhhc2hNYXAFB9rBwxZg0QMAAkYACmxvYWRGYWN0b3JJAAl0aHJlc2hvbGR4cD9AAAAAAAAGdwgAAAAIAAAABXQAB2xkYXAuZG50ABhjbj10ZXN0LG91PXBlb3BsZSxvPVRFU1R0AAFhdAADdl9hdAAYYXR0ci5sZGFwLnRlc3RfbGRhcF9hdHRydAAUdGVzdF9sZGFwX2F0dHJfdmFsdWV0AAFidAADdl9idAAWbGRhcC5vcmlnaW5hbC51c2VybmFtZXQAEm9yaWdpbmFsX3VzZXJfbmFtZXhxAH4ACHh0ABdzZXJpYWxpemF0aW9uX3Rlc3RfdXNlcnBzcgAlamF2YS51dGlsLkNvbGxlY3Rpb25zJFN5bmNocm9uaXplZFNldAbDwnkC7t88AgAAeHIALGphdmEudXRpbC5Db2xsZWN0aW9ucyRTeW5jaHJvbml6ZWRDb2xsZWN0aW9uKmH4TQmcmbUDAAJMAAFjdAAWTGphdmEvdXRpbC9Db2xsZWN0aW9uO0wABW11dGV4cQB+AAd4cHNyABFqYXZhLnV0aWwuSGFzaFNldLpEhZWWuLc0AwAAeHB3DAAAABA/QAAAAAAAA3QAA2JyMXQAA2JyM3QAA2JyMnhxAH4AGXhzcQB+ABZzcQB+ABp3DAAAABA/QAAAAAAAAnQAA3NyMnQAA3NyMXhxAH4AH3hxAH4AFA==";
73+
74+
User user = User.fromSerializedBase64(serialized);
75+
assertEquals(
76+
new User("serialization_test_user").withRoles(Arrays.asList("br1", "br2", "br3"))
77+
.withSecurityRoles(Arrays.asList("sr1", "sr2"))
78+
.withAttributes(ImmutableMap.of("a", "v_a", "b", "v_b")),
79+
user
80+
);
81+
}
82+
83+
@Test
84+
public void withRoles() {
85+
User original = new User("test_user").withRoles("a");
86+
User modified = original.withRoles("b");
87+
88+
assertEquals(ImmutableSet.of("a"), original.getRoles());
89+
assertEquals(ImmutableSet.of("a", "b"), modified.getRoles());
90+
}
91+
92+
@Test
93+
public void withRoles_unmodified() {
94+
User original = new User("test_user").withRoles("a");
95+
User unmodified = original.withRoles(ImmutableSet.of());
96+
97+
assertSame(original, unmodified);
98+
}
99+
100+
@Test
101+
public void withAttributes() {
102+
User original = new User("test_user").withAttributes(Map.of("a", "1"));
103+
User modified = original.withAttributes(Map.of("b", "2"));
104+
105+
assertEquals(ImmutableMap.of("a", "1"), original.getCustomAttributesMap());
106+
assertEquals(ImmutableMap.of("a", "1", "b", "2"), modified.getCustomAttributesMap());
107+
}
108+
109+
@Test
110+
public void withAttributes_unmodified() {
111+
User original = new User("test_user").withAttributes(Map.of("a", "1"));
112+
User unmodified = original.withAttributes(Map.of());
113+
114+
assertSame(original, unmodified);
115+
}
116+
117+
@Test
118+
public void withRequestedTenant() {
119+
User original = new User("test_user").withRequestedTenant("a");
120+
User modified = original.withRequestedTenant("b");
121+
122+
assertEquals("a", original.getRequestedTenant());
123+
assertEquals("b", modified.getRequestedTenant());
124+
}
125+
126+
@Test
127+
public void withRequestedTenant_unmodified() {
128+
User original = new User("test_user").withRequestedTenant("a");
129+
User unmodified = original.withRequestedTenant("a");
130+
131+
assertSame(original, unmodified);
132+
}
133+
134+
@Test(expected = IllegalArgumentException.class)
135+
public void illegalName() {
136+
new User("");
137+
}
138+
}

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

Lines changed: 20 additions & 69 deletions
Original file line numberDiff line numberDiff line change
@@ -11,91 +11,42 @@
1111

1212
package com.amazon.dlic.auth.ldap;
1313

14-
import java.util.Collections;
15-
import java.util.HashMap;
16-
import java.util.Map;
17-
18-
import org.opensearch.security.auth.ldap.util.Utils;
19-
import org.opensearch.security.support.WildcardMatcher;
20-
import org.opensearch.security.user.AuthCredentials;
21-
import org.opensearch.security.user.User;
22-
23-
import org.ldaptive.LdapAttribute;
24-
import org.ldaptive.LdapEntry;
25-
2614
/**
2715
* This class intentionally remains in the com.amazon.dlic.auth.ldap package
2816
* to maintain compatibility with serialization/deserialization in mixed cluster
2917
* environments (nodes running different versions). The class is serialized and
3018
* passed between nodes, and changing the package would break backward compatibility.
3119
*
32-
* Note: This class is planned to be replaced as part of making the User object
33-
* immutable in a future release or reconsidering java serialization.
20+
* This class is only used for deserialization. During deserialization, the readResolve()
21+
* method will automatically convert it to a org.opensearch.security.user.User user object.
22+
* It will never be used for serialization, only the org.opensearch.security.user.User user object
23+
* will be serialized. This is possible because the additional attributes of LdapUser were only
24+
* needed during the auth/auth phase, where no inter-node communication is necessary. Afterwards,
25+
* the user object is never used as LdapUser, but just as a plain User object.
26+
*
27+
* This class can be removed as soon as it is no longer possible that a mixed cluster can contain
28+
* nodes which send serialized LdapUser objects. This will be the case for OpenSearch 4.0.
3429
*
3530
* @see https://github.com/opensearch-project/security/pull/5223
3631
*/
37-
public class LdapUser extends User {
32+
public class LdapUser extends org.opensearch.security.user.serialized.User {
3833

3934
private static final long serialVersionUID = 1L;
40-
private final transient LdapEntry userEntry;
4135
private final String originalUsername;
4236

43-
public LdapUser(
44-
final String name,
45-
String originalUsername,
46-
final LdapEntry userEntry,
47-
final AuthCredentials credentials,
48-
int customAttrMaxValueLen,
49-
WildcardMatcher allowlistedCustomLdapAttrMatcher
50-
) {
51-
super(name, null, credentials);
52-
this.originalUsername = originalUsername;
53-
this.userEntry = userEntry;
54-
Map<String, String> attributes = getCustomAttributesMap();
55-
attributes.putAll(extractLdapAttributes(originalUsername, userEntry, customAttrMaxValueLen, allowlistedCustomLdapAttrMatcher));
37+
public LdapUser() {
38+
this.originalUsername = null;
5639
}
5740

5841
/**
59-
* May return null because ldapEntry is transient
60-
*
61-
* @return ldapEntry or null if object was deserialized
42+
* Converts this objects back to User, just after deserialization.
43+
* <p>
44+
* Note: We do not convert back to LdapUser, but just to User. The additional attributes of
45+
* LdapUser were only needed during the auth/auth phase, where no inter-node communication
46+
* is necessary. Afterwards, the user object is never used as LdapUser, but just as a plain User
47+
* object.
6248
*/
63-
public LdapEntry getUserEntry() {
64-
return userEntry;
65-
}
66-
67-
public String getDn() {
68-
return userEntry.getDn();
69-
}
70-
71-
public String getOriginalUsername() {
72-
return originalUsername;
73-
}
74-
75-
public static Map<String, String> extractLdapAttributes(
76-
String originalUsername,
77-
final LdapEntry userEntry,
78-
int customAttrMaxValueLen,
79-
WildcardMatcher allowlistedCustomLdapAttrMatcher
80-
) {
81-
Map<String, String> attributes = new HashMap<>();
82-
attributes.put("ldap.original.username", originalUsername);
83-
attributes.put("ldap.dn", userEntry.getDn());
84-
85-
if (customAttrMaxValueLen > 0) {
86-
for (LdapAttribute attr : userEntry.getAttributes()) {
87-
if (attr != null && !attr.isBinary() && !attr.getName().toLowerCase().contains("password")) {
88-
final String val = Utils.getSingleStringValue(attr);
89-
// only consider attributes which are not binary and where its value is not
90-
// longer than customAttrMaxValueLen characters
91-
if (val != null && val.length() > 0 && val.length() <= customAttrMaxValueLen) {
92-
if (allowlistedCustomLdapAttrMatcher.test(attr.getName())) {
93-
attributes.put("attr.ldap." + attr.getName(), val);
94-
}
95-
}
96-
}
97-
}
98-
}
99-
return Collections.unmodifiableMap(attributes);
49+
protected Object readResolve() {
50+
return super.readResolve();
10051
}
10152
}

0 commit comments

Comments
 (0)