Skip to content

Commit 4012ecf

Browse files
markdboydcwperks
markdboyd
and
cwperks
authored
Update user parsing to include custom attributes (#827)
* update junit library to 5.11.4 to enable debugging in VScode. see microsoft/vscode-java-test#1740 Signed-off-by: Mark Boyd <[email protected]> * update User.parse() to parse custom attribute names from string Signed-off-by: Mark Boyd <[email protected]> * update unit test to verify that User.parse() parses custom attribute names correctly from user string Signed-off-by: Mark Boyd <[email protected]> * apply spotless formatting Signed-off-by: Mark Boyd <[email protected]> * add more debug logs Signed-off-by: Mark Boyd <[email protected]> * update debug statements Signed-off-by: Mark Boyd <[email protected]> * remove debug logging Signed-off-by: Mark Boyd <[email protected]> * remove changes to build.gradle Signed-off-by: Mark Boyd <[email protected]> * add logic in User.parse to handle case where tenant is "null" as a literal string Signed-off-by: Mark Boyd <[email protected]> * add test for User.parse where tenant is literal string "null" Signed-off-by: Mark Boyd <[email protected]> * add initial pass at deserializing user custom attributes from thread context Signed-off-by: Mark Boyd <[email protected]> * apply spotless formatting Signed-off-by: Mark Boyd <[email protected]> * add Base64Helper for deserializing base 64 encoded content Signed-off-by: Mark Boyd <[email protected]> * move files into correct directory structure Signed-off-by: Mark Boyd <[email protected]> * update junit-jupiter-engine to 5.11.4 Signed-off-by: Mark Boyd <[email protected]> * remove unnecessary throws IOException in definition of User.parse() Signed-off-by: Mark Boyd <[email protected]> * update tests for User Signed-off-by: Mark Boyd <[email protected]> * update InjectSecurityTest for compatibility with user custom attributes as a map instead of a list Signed-off-by: Mark Boyd <[email protected]> * update testParseUserString to test for serialized custom user attributes Signed-off-by: Mark Boyd <[email protected]> * refactor parsing of tenant information from string in User.parse Signed-off-by: Mark Boyd <[email protected]> * add additional classes to SafeSerializationUtils.SAFE_CLASS_NAMES Signed-off-by: Mark Boyd <[email protected]> * Update src/test/java/org/opensearch/commons/authuser/UserTest.java Co-authored-by: Craig Perkins <[email protected]> Signed-off-by: Mark Boyd <[email protected]> * fix variable reference Signed-off-by: Mark Boyd <[email protected]> * stub out test for User.parse of XContent Signed-off-by: Mark Boyd <[email protected]> * fix arguments to create User in TestHelpers.kt Signed-off-by: Mark Boyd <[email protected]> * fix testParseUserXContent test and add more test assertions for other user properties Signed-off-by: Mark Boyd <[email protected]> * add Base64HelperTest Signed-off-by: Mark Boyd <[email protected]> * add SafeSerializationUtilsTest.java Signed-off-by: Mark Boyd <[email protected]> * run spotlessApply Signed-off-by: Mark Boyd <[email protected]> * update User.toString() to use TreeMap for custom attributes to keep attributes naturally sorted by key Signed-off-by: Mark Boyd <[email protected]> * update User tests for parsing from JSON string to include custom attributes as an object Signed-off-by: Mark Boyd <[email protected]> * add test for parsing user custom attributes from JSON Signed-off-by: Mark Boyd <[email protected]> * update custom_attributes in XContentTests fixture Signed-off-by: Mark Boyd <[email protected]> * remove special handling of requestedTenant in User.parse() Signed-off-by: Mark Boyd <[email protected]> * update expectation for tenant in testParseUserStringNameWithNullTenant Signed-off-by: Mark Boyd <[email protected]> * update User to preserve backwards compatibilty for custom attributes when parsing XContent Signed-off-by: Mark Boyd <[email protected]> * include parsing of custom attribute names from JSON in User.parse() for backwards compatibility Signed-off-by: Mark Boyd <[email protected]> * apply spotless formatting Signed-off-by: Mark Boyd <[email protected]> * update ser/de for user to stream to handle backwards compatibility for custom attribute names Signed-off-by: Mark Boyd <[email protected]> * add test for streaming user on older versions of OpenSearch for backwards compatibility Signed-off-by: Mark Boyd <[email protected]> * fix bad rebasing Signed-off-by: Mark Boyd <[email protected]> * fix bad rebasing Signed-off-by: Mark Boyd <[email protected]> * update comment on User.parse() for string to specify expected string format Signed-off-by: Mark Boyd <[email protected]> * re-add constructor for User with custom attribute names for backwards compatibility Signed-off-by: Mark Boyd <[email protected]> * re-add getter for custom attribute names to User for backwards compatibility Signed-off-by: Mark Boyd <[email protected]> * add test of User constructor with custom attribute names for backwards compatibility Signed-off-by: Mark Boyd <[email protected]> * fix spotless formatting issues Signed-off-by: Mark Boyd <[email protected]> * update InjectSecurity.injectUserInfo() to set user custom attributes in a custom thread context Signed-off-by: Mark Boyd <[email protected]> * apply spotless formatting Signed-off-by: Mark Boyd <[email protected]> * update User.toXContent() to conditionally include custom attributes when non-empty and fall back to empty list of custom attribute names when custom attributes are empty Signed-off-by: Mark Boyd <[email protected]> * fix unit test expectation Signed-off-by: Mark Boyd <[email protected]> * apply spotless formatting Signed-off-by: Mark Boyd <[email protected]> --------- Signed-off-by: Mark Boyd <[email protected]> Signed-off-by: Mark Boyd <[email protected]> Co-authored-by: Craig Perkins <[email protected]>
1 parent 2ec001f commit 4012ecf

File tree

11 files changed

+689
-36
lines changed

11 files changed

+689
-36
lines changed

‎build.gradle‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -92,12 +92,12 @@ dependencies {
9292
testImplementation "org.opensearch.test:framework:${opensearch_version}"
9393
testImplementation "org.jetbrains.kotlin:kotlin-test:${kotlin_version}"
9494
testImplementation "org.mockito:mockito-core:3.10.0"
95-
testImplementation 'org.junit.jupiter:junit-jupiter-api:5.7.2'
95+
testImplementation 'org.junit.jupiter:junit-jupiter-api:5.11.4'
9696
testImplementation 'org.mockito:mockito-junit-jupiter:3.10.0'
9797
testImplementation "com.nhaarman.mockitokotlin2:mockito-kotlin:2.2.0"
9898
testImplementation "com.cronutils:cron-utils:9.2.1"
9999
testImplementation "commons-validator:commons-validator:1.7"
100-
testRuntimeOnly 'org.junit.jupiter:junit-jupiter-engine:5.7.2'
100+
testRuntimeOnly 'org.junit.jupiter:junit-jupiter-engine:5.11.4'
101101

102102
ktlint "com.pinterest:ktlint:0.47.1"
103103
}

‎src/main/java/org/opensearch/commons/ConfigConstants.java‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,7 @@ private static Setting<SecureString> createFallbackInsecureSetting(String key) {
5252
);
5353
public static final String OPENSEARCH_SECURITY_INJECTED_ROLES = "opendistro_security_injected_roles";
5454
public static final String INJECTED_USER = "injected_user";
55+
public static final String INJECTED_USER_CUSTOM_ATTRIBUTES = "injected_user_custom_attributes";
5556
public static final String OPENSEARCH_SECURITY_USE_INJECTED_USER_FOR_PLUGINS = "plugins.security_use_injected_user_for_plugins";
5657
public static final String OPENSEARCH_SECURITY_SSL_HTTP_ENABLED = "plugins.security.ssl.http.enabled";
5758
public static final String OPENSEARCH_SECURITY_AUTHCZ_ADMIN_DN = "plugins.security.authcz.admin_dn";

‎src/main/java/org/opensearch/commons/InjectSecurity.java‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
package org.opensearch.commons;
77

88
import static org.opensearch.commons.ConfigConstants.INJECTED_USER;
9+
import static org.opensearch.commons.ConfigConstants.INJECTED_USER_CUSTOM_ATTRIBUTES;
910
import static org.opensearch.commons.ConfigConstants.OPENSEARCH_SECURITY_INJECTED_ROLES;
1011
import static org.opensearch.commons.ConfigConstants.OPENSEARCH_SECURITY_USE_INJECTED_USER_FOR_PLUGINS;
1112
import static org.opensearch.commons.authuser.Utils.escapePipe;
@@ -155,7 +156,15 @@ public void injectUserInfo(final User user) {
155156
if (!Strings.isNullOrEmpty(requestedTenant)) {
156157
joiner.add(escapePipe(requestedTenant));
157158
}
159+
158160
threadContext.putTransient(ConfigConstants.OPENSEARCH_SECURITY_USER_INFO_THREAD_CONTEXT, joiner.toString());
161+
162+
if (threadContext.getTransient(INJECTED_USER_CUSTOM_ATTRIBUTES) == null) {
163+
threadContext.putTransient(INJECTED_USER_CUSTOM_ATTRIBUTES, user.getCustomAttributes());
164+
log.debug("{}, InjectSecurity - inject user custom attributes: {}", Thread.currentThread().getName(), id);
165+
} else {
166+
log.error("{}, InjectSecurity - most likely thread context corruption : {}", Thread.currentThread().getName(), id);
167+
}
159168
}
160169

161170
/**

‎src/main/java/org/opensearch/commons/authuser/User.java‎

Lines changed: 89 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -12,9 +12,12 @@
1212
import java.util.ArrayList;
1313
import java.util.Arrays;
1414
import java.util.Collections;
15+
import java.util.HashMap;
1516
import java.util.List;
1617
import java.util.Map;
1718
import java.util.Objects;
19+
import java.util.TreeMap;
20+
import java.util.stream.Collectors;
1821

1922
import org.apache.hc.core5.http.ParseException;
2023
import org.apache.hc.core5.http.io.entity.EntityUtils;
@@ -26,6 +29,7 @@
2629
import org.opensearch.common.xcontent.XContentHelper;
2730
import org.opensearch.common.xcontent.json.JsonXContent;
2831
import org.opensearch.commons.ConfigConstants;
32+
import org.opensearch.commons.authuser.util.Base64Helper;
2933
import org.opensearch.core.common.Strings;
3034
import org.opensearch.core.common.io.stream.StreamInput;
3135
import org.opensearch.core.common.io.stream.StreamOutput;
@@ -45,13 +49,14 @@ final public class User implements Writeable, ToXContent {
4549
public static final String BACKEND_ROLES_FIELD = "backend_roles";
4650
public static final String ROLES_FIELD = "roles";
4751
public static final String CUSTOM_ATTRIBUTE_NAMES_FIELD = "custom_attribute_names";
52+
public static final String CUSTOM_ATTRIBUTES_FIELD = "custom_attributes";
4853
public static final String REQUESTED_TENANT_FIELD = "user_requested_tenant";
4954
public static final String REQUESTED_TENANT_ACCESS = "user_requested_tenant_access";
5055

5156
private final String name;
5257
private final List<String> backendRoles;
5358
private final List<String> roles;
54-
private final List<String> customAttNames;
59+
private final Map<String, String> customAttributes;
5560
@Nullable
5661
private final String requestedTenant;
5762
@Nullable
@@ -61,16 +66,25 @@ public User() {
6166
name = "";
6267
backendRoles = new ArrayList<>();
6368
roles = new ArrayList<>();
64-
customAttNames = new ArrayList<>();
69+
customAttributes = new HashMap<>();
6570
requestedTenant = null;
6671
requestedTenantAccess = null;
6772
}
6873

74+
public User(final String name, final List<String> backendRoles, List<String> roles, Map<String, String> customAttributes) {
75+
this.name = name;
76+
this.backendRoles = backendRoles;
77+
this.roles = roles;
78+
this.customAttributes = customAttributes;
79+
this.requestedTenant = null;
80+
this.requestedTenantAccess = null;
81+
}
82+
6983
public User(final String name, final List<String> backendRoles, List<String> roles, List<String> customAttNames) {
7084
this.name = name;
7185
this.backendRoles = backendRoles;
7286
this.roles = roles;
73-
this.customAttNames = customAttNames;
87+
this.customAttributes = this.convertCustomAttributeNamesToMap(customAttNames);
7488
this.requestedTenant = null;
7589
this.requestedTenantAccess = null;
7690
}
@@ -79,13 +93,13 @@ public User(
7993
final String name,
8094
final List<String> backendRoles,
8195
final List<String> roles,
82-
final List<String> customAttNames,
96+
final Map<String, String> customAttributes,
8397
@Nullable final String requestedTenant
8498
) {
8599
this.name = name;
86100
this.backendRoles = backendRoles;
87101
this.roles = roles;
88-
this.customAttNames = customAttNames;
102+
this.customAttributes = customAttributes;
89103
this.requestedTenant = requestedTenant;
90104
this.requestedTenantAccess = null;
91105
}
@@ -94,14 +108,14 @@ public User(
94108
final String name,
95109
final List<String> backendRoles,
96110
final List<String> roles,
97-
final List<String> customAttNames,
111+
final Map<String, String> customAttributes,
98112
@Nullable final String requestedTenant,
99113
@Nullable final String requestedTenantAccess
100114
) {
101115
this.name = name;
102116
this.backendRoles = backendRoles;
103117
this.roles = roles;
104-
this.customAttNames = customAttNames;
118+
this.customAttributes = customAttributes;
105119
this.requestedTenant = requestedTenant;
106120
this.requestedTenantAccess = requestedTenantAccess;
107121
}
@@ -125,7 +139,16 @@ public User(String json) {
125139
name = (String) mapValue.get("user_name");
126140
backendRoles = (List<String>) mapValue.get("backend_roles");
127141
roles = (List<String>) mapValue.get("roles");
128-
customAttNames = (List<String>) mapValue.get("custom_attribute_names");
142+
143+
Map<String, String> customAttributesFromJson = (Map<String, String>) mapValue.get("custom_attributes");
144+
List<String> customAttNames = (List<String>) mapValue.get("custom_attribute_names");
145+
146+
if (customAttributesFromJson != null) {
147+
customAttributes = customAttributesFromJson;
148+
} else {
149+
customAttributes = this.convertCustomAttributeNamesToMap(customAttNames);
150+
}
151+
129152
requestedTenant = (String) mapValue.getOrDefault("user_requested_tenant", null);
130153
requestedTenantAccess = (String) mapValue.getOrDefault("user_requested_tenant_access", null);
131154
}
@@ -134,7 +157,12 @@ public User(StreamInput in) throws IOException {
134157
name = in.readString();
135158
backendRoles = in.readStringList();
136159
roles = in.readStringList();
137-
customAttNames = in.readStringList();
160+
if (in.getVersion().onOrAfter(Version.V_3_2_0)) {
161+
customAttributes = in.readMap(StreamInput::readString, StreamInput::readString);
162+
} else {
163+
List<String> customAttNames = in.readStringList();
164+
customAttributes = this.convertCustomAttributeNamesToMap(customAttNames);
165+
}
138166
requestedTenant = in.readOptionalString();
139167
if (in.getVersion().onOrAfter(Version.V_3_2_0)) {
140168
requestedTenantAccess = in.readOptionalString();
@@ -147,7 +175,7 @@ public static User parse(XContentParser parser) throws IOException {
147175
String name = "";
148176
List<String> backendRoles = new ArrayList<>();
149177
List<String> roles = new ArrayList<>();
150-
List<String> customAttNames = new ArrayList<>();
178+
Map<String, String> customAttributes = new HashMap<>();
151179
String requestedTenant = null;
152180
String requestedTenantAccess = null;
153181

@@ -171,10 +199,19 @@ public static User parse(XContentParser parser) throws IOException {
171199
roles.add(parser.text());
172200
}
173201
break;
202+
case CUSTOM_ATTRIBUTES_FIELD:
203+
ensureExpectedToken(XContentParser.Token.START_OBJECT, parser.currentToken(), parser);
204+
while (parser.nextToken() != XContentParser.Token.END_OBJECT) {
205+
String attrName = parser.currentName();
206+
parser.nextToken();
207+
String attrValue = parser.text();
208+
customAttributes.put(attrName, attrValue);
209+
}
210+
break;
174211
case CUSTOM_ATTRIBUTE_NAMES_FIELD:
175212
ensureExpectedToken(XContentParser.Token.START_ARRAY, parser.currentToken(), parser);
176213
while (parser.nextToken() != XContentParser.Token.END_ARRAY) {
177-
customAttNames.add(parser.text());
214+
customAttributes.put(parser.text(), null);
178215
}
179216
break;
180217
case REQUESTED_TENANT_FIELD:
@@ -187,14 +224,16 @@ public static User parse(XContentParser parser) throws IOException {
187224
break;
188225
}
189226
}
190-
return new User(name, backendRoles, roles, customAttNames, requestedTenant, requestedTenantAccess);
227+
228+
return new User(name, backendRoles, roles, customAttributes, requestedTenant, requestedTenantAccess);
191229
}
192230

193231
/**
194-
* User String format must be pipe separated as : user_name|backendrole1,backendrole2|roles1,role2
232+
* User String format must be pipe separated as : user_name|backendrole1,backendrole2|roles1,role2|tenant|tenantAccess|base64-encoded(serialized(custom atttributes))
195233
* @param userString
196234
* @return
197235
*/
236+
@SuppressWarnings("unchecked")
198237
public static User parse(final String userString) {
199238
if (Strings.isNullOrEmpty(userString)) {
200239
return null;
@@ -212,6 +251,7 @@ public static User parse(final String userString) {
212251
List<String> roles = new ArrayList<>();
213252
String requestedTenant = null;
214253
String requestedTenantAccess = null;
254+
Map<String, String> customAttributes = new HashMap<>();
215255

216256
if ((strs.length > 1) && !Strings.isNullOrEmpty(strs[1])) {
217257
backendRoles.addAll(Arrays.stream(strs[1].split(",")).map(Utils::unescapePipe).toList());
@@ -225,7 +265,11 @@ public static User parse(final String userString) {
225265
if ((strs.length > 4) && !Strings.isNullOrEmpty(strs[4])) {
226266
requestedTenantAccess = strs[4].trim();
227267
}
228-
return new User(userName, backendRoles, roles, Arrays.asList(), requestedTenant, requestedTenantAccess);
268+
if ((strs.length > 5) && !Strings.isNullOrEmpty(strs[5])) {
269+
customAttributes = (Map<String, String>) Base64Helper.deserializeObject(strs[5]);
270+
}
271+
272+
return new User(userName, backendRoles, roles, customAttributes, requestedTenant, requestedTenantAccess);
229273
}
230274

231275
@Override
@@ -235,9 +279,15 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
235279
.field(NAME_FIELD, name)
236280
.field(BACKEND_ROLES_FIELD, backendRoles)
237281
.field(ROLES_FIELD, roles)
238-
.field(CUSTOM_ATTRIBUTE_NAMES_FIELD, customAttNames)
239282
.field(REQUESTED_TENANT_FIELD, requestedTenant)
240283
.field(REQUESTED_TENANT_ACCESS, requestedTenantAccess);
284+
285+
if (customAttributes.size() > 0) {
286+
builder.field(CUSTOM_ATTRIBUTES_FIELD, customAttributes);
287+
} else {
288+
builder.field(CUSTOM_ATTRIBUTE_NAMES_FIELD, new ArrayList<>());
289+
}
290+
241291
return builder.endObject();
242292
}
243293

@@ -246,7 +296,12 @@ public void writeTo(StreamOutput out) throws IOException {
246296
out.writeString(name);
247297
out.writeStringCollection(backendRoles);
248298
out.writeStringCollection(roles);
249-
out.writeStringCollection(customAttNames);
299+
if (out.getVersion().onOrAfter(Version.V_3_2_0)) {
300+
out.writeMap(customAttributes, StreamOutput::writeString, StreamOutput::writeString);
301+
} else {
302+
List<String> customAttributeNames = new ArrayList<>(customAttributes.keySet());
303+
out.writeStringCollection(customAttributeNames);
304+
}
250305
out.writeOptionalString(requestedTenant);
251306
if (out.getVersion().onOrAfter(Version.V_3_2_0)) {
252307
out.writeOptionalString(requestedTenantAccess);
@@ -259,7 +314,9 @@ public String toString() {
259314
builder.add(NAME_FIELD, name);
260315
builder.add(BACKEND_ROLES_FIELD, backendRoles);
261316
builder.add(ROLES_FIELD, roles);
262-
builder.add(CUSTOM_ATTRIBUTE_NAMES_FIELD, customAttNames);
317+
TreeMap<String, String> sortedCustomAttributes = new TreeMap<>();
318+
sortedCustomAttributes.putAll(customAttributes);
319+
builder.add(CUSTOM_ATTRIBUTES_FIELD, sortedCustomAttributes);
263320
builder.add(REQUESTED_TENANT_FIELD, requestedTenant);
264321
builder.add(REQUESTED_TENANT_ACCESS, requestedTenantAccess);
265322
return builder.toString();
@@ -274,7 +331,7 @@ public boolean equals(Object obj) {
274331
return this.name.equals(that.name)
275332
&& this.getBackendRoles().equals(that.backendRoles)
276333
&& this.getRoles().equals(that.roles)
277-
&& this.getCustomAttNames().equals(that.customAttNames)
334+
&& this.getCustomAttributes().equals(that.customAttributes)
278335
&& (Objects.equals(this.requestedTenant, that.requestedTenant))
279336
&& (Objects.equals(this.requestedTenantAccess, that.requestedTenantAccess));
280337
}
@@ -291,8 +348,12 @@ public List<String> getRoles() {
291348
return roles;
292349
}
293350

351+
public Map<String, String> getCustomAttributes() {
352+
return customAttributes;
353+
}
354+
294355
public List<String> getCustomAttNames() {
295-
return customAttNames;
356+
return this.getCustomAttributeNamesFromMap(this.customAttributes);
296357
}
297358

298359
@Nullable
@@ -312,4 +373,13 @@ public boolean isAdminDn(Settings settings) {
312373
List<String> adminDns = settings.getAsList(ConfigConstants.OPENSEARCH_SECURITY_AUTHCZ_ADMIN_DN, Collections.emptyList());
313374
return adminDns.contains(this.name);
314375
}
376+
377+
private Map<String, String> convertCustomAttributeNamesToMap(List<String> customAttNames) {
378+
return customAttNames.stream().collect(Collectors.toMap(key -> key, key -> "null"));
379+
}
380+
381+
private List<String> getCustomAttributeNamesFromMap(Map<String, String> customAttributes) {
382+
List<String> customAttNames = new ArrayList<>(this.customAttributes.keySet());
383+
return customAttNames;
384+
}
315385
}

0 commit comments

Comments
 (0)