More robust and consistent allowAll indicesAccessControl (#79415)

This PR ensures that AllowAllIndicesAccessControl is able to behave well
for all superclass's methods. Previously it throws NPE when it is asked
about Fls/Dls usage because it has a null index permissions map as a
placeholder. In this PR, we also get rid of the null and also mandate
non-null in the constructor of IndicesAccessControl.

In additional, whether a role has DLS/FLS and whether an
AllowAllIndicesAccessControl should be used for short circuit is
determined more consistently. In both places, whether a group has total
access to all indices is used as part of the criteria. Previously it is
possible that the role reports it has DLS/FLS while the
cindicesAccessControl does not have it. This could happen when one of
the group has DLS/FLS but another group has total access to all indices.
In this case, the code now correctly reports no DLS/FLS in both places.

Resolves: #79361

Author: ywangd

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/accesscontrol/IndicesAccessControl.java

@@ -40,7 +40,7 @@ public class IndicesAccessControl {
 
     public IndicesAccessControl(boolean granted, Map<String, IndexAccessControl> indexPermissions) {
         this.granted = granted;
-        this.indexPermissions = indexPermissions;
+        this.indexPermissions = Objects.requireNonNull(indexPermissions);
     }
 
     /**
@@ -292,7 +292,7 @@ private static class AllowAllIndicesAccessControl extends IndicesAccessControl {
         private final IndexAccessControl allowAllIndexAccessControl = new IndexAccessControl(true, null, null);
 
         private AllowAllIndicesAccessControl() {
-            super(true, null);
+            super(true, Map.of());
         }
 
         @Override
@@ -301,13 +301,8 @@ public IndexAccessControl getIndexPermissions(String index) {
         }
 
         @Override
-        public boolean isGranted() {
-            return true;
-        }
-
-        @Override
-        public Collection<?> getDeniedIndices() {
-            return Set.of();
+        public String toString() {
+            return "AllowAllIndicesAccessControl{}";
         }
     }
 

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/IndicesPermission.java

@@ -86,8 +86,8 @@ private IndicesPermission(Automaton restrictedNamesAutomaton, Group[] groups) {
         this.restrictedNamesAutomaton = restrictedNamesAutomaton;
         this.characterRunAutomaton = new CharacterRunAutomaton(restrictedNamesAutomaton);
         this.groups = groups;
-        this.hasFieldOrDocumentLevelSecurity = Arrays.stream(groups)
-            .anyMatch(g -> g.hasQuery() || g.fieldPermissions.hasFieldLevelSecurity());
+        this.hasFieldOrDocumentLevelSecurity = Arrays.stream(groups).noneMatch(Group::isTotal)
+            && Arrays.stream(groups).anyMatch(g -> g.hasQuery() || g.fieldPermissions.hasFieldLevelSecurity());
     }
 
     /**

x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/accesscontrol/IndicesAccessControlTests.java

@@ -16,8 +16,10 @@
 import org.elasticsearch.xpack.core.security.authz.permission.FieldPermissionsDefinition;
 
 import java.util.Collections;
+import java.util.Map;
 import java.util.Set;
 
+import static org.hamcrest.Matchers.emptyIterable;
 import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.is;
 import static org.hamcrest.Matchers.notNullValue;
@@ -122,4 +124,19 @@ public void testSLimitedIndicesAccessControl() {
         assertThat(result.getIndexPermissions("_index").getDocumentPermissions().getQueries(), is(nullValue()));
         assertThat(result.getIndexPermissions("_index").getDocumentPermissions().getLimitedByQueries(), equalTo(queries));
     }
+
+    public void testAllowAllIndicesAccessControl() {
+        final IndicesAccessControl allowAll = IndicesAccessControl.allowAll();
+        final IndexAccessControl indexAccessControl = allowAll.getIndexPermissions(randomAlphaOfLengthBetween(3, 8));
+        assertThat(indexAccessControl.isGranted(), is(true));
+        assertThat(indexAccessControl.getDocumentPermissions(), is(DocumentPermissions.allowAll()));
+        assertThat(indexAccessControl.getFieldPermissions(), is(FieldPermissions.DEFAULT));
+        assertThat(allowAll.getDeniedIndices(), emptyIterable());
+        assertThat(allowAll.getFieldAndDocumentLevelSecurityUsage(), is(IndicesAccessControl.DlsFlsUsage.NONE));
+        assertThat(allowAll.getIndicesWithFieldOrDocumentLevelSecurity(), emptyIterable());
+
+        final IndicesAccessControl indicesAccessControl = new IndicesAccessControl(randomBoolean(), Map.of());
+        assertThat(allowAll.limitIndicesAccessControl(indicesAccessControl), is(indicesAccessControl));
+        assertThat(indicesAccessControl.limitIndicesAccessControl(allowAll), is(indicesAccessControl));
+    }
 }

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

@@ -503,6 +503,37 @@ public void testAuthorizationForMappingUpdates() {
         }
     }
 
+    public void testIndicesPermissionHasFieldOrDocumentLevelSecurity() {
+        // Make sure we have at least one of fieldPermissions and documentPermission
+        final FieldPermissions fieldPermissions = randomBoolean() ?
+            new FieldPermissions(new FieldPermissionsDefinition(Strings.EMPTY_ARRAY, Strings.EMPTY_ARRAY)) :
+            FieldPermissions.DEFAULT;
+        final Set<BytesReference> queries;
+        if (fieldPermissions == FieldPermissions.DEFAULT) {
+            queries = Set.of(new BytesArray("a query"));
+        } else {
+            queries = randomBoolean() ? Set.of(new BytesArray("a query")) : null;
+        }
+
+        final IndicesPermission indicesPermission1 = new IndicesPermission.Builder(RESTRICTED_INDICES_AUTOMATON)
+            .addGroup(IndexPrivilege.ALL, fieldPermissions, queries, randomBoolean(), "*")
+            .build();
+        assertThat(indicesPermission1.hasFieldOrDocumentLevelSecurity(), is(true));
+
+        // IsTotal means no DLS/FLS
+        final IndicesPermission indicesPermission2 = new IndicesPermission.Builder(RESTRICTED_INDICES_AUTOMATON)
+            .addGroup(IndexPrivilege.ALL, FieldPermissions.DEFAULT, null, true, "*")
+            .build();
+        assertThat(indicesPermission2.hasFieldOrDocumentLevelSecurity(), is(false));
+
+        // IsTotal means NO DLS/FLS even when there is another group that has DLS/FLS
+        final IndicesPermission indicesPermission3 = new IndicesPermission.Builder(RESTRICTED_INDICES_AUTOMATON)
+            .addGroup(IndexPrivilege.ALL, FieldPermissions.DEFAULT, null, true, "*")
+            .addGroup(IndexPrivilege.NONE, fieldPermissions, queries, randomBoolean(), "*")
+            .build();
+        assertThat(indicesPermission3.hasFieldOrDocumentLevelSecurity(), is(false));
+    }
+
     private static IndexMetadata createIndexMetadata(String name) {
         Settings.Builder settingsBuilder = Settings.builder()
             .put(IndexMetadata.SETTING_VERSION_CREATED, Version.CURRENT)