From e10c2fee08a99fe0d8a88f7925bab339ee0968fe Mon Sep 17 00:00:00 2001 From: Darshit Chanpura Date: Tue, 5 Nov 2024 17:24:02 -0500 Subject: [PATCH] Adds and updates tests Signed-off-by: Darshit Chanpura --- .../security/rest/AuthZinRestLayerTests.java | 14 +++ .../opensearch/security/rest/WhoAmITests.java | 18 +++- .../framework/cluster/TestRestClient.java | 4 + .../security/filter/SecurityRestFilter.java | 2 +- .../filter/SecurityRestFilterUnitTests.java | 86 +++++++++++++++++++ 5 files changed, 120 insertions(+), 4 deletions(-) diff --git a/src/integrationTest/java/org/opensearch/security/rest/AuthZinRestLayerTests.java b/src/integrationTest/java/org/opensearch/security/rest/AuthZinRestLayerTests.java index c38ecbb611..72f60166e4 100644 --- a/src/integrationTest/java/org/opensearch/security/rest/AuthZinRestLayerTests.java +++ b/src/integrationTest/java/org/opensearch/security/rest/AuthZinRestLayerTests.java @@ -26,11 +26,13 @@ import org.opensearch.test.framework.audit.AuditLogsRule; import org.opensearch.test.framework.cluster.ClusterManager; import org.opensearch.test.framework.cluster.LocalCluster; +import org.opensearch.test.framework.cluster.RestClientException; import org.opensearch.test.framework.cluster.TestRestClient; import org.opensearch.test.framework.testplugins.dummy.CustomLegacyTestPlugin; import org.opensearch.test.framework.testplugins.dummyprotected.CustomRestProtectedTestPlugin; import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; import static org.opensearch.rest.RestRequest.Method.GET; import static org.opensearch.rest.RestRequest.Method.POST; @@ -40,6 +42,7 @@ import static org.opensearch.test.framework.TestSecurityConfig.AuthcDomain.AUTHC_HTTPBASIC_INTERNAL; import static org.opensearch.test.framework.audit.AuditMessagePredicate.privilegePredicateRESTLayer; import static org.opensearch.test.framework.audit.AuditMessagePredicate.privilegePredicateTransportLayer; +import static org.junit.Assert.assertThrows; @RunWith(com.carrotsearch.randomizedtesting.RandomizedRunner.class) @ThreadLeakScope(ThreadLeakScope.Scope.NONE) @@ -111,6 +114,17 @@ public void testAccessDeniedForUserWithNoPermissions() { } } + @Test + public void testShouldFailWithoutPermForPathWithoutLeadingSlashes() { + try (TestRestClient client = cluster.getRestClient(NO_PERM)) { + + // Protected Routes plugin + Exception exception = assertThrows(RestClientException.class, () -> { client.getWithoutLeadingSlash(PROTECTED_API); }); + + assertThat(exception.getMessage(), containsString("Error occured during HTTP request execution")); + } + } + /** AuthZ in REST Layer check */ @Test diff --git a/src/integrationTest/java/org/opensearch/security/rest/WhoAmITests.java b/src/integrationTest/java/org/opensearch/security/rest/WhoAmITests.java index c41b5f4cda..742ea89a16 100644 --- a/src/integrationTest/java/org/opensearch/security/rest/WhoAmITests.java +++ b/src/integrationTest/java/org/opensearch/security/rest/WhoAmITests.java @@ -37,14 +37,13 @@ import org.opensearch.test.framework.audit.AuditLogsRule; import org.opensearch.test.framework.cluster.ClusterManager; import org.opensearch.test.framework.cluster.LocalCluster; +import org.opensearch.test.framework.cluster.RestClientException; import org.opensearch.test.framework.cluster.TestRestClient; import joptsimple.internal.Strings; import static org.hamcrest.MatcherAssert.assertThat; -import static org.hamcrest.Matchers.equalTo; -import static org.hamcrest.Matchers.greaterThan; -import static org.hamcrest.Matchers.lessThan; +import static org.hamcrest.Matchers.*; import static org.opensearch.rest.RestRequest.Method.GET; import static org.opensearch.security.auditlog.impl.AuditCategory.GRANTED_PRIVILEGES; import static org.opensearch.security.auditlog.impl.AuditCategory.MISSING_PRIVILEGES; @@ -52,6 +51,7 @@ import static org.opensearch.test.framework.audit.AuditMessagePredicate.grantedPrivilege; import static org.opensearch.test.framework.audit.AuditMessagePredicate.privilegePredicateRESTLayer; import static org.opensearch.test.framework.audit.AuditMessagePredicate.userAuthenticatedPredicate; +import static org.junit.Assert.assertThrows; import static org.junit.Assert.assertTrue; @RunWith(com.carrotsearch.randomizedtesting.RandomizedRunner.class) @@ -138,6 +138,18 @@ public void testWhoAmIWithoutGetPermissions() { } } + @Test + public void testWhoAmIWithoutGetPermissionsWithoutLeadingSlashInPath() { + try (TestRestClient client = cluster.getRestClient(WHO_AM_I_NO_PERM)) { + Exception exception = assertThrows( + RestClientException.class, + () -> { client.getWithoutLeadingSlash(WHOAMI_PROTECTED_ENDPOINT); } + ); + + assertThat(exception.getMessage(), containsString("Error occured during HTTP request execution")); + } + } + @Test public void testWhoAmIPost() { try (TestRestClient client = cluster.getRestClient(WHO_AM_I)) { diff --git a/src/integrationTest/java/org/opensearch/test/framework/cluster/TestRestClient.java b/src/integrationTest/java/org/opensearch/test/framework/cluster/TestRestClient.java index e7355711fc..c751aae083 100644 --- a/src/integrationTest/java/org/opensearch/test/framework/cluster/TestRestClient.java +++ b/src/integrationTest/java/org/opensearch/test/framework/cluster/TestRestClient.java @@ -110,6 +110,10 @@ public HttpResponse get(String path, Header... headers) { return executeRequest(new HttpGet(getHttpServerUri() + "/" + path), headers); } + public void getWithoutLeadingSlash(String path, Header... headers) { + executeRequest(new HttpGet(getHttpServerUri() + path), headers); + } + public HttpResponse getAuthInfo(Header... headers) { return executeRequest(new HttpGet(getHttpServerUri() + "/_opendistro/_security/authinfo?pretty"), headers); } diff --git a/src/main/java/org/opensearch/security/filter/SecurityRestFilter.java b/src/main/java/org/opensearch/security/filter/SecurityRestFilter.java index a4c12bc28d..0fc18588bb 100644 --- a/src/main/java/org/opensearch/security/filter/SecurityRestFilter.java +++ b/src/main/java/org/opensearch/security/filter/SecurityRestFilter.java @@ -335,7 +335,7 @@ public void onAllowlistingSettingChanged(AllowlistingSettings allowlistingSettin * @param handlerPath The path from the {@link RestHandler.Route} * @return true if the request path matches the route */ - private boolean restPathMatches(String requestPath, String handlerPath) { + boolean restPathMatches(String requestPath, String handlerPath) { // Trim leading and trailing slashes requestPath = requestPath.replaceAll("^/+", "").replaceAll("/+$", ""); handlerPath = handlerPath.replaceAll("^/+", "").replaceAll("/+$", ""); diff --git a/src/test/java/org/opensearch/security/filter/SecurityRestFilterUnitTests.java b/src/test/java/org/opensearch/security/filter/SecurityRestFilterUnitTests.java index 1727fddcc3..c387dd68ac 100644 --- a/src/test/java/org/opensearch/security/filter/SecurityRestFilterUnitTests.java +++ b/src/test/java/org/opensearch/security/filter/SecurityRestFilterUnitTests.java @@ -15,6 +15,8 @@ import org.junit.Before; import org.junit.Test; +import org.junit.experimental.runners.Enclosed; +import org.junit.runner.RunWith; import org.opensearch.client.node.NodeClient; import org.opensearch.common.settings.Settings; @@ -41,6 +43,7 @@ import static org.mockito.Mockito.spy; import static org.mockito.Mockito.verify; +@RunWith(Enclosed.class) public class SecurityRestFilterUnitTests { SecurityRestFilter sf; @@ -100,4 +103,87 @@ public void testDoesCallDelegateOnSuccessfulAuthorization() throws Exception { verify(testRestHandlerSpy).handleRequest(any(), any(), any()); } + + // Mini Test Suite for restPathMatches + public static class RestPathMatchesTests { + + private SecurityRestFilter sf; + + @Before + public void setUp() { + sf = new SecurityRestFilter( + mock(BackendRegistry.class), + mock(RestLayerPrivilegesEvaluator.class), + mock(AuditLog.class), + mock(ThreadPool.class), + mock(PrincipalExtractor.class), + Settings.EMPTY, + mock(Path.class), + mock(CompatConfig.class) + ); + } + + @Test + public void testExactPathMatch() { + String requestPath = "/api/v1/resource"; + String handlerPath = "/api/v1/resource"; + assertTrue(sf.restPathMatches(requestPath, handlerPath)); + } + + @Test + public void testPathsDoNotMatch() { + String requestPath = "/api/v1/resource"; + String handlerPath = "/api/v2/resource"; + assertFalse(sf.restPathMatches(requestPath, handlerPath)); + } + + @Test + public void testMatchWithLeadingSlashDifference() { + String requestPath = "api/v1/resource"; + String handlerPath = "/api/v1/resource"; + assertTrue(sf.restPathMatches(requestPath, handlerPath)); + } + + @Test + public void testMatchWithTrailingSlashDifference() { + String requestPath = "/api/v1/resource/"; + String handlerPath = "/api/v1/resource"; + assertTrue(sf.restPathMatches(requestPath, handlerPath)); + } + + @Test + public void testPathsMatchWithNamedParameter() { + String requestPath = "/api/v1/resource/123"; + String handlerPath = "/api/v1/resource/{id}"; + assertTrue(sf.restPathMatches(requestPath, handlerPath)); + } + + @Test + public void testPathsDoNotMatchWithDifferentNamedParameter() { + String requestPath = "/api/v1/resource/123"; + String handlerPath = "/api/v1/resource/{name}"; + assertTrue(sf.restPathMatches(requestPath, handlerPath)); + } + + @Test + public void testDifferentSegmentCount() { + String requestPath = "/api/v1/resource/123/extra"; + String handlerPath = "/api/v1/resource/{id}"; + assertFalse(sf.restPathMatches(requestPath, handlerPath)); + } + + @Test + public void testPathsMatchWithMultipleNamedParameters() { + String requestPath = "/api/v1/resource/123/details"; + String handlerPath = "/api/v1/resource/{id}/details"; + assertTrue(sf.restPathMatches(requestPath, handlerPath)); + } + + @Test + public void testPathsDoNotMatchWithNonMatchingNamedParameterSegment() { + String requestPath = "/api/v1/resource/123/details"; + String handlerPath = "/api/v1/resource/{id}/summary"; + assertFalse(sf.restPathMatches(requestPath, handlerPath)); + } + } }