Enhance RestLayerPrivilegesEvaluator Code Coverage (#3218)

Enhance RestLayerPrivilegesEvaluator Code Coverage Signed-off-by: Stephen Crawford <steecraw@amazon.com> Signed-off-by: Peter Nied <petern@amazon.com> Signed-off-by: Darshit Chanpura <dchanp@amazon.com> Co-authored-by: Peter Nied <petern@amazon.com> Co-authored-by: Darshit Chanpura <dchanp@amazon.com>
opensearch-project · Aug 23, 2023 · 46dfd84 · 46dfd84
1 parent 683c4f5
commit 46dfd84
Show file tree

Hide file tree

Showing 6 changed files with 106 additions and 93 deletions.
diff --git a/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java b/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java
@@ -1023,7 +1023,7 @@ public Collection<Object> createComponents(
             principalExtractor = ReflectionHelper.instantiatePrincipalExtractor(principalExtractorClass);
         }
 
-        restLayerEvaluator = new RestLayerPrivilegesEvaluator(clusterService, threadPool, auditLog, cih, namedXContentRegistry);
+        restLayerEvaluator = new RestLayerPrivilegesEvaluator(clusterService, threadPool);
 
         securityRestHandler = new SecurityRestFilter(
             backendRegistry,

diff --git a/src/main/java/org/opensearch/security/privileges/RestLayerPrivilegesEvaluator.java b/src/main/java/org/opensearch/security/privileges/RestLayerPrivilegesEvaluator.java
@@ -12,7 +12,6 @@
 package org.opensearch.security.privileges;
 
 import java.util.Set;
-import java.util.concurrent.atomic.AtomicReference;
 
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
@@ -22,11 +21,7 @@
 import org.opensearch.cluster.service.ClusterService;
 import org.opensearch.core.common.transport.TransportAddress;
 import org.opensearch.common.util.concurrent.ThreadContext;
-import org.opensearch.core.xcontent.NamedXContentRegistry;
-import org.opensearch.security.auditlog.AuditLog;
-import org.opensearch.security.configuration.ClusterInfoHolder;
 import org.opensearch.security.securityconf.ConfigModel;
-import org.opensearch.security.securityconf.DynamicConfigModel;
 import org.opensearch.security.securityconf.SecurityRoles;
 import org.opensearch.security.support.ConfigConstants;
 import org.opensearch.security.user.User;
@@ -35,49 +30,28 @@
 public class RestLayerPrivilegesEvaluator {
     protected final Logger log = LogManager.getLogger(this.getClass());
     private final ClusterService clusterService;
-    private final AuditLog auditLog;
     private ThreadContext threadContext;
-    private final ClusterInfoHolder clusterInfoHolder;
     private ConfigModel configModel;
-    private DynamicConfigModel dcm;
-    private final AtomicReference<NamedXContentRegistry> namedXContentRegistry;
-
-    public RestLayerPrivilegesEvaluator(
-        final ClusterService clusterService,
-        final ThreadPool threadPool,
-        AuditLog auditLog,
-        final ClusterInfoHolder clusterInfoHolder,
-        AtomicReference<NamedXContentRegistry> namedXContentRegistry
-    ) {
-        this.clusterService = clusterService;
-        this.auditLog = auditLog;
 
+    public RestLayerPrivilegesEvaluator(final ClusterService clusterService, final ThreadPool threadPool) {
+        this.clusterService = clusterService;
         this.threadContext = threadPool.getThreadContext();
-
-        this.clusterInfoHolder = clusterInfoHolder;
-        this.namedXContentRegistry = namedXContentRegistry;
     }
 
     @Subscribe
-    public void onConfigModelChanged(ConfigModel configModel) {
+    public void onConfigModelChanged(final ConfigModel configModel) {
         this.configModel = configModel;
     }
 
-    @Subscribe
-    public void onDynamicConfigModelChanged(DynamicConfigModel dcm) {
-        this.dcm = dcm;
-    }
-
-    private SecurityRoles getSecurityRoles(Set<String> roles) {
+    SecurityRoles getSecurityRoles(final Set<String> roles) {
         return configModel.getSecurityRoles().filter(roles);
     }
 
-    public boolean isInitialized() {
-        return configModel != null && configModel.getSecurityRoles() != null && dcm != null;
+    boolean isInitialized() {
+        return configModel != null && configModel.getSecurityRoles() != null;
     }
 
-    public PrivilegesEvaluatorResponse evaluate(final User user, Set<String> actions) {
-
+    public PrivilegesEvaluatorResponse evaluate(final User user, final Set<String> actions) {
         if (!isInitialized()) {
             throw new OpenSearchSecurityException("OpenSearch Security is not initialized.");
         }
@@ -86,7 +60,7 @@ public PrivilegesEvaluatorResponse evaluate(final User user, Set<String> actions
 
         final TransportAddress caller = threadContext.getTransient(ConfigConstants.OPENDISTRO_SECURITY_REMOTE_ADDRESS);
 
-        Set<String> mappedRoles = mapRoles(user, caller);
+        final Set<String> mappedRoles = mapRoles(user, caller);
 
         presponse.resolvedSecurityRoles.addAll(mappedRoles);
         final SecurityRoles securityRoles = getSecurityRoles(mappedRoles);
@@ -98,7 +72,7 @@ public PrivilegesEvaluatorResponse evaluate(final User user, Set<String> actions
             log.debug("Mapped roles: {}", mappedRoles.toString());
         }
 
-        for (String action : actions) {
+        for (final String action : actions) {
             if (!securityRoles.impliesClusterPermissionPermission(action)) {
                 presponse.missingPrivileges.add(action);
                 presponse.allowed = false;
@@ -123,8 +97,7 @@ public PrivilegesEvaluatorResponse evaluate(final User user, Set<String> actions
         return presponse;
     }
 
-    public Set<String> mapRoles(final User user, final TransportAddress caller) {
+    Set<String> mapRoles(final User user, final TransportAddress caller) {
         return this.configModel.mapSecurityRoles(user, caller);
     }
-
 }
diff --git a/.../security/filter/RestPathMatchesTest.java → ...security/filter/RestPathMatchesTests.java b/.../security/filter/RestPathMatchesTest.java → ...security/filter/RestPathMatchesTests.java
@@ -18,7 +18,7 @@
 import static org.junit.jupiter.api.Assertions.assertTrue;
 import static org.mockito.Mockito.mock;
 
-public class RestPathMatchesTest {
+public class RestPathMatchesTests {
     Method restPathMatches;
     SecurityRestFilter securityRestFilter;
 
@@ -63,4 +63,25 @@ public void testDifferentPathSegments() throws InvocationTargetException, Illega
         String handlerPath = "_plugins/security/api/x/y";
         assertFalse((Boolean) restPathMatches.invoke(securityRestFilter, requestPath, handlerPath));
     }
+
+    @Test
+    public void testRequestPathWithNamedParam() throws InvocationTargetException, IllegalAccessException {
+        String requestPath = "_plugins/security/api/123/y";
+        String handlerPath = "_plugins/security/api/{id}/z";
+        assertFalse((Boolean) restPathMatches.invoke(securityRestFilter, requestPath, handlerPath));
+    }
+
+    @Test
+    public void testRequestPathMismatch() throws InvocationTargetException, IllegalAccessException {
+        String requestPath = "_plugins/security/api/x/y";
+        String handlerPath = "_plugins/security/api/z/y";
+        assertFalse((Boolean) restPathMatches.invoke(securityRestFilter, requestPath, handlerPath));
+    }
+
+    @Test
+    public void testRequestPathWithExtraSegments() throws InvocationTargetException, IllegalAccessException {
+        String requestPath = "_plugins/security/api/x/y/z";
+        String handlerPath = "_plugins/security/api/x/y";
+        assertFalse((Boolean) restPathMatches.invoke(securityRestFilter, requestPath, handlerPath));
+    }
 }
diff --git a/...h/security/filter/SecurityFilterTest.java → .../security/filter/SecurityFilterTests.java b/...h/security/filter/SecurityFilterTest.java → .../security/filter/SecurityFilterTests.java
@@ -47,12 +47,12 @@
 import static org.mockito.Mockito.when;
 
 @RunWith(Parameterized.class)
-public class SecurityFilterTest {
+public class SecurityFilterTests {
 
     private final Settings settings;
     private final WildcardMatcher expected;
 
-    public SecurityFilterTest(Settings settings, WildcardMatcher expected) {
+    public SecurityFilterTests(Settings settings, WildcardMatcher expected) {
         this.settings = settings;
         this.expected = expected;
     }

diff --git a/...curity/filter/SecurityRestFilterTest.java → ...urity/filter/SecurityRestFilterTests.java b/...curity/filter/SecurityRestFilterTest.java → ...urity/filter/SecurityRestFilterTests.java
@@ -27,7 +27,7 @@
  * Currently tests that the whitelisting functionality works correctly.
  * Uses the test/resources/restapi folder for setup.
  */
-public class SecurityRestFilterTest extends AbstractRestApiUnitTest {
+public class SecurityRestFilterTests extends AbstractRestApiUnitTest {
 
     private RestHelper.HttpResponse response;
 

diff --git a/src/test/java/org/opensearch/security/privileges/RestLayerPrivilegesEvaluatorTest.java b/src/test/java/org/opensearch/security/privileges/RestLayerPrivilegesEvaluatorTest.java
@@ -13,76 +13,77 @@
 
 import java.util.Collections;
 import java.util.Set;
-import java.util.concurrent.atomic.AtomicReference;
 
+import org.apache.logging.log4j.Level;
+import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
+import org.apache.logging.log4j.core.config.Configurator;
+import org.hamcrest.Matchers;
+import org.junit.After;
 import org.junit.Before;
 import org.junit.Test;
+import org.junit.runner.RunWith;
 import org.mockito.Mock;
-import org.mockito.MockitoAnnotations;
-
+import org.mockito.junit.MockitoJUnitRunner;
+import org.mockito.quality.Strictness;
 import org.opensearch.OpenSearchSecurityException;
+import org.opensearch.cluster.node.DiscoveryNode;
 import org.opensearch.cluster.service.ClusterService;
 import org.opensearch.common.settings.Settings;
 import org.opensearch.common.util.concurrent.ThreadContext;
-import org.opensearch.core.xcontent.NamedXContentRegistry;
-import org.opensearch.security.auditlog.AuditLog;
-import org.opensearch.security.configuration.ClusterInfoHolder;
 import org.opensearch.security.securityconf.ConfigModel;
-import org.opensearch.security.securityconf.DynamicConfigModel;
 import org.opensearch.security.securityconf.SecurityRoles;
 import org.opensearch.security.user.User;
 import org.opensearch.threadpool.ThreadPool;
 
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertFalse;
-import static org.junit.Assert.assertNotNull;
-import static org.junit.Assert.assertTrue;
-import static org.mockito.Mockito.any;
+import static org.hamcrest.Matchers.equalTo;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.junit.Assert.assertThrows;
+import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.never;
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.verifyNoInteractions;
 import static org.mockito.Mockito.when;
+import static org.mockito.Mockito.withSettings;
 
+@RunWith(MockitoJUnitRunner.class)
 public class RestLayerPrivilegesEvaluatorTest {
 
-    @Mock
+    @Mock(strictness = Mock.Strictness.LENIENT)
     private ClusterService clusterService;
     @Mock
     private ThreadPool threadPool;
     @Mock
-    private AtomicReference<NamedXContentRegistry> namedXContentRegistry;
-    @Mock
     private ConfigModel configModel;
-    @Mock
-    private DynamicConfigModel dcm;
-    @Mock
-    private PrivilegesEvaluatorResponse presponse;
-    @Mock
-    private Logger log;
 
     private RestLayerPrivilegesEvaluator privilegesEvaluator;
 
     private static final User TEST_USER = new User("test_user");
 
-    @Before
-    public void setUp() throws InstantiationException, IllegalAccessException {
-        MockitoAnnotations.openMocks(this);
+    private void setLoggingLevel(final Level level) {
+        final Logger restLayerPrivilegesEvaluatorLogger = LogManager.getLogger(RestLayerPrivilegesEvaluator.class);
+        Configurator.setLevel(restLayerPrivilegesEvaluatorLogger, level);
+    }
 
-        ThreadContext context = new ThreadContext(Settings.EMPTY);
-        when(threadPool.getThreadContext()).thenReturn(context);
+    @Before
+    public void setUp() {
+        when(threadPool.getThreadContext()).thenReturn(new ThreadContext(Settings.EMPTY));
 
+        when(clusterService.localNode()).thenReturn(mock(DiscoveryNode.class, withSettings().strictness(Strictness.LENIENT)));
         privilegesEvaluator = new RestLayerPrivilegesEvaluator(
             clusterService,
-            threadPool,
-            mock(AuditLog.class),
-            mock(ClusterInfoHolder.class),
-            namedXContentRegistry
+            threadPool
         );
-        privilegesEvaluator.onConfigModelChanged(configModel);
-        privilegesEvaluator.onDynamicConfigModelChanged(dcm);
+        privilegesEvaluator.onConfigModelChanged(configModel); // Defaults to the mocked config model
+        verify(threadPool).getThreadContext(); // Called during construction of RestLayerPrivilegesEvaluator
+        setLoggingLevel(Level.DEBUG); // Enable debug logging scenarios for verification
+    }
 
-        when(log.isDebugEnabled()).thenReturn(false);
+    @After
+    public void after() {
+        setLoggingLevel(Level.INFO);
     }
 
     @Test
@@ -95,28 +96,45 @@ public void testEvaluate_Initialized_Success() {
 
         PrivilegesEvaluatorResponse response = privilegesEvaluator.evaluate(TEST_USER, Set.of(action));
 
-        assertNotNull(response);
-        assertFalse(response.isAllowed());
-        assertFalse(response.getMissingPrivileges().isEmpty());
-        assertTrue(response.getResolvedSecurityRoles().isEmpty());
+        assertThat(response.isAllowed(), equalTo(false));
+        assertThat(response.getMissingPrivileges(), equalTo(Set.of(action)));
+        assertThat(response.getResolvedSecurityRoles(), Matchers.empty());
         verify(configModel, times(3)).getSecurityRoles();
     }
 
-    @Test(expected = OpenSearchSecurityException.class)
-    public void testEvaluate_NotInitialized_ExceptionThrown() throws Exception {
-        String action = "action";
-        privilegesEvaluator.evaluate(TEST_USER, Set.of(action));
+    @Test
+    public void testEvaluate_NotInitialized_NullModel_ExceptionThrown() {
+        // Null out the config model
+        privilegesEvaluator.onConfigModelChanged(null);
+        final OpenSearchSecurityException exception = assertThrows(
+            OpenSearchSecurityException.class,
+            () -> privilegesEvaluator.evaluate(TEST_USER, null)
+        );
+        assertThat(exception.getMessage(), equalTo("OpenSearch Security is not initialized."));
+        verify(configModel, never()).getSecurityRoles();
+    }
+
+    @Test
+    public void testEvaluate_NotInitialized_NoSecurityRoles_ExceptionThrown() {
+        final OpenSearchSecurityException exception = assertThrows(
+            OpenSearchSecurityException.class,
+            () -> privilegesEvaluator.evaluate(TEST_USER, null)
+        );
+        assertThat(exception.getMessage(), equalTo("OpenSearch Security is not initialized."));
+        verify(configModel).getSecurityRoles();
     }
 
     @Test
     public void testMapRoles_ReturnsMappedRoles() {
-        Set<String> mappedRoles = Collections.singleton("role1");
+        final User user = mock(User.class);
+        final Set<String> mappedRoles = Collections.singleton("role1");
         when(configModel.mapSecurityRoles(any(), any())).thenReturn(mappedRoles);
 
-        Set<String> result = privilegesEvaluator.mapRoles(any(), any());
+        final Set<String> result = privilegesEvaluator.mapRoles(user, null);
 
-        assertEquals(mappedRoles, result);
-        verify(configModel).mapSecurityRoles(any(), any());
+        assertThat(result, equalTo(mappedRoles));
+        verifyNoInteractions(user);
+        verify(configModel).mapSecurityRoles(user, null);
     }
 
     @Test
@@ -129,8 +147,8 @@ public void testEvaluate_Successful_NewPermission() {
 
         PrivilegesEvaluatorResponse response = privilegesEvaluator.evaluate(TEST_USER, Set.of(action));
 
-        assertTrue(response.allowed);
-        verify(securityRoles).impliesClusterPermissionPermission(any());
+        assertThat(response.allowed, equalTo(true));
+        verify(securityRoles).impliesClusterPermissionPermission(action);
     }
 
     @Test
@@ -143,8 +161,9 @@ public void testEvaluate_Successful_LegacyPermission() {
 
         PrivilegesEvaluatorResponse response = privilegesEvaluator.evaluate(TEST_USER, Set.of(action));
 
-        assertTrue(response.allowed);
-        verify(securityRoles).impliesClusterPermissionPermission(any());
+        assertThat(response.allowed, equalTo(true));
+        verify(securityRoles).impliesClusterPermissionPermission(action);
+        verify(configModel, times(3)).getSecurityRoles();
     }
 
     @Test
@@ -157,7 +176,7 @@ public void testEvaluate_Unsuccessful() {
 
         PrivilegesEvaluatorResponse response = privilegesEvaluator.evaluate(TEST_USER, Set.of(action));
 
-        assertFalse(response.allowed);
-        verify(securityRoles).impliesClusterPermissionPermission(any());
+        assertThat(response.allowed, equalTo(false));
+        verify(securityRoles).impliesClusterPermissionPermission(action);
     }
 }