Uses jackson object-mapper to read resource class and updates the int…

…egration tests as well as security policy Signed-off-by: Darshit Chanpura <[email protected]>
opensearch-project · Dec 18, 2024 · 686f037 · 686f037
1 parent bcd0f2b
commit 686f037
Show file tree

Hide file tree

Showing 5 changed files with 55 additions and 59 deletions.
diff --git a/libs/core/build.gradle b/libs/core/build.gradle
@@ -40,6 +40,8 @@ dependencies {
   api project(':libs:opensearch-common')
 
   api "com.fasterxml.jackson.core:jackson-core:${versions.jackson}"
+  api "com.fasterxml.jackson.core:jackson-databind:${versions.jackson_databind}"
+  api "com.fasterxml.jackson.core:jackson-annotations:${versions.jackson}"
 
   // lucene
   api "org.apache.lucene:lucene-core:${versions.lucene}"

diff --git a/...org/opensearch/accesscontrol/resources/fallback/DefaultResourceAccessControlPluginIT.java b/...org/opensearch/accesscontrol/resources/fallback/DefaultResourceAccessControlPluginIT.java
@@ -12,7 +12,6 @@
 import org.opensearch.core.xcontent.XContentBuilder;
 import org.opensearch.plugins.Plugin;
 import org.opensearch.plugins.ResourceAccessControlPlugin;
-import org.opensearch.test.InternalTestCluster;
 import org.opensearch.test.OpenSearchIntegTestCase;
 import org.opensearch.threadpool.ThreadPool;
 import org.hamcrest.MatcherAssert;
@@ -22,7 +21,7 @@
 import java.util.List;
 import java.util.Set;
 
-import static org.opensearch.accesscontrol.resources.fallback.SampleTestResourcePlugin.SAMPLE_TEST_INDEX;
+import static org.opensearch.accesscontrol.resources.fallback.TestResourcePlugin.SAMPLE_TEST_INDEX;
 import static org.opensearch.common.xcontent.XContentFactory.jsonBuilder;
 import static org.hamcrest.Matchers.hasItem;
 import static org.hamcrest.Matchers.hasProperty;
@@ -32,7 +31,7 @@
 public class DefaultResourceAccessControlPluginIT extends OpenSearchIntegTestCase {
     @Override
     protected Collection<Class<? extends Plugin>> nodePlugins() {
-        return List.of(SampleTestResourcePlugin.class);
+        return List.of(TestResourcePlugin.class);
     }
 
     public void testGetResources() throws IOException {
@@ -41,33 +40,34 @@ public void testGetResources() throws IOException {
         createIndex(SAMPLE_TEST_INDEX);
         indexSampleDocuments();
 
-        Set<TestResource> resources;
+        Set<TestResourcePlugin.TestResource> resources;
         try (
-            InternalTestCluster cluster = internalCluster();
             DefaultResourceAccessControlPlugin plugin = new DefaultResourceAccessControlPlugin(
                 client,
-                cluster.getInstance(ThreadPool.class)
+                internalCluster().getInstance(ThreadPool.class)
             )
         ) {
+            resources = plugin.getAccessibleResourcesForCurrentUser(SAMPLE_TEST_INDEX, TestResourcePlugin.TestResource.class);
 
-            resources = plugin.getAccessibleResourcesForCurrentUser(SAMPLE_TEST_INDEX, TestResource.class);
-        }
-
-        assertNotNull(resources);
-        MatcherAssert.assertThat(resources, hasSize(2));
+            assertNotNull(resources);
+            MatcherAssert.assertThat(resources, hasSize(2));
 
-        MatcherAssert.assertThat(resources, hasItem(hasProperty("id", is("1"))));
-        MatcherAssert.assertThat(resources, hasItem(hasProperty("id", is("2"))));
+            MatcherAssert.assertThat(resources, hasItem(hasProperty("id", is("1"))));
+            MatcherAssert.assertThat(resources, hasItem(hasProperty("id", is("2"))));
+        }
     }
 
-    public void testSampleResourcePluginCallsDefaultPlugin() throws IOException {
+    public void testSampleResourcePluginCallsDefaultRACPlugin() throws IOException {
         createIndex(SAMPLE_TEST_INDEX);
         indexSampleDocuments();
 
-        ResourceAccessControlPlugin racPlugin = SampleTestResourcePlugin.GuiceHolder.getResourceService().getResourceAccessControlPlugin();
+        ResourceAccessControlPlugin racPlugin = TestResourcePlugin.GuiceHolder.getResourceService().getResourceAccessControlPlugin();
         MatcherAssert.assertThat(racPlugin.getClass(), is(DefaultResourceAccessControlPlugin.class));
 
-        Set<TestResource> resources = racPlugin.getAccessibleResourcesForCurrentUser(SAMPLE_TEST_INDEX, TestResource.class);
+        Set<TestResourcePlugin.TestResource> resources = racPlugin.getAccessibleResourcesForCurrentUser(
+            SAMPLE_TEST_INDEX,
+            TestResourcePlugin.TestResource.class
+        );
 
         assertNotNull(resources);
         MatcherAssert.assertThat(resources, hasSize(2));
@@ -89,19 +89,4 @@ private void indexSampleDocuments() throws IOException {
             client.admin().indices().prepareRefresh(SAMPLE_TEST_INDEX).get();
         }
     }
-
-    public static class TestResource {
-        public String id;
-        public String name;
-
-        public TestResource() {}
-
-        public String getId() {
-            return id;
-        }
-
-        public String getName() {
-            return name;
-        }
-    }
 }
diff --git a/...es/fallback/SampleTestResourcePlugin.java → ...esources/fallback/TestResourcePlugin.java b/...es/fallback/SampleTestResourcePlugin.java → ...esources/fallback/TestResourcePlugin.java
@@ -21,7 +21,7 @@
 import java.util.List;
 
 // Sample test resource plugin
-public class SampleTestResourcePlugin extends Plugin implements ResourcePlugin {
+public class TestResourcePlugin extends Plugin implements ResourcePlugin {
 
     public static final String SAMPLE_TEST_INDEX = ".sample_test_resource";
 
@@ -76,4 +76,19 @@ public void start() {}
         public void stop() {}
 
     }
+
+    public static class TestResource {
+        public String id;
+        public String name;
+
+        public TestResource() {}
+
+        public String getId() {
+            return id;
+        }
+
+        public String getName() {
+            return name;
+        }
+    }
 }
diff --git a/...a/org/opensearch/accesscontrol/resources/fallback/DefaultResourceAccessControlPlugin.java b/...a/org/opensearch/accesscontrol/resources/fallback/DefaultResourceAccessControlPlugin.java
@@ -8,6 +8,8 @@
 
 package org.opensearch.accesscontrol.resources.fallback;
 
+import com.fasterxml.jackson.databind.ObjectMapper;
+
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
 import org.opensearch.OpenSearchException;
@@ -27,13 +29,12 @@
 import org.opensearch.search.builder.SearchSourceBuilder;
 import org.opensearch.threadpool.ThreadPool;
 
-import java.lang.reflect.Field;
 import java.security.AccessController;
-import java.security.PrivilegedAction;
+import java.security.PrivilegedActionException;
+import java.security.PrivilegedExceptionAction;
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.HashSet;
-import java.util.Map;
 import java.util.Set;
 import java.util.stream.Stream;
 
@@ -42,14 +43,14 @@
  *
  * @opensearch.experimental
  */
-@SuppressWarnings("removal")
 public class DefaultResourceAccessControlPlugin extends Plugin implements ResourceAccessControlPlugin {
 
     private static final Logger log = LogManager.getLogger(DefaultResourceAccessControlPlugin.class);
 
-    private Client client;
+    private final Client client;
 
     private final ThreadPool threadPool;
+    private static final ObjectMapper objectMapper = new ObjectMapper();
 
     @Inject
     public DefaultResourceAccessControlPlugin(Client client, ThreadPool threadPool) {
@@ -79,7 +80,7 @@ public <T> Set<T> getAccessibleResourcesForCurrentUser(String resourceIndex, Cla
             SearchHit[] searchHits = searchResponse.getHits().getHits();
 
             while (searchHits != null && searchHits.length > 0) {
-                parseAndAddToDocuments(Arrays.stream(searchHits).map(SearchHit::getSourceAsMap), clazz, documents);
+                parseAndAddToDocuments(Arrays.stream(searchHits), clazz, documents);
 
                 final SearchScrollRequest scrollRequest = new SearchScrollRequest(scrollId);
                 scrollRequest.scroll(scrollTimeout);
@@ -112,26 +113,17 @@ private SearchRequest buildSearchRequest(String resourceIndex, TimeValue scrollT
         return searchRequest;
     }
 
-    private static <T> T parse(Map<String, Object> sourceMap, Class<T> clazz) {
-        return AccessController.doPrivileged((PrivilegedAction<T>) () -> {
-            try {
-                final T instance = clazz.getDeclaredConstructor().newInstance();
-                for (Field field : clazz.getDeclaredFields()) {
-                    field.setAccessible(true);
-                    Object value = sourceMap.get(field.getName());
-                    if (value != null) {
-                        field.set(instance, value);
-                    }
-                }
-                return instance;
-            } catch (Exception e) {
-                throw new OpenSearchException("Failed to parse source map into " + clazz.getName(), e);
-            }
-        });
+    private static <T> T parse(String source, Class<T> clazz) {
+        try {
+            return AccessController.doPrivileged((PrivilegedExceptionAction<T>) () -> objectMapper.readValue(source, clazz));
+        } catch (PrivilegedActionException e) {
+            log.error("Error parsing source: {}", e.toString());
+            throw new OpenSearchException("Error parsing source into : " + clazz.getName(), e.getException());
+        }
     }
 
-    private static <T> void parseAndAddToDocuments(Stream<Map<String, Object>> searchHits, Class<T> clazz, Set<T> documents) {
-        searchHits.map(hit -> parse(hit, clazz)).forEach(documents::add);
+    private static <T> void parseAndAddToDocuments(Stream<SearchHit> searchHits, Class<T> clazz, Set<T> documents) {
+        searchHits.map(hit -> parse(hit.getSourceAsString(), clazz)).forEach(documents::add);
     }
 
 }
diff --git a/server/src/main/resources/org/opensearch/bootstrap/security.policy b/server/src/main/resources/org/opensearch/bootstrap/security.policy
@@ -48,14 +48,16 @@ grant codeBase "${codebase.opensearch}" {
   permission java.lang.RuntimePermission "setContextClassLoader";
   // needed for SPI class loading
   permission java.lang.RuntimePermission "accessDeclaredMembers";
+  // needed for DefaultResourceAccessPlugin
+  permission java.lang.reflect.ReflectPermission "suppressAccessChecks";
   permission org.opensearch.secure_sm.ThreadContextPermission "markAsSystemContext";
   permission org.opensearch.secure_sm.ThreadContextPermission "stashWithOrigin";
 };
 
-//// Permission specific to DefaultResourceAccessControlPlugin
-grant codeBase "${codebase.opensearch}" {
-  // ReflectPermission granted only for the DefaultResourceAccessControlPlugin to allow parsing the Resources
+//// Permission specific to jackson-databind used in DefaultResourceAccessPlugin
+grant codeBase "${codebase.jackson-databind}" {
   permission java.lang.reflect.ReflectPermission "suppressAccessChecks";
+  permission java.lang.RuntimePermission "accessDeclaredMembers";
 };
 
 //// Very special jar permissions: