From 26fb1a699741653368b6d3ed741a09c4f7ffc351 Mon Sep 17 00:00:00 2001
From: Tim Vernum <tim@adjective.org>
Date: Wed, 5 Apr 2017 03:14:20 +1000
Subject: [PATCH] Don't assume FLS arrays are pre-sorted
 (elastic/x-pack-elasticsearch#939)

The `FieldPermissions` class incorrectly assumed that the `granted` and `denied` arrays were
sorted, so it could do a `binarySearch` to see if `_all` was in the arrays.

Original commit: elastic/x-pack-elasticsearch@49b5875602d7ef4704074e1c39e47d765a6ed7b0
---
 .../authz/permission/FieldPermissions.java    |  9 +++++---
 .../permission/FieldPermissionsTests.java     | 23 +++++++++++++++++++
 2 files changed, 29 insertions(+), 3 deletions(-)

diff --git a/plugin/src/main/java/org/elasticsearch/xpack/security/authz/permission/FieldPermissions.java b/plugin/src/main/java/org/elasticsearch/xpack/security/authz/permission/FieldPermissions.java
index b97f78c51b9..8fd4c3a7554 100644
--- a/plugin/src/main/java/org/elasticsearch/xpack/security/authz/permission/FieldPermissions.java
+++ b/plugin/src/main/java/org/elasticsearch/xpack/security/authz/permission/FieldPermissions.java
@@ -153,9 +153,8 @@ public final class FieldPermissions implements Accountable {
                     Strings.arrayToCommaDelimitedString(grantedFields));
         }
 
-        if ((grantedFields == null || Arrays.binarySearch(grantedFields, AllFieldMapper.NAME) < 0) &&
-                (deniedFields == null || Arrays.binarySearch(deniedFields, AllFieldMapper.NAME) < 0)) {
-            // It is not explicitly stated whether _all should be allowed
+        if (!containsAllField(grantedFields) && !containsAllField(deniedFields)) {
+            // It is not explicitly stated whether _all should be allowed/denied
             // In that case we automatically disable _all, unless all fields would match
             if (Operations.isTotal(grantedFieldsAutomaton) && Operations.isEmpty(deniedFieldsAutomaton)) {
                 // all fields are accepted, so using _all is fine
@@ -168,6 +167,10 @@ public final class FieldPermissions implements Accountable {
         return grantedFieldsAutomaton;
     }
 
+    private static boolean containsAllField(String[] fields) {
+        return fields != null && Arrays.stream(fields).anyMatch(AllFieldMapper.NAME::equals);
+    }
+
     /**
      * Returns true if this field permission policy allows access to the field and false if not.
      * fieldName can be a wildcard.
diff --git a/plugin/src/test/java/org/elasticsearch/xpack/security/authz/permission/FieldPermissionsTests.java b/plugin/src/test/java/org/elasticsearch/xpack/security/authz/permission/FieldPermissionsTests.java
index d0fc1590451..079b5802477 100644
--- a/plugin/src/test/java/org/elasticsearch/xpack/security/authz/permission/FieldPermissionsTests.java
+++ b/plugin/src/test/java/org/elasticsearch/xpack/security/authz/permission/FieldPermissionsTests.java
@@ -12,6 +12,8 @@ import org.elasticsearch.test.ESTestCase;
 import org.elasticsearch.xpack.security.authz.RoleDescriptor;
 
 import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
 import java.util.List;
 import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.atomic.AtomicReferenceArray;
@@ -222,4 +224,25 @@ public class FieldPermissionsTests extends ESTestCase {
             assertEquals((Integer) hashCode, hashCodes.get(i));
         }
     }
+
+    public void testAllFieldIsAutomaticallyExcludedIfNotExplicitlyGranted() throws Exception {
+        final FieldPermissions fieldPermissions = new FieldPermissions(
+                new FieldPermissionsDefinition(new String[] { "_a*" }, new String[0]));
+        assertTrue(fieldPermissions.grantsAccessTo("_animal"));
+        assertFalse(fieldPermissions.grantsAccessTo("_all"));
+    }
+
+    public void testAllFieldIsNotExcludedIfExplicitlyGranted() throws Exception {
+        final String[] grant = { "foo", "bar", "baz", "_all" };
+        Collections.shuffle(Arrays.asList(grant), random());
+
+        final FieldPermissions fieldPermissions = new FieldPermissions(
+                new FieldPermissionsDefinition(grant, new String[0]));
+
+        assertTrue(fieldPermissions.grantsAccessTo("_all"));
+        assertTrue(fieldPermissions.grantsAccessTo("foo"));
+        assertTrue(fieldPermissions.grantsAccessTo("bar"));
+        assertTrue(fieldPermissions.grantsAccessTo("baz"));
+        assertFalse(fieldPermissions.grantsAccessTo(randomAlphaOfLengthBetween(5, 8)));
+    }
 }