address PR coments and fix a bug in RolesApiTest unit tests

opensearch-project · May 6, 2021 · 7f3178f · 7f3178f
1 parent 0953fff
commit 7f3178f
Show file tree

Hide file tree

Showing 10 changed files with 98 additions and 25 deletions.
diff --git a/...ndistroforelasticsearch/security/dlic/rest/validation/AbstractConfigurationValidator.java b/...ndistroforelasticsearch/security/dlic/rest/validation/AbstractConfigurationValidator.java
@@ -23,6 +23,7 @@
 import java.util.Map.Entry;
 import java.util.Set;
 import java.util.Arrays;
+import java.io.IOException;
 
 import com.amazon.opendistroforelasticsearch.security.support.ConfigConstants;
 import com.amazon.opendistroforelasticsearch.security.DefaultObjectMapper;
@@ -168,25 +169,6 @@ public boolean validate() {
         mandatory.removeAll(requested);
         missingMandatoryKeys.addAll(mandatory);
 
-        //null element in the values of all the possible keys with DataType as ARRAY
-        final Map<String, Object> contentAsMap = XContentHelper.convertToMap(this.content, false, XContentType.JSON).v2();
-        for (String allowedKey: allowedKeys.keySet()) {
-            DataType dataType = allowedKeys.get(allowedKey);
-            if(dataType == DataType.ARRAY) {
-                Object valuesObject = contentAsMap.get(allowedKey);
-                try {
-                    if (Arrays.asList(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;
-                    return false;
-
-                }
-            }
-        }
         // invalid settings
         Set<String> allowed = new HashSet<>(allowedKeys.keySet());
         requested.removeAll(allowed);
@@ -208,6 +190,31 @@ public boolean validate() {
             return false;
         }
 
+        //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;
+                        return false;
+                    }
+                }
+            }
+        } catch(IOException e) {
+            log.error(ErrorType.BODY_NOT_PARSEABLE.toString(), e);
+            this.errorType = ErrorType.BODY_NOT_PARSEABLE;
+            return false;
+        }
+
         return valid;
     }
 

diff --git a/...ava/com/amazon/opendistroforelasticsearch/security/dlic/rest/api/ActionGroupsApiTest.java b/...ava/com/amazon/opendistroforelasticsearch/security/dlic/rest/api/ActionGroupsApiTest.java
@@ -359,4 +359,15 @@ public void testActionGroupsApiForNonSuperAdmin() throws Exception {
 
     }
 
+    @Test
+    public void checkNullElementsInArray() throws Exception{
+        setup();
+        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]);
+        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
@@ -192,4 +192,15 @@ public void testNodesDnApiAuditComplianceLogging() throws Exception {
 
         assertThat(actualCategoryCounts, equalTo(expectedCategoryCounts));
     }
+
+    @Test
+    public void checkNullElementsInArray() throws Exception{
+        setup();
+        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]);
+        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
@@ -469,8 +469,9 @@ public void testRolesApiForNonSuperAdmin() throws Exception {
         Assert.assertEquals(HttpStatus.SC_FORBIDDEN, response.getStatusCode());
 
         // Put read only roles
-        response = rh.executePutRequest("/_opendistro/_security/api/roles/opendistro_security_transport_client", "[{ \"op\": \"replace\", \"path\": \"/description\", \"value\": \"foo\" }]", new Header[0]);
-        Assert.assertEquals(HttpStatus.SC_FORBIDDEN, response.getStatusCode());
+        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());
 
         // 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]);
@@ -489,8 +490,9 @@ public void testRolesApiForNonSuperAdmin() throws Exception {
         Assert.assertEquals(HttpStatus.SC_NOT_FOUND, response.getStatusCode());
 
         // put hidden role
-        response = rh.executePutRequest("/_opendistro/_security/api/roles/opendistro_security_internal", "[{ \"op\": \"replace\", \"path\": \"/description\", \"value\": \"foo\" }]", new Header[0]);
-        Assert.assertEquals(HttpStatus.SC_NOT_FOUND, response.getStatusCode());
+        response = rh.executePutRequest("/_opendistro/_security/api/roles/opendistro_security_internal",
+                                        FileHelper.loadFile("restapi/roles_captains.json"), new Header[0]);
+        Assert.assertEquals(org.apache.http.HttpStatus.SC_NOT_FOUND, response.getStatusCode());
 
         // Patch single hidden roles
         response = rh.executePatchRequest("/_opendistro/_security/api/roles/opendistro_security_internal", "[{ \"op\": \"replace\", \"path\": \"/description\", \"value\": \"foo\" }]", new Header[0]);
@@ -502,4 +504,16 @@ public void testRolesApiForNonSuperAdmin() throws Exception {
 
     }
 
+    @Test
+    public void checkNullElementsInArray() throws Exception{
+        setup();
+        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());
+        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
@@ -401,7 +401,6 @@ public void checkNullElementsInArray() throws Exception{
 
 		HttpResponse response = rh.executePutRequest("/_opendistro/_security/api/rolesmapping/opendistro_security_role_starfleet_captains",
 													 FileHelper.loadFile("restapi/rolesmapping_null_array_element.json"), new Header[0]);
-		System.out.println(response.getBody());
-		Assert.assertTrue(response.getBody().contains("Invalid array element"));
+		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
@@ -593,4 +593,15 @@ public void testUserApiForNonSuperAdmin() throws Exception {
 
     }
 
+    @Test
+    public void checkNullElementsInArray() throws Exception{
+        setup();
+        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]);
+        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
@@ -0,0 +1,3 @@
+{
+ "allowed_actions": [null]
+}
diff --git a/src/test/resources/restapi/nodesdn_null_array_element.json b/src/test/resources/restapi/nodesdn_null_array_element.json
@@ -0,0 +1,3 @@
+{
+  "nodes_dn" : [null]
+}
diff --git a/src/test/resources/restapi/roles_null_array_element.json b/src/test/resources/restapi/roles_null_array_element.json
@@ -0,0 +1,9 @@
+{
+  "cluster_permissions" : [null],
+  "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" : [ ]
+}
diff --git a/src/test/resources/restapi/users_null_array_element.json b/src/test/resources/restapi/users_null_array_element.json
@@ -0,0 +1,5 @@
+{
+  "backend_roles": [
+    null
+  ]
+}