From bca07c70680a2b961f07a91a3f153709966e8ce4 Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Tue, 19 Oct 2021 17:29:57 +1100 Subject: [PATCH 1/2] 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 --- .../accesscontrol/IndicesAccessControl.java | 13 +++----- .../authz/permission/IndicesPermission.java | 4 +-- .../IndicesAccessControlTests.java | 17 ++++++++++ .../accesscontrol/IndicesPermissionTests.java | 31 +++++++++++++++++++ 4 files changed, 54 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..44b82c536e11c 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 @@ -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)); + } } 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..43aa30cf500c5 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,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 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) From 0dbcb47a4c5227c5a355ab8406b862dfe67dffc9 Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Tue, 19 Oct 2021 18:36:16 +1100 Subject: [PATCH 2/2] fix for 7.x quirks --- .../IndicesAccessControlTests.java | 3 +-- .../accesscontrol/IndicesPermissionTests.java | 21 ++++++++----------- 2 files changed, 10 insertions(+), 14 deletions(-) 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 44b82c536e11c..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 @@ -16,7 +16,6 @@ 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; @@ -135,7 +134,7 @@ public void testAllowAllIndicesAccessControl() { assertThat(allowAll.getFieldAndDocumentLevelSecurityUsage(), is(IndicesAccessControl.DlsFlsUsage.NONE)); assertThat(allowAll.getIndicesWithFieldOrDocumentLevelSecurity(), emptyIterable()); - final IndicesAccessControl indicesAccessControl = new IndicesAccessControl(randomBoolean(), Map.of()); + 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 43aa30cf500c5..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 @@ -476,27 +476,24 @@ public void testIndicesPermissionHasFieldOrDocumentLevelSecurity() { FieldPermissions.DEFAULT; final Set queries; if (fieldPermissions == FieldPermissions.DEFAULT) { - queries = Set.of(new BytesArray("a query")); + queries = org.elasticsearch.core.Set.of(new BytesArray("a query")); } else { - queries = randomBoolean() ? Set.of(new BytesArray("a query")) : null; + queries = randomBoolean() ? org.elasticsearch.core.Set.of(new BytesArray("a query")) : null; } - final IndicesPermission indicesPermission1 = new IndicesPermission.Builder(RESTRICTED_INDICES_AUTOMATON) - .addGroup(IndexPrivilege.ALL, fieldPermissions, queries, randomBoolean(), "*") - .build(); + 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.Builder(RESTRICTED_INDICES_AUTOMATON) - .addGroup(IndexPrivilege.ALL, FieldPermissions.DEFAULT, null, true, "*") - .build(); + 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.Builder(RESTRICTED_INDICES_AUTOMATON) - .addGroup(IndexPrivilege.ALL, FieldPermissions.DEFAULT, null, true, "*") - .addGroup(IndexPrivilege.NONE, fieldPermissions, queries, randomBoolean(), "*") - .build(); + 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)); }