RestController should not consume request content (#44902)

tlrx · jkakavas · commit 929ef7facd72 · 2019-07-31T18:43:25.000+03:00
The change #37504 modifies the BaseRestHandler to make it reject all requests that have an unconsumed body. The notion of consumed or unconsumed body is carried by the RestRequest object and its contentConsumed attribute, which is set to true when the content() or content(true) methods are used. In our REST layer, we usually expect the RestHandlers to consume the request content when needed, but it appears that the RestController always consumes the content upfront. This commit changes the content() method used by the RestController so that it does not mark the content as consumed.
diff --git a/client/rest-high-level/src/main/java/org/elasticsearch/client/RollupRequestConverters.java b/client/rest-high-level/src/main/java/org/elasticsearch/client/RollupRequestConverters.java
@@ -90,9 +90,7 @@ static Request deleteJob(final DeleteRollupJobRequest deleteRollupJobRequest) th
             .addPathPartAsIs("_rollup", "job")
             .addPathPart(deleteRollupJobRequest.getId())
             .build();
-        Request request = new Request(HttpDelete.METHOD_NAME, endpoint);
-        request.setEntity(createEntity(deleteRollupJobRequest, REQUEST_BODY_CONTENT_TYPE));
-        return request;
+        return new Request(HttpDelete.METHOD_NAME, endpoint);
     }
 
     static Request search(final SearchRequest request) throws IOException {
@@ -104,18 +102,14 @@ static Request getRollupCaps(final GetRollupCapsRequest getRollupCapsRequest) th
             .addPathPartAsIs("_rollup", "data")
             .addPathPart(getRollupCapsRequest.getIndexPattern())
             .build();
-        Request request = new Request(HttpGet.METHOD_NAME, endpoint);
-        request.setEntity(createEntity(getRollupCapsRequest, REQUEST_BODY_CONTENT_TYPE));
-        return request;
+        return new Request(HttpGet.METHOD_NAME, endpoint);
     }
 
     static Request getRollupIndexCaps(final GetRollupIndexCapsRequest getRollupIndexCapsRequest) throws IOException {
         String endpoint = new RequestConverters.EndpointBuilder()
             .addCommaSeparatedPathParts(getRollupIndexCapsRequest.indices())
             .addPathPartAsIs("_rollup", "data")
             .build();
-        Request request = new Request(HttpGet.METHOD_NAME, endpoint);
-        request.setEntity(createEntity(getRollupIndexCapsRequest, REQUEST_BODY_CONTENT_TYPE));
-        return request;
+        return new Request(HttpGet.METHOD_NAME, endpoint);
     }
 }
diff --git a/client/rest-high-level/src/main/java/org/elasticsearch/client/rollup/DeleteRollupJobRequest.java b/client/rest-high-level/src/main/java/org/elasticsearch/client/rollup/DeleteRollupJobRequest.java
@@ -19,22 +19,14 @@
 package org.elasticsearch.client.rollup;
 
 import org.elasticsearch.client.Validatable;
-import org.elasticsearch.common.ParseField;
-import org.elasticsearch.common.xcontent.ConstructingObjectParser;
-import org.elasticsearch.common.xcontent.ToXContentObject;
-import org.elasticsearch.common.xcontent.XContentBuilder;
-import org.elasticsearch.common.xcontent.XContentParser;
 
-import java.io.IOException;
 import java.util.Objects;
 
 
-public class DeleteRollupJobRequest implements Validatable, ToXContentObject {
+public class DeleteRollupJobRequest implements Validatable {
 
-    private static final ParseField ID_FIELD = new ParseField("id");
     private final String id;
 
-
     public DeleteRollupJobRequest(String id) {
         this.id = Objects.requireNonNull(id, "id parameter must not be null");
     }
@@ -43,27 +35,6 @@ public String getId() {
         return id;
     }
 
-    private static final ConstructingObjectParser<DeleteRollupJobRequest, Void> PARSER =
-        new ConstructingObjectParser<>("request",  a -> {
-            return new DeleteRollupJobRequest((String) a[0]);
-        });
-
-    static {
-        PARSER.declareString(ConstructingObjectParser.constructorArg(), ID_FIELD);
-    }
-
-    public static DeleteRollupJobRequest fromXContent(XContentParser parser) {
-        return PARSER.apply(parser, null);
-    }
-
-    @Override
-    public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
-        builder.startObject();
-        builder.field(ID_FIELD.getPreferredName(), this.id);
-        builder.endObject();
-        return builder;
-    }
-
     @Override
     public boolean equals(Object o) {
         if (this == o) return true;
diff --git a/client/rest-high-level/src/main/java/org/elasticsearch/client/rollup/GetRollupCapsRequest.java b/client/rest-high-level/src/main/java/org/elasticsearch/client/rollup/GetRollupCapsRequest.java
@@ -21,14 +21,11 @@
 import org.elasticsearch.client.Validatable;
 import org.elasticsearch.cluster.metadata.MetaData;
 import org.elasticsearch.common.Strings;
-import org.elasticsearch.common.xcontent.ToXContentObject;
-import org.elasticsearch.common.xcontent.XContentBuilder;
 
-import java.io.IOException;
 import java.util.Objects;
 
-public class GetRollupCapsRequest implements Validatable, ToXContentObject {
-    private static final String ID = "id";
+public class GetRollupCapsRequest implements Validatable {
+
     private final String indexPattern;
 
     public GetRollupCapsRequest(final String indexPattern) {
@@ -43,14 +40,6 @@ public String getIndexPattern() {
         return indexPattern;
     }
 
-    @Override
-    public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
-        builder.startObject();
-        builder.field(ID, indexPattern);
-        builder.endObject();
-        return builder;
-    }
-
     @Override
     public int hashCode() {
         return Objects.hash(indexPattern);
diff --git a/client/rest-high-level/src/main/java/org/elasticsearch/client/rollup/GetRollupIndexCapsRequest.java b/client/rest-high-level/src/main/java/org/elasticsearch/client/rollup/GetRollupIndexCapsRequest.java
@@ -21,16 +21,11 @@
 import org.elasticsearch.action.support.IndicesOptions;
 import org.elasticsearch.client.Validatable;
 import org.elasticsearch.common.Strings;
-import org.elasticsearch.common.xcontent.ToXContentObject;
-import org.elasticsearch.common.xcontent.XContentBuilder;
 
-import java.io.IOException;
 import java.util.Arrays;
 import java.util.Objects;
 
-public class GetRollupIndexCapsRequest implements Validatable, ToXContentObject {
-    private static final String INDICES = "indices";
-    private static final String INDICES_OPTIONS = "indices_options";
+public class GetRollupIndexCapsRequest implements Validatable {
 
     private String[] indices;
     private IndicesOptions options;
@@ -60,21 +55,6 @@ public String[] indices() {
         return indices;
     }
 
-    @Override
-    public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
-        builder.startObject();
-        {
-            builder.array(INDICES, indices);
-            builder.startObject(INDICES_OPTIONS);
-            {
-                options.toXContent(builder, params);
-            }
-            builder.endObject();
-        }
-        builder.endObject();
-        return builder;
-    }
-
     @Override
     public int hashCode() {
         return Objects.hash(Arrays.hashCode(indices), options);
diff --git a/client/rest-high-level/src/test/java/org/elasticsearch/client/rollup/DeleteRollupJobRequestTests.java b/client/rest-high-level/src/test/java/org/elasticsearch/client/rollup/DeleteRollupJobRequestTests.java
@@ -18,39 +18,12 @@
  */
 package org.elasticsearch.client.rollup;
 
-import org.elasticsearch.common.xcontent.XContentParser;
-import org.elasticsearch.test.AbstractXContentTestCase;
-import org.junit.Before;
+import org.elasticsearch.test.ESTestCase;
 
-import java.io.IOException;
-
-public class DeleteRollupJobRequestTests extends AbstractXContentTestCase<DeleteRollupJobRequest> {
-
-    private String jobId;
-
-    @Before
-    public void setUpOptionalId() {
-        jobId = randomAlphaOfLengthBetween(1, 10);
-    }
-
-    @Override
-    protected DeleteRollupJobRequest createTestInstance() {
-        return new DeleteRollupJobRequest(jobId);
-    }
-
-    @Override
-    protected DeleteRollupJobRequest doParseInstance(final XContentParser parser) throws IOException {
-        return DeleteRollupJobRequest.fromXContent(parser);
-    }
-
-    @Override
-    protected boolean supportsUnknownFields() {
-        return false;
-    }
+public class DeleteRollupJobRequestTests extends ESTestCase {
 
     public void testRequireConfiguration() {
         final NullPointerException e = expectThrows(NullPointerException.class, ()-> new DeleteRollupJobRequest(null));
         assertEquals("id parameter must not be null", e.getMessage());
     }
-
 }
diff --git a/server/src/main/java/org/elasticsearch/rest/RestController.java b/server/src/main/java/org/elasticsearch/rest/RestController.java
@@ -209,7 +209,7 @@ public void dispatchBadRequest(final RestRequest request, final RestChannel chan
      */
     boolean dispatchRequest(final RestRequest request, final RestChannel channel, final NodeClient client,
                             final Optional<RestHandler> mHandler) throws Exception {
-        final int contentLength = request.hasContent() ? request.content().length() : 0;
+        final int contentLength = request.contentLength();
 
         RestChannel responseChannel = channel;
         // Indicator of whether a response was sent or not
diff --git a/server/src/main/java/org/elasticsearch/rest/RestRequest.java b/server/src/main/java/org/elasticsearch/rest/RestRequest.java
@@ -185,15 +185,15 @@ public final String path() {
     }
 
     public boolean hasContent() {
-        return content(false).length() > 0;
+        return contentLength() > 0;
     }
 
-    public BytesReference content() {
-        return content(true);
+    public int contentLength() {
+        return httpRequest.content().length();
     }
 
-    protected BytesReference content(final boolean contentConsumed) {
-        this.contentConsumed = this.contentConsumed | contentConsumed;
+    public BytesReference content() {
+        this.contentConsumed = true;
         return httpRequest.content();
     }
 
diff --git a/server/src/test/java/org/elasticsearch/action/admin/indices/forcemerge/RestForceMergeActionTests.java b/server/src/test/java/org/elasticsearch/action/admin/indices/forcemerge/RestForceMergeActionTests.java
@@ -19,33 +19,58 @@
 
 package org.elasticsearch.action.admin.indices.forcemerge;
 
-import org.elasticsearch.client.node.NodeClient;
+import org.apache.lucene.util.SetOnce;
 import org.elasticsearch.common.bytes.BytesArray;
 import org.elasticsearch.common.settings.Settings;
+import org.elasticsearch.common.util.concurrent.ThreadContext;
 import org.elasticsearch.common.xcontent.NamedXContentRegistry;
 import org.elasticsearch.common.xcontent.XContentType;
 import org.elasticsearch.common.xcontent.json.JsonXContent;
-import org.elasticsearch.rest.RestController;
+import org.elasticsearch.rest.AbstractRestChannel;
+import org.elasticsearch.rest.RestChannel;
+import org.elasticsearch.rest.RestRequest;
+import org.elasticsearch.rest.RestResponse;
+import org.elasticsearch.rest.RestStatus;
 import org.elasticsearch.rest.action.admin.indices.RestForceMergeAction;
-import org.elasticsearch.test.ESTestCase;
-import org.elasticsearch.test.rest.FakeRestChannel;
 import org.elasticsearch.test.rest.FakeRestRequest;
+import org.elasticsearch.test.rest.RestActionTestCase;
+import org.junit.Before;
 
-import static org.hamcrest.Matchers.equalTo;
-import static org.mockito.Mockito.mock;
+import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.is;
+import static org.hamcrest.Matchers.notNullValue;
 
-public class RestForceMergeActionTests extends ESTestCase {
+public class RestForceMergeActionTests extends RestActionTestCase {
+
+    @Before
+    public void setUpAction() {
+        new RestForceMergeAction(Settings.EMPTY, controller());
+    }
 
     public void testBodyRejection() throws Exception {
-        final RestForceMergeAction handler = new RestForceMergeAction(Settings.EMPTY, mock(RestController.class));
         String json = JsonXContent.contentBuilder().startObject().field("max_num_segments", 1).endObject().toString();
         final FakeRestRequest request = new FakeRestRequest.Builder(NamedXContentRegistry.EMPTY)
                 .withContent(new BytesArray(json), XContentType.JSON)
+                .withMethod(RestRequest.Method.POST)
                 .withPath("/_forcemerge")
                 .build();
-        IllegalArgumentException e = expectThrows(IllegalArgumentException.class,
-            () -> handler.handleRequest(request, new FakeRestChannel(request, randomBoolean(), 1), mock(NodeClient.class)));
-        assertThat(e.getMessage(), equalTo("request [GET /_forcemerge] does not support having a body"));
+
+        final SetOnce<RestResponse> responseSetOnce = new SetOnce<>();
+        dispatchRequest(request, new AbstractRestChannel(request, true) {
+            @Override
+            public void sendResponse(RestResponse response) {
+                responseSetOnce.set(response);
+            }
+        });
+
+        final RestResponse response = responseSetOnce.get();
+        assertThat(response, notNullValue());
+        assertThat(response.status(), is(RestStatus.BAD_REQUEST));
+        assertThat(response.content().utf8ToString(), containsString("request [POST /_forcemerge] does not support having a body"));
     }
 
+    protected void dispatchRequest(final RestRequest request, final RestChannel channel) {
+        ThreadContext threadContext = new ThreadContext(Settings.EMPTY);
+        controller().dispatchRequest(request, channel, threadContext);
+    }
 }
diff --git a/server/src/test/java/org/elasticsearch/rest/RestControllerTests.java b/server/src/test/java/org/elasticsearch/rest/RestControllerTests.java
@@ -32,6 +32,7 @@
 import org.elasticsearch.common.unit.ByteSizeValue;
 import org.elasticsearch.common.util.concurrent.ThreadContext;
 import org.elasticsearch.common.xcontent.NamedXContentRegistry;
+import org.elasticsearch.common.xcontent.XContentBuilder;
 import org.elasticsearch.common.xcontent.XContentType;
 import org.elasticsearch.common.xcontent.yaml.YamlXContent;
 import org.elasticsearch.http.HttpInfo;
@@ -466,6 +467,38 @@ public void testDispatchBadRequest() {
         assertThat(channel.getRestResponse().content().utf8ToString(), containsString("bad request"));
     }
 
+    public void testDoesNotConsumeContent() throws Exception {
+        final RestRequest.Method method = randomFrom(RestRequest.Method.values());
+        restController.registerHandler(method, "/notconsumed", new RestHandler() {
+            @Override
+            public void handleRequest(RestRequest request, RestChannel channel, NodeClient client) throws Exception {
+                channel.sendResponse(new BytesRestResponse(RestStatus.OK, BytesRestResponse.TEXT_CONTENT_TYPE, BytesArray.EMPTY));
+            }
+
+            @Override
+            public boolean canTripCircuitBreaker() {
+                return false;
+            }
+        });
+
+        final XContentBuilder content = XContentBuilder.builder(randomFrom(XContentType.values()).xContent())
+            .startObject().field("field", "value").endObject();
+        final FakeRestRequest restRequest = new FakeRestRequest.Builder(xContentRegistry())
+            .withPath("/notconsumed")
+            .withMethod(method)
+            .withContent(BytesReference.bytes(content), content.contentType())
+            .build();
+
+        final AssertingChannel channel = new AssertingChannel(restRequest, true, RestStatus.OK);
+        assertFalse(channel.getSendResponseCalled());
+        assertFalse(restRequest.isContentConsumed());
+
+        restController.dispatchRequest(restRequest, channel, new ThreadContext(Settings.EMPTY));
+
+        assertTrue(channel.getSendResponseCalled());
+        assertFalse("RestController must not consume request content", restRequest.isContentConsumed());
+    }
+
     public void testDispatchBadRequestUnknownCause() {
         final FakeRestRequest fakeRestRequest = new FakeRestRequest.Builder(NamedXContentRegistry.EMPTY).build();
         final AssertingChannel channel = new AssertingChannel(fakeRestRequest, true, RestStatus.BAD_REQUEST);
diff --git a/server/src/test/java/org/elasticsearch/rest/RestRequestTests.java b/server/src/test/java/org/elasticsearch/rest/RestRequestTests.java
@@ -82,6 +82,10 @@ public void testHasContentDoesNotConsumesContent() {
         runConsumesContentTest(RestRequest::hasContent, false);
     }
 
+    public void testContentLengthDoesNotConsumesContent() {
+        runConsumesContentTest(RestRequest::contentLength, false);
+    }
+
     private <T extends Exception> void runConsumesContentTest(
             final CheckedConsumer<RestRequest, T> consumer, final boolean expected) {
         final HttpRequest httpRequest = mock(HttpRequest.class);
diff --git a/x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/rest/RestForgetFollowerAction.java b/x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/rest/RestForgetFollowerAction.java
@@ -10,12 +10,11 @@
 import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.common.xcontent.XContentParser;
 import org.elasticsearch.rest.BaseRestHandler;
-import org.elasticsearch.rest.BytesRestResponse;
 import org.elasticsearch.rest.RestController;
 import org.elasticsearch.rest.RestRequest;
-import org.elasticsearch.rest.RestStatus;
 import org.elasticsearch.rest.action.RestToXContentListener;
 import org.elasticsearch.xpack.core.ccr.action.ForgetFollowerAction;
+import org.elasticsearch.xpack.core.ccr.action.ForgetFollowerAction.Request;
 
 import java.io.IOException;
 import java.util.Objects;
@@ -34,18 +33,14 @@ public String getName() {
     }
 
     @Override
-    protected RestChannelConsumer prepareRequest(final RestRequest restRequest, final NodeClient client) {
-        final String leaderIndex = restRequest.param("index");
-
-        return channel -> {
-            try (XContentParser parser = restRequest.contentOrSourceParamParser()) {
-                final ForgetFollowerAction.Request request = ForgetFollowerAction.Request.fromXContent(parser, leaderIndex);
-                client.execute(ForgetFollowerAction.INSTANCE, request, new RestToXContentListener<>(channel));
-            } catch (final IOException e) {
-                channel.sendResponse(new BytesRestResponse(channel, RestStatus.INTERNAL_SERVER_ERROR, e));
-            }
-        };
-
+    protected RestChannelConsumer prepareRequest(final RestRequest restRequest, final NodeClient client) throws IOException {
+        final Request request = createRequest(restRequest, restRequest.param("index"));
+        return channel -> client.execute(ForgetFollowerAction.INSTANCE, request, new RestToXContentListener<>(channel));
     }
 
+    private static Request createRequest(final RestRequest restRequest, final String leaderIndex) throws IOException {
+        try (XContentParser parser = restRequest.contentOrSourceParamParser()) {
+            return Request.fromXContent(parser, leaderIndex);
+        }
+    }
 }
diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/rest/action/user/RestChangePasswordAction.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/rest/action/user/RestChangePasswordAction.java

Original file line number	Diff line number	Diff line change
`@@ -185,15 +185,15 @@ public final String path() {`
`185`	`185`	`}`
`186`	`186`
`187`	`187`	`public boolean hasContent() {`
`188`		`- return content(false).length() > 0;`
	`188`	`+ return contentLength() > 0;`
`189`	`189`	`}`
`190`	`190`
`191`		`- public BytesReference content() {`
`192`		`- return content(true);`
	`191`	`+ public int contentLength() {`
	`192`	`+ return httpRequest.content().length();`
`193`	`193`	`}`
`194`	`194`
`195`		`- protected BytesReference content(final boolean contentConsumed) {`
`196`		`- this.contentConsumed = this.contentConsumed \| contentConsumed;`
	`195`	`+ public BytesReference content() {`
	`196`	`+ this.contentConsumed = true;`
`197`	`197`	`return httpRequest.content();`
`198`	`198`	`}`
`199`	`199`