From 62ba0fca5ce699ad556c2ce4f84d8ca652308c45 Mon Sep 17 00:00:00 2001 From: Rob Winch Date: Tue, 5 Apr 2011 21:08:18 -0500 Subject: [PATCH] SEC-1710: Added shutdown method to OpenID4JavaConsumer that invokes MultiThreadedHttpConnectionManager.shutdownAll() --- openid/openid.gradle | 9 ++++- .../security/openid/OpenID4JavaConsumer.java | 29 +++++++++++++++- .../openid/OpenID4JavaConsumerTests.java | 34 +++++++++++++++++++ 3 files changed, 70 insertions(+), 2 deletions(-) diff --git a/openid/openid.gradle b/openid/openid.gradle index 8d5f6b5870..e4decbcc32 100644 --- a/openid/openid.gradle +++ b/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" } diff --git a/openid/src/main/java/org/springframework/security/openid/OpenID4JavaConsumer.java b/openid/src/main/java/org/springframework/security/openid/OpenID4JavaConsumer.java index 1863fea7c5..75542c2955 100644 --- a/openid/src/main/java/org/springframework/security/openid/OpenID4JavaConsumer.java +++ b/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 false will invoke {@link MultiThreadedHttpConnectionManager#shutdownAll()} + * when the bean is destroyed. This ensures that threads are + * shutdown to prevent memory leaks. Default is false. + * + * @param shutdownConnectionManager + * false (default value) if should shutdown + * MultiThreadedHttpConnectionManager on destroy, otherwise + * true. + */ + 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."); + } + } } diff --git a/openid/src/test/java/org/springframework/security/openid/OpenID4JavaConsumerTests.java b/openid/src/test/java/org/springframework/security/openid/OpenID4JavaConsumerTests.java index e7212804e5..2db95fb7dc 100644 --- a/openid/src/test/java/org/springframework/security/openid/OpenID4JavaConsumerTests.java +++ b/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 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(); + } }