QPID-3997: Fix test failure under JDK 1.7 when AuthenticationManagers would not always be properly closed

ApplicationRegistry refactored: Resposibilities for lifecycle of AuthenticationManager objects transfered to
AuthicationManagerRegistry (better separation of concerns). Implemented new pure unit test to expose
the failing test and then changed algorithm so that AuthenticationManagers are closed on all paths.

git-svn-id: https://svn.apache.org/repos/asf/qpid/trunk@1345607 13f79535-47bb-0310-9956-ffa450edef68

4 files changed, 239 insertions, 121 deletions

diff --git a/qpid/java/broker/src/main/java/org/apache/qpid/server/registry/ApplicationRegistry.java b/qpid/java/broker/src/main/java/org/apache/qpid/server/registry/ApplicationRegistry.java
index 4aa1ba47cd..ec6f6d0410 100644
--- a/qpid/java/broker/src/main/java/org/apache/qpid/server/registry/ApplicationRegistry.java
+++ b/qpid/java/broker/src/main/java/org/apache/qpid/server/registry/ApplicationRegistry.java
@@ -20,7 +20,6 @@
  */
 package org.apache.qpid.server.registry;
 
-import java.net.UnknownHostException;
 import org.apache.commons.configuration.ConfigurationException;
 import org.apache.log4j.Logger;
 import org.osgi.framework.BundleContext;
@@ -48,12 +47,11 @@ import org.apache.qpid.server.logging.messages.BrokerMessages;
 import org.apache.qpid.server.logging.messages.VirtualHostMessages;
 import org.apache.qpid.server.management.ManagedObjectRegistry;
 import org.apache.qpid.server.management.NoopManagedObjectRegistry;
-import org.apache.qpid.server.plugins.Plugin;
 import org.apache.qpid.server.plugins.PluginManager;
 import org.apache.qpid.server.security.SecurityManager;
-import org.apache.qpid.server.security.SecurityManager.SecurityConfiguration;
 import org.apache.qpid.server.security.auth.manager.AuthenticationManager;
-import org.apache.qpid.server.security.auth.manager.AuthenticationManagerPluginFactory;
+import org.apache.qpid.server.security.auth.manager.AuthenticationManagerRegistry;
+import org.apache.qpid.server.security.auth.manager.IAuthenticationManagerRegistry;
 import org.apache.qpid.server.stats.StatisticsCounter;
 import org.apache.qpid.server.transport.QpidAcceptor;
 import org.apache.qpid.server.virtualhost.VirtualHost;
@@ -85,9 +83,7 @@ public abstract class ApplicationRegistry implements IApplicationRegistry
 
     private ManagedObjectRegistry _managedObjectRegistry;
 
-    private AuthenticationManager _defaultAuthenticationManager;
-
-    private Map<Integer,AuthenticationManager> _authenticationManagers;
+    private IAuthenticationManagerRegistry _authenticationManagerRegistry;
 
     private VirtualHostRegistry _virtualHostRegistry;
 
@@ -115,6 +111,7 @@ public abstract class ApplicationRegistry implements IApplicationRegistry
 
     private BundleContext _bundleContext;
 
+
     protected Map<InetSocketAddress, QpidAcceptor> getAcceptors()
     {
         return _acceptors;
@@ -309,10 +306,7 @@ public abstract class ApplicationRegistry implements IApplicationRegistry
 
             _securityManager = new SecurityManager(_configuration, _pluginManager);
 
-            _authenticationManagers  = createAuthenticationManagers();
-
-            // The default authentication manager is provided in the map associated with the null key
-            _defaultAuthenticationManager = _authenticationManagers.get(null);
+            _authenticationManagerRegistry = createAuthenticationManagerRegistry(_configuration, _pluginManager);
 
             _managedObjectRegistry.start();
         }
@@ -335,93 +329,10 @@ public abstract class ApplicationRegistry implements IApplicationRegistry
         }
     }
 
-
-    /**
-     * Iterates across all discovered authentication manager factories, offering the security configuration to each.
-     *
-     * If more than one authentication manager is configured, one MUST be specified as the default
-     *
-     * It not to configure any authentication managers.
-     *
-     * @return map from port to authentication manager, with the null key being used to indicate the default.
-     * @throws ConfigurationException
-     */
-    protected Map<Integer, AuthenticationManager> createAuthenticationManagers()
-            throws ConfigurationException, UnknownHostException
+    protected IAuthenticationManagerRegistry createAuthenticationManagerRegistry(ServerConfiguration _configuration, PluginManager _pluginManager)
+            throws ConfigurationException
     {
-        final SecurityConfiguration securityConfiguration = _configuration.getConfiguration(SecurityConfiguration.class.getName());
-        final Collection<AuthenticationManagerPluginFactory<? extends Plugin>> factories = _pluginManager.getAuthenticationManagerPlugins().values();
-        
-        if (factories.size() == 0)
-        {
-            throw new ConfigurationException("No authentication manager factory plugins found.  Check the desired authentication" +
-                    "manager plugin has been placed in the plugins directory.");
-        }
-        
-        AuthenticationManager defaultAuthMgr;
-
-        Map<String,AuthenticationManager> authManagersByClass = new HashMap<String,AuthenticationManager>();
-        for (final Iterator<AuthenticationManagerPluginFactory<? extends Plugin>> iterator = factories.iterator(); iterator.hasNext();)
-        {
-            final AuthenticationManagerPluginFactory<? extends Plugin> factory = (AuthenticationManagerPluginFactory<? extends Plugin>) iterator.next();
-            final AuthenticationManager tmp = factory.newInstance(securityConfiguration);
-            if (tmp != null)
-            {
-                if(authManagersByClass.containsKey(tmp.getClass().getSimpleName()))
-                {
-                    throw new ConfigurationException("Cannot configure more than one authentication manager of type"
-                                                     + tmp.getClass().getSimpleName() + "."
-                                                     + " Remove configuration for one of the authentication managers.");
-                }
-                authManagersByClass.put(tmp.getClass().getSimpleName(),tmp);
-            }
-
-        }
-
-        if(authManagersByClass.isEmpty())
-        {
-            throw new ConfigurationException("No authentication managers configured within the configure file.");
-        }
-        if(authManagersByClass.size() == 1)
-        {
-            defaultAuthMgr = authManagersByClass.values().iterator().next();
-        }
-        else if(!authManagersByClass.isEmpty() && _configuration.getDefaultAuthenticationManager() != null)
-        {
-            defaultAuthMgr = authManagersByClass.get(_configuration.getDefaultAuthenticationManager());
-            if(defaultAuthMgr == null)
-            {
-                throw new ConfigurationException("No authentication managers configured of type "
-                                                 + _configuration.getDefaultAuthenticationManager()
-                                                 + " which is specified as the default.  Available managers are: "
-                                                 + authManagersByClass.keySet());
-            }
-        }
-        else
-        {
-            for (AuthenticationManager authenticationManger : authManagersByClass.values())
-            {
-                authenticationManger.close();
-            }
-            throw new ConfigurationException("If more than one authentication manager is configured a default MUST be specified.");
-        }
-
-        Map<Integer,AuthenticationManager> authManagers = new HashMap<Integer, AuthenticationManager>();
-        authManagers .put(null, defaultAuthMgr);
-
-        for(Map.Entry<Integer,String> portMapping : _configuration.getPortAuthenticationMappings().entrySet())
-        {
-
-            AuthenticationManager authenticationManager = authManagersByClass.get(portMapping.getValue());
-            if(authenticationManager == null)
-            {
-                throw new ConfigurationException("Unknown authentication manager class " + portMapping.getValue() +
-                                                " configured for port " + portMapping.getKey());
-            }
-            authManagers.put(portMapping.getKey(), authenticationManager);
-        }
-        
-        return authManagers;
+        return new AuthenticationManagerRegistry(_configuration, _pluginManager);
     }
 
     protected void initialiseVirtualHosts() throws Exception
@@ -578,7 +489,7 @@ public abstract class ApplicationRegistry implements IApplicationRegistry
             //Shutdown virtualhosts
             close(_virtualHostRegistry);
 
-            close(_defaultAuthenticationManager);
+            close(_authenticationManagerRegistry);
 
             close(_qmfService);
 
@@ -650,25 +561,12 @@ public abstract class ApplicationRegistry implements IApplicationRegistry
         return _managedObjectRegistry;
     }
 
-    public AuthenticationManager getDefaultAuthenticationManager()
-    {
-        return _defaultAuthenticationManager;
-    }
-
-
     @Override
     public AuthenticationManager getAuthenticationManager(SocketAddress address)
     {
-        AuthenticationManager authManager =
-                address instanceof InetSocketAddress
-                        ? _authenticationManagers.get(((InetSocketAddress)address).getPort())
-                        : null;
-
-        return authManager == null ? _defaultAuthenticationManager : authManager;
+        return _authenticationManagerRegistry.getAuthenticationManagerFor(address);
     }
 
-
-
     public PluginManager getPluginManager()
     {
         return _pluginManager;
diff --git a/qpid/java/broker/src/main/java/org/apache/qpid/server/registry/IApplicationRegistry.java b/qpid/java/broker/src/main/java/org/apache/qpid/server/registry/IApplicationRegistry.java
index 42a4927555..35e7fe3f61 100644
--- a/qpid/java/broker/src/main/java/org/apache/qpid/server/registry/IApplicationRegistry.java
+++ b/qpid/java/broker/src/main/java/org/apache/qpid/server/registry/IApplicationRegistry.java
@@ -64,15 +64,6 @@ public interface IApplicationRegistry extends StatisticsGatherer
     ManagedObjectRegistry getManagedObjectRegistry();
 
     /**
-     * Get the default AuthenticationManager
-     *
-     * @deprecated
-     *
-     * @return the AuthenticationManager
-     */
-    AuthenticationManager getDefaultAuthenticationManager();
-
-    /**
      * Get the AuthenticationManager for the given socket address
      *
      * If no AuthenticationManager has been specifically set for the given address, then use the default
diff --git a/qpid/java/broker/src/main/java/org/apache/qpid/server/security/auth/manager/AuthenticationManagerRegistry.java b/qpid/java/broker/src/main/java/org/apache/qpid/server/security/auth/manager/AuthenticationManagerRegistry.java
new file mode 100644
index 0000000000..34f038b037
--- /dev/null
+++ b/qpid/java/broker/src/main/java/org/apache/qpid/server/security/auth/manager/AuthenticationManagerRegistry.java
@@ -0,0 +1,183 @@
+/*
+ * 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
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+package org.apache.qpid.server.security.auth.manager;
+
+import java.net.InetSocketAddress;
+import java.net.SocketAddress;
+import java.util.Collection;
+import java.util.HashMap;
+import java.util.Iterator;
+import java.util.Map;
+
+import org.apache.commons.configuration.ConfigurationException;
+import org.apache.qpid.common.Closeable;
+import org.apache.qpid.server.configuration.ServerConfiguration;
+import org.apache.qpid.server.plugins.Plugin;
+import org.apache.qpid.server.plugins.PluginManager;
+import org.apache.qpid.server.security.SecurityManager.SecurityConfiguration;
+
+/**
+ * A concrete implementation of {@link IAuthenticationManagerRegistry} that registers all {@link AuthenticationManager}
+ * instances defined in the configuration, building an optional mapping between port number and AuthenticationManager.
+ *
+ * <p>The default AuthenticationManager is either the one nominated as default within the configuration with
+ * {@link ServerConfiguration#getDefaultAuthenticationManager()}, or if there is only one, it is implicitly
+ * the default.</p>
+ *
+ * <p>It is important to {@link #close()} the registry after use and this allows the AuthenticationManagers
+ * to reverse any security registrations they have performed.</p>
+ */
+public class AuthenticationManagerRegistry implements Closeable, IAuthenticationManagerRegistry
+{
+    private final Map<String,AuthenticationManager> _classToAuthManagerMap = new HashMap<String,AuthenticationManager>();
+    private final AuthenticationManager _defaultAuthenticationManager;
+    private final Map<Integer,AuthenticationManager> _portToAuthenticationManagerMap;
+
+    public AuthenticationManagerRegistry(ServerConfiguration serverConfiguration, PluginManager _pluginManager)
+    throws ConfigurationException
+    {
+        final Collection<AuthenticationManagerPluginFactory<? extends Plugin>> factories = _pluginManager.getAuthenticationManagerPlugins().values();
+
+        if (factories.size() == 0)
+        {
+            throw new ConfigurationException("No authentication manager factory plugins found. Check the desired authentication" +
+                    " manager plugin has been placed in the plugins directory.");
+        }
+
+        final SecurityConfiguration securityConfiguration = serverConfiguration.getConfiguration(SecurityConfiguration.class.getName());
+
+        boolean willClose = true;
+        try
+        {
+            createAuthenticationManagersRejectingDuplicates(factories, securityConfiguration);
+
+            if(_classToAuthManagerMap.isEmpty())
+            {
+                throw new ConfigurationException("No authentication managers configured within the configuration file.");
+            }
+
+            _defaultAuthenticationManager = getDefaultAuthenticationManager(serverConfiguration);
+
+            _portToAuthenticationManagerMap = getPortToAuthenticationManagerMap(serverConfiguration);
+            willClose = false;
+        }
+        finally
+        {
+            // if anything went wrong whilst configuring the registry, try to close all the AuthentcationManagers instantiated so far.
+            // This is done to allow the AuthenticationManager to undo any security registrations that they have performed.
+            if (willClose)
+            {
+                close();
+            }
+        }
+    }
+
+    @Override
+    public AuthenticationManager getAuthenticationManagerFor(SocketAddress address)
+    {
+        AuthenticationManager authManager =
+                address instanceof InetSocketAddress
+                        ? _portToAuthenticationManagerMap.get(((InetSocketAddress)address).getPort())
+                        : null;
+
+        return authManager == null ? _defaultAuthenticationManager : authManager;
+    }
+
+    @Override
+    public void close()
+    {
+        for (AuthenticationManager authManager : _classToAuthManagerMap.values())
+        {
+            authManager.close();
+        }
+    }
+
+    private void createAuthenticationManagersRejectingDuplicates(
+            final Collection<AuthenticationManagerPluginFactory<? extends Plugin>> factories,
+            final SecurityConfiguration securityConfiguration)
+            throws ConfigurationException
+    {
+        for (final Iterator<AuthenticationManagerPluginFactory<? extends Plugin>> iterator = factories.iterator(); iterator.hasNext();)
+        {
+            final AuthenticationManagerPluginFactory<? extends Plugin> factory = (AuthenticationManagerPluginFactory<? extends Plugin>) iterator.next();
+            final AuthenticationManager tmp = factory.newInstance(securityConfiguration);
+            if (tmp != null)
+            {
+                if(_classToAuthManagerMap.containsKey(tmp.getClass().getSimpleName()))
+                {
+                    throw new ConfigurationException("Cannot configure more than one authentication manager of type "
+                                                     + tmp.getClass().getSimpleName() + "."
+                                                     + " Remove configuration for one of the authentication managers.");
+                }
+                _classToAuthManagerMap.put(tmp.getClass().getSimpleName(),tmp);
+            }
+        }
+    }
+
+    private AuthenticationManager getDefaultAuthenticationManager(
+            ServerConfiguration serverConfiguration)
+            throws ConfigurationException
+    {
+        final AuthenticationManager defaultAuthenticationManager;
+        if(_classToAuthManagerMap.size() == 1)
+        {
+            defaultAuthenticationManager = _classToAuthManagerMap.values().iterator().next();
+        }
+        else if(serverConfiguration.getDefaultAuthenticationManager() != null)
+        {
+            defaultAuthenticationManager = _classToAuthManagerMap.get(serverConfiguration.getDefaultAuthenticationManager());
+            if(defaultAuthenticationManager == null)
+            {
+                throw new ConfigurationException("No authentication managers configured of type "
+                                                 + serverConfiguration.getDefaultAuthenticationManager()
+                                                 + " which is specified as the default.  Available managers are: "
+                                                 + _classToAuthManagerMap.keySet());
+            }
+        }
+        else
+        {
+            throw new ConfigurationException("If more than one authentication manager is configured a default MUST be specified.");
+        }
+        return defaultAuthenticationManager;
+    }
+
+    private Map<Integer,AuthenticationManager> getPortToAuthenticationManagerMap(
+            ServerConfiguration serverConfiguration)
+            throws ConfigurationException
+    {
+        Map<Integer,AuthenticationManager> portToAuthenticationManagerMap = new HashMap<Integer, AuthenticationManager>();
+
+        for(Map.Entry<Integer,String> portMapping : serverConfiguration.getPortAuthenticationMappings().entrySet())
+        {
+
+            AuthenticationManager authenticationManager = _classToAuthManagerMap.get(portMapping.getValue());
+            if(authenticationManager == null)
+            {
+                throw new ConfigurationException("Unknown authentication manager class " + portMapping.getValue() +
+                                                " configured for port " + portMapping.getKey());
+            }
+            portToAuthenticationManagerMap.put(portMapping.getKey(), authenticationManager);
+        }
+
+        return portToAuthenticationManagerMap;
+    }
+
+
+}
diff --git a/qpid/java/broker/src/main/java/org/apache/qpid/server/security/auth/manager/IAuthenticationManagerRegistry.java b/qpid/java/broker/src/main/java/org/apache/qpid/server/security/auth/manager/IAuthenticationManagerRegistry.java
new file mode 100644
index 0000000000..5c20d77804
--- /dev/null
+++ b/qpid/java/broker/src/main/java/org/apache/qpid/server/security/auth/manager/IAuthenticationManagerRegistry.java
@@ -0,0 +1,46 @@
+/*
+ * 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
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+package org.apache.qpid.server.security.auth.manager;
+
+import java.net.SocketAddress;
+
+import org.apache.qpid.common.Closeable;
+
+/**
+ * Registry for {@link AuthenticationManager} instances.
+ *
+ * <p>A lookup method {@link #getAuthenticationManagerFor(SocketAddress)} allows a caller to determine
+ * the AuthenticationManager associated with a particular port number.</p>
+ *
+ * <p>It is important to {@link #close()} the registry after use and this allows the AuthenticationManagers
+ * to reverse any security registrations they have performed.</p>
+ */
+public interface IAuthenticationManagerRegistry extends Closeable
+{
+    /**
+     * Returns the {@link AuthenticationManager} associated with a particular {@link SocketAddress}.
+     * If no authentication manager is associated with this address, a default authentication manager will be
+     * returned.  Null is never returned.
+     *
+     * @param address
+     * @return authentication manager.
+     */
+    public AuthenticationManager getAuthenticationManagerFor(SocketAddress address);
+}
\ No newline at end of file