From ba5032d0418dbeb9e3913f225c0ba53f4973dbc8 Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Tue, 19 Oct 2021 19:36:16 +1100 Subject: [PATCH] More robust and consistent allowAll indicesAccessControl (#79415) (#79427) * 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 * fix for 7.x quirks --- .../accesscontrol/IndicesAccessControl.java | 13 +++------ .../authz/permission/IndicesPermission.java | 4 +-- .../IndicesAccessControlTests.java | 16 +++++++++++ .../accesscontrol/IndicesPermissionTests.java | 28 +++++++++++++++++++ 4 files changed, 50 insertions(+), 11 deletions(-) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/accesscontrol/IndicesAccessControl.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/accesscontrol/IndicesAccessControl.java index 168d2e9300ad1..387591eb598d3 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/accesscontrol/IndicesAccessControl.java +++ b/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 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, org.elasticsearch.core.Map.of()); } @Override @@ -301,13 +301,8 @@ public IndexAccessControl getIndexPermissions(String index) { } @Override - public boolean isGranted() { - return true; - } - - @Override - public Collection getDeniedIndices() { - return org.elasticsearch.core.Set.of(); + public String toString() { + return "AllowAllIndicesAccessControl{}"; } } diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/IndicesPermission.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/IndicesPermission.java index df4b94730ea27..33b5c964a7fb4 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/IndicesPermission.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/IndicesPermission.java @@ -59,8 +59,8 @@ public final class IndicesPermission { public IndicesPermission(Group... groups) { 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()); } private static StringMatcher indexMatcher(Collection ordinaryIndices, Collection restrictedIndices) { diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/accesscontrol/IndicesAccessControlTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/accesscontrol/IndicesAccessControlTests.java index 8de0f3bee78a8..fbe9eba093a0b 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/accesscontrol/IndicesAccessControlTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/accesscontrol/IndicesAccessControlTests.java @@ -18,6 +18,7 @@ import java.util.Collections; 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 +123,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(), org.elasticsearch.core.Map.of()); + assertThat(allowAll.limitIndicesAccessControl(indicesAccessControl), is(indicesAccessControl)); + assertThat(indicesAccessControl.limitIndicesAccessControl(allowAll), is(indicesAccessControl)); + } } diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/accesscontrol/IndicesPermissionTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/accesscontrol/IndicesPermissionTests.java index 1bf8e280741d9..5d58115604bf2 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/accesscontrol/IndicesPermissionTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/accesscontrol/IndicesPermissionTests.java @@ -469,6 +469,34 @@ 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 queries; + if (fieldPermissions == FieldPermissions.DEFAULT) { + queries = org.elasticsearch.core.Set.of(new BytesArray("a query")); + } else { + queries = randomBoolean() ? org.elasticsearch.core.Set.of(new BytesArray("a query")) : null; + } + + final IndicesPermission indicesPermission1 = new IndicesPermission( + new IndicesPermission.Group(IndexPrivilege.ALL, fieldPermissions, queries, randomBoolean(), "*")); + assertThat(indicesPermission1.hasFieldOrDocumentLevelSecurity(), is(true)); + + // IsTotal means no DLS/FLS + final IndicesPermission indicesPermission2 = new IndicesPermission( + new IndicesPermission.Group(IndexPrivilege.ALL, FieldPermissions.DEFAULT, null, true, "*")); + 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( + new IndicesPermission.Group(IndexPrivilege.ALL, FieldPermissions.DEFAULT, null, true, "*"), + new IndicesPermission.Group(IndexPrivilege.NONE, fieldPermissions, queries, randomBoolean(), "*")); + assertThat(indicesPermission3.hasFieldOrDocumentLevelSecurity(), is(false)); + } + private static IndexMetadata createIndexMetadata(String name) { Settings.Builder settingsBuilder = Settings.builder() .put(IndexMetadata.SETTING_VERSION_CREATED, Version.CURRENT)