Skip to content

Commit d563999

Browse files
jimczijimczi
authored
Improve handling of nested fields in index reader wrappers (#118757) (#118981)
This update enhances the handling of parent filters to ensure proper exclusion of child documents.
1 parent 6ae803b commit d563999

File tree

4 files changed

+193
-2
lines changed

4 files changed

+193
-2
lines changed

‎docs/changelog/118757.yaml‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
pr: 118757
2+
summary: Improve handling of nested fields in index reader wrappers
3+
area: Authorization
4+
type: enhancement
5+
issues: []

‎test/framework/src/main/java/org/elasticsearch/test/AbstractBuilderTestCase.java‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -233,6 +233,10 @@ protected static IndexSettings indexSettings() {
233233
return serviceHolder.idxSettings;
234234
}
235235

236+
protected static MapperService mapperService() {
237+
return serviceHolder.mapperService;
238+
}
239+
236240
protected static String expectedFieldName(String builderFieldName) {
237241
return ALIAS_TO_CONCRETE_FIELD_NAME.getOrDefault(builderFieldName, builderFieldName);
238242
}

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

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -159,15 +159,17 @@ private static void buildRoleQuery(
159159
if (queryBuilder != null) {
160160
failIfQueryUsesClient(queryBuilder, context);
161161
Query roleQuery = context.toQuery(queryBuilder).query();
162-
filter.add(roleQuery, SHOULD);
163162
NestedLookup nestedLookup = context.nestedLookup();
164-
if (nestedLookup != NestedLookup.EMPTY) {
163+
if (nestedLookup == NestedLookup.EMPTY) {
164+
filter.add(roleQuery, SHOULD);
165+
} else {
165166
NestedHelper nestedHelper = new NestedHelper(nestedLookup, context::isFieldMapped);
166167
if (nestedHelper.mightMatchNestedDocs(roleQuery)) {
167168
roleQuery = new BooleanQuery.Builder().add(roleQuery, FILTER)
168169
.add(Queries.newNonNestedFilter(context.indexVersionCreated()), FILTER)
169170
.build();
170171
}
172+
filter.add(roleQuery, SHOULD);
171173
// If access is allowed on root doc then also access is allowed on all nested docs of that root document:
172174
BitSetProducer rootDocs = context.bitsetFilter(Queries.newNonNestedFilter(context.indexVersionCreated()));
173175
ToChildBlockJoinQuery includeNestedDocs = new ToChildBlockJoinQuery(roleQuery, rootDocs);

‎x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/accesscontrol/SecurityIndexReaderWrapperIntegrationTests.java‎

Lines changed: 180 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,19 +23,24 @@
2323
import org.apache.lucene.search.TotalHitCountCollectorManager;
2424
import org.apache.lucene.store.Directory;
2525
import org.elasticsearch.client.internal.Client;
26+
import org.elasticsearch.common.Strings;
2627
import org.elasticsearch.common.bytes.BytesArray;
2728
import org.elasticsearch.common.bytes.BytesReference;
29+
import org.elasticsearch.common.compress.CompressedXContent;
2830
import org.elasticsearch.common.lucene.index.ElasticsearchDirectoryReader;
31+
import org.elasticsearch.common.lucene.search.Queries;
2932
import org.elasticsearch.common.settings.Settings;
3033
import org.elasticsearch.common.util.concurrent.ThreadContext;
3134
import org.elasticsearch.index.IndexSettings;
3235
import org.elasticsearch.index.mapper.FieldMapper;
3336
import org.elasticsearch.index.mapper.KeywordFieldMapper.KeywordFieldType;
3437
import org.elasticsearch.index.mapper.MappedFieldType;
3538
import org.elasticsearch.index.mapper.MapperMetrics;
39+
import org.elasticsearch.index.mapper.MapperService;
3640
import org.elasticsearch.index.mapper.Mapping;
3741
import org.elasticsearch.index.mapper.MappingLookup;
3842
import org.elasticsearch.index.mapper.MockFieldMapper;
43+
import org.elasticsearch.index.mapper.SourceToParse;
3944
import org.elasticsearch.index.query.ParsedQuery;
4045
import org.elasticsearch.index.query.SearchExecutionContext;
4146
import org.elasticsearch.index.query.TermsQueryBuilder;
@@ -45,13 +50,18 @@
4550
import org.elasticsearch.search.internal.ContextIndexSearcher;
4651
import org.elasticsearch.test.AbstractBuilderTestCase;
4752
import org.elasticsearch.test.IndexSettingsModule;
53+
import org.elasticsearch.xcontent.XContentBuilder;
54+
import org.elasticsearch.xcontent.XContentFactory;
55+
import org.elasticsearch.xcontent.XContentType;
4856
import org.elasticsearch.xpack.core.security.SecurityContext;
4957
import org.elasticsearch.xpack.core.security.authc.Authentication;
5058
import org.elasticsearch.xpack.core.security.authc.AuthenticationTestHelper;
5159
import org.elasticsearch.xpack.core.security.authc.support.AuthenticationContextSerializer;
5260
import org.elasticsearch.xpack.core.security.authz.permission.DocumentPermissions;
5361
import org.elasticsearch.xpack.core.security.authz.permission.FieldPermissions;
5462

63+
import java.io.IOException;
64+
import java.util.Arrays;
5565
import java.util.HashSet;
5666
import java.util.List;
5767
import java.util.Set;
@@ -340,6 +350,176 @@ protected IndicesAccessControl getIndicesAccessControl() {
340350
directory.close();
341351
}
342352

353+
@Override
354+
protected void initializeAdditionalMappings(MapperService mapperService) throws IOException {
355+
XContentBuilder builder = XContentFactory.jsonBuilder()
356+
.startObject()
357+
.startObject("properties")
358+
.startObject("f1")
359+
.field("type", "keyword")
360+
.endObject()
361+
.startObject("nested1")
362+
.field("type", "nested")
363+
.startObject("properties")
364+
.startObject("field")
365+
.field("type", "keyword")
366+
.endObject()
367+
.endObject()
368+
.endObject()
369+
.endObject()
370+
.endObject();
371+
mapperService.merge(
372+
MapperService.SINGLE_MAPPING_NAME,
373+
new CompressedXContent(Strings.toString(builder)),
374+
MapperService.MergeReason.MAPPING_UPDATE
375+
);
376+
}
377+
378+
public void testDLSWithNestedDocs() throws Exception {
379+
Directory directory = newDirectory();
380+
try (
381+
IndexWriter iw = new IndexWriter(
382+
directory,
383+
new IndexWriterConfig(new StandardAnalyzer()).setMergePolicy(NoMergePolicy.INSTANCE)
384+
)
385+
) {
386+
var parser = mapperService().documentParser();
387+
String doc = """
388+
{
389+
"f1": "value",
390+
"nested1": [
391+
{
392+
"field": "0"
393+
},
394+
{
395+
"field": "1"
396+
},
397+
{}
398+
]
399+
}
400+
""";
401+
var parsedDoc = parser.parseDocument(
402+
new SourceToParse("0", new BytesArray(doc), XContentType.JSON),
403+
mapperService().mappingLookup()
404+
);
405+
iw.addDocuments(parsedDoc.docs());
406+
407+
doc = """
408+
{
409+
"nested1": [
410+
{
411+
"field": "12"
412+
},
413+
{
414+
"field": "13"
415+
},
416+
{}
417+
]
418+
}
419+
""";
420+
parsedDoc = parser.parseDocument(
421+
new SourceToParse("1", new BytesArray(doc), XContentType.JSON),
422+
mapperService().mappingLookup()
423+
);
424+
iw.addDocuments(parsedDoc.docs());
425+
426+
doc = """
427+
{
428+
"f1": "value",
429+
"nested1": [
430+
{
431+
"field": "12"
432+
},
433+
{}
434+
]
435+
}
436+
""";
437+
parsedDoc = parser.parseDocument(
438+
new SourceToParse("2", new BytesArray(doc), XContentType.JSON),
439+
mapperService().mappingLookup()
440+
);
441+
iw.addDocuments(parsedDoc.docs());
442+
443+
doc = """
444+
{
445+
"nested1": [
446+
{
447+
"field": "12"
448+
},
449+
{}
450+
]
451+
}
452+
""";
453+
parsedDoc = parser.parseDocument(
454+
new SourceToParse("3", new BytesArray(doc), XContentType.JSON),
455+
mapperService().mappingLookup()
456+
);
457+
iw.addDocuments(parsedDoc.docs());
458+
459+
iw.commit();
460+
}
461+
462+
DirectoryReader directoryReader = ElasticsearchDirectoryReader.wrap(
463+
DirectoryReader.open(directory),
464+
new ShardId(indexSettings().getIndex(), 0)
465+
);
466+
SearchExecutionContext context = createSearchExecutionContext(new IndexSearcher(directoryReader));
467+
468+
final ThreadContext threadContext = new ThreadContext(Settings.EMPTY);
469+
final SecurityContext securityContext = new SecurityContext(Settings.EMPTY, threadContext);
470+
final Authentication authentication = AuthenticationTestHelper.builder().build();
471+
new AuthenticationContextSerializer().writeToContext(authentication, threadContext);
472+
473+
Set<BytesReference> queries = new HashSet<>();
474+
queries.add(new BytesArray("{\"bool\": { \"must_not\": { \"exists\": { \"field\": \"f1\" } } } }"));
475+
IndicesAccessControl.IndexAccessControl indexAccessControl = new IndicesAccessControl.IndexAccessControl(
476+
FieldPermissions.DEFAULT,
477+
DocumentPermissions.filteredBy(queries)
478+
);
479+
480+
DocumentSubsetBitsetCache bitsetCache = new DocumentSubsetBitsetCache(Settings.EMPTY, Executors.newSingleThreadExecutor());
481+
482+
final MockLicenseState licenseState = mock(MockLicenseState.class);
483+
when(licenseState.isAllowed(DOCUMENT_LEVEL_SECURITY_FEATURE)).thenReturn(true);
484+
ScriptService scriptService = mock(ScriptService.class);
485+
SecurityIndexReaderWrapper wrapper = new SecurityIndexReaderWrapper(
486+
s -> context,
487+
bitsetCache,
488+
securityContext,
489+
licenseState,
490+
scriptService
491+
) {
492+
493+
@Override
494+
protected IndicesAccessControl getIndicesAccessControl() {
495+
IndicesAccessControl indicesAccessControl = new IndicesAccessControl(
496+
true,
497+
singletonMap(indexSettings().getIndex().getName(), indexAccessControl)
498+
);
499+
return indicesAccessControl;
500+
}
501+
};
502+
503+
DirectoryReader wrappedDirectoryReader = wrapper.apply(directoryReader);
504+
IndexSearcher indexSearcher = new ContextIndexSearcher(
505+
wrappedDirectoryReader,
506+
IndexSearcher.getDefaultSimilarity(),
507+
IndexSearcher.getDefaultQueryCache(),
508+
IndexSearcher.getDefaultQueryCachingPolicy(),
509+
true
510+
);
511+
512+
ScoreDoc[] hits = indexSearcher.search(new MatchAllDocsQuery(), 1000).scoreDocs;
513+
assertThat(Arrays.stream(hits).map(h -> h.doc).collect(Collectors.toSet()), containsInAnyOrder(4, 5, 6, 7, 11, 12, 13));
514+
515+
hits = indexSearcher.search(Queries.newNonNestedFilter(context.indexVersionCreated()), 1000).scoreDocs;
516+
assertThat(Arrays.stream(hits).map(h -> h.doc).collect(Collectors.toSet()), containsInAnyOrder(7, 13));
517+
518+
bitsetCache.close();
519+
directoryReader.close();
520+
directory.close();
521+
}
522+
343523
private static MappingLookup createMappingLookup(List<MappedFieldType> concreteFields) {
344524
List<FieldMapper> mappers = concreteFields.stream().map(MockFieldMapper::new).collect(Collectors.toList());
345525
return MappingLookup.fromMappers(Mapping.EMPTY, mappers, emptyList());

0 commit comments

Comments
 (0)