From 3b4078a8903b7cef0880be6704d451af50867431 Mon Sep 17 00:00:00 2001 From: Jim Ferenczi Date: Wed, 18 Dec 2024 14:01:05 +0000 Subject: [PATCH] Improve handling of nested fields in index reader wrappers (#118757) This update enhances the handling of parent filters to ensure proper exclusion of child documents. --- docs/changelog/118757.yaml | 5 + .../test/AbstractBuilderTestCase.java | 4 + .../authz/permission/DocumentPermissions.java | 6 +- ...ityIndexReaderWrapperIntegrationTests.java | 180 ++++++++++++++++++ 4 files changed, 193 insertions(+), 2 deletions(-) create mode 100644 docs/changelog/118757.yaml diff --git a/docs/changelog/118757.yaml b/docs/changelog/118757.yaml new file mode 100644 index 0000000000000..956e220f21aeb --- /dev/null +++ b/docs/changelog/118757.yaml @@ -0,0 +1,5 @@ +pr: 118757 +summary: Improve handling of nested fields in index reader wrappers +area: Authorization +type: enhancement +issues: [] diff --git a/test/framework/src/main/java/org/elasticsearch/test/AbstractBuilderTestCase.java b/test/framework/src/main/java/org/elasticsearch/test/AbstractBuilderTestCase.java index 20cb66affddee..d239c6453a7fe 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/AbstractBuilderTestCase.java +++ b/test/framework/src/main/java/org/elasticsearch/test/AbstractBuilderTestCase.java @@ -241,6 +241,10 @@ protected static IndexSettings indexSettings() { return serviceHolder.idxSettings; } + protected static MapperService mapperService() { + return serviceHolder.mapperService; + } + protected static String expectedFieldName(String builderFieldName) { return ALIAS_TO_CONCRETE_FIELD_NAME.getOrDefault(builderFieldName, builderFieldName); } diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/DocumentPermissions.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/DocumentPermissions.java index 14ecf4cb0d6e9..9898e4c248931 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/DocumentPermissions.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/DocumentPermissions.java @@ -159,15 +159,17 @@ private static void buildRoleQuery( if (queryBuilder != null) { failIfQueryUsesClient(queryBuilder, context); Query roleQuery = context.toQuery(queryBuilder).query(); - filter.add(roleQuery, SHOULD); NestedLookup nestedLookup = context.nestedLookup(); - if (nestedLookup != NestedLookup.EMPTY) { + if (nestedLookup == NestedLookup.EMPTY) { + filter.add(roleQuery, SHOULD); + } else { NestedHelper nestedHelper = new NestedHelper(nestedLookup, context::isFieldMapped); if (nestedHelper.mightMatchNestedDocs(roleQuery)) { roleQuery = new BooleanQuery.Builder().add(roleQuery, FILTER) .add(Queries.newNonNestedFilter(context.indexVersionCreated()), FILTER) .build(); } + filter.add(roleQuery, SHOULD); // If access is allowed on root doc then also access is allowed on all nested docs of that root document: BitSetProducer rootDocs = context.bitsetFilter(Queries.newNonNestedFilter(context.indexVersionCreated())); ToChildBlockJoinQuery includeNestedDocs = new ToChildBlockJoinQuery(roleQuery, rootDocs); 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 df64c4f87410a..0da6ebb8318b9 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 @@ -23,9 +23,12 @@ import org.apache.lucene.search.TotalHitCountCollectorManager; import org.apache.lucene.store.Directory; import org.elasticsearch.client.internal.Client; +import org.elasticsearch.common.Strings; import org.elasticsearch.common.bytes.BytesArray; import org.elasticsearch.common.bytes.BytesReference; +import org.elasticsearch.common.compress.CompressedXContent; import org.elasticsearch.common.lucene.index.ElasticsearchDirectoryReader; +import org.elasticsearch.common.lucene.search.Queries; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.util.concurrent.ThreadContext; import org.elasticsearch.index.IndexSettings; @@ -33,9 +36,11 @@ import org.elasticsearch.index.mapper.KeywordFieldMapper.KeywordFieldType; import org.elasticsearch.index.mapper.MappedFieldType; import org.elasticsearch.index.mapper.MapperMetrics; +import org.elasticsearch.index.mapper.MapperService; import org.elasticsearch.index.mapper.Mapping; import org.elasticsearch.index.mapper.MappingLookup; import org.elasticsearch.index.mapper.MockFieldMapper; +import org.elasticsearch.index.mapper.SourceToParse; import org.elasticsearch.index.query.ParsedQuery; import org.elasticsearch.index.query.SearchExecutionContext; import org.elasticsearch.index.query.TermsQueryBuilder; @@ -45,6 +50,9 @@ import org.elasticsearch.search.internal.ContextIndexSearcher; import org.elasticsearch.test.AbstractBuilderTestCase; import org.elasticsearch.test.IndexSettingsModule; +import org.elasticsearch.xcontent.XContentBuilder; +import org.elasticsearch.xcontent.XContentFactory; +import org.elasticsearch.xcontent.XContentType; import org.elasticsearch.xpack.core.security.SecurityContext; import org.elasticsearch.xpack.core.security.authc.Authentication; import org.elasticsearch.xpack.core.security.authc.AuthenticationTestHelper; @@ -52,6 +60,8 @@ import org.elasticsearch.xpack.core.security.authz.permission.DocumentPermissions; import org.elasticsearch.xpack.core.security.authz.permission.FieldPermissions; +import java.io.IOException; +import java.util.Arrays; import java.util.HashSet; import java.util.List; import java.util.Set; @@ -340,6 +350,176 @@ protected IndicesAccessControl getIndicesAccessControl() { directory.close(); } + @Override + protected void initializeAdditionalMappings(MapperService mapperService) throws IOException { + XContentBuilder builder = XContentFactory.jsonBuilder() + .startObject() + .startObject("properties") + .startObject("f1") + .field("type", "keyword") + .endObject() + .startObject("nested1") + .field("type", "nested") + .startObject("properties") + .startObject("field") + .field("type", "keyword") + .endObject() + .endObject() + .endObject() + .endObject() + .endObject(); + mapperService.merge( + MapperService.SINGLE_MAPPING_NAME, + new CompressedXContent(Strings.toString(builder)), + MapperService.MergeReason.MAPPING_UPDATE + ); + } + + public void testDLSWithNestedDocs() throws Exception { + Directory directory = newDirectory(); + try ( + IndexWriter iw = new IndexWriter( + directory, + new IndexWriterConfig(new StandardAnalyzer()).setMergePolicy(NoMergePolicy.INSTANCE) + ) + ) { + var parser = mapperService().documentParser(); + String doc = """ + { + "f1": "value", + "nested1": [ + { + "field": "0" + }, + { + "field": "1" + }, + {} + ] + } + """; + var parsedDoc = parser.parseDocument( + new SourceToParse("0", new BytesArray(doc), XContentType.JSON), + mapperService().mappingLookup() + ); + iw.addDocuments(parsedDoc.docs()); + + doc = """ + { + "nested1": [ + { + "field": "12" + }, + { + "field": "13" + }, + {} + ] + } + """; + parsedDoc = parser.parseDocument( + new SourceToParse("1", new BytesArray(doc), XContentType.JSON), + mapperService().mappingLookup() + ); + iw.addDocuments(parsedDoc.docs()); + + doc = """ + { + "f1": "value", + "nested1": [ + { + "field": "12" + }, + {} + ] + } + """; + parsedDoc = parser.parseDocument( + new SourceToParse("2", new BytesArray(doc), XContentType.JSON), + mapperService().mappingLookup() + ); + iw.addDocuments(parsedDoc.docs()); + + doc = """ + { + "nested1": [ + { + "field": "12" + }, + {} + ] + } + """; + parsedDoc = parser.parseDocument( + new SourceToParse("3", new BytesArray(doc), XContentType.JSON), + mapperService().mappingLookup() + ); + iw.addDocuments(parsedDoc.docs()); + + iw.commit(); + } + + DirectoryReader directoryReader = ElasticsearchDirectoryReader.wrap( + DirectoryReader.open(directory), + new ShardId(indexSettings().getIndex(), 0) + ); + SearchExecutionContext context = createSearchExecutionContext(new IndexSearcher(directoryReader)); + + final ThreadContext threadContext = new ThreadContext(Settings.EMPTY); + final SecurityContext securityContext = new SecurityContext(Settings.EMPTY, threadContext); + final Authentication authentication = AuthenticationTestHelper.builder().build(); + new AuthenticationContextSerializer().writeToContext(authentication, threadContext); + + Set queries = new HashSet<>(); + queries.add(new BytesArray("{\"bool\": { \"must_not\": { \"exists\": { \"field\": \"f1\" } } } }")); + IndicesAccessControl.IndexAccessControl indexAccessControl = new IndicesAccessControl.IndexAccessControl( + FieldPermissions.DEFAULT, + DocumentPermissions.filteredBy(queries) + ); + + DocumentSubsetBitsetCache bitsetCache = new DocumentSubsetBitsetCache(Settings.EMPTY, Executors.newSingleThreadExecutor()); + + final MockLicenseState licenseState = mock(MockLicenseState.class); + when(licenseState.isAllowed(DOCUMENT_LEVEL_SECURITY_FEATURE)).thenReturn(true); + ScriptService scriptService = mock(ScriptService.class); + SecurityIndexReaderWrapper wrapper = new SecurityIndexReaderWrapper( + s -> context, + bitsetCache, + securityContext, + licenseState, + scriptService + ) { + + @Override + protected IndicesAccessControl getIndicesAccessControl() { + IndicesAccessControl indicesAccessControl = new IndicesAccessControl( + true, + singletonMap(indexSettings().getIndex().getName(), indexAccessControl) + ); + return indicesAccessControl; + } + }; + + DirectoryReader wrappedDirectoryReader = wrapper.apply(directoryReader); + IndexSearcher indexSearcher = new ContextIndexSearcher( + wrappedDirectoryReader, + IndexSearcher.getDefaultSimilarity(), + IndexSearcher.getDefaultQueryCache(), + IndexSearcher.getDefaultQueryCachingPolicy(), + true + ); + + ScoreDoc[] hits = indexSearcher.search(new MatchAllDocsQuery(), 1000).scoreDocs; + assertThat(Arrays.stream(hits).map(h -> h.doc).collect(Collectors.toSet()), containsInAnyOrder(4, 5, 6, 7, 11, 12, 13)); + + hits = indexSearcher.search(Queries.newNonNestedFilter(context.indexVersionCreated()), 1000).scoreDocs; + assertThat(Arrays.stream(hits).map(h -> h.doc).collect(Collectors.toSet()), containsInAnyOrder(7, 13)); + + bitsetCache.close(); + directoryReader.close(); + directory.close(); + } + private static MappingLookup createMappingLookup(List concreteFields) { List mappers = concreteFields.stream().map(MockFieldMapper::new).collect(Collectors.toList()); return MappingLookup.fromMappers(Mapping.EMPTY, mappers, emptyList());