SEC-1710: Added shutdown method to OpenID4JavaConsumer that invokes MultiThreadedHttpConnectionManager.shutdownAll()

openid/openid.gradle

@@ -1,9 +1,12 @@
 // OpenID Module build file
 
+powermockVersion = '1.4.8'
+
 dependencies {
     compile project(':spring-security-core'),
             project(':spring-security-web'),
             'org.openid4java:openid4java-nodeps:0.9.5',
+            'commons-httpclient:commons-httpclient:3.1',
             "org.springframework:spring-aop:$springVersion",
             "org.springframework:spring-context:$springVersion",
             "org.springframework:spring-beans:$springVersion",
@@ -11,5 +14,9 @@ dependencies {
 
     provided 'javax.servlet:servlet-api:2.5'
 
-    runtime 'commons-httpclient:commons-httpclient:3.1'
+    testCompile "org.powermock:powermock-core:$powermockVersion",
+                "org.powermock:powermock-api-support:$powermockVersion",
+                "org.powermock:powermock-api-mockito:$powermockVersion",
+                "org.powermock:powermock-module-junit4:$powermockVersion",
+                "org.powermock:powermock-module-junit4-common:$powermockVersion"
 }

openid/src/main/java/org/springframework/security/openid/OpenID4JavaConsumer.java

@@ -20,6 +20,7 @@ import java.util.List;
 
 import javax.servlet.http.HttpServletRequest;
 
+import org.apache.commons.httpclient.MultiThreadedHttpConnectionManager;
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
 import org.openid4java.association.AssociationException;
@@ -37,15 +38,17 @@ import org.openid4java.message.ParameterList;
 import org.openid4java.message.ax.AxMessage;
 import org.openid4java.message.ax.FetchRequest;
 import org.openid4java.message.ax.FetchResponse;
+import org.springframework.beans.factory.DisposableBean;
 import org.springframework.util.StringUtils;
 
 
 /**
  * @author Ray Krueger
  * @author Luke Taylor
+ * @author Rob Winch
  */
 @SuppressWarnings("unchecked")
-public class OpenID4JavaConsumer implements OpenIDConsumer {
+public class OpenID4JavaConsumer implements OpenIDConsumer, DisposableBean {
     private static final String DISCOVERY_INFO_KEY = DiscoveryInformation.class.getName();
     private static final String ATTRIBUTE_LIST_KEY = "SPRING_SECURITY_OPEN_ID_ATTRIBUTES_FETCH_LIST";
 
@@ -55,6 +58,7 @@ public class OpenID4JavaConsumer implements OpenIDConsumer {
 
     private final ConsumerManager consumerManager;
     private final AxFetchListFactory attributesToFetchFactory;
+    private boolean skipShutdownConnectionManager;
 
     //~ Constructors ===================================================================================================
 
@@ -223,4 +227,27 @@ public class OpenID4JavaConsumer implements OpenIDConsumer {
 
         return attributes;
     }
+
+    /**
+     * If <code>false</code> will invoke {@link MultiThreadedHttpConnectionManager#shutdownAll()}
+     * when the bean is destroyed. This ensures that threads are
+     * shutdown to prevent memory leaks. Default is <code>false</code>.
+     *
+     * @param shutdownConnectionManager
+     *            <code>false</code> (default value) if should shutdown
+     *            MultiThreadedHttpConnectionManager on destroy, otherwise
+     *            <code>true</code>.
+     */
+    public void setSkipShutdownConnectionManager(boolean skipShutdownConnectionManager) {
+        this.skipShutdownConnectionManager = skipShutdownConnectionManager;
+    }
+
+    public void destroy() throws Exception {
+        if(!skipShutdownConnectionManager) {
+            MultiThreadedHttpConnectionManager.shutdownAll();
+        }else if(logger.isDebugEnabled()) {
+            logger.debug("Skipping calling MultiThreadedHttpConnectionManager.shutdownAll(). "
+                    + "Note this could cause memory leaks if the resources are not cleaned up else where.");
+        }
+    }
 }

openid/src/test/java/org/springframework/security/openid/OpenID4JavaConsumerTests.java

@@ -3,9 +3,15 @@ package org.springframework.security.openid;
 import static org.junit.Assert.*;
 import static org.mockito.Matchers.*;
 import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.never;
 import static org.mockito.Mockito.when;
 
+import static org.powermock.api.mockito.PowerMockito.*;
+
+import org.apache.commons.httpclient.MultiThreadedHttpConnectionManager;
 import org.junit.*;
+import org.junit.runner.RunWith;
+
 import org.openid4java.association.AssociationException;
 import org.openid4java.consumer.ConsumerException;
 import org.openid4java.consumer.ConsumerManager;
@@ -19,13 +25,21 @@ import org.openid4java.message.MessageException;
 import org.openid4java.message.ParameterList;
 import org.openid4java.message.ax.AxMessage;
 import org.openid4java.message.ax.FetchResponse;
+
 import org.springframework.mock.web.MockHttpServletRequest;
 
 import java.util.*;
 
+import org.powermock.core.classloader.annotations.PrepareForTest;
+import org.powermock.modules.junit4.PowerMockRunner;
+import org.springframework.mock.web.MockHttpServletRequest;
+
 /**
  * @author Luke Taylor
+ * @author Rob Winch
  */
+@RunWith(PowerMockRunner.class)
+@PrepareForTest(MultiThreadedHttpConnectionManager.class)
 public class OpenID4JavaConsumerTests {
     List<OpenIDAttribute> attributes = Arrays.asList(new OpenIDAttribute("a","b"), new OpenIDAttribute("b","b", Arrays.asList("c")));
 
@@ -194,4 +208,24 @@ public class OpenID4JavaConsumerTests {
         new OpenID4JavaConsumer(attributes);
     }
 
+    @Test
+    public void destroyInvokesShutdownAll() throws Exception {
+        mockStatic(MultiThreadedHttpConnectionManager.class);
+        new OpenID4JavaConsumer().destroy();
+
+        verifyStatic();
+        MultiThreadedHttpConnectionManager.shutdownAll();
+    }
+
+    @Test
+    public void destroyOverrideShutdownAll() throws Exception {
+        mockStatic(MultiThreadedHttpConnectionManager.class);
+        OpenID4JavaConsumer consumer = new OpenID4JavaConsumer();
+        consumer.setSkipShutdownConnectionManager(true);
+
+        consumer.destroy();
+
+        verifyStatic(never());
+        MultiThreadedHttpConnectionManager.shutdownAll();
+    }
 }