Improve REST error handling when endpoint does not support HTTP verb, add OPTIONS support (#24437)

dakrone · web-flow · commit 8aa0a5c11144 · 2017-07-07T09:01:23.000-06:00
* Improved REST endpoint exception handling, see #15335 Also improved OPTIONS http method handling to better conform with the http spec. * Tidied up formatting and comments See #15335 * Tests for #15335 * Cleaned up comments, added section number * Swapped out tab indents for space indents * Test class now extends ESSingleNodeTestCase * Capture RestResponse so it can be examined in test cases Simple addition to surface the RestResponse object so we can run tests against it (see issue #15335). * Refactored class name, included feedback See #15335. * Unit test for REST error handling enhancements Randomizing unit test for enhanced REST response error handling. See issue #15335 for more details. * Cleaned up formatting * New constructor to set HTTP method Constructor added to support RestController test cases. * Refactored FakeRestRequest, streamlined test case. * Cleaned up conflicts * Tests for #15335 * Added functionality to ignore or include path wildcards See #15335 * Further enhancements to request handling Refactored executeHandler to prioritize explicit path matches. See #15335 for more information. * Cosmetic fixes * Refactored method handlers * Removed redundant import * Updated integration tests * Refactoring to address issue #17853 * Cleaned up test assertions * Fixed edge case if OPTIONS method randomly selected as invalid method In this test, an OPTIONS method request is valid, and should not return a 405 error. * Remove redundant static modifier * Hook the multiple PathTrie attempts into RestHandler.dispatchRequest * Add missing space * Correctly retrieve new handler for each Trie strategy * Only copy headers to threadcontext once * Fix test after REST header copying moved higher up * Restore original params when trying the next trie candidate * Remove OPTIONS for invalidHttpMethodArray so a 405 is guaranteed in tests * Re-add the fix I already added and got removed during merge :-/ * Add missing GET method to test * Add documentation to migration guide about breaking 404 -> 405 changes * Explain boolean response, pull into local var * fixup! Explain boolean response, pull into local var * Encapsulate multiple HTTP methods into PathTrie<MethodHandlers> * Add PathTrie.retrieveAll where all matching modes can be retrieved Then TrieMatchingMode can be package private and not leak into RestController * Include body of error with 405 responses to give hint about valid methods * Fix missing usageService handler addition I accidentally removed this :X * Initialize PathTrieIterator modes with Arrays.asList * Use "== false" instead of ! * Missing paren :-/
diff --git a/core/src/main/java/org/elasticsearch/rest/RestController.java b/core/src/main/java/org/elasticsearch/rest/RestController.java
@@ -56,10 +56,12 @@
 import java.util.function.UnaryOperator;
 
 import static org.elasticsearch.rest.RestStatus.BAD_REQUEST;
+import static org.elasticsearch.rest.RestStatus.METHOD_NOT_ALLOWED;
 import static org.elasticsearch.rest.RestStatus.FORBIDDEN;
 import static org.elasticsearch.rest.RestStatus.INTERNAL_SERVER_ERROR;
 import static org.elasticsearch.rest.RestStatus.NOT_ACCEPTABLE;
 import static org.elasticsearch.rest.RestStatus.OK;
+import static org.elasticsearch.rest.BytesRestResponse.TEXT_CONTENT_TYPE;
 
 public class RestController extends AbstractComponent implements HttpServerTransport.Dispatcher {
 
@@ -141,11 +143,11 @@ public void registerWithDeprecatedHandler(RestRequest.Method method, String path
     }
 
     /**
-     * Registers a REST handler to be executed when the provided method and path match the request.
+     * Registers a REST handler to be executed when one of the provided methods and path match the request.
      *
-     * @param method GET, POST, etc.
      * @param path Path to handle (e.g., "/{index}/{type}/_bulk")
      * @param handler The handler to actually execute
+     * @param method GET, POST, etc.
      */
     public void registerHandler(RestRequest.Method method, String path, RestHandler handler) {
         if (handler instanceof BaseRestHandler) {
@@ -183,11 +185,8 @@ public void dispatchRequest(RestRequest request, RestChannel channel, ThreadCont
     }
 
     @Override
-    public void dispatchBadRequest(
-            final RestRequest request,
-            final RestChannel channel,
-            final ThreadContext threadContext,
-            final Throwable cause) {
+    public void dispatchBadRequest(final RestRequest request, final RestChannel channel,
+                                   final ThreadContext threadContext, final Throwable cause) {
         try {
             final Exception e;
             if (cause == null) {
@@ -211,7 +210,7 @@ public void dispatchBadRequest(
      * Dispatch the request, if possible, returning true if a response was sent or false otherwise.
      */
     boolean dispatchRequest(final RestRequest request, final RestChannel channel, final NodeClient client,
-                            ThreadContext threadContext, final Optional<RestHandler> mHandler) throws Exception {
+                            final Optional<RestHandler> mHandler) throws Exception {
         final int contentLength = request.hasContent() ? request.content().length() : 0;
 
         RestChannel responseChannel = channel;
@@ -228,6 +227,7 @@ boolean dispatchRequest(final RestRequest request, final RestChannel channel, fi
                     "] does not support stream parsing. Use JSON or SMILE instead"));
             requestHandled = true;
         } else if (mHandler.isPresent()) {
+
             try {
                 if (canTripCircuitBreaker(mHandler)) {
                     inFlightRequestsBreaker(circuitBreakerService).addEstimateBytesAndMaybeBreak(contentLength, "<http_request>");
@@ -246,10 +246,19 @@ boolean dispatchRequest(final RestRequest request, final RestChannel channel, fi
                 requestHandled = true;
             }
         } else {
-            if (request.method() == RestRequest.Method.OPTIONS) {
-                // when we have OPTIONS request, simply send OK by default (with the Access Control Origin header which gets automatically added)
-
-                channel.sendResponse(new BytesRestResponse(OK, BytesRestResponse.TEXT_CONTENT_TYPE, BytesArray.EMPTY));
+            // Get the map of matching handlers for a request, for the full set of HTTP methods.
+            final Set<RestRequest.Method> validMethodSet = getValidHandlerMethodSet(request);
+            if (validMethodSet.size() > 0
+                && validMethodSet.contains(request.method()) == false
+                && request.method() != RestRequest.Method.OPTIONS) {
+                // If an alternative handler for an explicit path is registered to a
+                // different HTTP method than the one supplied - return a 405 Method
+                // Not Allowed error.
+                handleUnsupportedHttpMethod(request, channel, validMethodSet);
+                requestHandled = true;
+            } else if (validMethodSet.contains(request.method()) == false
+                && (request.method() == RestRequest.Method.OPTIONS)) {
+                handleOptionsRequest(request, channel, validMethodSet);
                 requestHandled = true;
             } else {
                 requestHandled = false;
@@ -263,9 +272,9 @@ boolean dispatchRequest(final RestRequest request, final RestChannel channel, fi
      * If a request contains content, this method will return {@code true} if the {@code Content-Type} header is present, matches an
      * {@link XContentType} or the handler supports a content stream and the content type header is for newline delimited JSON,
      */
-    private boolean hasContentType(final RestRequest restRequest, final RestHandler restHandler) {
+    private static boolean hasContentType(final RestRequest restRequest, final RestHandler restHandler) {
         if (restRequest.getXContentType() == null) {
-            if (restHandler != null && restHandler.supportsContentStream() && restRequest.header("Content-Type") != null) {
+            if (restHandler.supportsContentStream() && restRequest.header("Content-Type") != null) {
                 final String lowercaseMediaType = restRequest.header("Content-Type").toLowerCase(Locale.ROOT);
                 // we also support newline delimited JSON: http://specs.okfnlabs.org/ndjson/
                 if (lowercaseMediaType.equals("application/x-ndjson")) {
@@ -325,7 +334,7 @@ void tryAllHandlers(final RestRequest request, final RestChannel channel, final
         Iterator<MethodHandlers> allHandlers = getAllHandlers(request);
         for (Iterator<MethodHandlers> it = allHandlers; it.hasNext(); ) {
             final Optional<RestHandler> mHandler = Optional.ofNullable(it.next()).flatMap(mh -> mh.getHandler(request.method()));
-            requestHandled = dispatchRequest(request, channel, client, threadContext, mHandler);
+            requestHandled = dispatchRequest(request, channel, client, mHandler);
             if (requestHandled) {
                 break;
             }
@@ -349,6 +358,47 @@ Iterator<MethodHandlers> getAllHandlers(final RestRequest request) {
         });
     }
 
+    /**
+     * Handle requests to a valid REST endpoint using an unsupported HTTP
+     * method. A 405 HTTP response code is returned, and the response 'Allow'
+     * header includes a list of valid HTTP methods for the endpoint (see
+     * <a href="https://tools.ietf.org/html/rfc2616#section-10.4.6">HTTP/1.1 -
+     * 10.4.6 - 405 Method Not Allowed</a>).
+     */
+    private void handleUnsupportedHttpMethod(RestRequest request, RestChannel channel, Set<RestRequest.Method> validMethodSet) {
+        try {
+            BytesRestResponse bytesRestResponse = BytesRestResponse.createSimpleErrorResponse(channel, METHOD_NOT_ALLOWED,
+                "Incorrect HTTP method for uri [" + request.uri() + "] and method [" + request.method() + "], allowed: " + validMethodSet);
+            bytesRestResponse.addHeader("Allow", Strings.collectionToDelimitedString(validMethodSet, ","));
+            channel.sendResponse(bytesRestResponse);
+        } catch (final IOException e) {
+            logger.warn("failed to send bad request response", e);
+            channel.sendResponse(new BytesRestResponse(INTERNAL_SERVER_ERROR, BytesRestResponse.TEXT_CONTENT_TYPE, BytesArray.EMPTY));
+        }
+    }
+
+    /**
+     * Handle HTTP OPTIONS requests to a valid REST endpoint. A 200 HTTP
+     * response code is returned, and the response 'Allow' header includes a
+     * list of valid HTTP methods for the endpoint (see
+     * <a href="https://tools.ietf.org/html/rfc2616#section-9.2">HTTP/1.1 - 9.2
+     * - Options</a>).
+     */
+    private void handleOptionsRequest(RestRequest request, RestChannel channel, Set<RestRequest.Method> validMethodSet) {
+        if (request.method() == RestRequest.Method.OPTIONS && validMethodSet.size() > 0) {
+            BytesRestResponse bytesRestResponse = new BytesRestResponse(OK, TEXT_CONTENT_TYPE, BytesArray.EMPTY);
+            bytesRestResponse.addHeader("Allow", Strings.collectionToDelimitedString(validMethodSet, ","));
+            channel.sendResponse(bytesRestResponse);
+        } else if (request.method() == RestRequest.Method.OPTIONS && validMethodSet.size() == 0) {
+            /*
+             * When we have an OPTIONS HTTP request and no valid handlers,
+             * simply send OK by default (with the Access Control Origin header
+             * which gets automatically added).
+             */
+            channel.sendResponse(new BytesRestResponse(OK, TEXT_CONTENT_TYPE, BytesArray.EMPTY));
+        }
+    }
+
     /**
      * Handle a requests with no candidate handlers (return a 400 Bad Request
      * error).
diff --git a/core/src/test/java/org/elasticsearch/rest/RestControllerTests.java b/core/src/test/java/org/elasticsearch/rest/RestControllerTests.java
@@ -212,8 +212,7 @@ public void testRestHandlerWrapper() throws Exception {
         final RestController restController = new RestController(Settings.EMPTY, Collections.emptySet(), wrapper, null,
                 circuitBreakerService, usageService);
         final ThreadContext threadContext = new ThreadContext(Settings.EMPTY);
-        restController.dispatchRequest(new FakeRestRequest.Builder(xContentRegistry()).build(),
-            null, null, threadContext, Optional.of(handler));
+        restController.dispatchRequest(new FakeRestRequest.Builder(xContentRegistry()).build(), null, null, Optional.of(handler));
         assertTrue(wrapperCalled.get());
         assertFalse(handlerCalled.get());
     }
diff --git a/core/src/test/java/org/elasticsearch/rest/RestHttpResponseHeadersTests.java b/core/src/test/java/org/elasticsearch/rest/RestHttpResponseHeadersTests.java
@@ -0,0 +1,159 @@
+/*
+ * Licensed to Elasticsearch under one or more contributor
+ * license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright
+ * ownership. Elasticsearch licenses this file to you under
+ * the Apache License, Version 2.0 (the "License"); you may
+ * not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.elasticsearch.rest;
+
+import org.elasticsearch.client.node.NodeClient;
+import org.elasticsearch.common.bytes.BytesReference;
+import org.elasticsearch.common.settings.ClusterSettings;
+import org.elasticsearch.common.settings.Settings;
+import org.elasticsearch.common.util.concurrent.ThreadContext;
+import org.elasticsearch.indices.breaker.CircuitBreakerService;
+import org.elasticsearch.indices.breaker.HierarchyCircuitBreakerService;
+import org.elasticsearch.test.ESTestCase;
+import org.elasticsearch.test.rest.FakeRestChannel;
+import org.elasticsearch.test.rest.FakeRestRequest;
+import org.elasticsearch.usage.UsageService;
+
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+import java.util.stream.Collectors;
+
+import static org.hamcrest.CoreMatchers.notNullValue;
+import static org.hamcrest.Matchers.containsInAnyOrder;
+import static org.hamcrest.Matchers.is;
+import static org.mockito.Mockito.mock;
+
+public class RestHttpResponseHeadersTests extends ESTestCase {
+
+    /**
+     * For requests to a valid REST endpoint using an unsupported HTTP method,
+     * verify that a 405 HTTP response code is returned, and that the response
+     * 'Allow' header includes a list of valid HTTP methods for the endpoint
+     * (see
+     * <a href="https://tools.ietf.org/html/rfc2616#section-10.4.6">HTTP/1.1 -
+     * 10.4.6 - 405 Method Not Allowed</a>).
+     */
+    public void testUnsupportedMethodResponseHttpHeader() throws Exception {
+
+        /*
+         * Generate a random set of candidate valid HTTP methods to register
+         * with the test RestController endpoint. Enums are returned in the
+         * order they are declared, so the first step is to shuffle the HTTP
+         * method list, passing in the RandomizedContext's Random instance,
+         * before picking out a candidate sublist.
+         */
+        List<RestRequest.Method> validHttpMethodArray = new ArrayList<RestRequest.Method>(Arrays.asList(RestRequest.Method.values()));
+        validHttpMethodArray.remove(RestRequest.Method.OPTIONS);
+        Collections.shuffle(validHttpMethodArray, random());
+
+        /*
+         * The upper bound of the potential sublist is one less than the size of
+         * the array, so we are guaranteed at least one invalid method to test.
+         */
+        validHttpMethodArray = validHttpMethodArray.subList(0, randomIntBetween(1, validHttpMethodArray.size() - 1));
+        assert(validHttpMethodArray.size() > 0);
+        assert(validHttpMethodArray.size() < RestRequest.Method.values().length);
+
+        /*
+         * Generate an inverse list of one or more candidate invalid HTTP
+         * methods, so we have a candidate method to fire at the test endpoint.
+         */
+        List<RestRequest.Method> invalidHttpMethodArray = new ArrayList<RestRequest.Method>(Arrays.asList(RestRequest.Method.values()));
+        invalidHttpMethodArray.removeAll(validHttpMethodArray);
+        // Remove OPTIONS, or else we'll get a 200 instead of 405
+        invalidHttpMethodArray.remove(RestRequest.Method.OPTIONS);
+        assert(invalidHttpMethodArray.size() > 0);
+
+        // Initialize test candidate RestController
+        CircuitBreakerService circuitBreakerService = new HierarchyCircuitBreakerService(Settings.EMPTY,
+                new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS));
+
+        final Settings settings = Settings.EMPTY;
+        UsageService usageService = new UsageService(settings);
+        RestController restController = new RestController(settings, Collections.emptySet(),
+                null, null, circuitBreakerService, usageService);
+
+        // A basic RestHandler handles requests to the endpoint
+        RestHandler restHandler = new RestHandler() {
+
+            @Override
+            public void handleRequest(RestRequest request, RestChannel channel, NodeClient client) throws Exception {
+                channel.sendResponse(new TestResponse());
+            }
+
+        };
+
+        // Register valid test handlers with test RestController
+        for (RestRequest.Method method : validHttpMethodArray) {
+            restController.registerHandler(method, "/", restHandler);
+        }
+
+        // Generate a test request with an invalid HTTP method
+        FakeRestRequest.Builder fakeRestRequestBuilder = new FakeRestRequest.Builder(xContentRegistry());
+        fakeRestRequestBuilder.withMethod(invalidHttpMethodArray.get(0));
+        RestRequest restRequest = fakeRestRequestBuilder.build();
+
+        // Send the request and verify the response status code
+        FakeRestChannel restChannel = new FakeRestChannel(restRequest, false, 1);
+        NodeClient client = mock(NodeClient.class);
+        restController.dispatchRequest(restRequest, restChannel, new ThreadContext(Settings.EMPTY));
+        assertThat(restChannel.capturedResponse().status().getStatus(), is(405));
+
+        /*
+         * Verify the response allow header contains the valid methods for the
+         * test endpoint
+         */
+        assertThat(restChannel.capturedResponse().getHeaders().get("Allow"), notNullValue());
+        String responseAllowHeader = restChannel.capturedResponse().getHeaders().get("Allow").get(0);
+        List<String> responseAllowHeaderArray = Arrays.asList(responseAllowHeader.split(","));
+        assertThat(responseAllowHeaderArray.size(), is(validHttpMethodArray.size()));
+        assertThat(responseAllowHeaderArray, containsInAnyOrder(getMethodNameStringArray(validHttpMethodArray).toArray()));
+    }
+
+    private static class TestResponse extends RestResponse {
+
+        @Override
+        public String contentType() {
+            return null;
+        }
+
+        @Override
+        public BytesReference content() {
+            return null;
+        }
+
+        @Override
+        public RestStatus status() {
+            return RestStatus.OK;
+        }
+
+    }
+
+    /**
+     * Convert an RestRequest.Method array to a String array, so it can be
+     * compared with the expected 'Allow' header String array.
+     */
+    private List<String> getMethodNameStringArray(List<RestRequest.Method> methodArray) {
+        return methodArray.stream().map(method -> method.toString()).collect(Collectors.toList());
+    }
+
+}
diff --git a/docs/reference/migration/migrate_6_0/rest.asciidoc b/docs/reference/migration/migrate_6_0/rest.asciidoc
@@ -61,7 +61,7 @@ In previous versions of Elasticsearch, delete by query requests without an expli
 were accepted, match_all was used as the default query and all documents were deleted
 as a result. From version 6.0.0, delete by query requests require an explicit query.
 
-=== DELETE document calls now implicitly create the type
+==== DELETE document calls now implicitly create the type
 
 Running `DELETE index/type/id` now implicitly creates `type` with a default
 mapping if it did not exist yet.
@@ -76,8 +76,39 @@ removed.. `GET /_all` can be used to retrieve all aliases, settings, and
 mappings for all indices. In order to retrieve only the mappings for an index,
 `GET /myindex/_mappings` (or `_aliases`, or `_settings`).
 
+==== Requests to existing endpoints with incorrect HTTP verb now return 405 responses
+
+Issuing a request to an endpoint that exists, but with an incorrect HTTP verb
+(such as a `POST` request to `/myindex/_settings`) now returns an HTTP 405
+response instead of a 404. An `Allow` header is added to the 405 responses
+containing the allowed verbs. For example:
+
+[source,text]
+-------------------------------------------
+$ curl -v -XPOST 'localhost:9200/my_index/_settings'
+*   Trying 127.0.0.1...
+* TCP_NODELAY set
+* Connected to localhost (127.0.0.1) port 9200 (#0)
+> POST /my_index/_settings HTTP/1.1
+> Host: localhost:9200
+> User-Agent: curl/7.51.0
+> Accept: */*
+> 
+< HTTP/1.1 405 Method Not Allowed
+< Allow: PUT,GET
+< content-type: application/json; charset=UTF-8
+< content-length: 134
+< 
+{
+  "error" : "Incorrect HTTP method for uri [/my_index/_settings] and method [POST], allowed: [PUT, GET]",
+  "status" : 405
+}
+* Curl_http_done: called premature == 0
+* Connection #0 to host localhost left intact
+--------------------------------------------
+
 ==== Dissallow using `_cache` and `_cache_key`
 
 The `_cache` and `_cache_key` options in queries have been deprecated since version 2.0.0 and
 have been ignored since then, issuing a deprecation warning. These options have now been completely
-removed, so using them now will throw an error.
+removed, so using them now will throw an error.
diff --git a/qa/smoke-test-http/src/test/java/org/elasticsearch/http/RestHttpResponseHeadersIT.java b/qa/smoke-test-http/src/test/java/org/elasticsearch/http/RestHttpResponseHeadersIT.java
diff --git a/test/framework/src/main/java/org/elasticsearch/test/rest/FakeRestChannel.java b/test/framework/src/main/java/org/elasticsearch/test/rest/FakeRestChannel.java
