Always consume the body in has privileges (#50298)

Our REST infrastructure will reject requests that have a body where the
body of the request is never consumed. This ensures that we reject
requests on endpoints that do not support having a body. This requires
cooperation from the REST handlers though, to actually consume the body,
otherwise the REST infrastructure will proceed with rejecting the
request. This commit addresses an issue in the has privileges API where
we would prematurely try to reject a request for not having a username,
before consuming the body. Since the body was not consumed, the REST
infrastructure would instead reject the request as a bad request.

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/rest/action/user/RestHasPrivilegesAction.java

@@ -70,11 +70,15 @@ public class RestHasPrivilegesAction extends SecurityBaseRestHandler {
 
     @Override
     public RestChannelConsumer innerPrepareRequest(RestRequest request, NodeClient client) throws IOException {
+        /*
+         * Consume the body immediately. This ensures that if there is a body and we later reject the request (e.g., because security is not
+         * enabled) that the REST infrastructure will not reject the request for not having consumed the body.
+         */
+        final Tuple<XContentType, BytesReference> content = request.contentOrSourceParam();
         final String username = getUsername(request);
         if (username == null) {
             return restChannel -> { throw new ElasticsearchSecurityException("there is no authenticated user"); };
         }
-        final Tuple<XContentType, BytesReference> content = request.contentOrSourceParam();
         HasPrivilegesRequestBuilder requestBuilder = new SecurityClient(client).prepareHasPrivileges(username, content.v2(), content.v1());
         return channel -> requestBuilder.execute(new RestBuilderListener<HasPrivilegesResponse>(channel) {
             @Override

x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/rest/action/user/RestHasPrivilegesActionTests.java

@@ -6,9 +6,15 @@
 package org.elasticsearch.xpack.security.rest.action.user;
 
 import org.elasticsearch.client.node.NodeClient;
+import org.elasticsearch.common.bytes.BytesArray;
 import org.elasticsearch.common.settings.Settings;
+import org.elasticsearch.common.xcontent.XContentBuilder;
+import org.elasticsearch.common.xcontent.XContentType;
+import org.elasticsearch.common.xcontent.json.JsonXContent;
 import org.elasticsearch.license.XPackLicenseState;
+import org.elasticsearch.rest.RestChannel;
 import org.elasticsearch.rest.RestController;
+import org.elasticsearch.rest.RestRequest;
 import org.elasticsearch.rest.RestStatus;
 import org.elasticsearch.test.ESTestCase;
 import org.elasticsearch.test.rest.FakeRestChannel;
@@ -23,17 +29,45 @@ import static org.mockito.Mockito.when;
 
 public class RestHasPrivilegesActionTests extends ESTestCase {
 
+    /*
+     * Previously we would reject requests that had a body that did not have a username set on the request. This happened because we did not
+     * consume the body until after checking if there was a username set on the request. If there was not a username set on the request,
+     * then the body would never be consumed. This means that the REST infrastructure would reject the request as not having a consumed body
+     * despite the endpoint supporting having a body. Now, we consume the body before checking if there is a username on the request. This
+     * test ensures that we maintain that behavior.
+     */
+    public void testBodyConsumed() throws Exception {
+        final XPackLicenseState licenseState = mock(XPackLicenseState.class);
+        final RestHasPrivilegesAction action =
+            new RestHasPrivilegesAction(Settings.EMPTY, mock(RestController.class), mock(SecurityContext.class), licenseState);
+        try (XContentBuilder bodyBuilder = JsonXContent.contentBuilder().startObject().endObject()) {
+            final RestRequest request = new FakeRestRequest.Builder(xContentRegistry())
+                .withPath("/_security/user/_has_privileges/")
+                .withContent(new BytesArray(bodyBuilder.toString()), XContentType.JSON)
+                .build();
+            final RestChannel channel = new FakeRestChannel(request, true, 1);
+            action.handleRequest(request, channel, mock(NodeClient.class));
+        }
+    }
+
     public void testBasicLicense() throws Exception {
         final XPackLicenseState licenseState = mock(XPackLicenseState.class);
         final RestHasPrivilegesAction action = new RestHasPrivilegesAction(Settings.EMPTY, mock(RestController.class),
             mock(SecurityContext.class), licenseState);
         when(licenseState.isSecurityAvailable()).thenReturn(false);
-        final FakeRestRequest request = new FakeRestRequest();
+        try (XContentBuilder bodyBuilder = JsonXContent.contentBuilder().startObject().endObject()) {
+            final RestRequest request = new FakeRestRequest.Builder(xContentRegistry())
+                .withPath("/_security/user/_has_privileges/")
+                .withContent(new BytesArray(bodyBuilder.toString()), XContentType.JSON)
+                .build();
             final FakeRestChannel channel = new FakeRestChannel(request, true, 1);
             action.handleRequest(request, channel, mock(NodeClient.class));
             assertThat(channel.capturedResponse(), notNullValue());
             assertThat(channel.capturedResponse().status(), equalTo(RestStatus.FORBIDDEN));
-        assertThat(channel.capturedResponse().content().utf8ToString(), containsString("current license is non-compliant for [security]"));
+            assertThat(
+                channel.capturedResponse().content().utf8ToString(),
+                containsString("current license is non-compliant for [security]"));
+        }
     }
 
 }