Address more PR comments

opensearch-project · May 11, 2021 · a5c63b3 · a5c63b3
1 parent 7f3178f
commit a5c63b3
Show file tree

Hide file tree

Showing 18 changed files with 154 additions and 41 deletions.
diff --git a/...ndistroforelasticsearch/security/dlic/rest/validation/AbstractConfigurationValidator.java b/...ndistroforelasticsearch/security/dlic/rest/validation/AbstractConfigurationValidator.java
@@ -22,8 +22,9 @@
 import java.util.Map;
 import java.util.Map.Entry;
 import java.util.Set;
-import java.util.Arrays;
-import java.io.IOException;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.HashMap;
 
 import com.amazon.opendistroforelasticsearch.security.support.ConfigConstants;
 import com.amazon.opendistroforelasticsearch.security.DefaultObjectMapper;
@@ -191,30 +192,41 @@ public boolean validate() {
         }
 
         //null element in the values of all the possible keys with DataType as ARRAY
-        try {
-            HashMap<String,Object> contentAsMap = DefaultObjectMapper.readValue(content.utf8ToString(), HashMap.class);
-            for (String allowedKey : allowedKeys.keySet()) {
-                DataType dataType = allowedKeys.get(allowedKey);
-                if (dataType == DataType.ARRAY && contentAsMap.containsKey(allowedKey)) {
-                    Object valuesObject = contentAsMap.get(allowedKey);
-                    try {
-                        if (((java.util.List) valuesObject).contains(null)) {
-                            this.errorType = ErrorType.INVALID_ARRAY_ELEMENT;
-                            return false;
-                        }
-                    } catch (Exception e) {
-                        log.error(ErrorType.BODY_NOT_PARSEABLE.toString(), e);
-                        this.errorType = ErrorType.BODY_NOT_PARSEABLE;
+        for (Entry<String, DataType> allowedKey : allowedKeys.entrySet()) {
+            DataType dataType = allowedKey.getValue();
+            JsonNode value = contentAsNode.get(allowedKey.getKey());
+            if (dataType == DataType.ARRAY && value != null) {
+                try {
+                    List contentArray = DefaultObjectMapper.objectMapper.convertValue(value, java.util.List.class);
+                    if (contentArray.contains(null)) {
+                        this.errorType = ErrorType.NULL_ARRAY_ELEMENT;
                         return false;
                     }
+                    else {
+                        for (int i = 0; i < contentArray.size(); i++) {
+                            if (contentArray.get(i) instanceof HashMap) {
+                                for (Object valueList : ((HashMap) contentArray.get(i)).values()) {
+                                    if (((List) valueList).contains(null)) {
+                                        this.errorType = ErrorType.NULL_ARRAY_ELEMENT;
+                                        return false;
+                                    }
+                                }
+                            }
+                            else if(contentArray.get(i) instanceof ArrayList){
+                                if(((ArrayList) contentArray.get(i)).contains(null)){
+                                    this.errorType = ErrorType.NULL_ARRAY_ELEMENT;
+                                    return false;
+                                }
+                            }
+                        }
+                    }
+                } catch (Exception e) {
+                    log.error(ErrorType.BODY_NOT_PARSEABLE.toString(), e);
+                    this.errorType = ErrorType.BODY_NOT_PARSEABLE;
+                    return false;
                 }
             }
-        } catch(IOException e) {
-            log.error(ErrorType.BODY_NOT_PARSEABLE.toString(), e);
-            this.errorType = ErrorType.BODY_NOT_PARSEABLE;
-            return false;
         }
-
         return valid;
     }
 
@@ -280,9 +292,9 @@ public XContentBuilder errorsAsXContent(RestChannel channel) {
                         builder.field(entry.getKey(), entry.getValue());
                     }
                     break;
-                case INVALID_ARRAY_ELEMENT:
+                case NULL_ARRAY_ELEMENT:
                     builder.field("status", "error");
-                    builder.field("reason", ErrorType.INVALID_ARRAY_ELEMENT.getMessage());
+                    builder.field("reason", ErrorType.NULL_ARRAY_ELEMENT.getMessage());
                     break;
                 default:
                     builder.field("status", "error");
@@ -313,7 +325,7 @@ public static enum ErrorType {
         NONE("ok"), INVALID_CONFIGURATION("Invalid configuration"), INVALID_PASSWORD("Invalid password"), WRONG_DATATYPE("Wrong datatype"),
         BODY_NOT_PARSEABLE("Could not parse content of request."), PAYLOAD_NOT_ALLOWED("Request body not allowed for this action."),
         PAYLOAD_MANDATORY("Request body required for this action."), SECURITY_NOT_INITIALIZED("Security index not initialized"),
-        INVALID_ARRAY_ELEMENT("Invalid array element");
+        NULL_ARRAY_ELEMENT("`null` is not allowed as json array element");
 
         private String message;
 

diff --git a/...ava/com/amazon/opendistroforelasticsearch/security/dlic/rest/api/ActionGroupsApiTest.java b/...ava/com/amazon/opendistroforelasticsearch/security/dlic/rest/api/ActionGroupsApiTest.java
@@ -365,8 +365,8 @@ public void checkNullElementsInArray() throws Exception{
         rh.keystore = "restapi/kirk-keystore.jks";
         rh.sendAdminCertificate = true;
 
-        HttpResponse response = rh.executePutRequest("/_opendistro/_security/api/actiongroups/CRUD_UT",
-                                                     FileHelper.loadFile("restapi/actiongroup_null_array_element.json"), new Header[0]);
+        String body = FileHelper.loadFile("restapi/actiongroup_null_array_element.json");
+        HttpResponse response = rh.executePutRequest("/_opendistro/_security/api/actiongroups/CRUD_UT", body, new Header[0]);
         Assert.assertEquals(HttpStatus.SC_BAD_REQUEST, response.getStatusCode());
     }
 

diff --git a/...est/java/com/amazon/opendistroforelasticsearch/security/dlic/rest/api/NodesDnApiTest.java b/...est/java/com/amazon/opendistroforelasticsearch/security/dlic/rest/api/NodesDnApiTest.java
@@ -20,6 +20,7 @@
 import com.amazon.opendistroforelasticsearch.security.auditlog.integration.TestAuditlogImpl;
 import com.amazon.opendistroforelasticsearch.security.support.ConfigConstants;
 import com.amazon.opendistroforelasticsearch.security.test.helper.rest.RestHelper.HttpResponse;
+import com.amazon.opendistroforelasticsearch.security.test.helper.file.FileHelper;
 import com.fasterxml.jackson.databind.JsonNode;
 import com.fasterxml.jackson.databind.ObjectMapper;
 import com.google.common.collect.ImmutableMap;
@@ -199,8 +200,8 @@ public void checkNullElementsInArray() throws Exception{
         rh.keystore = "restapi/kirk-keystore.jks";
         rh.sendAdminCertificate = true;
 
-        HttpResponse response = rh.executePutRequest("/_opendistro/_security/api/nodesdn",
-                                                     com.amazon.opendistroforelasticsearch.security.test.helper.file.FileHelper.loadFile("restapi/nodesdn_null_array_element.json"), new Header[0]);
+        String body = FileHelper.loadFile("restapi/nodesdn_null_array_element.json");
+        HttpResponse response = rh.executePutRequest("/_opendistro/_security/api/nodesdn", body, new Header[0]);
         Assert.assertEquals(HttpStatus.SC_BAD_REQUEST, response.getStatusCode());
     }
 }
diff --git a/src/test/java/com/amazon/opendistroforelasticsearch/security/dlic/rest/api/RolesApiTest.java b/src/test/java/com/amazon/opendistroforelasticsearch/security/dlic/rest/api/RolesApiTest.java
@@ -471,7 +471,7 @@ public void testRolesApiForNonSuperAdmin() throws Exception {
         // Put read only roles
         response = rh.executePutRequest("/_opendistro/_security/api/roles/opendistro_security_transport_client",
                                         FileHelper.loadFile("restapi/roles_captains.json"), new Header[0]);
-        Assert.assertEquals(org.apache.http.HttpStatus.SC_FORBIDDEN, response.getStatusCode());
+        Assert.assertEquals(HttpStatus.SC_FORBIDDEN, response.getStatusCode());
 
         // Patch single read only roles
         response = rh.executePatchRequest("/_opendistro/_security/api/roles/opendistro_security_transport_client", "[{ \"op\": \"replace\", \"path\": \"/description\", \"value\": \"foo\" }]", new Header[0]);
@@ -510,9 +510,28 @@ public void checkNullElementsInArray() throws Exception{
         rh.keystore = "restapi/kirk-keystore.jks";
         rh.sendAdminCertificate = true;
 
-        HttpResponse response = rh.executePutRequest("/_opendistro/_security/api/roles/opendistro_security_role_starfleet",
-                                                     FileHelper.loadFile("restapi/roles_null_array_element.json"), new Header[0]);
-        System.out.println(response.getBody());
+        String body = FileHelper.loadFile("restapi/roles_null_array_element_cluster_permissions.json");
+        HttpResponse response = rh.executePutRequest("/_opendistro/_security/api/roles/opendistro_security_role_starfleet", body, new Header[0]);
+        Assert.assertEquals(HttpStatus.SC_BAD_REQUEST, response.getStatusCode());
+
+        body = FileHelper.loadFile("restapi/roles_null_array_element_index_permissions.json");
+        response = rh.executePutRequest("/_opendistro/_security/api/roles/opendistro_security_role_starfleet", body, new Header[0]);
+        Assert.assertEquals(HttpStatus.SC_BAD_REQUEST, response.getStatusCode());
+
+        body = FileHelper.loadFile("restapi/roles_null_array_element_tenant_permissions.json");
+        response = rh.executePutRequest("/_opendistro/_security/api/roles/opendistro_security_role_starfleet", body, new Header[0]);
+        Assert.assertEquals(HttpStatus.SC_BAD_REQUEST, response.getStatusCode());
+
+        body = FileHelper.loadFile("restapi/roles_null_array_element_index_patterns.json");
+        response = rh.executePutRequest("/_opendistro/_security/api/roles/opendistro_security_role_starfleet", body, new Header[0]);
+        Assert.assertEquals(HttpStatus.SC_BAD_REQUEST, response.getStatusCode());
+
+        body = FileHelper.loadFile("restapi/roles_null_array_element_masked_fields.json");
+        response = rh.executePutRequest("/_opendistro/_security/api/roles/opendistro_security_role_starfleet", body, new Header[0]);
+        Assert.assertEquals(HttpStatus.SC_BAD_REQUEST, response.getStatusCode());
+
+        body = FileHelper.loadFile("restapi/roles_null_array_element_allowed_actions.json");
+        response = rh.executePutRequest("/_opendistro/_security/api/roles/opendistro_security_role_starfleet", body, new Header[0]);
         Assert.assertEquals(HttpStatus.SC_BAD_REQUEST, response.getStatusCode());
     }
 

diff --git a/...ava/com/amazon/opendistroforelasticsearch/security/dlic/rest/api/RolesMappingApiTest.java b/...ava/com/amazon/opendistroforelasticsearch/security/dlic/rest/api/RolesMappingApiTest.java
@@ -399,8 +399,19 @@ public void checkNullElementsInArray() throws Exception{
 		rh.keystore = "restapi/kirk-keystore.jks";
 		rh.sendAdminCertificate = true;
 
+		String body = FileHelper.loadFile("restapi/rolesmapping_null_array_element_users.json");
 		HttpResponse response = rh.executePutRequest("/_opendistro/_security/api/rolesmapping/opendistro_security_role_starfleet_captains",
-													 FileHelper.loadFile("restapi/rolesmapping_null_array_element.json"), new Header[0]);
+													 body, new Header[0]);
+		Assert.assertEquals(HttpStatus.SC_BAD_REQUEST, response.getStatusCode());
+
+		body = FileHelper.loadFile("restapi/rolesmapping_null_array_element_backend_roles.json");
+		response = rh.executePutRequest("/_opendistro/_security/api/rolesmapping/opendistro_security_role_starfleet_captains",
+													 body, new Header[0]);
+		Assert.assertEquals(HttpStatus.SC_BAD_REQUEST, response.getStatusCode());
+
+		body = FileHelper.loadFile("restapi/rolesmapping_null_array_element_hosts.json");
+		response = rh.executePutRequest("/_opendistro/_security/api/rolesmapping/opendistro_security_role_starfleet_captains",
+										 body, new Header[0]);
 		Assert.assertEquals(HttpStatus.SC_BAD_REQUEST, response.getStatusCode());
 	}
 }
diff --git a/src/test/java/com/amazon/opendistroforelasticsearch/security/dlic/rest/api/UserApiTest.java b/src/test/java/com/amazon/opendistroforelasticsearch/security/dlic/rest/api/UserApiTest.java
@@ -599,8 +599,8 @@ public void checkNullElementsInArray() throws Exception{
         rh.keystore = "restapi/kirk-keystore.jks";
         rh.sendAdminCertificate = true;
 
-        HttpResponse response = rh.executePutRequest("/_opendistro/_security/api/internalusers/picard",
-                                                     FileHelper.loadFile("restapi/users_null_array_element.json"), new Header[0]);
+        String body = FileHelper.loadFile("restapi/users_null_array_element.json");
+        HttpResponse response = rh.executePutRequest("/_opendistro/_security/api/internalusers/picard", body, new Header[0]);
         Assert.assertEquals(HttpStatus.SC_BAD_REQUEST, response.getStatusCode());
     }
 

diff --git a/src/test/resources/restapi/actiongroup_null_array_element.json b/src/test/resources/restapi/actiongroup_null_array_element.json
@@ -1,3 +1,4 @@
 {
- "allowed_actions": [null]
-}
+ "allowed_actions": [null, "OPENDISTRO_SECURITY_WRITE"]
+}
+
diff --git a/src/test/resources/restapi/nodesdn_null_array_element.json b/src/test/resources/restapi/nodesdn_null_array_element.json
@@ -1,3 +1,3 @@
 {
   "nodes_dn" : [null]
-}
+}
diff --git a/src/test/resources/restapi/roles_null_array_element_allowed_actions.json b/src/test/resources/restapi/roles_null_array_element_allowed_actions.json
@@ -0,0 +1,9 @@
+{
+  "cluster_permissions" : ["cluster:monitor1*"],
+  "index_permissions" : [{
+    "index_patterns" : [ "*" ],
+    "masked_fields" : [ "abvfg", "*ip_dest*::/[0-9]{1,3}$/::XXX", "*ip_source*::/[0-9]{1,3}$/::XXX::/^[0-9]{1,3}/::***", "customer.name::SHA-512", "/[0-9]{1,3}/::SHA-512" ],
+    "allowed_actions" : [ "123", null ]
+  }],
+  "tenant_permissions" : [ ]
+}
diff --git a/...ces/restapi/roles_null_array_element.json → ...ll_array_element_cluster_permissions.json b/...ces/restapi/roles_null_array_element.json → ...ll_array_element_cluster_permissions.json
@@ -6,4 +6,4 @@
     "allowed_actions" : [ "123", "indices:data/read/*" ]
   } ],
   "tenant_permissions" : [ ]
-}
+}
diff --git a/src/test/resources/restapi/roles_null_array_element_index_patterns.json b/src/test/resources/restapi/roles_null_array_element_index_patterns.json
@@ -0,0 +1,9 @@
+{
+  "cluster_permissions" : ["cluster:monitor1*"],
+  "index_permissions" : [{
+    "index_patterns" : [null],
+    "masked_fields" : [ "abvfg", "*ip_dest*::/[0-9]{1,3}$/::XXX", "*ip_source*::/[0-9]{1,3}$/::XXX::/^[0-9]{1,3}/::***", "customer.name::SHA-512", "/[0-9]{1,3}/::SHA-512" ],
+    "allowed_actions" : [ "123", "indices:data/read/*" ]
+  }],
+  "tenant_permissions" : [ ]
+}
diff --git a/src/test/resources/restapi/roles_null_array_element_index_permissions.json b/src/test/resources/restapi/roles_null_array_element_index_permissions.json
@@ -0,0 +1,5 @@
+{
+  "cluster_permissions" : ["cluster:monitor1*"],
+  "index_permissions" : [null],
+  "tenant_permissions" : [ ]
+}
diff --git a/src/test/resources/restapi/roles_null_array_element_masked_fields.json b/src/test/resources/restapi/roles_null_array_element_masked_fields.json
@@ -0,0 +1,9 @@
+{
+  "cluster_permissions" : ["cluster:monitor1*"],
+  "index_permissions" : [{
+    "index_patterns" : [ "*" ],
+    "masked_fields" : [ "abvfg", null, "*ip_source*::/[0-9]{1,3}$/::XXX::/^[0-9]{1,3}/::***", "customer.name::SHA-512", "/[0-9]{1,3}/::SHA-512" ],
+    "allowed_actions" : [ "123", "indices:data/read/*" ]
+  }],
+  "tenant_permissions" : [ ]
+}
diff --git a/src/test/resources/restapi/roles_null_array_element_tenant_permissions.json b/src/test/resources/restapi/roles_null_array_element_tenant_permissions.json
@@ -0,0 +1,9 @@
+{
+  "cluster_permissions" : ["cluster:monitor1*"],
+  "index_permissions" : [{
+    "index_patterns" : [ "*" ],
+    "masked_fields" : [ "abvfg", "*ip_dest*::/[0-9]{1,3}$/::XXX", "*ip_source*::/[0-9]{1,3}$/::XXX::/^[0-9]{1,3}/::***", "customer.name::SHA-512", "/[0-9]{1,3}/::SHA-512" ],
+    "allowed_actions" : [ "123", "indices:data/read/*" ]
+  }],
+  "tenant_permissions" : [null]
+}
diff --git a/src/test/resources/restapi/rolesmapping_null_array_element_backend_roles.json b/src/test/resources/restapi/rolesmapping_null_array_element_backend_roles.json
@@ -0,0 +1,14 @@
+{
+  "users": [
+    "user1",
+    "user2"
+  ],
+  "backend_roles": [
+    "captains",
+    null
+  ],
+  "hosts": [
+    "8.8.8.8",
+    "8.8.4.4"
+  ]
+}
diff --git a/src/test/resources/restapi/rolesmapping_null_array_element_hosts.json b/src/test/resources/restapi/rolesmapping_null_array_element_hosts.json
@@ -0,0 +1,14 @@
+{
+  "users": [
+    "user1",
+    "user2"
+  ],
+  "backend_roles": [
+    "captains",
+    "role2"
+  ],
+  "hosts": [
+    null,
+    "8.8.4.4"
+  ]
+}
diff --git a/...tapi/rolesmapping_null_array_element.json → ...olesmapping_null_array_element_users.json b/...tapi/rolesmapping_null_array_element.json → ...olesmapping_null_array_element_users.json
@@ -8,4 +8,4 @@
   "hosts": [
     "8.8.8.8"
   ]
-}
+}
diff --git a/src/test/resources/restapi/users_null_array_element.json b/src/test/resources/restapi/users_null_array_element.json
@@ -2,4 +2,4 @@
   "backend_roles": [
     null
   ]
-}
+}
-Original file line number
+Diff line change
@@ Expand Up / @@ -6,4 +6,4 @@ @@
         "allowed_actions" : [ "123", "indices:data/read/*" ]
       } ],
       "tenant_permissions" : [ ]
-    }
+    }