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@49b5875602

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) &&
+        if (!containsAllField(grantedFields) && !containsAllField(deniedFields)) {
-                (deniedFields == null || Arrays.binarySearch(deniedFields, AllFieldMapper.NAME) < 0)) {
+            // It is not explicitly stated whether _all should be allowed/denied
-            // It is not explicitly stated whether _all should be allowed
             // 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.

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)));
+    }
 }