You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@ofbiz.apache.org by nm...@apache.org on 2019/01/02 15:54:01 UTC
svn commit: r1850171 - in /ofbiz/ofbiz-framework/trunk/framework/webapp/src:
main/java/org/apache/ofbiz/webapp/control/ControlFilter.java
test/java/org/apache/ofbiz/webapp/control/ControlFilterTests.java
Author: nmalin
Date: Wed Jan 2 15:54:01 2019
New Revision: 1850171
URL: http://svn.apache.org/viewvc?rev=1850171&view=rev
Log:
Improved: Refactor ControlFilter
(OFBIZ-10449)
various improvements:
* Inheriting from HttpFilter instead of implementing Filter
* Using streams when getting the allowed paths
No functional change
Thanks to Mathieu Lirzin for this refactoring
Modified:
ofbiz/ofbiz-framework/trunk/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/ControlFilter.java
ofbiz/ofbiz-framework/trunk/framework/webapp/src/test/java/org/apache/ofbiz/webapp/control/ControlFilterTests.java
Modified: ofbiz/ofbiz-framework/trunk/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/ControlFilter.java
URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/ControlFilter.java?rev=1850171&r1=1850170&r2=1850171&view=diff
==============================================================================
--- ofbiz/ofbiz-framework/trunk/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/ControlFilter.java (original)
+++ ofbiz/ofbiz-framework/trunk/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/ControlFilter.java Wed Jan 2 15:54:01 2019
@@ -19,21 +19,25 @@
package org.apache.ofbiz.webapp.control;
import java.io.IOException;
-import java.util.HashSet;
+import java.util.Arrays;
+import java.util.Collections;
import java.util.Set;
+import java.util.stream.Collectors;
import javax.servlet.Filter;
import javax.servlet.FilterChain;
import javax.servlet.FilterConfig;
import javax.servlet.ServletException;
-import javax.servlet.ServletRequest;
-import javax.servlet.ServletResponse;
+import javax.servlet.http.HttpFilter;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;
+import javax.servlet.http.HttpSession;
+import org.apache.commons.lang.BooleanUtils;
+import org.apache.commons.validator.routines.UrlValidator;
import org.apache.ofbiz.base.util.Debug;
-/*
+/**
* A Filter used to specify a whitelist of allowed paths to the OFBiz application.
* Requests that do not match any of the paths listed in allowedPaths are redirected to redirectPath, or an error code
* is returned (the error code can be set in errorCode, the default value is 403).
@@ -58,108 +62,121 @@ import org.apache.ofbiz.base.util.Debug;
* with name _FORWARDED_FROM_SERVLET_ is present; this attribute is typically set by the ControlServlet to indicate
* that the request path is safe and should not be checked again
*/
-
-
-public class ControlFilter implements Filter {
+@SuppressWarnings("serial")
+public class ControlFilter extends HttpFilter {
public static final String FORWARDED_FROM_SERVLET = "_FORWARDED_FROM_SERVLET_";
-
+ public static final int DEFAULT_HTTP_ERROR_CODE = 403;
private static final String module = ControlFilter.class.getName();
+
+ /** The path used for redirection. */
+ private String redirectPath;
+ /** True when all traffic must be redirected to {@code redirectPath}. */
private boolean redirectAll;
+ /** True when redirectPath is an absolute URI. */
private boolean redirectPathIsUrl;
- private String redirectPath;
- protected int errorCode;
- private Set<String> allowedPaths = new HashSet<>();
+ /** The error code used when current path is not allowed and {@code redirectPath} is null. */
+ private int errorCode;
+ /** The list of all path prefixes that are allowed. */
+ private Set<String> allowedPaths;
@Override
- public void init(FilterConfig filterConfig) throws ServletException {
- redirectPath = filterConfig.getInitParameter("redirectPath");
- redirectPathIsUrl = (redirectPath != null && redirectPath.toLowerCase().startsWith("http"));
- String redirectAllString = filterConfig.getInitParameter("forceRedirectAll");
- redirectAll = (redirectPath != null && redirectAllString != null && "Y".equalsIgnoreCase(redirectAllString));
- String errorCodeString = filterConfig.getInitParameter("errorCode");
- errorCode = 403;
- if (errorCodeString != null) {
- try {
- errorCode = Integer.parseInt(errorCodeString);
- } catch (NumberFormatException nfe) {
- Debug.logWarning(nfe, "Error code specified would not parse to Integer: " + errorCodeString, module);
- Debug.logWarning(nfe, "The default error code will be used: " + errorCode, module);
- }
+ public void init(FilterConfig conf) throws ServletException {
+ redirectPath = conf.getInitParameter("redirectPath");
+ redirectPathIsUrl = UrlValidator.getInstance().isValid(redirectPath);
+ errorCode = readErrorCode(conf.getInitParameter("errorCode"));
+ allowedPaths = readAllowedPaths(conf.getInitParameter("allowedPaths"));
+ redirectAll = (redirectPath != null)
+ && BooleanUtils.toBoolean(conf.getInitParameter("forceRedirectAll"));
+
+ // Ensure that the path used for local redirections is allowed.
+ if (redirectPath != null && !redirectPathIsUrl) {
+ allowedPaths.add(redirectPath);
}
- String allowedPathsString = filterConfig.getInitParameter("allowedPaths");
- if (allowedPathsString != null) {
- String[] result = allowedPathsString.split(":");
- for (int x = 0; x < result.length; x++) {
- allowedPaths.add(result[x]);
- }
- // if an URI is specified in the redirectPath parameter, it is added to the allowed list
- if (redirectPath != null && !redirectPathIsUrl) {
- allowedPaths.add(redirectPath);
- }
+ }
+ /**
+ * Converts {@code code} string to an integer. If conversion fails, Return
+ * {@code DEFAULT_HTTP_ERROR_STATUS} instead.
+ *
+ * @param code an arbitrary string which can be {@code null}
+ * @return the integer matching {@code code}
+ */
+ private static int readErrorCode(String code) {
+ try {
+ return (code == null) ? DEFAULT_HTTP_ERROR_CODE : Integer.parseInt(code);
+ } catch (NumberFormatException err) {
+ Debug.logWarning(err, "Error code specified would not parse to Integer: " + code, module);
+ Debug.logWarning(err, "The default error code will be used: " + DEFAULT_HTTP_ERROR_CODE, module);
+ return DEFAULT_HTTP_ERROR_CODE;
}
}
+ /**
+ * Splits the paths defined by {@code paths}.
+ *
+ * @param paths a string which can be either {@code null} or a list of
+ * paths separated by ':'.
+ * @return a set of string
+ */
+ private static Set<String> readAllowedPaths(String paths) {
+ return (paths == null) ? Collections.emptySet()
+ : Arrays.stream(paths.split(":")).collect(Collectors.toSet());
+ }
+
+ /**
+ * Makes allowed paths pass through while redirecting the others to a fix location.
+ *
+ * @see Filter#doFilter
+ */
@Override
- public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) throws IOException, ServletException {
- HttpServletRequest httpRequest = (HttpServletRequest) request;
- HttpServletResponse httpResponse = (HttpServletResponse) response;
+ public void doFilter(HttpServletRequest req, HttpServletResponse resp, FilterChain chain)
+ throws IOException, ServletException {
+ String context = req.getContextPath();
+ HttpSession session = req.getSession();
- // check if we are told to redirect everything
+ // Check if we are told to redirect everything.
if (redirectAll) {
// little trick here so we don't loop on ourselves
- if (httpRequest.getSession().getAttribute("_FORCE_REDIRECT_") == null) {
- httpRequest.getSession().setAttribute("_FORCE_REDIRECT_", "true");
+ if (session.getAttribute("_FORCE_REDIRECT_") == null) {
+ session.setAttribute("_FORCE_REDIRECT_", "true");
Debug.logWarning("Redirecting user to: " + redirectPath, module);
- if (redirectPathIsUrl) {
- httpResponse.sendRedirect(redirectPath);
- } else {
- httpResponse.sendRedirect(httpRequest.getContextPath() + redirectPath);
- }
- return;
+ redirect(resp, context);
} else {
- httpRequest.getSession().removeAttribute("_FORCE_REDIRECT_");
- chain.doFilter(httpRequest, httpResponse);
- return;
- }
- }
-
- if (httpRequest.getAttribute(FORWARDED_FROM_SERVLET) == null && !allowedPaths.isEmpty()) {
- // check to make sure the requested url is allowed
- // get the request URI without the webapp mount point
- String requestUri = httpRequest.getRequestURI().substring(httpRequest.getContextPath().length());
- int offset = requestUri.indexOf("/", 1);
- if (offset == -1) {
- offset = requestUri.length();
+ session.removeAttribute("_FORCE_REDIRECT_");
+ chain.doFilter(req, resp);
}
- while (!allowedPaths.contains(requestUri.substring(0, offset))) {
- offset = requestUri.indexOf("/", offset + 1);
- if (offset == -1) {
- if (allowedPaths.contains(requestUri)) {
- break;
- }
- // path not allowed
- if (redirectPath == null) {
- httpResponse.sendError(errorCode, httpRequest.getRequestURI());
- } else {
- if (redirectPathIsUrl) {
- httpResponse.sendRedirect(redirectPath);
- } else {
- httpResponse.sendRedirect(httpRequest.getContextPath() + redirectPath);
- }
- }
- if (Debug.infoOn()) {
- Debug.logInfo("[Filtered request]: " + httpRequest.getRequestURI() + " --> " + (redirectPath == null? errorCode: redirectPath), module);
- }
- return;
+ } else if (req.getAttribute(FORWARDED_FROM_SERVLET) == null
+ && !allowedPaths.isEmpty()) {
+ // Get the request URI without the webapp mount point.
+ String uriWithContext = req.getRequestURI();
+ String uri = uriWithContext.substring(context.length());
+
+ // Check if the requested URI is allowed.
+ if (allowedPaths.stream().anyMatch(uri::startsWith)) {
+ chain.doFilter(req, resp);
+ } else {
+ if (redirectPath == null) {
+ resp.sendError(errorCode, uriWithContext);
+ } else {
+ redirect(resp, context);
+ }
+ if (Debug.infoOn()) {
+ Debug.logInfo("[Filtered request]: " + uriWithContext + " --> "
+ + (redirectPath == null ? errorCode : redirectPath), module);
}
}
- chain.doFilter(request, httpResponse);
}
}
- @Override
- public void destroy() {
-
+ /**
+ * Sends an HTTP response redirecting to {@code redirectPath}.
+ *
+ * @param resp The response to send
+ * @param contextPath the prefix to add to the redirection when
+ * {@code redirectPath} is a relative URI.
+ * @throws IOException when redirection has not been properly sent.
+ */
+ private void redirect(HttpServletResponse resp, String contextPath) throws IOException {
+ resp.sendRedirect(redirectPathIsUrl ? redirectPath : (contextPath + redirectPath));
}
}
Modified: ofbiz/ofbiz-framework/trunk/framework/webapp/src/test/java/org/apache/ofbiz/webapp/control/ControlFilterTests.java
URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/framework/webapp/src/test/java/org/apache/ofbiz/webapp/control/ControlFilterTests.java?rev=1850171&r1=1850170&r2=1850171&view=diff
==============================================================================
--- ofbiz/ofbiz-framework/trunk/framework/webapp/src/test/java/org/apache/ofbiz/webapp/control/ControlFilterTests.java (original)
+++ ofbiz/ofbiz-framework/trunk/framework/webapp/src/test/java/org/apache/ofbiz/webapp/control/ControlFilterTests.java Wed Jan 2 15:54:01 2019
@@ -1,4 +1,4 @@
-/*******************************************************************************
+/*
* 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
@@ -15,50 +15,164 @@
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
- *******************************************************************************/
+ */
package org.apache.ofbiz.webapp.control;
-import static org.junit.Assert.assertEquals;
import static org.mockito.Mockito.mock;
-import static org.mockito.Mockito.verify;
-import static org.mockito.Mockito.verifyNoMoreInteractions;
import static org.mockito.Mockito.when;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.anyString;
+import static org.mockito.Mockito.times;
+import javax.servlet.FilterChain;
import javax.servlet.FilterConfig;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletResponse;
+import javax.servlet.http.HttpSession;
+import org.junit.Before;
import org.junit.Test;
public class ControlFilterTests {
+
+ private FilterConfig config;
+ private ControlFilter filter;
+ private HttpServletRequest req;
+ private HttpServletResponse resp;
+ private FilterChain next;
+ private HttpSession session;
+
+ @Before
+ public void setUp() {
+ config = mock(FilterConfig.class);
+ when(config.getInitParameter(anyString())).thenReturn(null);
+ session = mock(HttpSession.class);
+ when(session.getAttribute(anyString())).thenReturn(null);
+ req = mock(HttpServletRequest.class);
+ when(req.getSession()).thenReturn(session);
+ when(req.getContextPath()).thenReturn("");
+ resp = mock(HttpServletResponse.class);
+ next = mock(FilterChain.class);
+ filter = new ControlFilter();
+ }
+
+ @Test
+ public void filterWithExactAllowedPath() throws Exception {
+ when(config.getInitParameter("redirectPath")).thenReturn("/foo");
+ when(config.getInitParameter("allowedPaths")).thenReturn("/foo:/bar");
+ when(req.getRequestURI()).thenReturn("/servlet/bar");
+ when(req.getContextPath()).thenReturn("/servlet");
+
+ filter.init(config);
+ filter.doFilter(req, resp, next);
+ verify(next).doFilter(req, resp);
+ }
+
@Test
- public void initRetrievesAllInitParameters() throws Exception {
- FilterConfig config = mock(FilterConfig.class);
- ControlFilter cf = new ControlFilter();
- cf.init(config);
- verify(config).getInitParameter("redirectPath");
- verify(config).getInitParameter("forceRedirectAll");
- verify(config).getInitParameter("errorCode");
- verify(config).getInitParameter("allowedPaths");
- verifyNoMoreInteractions(config);
+ public void filterWithAllowedSubPath() throws Exception {
+ when(config.getInitParameter("redirectPath")).thenReturn("/foo");
+ when(config.getInitParameter("allowedPaths")).thenReturn("/foo:/bar");
+ when(req.getRequestURI()).thenReturn("/servlet/bar/baz");
+ when(req.getContextPath()).thenReturn("/servlet");
+
+ filter.init(config);
+ filter.doFilter(req, resp, next);
+ verify(next).doFilter(req, resp);
+ }
+
+ @Test
+ public void filterWithRedirection() throws Exception {
+ when(config.getInitParameter("redirectPath")).thenReturn("/foo");
+ when(config.getInitParameter("allowedPaths")).thenReturn("/bar:/baz");
+ when(req.getRequestURI()).thenReturn("/missing/path");
+
+ filter.init(config);
+ filter.doFilter(req, resp, next);
+ verify(resp).sendRedirect("/foo");
+ }
+
+ @Test
+ public void filterWithURIredirection() throws Exception {
+ when(config.getInitParameter("redirectPath")).thenReturn("http://example.org/foo");
+ when(config.getInitParameter("allowedPaths")).thenReturn("/foo:/bar");
+ when(req.getRequestURI()).thenReturn("/baz");
+
+ filter.init(config);
+ filter.doFilter(req, resp, next);
+ verify(resp).sendRedirect("http://example.org/foo");
}
@Test
- public void initSetsProperErrorCode() throws Exception {
- FilterConfig config = mock(FilterConfig.class);
- ControlFilter cf = new ControlFilter();
+ public void bailsOutWithVariousErrorCodes() throws Exception {
+ when(config.getInitParameter("allowedPaths")).thenReturn("/foo");
+ when(req.getRequestURI()).thenReturn("/baz");
+
// if no errorCode parameter is specified then the default error code is 403
- cf.init(config);
- assertEquals(cf.errorCode, 403);
+ when(config.getInitParameter("errorCode")).thenReturn(null);
+ filter.init(config);
+ filter.doFilter(req, resp, next);
+
// if the errorCode parameter is empty then the default error code is 403
when(config.getInitParameter("errorCode")).thenReturn("");
- cf.init(config);
- assertEquals(cf.errorCode, 403); // default error code is 403
+ filter.init(config);
+ filter.doFilter(req, resp, next);
+
// if an invalid errorCode parameter is specified then the default error code is 403
when(config.getInitParameter("errorCode")).thenReturn("NaN");
- cf.init(config);
- assertEquals(cf.errorCode, 403);
+ filter.init(config);
+ filter.doFilter(req, resp, next);
+
+ verify(resp, times(3)).sendError(403, "/baz");
+
// if the errorCode parameter is specified then it is set in the filter
when(config.getInitParameter("errorCode")).thenReturn("404");
- cf.init(config);
- assertEquals(cf.errorCode, 404);
+ filter.init(config);
+ filter.doFilter(req, resp, next);
+ verify(resp).sendError(404, "/baz");
+ }
+
+ @Test
+ public void redirectAllAllowed() throws Exception {
+ when(config.getInitParameter("redirectPath")).thenReturn("/bar");
+ when(config.getInitParameter("forceRedirectAll")).thenReturn("Y");
+ when(config.getInitParameter("allowedPaths")).thenReturn("/foo");
+ when(req.getRequestURI()).thenReturn("/foo");
+
+ filter.init(config);
+ filter.doFilter(req, resp, next);
+ verify(resp).sendRedirect("/bar");
+ }
+
+ @Test
+ public void redirectAllNotAllowed() throws Exception {
+ when(config.getInitParameter("redirectPath")).thenReturn("/bar");
+ when(config.getInitParameter("forceRedirectAll")).thenReturn("Y");
+ when(config.getInitParameter("allowedPaths")).thenReturn("/foo");
+ when(req.getRequestURI()).thenReturn("/baz");
+
+ filter.init(config);
+ filter.doFilter(req, resp, next);
+ verify(resp).sendRedirect("/bar");
+ }
+
+ @Test
+ public void redirectAllRecursive() throws Exception {
+ when(config.getInitParameter("redirectPath")).thenReturn("/foo");
+ when(config.getInitParameter("forceRedirectAll")).thenReturn("Y");
+ when(config.getInitParameter("allowedPaths")).thenReturn("/foo");
+ when(req.getRequestURI()).thenReturn("/foo");
+
+ // Initial Call
+ when(session.getAttribute("_FORCE_REDIRECT_")).thenReturn(null);
+ filter.init(config);
+ filter.doFilter(req, resp, next);
+ verify(resp).sendRedirect("/foo");
+ verify(session).setAttribute("_FORCE_REDIRECT_", "true");
+
+ // Recursive Call
+ when(session.getAttribute("_FORCE_REDIRECT_")).thenReturn("true");
+ filter.doFilter(req, resp, next);
+ verify(next).doFilter(req, resp);
+ verify(session).removeAttribute("_FORCE_REDIRECT_");
}
}