From 7096856d65ebe05d543db8f53931584fc3e4d645 Mon Sep 17 00:00:00 2001 From: Jay Modi Date: Mon, 14 Dec 2020 15:21:39 -0700 Subject: [PATCH] RestController should not consume request content (#66043) 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. Backport of #44902 Closes #65242 Co-authored-by: Tanguy Leroux --- .../client/RollupRequestConverters.java | 12 ++---- .../client/rollup/DeleteRollupJobRequest.java | 31 +-------------- .../client/rollup/GetRollupCapsRequest.java | 15 +------- .../rollup/GetRollupIndexCapsRequest.java | 22 +---------- .../rollup/DeleteRollupJobRequestTests.java | 31 +-------------- .../elasticsearch/rest/RestController.java | 2 +- .../org/elasticsearch/rest/RestRequest.java | 10 ++--- .../forcemerge/RestForceMergeActionTests.java | 38 +++++++++++++++---- .../rest/RestControllerTests.java | 33 ++++++++++++++++ .../elasticsearch/rest/RestRequestTests.java | 4 ++ .../ccr/rest/RestForgetFollowerAction.java | 23 +++++------ .../action/user/RestChangePasswordAction.java | 4 +- 12 files changed, 94 insertions(+), 131 deletions(-) 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 index 48d297a5eae35..ac7b59040718e 100644 --- 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 { @@ -114,9 +112,7 @@ 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 { @@ -124,8 +120,6 @@ static Request getRollupIndexCaps(final GetRollupIndexCapsRequest getRollupIndex .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 index 9b7a322b23827..7302dbca6916c 100644 --- 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 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 index b07089cf13876..44593c3d66952 100644 --- 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 index 6bb0c9b48d614..bbd341b5daf6d 100644 --- 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 index c1271207d41bc..c4a53c4d14c69 100644 --- 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 { - - 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 index 730d9d85b140e..059d085af6222 100644 --- a/server/src/main/java/org/elasticsearch/rest/RestController.java +++ b/server/src/main/java/org/elasticsearch/rest/RestController.java @@ -222,7 +222,7 @@ public void dispatchBadRequest(final RestChannel channel, final ThreadContext th } private void dispatchRequest(RestRequest request, RestChannel channel, RestHandler handler) throws Exception { - final int contentLength = request.content().length(); + final int contentLength = request.contentLength(); if (contentLength > 0) { final XContentType xContentType = request.getXContentType(); if (xContentType == null) { diff --git a/server/src/main/java/org/elasticsearch/rest/RestRequest.java b/server/src/main/java/org/elasticsearch/rest/RestRequest.java index eb7dcc24d555f..512bf72e9c0d3 100644 --- a/server/src/main/java/org/elasticsearch/rest/RestRequest.java +++ b/server/src/main/java/org/elasticsearch/rest/RestRequest.java @@ -209,15 +209,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 index 102dcbcfa11fc..f827d592c3429 100644 --- 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,14 +19,19 @@ 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.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.rest.FakeRestChannel; import org.elasticsearch.test.rest.FakeRestRequest; import org.elasticsearch.test.rest.RestActionTestCase; import org.junit.Before; @@ -34,8 +39,9 @@ import java.util.HashMap; import java.util.Map; -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 RestActionTestCase { @@ -44,16 +50,27 @@ public void setUpAction() { controller().registerHandler(new RestForceMergeAction()); } + public void testBodyRejection() throws Exception { - final RestForceMergeAction handler = new RestForceMergeAction(); 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 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")); } public void testDeprecationMessage() { @@ -75,4 +92,9 @@ public void testDeprecationMessage() { assertWarnings("setting only_expunge_deletes and max_num_segments at the same time is deprecated " + "and will be rejected in a future version"); } + + 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 index 0e8191f38549d..2d290cf7e0a8c 100644 --- a/server/src/test/java/org/elasticsearch/rest/RestControllerTests.java +++ b/server/src/test/java/org/elasticsearch/rest/RestControllerTests.java @@ -31,6 +31,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.core.internal.io.IOUtils; @@ -482,6 +483,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 index b0d9847c94732..6e12a96d8a6c2 100644 --- a/server/src/test/java/org/elasticsearch/rest/RestRequestTests.java +++ b/server/src/test/java/org/elasticsearch/rest/RestRequestTests.java @@ -83,6 +83,10 @@ public void testHasContentDoesNotConsumesContent() { runConsumesContentTest(RestRequest::hasContent, false); } + public void testContentLengthDoesNotConsumesContent() { + runConsumesContentTest(RestRequest::contentLength, false); + } + private void runConsumesContentTest( final CheckedConsumer 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 index 2b3e71917decd..bb8246a7a780b 100644 --- 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 @@ -9,11 +9,10 @@ import org.elasticsearch.client.node.NodeClient; import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.rest.BaseRestHandler; -import org.elasticsearch.rest.BytesRestResponse; 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.List; @@ -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 index d9747f9e5d577..79a8aeb5ef1b5 100644 --- 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 @@ -7,6 +7,7 @@ import org.elasticsearch.action.ActionResponse; import org.elasticsearch.client.node.NodeClient; +import org.elasticsearch.common.bytes.BytesReference; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.license.XPackLicenseState; @@ -79,9 +80,10 @@ public RestChannelConsumer innerPrepareRequest(RestRequest request, NodeClient c } final String refresh = request.param("refresh"); + final BytesReference content = request.requiredContent(); return channel -> new SecurityClient(client) - .prepareChangePassword(username, request.requiredContent(), request.getXContentType(), passwordHasher) + .prepareChangePassword(username, content, request.getXContentType(), passwordHasher) .setRefreshPolicy(refresh) .execute(new RestBuilderListener(channel) { @Override