diff --git a/src/integrationTest/java/org/opensearch/security/api/AbstractApiIntegrationTest.java b/src/integrationTest/java/org/opensearch/security/api/AbstractApiIntegrationTest.java index eb93cd8768..bdba22e103 100644 --- a/src/integrationTest/java/org/opensearch/security/api/AbstractApiIntegrationTest.java +++ b/src/integrationTest/java/org/opensearch/security/api/AbstractApiIntegrationTest.java @@ -44,6 +44,7 @@ import org.opensearch.test.framework.cluster.LocalCluster; import org.opensearch.test.framework.cluster.TestRestClient; +import static org.hamcrest.CoreMatchers.containsString; import static org.hamcrest.CoreMatchers.is; import static org.hamcrest.CoreMatchers.not; import static org.hamcrest.MatcherAssert.assertThat; @@ -360,6 +361,16 @@ TestRestClient.HttpResponse ok(final CheckedSupplier endpointCallback, + final String expectedMessage + ) throws Exception { + final var response = endpointCallback.get(); + assertThat(response.getBody(), response.getStatusCode(), equalTo(HttpStatus.SC_OK)); + assertResponseBody(response.getBody(), expectedMessage); + return response; + } + TestRestClient.HttpResponse unauthorized(final CheckedSupplier endpointCallback) throws Exception { final var response = endpointCallback.get(); @@ -373,6 +384,12 @@ void assertResponseBody(final String responseBody) { assertThat(responseBody, not(equalTo(""))); } + void assertResponseBody(final String responseBody, final String expectedMessage) { + assertThat(responseBody, notNullValue()); + assertThat(responseBody, not(equalTo(""))); + assertThat(responseBody, containsString(expectedMessage)); + } + static ToXContentObject configJsonArray(final String... values) { return (builder, params) -> { builder.startArray(); diff --git a/src/integrationTest/java/org/opensearch/security/api/ConfigRestApiIntegrationTest.java b/src/integrationTest/java/org/opensearch/security/api/ConfigRestApiIntegrationTest.java index 3ccafda1d2..9b50b160ee 100644 --- a/src/integrationTest/java/org/opensearch/security/api/ConfigRestApiIntegrationTest.java +++ b/src/integrationTest/java/org/opensearch/security/api/ConfigRestApiIntegrationTest.java @@ -106,7 +106,14 @@ void verifyUpdate(final TestRestClient client) throws Exception { final var dynamicConfigJson = (ObjectNode) configJson.get("config").get("dynamic"); dynamicConfigJson.set("auth_failure_listeners", authFailureListeners); ok(() -> client.putJson(securityConfigPath("config"), DefaultObjectMapper.writeValueAsString(configJson.get("config"), false))); - ok(() -> client.patch(securityConfigPath(), patch(replaceOp("/config/dynamic/hosts_resolver_mode", "other")))); + String originalHostResolverMode = configJson.get("config").get("dynamic").get("hosts_resolver_mode").asText(); + String nextOriginalHostResolverMode = originalHostResolverMode.equals("other") ? "ip-only" : "other"; + ok(() -> client.patch(securityConfigPath(), patch(replaceOp("/config/dynamic/hosts_resolver_mode", nextOriginalHostResolverMode)))); + ok(() -> client.patch(securityConfigPath(), patch(replaceOp("/config/dynamic/hosts_resolver_mode", originalHostResolverMode)))); + ok( + () -> client.patch(securityConfigPath(), patch(replaceOp("/config/dynamic/hosts_resolver_mode", originalHostResolverMode))), + "No updates required" + ); } void verifyNotAllowedMethods(final TestRestClient client) throws Exception { diff --git a/src/integrationTest/java/org/opensearch/security/api/RolesMappingRestApiIntegrationTest.java b/src/integrationTest/java/org/opensearch/security/api/RolesMappingRestApiIntegrationTest.java index c6b40bdf11..ac5e4b21d4 100644 --- a/src/integrationTest/java/org/opensearch/security/api/RolesMappingRestApiIntegrationTest.java +++ b/src/integrationTest/java/org/opensearch/security/api/RolesMappingRestApiIntegrationTest.java @@ -269,6 +269,7 @@ void verifyCrudOperations(Boolean hidden, Boolean reserved, TestRestClient clien ok(() -> client.patch(apiPath(roleName), patch(addOp("hosts", configJsonArray("e", "f"))))); ok(() -> client.patch(apiPath(roleName), patch(addOp("users", configJsonArray("g", "h"))))); ok(() -> client.patch(apiPath(roleName), patch(addOp("and_backend_roles", configJsonArray("i", "j"))))); + ok(() -> client.patch(apiPath(roleName), patch(addOp("and_backend_roles", configJsonArray("i", "j")))), "No updates required"); ok(() -> client.patch(apiPath(), patch(removeOp(roleName)))); notFound(() -> client.get(apiPath(roleName))); diff --git a/src/integrationTest/java/org/opensearch/security/api/RolesRestApiIntegrationTest.java b/src/integrationTest/java/org/opensearch/security/api/RolesRestApiIntegrationTest.java index 178436b75a..f52fe5fbfd 100644 --- a/src/integrationTest/java/org/opensearch/security/api/RolesRestApiIntegrationTest.java +++ b/src/integrationTest/java/org/opensearch/security/api/RolesRestApiIntegrationTest.java @@ -101,7 +101,14 @@ void verifyCrudOperations(final Boolean hidden, final Boolean reserved, final Te ); // TODO related to issue #4426 - ok(() -> client.patch(apiPath("new_role_for_patch"), patch(replaceOp("cluster_permissions", configJsonArray("a", "b"))))); + ok( + () -> client.patch(apiPath("new_role_for_patch"), patch(replaceOp("cluster_permissions", configJsonArray("a", "b")))), + "No updates required" + ); + ok( + () -> client.patch(apiPath("new_role_for_patch"), patch(replaceOp("cluster_permissions", configJsonArray("a", "b", "c")))), + "'new_role_for_patch' updated." + ); ok(() -> client.patch(apiPath("new_role_for_patch"), patch(addOp("index_permissions", randomIndexPermissions(false))))); ok(() -> client.patch(apiPath("new_role_for_patch"), patch(addOp("tenant_permissions", randomTenantPermissions(false))))); diff --git a/src/main/java/org/opensearch/security/dlic/rest/api/AbstractApiAction.java b/src/main/java/org/opensearch/security/dlic/rest/api/AbstractApiAction.java index bee0fe5579..eb82bed908 100644 --- a/src/main/java/org/opensearch/security/dlic/rest/api/AbstractApiAction.java +++ b/src/main/java/org/opensearch/security/dlic/rest/api/AbstractApiAction.java @@ -66,6 +66,7 @@ import org.opensearch.security.user.User; import org.opensearch.threadpool.ThreadPool; +import com.flipkart.zjsonpatch.JsonDiff; import com.flipkart.zjsonpatch.JsonPatch; import com.flipkart.zjsonpatch.JsonPatchApplicationException; @@ -203,7 +204,7 @@ protected final ValidationResult patchEntity( final var entityAsJson = (ObjectNode) configurationAsJson.get(entityName); return withJsonPatchException( () -> endpointValidator.createRequestContentValidator(entityName) - .validate(request, JsonPatch.apply(patchContent, entityAsJson)) + .validate(request, JsonPatch.apply(patchContent, entityAsJson), configurationAsJson.get(entityName)) .map( patchedEntity -> endpointValidator.onConfigChange( SecurityConfiguration.of(patchedEntity, entityName, configuration) @@ -238,6 +239,10 @@ protected ValidationResult patchEntities( final var configurationAsJson = (ObjectNode) Utils.convertJsonToJackson(configuration, true); return withIOException(() -> withJsonPatchException(() -> { final var patchedConfigurationAsJson = JsonPatch.apply(patchContent, configurationAsJson); + JsonNode patch = JsonDiff.asJson(configurationAsJson, patchedConfigurationAsJson); + if (patch.isEmpty()) { + return ValidationResult.error(RestStatus.OK, payload(RestStatus.OK, "No updates required")); + } for (final var entityName : patchEntityNames(patchContent)) { final var beforePatchEntity = configurationAsJson.get(entityName); final var patchedEntity = patchedConfigurationAsJson.get(entityName); diff --git a/src/main/java/org/opensearch/security/dlic/rest/validation/RequestContentValidator.java b/src/main/java/org/opensearch/security/dlic/rest/validation/RequestContentValidator.java index 4d9faf096c..db6d3b4883 100644 --- a/src/main/java/org/opensearch/security/dlic/rest/validation/RequestContentValidator.java +++ b/src/main/java/org/opensearch/security/dlic/rest/validation/RequestContentValidator.java @@ -33,6 +33,9 @@ import org.opensearch.rest.RestRequest; import org.opensearch.security.DefaultObjectMapper; +import com.flipkart.zjsonpatch.JsonDiff; + +import static org.opensearch.security.dlic.rest.api.Responses.payload; import static org.opensearch.security.support.ConfigConstants.SECURITY_RESTAPI_PASSWORD_VALIDATION_ERROR_MESSAGE; public class RequestContentValidator implements ToXContent { @@ -132,6 +135,18 @@ public ValidationResult validate(final RestRequest request, final Json .map(ignored -> validatePassword(request, jsonContent)); } + public ValidationResult validate(final RestRequest request, final JsonNode patchedContent, final JsonNode originalContent) + throws IOException { + JsonNode patch = JsonDiff.asJson(originalContent, patchedContent); + if (patch.isEmpty()) { + return ValidationResult.error(RestStatus.OK, payload(RestStatus.OK, "No updates required")); + } + return validateContentSize(patchedContent).map(this::validateJsonKeys) + .map(this::validateDataType) + .map(this::nullValuesInArrayValidator) + .map(ignored -> validatePassword(request, patchedContent)); + } + private ValidationResult parseRequestContent(final RestRequest request) { try { final JsonNode jsonContent = DefaultObjectMapper.readTree(request.content().utf8ToString()); diff --git a/src/test/java/org/opensearch/security/dlic/rest/api/AuditApiActionTest.java b/src/test/java/org/opensearch/security/dlic/rest/api/AuditApiActionTest.java index 92ce7c9112..929f58c23c 100644 --- a/src/test/java/org/opensearch/security/dlic/rest/api/AuditApiActionTest.java +++ b/src/test/java/org/opensearch/security/dlic/rest/api/AuditApiActionTest.java @@ -13,8 +13,10 @@ import java.io.IOException; import java.util.Collections; +import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.Set; import java.util.stream.Collectors; import com.google.common.collect.ImmutableList; @@ -344,6 +346,7 @@ private void testReadonlyMap(final ObjectNode json, final String config, final S adminCredsHeader ); assertEquals(HttpStatus.SC_OK, response.getStatusCode()); + assertTrue(response.getBody().contains("No updates required")); } private void testReadonlyCategories(final ObjectNode json, final String config, final String resource) throws Exception { @@ -587,7 +590,9 @@ private void testMap( "[{\"op\": \"add\",\"path\": \"" + patchResource + "\",\"value\": {}}]", headers ); - assertEquals(expectedStatus, response.getStatusCode()); + Set expectedSet = new HashSet<>(List.of(expectedStatus)); + expectedSet.add(HttpStatus.SC_BAD_REQUEST); + assertTrue(expectedSet.contains(response.getStatusCode())); if (expectedStatus == HttpStatus.SC_OK) { assertEquals(0, readTree(rh.executeGetRequest(ENDPOINT, headers).getBody()).at(patchResource).size()); } @@ -663,8 +668,15 @@ public void testPatchRequest() throws Exception { assertEquals(HttpStatus.SC_OK, response.getStatusCode()); // make patch request + response = rh.executePatchRequest(ENDPOINT, "[{\"op\": \"add\",\"path\": \"" + "/config/enabled" + "\",\"value\": " + false + "}]"); + assertEquals(HttpStatus.SC_OK, response.getStatusCode()); + + response = rh.executePatchRequest(ENDPOINT, "[{\"op\": \"add\",\"path\": \"" + "/config/enabled" + "\",\"value\": " + true + "}]"); + assertEquals(HttpStatus.SC_OK, response.getStatusCode()); + response = rh.executePatchRequest(ENDPOINT, "[{\"op\": \"add\",\"path\": \"" + "/config/enabled" + "\",\"value\": " + true + "}]"); assertEquals(HttpStatus.SC_OK, response.getStatusCode()); + assertTrue(response.getBody().contains("No updates required")); // get config response = rh.executeGetRequest(ENDPOINT);