svn commit: r1850250 - in /ofbiz/ofbiz-framework/trunk/framework/webapp/src: main/java/org/apache/ofbiz/webapp/control/RequestHandler.java test/java/org/apache/ofbiz/webapp/control/RequestHandlerTests.java

[pgil]

Author: pgil
Date: Thu Jan  3 16:28:27 2019
New Revision: 1850250

URL: http://svn.apache.org/viewvc?rev=1850250&view=rev
Log:
Improved: Extract verification of certificates in ‘RequestHandler’
(OFBIZ-10450)

No functional change. Reducte the size of RequestHandler::doRequest method.
Add somes unit tests.
Thanks Mathieu

Modified:
    ofbiz/ofbiz-framework/trunk/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/RequestHandler.java
    ofbiz/ofbiz-framework/trunk/framework/webapp/src/test/java/org/apache/ofbiz/webapp/control/RequestHandlerTests.java

Modified: ofbiz/ofbiz-framework/trunk/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/RequestHandler.java
URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/RequestHandler.java?rev=1850250&r1=1850249&r2=1850250&view=diff
==============================================================================
--- ofbiz/ofbiz-framework/trunk/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/RequestHandler.java (original)
+++ ofbiz/ofbiz-framework/trunk/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/RequestHandler.java Thu Jan  3 16:28:27 2019
@@ -31,7 +31,10 @@ import java.util.HashMap;
 import java.util.List;
 import java.util.Locale;
 import java.util.Map;
+import java.util.Objects;
 import java.util.Optional;
+import java.util.function.Predicate;
+import java.util.stream.Stream;
 
 import javax.servlet.ServletContext;
 import javax.servlet.http.HttpServletRequest;
@@ -372,33 +375,7 @@ public class RequestHandler {
 
             // Check for HTTPS client (x.509) security
             if (request.isSecure() && requestMap.securityCert) {
-                X509Certificate[] clientCerts = (X509Certificate[]) request.getAttribute("javax.servlet.request.X509Certificate"); // 2.2 spec
-                if (clientCerts == null) {
-                    clientCerts = (X509Certificate[]) request.getAttribute("javax.net.ssl.peer_certificates"); // 2.1 spec
-                }
-                if (clientCerts == null) {
-                    Debug.logWarning("Received no client certificates from browser", module);
-                }
-
-                // check if the client has a valid certificate (in our db store)
-                boolean foundTrustedCert = false;
-
-                if (clientCerts == null) {
-                    throw new RequestHandlerException(requestMissingErrorMessage);
-                } else {
-                    if (Debug.infoOn()) {
-                        for (int i = 0; i < clientCerts.length; i++) {
-                            Debug.logInfo(clientCerts[i].getSubjectX500Principal().getName(), module);
-                        }
-                    }
-
-                    // check if this is a trusted cert
-                    if (SSLUtil.isClientTrusted(clientCerts, null)) {
-                        foundTrustedCert = true;
-                    }
-                }
-
-                if (!foundTrustedCert) {
+                if (!checkCertificates(request, certs -> SSLUtil.isClientTrusted(certs, null))) {
                     Debug.logWarning(requestMissingErrorMessage, module);
                     throw new RequestHandlerException(requestMissingErrorMessage);
                 }
@@ -1322,4 +1299,31 @@ public class RequestHandler {
         }
         return " Hidden sessionId by default.";
     }
+
+    /**
+     * Checks that the request contains some valid certificates.
+     *
+     * @param request the request to verify
+     * @param validator the predicate applied the certificates found
+     * @return true if the request contains some valid certificates, otherwise false.
+     */
+    static boolean checkCertificates(HttpServletRequest request, Predicate<X509Certificate[]> validator) {
+        return Stream.of("javax.servlet.request.X509Certificate", // 2.2 spec
+                         "javax.net.ssl.peer_certificates")       // 2.1 spec
+                .map(request::getAttribute)
+                .filter(Objects::nonNull)
+                .map(X509Certificate[].class::cast)
+                .peek(certs -> {
+                    if (Debug.infoOn()) {
+                        for (X509Certificate cert : certs) {
+                            Debug.logInfo(cert.getSubjectX500Principal().getName(), module);
+                        }
+                    }
+                })
+                .map(validator::test)
+                .findFirst().orElseGet(() -> {
+                    Debug.logWarning("Received no client certificates from browser", module);
+                    return false;
+                });
+    }
 }

Modified: ofbiz/ofbiz-framework/trunk/framework/webapp/src/test/java/org/apache/ofbiz/webapp/control/RequestHandlerTests.java
URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/framework/webapp/src/test/java/org/apache/ofbiz/webapp/control/RequestHandlerTests.java?rev=1850250&r1=1850249&r2=1850250&view=diff
==============================================================================
--- ofbiz/ofbiz-framework/trunk/framework/webapp/src/test/java/org/apache/ofbiz/webapp/control/RequestHandlerTests.java (original)
+++ ofbiz/ofbiz-framework/trunk/framework/webapp/src/test/java/org/apache/ofbiz/webapp/control/RequestHandlerTests.java Thu Jan  3 16:28:27 2019
@@ -25,9 +25,12 @@ import static org.hamcrest.CoreMatchers.
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertThat;
 import static org.junit.Assert.assertTrue;
+import static org.mockito.ArgumentMatchers.anyString;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
 
+import java.security.cert.CertificateEncodingException;
+import java.security.cert.X509Certificate;
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.HashMap;
@@ -197,4 +200,43 @@ public class RequestHandlerTests {
             assertTrue(RequestHandler.resolveMethod("delete", rmaps).isPresent());
         }
     }
+
+    public static class checkCertificatesTests {
+        private HttpServletRequest req;
+
+        @Before
+        public void setUp() {
+            req = mock(HttpServletRequest.class);
+            when(req.getAttribute(anyString())).thenReturn(null);
+        }
+
+        @Test
+        // Check that the verification fails when the request does not contain any certificate.
+        public void checkCertificatesFailure() {
+            assertFalse(RequestHandler.checkCertificates(req, x -> true));
+        }
+
+        @Test
+        // Check that certificates with 2.2 spec are handled correctly.
+        public void checkCertificates22() throws CertificateEncodingException {
+            when(req.getAttribute("javax.servlet.request.X509Certificate")).thenReturn(new X509Certificate[] {});
+            assertTrue(RequestHandler.checkCertificates(req, x -> true));
+            assertFalse(RequestHandler.checkCertificates(req, x -> false));
+        }
+
+        @Test
+        // Check that certificates with 2.1 spec are handled correctly.
+        public void checkCertificates21() {
+            when(req.getAttribute("javax.net.ssl.peer_certificates")).thenReturn(new X509Certificate[] {});
+            assertTrue(RequestHandler.checkCertificates(req, x -> true));
+            assertFalse(RequestHandler.checkCertificates(req, x -> false));
+        }
+
+        @Test
+        // Check that certificates in an invalid attribute are ignored.
+        public void checkCertificatesUnrecognized() {
+            when(req.getAttribute("NOT_RECOGNIZED")).thenReturn(new X509Certificate[] {});
+            assertFalse(RequestHandler.checkCertificates(req, x -> true));
+        }
+    }
 }