Update PATCH API to fail validation if nothing changes (opensearch-pr…

…oject#4530)

Signed-off-by: Craig Perkins <[email protected]>

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<TestRestClient.HttpResponse
         return response;
     }
 
+    TestRestClient.HttpResponse ok(
+        final CheckedSupplier<TestRestClient.HttpResponse, Exception> 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<TestRestClient.HttpResponse, Exception> 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();

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 {

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)));

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)))));
 

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<SecurityConfiguration> 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<SecurityConfiguration> 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);

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<JsonNode> validate(final RestRequest request, final Json
             .map(ignored -> validatePassword(request, jsonContent));
     }
 
+    public ValidationResult<JsonNode> 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<JsonNode> parseRequestContent(final RestRequest request) {
         try {
             final JsonNode jsonContent = DefaultObjectMapper.readTree(request.content().utf8ToString());

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<Integer> 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);