From 8bb213c99acc4fdbf88efcf497ab3acd585954ef Mon Sep 17 00:00:00 2001 From: Peter Nied Date: Tue, 22 Aug 2023 11:28:21 -0500 Subject: [PATCH] Increase coverage target In RestLayerPrivilegesEvaluator there was a number of unused parameters removed those to trim down the test requirements. For the test cases switched to assertThat for easy of reading, enabled debug logging to exercise debug logging code, adjusted usages of mocks for cleaner test flow in a couple of spaces. Verified locally with the following command and then checked the jacoco report under build/reports/jacoco/test/html/... ./gradlew jacocoTestReport test --tests \ org.opensearch.security.privileges.RestLayerPrivilegesEvaluatorTest Signed-off-by: Peter Nied --- .../security/OpenSearchSecurityPlugin.java | 2 +- .../RestLayerPrivilegesEvaluator.java | 47 ++----- .../RestLayerPrivilegesEvaluatorTest.java | 117 +++++++++++------- 3 files changed, 82 insertions(+), 84 deletions(-) diff --git a/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java b/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java index a43afcb187..c7c666bdaf 100644 --- a/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java +++ b/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java @@ -1023,7 +1023,7 @@ public Collection 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 index 8602fb91e6..dc95e98d11 100644 --- 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; - - public RestLayerPrivilegesEvaluator( - final ClusterService clusterService, - final ThreadPool threadPool, - AuditLog auditLog, - final ClusterInfoHolder clusterInfoHolder, - AtomicReference 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 roles) { + SecurityRoles getSecurityRoles(final Set 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 actions) { - + public PrivilegesEvaluatorResponse evaluate(final User user, final Set actions) { if (!isInitialized()) { throw new OpenSearchSecurityException("OpenSearch Security is not initialized."); } @@ -86,7 +60,7 @@ public PrivilegesEvaluatorResponse evaluate(final User user, Set actions final TransportAddress caller = threadContext.getTransient(ConfigConstants.OPENDISTRO_SECURITY_REMOTE_ADDRESS); - Set mappedRoles = mapRoles(user, caller); + final Set mappedRoles = mapRoles(user, caller); presponse.resolvedSecurityRoles.addAll(mappedRoles); final SecurityRoles securityRoles = getSecurityRoles(mappedRoles); @@ -98,7 +72,7 @@ public PrivilegesEvaluatorResponse evaluate(final User user, Set 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 actions return presponse; } - public Set mapRoles(final User user, final TransportAddress caller) { + Set mapRoles(final User user, final TransportAddress caller) { return this.configModel.mapSecurityRoles(user, caller); } - } diff --git a/src/test/java/org/opensearch/security/privileges/RestLayerPrivilegesEvaluatorTest.java b/src/test/java/org/opensearch/security/privileges/RestLayerPrivilegesEvaluatorTest.java index 3c8145c393..145bf56d9d 100644 --- a/src/test/java/org/opensearch/security/privileges/RestLayerPrivilegesEvaluatorTest.java +++ b/src/test/java/org/opensearch/security/privileges/RestLayerPrivilegesEvaluatorTest.java @@ -13,76 +13,83 @@ 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.core.common.transport.TransportAddress; 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; - @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"); + private void setLoggingLevel(final Level level) { + final Logger restLayerPrivilegesEvaluatorLogger = LogManager.getLogger(RestLayerPrivilegesEvaluator.class); + Configurator.setLevel(restLayerPrivilegesEvaluatorLogger, level); + } + @Before public void setUp() throws InstantiationException, IllegalAccessException { - MockitoAnnotations.openMocks(this); - - ThreadContext context = new ThreadContext(Settings.EMPTY); - when(threadPool.getThreadContext()).thenReturn(context); + 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 +102,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() throws Exception { + // 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() throws Exception { + 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 mappedRoles = Collections.singleton("role1"); - when(configModel.mapSecurityRoles(any(), any())).thenReturn(mappedRoles); + final User user = mock(User.class); + final Set mappedRoles = Collections.singleton("role1"); + when(configModel.mapSecurityRoles(user, any(TransportAddress.class))).thenReturn(mappedRoles); - Set result = privilegesEvaluator.mapRoles(any(), any()); + final Set 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 +153,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 +167,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 +182,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); } }