From 88149095a3e073f73907b53f1725c008d7a102c3 Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Fri, 28 Oct 2022 12:33:39 +1100 Subject: [PATCH 1/7] wip: production code changes --- .../authz/permission/FieldPermissions.java | 62 ++++++++----------- .../permission/FieldPermissionsCache.java | 19 +++--- .../authz/permission/IndicesPermission.java | 3 +- ...ityIndexReaderWrapperIntegrationTests.java | 6 +- .../FieldPermissionsCacheTests.java | 14 ++--- .../permission/FieldPermissionsTests.java | 8 +-- .../xpack/security/authz/RBACEngine.java | 8 ++- .../IndicesAccessControlTests.java | 16 ++--- .../accesscontrol/IndicesPermissionTests.java | 22 +++---- .../accesscontrol/OptOutQueryCacheTests.java | 2 +- ...IndicesAliasesRequestInterceptorTests.java | 2 +- .../ResizeRequestInterceptorTests.java | 2 +- 12 files changed, 77 insertions(+), 87 deletions(-) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/FieldPermissions.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/FieldPermissions.java index bc665e301f095..d7cdba1eb2be9 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/FieldPermissions.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/FieldPermissions.java @@ -18,7 +18,7 @@ import org.elasticsearch.common.Strings; import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.common.regex.Regex; -import org.elasticsearch.core.Nullable; +import org.elasticsearch.common.util.CollectionUtils; import org.elasticsearch.xpack.core.security.authz.accesscontrol.FieldSubsetReader; import org.elasticsearch.xpack.core.security.authz.permission.FieldPermissionsDefinition.FieldGrantExcludeGroup; import org.elasticsearch.xpack.core.security.authz.support.SecurityQueryTemplateEvaluator.DlsQueryEvaluationContext; @@ -61,9 +61,8 @@ public final class FieldPermissions implements Accountable, CacheKey { BASE_HASHSET_ENTRY_SIZE = mapEntryShallowSize + 2 * RamUsageEstimator.NUM_BYTES_OBJECT_REF; } - private final FieldPermissionsDefinition fieldPermissionsDefinition; - @Nullable - private final FieldPermissionsDefinition limitedByFieldPermissionsDefinition; + private final List fieldPermissionsDefinitions; + // an automaton that represents a union of one more sets of permitted and denied fields private final CharacterRunAutomaton permittedFieldsAutomaton; private final boolean permittedFieldsAutomatonIsTotal; @@ -72,10 +71,11 @@ public final class FieldPermissions implements Accountable, CacheKey { private final long ramBytesUsed; /** Constructor that does not enable field-level security: all fields are accepted. */ - public FieldPermissions() { + private FieldPermissions() { this(new FieldPermissionsDefinition(null, null), Automatons.MATCH_ALL); } + // TODO: This constructor is only used by tests. We should remove it in future /** Constructor that enables field-level security based on include/exclude rules. Exclude rules * have precedence over include rules. */ public FieldPermissions(FieldPermissionsDefinition fieldPermissionsDefinition) { @@ -85,33 +85,30 @@ public FieldPermissions(FieldPermissionsDefinition fieldPermissionsDefinition) { /** Constructor that enables field-level security based on include/exclude rules. Exclude rules * have precedence over include rules. */ FieldPermissions(FieldPermissionsDefinition fieldPermissionsDefinition, Automaton permittedFieldsAutomaton) { - this(fieldPermissionsDefinition, null, permittedFieldsAutomaton); + this( + List.of(Objects.requireNonNull(fieldPermissionsDefinition, "field permission definition cannot be null")), + permittedFieldsAutomaton + ); } /** Constructor that enables field-level security based on include/exclude rules. Exclude rules * have precedence over include rules. */ - private FieldPermissions( - FieldPermissionsDefinition fieldPermissionsDefinition, - @Nullable FieldPermissionsDefinition limitedByFieldPermissionsDefinition, - Automaton permittedFieldsAutomaton - ) { + private FieldPermissions(List fieldPermissionsDefinitions, Automaton permittedFieldsAutomaton) { if (permittedFieldsAutomaton.isDeterministic() == false && permittedFieldsAutomaton.getNumStates() > 1) { // we only accept deterministic automata so that the CharacterRunAutomaton constructor // directly wraps the provided automaton throw new IllegalArgumentException("Only accepts deterministic automata"); } - this.fieldPermissionsDefinition = Objects.requireNonNull(fieldPermissionsDefinition, "field permission definition cannot be null"); - this.limitedByFieldPermissionsDefinition = limitedByFieldPermissionsDefinition; + this.fieldPermissionsDefinitions = fieldPermissionsDefinitions; this.originalAutomaton = permittedFieldsAutomaton; this.permittedFieldsAutomaton = new CharacterRunAutomaton(permittedFieldsAutomaton); // we cache the result of isTotal since this might be a costly operation this.permittedFieldsAutomatonIsTotal = Operations.isTotal(permittedFieldsAutomaton); long ramBytesUsed = BASE_FIELD_PERM_DEF_BYTES; - ramBytesUsed += ramBytesUsedForFieldPermissionsDefinition(this.fieldPermissionsDefinition); - if (this.limitedByFieldPermissionsDefinition != null) { - ramBytesUsed += ramBytesUsedForFieldPermissionsDefinition(this.limitedByFieldPermissionsDefinition); - } + ramBytesUsed += this.fieldPermissionsDefinitions.stream() + .mapToLong(FieldPermissions::ramBytesUsedForFieldPermissionsDefinition) + .sum(); ramBytesUsed += permittedFieldsAutomaton.ramBytesUsed(); ramBytesUsed += runAutomatonRamBytesUsed(permittedFieldsAutomaton); this.ramBytesUsed = ramBytesUsed; @@ -199,12 +196,16 @@ public static Automaton buildPermittedFieldsAutomaton(final String[] grantedFiel */ public FieldPermissions limitFieldPermissions(FieldPermissions limitedBy) { if (hasFieldLevelSecurity() && limitedBy != null && limitedBy.hasFieldLevelSecurity()) { + // TODO: the automaton computation is not cached Automaton _permittedFieldsAutomaton = Automatons.intersectAndMinimize(getIncludeAutomaton(), limitedBy.getIncludeAutomaton()); - return new FieldPermissions(fieldPermissionsDefinition, limitedBy.fieldPermissionsDefinition, _permittedFieldsAutomaton); + return new FieldPermissions( + CollectionUtils.concatLists(fieldPermissionsDefinitions, limitedBy.fieldPermissionsDefinitions), + _permittedFieldsAutomaton + ); } else if (limitedBy != null && limitedBy.hasFieldLevelSecurity()) { - return new FieldPermissions(limitedBy.getFieldPermissionsDefinition(), limitedBy.getIncludeAutomaton()); + return new FieldPermissions(limitedBy.fieldPermissionsDefinitions, limitedBy.getIncludeAutomaton()); } else if (hasFieldLevelSecurity()) { - return new FieldPermissions(this.getFieldPermissionsDefinition(), getIncludeAutomaton()); + return new FieldPermissions(fieldPermissionsDefinitions, getIncludeAutomaton()); } return FieldPermissions.DEFAULT; } @@ -217,23 +218,13 @@ public boolean grantsAccessTo(String fieldName) { return permittedFieldsAutomatonIsTotal || permittedFieldsAutomaton.run(fieldName); } - public FieldPermissionsDefinition getFieldPermissionsDefinition() { - return fieldPermissionsDefinition; - } - - public FieldPermissionsDefinition getLimitedByFieldPermissionsDefinition() { - return limitedByFieldPermissionsDefinition; + public List getFieldPermissionsDefinitions() { + return fieldPermissionsDefinitions; } @Override public void buildCacheKey(StreamOutput out, DlsQueryEvaluationContext context) throws IOException { - fieldPermissionsDefinition.buildCacheKey(out, context); - if (limitedByFieldPermissionsDefinition != null) { - out.writeBoolean(true); - limitedByFieldPermissionsDefinition.buildCacheKey(out, context); - } else { - out.writeBoolean(false); - } + out.writeCollection(fieldPermissionsDefinitions, (o, fpd) -> fpd.buildCacheKey(o, context)); } /** Return whether field-level security is enabled, ie. whether any field might be filtered out. */ @@ -259,13 +250,12 @@ public boolean equals(Object o) { if (o == null || getClass() != o.getClass()) return false; FieldPermissions that = (FieldPermissions) o; return permittedFieldsAutomatonIsTotal == that.permittedFieldsAutomatonIsTotal - && fieldPermissionsDefinition.equals(that.fieldPermissionsDefinition) - && Objects.equals(limitedByFieldPermissionsDefinition, that.limitedByFieldPermissionsDefinition); + && fieldPermissionsDefinitions.equals(that.fieldPermissionsDefinitions); } @Override public int hashCode() { - return Objects.hash(fieldPermissionsDefinition, limitedByFieldPermissionsDefinition, permittedFieldsAutomatonIsTotal); + return Objects.hash(fieldPermissionsDefinitions, permittedFieldsAutomatonIsTotal); } @Override diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/FieldPermissionsCache.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/FieldPermissionsCache.java index 2845120c5246a..51bc57bc934bd 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/FieldPermissionsCache.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/FieldPermissionsCache.java @@ -72,26 +72,25 @@ public FieldPermissions getFieldPermissions(FieldPermissionsDefinition fieldPerm } /** - * Returns a field permissions object that corresponds to the merging of the given field permissions and caches the instance if one was - * not found in the cache. + * Returns a field permissions object that corresponds to the union of the given field permissions. + * Union means a field is granted if it is granted by any of the FieldPermissions from the given + * collection. + * The returned instance is cached if one was not found in the cache. */ - FieldPermissions getFieldPermissions(Collection fieldPermissionsCollection) { + FieldPermissions union(Collection fieldPermissionsCollection) { Optional allowAllFieldPermissions = fieldPermissionsCollection.stream() .filter(((Predicate) (FieldPermissions::hasFieldLevelSecurity)).negate()) .findFirst(); return allowAllFieldPermissions.orElseGet(() -> { final Set fieldGrantExcludeGroups = new HashSet<>(); for (FieldPermissions fieldPermissions : fieldPermissionsCollection) { - final FieldPermissionsDefinition definition = fieldPermissions.getFieldPermissionsDefinition(); - final FieldPermissionsDefinition limitedByDefinition = fieldPermissions.getLimitedByFieldPermissionsDefinition(); - if (definition == null) { - throw new IllegalArgumentException("Expected field permission definition, but found null"); - } else if (limitedByDefinition != null) { + final List fieldPermissionsDefinitions = fieldPermissions.getFieldPermissionsDefinitions(); + if (fieldPermissionsDefinitions.size() != 1) { throw new IllegalArgumentException( - "Expected no limited-by field permission definition, but found [" + limitedByDefinition + "]" + "Expected a single field permission definition, but found [" + fieldPermissionsDefinitions + "]" ); } - fieldGrantExcludeGroups.addAll(definition.getFieldGrantExcludeGroups()); + fieldGrantExcludeGroups.addAll(fieldPermissionsDefinitions.get(0).getFieldGrantExcludeGroups()); } final FieldPermissionsDefinition combined = new FieldPermissionsDefinition(fieldGrantExcludeGroups); try { 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 3782159af8697..806ff095d5697 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 @@ -494,7 +494,7 @@ && containsPrivilegeThatGrantsMappingUpdatesForBwc(group))) { if (indexFieldPermissions != null && indexFieldPermissions.isEmpty() == false) { fieldPermissions = indexFieldPermissions.size() == 1 ? indexFieldPermissions.iterator().next() - : fieldPermissionsCache.getFieldPermissions(indexFieldPermissions); + : fieldPermissionsCache.union(indexFieldPermissions); } else { fieldPermissions = FieldPermissions.DEFAULT; } @@ -609,6 +609,7 @@ public static class Group { private final String[] indices; private final StringMatcher indexNameMatcher; private final Supplier indexNameAutomaton; + // TODO: Use FieldPermissionsDefinition instead of FieldPermissions. The former is more suitable to this layer (user input) private final FieldPermissions fieldPermissions; private final Set query; // by default certain restricted indices are exempted when granting privileges, as they should generally be hidden for ordinary diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/accesscontrol/SecurityIndexReaderWrapperIntegrationTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/accesscontrol/SecurityIndexReaderWrapperIntegrationTests.java index 1796431afdd2c..6342f573a838c 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/accesscontrol/SecurityIndexReaderWrapperIntegrationTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/accesscontrol/SecurityIndexReaderWrapperIntegrationTests.java @@ -159,7 +159,7 @@ public void testDLS() throws Exception { for (int i = 0; i < numValues; i++) { String termQuery = "{\"term\": {\"field\": \"" + values[i] + "\"} }"; IndicesAccessControl.IndexAccessControl indexAccessControl = new IndicesAccessControl.IndexAccessControl( - new FieldPermissions(), + FieldPermissions.DEFAULT, DocumentPermissions.filteredBy(singleton(new BytesArray(termQuery))) ); SecurityIndexReaderWrapper wrapper = new SecurityIndexReaderWrapper( @@ -224,7 +224,7 @@ public void testDLSWithLimitedPermissions() throws Exception { queries.add(new BytesArray("{\"terms\" : { \"f2\" : [\"fv22\"] } }")); queries.add(new BytesArray("{\"terms\" : { \"f2\" : [\"fv32\"] } }")); IndicesAccessControl.IndexAccessControl indexAccessControl = new IndicesAccessControl.IndexAccessControl( - new FieldPermissions(), + FieldPermissions.DEFAULT, DocumentPermissions.filteredBy(queries) ); queries = singleton(new BytesArray("{\"terms\" : { \"f1\" : [\"fv11\", \"fv21\", \"fv31\"] } }")); @@ -232,7 +232,7 @@ public void testDLSWithLimitedPermissions() throws Exception { queries = singleton(new BytesArray("{\"terms\" : { \"f1\" : [\"fv11\", \"fv31\"] } }")); } IndicesAccessControl.IndexAccessControl limitedIndexAccessControl = new IndicesAccessControl.IndexAccessControl( - new FieldPermissions(), + FieldPermissions.DEFAULT, DocumentPermissions.filteredBy(queries) ); IndexSettings indexSettings = IndexSettingsModule.newIndexSettings( diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/permission/FieldPermissionsCacheTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/permission/FieldPermissionsCacheTests.java index 4a3ecab2b3d0c..d5429ff62b2c3 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/permission/FieldPermissionsCacheTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/permission/FieldPermissionsCacheTests.java @@ -44,9 +44,7 @@ public void testMergeFieldPermissions() { FieldPermissions fieldPermissions2 = randomBoolean() ? fieldPermissionsCache.getFieldPermissions(allowed2, denied2) : new FieldPermissions(fieldPermissionDef(allowed2, denied2)); - FieldPermissions mergedFieldPermissions = fieldPermissionsCache.getFieldPermissions( - Arrays.asList(fieldPermissions1, fieldPermissions2) - ); + FieldPermissions mergedFieldPermissions = fieldPermissionsCache.union(Arrays.asList(fieldPermissions1, fieldPermissions2)); assertTrue(mergedFieldPermissions.grantsAccessTo(allowedPrefix1 + "b")); assertTrue(mergedFieldPermissions.grantsAccessTo(allowedPrefix2 + "b")); assertFalse(mergedFieldPermissions.grantsAccessTo(denied1[0])); @@ -62,7 +60,7 @@ public void testMergeFieldPermissions() { fieldPermissions2 = randomBoolean() ? fieldPermissionsCache.getFieldPermissions(allowed2, denied2) : new FieldPermissions(fieldPermissionDef(allowed2, denied2)); - mergedFieldPermissions = fieldPermissionsCache.getFieldPermissions(Arrays.asList(fieldPermissions1, fieldPermissions2)); + mergedFieldPermissions = fieldPermissionsCache.union(Arrays.asList(fieldPermissions1, fieldPermissions2)); assertFalse(mergedFieldPermissions.hasFieldLevelSecurity()); allowed1 = new String[] {}; @@ -75,7 +73,7 @@ public void testMergeFieldPermissions() { fieldPermissions2 = randomBoolean() ? fieldPermissionsCache.getFieldPermissions(allowed2, denied2) : new FieldPermissions(fieldPermissionDef(allowed2, denied2)); - mergedFieldPermissions = fieldPermissionsCache.getFieldPermissions(Arrays.asList(fieldPermissions1, fieldPermissions2)); + mergedFieldPermissions = fieldPermissionsCache.union(Arrays.asList(fieldPermissions1, fieldPermissions2)); for (String field : allowed2) { assertTrue(mergedFieldPermissions.grantsAccessTo(field)); } @@ -93,7 +91,7 @@ public void testMergeFieldPermissions() { fieldPermissions2 = randomBoolean() ? fieldPermissionsCache.getFieldPermissions(allowed2, denied2) : new FieldPermissions(fieldPermissionDef(allowed2, denied2)); - mergedFieldPermissions = fieldPermissionsCache.getFieldPermissions(Arrays.asList(fieldPermissions1, fieldPermissions2)); + mergedFieldPermissions = fieldPermissionsCache.union(Arrays.asList(fieldPermissions1, fieldPermissions2)); assertTrue(mergedFieldPermissions.grantsAccessTo("a")); assertTrue(mergedFieldPermissions.grantsAccessTo("b")); @@ -107,7 +105,7 @@ public void testMergeFieldPermissions() { fieldPermissions2 = randomBoolean() ? fieldPermissionsCache.getFieldPermissions(allowed2, denied2) : new FieldPermissions(fieldPermissionDef(allowed2, denied2)); - mergedFieldPermissions = fieldPermissionsCache.getFieldPermissions(Arrays.asList(fieldPermissions1, fieldPermissions2)); + mergedFieldPermissions = fieldPermissionsCache.union(Arrays.asList(fieldPermissions1, fieldPermissions2)); assertTrue(mergedFieldPermissions.grantsAccessTo("a")); assertTrue(mergedFieldPermissions.grantsAccessTo("b")); assertFalse(mergedFieldPermissions.grantsAccessTo("aa")); @@ -126,7 +124,7 @@ public void testNonFlsAndFlsMerging() { FieldPermissionsCache cache = new FieldPermissionsCache(Settings.EMPTY); for (int i = 0; i < scaledRandomIntBetween(1, 12); i++) { Collections.shuffle(permissionsList, random()); - FieldPermissions result = cache.getFieldPermissions(permissionsList); + FieldPermissions result = cache.union(permissionsList); assertFalse(result.hasFieldLevelSecurity()); } } diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/permission/FieldPermissionsTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/permission/FieldPermissionsTests.java index 70709525f9951..ea1201413879e 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/permission/FieldPermissionsTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/permission/FieldPermissionsTests.java @@ -36,13 +36,13 @@ public void testFieldPermissionsIntersection() { ); { - FieldPermissions result = fieldPermissions.limitFieldPermissions(randomFrom(new FieldPermissions(), null)); + FieldPermissions result = fieldPermissions.limitFieldPermissions(randomFrom(FieldPermissions.DEFAULT, null)); assertThat(result, is(notNullValue())); assertThat(result, IsSame.sameInstance(FieldPermissions.DEFAULT)); } { - FieldPermissions result = fieldPermissions1.limitFieldPermissions(new FieldPermissions()); + FieldPermissions result = fieldPermissions1.limitFieldPermissions(FieldPermissions.DEFAULT); assertThat(result, is(notNullValue())); assertThat(result, not(same(fieldPermissions))); assertThat(result, not(same(fieldPermissions1))); @@ -83,7 +83,7 @@ public void testFieldPermissionsIntersection() { } public void testMustHaveNonNullFieldPermissionsDefinition() { - final FieldPermissions fieldPermissions0 = new FieldPermissions(); + final FieldPermissions fieldPermissions0 = FieldPermissions.DEFAULT; assertThat(fieldPermissions0.getFieldPermissionsDefinition(), notNullValue()); expectThrows(NullPointerException.class, () -> new FieldPermissions(null)); expectThrows(NullPointerException.class, () -> new FieldPermissions(null, Automatons.MATCH_ALL)); @@ -154,7 +154,7 @@ public void testWriteCacheKeyWillDistinguishBetweenDefinitionAndLimitedByDefinit // Just limited by final BytesStreamOutput out3 = new BytesStreamOutput(); - final FieldPermissions fieldPermissions3 = new FieldPermissions().limitFieldPermissions( + final FieldPermissions fieldPermissions3 = FieldPermissions.DEFAULT.limitFieldPermissions( new FieldPermissions( new FieldPermissionsDefinition( Set.of( diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/RBACEngine.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/RBACEngine.java index 54d174ea6bf10..d7be0475514a5 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/RBACEngine.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/RBACEngine.java @@ -724,9 +724,11 @@ static GetUserPrivilegesResponse buildUserPrivilegesResponseObject(Role userRole final Set queries = group.getQuery() == null ? Collections.emptySet() : group.getQuery(); final Set fieldSecurity; if (group.getFieldPermissions().hasFieldLevelSecurity()) { - final FieldPermissionsDefinition definition = group.getFieldPermissions().getFieldPermissionsDefinition(); - assert group.getFieldPermissions().getLimitedByFieldPermissionsDefinition() == null - : "limited-by field must not exist since we do not support reporting user privileges for limited roles"; + final List fieldPermissionsDefinitions = group.getFieldPermissions() + .getFieldPermissionsDefinitions(); + assert fieldPermissionsDefinitions.size() == 1 + : "imited-by field must not exist since we do not support reporting user privileges for limited roles"; + final FieldPermissionsDefinition definition = fieldPermissionsDefinitions.get(0); fieldSecurity = definition.getFieldGrantExcludeGroups(); } else { fieldSecurity = Collections.emptySet(); 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 9d3b7dbe20507..87600410a640e 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 @@ -83,7 +83,7 @@ public void testLimitedIndicesAccessControl() { indicesAccessControl = new IndicesAccessControl( true, - Collections.singletonMap("_index", new IndexAccessControl(new FieldPermissions(), DocumentPermissions.allowAll())) + Collections.singletonMap("_index", new IndexAccessControl(FieldPermissions.DEFAULT, DocumentPermissions.allowAll())) ); limitedByIndicesAccessControl = new IndicesAccessControl(true, Collections.emptyMap()); result = indicesAccessControl.limitIndicesAccessControl(limitedByIndicesAccessControl); @@ -95,11 +95,11 @@ public void testLimitedIndicesAccessControl() { indicesAccessControl = new IndicesAccessControl( true, - Collections.singletonMap("_index", new IndexAccessControl(new FieldPermissions(), DocumentPermissions.allowAll())) + Collections.singletonMap("_index", new IndexAccessControl(FieldPermissions.DEFAULT, DocumentPermissions.allowAll())) ); limitedByIndicesAccessControl = new IndicesAccessControl( true, - Collections.singletonMap("_index", new IndexAccessControl(new FieldPermissions(), DocumentPermissions.allowAll())) + Collections.singletonMap("_index", new IndexAccessControl(FieldPermissions.DEFAULT, DocumentPermissions.allowAll())) ); result = indicesAccessControl.limitIndicesAccessControl(limitedByIndicesAccessControl); assertThat(result, is(notNullValue())); @@ -120,7 +120,7 @@ public void testLimitedIndicesAccessControl() { true, Map.ofEntries( Map.entry("_index", new IndexAccessControl(fieldPermissions1, DocumentPermissions.allowAll())), - Map.entry("another-index", new IndexAccessControl(new FieldPermissions(), DocumentPermissions.allowAll())) + Map.entry("another-index", new IndexAccessControl(FieldPermissions.DEFAULT, DocumentPermissions.allowAll())) ) ); limitedByIndicesAccessControl = new IndicesAccessControl( @@ -159,15 +159,15 @@ public void testLimitedIndicesAccessControl() { indicesAccessControl = new IndicesAccessControl( true, Map.ofEntries( - Map.entry("_index", new IndexAccessControl(new FieldPermissions(), DocumentPermissions.allowAll())), - Map.entry("another-index", new IndexAccessControl(new FieldPermissions(), documentPermissions2)) + Map.entry("_index", new IndexAccessControl(FieldPermissions.DEFAULT, DocumentPermissions.allowAll())), + Map.entry("another-index", new IndexAccessControl(FieldPermissions.DEFAULT, documentPermissions2)) ) ); limitedByIndicesAccessControl = new IndicesAccessControl( true, Map.ofEntries( - Map.entry("_index", new IndexAccessControl(new FieldPermissions(), documentPermissions1)), - Map.entry("another-index", new IndexAccessControl(new FieldPermissions(), DocumentPermissions.allowAll())) + Map.entry("_index", new IndexAccessControl(FieldPermissions.DEFAULT, documentPermissions1)), + Map.entry("another-index", new IndexAccessControl(FieldPermissions.DEFAULT, DocumentPermissions.allowAll())) ) ); result = indicesAccessControl.limitIndicesAccessControl(limitedByIndicesAccessControl); 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 18ef6a14e1513..48dcbc349b48c 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 @@ -95,7 +95,7 @@ public void testAuthorize() { // no field level security: role = Role.builder(RESTRICTED_INDICES, "_role") - .add(new FieldPermissions(), query, IndexPrivilege.ALL, randomBoolean(), "_index") + .add(FieldPermissions.DEFAULT, query, IndexPrivilege.ALL, randomBoolean(), "_index") .build(); permissions = role.authorize(SearchAction.NAME, Sets.newHashSet("_index"), lookup, fieldPermissionsCache); assertThat(permissions.getIndexPermissions("_index"), notNullValue()); @@ -258,7 +258,7 @@ public void testCorePermissionAuthorize() { FieldPermissionsCache fieldPermissionsCache = new FieldPermissionsCache(Settings.EMPTY); IndicesPermission core = new IndicesPermission.Builder(RESTRICTED_INDICES).addGroup( IndexPrivilege.ALL, - new FieldPermissions(), + FieldPermissions.DEFAULT, null, randomBoolean(), "a1" @@ -285,7 +285,7 @@ public void testCorePermissionAuthorize() { // test with two indices core = new IndicesPermission.Builder(RESTRICTED_INDICES).addGroup( IndexPrivilege.ALL, - new FieldPermissions(), + FieldPermissions.DEFAULT, null, randomBoolean(), "a1" @@ -337,7 +337,7 @@ public void testErrorMessageIfIndexPatternIsTooComplex() { ElasticsearchSecurityException.class, () -> new IndicesPermission.Group( IndexPrivilege.ALL, - new FieldPermissions(), + FieldPermissions.DEFAULT, null, randomBoolean(), RESTRICTED_INDICES, @@ -368,7 +368,7 @@ public void testSecurityIndicesPermissions() { // allow_restricted_indices: false IndicesPermission indicesPermission = new IndicesPermission.Builder(RESTRICTED_INDICES).addGroup( IndexPrivilege.ALL, - new FieldPermissions(), + FieldPermissions.DEFAULT, null, false, "*" @@ -388,7 +388,7 @@ public void testSecurityIndicesPermissions() { // allow_restricted_indices: true indicesPermission = new IndicesPermission.Builder(RESTRICTED_INDICES).addGroup( IndexPrivilege.ALL, - new FieldPermissions(), + FieldPermissions.DEFAULT, null, true, "*" @@ -419,7 +419,7 @@ public void testAsyncSearchIndicesPermissions() { // allow_restricted_indices: false IndicesPermission indicesPermission = new IndicesPermission.Builder(RESTRICTED_INDICES).addGroup( IndexPrivilege.ALL, - new FieldPermissions(), + FieldPermissions.DEFAULT, null, false, "*" @@ -437,7 +437,7 @@ public void testAsyncSearchIndicesPermissions() { // allow_restricted_indices: true indicesPermission = new IndicesPermission.Builder(RESTRICTED_INDICES).addGroup( IndexPrivilege.ALL, - new FieldPermissions(), + FieldPermissions.DEFAULT, null, true, "*" @@ -470,7 +470,7 @@ public void testAuthorizationForBackingIndices() { SortedMap lookup = metadata.getIndicesLookup(); IndicesPermission indicesPermission = new IndicesPermission.Builder(RESTRICTED_INDICES).addGroup( IndexPrivilege.READ, - new FieldPermissions(), + FieldPermissions.DEFAULT, null, false, dataStreamName @@ -490,7 +490,7 @@ public void testAuthorizationForBackingIndices() { indicesPermission = new IndicesPermission.Builder(RESTRICTED_INDICES).addGroup( IndexPrivilege.CREATE_DOC, - new FieldPermissions(), + FieldPermissions.DEFAULT, null, false, dataStreamName @@ -535,7 +535,7 @@ public void testAuthorizationForMappingUpdates() { FieldPermissionsCache fieldPermissionsCache = new FieldPermissionsCache(Settings.EMPTY); IndicesPermission core = new IndicesPermission.Builder(RESTRICTED_INDICES).addGroup( IndexPrivilege.INDEX, - new FieldPermissions(), + FieldPermissions.DEFAULT, null, randomBoolean(), "test*" diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/accesscontrol/OptOutQueryCacheTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/accesscontrol/OptOutQueryCacheTests.java index a3828b8f957f5..102fa69ab6a82 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/accesscontrol/OptOutQueryCacheTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/accesscontrol/OptOutQueryCacheTests.java @@ -174,7 +174,7 @@ public void testOptOutQueryCacheIndexDoesNotHaveFieldLevelSecurity() { final IndicesQueryCache indicesQueryCache = mock(IndicesQueryCache.class); final ThreadContext threadContext = new ThreadContext(Settings.EMPTY); final IndicesAccessControl.IndexAccessControl indexAccessControl = mock(IndicesAccessControl.IndexAccessControl.class); - when(indexAccessControl.getFieldPermissions()).thenReturn(new FieldPermissions()); + when(indexAccessControl.getFieldPermissions()).thenReturn(FieldPermissions.DEFAULT); final IndicesAccessControl indicesAccessControl = mock(IndicesAccessControl.class); when(indicesAccessControl.getIndexPermissions("index")).thenReturn(indexAccessControl); when(indicesAccessControl.isGranted()).thenReturn(true); diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/interceptor/IndicesAliasesRequestInterceptorTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/interceptor/IndicesAliasesRequestInterceptorTests.java index 2c64cdbd53795..1c94f6e9018de 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/interceptor/IndicesAliasesRequestInterceptorTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/interceptor/IndicesAliasesRequestInterceptorTests.java @@ -68,7 +68,7 @@ public void testInterceptorThrowsWhenFLSDLSEnabled() { if (useFls) { fieldPermissions = new FieldPermissions(new FieldPermissionsDefinition(new String[] { "foo" }, null)); } else { - fieldPermissions = new FieldPermissions(); + fieldPermissions = FieldPermissions.DEFAULT; } final boolean useDls = (useFls == false) || randomBoolean(); final Set queries; diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/interceptor/ResizeRequestInterceptorTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/interceptor/ResizeRequestInterceptorTests.java index 8cfb23709b580..4f40487fec46a 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/interceptor/ResizeRequestInterceptorTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/interceptor/ResizeRequestInterceptorTests.java @@ -69,7 +69,7 @@ public void testResizeRequestInterceptorThrowsWhenFLSDLSEnabled() { if (useFls) { fieldPermissions = new FieldPermissions(new FieldPermissionsDefinition(new String[] { "foo" }, null)); } else { - fieldPermissions = new FieldPermissions(); + fieldPermissions = FieldPermissions.DEFAULT; } final boolean useDls = (useFls == false) || randomBoolean(); final Set queries; From 28eb65dad4b0a658279399568f52b221866f88f2 Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Fri, 28 Oct 2022 14:07:51 +1100 Subject: [PATCH 2/7] Fix tests --- .../authz/permission/FieldPermissions.java | 5 +++- .../permission/FieldPermissionsTests.java | 25 ++++++++++++++----- 2 files changed, 23 insertions(+), 7 deletions(-) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/FieldPermissions.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/FieldPermissions.java index d7cdba1eb2be9..0b8e02b23dc99 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/FieldPermissions.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/FieldPermissions.java @@ -99,7 +99,10 @@ private FieldPermissions(List fieldPermissionsDefini // directly wraps the provided automaton throw new IllegalArgumentException("Only accepts deterministic automata"); } - this.fieldPermissionsDefinitions = fieldPermissionsDefinitions; + this.fieldPermissionsDefinitions = Objects.requireNonNull( + fieldPermissionsDefinitions, + "field permission definitions cannot be null" + ); this.originalAutomaton = permittedFieldsAutomaton; this.permittedFieldsAutomaton = new CharacterRunAutomaton(permittedFieldsAutomaton); // we cache the result of isTotal since this might be a costly operation diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/permission/FieldPermissionsTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/permission/FieldPermissionsTests.java index ea1201413879e..411a72bb26e2f 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/permission/FieldPermissionsTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/permission/FieldPermissionsTests.java @@ -16,8 +16,10 @@ import java.io.IOException; import java.util.Arrays; +import java.util.List; import java.util.Set; +import static org.hamcrest.Matchers.hasSize; import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.not; import static org.hamcrest.Matchers.notNullValue; @@ -84,7 +86,7 @@ public void testFieldPermissionsIntersection() { public void testMustHaveNonNullFieldPermissionsDefinition() { final FieldPermissions fieldPermissions0 = FieldPermissions.DEFAULT; - assertThat(fieldPermissions0.getFieldPermissionsDefinition(), notNullValue()); + assertNonNullFieldPermissionDefinitions(fieldPermissions0.getFieldPermissionsDefinitions()); expectThrows(NullPointerException.class, () -> new FieldPermissions(null)); expectThrows(NullPointerException.class, () -> new FieldPermissions(null, Automatons.MATCH_ALL)); @@ -92,13 +94,15 @@ public void testMustHaveNonNullFieldPermissionsDefinition() { FieldPermissions.DEFAULT, new FieldPermissions(fieldPermissionDef(new String[] { "f1", "f2", "f3*" }, new String[] { "f3" })) ); - assertThat(fieldPermissions03.limitFieldPermissions(null).getFieldPermissionsDefinition(), notNullValue()); - assertThat(fieldPermissions03.limitFieldPermissions(FieldPermissions.DEFAULT).getFieldPermissionsDefinition(), notNullValue()); - assertThat( + assertNonNullFieldPermissionDefinitions(fieldPermissions03.limitFieldPermissions(null).getFieldPermissionsDefinitions()); + assertNonNullFieldPermissionDefinitions( + fieldPermissions03.limitFieldPermissions(FieldPermissions.DEFAULT).getFieldPermissionsDefinitions() + ); + assertNonNullFieldPermissionDefinitions( fieldPermissions03.limitFieldPermissions( new FieldPermissions(fieldPermissionDef(new String[] { "f1", "f3*", "f4" }, new String[] { "f3" })) - ).getFieldPermissionsDefinition(), - notNullValue() + ).getFieldPermissionsDefinitions(), + fieldPermissions03.hasFieldLevelSecurity() ? 2 : 1 ); } @@ -180,4 +184,13 @@ private static FieldPermissionsDefinition fieldPermissionDef(String[] granted, S return new FieldPermissionsDefinition(granted, denied); } + private void assertNonNullFieldPermissionDefinitions(List fieldPermissionsDefinitions) { + assertNonNullFieldPermissionDefinitions(fieldPermissionsDefinitions, 1); + } + + private void assertNonNullFieldPermissionDefinitions(List fieldPermissionsDefinitions, int expectedSize) { + assertThat(fieldPermissionsDefinitions, notNullValue()); + assertThat(fieldPermissionsDefinitions, hasSize(expectedSize)); + fieldPermissionsDefinitions.forEach(fieldPermissionsDefinition -> assertThat(fieldPermissionsDefinition, notNullValue())); + } } From 970540b29ed1bd6eac7c14ec664f9b66c0a089cd Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Fri, 28 Oct 2022 14:24:13 +1100 Subject: [PATCH 3/7] add more tests --- .../authz/permission/FieldPermissions.java | 1 - .../permission/FieldPermissionsTests.java | 39 +++++++++++++++++++ 2 files changed, 39 insertions(+), 1 deletion(-) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/FieldPermissions.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/FieldPermissions.java index 0b8e02b23dc99..8fb8d4488081f 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/FieldPermissions.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/FieldPermissions.java @@ -75,7 +75,6 @@ private FieldPermissions() { this(new FieldPermissionsDefinition(null, null), Automatons.MATCH_ALL); } - // TODO: This constructor is only used by tests. We should remove it in future /** Constructor that enables field-level security based on include/exclude rules. Exclude rules * have precedence over include rules. */ public FieldPermissions(FieldPermissionsDefinition fieldPermissionsDefinition) { diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/permission/FieldPermissionsTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/permission/FieldPermissionsTests.java index 411a72bb26e2f..e72c4145c0f07 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/permission/FieldPermissionsTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/permission/FieldPermissionsTests.java @@ -19,6 +19,7 @@ import java.util.List; import java.util.Set; +import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.hasSize; import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.not; @@ -84,6 +85,44 @@ public void testFieldPermissionsIntersection() { } } + public void testMultipleLimiting() { + // Basic test for a number of permission definitions + FieldPermissions fieldPermissions = FieldPermissions.DEFAULT; + final int nSets = randomIntBetween(2, 8); + final FieldPermissionsDefinition fieldPermissionsDefinition = fieldPermissionDef( + new String[] { "f1", "f2", "f3*" }, + new String[] { "f3" } + ); + for (int i = 0; i < nSets; i++) { + fieldPermissions = fieldPermissions.limitFieldPermissions(new FieldPermissions(fieldPermissionsDefinition)); + } + final List fieldPermissionsDefinitions = fieldPermissions.getFieldPermissionsDefinitions(); + assertNonNullFieldPermissionDefinitions(fieldPermissionsDefinitions, nSets); + fieldPermissionsDefinitions.forEach(fpd -> assertThat(fpd, equalTo(fieldPermissionsDefinition))); + assertThat(fieldPermissions.grantsAccessTo(randomFrom("f1", "f2", "f31")), is(true)); + assertThat(fieldPermissions.grantsAccessTo(randomFrom("f1", "f2", "f3")), is(false)); + + // More realistic intersection + fieldPermissions = FieldPermissions.DEFAULT; + fieldPermissions = fieldPermissions.limitFieldPermissions( + new FieldPermissions(fieldPermissionDef(new String[] { "f1", "f2", "f3*", "f4*" }, new String[] { "f3" })) + ); + fieldPermissions = fieldPermissions.limitFieldPermissions( + new FieldPermissions(fieldPermissionDef(new String[] { "f2", "f3*", "f4*", "f5*" }, new String[] { "f4" })) + ); + fieldPermissions = fieldPermissions.limitFieldPermissions( + new FieldPermissions(fieldPermissionDef(new String[] { "f3*", "f4*", "f5*", "f6" }, new String[] { "f5" })) + ); + assertNonNullFieldPermissionDefinitions(fieldPermissions.getFieldPermissionsDefinitions(), 3); + + assertThat(fieldPermissions.grantsAccessTo(randomFrom("f1", "f2", "f5", "f6") + randomAlphaOfLengthBetween(0, 10)), is(false)); + assertThat(fieldPermissions.grantsAccessTo("f3"), is(false)); + assertThat(fieldPermissions.grantsAccessTo("f4"), is(false)); + + assertThat(fieldPermissions.grantsAccessTo("f3" + randomAlphaOfLengthBetween(1, 10)), is(true)); + assertThat(fieldPermissions.grantsAccessTo("f4" + randomAlphaOfLengthBetween(1, 10)), is(true)); + } + public void testMustHaveNonNullFieldPermissionsDefinition() { final FieldPermissions fieldPermissions0 = FieldPermissions.DEFAULT; assertNonNullFieldPermissionDefinitions(fieldPermissions0.getFieldPermissionsDefinitions()); From e4213f17bb16a709ecf7a68a8de738ae26453ce1 Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Fri, 28 Oct 2022 14:43:38 +1100 Subject: [PATCH 4/7] tweak --- .../xpack/core/security/authz/permission/IndicesPermission.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 806ff095d5697..70a418e1e1c2f 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 @@ -609,7 +609,7 @@ public static class Group { private final String[] indices; private final StringMatcher indexNameMatcher; private final Supplier indexNameAutomaton; - // TODO: Use FieldPermissionsDefinition instead of FieldPermissions. The former is more suitable to this layer (user input) + // TODO: Use FieldPermissionsDefinition instead of FieldPermissions. The former is a better counterpart to query private final FieldPermissions fieldPermissions; private final Set query; // by default certain restricted indices are exempted when granting privileges, as they should generally be hidden for ordinary From e09ef685d202a98a8adbdc07cd89f50d9e419f2f Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Sat, 29 Oct 2022 08:49:34 +1100 Subject: [PATCH 5/7] address feedback --- .../xpack/core/security/authz/permission/FieldPermissions.java | 2 +- .../core/security/authz/permission/FieldPermissionsTests.java | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/FieldPermissions.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/FieldPermissions.java index 8fb8d4488081f..8f2088f55ade6 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/FieldPermissions.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/FieldPermissions.java @@ -198,7 +198,7 @@ public static Automaton buildPermittedFieldsAutomaton(final String[] grantedFiel */ public FieldPermissions limitFieldPermissions(FieldPermissions limitedBy) { if (hasFieldLevelSecurity() && limitedBy != null && limitedBy.hasFieldLevelSecurity()) { - // TODO: the automaton computation is not cached + // TODO: cache the automaton computation with FieldPermissionsCache Automaton _permittedFieldsAutomaton = Automatons.intersectAndMinimize(getIncludeAutomaton(), limitedBy.getIncludeAutomaton()); return new FieldPermissions( CollectionUtils.concatLists(fieldPermissionsDefinitions, limitedBy.fieldPermissionsDefinitions), diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/permission/FieldPermissionsTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/permission/FieldPermissionsTests.java index e72c4145c0f07..ead26cc9f9973 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/permission/FieldPermissionsTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/permission/FieldPermissionsTests.java @@ -100,7 +100,7 @@ public void testMultipleLimiting() { assertNonNullFieldPermissionDefinitions(fieldPermissionsDefinitions, nSets); fieldPermissionsDefinitions.forEach(fpd -> assertThat(fpd, equalTo(fieldPermissionsDefinition))); assertThat(fieldPermissions.grantsAccessTo(randomFrom("f1", "f2", "f31")), is(true)); - assertThat(fieldPermissions.grantsAccessTo(randomFrom("f1", "f2", "f3")), is(false)); + assertThat(fieldPermissions.grantsAccessTo("f3"), is(false)); // More realistic intersection fieldPermissions = FieldPermissions.DEFAULT; From 49cf6b2b43ef05cc6e25fd94622388f79c89ba20 Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Sat, 29 Oct 2022 09:01:00 +1100 Subject: [PATCH 6/7] address feedback --- .../authz/permission/FieldPermissionsTests.java | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/permission/FieldPermissionsTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/permission/FieldPermissionsTests.java index ead26cc9f9973..6608216fad960 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/permission/FieldPermissionsTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/permission/FieldPermissionsTests.java @@ -28,6 +28,16 @@ public class FieldPermissionsTests extends ESTestCase { + public void test() { + FieldPermissions fieldPermissions = FieldPermissions.DEFAULT; + fieldPermissions = fieldPermissions.limitFieldPermissions( + new FieldPermissions(fieldPermissionDef(new String[] { "f1", "f2" }, new String[] { "" })) + ); + + assertThat(fieldPermissions.grantsAccessTo("f1"), is(true)); + assertThat(fieldPermissions.grantsAccessTo("f2"), is(true)); + } + public void testFieldPermissionsIntersection() { final FieldPermissions fieldPermissions = FieldPermissions.DEFAULT; From cf27ccafaf10290372938e1cf943342032d2b5d1 Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Wed, 2 Nov 2022 10:59:45 +1100 Subject: [PATCH 7/7] Update x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/RBACEngine.java Co-authored-by: Jake Landis --- .../java/org/elasticsearch/xpack/security/authz/RBACEngine.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/RBACEngine.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/RBACEngine.java index d7be0475514a5..0a43b01019b4b 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/RBACEngine.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/RBACEngine.java @@ -727,7 +727,7 @@ static GetUserPrivilegesResponse buildUserPrivilegesResponseObject(Role userRole final List fieldPermissionsDefinitions = group.getFieldPermissions() .getFieldPermissionsDefinitions(); assert fieldPermissionsDefinitions.size() == 1 - : "imited-by field must not exist since we do not support reporting user privileges for limited roles"; + : "limited-by field must not exist since we do not support reporting user privileges for limited roles"; final FieldPermissionsDefinition definition = fieldPermissionsDefinitions.get(0); fieldSecurity = definition.getFieldGrantExcludeGroups(); } else {