diff --git a/src/main/java/org/opensearch/commons/authuser/User.java b/src/main/java/org/opensearch/commons/authuser/User.java index 0859f415..9d6b5041 100644 --- a/src/main/java/org/opensearch/commons/authuser/User.java +++ b/src/main/java/org/opensearch/commons/authuser/User.java @@ -17,7 +17,6 @@ import java.util.Map; import java.util.Objects; import java.util.TreeMap; -import java.util.stream.Collectors; import org.apache.hc.core5.http.ParseException; import org.apache.hc.core5.http.io.entity.EntityUtils; @@ -49,7 +48,6 @@ final public class User implements Writeable, ToXContent { public static final String BACKEND_ROLES_FIELD = "backend_roles"; public static final String ROLES_FIELD = "roles"; public static final String CUSTOM_ATTRIBUTE_NAMES_FIELD = "custom_attribute_names"; - public static final String CUSTOM_ATTRIBUTES_FIELD = "custom_attributes"; public static final String REQUESTED_TENANT_FIELD = "user_requested_tenant"; public static final String REQUESTED_TENANT_ACCESS = "user_requested_tenant_access"; @@ -80,7 +78,8 @@ public User(final String name, final List backendRoles, List rol this.requestedTenantAccess = null; } - public User(final String name, final List backendRoles, List roles, List customAttNames) { + public User(final String name, final List backendRoles, List roles, List customAttNames) + throws IllegalArgumentException { this.name = name; this.backendRoles = backendRoles; this.roles = roles; @@ -157,12 +156,8 @@ public User(StreamInput in) throws IOException { name = in.readString(); backendRoles = in.readStringList(); roles = in.readStringList(); - if (in.getVersion().onOrAfter(Version.V_3_2_0)) { - customAttributes = in.readMap(StreamInput::readString, StreamInput::readString); - } else { - List customAttNames = in.readStringList(); - customAttributes = this.convertCustomAttributeNamesToMap(customAttNames); - } + List customAttNames = in.readStringList(); + customAttributes = this.convertCustomAttributeNamesToMap(customAttNames); requestedTenant = in.readOptionalString(); if (in.getVersion().onOrAfter(Version.V_3_2_0)) { requestedTenantAccess = in.readOptionalString(); @@ -171,6 +166,19 @@ public User(StreamInput in) throws IOException { } } + /** + * Parse the user as XContent into a User + * + * For CUSTOM_ATTRIBUTE_NAMES_FIELD, the expected format is: + * + * ["attr=val"] + * + * where attr is the attribute name and val is the attribute value. If the format + * of the entries does not match "attr=val", an exception will be thrown. + * + * @param XContentParser parser + * @throws IOException + */ public static User parse(XContentParser parser) throws IOException { String name = ""; List backendRoles = new ArrayList<>(); @@ -199,19 +207,11 @@ public static User parse(XContentParser parser) throws IOException { roles.add(parser.text()); } break; - case CUSTOM_ATTRIBUTES_FIELD: - ensureExpectedToken(XContentParser.Token.START_OBJECT, parser.currentToken(), parser); - while (parser.nextToken() != XContentParser.Token.END_OBJECT) { - String attrName = parser.currentName(); - parser.nextToken(); - String attrValue = parser.text(); - customAttributes.put(attrName, attrValue); - } - break; case CUSTOM_ATTRIBUTE_NAMES_FIELD: ensureExpectedToken(XContentParser.Token.START_ARRAY, parser.currentToken(), parser); while (parser.nextToken() != XContentParser.Token.END_ARRAY) { - customAttributes.put(parser.text(), null); + Map attributeInfo = User.parseAttributeInfoFromCustomAttributeName(parser.text()); + customAttributes.put(attributeInfo.get("key"), attributeInfo.get("value")); } break; case REQUESTED_TENANT_FIELD: @@ -283,7 +283,7 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws .field(REQUESTED_TENANT_ACCESS, requestedTenantAccess); if (customAttributes.size() > 0) { - builder.field(CUSTOM_ATTRIBUTES_FIELD, customAttributes); + builder.field(CUSTOM_ATTRIBUTE_NAMES_FIELD, this.getCustomAttributeNamesFromMap(customAttributes)); } else { builder.field(CUSTOM_ATTRIBUTE_NAMES_FIELD, new ArrayList<>()); } @@ -296,12 +296,7 @@ public void writeTo(StreamOutput out) throws IOException { out.writeString(name); out.writeStringCollection(backendRoles); out.writeStringCollection(roles); - if (out.getVersion().onOrAfter(Version.V_3_2_0)) { - out.writeMap(customAttributes, StreamOutput::writeString, StreamOutput::writeString); - } else { - List customAttributeNames = new ArrayList<>(customAttributes.keySet()); - out.writeStringCollection(customAttributeNames); - } + out.writeStringCollection(this.getCustomAttributeNamesFromMap(customAttributes)); out.writeOptionalString(requestedTenant); if (out.getVersion().onOrAfter(Version.V_3_2_0)) { out.writeOptionalString(requestedTenantAccess); @@ -314,9 +309,7 @@ public String toString() { builder.add(NAME_FIELD, name); builder.add(BACKEND_ROLES_FIELD, backendRoles); builder.add(ROLES_FIELD, roles); - TreeMap sortedCustomAttributes = new TreeMap<>(); - sortedCustomAttributes.putAll(customAttributes); - builder.add(CUSTOM_ATTRIBUTES_FIELD, sortedCustomAttributes); + builder.add(CUSTOM_ATTRIBUTE_NAMES_FIELD, this.getCustomAttributeNamesFromMap(customAttributes)); builder.add(REQUESTED_TENANT_FIELD, requestedTenant); builder.add(REQUESTED_TENANT_ACCESS, requestedTenantAccess); return builder.toString(); @@ -375,11 +368,31 @@ public boolean isAdminDn(Settings settings) { } private Map convertCustomAttributeNamesToMap(List customAttNames) { - return customAttNames.stream().collect(Collectors.toMap(key -> key, key -> "null")); + Map customAttributes = new TreeMap<>(); + for (String entry : customAttNames) { + Map attributeInfo = User.parseAttributeInfoFromCustomAttributeName(entry); + customAttributes.put(attributeInfo.get("key"), attributeInfo.get("value")); + } + return customAttributes; + } + + private static Map parseAttributeInfoFromCustomAttributeName(String customAttributeName) { + // Find first index in string of "=" + int idx = customAttributeName.indexOf("="); + if (idx == -1) { + throw new IllegalArgumentException("No '=' present: " + customAttributeName); + } + String attrKey = customAttributeName.substring(0, idx); + String attrValue = customAttributeName.substring(idx + 1); + return Map.of("key", attrKey, "value", attrValue); } private List getCustomAttributeNamesFromMap(Map customAttributes) { - List customAttNames = new ArrayList<>(this.customAttributes.keySet()); + List customAttNames = new ArrayList<>(); + for (Map.Entry entry : this.customAttributes.entrySet()) { + customAttNames.add(entry.getKey() + "=" + entry.getValue()); + } + Collections.sort(customAttNames); return customAttNames; } } diff --git a/src/test/java/org/opensearch/commons/authuser/UserTest.java b/src/test/java/org/opensearch/commons/authuser/UserTest.java index 4db115c8..c4e1fd49 100644 --- a/src/test/java/org/opensearch/commons/authuser/UserTest.java +++ b/src/test/java/org/opensearch/commons/authuser/UserTest.java @@ -8,6 +8,7 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.opensearch.commons.ConfigConstants.OPENSEARCH_SECURITY_USER_INFO_THREAD_CONTEXT; @@ -164,7 +165,7 @@ public void testEmptyCustomAttributeNamesJsonConst() throws IOException { @Test public void testNonEmptyCustomAttributeNamesJsonConst() throws IOException { String json = - "{\"user\":\"User [name=chip, backend_roles=[admin], requestedTenant=__user__]\",\"user_name\":\"chip\",\"user_requested_tenant\":\"__user__\",\"remote_address\":\"127.0.0.1:52196\",\"backend_roles\":[\"admin\"],\"custom_attribute_names\":[\"attr1\"],\"roles\":[\"alerting_monitor_full\",\"ops_role\",\"own_index\"],\"tenants\":{\"chip\":true},\"principal\":null,\"peer_certificates\":\"0\",\"sso_logout_url\":null}"; + "{\"user\":\"User [name=chip, backend_roles=[admin], requestedTenant=__user__]\",\"user_name\":\"chip\",\"user_requested_tenant\":\"__user__\",\"remote_address\":\"127.0.0.1:52196\",\"backend_roles\":[\"admin\"],\"custom_attribute_names\":[\"attr1=val1\"],\"roles\":[\"alerting_monitor_full\",\"ops_role\",\"own_index\"],\"tenants\":{\"chip\":true},\"principal\":null,\"peer_certificates\":\"0\",\"sso_logout_url\":null}"; User user = new User(json); assertEquals("chip", user.getName()); @@ -172,10 +173,41 @@ public void testNonEmptyCustomAttributeNamesJsonConst() throws IOException { assertEquals(3, user.getRoles().size()); assertEquals(1, user.getCustomAttributes().size()); assertTrue(user.getCustomAttributes().containsKey("attr1")); - assertTrue(user.getCustomAttributes().containsValue("null")); + assertTrue(user.getCustomAttributes().containsValue("val1")); assertEquals("__user__", user.getRequestedTenant()); } + @Test + public void testNonEmptyCustomAttributeNamesJsonConstThrows() throws IOException { + String json = """ + { + "user": "User [name=chip, backend_roles=[admin], requestedTenant=__user__]", + "user_name": "chip", + "user_requested_tenant": "__user__", + "remote_address": "127.0.0.1:52196", + "backend_roles": [ + "admin" + ], + "custom_attribute_names": [ + "attr1" + ], + "roles": [ + "alerting_monitor_full", + "ops_role", + "own_index" + ], + "tenants": { + "chip": true + }, + "principal": null, + "peer_certificates": "0", + "sso_logout_url": null + } + """; + + assertThrows(IllegalArgumentException.class, () -> new User(json)); + } + @Test public void testStreamConstForNoTenantUser() throws IOException { User user = testNoTenantUser(); @@ -198,7 +230,7 @@ public void testStreamConstForUserBackwardsCompatibility() throws IOException { User newUser = new User(in); assertEquals(2, newUser.getCustomAttributes().size()); assertTrue(newUser.getCustomAttributes().containsKey("attr1")); - assertTrue(newUser.getCustomAttributes().containsValue("null")); + assertTrue(newUser.getCustomAttributes().containsValue("attrValue1")); } @Test @@ -451,9 +483,9 @@ public void testParseUserXContent() throws IOException { .value("role-2") .endArray() // End the array .field(User.REQUESTED_TENANT_FIELD, "tenant-1") - .startObject(User.CUSTOM_ATTRIBUTES_FIELD) // Start a nested object - .field("attr1", "val1") - .endObject() // End the nested object + .startArray(User.CUSTOM_ATTRIBUTE_NAMES_FIELD) // Start a nested object + .value("attr1=val1") + .endArray() // End the array .endObject(); // End the main object MediaType mediaType = MediaTypeRegistry.JSON; @@ -474,6 +506,37 @@ public void testParseUserXContent() throws IOException { assertEquals(1, user.getCustomAttributes().size()); assertTrue(user.getCustomAttributes().containsKey("attr1")); assertTrue(user.getCustomAttributes().containsValue("val1")); + assertEquals(user.getCustomAttNames(), Arrays.asList("attr1=val1")); + } + + @Test + public void testParseUserXContentMalformedCustomAttributeNamesThrows() throws IOException { + XContentBuilder builder = XContentFactory.jsonBuilder(); + + builder + .startObject() // Start a JSON object + .field(User.NAME_FIELD, "myuser") // Add a field + .startArray(User.BACKEND_ROLES_FIELD) // Start an array + .value("backend-role-1") + .value("backend-role-2") + .endArray() // End the array + .startArray(User.ROLES_FIELD) // Start an array + .value("role-1") + .value("role-2") + .endArray() // End the array + .field(User.REQUESTED_TENANT_FIELD, "tenant-1") + .startArray(User.CUSTOM_ATTRIBUTE_NAMES_FIELD) // Start a nested object + .value("attr1") + .endArray() // End the array + .endObject(); // End the main object + + MediaType mediaType = MediaTypeRegistry.JSON; + XContentParser parser = mediaType + .xContent() + .createParser(NamedXContentRegistry.EMPTY, DeprecationHandler.IGNORE_DEPRECATIONS, builder.toString()); + + parser.nextToken(); + assertThrows(IllegalArgumentException.class, () -> User.parse(parser)); } @Test @@ -519,14 +582,14 @@ public void testUserOrSettingsAreNullOrEmpty() { } @Test - public void testUserCustomAttributeNamesBackwardsCompatibility() { - User user = new User("chip", Arrays.asList("admin", "ops"), Arrays.asList("ops_data"), Arrays.asList("attr1")); + public void testUserCustomAttributeNamesBackwardsCompatibility() throws IllegalArgumentException { + User user = new User("chip", Arrays.asList("admin", "ops"), Arrays.asList("ops_data"), Arrays.asList("attr1=val1")); assertFalse(Strings.isNullOrEmpty(user.getName())); assertEquals(2, user.getBackendRoles().size()); assertEquals(1, user.getRoles().size()); assertEquals(1, user.getCustomAttributes().size()); assertTrue(user.getCustomAttributes().containsKey("attr1")); - assertTrue(user.getCustomAttributes().containsValue("null")); + assertTrue(user.getCustomAttributes().containsValue("val1")); assertNull(user.getRequestedTenant()); } @@ -541,9 +604,9 @@ public void testUserXContentIncludesCustomAttributes() throws IOException { "roles": ["ops_data"], "user_requested_tenant": null, "user_requested_tenant_access": null, - "custom_attributes": { - "attr1": "value1" - } + "custom_attribute_names": [ + "attr1=value1" + ] } """; assertEquals(expectedUserJson.replace("\n", "").replace("\s", ""), xcontent.toString().replace("\n", ""));