[Backport 2.x] Remove static metaFields list and get version-specific values from core (#4412)

opensearch-trigger-bot[bot] · github-actions[bot] · web-flow · commit b3df23eba32e · 2024-06-07T23:22:29.000+02:00
Signed-off-by: Craig Perkins &lt;cwperx@amazon.com&gt;
Signed-off-by: github-actions[bot] &lt;github-actions[bot]@users.noreply.github.com&gt;
Co-authored-by: github-actions[bot] &lt;github-actions[bot]@users.noreply.github.com&gt;
diff --git a/build.gradle b/build.gradle
@@ -724,6 +724,7 @@ dependencies {
         exclude(group: 'org.hamcrest', module: 'hamcrest')
     }
     integrationTestImplementation 'com.unboundid:unboundid-ldapsdk:4.0.14'
+    integrationTestImplementation "org.opensearch.plugin:mapper-size:${opensearch_version}"
     integrationTestImplementation "org.apache.httpcomponents:httpclient-cache:4.5.14"
     integrationTestImplementation "org.apache.httpcomponents:httpclient:4.5.14"
     integrationTestImplementation "org.apache.httpcomponents:fluent-hc:4.5.14"
diff --git a/src/integrationTest/java/org/opensearch/security/FlsAndFieldMaskingTests.java b/src/integrationTest/java/org/opensearch/security/FlsAndFieldMaskingTests.java
@@ -27,6 +27,7 @@
 import org.junit.runner.RunWith;
 
 import org.opensearch.action.admin.indices.alias.IndicesAliasesRequest;
+import org.opensearch.action.admin.indices.create.CreateIndexRequest;
 import org.opensearch.action.fieldcaps.FieldCapabilitiesRequest;
 import org.opensearch.action.fieldcaps.FieldCapabilitiesResponse;
 import org.opensearch.action.get.GetRequest;
@@ -43,10 +44,15 @@
 import org.opensearch.action.search.SearchScrollRequest;
 import org.opensearch.client.Client;
 import org.opensearch.client.RestHighLevelClient;
+import org.opensearch.index.mapper.SourceFieldMapper;
+import org.opensearch.index.mapper.size.SizeFieldMapper;
+import org.opensearch.index.query.MatchAllQueryBuilder;
 import org.opensearch.index.query.QueryBuilder;
 import org.opensearch.index.query.QueryBuilders;
+import org.opensearch.plugin.mapper.MapperSizePlugin;
 import org.opensearch.search.aggregations.Aggregation;
 import org.opensearch.search.aggregations.metrics.ParsedAvg;
+import org.opensearch.search.builder.SearchSourceBuilder;
 import org.opensearch.search.sort.SortOrder;
 import org.opensearch.test.framework.TestSecurityConfig;
 import org.opensearch.test.framework.cluster.ClusterManager;
@@ -55,6 +61,7 @@
 
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.allOf;
+import static org.hamcrest.Matchers.containsString;
 import static org.hamcrest.Matchers.everyItem;
 import static org.hamcrest.Matchers.greaterThan;
 import static org.hamcrest.Matchers.instanceOf;
@@ -94,6 +101,7 @@
 import static org.opensearch.test.framework.matcher.SearchResponseMatchers.isSuccessfulSearchResponse;
 import static org.opensearch.test.framework.matcher.SearchResponseMatchers.numberOfTotalHitsIsEqualTo;
 import static org.opensearch.test.framework.matcher.SearchResponseMatchers.searchHitContainsFieldWithValue;
+import static org.opensearch.test.framework.matcher.SearchResponseMatchers.searchHitDoesContainField;
 import static org.opensearch.test.framework.matcher.SearchResponseMatchers.searchHitDoesNotContainField;
 import static org.opensearch.test.framework.matcher.SearchResponseMatchers.searchHitsContainDocumentWithId;
 
@@ -212,6 +220,7 @@ public class FlsAndFieldMaskingTests {
         .nodeSettings(
             Map.of("plugins.security.restapi.roles_enabled", List.of("user_" + ADMIN_USER.getName() + "__" + ALL_ACCESS.getName()))
         )
+        .plugin(MapperSizePlugin.class)
         .authc(AUTHC_HTTPBASIC_INTERNAL)
         .users(
             ADMIN_USER,
@@ -426,6 +435,10 @@ public void flsEnabledFieldsAreHiddenForNormalUsers() throws IOException {
 
     private static List<String> createIndexWithDocs(String indexName, Song... songs) {
         try (Client client = cluster.getInternalNodeClient()) {
+            client.admin()
+                .indices()
+                .create(new CreateIndexRequest(indexName).mapping(Map.of("_size", Map.of("enabled", true))))
+                .actionGet();
             return Stream.of(songs).map(song -> {
                 IndexResponse response = client.index(new IndexRequest(indexName).setRefreshPolicy(IMMEDIATE).source(song.asMap()))
                     .actionGet();
@@ -469,6 +482,14 @@ private static void assertSearchHitsDoNotContainField(SearchResponse response, S
             .forEach(index -> assertThat(response, searchHitDoesNotContainField(index, excludedField)));
     }
 
+    private static void assertSearchHitsDoContainField(SearchResponse response, String includedField) {
+        assertThat(response, isSuccessfulSearchResponse());
+        assertThat(response.getHits().getHits().length, greaterThan(0));
+        IntStream.range(0, response.getHits().getHits().length)
+            .boxed()
+            .forEach(index -> assertThat(response, searchHitDoesContainField(index, includedField)));
+    }
+
     @Test
     public void searchForDocuments() throws IOException {
         // FIELD MASKING
@@ -811,4 +832,28 @@ public void getFieldCapabilities() throws IOException {
         }
     }
 
+    @Test
+    public void flsWithIncludesRulesIncludesFieldMappersFromPlugins() throws IOException {
+        String indexName = "fls_includes_index";
+        TestSecurityConfig.Role userRole = new TestSecurityConfig.Role("fls_include_stars_reader").clusterPermissions(
+            "cluster_composite_ops_ro"
+        ).indexPermissions("read").fls(FIELD_STARS).on("*");
+        TestSecurityConfig.User user = createUserWithRole("fls_includes_user", userRole);
+        List<String> docIds = createIndexWithDocs(indexName, SONGS[0], SONGS[1]);
+
+        try (RestHighLevelClient restHighLevelClient = cluster.getRestHighLevelClient(user)) {
+            SearchRequest searchRequest = new SearchRequest(indexName);
+            SearchSourceBuilder searchSourceBuilder = new SearchSourceBuilder();
+            MatchAllQueryBuilder matchAllQueryBuilder = QueryBuilders.matchAllQuery();
+            searchSourceBuilder.storedFields(List.of(SizeFieldMapper.NAME, SourceFieldMapper.NAME));
+            searchSourceBuilder.query(matchAllQueryBuilder);
+            searchRequest.source(searchSourceBuilder);
+            SearchResponse searchResponse = restHighLevelClient.search(searchRequest, DEFAULT);
+
+            assertSearchHitsDoContainField(searchResponse, FIELD_STARS);
+            assertThat(searchResponse.toString(), containsString(SizeFieldMapper.NAME));
+            assertSearchHitsDoNotContainField(searchResponse, FIELD_ARTIST);
+        }
+    }
+
 }
diff --git a/src/integrationTest/java/org/opensearch/test/framework/matcher/SearchHitDoesContainFieldMatcher.java b/src/integrationTest/java/org/opensearch/test/framework/matcher/SearchHitDoesContainFieldMatcher.java
@@ -0,0 +1,62 @@
+/*
+* Copyright OpenSearch Contributors
+* SPDX-License-Identifier: Apache-2.0
+*
+* The OpenSearch Contributors require contributions made to
+* this file be licensed under the Apache-2.0 license or a
+* compatible open source license.
+*
+*/
+package org.opensearch.test.framework.matcher;
+
+import java.util.Map;
+
+import org.hamcrest.Description;
+import org.hamcrest.TypeSafeDiagnosingMatcher;
+
+import org.opensearch.action.search.SearchResponse;
+import org.opensearch.search.SearchHit;
+
+import static java.util.Objects.requireNonNull;
+import static org.opensearch.test.framework.matcher.SearchResponseMatchers.readTotalHits;
+
+class SearchHitDoesContainFieldMatcher extends TypeSafeDiagnosingMatcher<SearchResponse> {
+
+    private final int hitIndex;
+
+    private final String fieldName;
+
+    public SearchHitDoesContainFieldMatcher(int hitIndex, String fieldName) {
+        this.hitIndex = hitIndex;
+        this.fieldName = requireNonNull(fieldName, "Field name is required.");
+    }
+
+    @Override
+    protected boolean matchesSafely(SearchResponse searchResponse, Description mismatchDescription) {
+        Long numberOfHits = readTotalHits(searchResponse);
+        if (numberOfHits == null) {
+            mismatchDescription.appendText("Total number of hits is unknown.");
+            return false;
+        }
+        if (hitIndex >= numberOfHits) {
+            mismatchDescription.appendText("Search result contain only ").appendValue(numberOfHits).appendText(" hits");
+            return false;
+        }
+        SearchHit searchHit = searchResponse.getHits().getAt(hitIndex);
+        Map<String, Object> source = searchHit.getSourceAsMap();
+        if (source == null) {
+            mismatchDescription.appendText("Source document is null, is fetch source option set to true?");
+            return false;
+        }
+        if (!source.containsKey(fieldName)) {
+            mismatchDescription.appendText(" document does not contain field ").appendValue(fieldName);
+            return false;
+        }
+        return true;
+    }
+
+    @Override
+    public void describeTo(Description description) {
+        description.appendText("search hit with index ").appendValue(hitIndex).appendText(" does contain field ").appendValue(fieldName);
+    }
+}
diff --git a/src/integrationTest/java/org/opensearch/test/framework/matcher/SearchResponseMatchers.java b/src/integrationTest/java/org/opensearch/test/framework/matcher/SearchResponseMatchers.java
@@ -44,6 +44,10 @@ public static Matcher<SearchResponse> searchHitDoesNotContainField(int hitIndex,
         return new SearchHitDoesNotContainFieldMatcher(hitIndex, fieldName);
     }
 
+    public static Matcher<SearchResponse> searchHitDoesContainField(int hitIndex, String fieldName) {
+        return new SearchHitDoesContainFieldMatcher(hitIndex, fieldName);
+    }
+
     public static Matcher<SearchResponse> searchHitsContainDocumentWithId(int hitIndex, String indexName, String documentId) {
         return new SearchHitsContainDocumentWithIdMatcher(hitIndex, indexName, documentId);
     }
diff --git a/src/main/java/org/opensearch/security/configuration/SecurityFlsDlsIndexSearcherWrapper.java b/src/main/java/org/opensearch/security/configuration/SecurityFlsDlsIndexSearcherWrapper.java
@@ -12,6 +12,8 @@
 package org.opensearch.security.configuration;
 
 import java.io.IOException;
+import java.util.Arrays;
+import java.util.Collections;
 import java.util.HashSet;
 import java.util.Map;
 import java.util.Set;
@@ -26,7 +28,7 @@
 import org.opensearch.common.settings.Settings;
 import org.opensearch.core.index.shard.ShardId;
 import org.opensearch.index.IndexService;
-import org.opensearch.index.mapper.IgnoredFieldMapper;
+import org.opensearch.index.mapper.SeqNoFieldMapper;
 import org.opensearch.index.query.QueryShardContext;
 import org.opensearch.index.shard.ShardUtils;
 import org.opensearch.security.auditlog.AuditLog;
@@ -38,24 +40,9 @@
 
 public class SecurityFlsDlsIndexSearcherWrapper extends SecurityIndexSearcherWrapper {
 
-    // TODO: the list is outdated. It is necessary to change how meta fields are handled in the near future.
-    // We may consider using MapperService.isMetadataField() instead of relying on the static set or
-    // (if it is too costly or does not meet requirements) use IndicesModule.getBuiltInMetadataFields()
-    // for OpenSearch version specific Set of meta fields
-    private static final Set<String> metaFields = Sets.newHashSet(
-        "_source",
-        "_version",
-        "_field_names",
-        "_seq_no",
-        "_primary_term",
-        "_id",
-        IgnoredFieldMapper.NAME,
-        "_index",
-        "_routing",
-        "_size",
-        "_timestamp",
-        "_ttl",
-        "_type"
+    private final Set<String> metaFields;
+    public static final Set<String> META_FIELDS_BEFORE_7DOT8 = Collections.unmodifiableSet(
+        new HashSet<>(Arrays.asList("_timestamp", "_ttl", "_type"))
     );
     private final ClusterService clusterService;
     private final IndexService indexService;
@@ -75,6 +62,11 @@ public SecurityFlsDlsIndexSearcherWrapper(
         final Salt salt
     ) {
         super(indexService, settings, adminDNs, evaluator);
+        Set<String> metadataFieldsCopy = new HashSet<>(indexService.mapperService().getMetadataFields());
+        SeqNoFieldMapper.SequenceIDFields sequenceIDFields = SeqNoFieldMapper.SequenceIDFields.emptySeqID();
+        metadataFieldsCopy.add(sequenceIDFields.primaryTerm.name());
+        metadataFieldsCopy.addAll(META_FIELDS_BEFORE_7DOT8);
+        metaFields = metadataFieldsCopy;
         ciol.setIs(indexService);
         this.clusterService = clusterService;
         this.indexService = indexService;

Original file line number	Diff line number	Diff line change
`@@ -724,6 +724,7 @@ dependencies {`
`724`	`724`	`exclude(group: 'org.hamcrest', module: 'hamcrest')`
`725`	`725`	`}`
`726`	`726`	`integrationTestImplementation 'com.unboundid:unboundid-ldapsdk:4.0.14'`
	`727`	`+ integrationTestImplementation "org.opensearch.plugin:mapper-size:${opensearch_version}"`
`727`	`728`	`integrationTestImplementation "org.apache.httpcomponents:httpclient-cache:4.5.14"`
`728`	`729`	`integrationTestImplementation "org.apache.httpcomponents:httpclient:4.5.14"`
`729`	`730`	`integrationTestImplementation "org.apache.httpcomponents:fluent-hc:4.5.14"`
Original file line number	Diff line number	Diff line change
`@@ -44,6 +44,10 @@ public static Matcher<SearchResponse> searchHitDoesNotContainField(int hitIndex,`
`44`	`44`	`return new SearchHitDoesNotContainFieldMatcher(hitIndex, fieldName);`
`45`	`45`	`}`
`46`	`46`
	`47`	`+ public static Matcher<SearchResponse> searchHitDoesContainField(int hitIndex, String fieldName) {`
	`48`	`+ return new SearchHitDoesContainFieldMatcher(hitIndex, fieldName);`
	`49`	`+ }`
	`50`	`+`
`47`	`51`	`public static Matcher<SearchResponse> searchHitsContainDocumentWithId(int hitIndex, String indexName, String documentId) {`
`48`	`52`	`return new SearchHitsContainDocumentWithIdMatcher(hitIndex, indexName, documentId);`
`49`	`53`	`}`