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/filter/RestPathMatchesTest.java b/src/test/java/org/opensearch/security/filter/RestPathMatchesTests.java similarity index 70% rename from src/test/java/org/opensearch/security/filter/RestPathMatchesTest.java rename to src/test/java/org/opensearch/security/filter/RestPathMatchesTests.java index 9a5335bdb7..eed095c9b6 100644 --- a/src/test/java/org/opensearch/security/filter/RestPathMatchesTest.java +++ b/src/test/java/org/opensearch/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/src/test/java/org/opensearch/security/filter/SecurityFilterTest.java b/src/test/java/org/opensearch/security/filter/SecurityFilterTests.java similarity index 97% rename from src/test/java/org/opensearch/security/filter/SecurityFilterTest.java rename to src/test/java/org/opensearch/security/filter/SecurityFilterTests.java index ea2978302e..58a12a84a8 100644 --- a/src/test/java/org/opensearch/security/filter/SecurityFilterTest.java +++ b/src/test/java/org/opensearch/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/src/test/java/org/opensearch/security/filter/SecurityRestFilterTest.java b/src/test/java/org/opensearch/security/filter/SecurityRestFilterTests.java similarity index 99% rename from src/test/java/org/opensearch/security/filter/SecurityRestFilterTest.java rename to src/test/java/org/opensearch/security/filter/SecurityRestFilterTests.java index 692a950fb1..5adcadb1f2 100644 --- a/src/test/java/org/opensearch/security/filter/SecurityRestFilterTest.java +++ b/src/test/java/org/opensearch/security/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 index 3c8145c393..2f6189bab2 100644 --- 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; - @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 mappedRoles = Collections.singleton("role1"); + final User user = mock(User.class); + final Set mappedRoles = Collections.singleton("role1"); when(configModel.mapSecurityRoles(any(), any())).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 +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); } }