This is an automated email from the ASF dual-hosted git repository. bdelacretaz pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/sling-org-apache-sling-servlets-resolver.git
commit cab46322ad915bfea68a0b62e33a50c8f473c067 Author: Bertrand Delacretaz <[email protected]> AuthorDate: Tue Jan 28 17:42:56 2020 +0100 SLING-8110 - PathBasedServletAcceptor added, with minimal integration tests for now --- pom.xml | 9 +- .../internal/PathBasedServletAcceptor.java | 96 ++++++++++++++++++++++ .../resolver/internal/SlingServletResolver.java | 18 +++- .../internal/resource/SlingServletConfig.java | 14 ++++ .../resolver/it/ServletResolverTestSupport.java | 28 +++++-- .../servlets/resolver/it/ServletSelectionIT.java | 25 +++++- 6 files changed, 175 insertions(+), 15 deletions(-) diff --git a/pom.xml b/pom.xml index 154432d..ee1d2d1 100644 --- a/pom.xml +++ b/pom.xml @@ -107,14 +107,17 @@ </execution> </executions> <configuration> - <argLine>${pax.vm.options}</argLine> <redirectTestOutputToFile>true</redirectTestOutputToFile> <!-- pax exam bug, often times out at exit --> <forkedProcessExitTimeoutInSeconds>1</forkedProcessExitTimeoutInSeconds> <systemProperties> <property> - <name>bundle.filename</name> - <value>${basedir}/target/${project.build.finalName}.jar</value> + <name>bundle.filename</name> + <value>${basedir}/target/${project.build.finalName}.jar</value> + </property> + <property> + <name>pax.vm.options</name> + <value>${pax.vm.options}</value> </property> </systemProperties> </configuration> diff --git a/src/main/java/org/apache/sling/servlets/resolver/internal/PathBasedServletAcceptor.java b/src/main/java/org/apache/sling/servlets/resolver/internal/PathBasedServletAcceptor.java new file mode 100644 index 0000000..cb5d1c2 --- /dev/null +++ b/src/main/java/org/apache/sling/servlets/resolver/internal/PathBasedServletAcceptor.java @@ -0,0 +1,96 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.sling.servlets.resolver.internal; + +import javax.servlet.Servlet; +import javax.servlet.ServletConfig; + +import org.apache.sling.api.SlingHttpServletRequest; +import org.apache.sling.api.servlets.ServletResolverConstants; +import org.apache.sling.engine.RequestUtil; +import org.apache.sling.servlets.resolver.internal.resource.SlingServletConfig; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * Implements the SLING-8110 extended selection mechanism for path-mounted + * servlets, which can take the extension, selectors and HTTP method into account + * if a specific service property is set to activate this mode. + */ +class PathBasedServletAcceptor { + public static final Logger LOGGER = LoggerFactory.getLogger(PathBasedServletAcceptor.class); + + // TODO should be in ServletResolverConstants + public static final String EXTPATHS_SERVICE_PROPERTY = "sling.servlet.extpaths"; + + boolean accept(SlingHttpServletRequest request, Servlet servlet) { + // Get OSGi service properties from the SlingServletConfig + final ServletConfig rawCfg = servlet.getServletConfig(); + if(!(rawCfg instanceof SlingServletConfig)) { + LOGGER.error("Did not get a SlingServletConfig for {}", RequestUtil.getServletName(servlet)); + return true; + } + final SlingServletConfig config = (SlingServletConfig)rawCfg; + final String servletName = RequestUtil.getServletName(servlet); + + // If the servlet properties have the "extpaths" option, check extension, selector etc. + boolean accepted = true; + final Object extpaths = config.getServiceProperty(EXTPATHS_SERVICE_PROPERTY); + if(extpaths != null && Boolean.valueOf(extpaths.toString())) { + accepted = + accept(servletName, config, ServletResolverConstants.SLING_SERVLET_EXTENSIONS, request.getRequestPathInfo().getExtension()) + && accept(servletName, config, ServletResolverConstants.SLING_SERVLET_SELECTORS, request.getRequestPathInfo().getSelectors()) + && accept(servletName, config, ServletResolverConstants.SLING_SERVLET_METHODS, request.getMethod()); + } + + LOGGER.debug("accepted={} for {}", accepted, servletName); + + return accepted; + } + + private boolean accept(String servletName, SlingServletConfig config, String servicePropertyKey, String ... requestValues) { + final String [] propValues = toStringArray(config.getServiceProperty(servicePropertyKey)); + if(propValues == null) { + LOGGER.debug("Property {} is null or empty, not checking that value for {}", servicePropertyKey, servletName); + return true; + } + + // requestValues must match at least one value in propValue + boolean accepted = false; + for(String rValue : requestValues) { + for(String pValue : propValues) { + if(rValue != null && rValue.equals(pValue)) { + accepted = true; + break; + } + } + } + LOGGER.debug("accepted={} for property {} and servlet {}", accepted, servicePropertyKey, servletName); + return accepted; + } + + private static String [] toStringArray(final Object value) { + if(value instanceof String) { + return new String[] { (String)value }; + } else if(value instanceof String []) { + return (String[]) value; + } + return null; + } +} diff --git a/src/main/java/org/apache/sling/servlets/resolver/internal/SlingServletResolver.java b/src/main/java/org/apache/sling/servlets/resolver/internal/SlingServletResolver.java index c9f63b8..20ebfdd 100644 --- a/src/main/java/org/apache/sling/servlets/resolver/internal/SlingServletResolver.java +++ b/src/main/java/org/apache/sling/servlets/resolver/internal/SlingServletResolver.java @@ -31,6 +31,7 @@ import java.util.Collections; import javax.servlet.Servlet; import javax.servlet.ServletContext; import javax.servlet.ServletException; +import javax.servlet.http.HttpServlet; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; @@ -126,6 +127,17 @@ public class SlingServletResolver */ private volatile String[] defaultExtensions; + private final PathBasedServletAcceptor pathBasedServletAcceptor = new PathBasedServletAcceptor(); + + private static final Servlet forbiddenPathServlet = new HttpServlet() { + private static final long serialVersionUID = 1L; + + @Override + public void service(HttpServletRequest request, HttpServletResponse response) throws IOException { + response.sendError(HttpServletResponse.SC_FORBIDDEN); + } + }; + // ---------- ServletResolver interface ----------------------------------- /** @@ -437,7 +449,11 @@ public class SlingServletResolver if ( isPathAllowed(scriptPath, this.executionPaths) ) { final Resource res = resolver.getResource(scriptPath); servlet = this.getServlet(res); - if (servlet != null && LOGGER.isDebugEnabled()) { + if(!pathBasedServletAcceptor.accept(request, servlet)) { + LOGGER.debug("Servlet {} rejected by {} returning FORBIDDEN status", RequestUtil.getServletName(servlet), + pathBasedServletAcceptor.getClass().getSimpleName()); + servlet = forbiddenPathServlet; + } else if (servlet != null && LOGGER.isDebugEnabled()) { LOGGER.debug("Servlet {} found using absolute resource type {}", RequestUtil.getServletName(servlet), scriptNameOrResourceType); } diff --git a/src/main/java/org/apache/sling/servlets/resolver/internal/resource/SlingServletConfig.java b/src/main/java/org/apache/sling/servlets/resolver/internal/resource/SlingServletConfig.java index 342ca87..775bc5b 100644 --- a/src/main/java/org/apache/sling/servlets/resolver/internal/resource/SlingServletConfig.java +++ b/src/main/java/org/apache/sling/servlets/resolver/internal/resource/SlingServletConfig.java @@ -89,4 +89,18 @@ public class SlingServletConfig implements ServletConfig { public String getServletName() { return this.name; } + + /** @return the value of an OSGi service property of this servlet + * (which can be an array that getInitParameter* messes up) + */ + public Object getServiceProperty(String key) { + return reference.getProperty(key); + } + + /** @return the OSGi service property keys of this servlet + * (which can be an array that getInitParameter* messes up) + */ + public String [] getServicePropertyKeys() { + return reference.getPropertyKeys(); + } } \ No newline at end of file diff --git a/src/test/java/org/apache/sling/servlets/resolver/it/ServletResolverTestSupport.java b/src/test/java/org/apache/sling/servlets/resolver/it/ServletResolverTestSupport.java index bca6901..3ce1b27 100644 --- a/src/test/java/org/apache/sling/servlets/resolver/it/ServletResolverTestSupport.java +++ b/src/test/java/org/apache/sling/servlets/resolver/it/ServletResolverTestSupport.java @@ -51,6 +51,7 @@ import org.apache.sling.api.resource.ResourceResolverFactory; import org.apache.sling.servlethelpers.MockSlingHttpServletRequest; import org.apache.sling.servlethelpers.MockSlingHttpServletResponse; import org.ops4j.pax.exam.options.CompositeOption; +import static org.ops4j.pax.exam.CoreOptions.vmOption; import org.osgi.framework.BundleContext; import org.osgi.framework.ServiceReference; @@ -70,14 +71,18 @@ public class ServletResolverTestSupport extends TestSupport { public static final String P_METHODS = "sling.servlet.methods"; public static final String P_EXTENSIONS = "sling.servlet.extensions"; public static final String P_SELECTORS = "sling.servlet.selectors"; + public static final String P_EXTPATHS = "sling.servlet.extpaths"; public static final String RT_DEFAULT = "sling/servlet/default"; public static final String M_GET = "GET"; public static final String M_POST = "POST"; @Configuration public Option[] configuration() { + final String vmOpt = System.getProperty("pax.vm.options"); + assertNotNull("Expecting non-null VM options", vmOpt); return remove( options( + vmOption(vmOpt), baseConfiguration(), slingQuickstartOakTar(workingDirectory(), httpPort), mavenBundle().groupId("org.apache.sling").artifactId("org.apache.sling.api").versionAsInProject(), @@ -155,24 +160,29 @@ public class ServletResolverTestSupport extends TestSupport { } if(expectedStatus > 0) { - assertEquals("Expected status " + expectedStatus + " at " + path, expectedStatus, response.getStatus()); + assertEquals("Expected status " + expectedStatus + " for " + method + + " at " + path, expectedStatus, response.getStatus()); } return response; } + protected void assertTestServlet(final String path, int expectedStatus) throws Exception { + assertTestServlet(M_GET, path, expectedStatus); + } + + protected void assertTestServlet(final String method, final String path, int expectedStatus) throws Exception { + executeRequest(method, path, expectedStatus); + } + protected void assertTestServlet(String path, String servletName) throws Exception { - assertTestServlet("GET", path, servletName); + assertTestServlet(M_GET, path, servletName); } protected void assertTestServlet(final String method, final String path, final String servletName) throws Exception { - if(servletName == null) { - executeRequest(method, path, 404); - } else { - final String output = executeRequest(method, path, 200).getOutputAsString(); - final String expected = TestServlet.SERVED_BY_PREFIX + servletName; - assertTrue("Expecting output to contain " + expected + ", got " + output, output.contains(expected)); - } + final String output = executeRequest(method, path, 200).getOutputAsString(); + final String expected = TestServlet.SERVED_BY_PREFIX + servletName; + assertTrue("Expecting output to contain " + expected + ", got " + output, output.contains(expected)); } // move below helpers for deep removal to Pax Exam diff --git a/src/test/java/org/apache/sling/servlets/resolver/it/ServletSelectionIT.java b/src/test/java/org/apache/sling/servlets/resolver/it/ServletSelectionIT.java index 3b7d730..fdcacfa 100644 --- a/src/test/java/org/apache/sling/servlets/resolver/it/ServletSelectionIT.java +++ b/src/test/java/org/apache/sling/servlets/resolver/it/ServletSelectionIT.java @@ -20,6 +20,8 @@ package org.apache.sling.servlets.resolver.it; import static org.junit.Assert.assertTrue; +import javax.servlet.http.HttpServletResponse; + import org.apache.sling.servlethelpers.MockSlingHttpServletResponse; import org.junit.Before; import org.junit.Test; @@ -65,6 +67,14 @@ public class ServletSelectionIT extends ServletResolverTestSupport { .with(P_EXTENSIONS, "testext") .with(P_SELECTORS, "testsel") .register(bundleContext); + + new TestServlet("ExtPaths") + .with(P_PATHS, "/extpaths") + .with(P_EXTPATHS, "true") + .with(P_METHODS, "POST") + .with(P_EXTENSIONS, "extPathsExt") + .with(P_SELECTORS, "extPathsSel") + .register(bundleContext); } @Test @@ -95,7 +105,7 @@ public class ServletSelectionIT extends ServletResolverTestSupport { @Test public void testFooPathServletWithPathSuffix() throws Exception { - assertTestServlet("/foo/path/suffix", null); + assertTestServlet("/foo/path/suffix", HttpServletResponse.SC_NOT_FOUND); assertTestServlet("/foo.someExtensions/path/suffix", "FooPathServlet"); assertTestServlet("/foo.someSelector.someExtension/path/suffix", "FooPathServlet"); } @@ -112,7 +122,7 @@ public class ServletSelectionIT extends ServletResolverTestSupport { @Test public void testNoServletForExtension() throws Exception { - assertTestServlet("/.yapas", null); + assertTestServlet("/.yapas", HttpServletResponse.SC_NOT_FOUND); } @Test @@ -132,4 +142,15 @@ public class ServletSelectionIT extends ServletResolverTestSupport { assertTestServlet(M_POST, "/allprops.seven.eight/suffix", "AllExceptPathsIgnored"); assertTestServlet(M_POST, "/allprops.nine/suffix", "AllExceptPathsIgnored"); } + + @Test + public void testExtPathsServlet() throws Exception { + // We just check that the "extpaths" feature is wired in, + // the details of its logic are verified in unit tests + assertTestServlet("/extpaths", HttpServletResponse.SC_FORBIDDEN); + assertTestServlet(M_POST, "/extpaths", HttpServletResponse.SC_FORBIDDEN); + assertTestServlet(M_POST, "/extpaths.extPathsExt", HttpServletResponse.SC_FORBIDDEN); + assertTestServlet(M_GET, "/extpaths.extPathsSel.extPathsExt", HttpServletResponse.SC_FORBIDDEN); + assertTestServlet(M_POST, "/extpaths.extPathsSel.extPathsExt", "ExtPaths"); + } } \ No newline at end of file
