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

[nmalin]

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_");
     }
 }