From 7f3178f13f6337ca6ae1aaa14985e764e97ee99e Mon Sep 17 00:00:00 2001 From: Azar Fazel Date: Thu, 6 May 2021 13:56:12 -0700 Subject: [PATCH] address PR coments and fix a bug in RolesApiTest unit tests --- .../AbstractConfigurationValidator.java | 45 +++++++++++-------- .../dlic/rest/api/ActionGroupsApiTest.java | 11 +++++ .../dlic/rest/api/NodesDnApiTest.java | 11 +++++ .../security/dlic/rest/api/RolesApiTest.java | 22 +++++++-- .../dlic/rest/api/RolesMappingApiTest.java | 3 +- .../security/dlic/rest/api/UserApiTest.java | 11 +++++ .../actiongroup_null_array_element.json | 3 ++ .../restapi/nodesdn_null_array_element.json | 3 ++ .../restapi/roles_null_array_element.json | 9 ++++ .../restapi/users_null_array_element.json | 5 +++ 10 files changed, 98 insertions(+), 25 deletions(-) create mode 100644 src/test/resources/restapi/actiongroup_null_array_element.json create mode 100644 src/test/resources/restapi/nodesdn_null_array_element.json create mode 100644 src/test/resources/restapi/roles_null_array_element.json create mode 100644 src/test/resources/restapi/users_null_array_element.json diff --git a/src/main/java/com/amazon/opendistroforelasticsearch/security/dlic/rest/validation/AbstractConfigurationValidator.java b/src/main/java/com/amazon/opendistroforelasticsearch/security/dlic/rest/validation/AbstractConfigurationValidator.java index 9821d5d467..4a19541344 100644 --- a/src/main/java/com/amazon/opendistroforelasticsearch/security/dlic/rest/validation/AbstractConfigurationValidator.java +++ b/src/main/java/com/amazon/opendistroforelasticsearch/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 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 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 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/src/test/java/com/amazon/opendistroforelasticsearch/security/dlic/rest/api/ActionGroupsApiTest.java b/src/test/java/com/amazon/opendistroforelasticsearch/security/dlic/rest/api/ActionGroupsApiTest.java index 308a0df58e..4897068365 100644 --- a/src/test/java/com/amazon/opendistroforelasticsearch/security/dlic/rest/api/ActionGroupsApiTest.java +++ b/src/test/java/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/src/test/java/com/amazon/opendistroforelasticsearch/security/dlic/rest/api/NodesDnApiTest.java b/src/test/java/com/amazon/opendistroforelasticsearch/security/dlic/rest/api/NodesDnApiTest.java index a9723cdd9f..9749a77fd4 100644 --- a/src/test/java/com/amazon/opendistroforelasticsearch/security/dlic/rest/api/NodesDnApiTest.java +++ b/src/test/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 index 6c00dfade4..e78f1fe97b 100644 --- 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/src/test/java/com/amazon/opendistroforelasticsearch/security/dlic/rest/api/RolesMappingApiTest.java b/src/test/java/com/amazon/opendistroforelasticsearch/security/dlic/rest/api/RolesMappingApiTest.java index 40b32d634f..3da9336af4 100644 --- a/src/test/java/com/amazon/opendistroforelasticsearch/security/dlic/rest/api/RolesMappingApiTest.java +++ b/src/test/java/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 index 584cf5df59..03cae7112a 100644 --- 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 new file mode 100644 index 0000000000..8b24ce1530 --- /dev/null +++ b/src/test/resources/restapi/actiongroup_null_array_element.json @@ -0,0 +1,3 @@ +{ + "allowed_actions": [null] +} \ No newline at end of file diff --git a/src/test/resources/restapi/nodesdn_null_array_element.json b/src/test/resources/restapi/nodesdn_null_array_element.json new file mode 100644 index 0000000000..91bb55827d --- /dev/null +++ b/src/test/resources/restapi/nodesdn_null_array_element.json @@ -0,0 +1,3 @@ +{ + "nodes_dn" : [null] +} \ No newline at end of file diff --git a/src/test/resources/restapi/roles_null_array_element.json b/src/test/resources/restapi/roles_null_array_element.json new file mode 100644 index 0000000000..beb9ceb48f --- /dev/null +++ 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" : [ ] +} \ No newline at end of file diff --git a/src/test/resources/restapi/users_null_array_element.json b/src/test/resources/restapi/users_null_array_element.json new file mode 100644 index 0000000000..f310394b8f --- /dev/null +++ b/src/test/resources/restapi/users_null_array_element.json @@ -0,0 +1,5 @@ +{ + "backend_roles": [ + null + ] +} \ No newline at end of file