Stricter UpdateSettingsRequest parsing on the REST layer (#79227)

As per the comment in issue #29268, in 8.0 this commit:

* Changes behaviour to disallow a request with mixed settings.
* A mixed request will no longer successfully parse.
*A mixed request will result in a 400 "Bad Request" response.

The test has been expanded to cover more cases per test invocation run,
rather than relying on randomization from run to run to cover mixed
request scenarios.

Author: ChrisHegarty

server/src/main/java/org/elasticsearch/action/admin/indices/settings/put/UpdateSettingsRequest.java

@@ -8,6 +8,7 @@
 
 package org.elasticsearch.action.admin.indices.settings.put;
 
+import org.elasticsearch.ElasticsearchParseException;
 import org.elasticsearch.Version;
 import org.elasticsearch.action.ActionRequestValidationException;
 import org.elasticsearch.action.IndicesRequest;
@@ -204,12 +205,25 @@ public UpdateSettingsRequest fromXContent(XContentParser parser) throws IOExcept
             @SuppressWarnings("unchecked")
             Map<String, Object> innerBodySettingsMap = (Map<String, Object>) innerBodySettings;
             settings.putAll(innerBodySettingsMap);
+            checkMixedRequest(bodySettings);
         } else {
             settings.putAll(bodySettings);
         }
         return this.settings(settings);
     }
 
+    /**
+     * Checks if the request is a "mixed request". A mixed request contains both a
+     * "settings" map and "other" properties. Detection of a mixed request
+     * will result in a parse exception being thrown.
+     */
+    private static void checkMixedRequest(Map<String, Object> bodySettings) {
+        assert bodySettings.containsKey("settings");
+        if (bodySettings.size() > 1) {
+            throw new ElasticsearchParseException("mix of settings map and top-level properties");
+        }
+    }
+
     @Override
     public String toString() {
         return "indices : " + Arrays.toString(indices) + "," + Strings.toString(this);

server/src/test/java/org/elasticsearch/action/admin/indices/settings/put/UpdateSettingsRequestTests.java

@@ -8,19 +8,61 @@
 
 package org.elasticsearch.action.admin.indices.settings.put;
 
+import org.elasticsearch.ElasticsearchParseException;
+import org.elasticsearch.common.bytes.BytesReference;
+import org.elasticsearch.common.xcontent.XContentHelper;
+import org.elasticsearch.test.XContentTestUtils;
+import org.elasticsearch.xcontent.ToXContent;
 import org.elasticsearch.xcontent.XContentBuilder;
+import org.elasticsearch.xcontent.XContentFactory;
 import org.elasticsearch.xcontent.XContentParser;
 import org.elasticsearch.test.AbstractXContentTestCase;
+import org.elasticsearch.xcontent.XContentType;
 
 import java.io.IOException;
 import java.util.function.Predicate;
 
+import static org.hamcrest.Matchers.equalTo;
+import static org.hamcrest.Matchers.nullValue;
+
 public class UpdateSettingsRequestTests extends AbstractXContentTestCase<UpdateSettingsRequest> {
 
-    private final boolean enclosedSettings = randomBoolean();
+    /** True if the setting should be enclosed in a settings map. */
+    private final boolean enclosedSettings;
+    /** True if the request should contain unknown top-level properties. */
+    private final boolean unknownFields;
+
+    public UpdateSettingsRequestTests() {
+        this(randomBoolean(), false);
+    }
+
+    private UpdateSettingsRequestTests(boolean enclosedSettings, boolean unknownFields) {
+        this.enclosedSettings = enclosedSettings;
+        this.unknownFields = unknownFields;
+    }
+
+    final UpdateSettingsRequestTests withEnclosedSettings() {
+        return new UpdateSettingsRequestTests(true, unknownFields);
+    }
+
+    final UpdateSettingsRequestTests withoutEnclosedSettings() {
+        return new UpdateSettingsRequestTests(false, unknownFields);
+    }
+
+    final UpdateSettingsRequestTests withUnknownFields() {
+        return new UpdateSettingsRequestTests(enclosedSettings, true);
+    }
+
+    final UpdateSettingsRequestTests withoutUnknownFields() {
+        return new UpdateSettingsRequestTests(enclosedSettings, false);
+    }
 
     @Override
     protected UpdateSettingsRequest createTestInstance() {
+        return createTestInstance(enclosedSettings);
+    }
+
+    private static UpdateSettingsRequest createTestInstance(boolean enclosedSettings) {
         UpdateSettingsRequest testRequest = UpdateSettingsRequestSerializationTests.createTestItem();
         if (enclosedSettings) {
             UpdateSettingsRequest requestWithEnclosingSettings = new UpdateSettingsRequest(testRequest.settings()) {
@@ -40,14 +82,21 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
 
     @Override
     protected UpdateSettingsRequest doParseInstance(XContentParser parser) throws IOException {
-        return new UpdateSettingsRequest().fromXContent(parser);
+        if (mixedRequest() == false) {
+            return new UpdateSettingsRequest().fromXContent(parser);
+        } else {
+            ElasticsearchParseException e = expectThrows(ElasticsearchParseException.class,
+                () -> (new UpdateSettingsRequest()).fromXContent(parser));
+            assertThat(e.getMessage(), equalTo("mix of settings map and top-level properties"));
+            return null;
+        }
     }
 
     @Override
     protected boolean supportsUnknownFields() {
-        // if the settings are enclose as a "settings" object
+        // if the settings are enclosed as a "settings" object
         // then all other top-level elements will be ignored during the parsing
-        return enclosedSettings;
+        return enclosedSettings && unknownFields;
     }
 
     @Override
@@ -62,8 +111,12 @@ protected Predicate<String> getRandomFieldsExcludeFilter() {
     protected void assertEqualInstances(UpdateSettingsRequest expectedInstance, UpdateSettingsRequest newInstance) {
         // here only the settings should be tested, as this test covers explicitly only the XContent parsing
         // the rest of the request fields are tested by the SerializingTests
-        super.assertEqualInstances(new UpdateSettingsRequest(expectedInstance.settings()),
-                new UpdateSettingsRequest(newInstance.settings()));
+        if (mixedRequest() == false) {
+            super.assertEqualInstances(new UpdateSettingsRequest(expectedInstance.settings()),
+                    new UpdateSettingsRequest(newInstance.settings()));
+        } else {
+            assertThat(newInstance, nullValue()); // sanity
+        }
     }
 
     @Override
@@ -73,4 +126,47 @@ protected boolean assertToXContentEquivalence() {
         return enclosedSettings == false;
     }
 
+    final boolean mixedRequest() {
+        return enclosedSettings && unknownFields;
+    }
+
+    public void testWithEnclosedSettingsWithUnknownFields() throws IOException {
+        testFromXContent((new UpdateSettingsRequestTests()).withEnclosedSettings().withUnknownFields());
+    }
+
+    public void testWithEnclosedSettingsWithoutUnknownFields() throws IOException {
+        testFromXContent((new UpdateSettingsRequestTests()).withEnclosedSettings().withoutUnknownFields());
+    }
+
+    public void testWithoutEnclosedSettingsWithoutUnknownFields() throws IOException {
+        testFromXContent((new UpdateSettingsRequestTests()).withoutEnclosedSettings().withoutUnknownFields());
+    }
+
+    private static void testFromXContent(UpdateSettingsRequestTests test) throws IOException {
+        AbstractXContentTestCase.testFromXContent(NUMBER_OF_TEST_RUNS / 2,
+            test::createTestInstance,
+            test.supportsUnknownFields(),
+            test.getShuffleFieldsExceptions(),
+            test.getRandomFieldsExcludeFilter(),
+            test::createParser,
+            test::doParseInstance,
+            test::assertEqualInstances,
+            test.assertToXContentEquivalence(),
+            test.getToXContentParams());
+    }
+
+    /** Tests that mixed requests, containing both an enclosed settings and top-level fields, generate a validation error message. */
+    public void testMixedFields() throws Exception {
+        UpdateSettingsRequestTests test = (new UpdateSettingsRequestTests()).withEnclosedSettings().withUnknownFields();
+        UpdateSettingsRequest updateSettingsRequest = test.createTestInstance();
+        XContentType xContentType = randomFrom(XContentType.values());
+        BytesReference originalXContent = XContentHelper.toXContent(updateSettingsRequest, xContentType,
+            ToXContent.EMPTY_PARAMS, false);
+        BytesReference updatedXContent = XContentTestUtils.insertRandomFields(xContentType, originalXContent,
+            test.getRandomFieldsExcludeFilter(), random());
+        XContentParser parser = test.createParser(XContentFactory.xContent(xContentType), updatedXContent);
+        ElasticsearchParseException e = expectThrows(ElasticsearchParseException.class,
+            () -> (new UpdateSettingsRequest()).fromXContent(parser));
+        assertThat(e.getMessage(), equalTo("mix of settings map and top-level properties"));
+    }
 }