Fix DLS query merging when there are groups with/without DLS (elastic/x-pack-elasticsearch#1203)

If a single permission set does not have a query defined then this should be considered as the user
not having document level security for the indices matching that pattern. However, the lack of
document level security was not being taken into account and document level security was being
applied when it should not have been.

Original commit: elastic/x-pack-elasticsearch@f5777c2f37

plugin/src/main/java/org/elasticsearch/xpack/security/authz/permission/IndicesPermission.java

@@ -100,7 +100,7 @@ public final class IndicesPermission implements Iterable<IndicesPermission.Group
 
         SortedMap<String, AliasOrIndex> allAliasesAndIndices = metaData.getAliasAndIndexLookup();
         Map<String, Set<FieldPermissions>> fieldPermissionsByIndex = new HashMap<>();
-        Map<String, Set<BytesReference>> roleQueriesByIndex = new HashMap<>();
+        Map<String, DocumentLevelPermissions> roleQueriesByIndex = new HashMap<>();
         Map<String, Boolean> grantedBuilder = new HashMap<>();
 
         for (String indexOrAlias : requestedIndicesOrAliases) {
@@ -119,9 +119,15 @@ public final class IndicesPermission implements Iterable<IndicesPermission.Group
                     for (String index : concreteIndices) {
                         Set<FieldPermissions> fieldPermissions = fieldPermissionsByIndex.computeIfAbsent(index, (k) -> new HashSet<>());
                         fieldPermissions.add(group.getFieldPermissions());
+                        DocumentLevelPermissions permissions =
+                                roleQueriesByIndex.computeIfAbsent(index, (k) -> new DocumentLevelPermissions());
                         if (group.hasQuery()) {
-                            Set<BytesReference> roleQueries = roleQueriesByIndex.computeIfAbsent(index, (k) -> new HashSet<>());
+                            permissions.addAll(group.getQuery());
-                            roleQueries.addAll(group.getQuery());
+                        } else {
+                            // if more than one permission matches for a concrete index here and if
+                            // a single permission doesn't have a role query then DLS will not be
+                            // applied even when other permissions do have a role query
+                            permissions.setAllowAll(true);
                         }
                     }
                 }
@@ -139,9 +145,12 @@ public final class IndicesPermission implements Iterable<IndicesPermission.Group
         Map<String, IndicesAccessControl.IndexAccessControl> indexPermissions = new HashMap<>();
         for (Map.Entry<String, Boolean> entry : grantedBuilder.entrySet()) {
             String index = entry.getKey();
-            Set<BytesReference> roleQueries = roleQueriesByIndex.get(index);
+            DocumentLevelPermissions permissions = roleQueriesByIndex.get(index);
-            if (roleQueries != null) {
+            final Set<BytesReference> roleQueries;
-                roleQueries = unmodifiableSet(roleQueries);
+            if (permissions != null && permissions.isAllowAll() == false) {
+                roleQueries = unmodifiableSet(permissions.queries);
+            } else {
+                roleQueries = null;
             }
 
             final FieldPermissions fieldPermissions;
@@ -206,4 +215,27 @@ public final class IndicesPermission implements Iterable<IndicesPermission.Group
             return query != null;
         }
     }
+
+    private static class DocumentLevelPermissions {
+
+        private Set<BytesReference> queries = null;
+        private boolean allowAll = false;
+
+        private void addAll(Set<BytesReference> query) {
+            if (allowAll == false) {
+                if (queries == null) {
+                    queries = new HashSet<>();
+                }
+                queries.addAll(query);
+            }
+        }
+
+        private boolean isAllowAll() {
+            return allowAll;
+        }
+
+        private void setAllowAll(boolean allowAll) {
+            this.allowAll = allowAll;
+        }
+    }
 }

plugin/src/test/java/org/elasticsearch/xpack/security/authz/accesscontrol/IndicesPermissionTests.java

@@ -99,6 +99,32 @@ public class IndicesPermissionTests extends ESTestCase {
         assertThat(permissions.getIndexPermissions("_index").getQueries(), equalTo(query));
     }
 
+    public void testAuthorizeMultipleGroupsMixedDls() {
+        IndexMetaData.Builder imbBuilder = IndexMetaData.builder("_index")
+                .settings(Settings.builder()
+                        .put(IndexMetaData.SETTING_NUMBER_OF_SHARDS, 1)
+                        .put(IndexMetaData.SETTING_NUMBER_OF_REPLICAS, 1)
+                        .put(IndexMetaData.SETTING_VERSION_CREATED, Version.CURRENT)
+                )
+                .putAlias(AliasMetaData.builder("_alias"));
+        MetaData md = MetaData.builder().put(imbBuilder).build();
+        FieldPermissionsCache fieldPermissionsCache = new FieldPermissionsCache(Settings.EMPTY);
+
+        Set<BytesReference> query = Collections.singleton(new BytesArray("{}"));
+        String[] fields = new String[]{"_field"};
+        Role role = Role.builder("_role")
+                .add(new FieldPermissions(fieldPermissionDef(fields, null)), query, IndexPrivilege.ALL, "_index")
+                .add(new FieldPermissions(fieldPermissionDef(null, null)), null, IndexPrivilege.ALL, "*")
+                .build();
+        IndicesAccessControl permissions = role.authorize(SearchAction.NAME, Sets.newHashSet("_index"), md, fieldPermissionsCache);
+        assertThat(permissions.getIndexPermissions("_index"), notNullValue());
+        assertTrue(permissions.getIndexPermissions("_index").getFieldPermissions().grantsAccessTo("_field"));
+        assertFalse(permissions.getIndexPermissions("_index").getFieldPermissions().hasFieldLevelSecurity());
+        // null implies that there is no DLS. Currently a index permissions only has queries defined
+        // on it and not a true document level permission object like fields
+        assertNull(permissions.getIndexPermissions("_index").getQueries());
+    }
+
     public void testIndicesPrivilegesStreaming() throws IOException {
         BytesStreamOutput out = new BytesStreamOutput();
         String[] allowed = new String[]{randomAlphaOfLength(5) + "*", randomAlphaOfLength(5) + "*", randomAlphaOfLength(5) + "*"};