shield: only wrap readers if the RequestContext can be located

Previously, when the RequestContext could not be located a FieldSubsetReader was returned that only
allowed meta fields to be read. This was done for safety in case there was an API missed so we did not
leak data. However, this causes issues because some requests in elasticsearch execute on a different
thread than the one with the RequestContext so we effectively lose this context and prevent access to
the fields in the document. This is especially problematic with update requests, because that means that
fields that aren't included in the updated document will be lost.

This commit removes the wrapping of the readers in this case and adds tests for bulk updates.

Closes elastic/elasticsearch#938

Original commit: elastic/x-pack-elasticsearch@74c8059da0

shield/src/main/java/org/elasticsearch/shield/authz/accesscontrol/ShieldIndexSearcherWrapper.java

@@ -17,7 +17,6 @@ import org.elasticsearch.common.logging.Loggers;
 import org.elasticsearch.common.logging.support.LoggerMessageFormat;
 import org.elasticsearch.index.IndexSettings;
 import org.elasticsearch.index.cache.bitset.BitsetFilterCache;
-import org.elasticsearch.index.engine.EngineConfig;
 import org.elasticsearch.index.engine.EngineException;
 import org.elasticsearch.index.mapper.DocumentMapper;
 import org.elasticsearch.index.mapper.MapperService;
@@ -84,8 +83,8 @@ public class ShieldIndexSearcherWrapper extends IndexSearcherWrapper {
         try {
             RequestContext context = RequestContext.current();
             if (context == null) {
-                logger.debug("couldn't locate the current request, field level security will only allow meta fields");
+                logger.debug("couldn't locate the current request, field level security will not be applied");
-                return FieldSubsetReader.wrap(reader, allowedMetaFields);
+                return reader;
             }
 
             IndicesAccessControl indicesAccessControl = context.getRequest().getFromContext(InternalAuthorizationService.INDICES_PERMISSIONS_KEY);

shield/src/test/java/org/elasticsearch/integration/BulkUpdateTests.java

@@ -0,0 +1,94 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the Elastic License;
+ * you may not use this file except in compliance with the Elastic License.
+ */
+package org.elasticsearch.integration;
+
+import org.elasticsearch.action.bulk.BulkResponse;
+import org.elasticsearch.action.get.GetResponse;
+import org.elasticsearch.action.update.UpdateResponse;
+import org.elasticsearch.common.settings.Settings;
+import org.elasticsearch.node.Node;
+import org.elasticsearch.shield.authc.support.SecuredString;
+import org.elasticsearch.shield.authc.support.UsernamePasswordToken;
+import org.elasticsearch.test.ShieldIntegTestCase;
+import org.elasticsearch.test.ShieldSettingsSource;
+import org.elasticsearch.test.rest.client.http.HttpResponse;
+
+import java.io.IOException;
+
+import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.equalTo;
+import static org.hamcrest.Matchers.is;
+
+public class BulkUpdateTests extends ShieldIntegTestCase {
+
+    @Override
+    public Settings nodeSettings(int nodeOrdinal) {
+        return Settings.builder()
+                .put(super.nodeSettings(nodeOrdinal))
+                .put(Node.HTTP_ENABLED, true)
+                .build();
+    }
+
+    public void testThatBulkUpdateDoesNotLoseFields() {
+        assertThat(client().prepareIndex("index1", "type").setSource("{\"test\": \"test\"}").setId("1").get().isCreated(), is(true));
+        GetResponse getResponse = internalCluster().transportClient().prepareGet("index1", "type", "1").setFields("test").get();
+        assertThat((String) getResponse.getField("test").getValue(), equalTo("test"));
+
+        if (randomBoolean()) {
+            flushAndRefresh();
+        }
+
+        // update with a new field
+        assertThat(internalCluster().transportClient().prepareUpdate("index1", "type", "1").setDoc("{\"not test\": \"not test\"}").get().isCreated(), is(false));
+        getResponse = internalCluster().transportClient().prepareGet("index1", "type", "1").setFields("test", "not test").get();
+        assertThat((String) getResponse.getField("test").getValue(), equalTo("test"));
+        assertThat((String) getResponse.getField("not test").getValue(), equalTo("not test"));
+
+        // this part is important. Without this, the document may be read from the translog which would bypass the bug where
+        // FLS kicks in because the request can't be found and only returns meta fields
+        flushAndRefresh();
+
+        // do it in a bulk
+        BulkResponse response = internalCluster().transportClient().prepareBulk().add(client().prepareUpdate("index1", "type", "1").setDoc("{\"bulk updated\": \"bulk updated\"}")).get();
+        assertThat(((UpdateResponse)response.getItems()[0].getResponse()).isCreated(), is(false));
+        getResponse = internalCluster().transportClient().prepareGet("index1", "type", "1").setFields("test", "not test", "bulk updated").get();
+        assertThat((String) getResponse.getField("test").getValue(), equalTo("test"));
+        assertThat((String) getResponse.getField("not test").getValue(), equalTo("not test"));
+        assertThat((String) getResponse.getField("bulk updated").getValue(), equalTo("bulk updated"));
+    }
+
+    public void testThatBulkUpdateDoesNotLoseFieldsHttp() throws IOException {
+        final String path = "/index1/type/1";
+        final String basicAuthHeader = UsernamePasswordToken.basicAuthHeaderValue(ShieldSettingsSource.DEFAULT_USER_NAME, new SecuredString(ShieldSettingsSource.DEFAULT_PASSWORD.toCharArray()));
+
+        httpClient().path(path).addHeader("Authorization", basicAuthHeader).method("PUT").body("{\"test\":\"test\"}").execute();
+        HttpResponse response = httpClient().path(path).addHeader("Authorization", basicAuthHeader).method("GET").execute();
+        assertThat(response.getBody(), containsString("\"test\":\"test\""));
+
+        if (randomBoolean()) {
+            flushAndRefresh();
+        }
+
+        //update with new field
+        httpClient().path(path + "/_update").addHeader("Authorization", basicAuthHeader).method("POST").body("{\"doc\": {\"not test\": \"not test\"}}").execute();
+        response = httpClient().path(path).addHeader("Authorization", basicAuthHeader).method("GET").execute();
+        assertThat(response.getBody(), containsString("\"test\":\"test\""));
+        assertThat(response.getBody(), containsString("\"not test\":\"not test\""));
+
+        // this part is important. Without this, the document may be read from the translog which would bypass the bug where
+        // FLS kicks in because the request can't be found and only returns meta fields
+        flushAndRefresh();
+
+        // update with bulk
+        httpClient().path("/_bulk").addHeader("Authorization", basicAuthHeader).method("POST")
+                .body("{\"update\": {\"_index\": \"index1\", \"_type\": \"type\", \"_id\": \"1\"}}\n{\"doc\": {\"bulk updated\":\"bulk updated\"}}\n")
+                .execute();
+        response = httpClient().path(path).addHeader("Authorization", basicAuthHeader).method("GET").execute();
+        assertThat(response.getBody(), containsString("\"test\":\"test\""));
+        assertThat(response.getBody(), containsString("\"not test\":\"not test\""));
+        assertThat(response.getBody(), containsString("\"bulk updated\":\"bulk updated\""));
+    }
+}