QPID-3249: Remove incomplete support for authentication at virtualhost level.

Applied patch from Keith Wall <keith.wall@gmail.com>

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

7 files changed, 264 insertions, 72 deletions

diff --git a/qpid/java/broker/src/main/java/org/apache/qpid/server/configuration/VirtualHostConfiguration.java b/qpid/java/broker/src/main/java/org/apache/qpid/server/configuration/VirtualHostConfiguration.java
index 48f2d776bb..a710230616 100644
--- a/qpid/java/broker/src/main/java/org/apache/qpid/server/configuration/VirtualHostConfiguration.java
+++ b/qpid/java/broker/src/main/java/org/apache/qpid/server/configuration/VirtualHostConfiguration.java
@@ -306,7 +306,13 @@ public class VirtualHostConfiguration extends ConfigurationPlugin
     @Override
     public void validateConfiguration() throws ConfigurationException
     {
-        //Currently doesn't do validation
+        // QPID-3249.  Support for specifying authentication name at vhost level is no longer supported.
+        if (getListValue("security.authentication.name").size() > 0)
+        {
+            String message = "Validation error : security/authentication/name is no longer a supported element within the configuration xml."
+                    + " It appears in virtual host definition : " + _name;
+            throw new ConfigurationException(message);
+        }
     }
 
     public int getHouseKeepingThreadCount()
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 72b2a68450..b6df0cc0a6 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
@@ -287,7 +287,7 @@ public abstract class ApplicationRegistry implements IApplicationRegistry
 
             createDatabaseManager(_configuration);
 
-            _authenticationManager = new PrincipalDatabaseAuthenticationManager(null, null);
+            _authenticationManager = new PrincipalDatabaseAuthenticationManager();
 
             _databaseManager.initialiseManagement(_configuration);
 
diff --git a/qpid/java/broker/src/main/java/org/apache/qpid/server/security/auth/manager/AuthenticationManager.java b/qpid/java/broker/src/main/java/org/apache/qpid/server/security/auth/manager/AuthenticationManager.java
index bc771162fd..39e1e07c57 100644
--- a/qpid/java/broker/src/main/java/org/apache/qpid/server/security/auth/manager/AuthenticationManager.java
+++ b/qpid/java/broker/src/main/java/org/apache/qpid/server/security/auth/manager/AuthenticationManager.java
@@ -26,6 +26,10 @@ import javax.security.sasl.SaslServer;
 import org.apache.qpid.common.Closeable;
 import org.apache.qpid.server.security.auth.AuthenticationResult;
 
+/**
+ * The AuthenticationManager class is the entity responsible for
+ * determining the authenticity of user credentials.
+ */
 public interface AuthenticationManager extends Closeable
 {
     String getMechanisms();
diff --git a/qpid/java/broker/src/main/java/org/apache/qpid/server/security/auth/manager/PrincipalDatabaseAuthenticationManager.java b/qpid/java/broker/src/main/java/org/apache/qpid/server/security/auth/manager/PrincipalDatabaseAuthenticationManager.java
index 2a967f02af..d10ad2c170 100644
--- a/qpid/java/broker/src/main/java/org/apache/qpid/server/security/auth/manager/PrincipalDatabaseAuthenticationManager.java
+++ b/qpid/java/broker/src/main/java/org/apache/qpid/server/security/auth/manager/PrincipalDatabaseAuthenticationManager.java
@@ -21,9 +21,6 @@
 package org.apache.qpid.server.security.auth.manager;
 
 import org.apache.log4j.Logger;
-import org.apache.commons.configuration.Configuration;
-import org.apache.commons.configuration.ConfigurationException;
-import org.apache.qpid.server.configuration.VirtualHostConfiguration;
 import org.apache.qpid.server.registry.ApplicationRegistry;
 import org.apache.qpid.server.security.auth.manager.AuthenticationManager;
 import org.apache.qpid.server.security.auth.database.PrincipalDatabase;
@@ -41,6 +38,11 @@ import java.util.HashMap;
 import java.util.TreeMap;
 import java.security.Security;
 
+/**
+ * Concrete implementation of the AuthenticationManager that determines if supplied
+ * user credentials match those appearing in a PrincipalDatabase.
+ *
+ */
 public class PrincipalDatabaseAuthenticationManager implements AuthenticationManager
 {
     private static final Logger _logger = Logger.getLogger(PrincipalDatabaseAuthenticationManager.class);
@@ -57,47 +59,17 @@ public class PrincipalDatabaseAuthenticationManager implements AuthenticationMan
      */
     private Map<String, Map<String, ?>> _serverCreationProperties = new HashMap<String, Map<String, ?>>();
 
-    private AuthenticationManager _default = null;
     /** The name for the required SASL Server mechanisms */
     public static final String PROVIDER_NAME= "AMQSASLProvider-Server";
 
-    public PrincipalDatabaseAuthenticationManager(String name, VirtualHostConfiguration hostConfig) throws Exception
+    public PrincipalDatabaseAuthenticationManager()  
     {
-        _logger.info("Initialising " + (name == null ? "Default" : "'" + name + "'")
-                     + " PrincipalDatabase authentication manager.");
-
-        // Fixme This should be done per Vhost but allowing global hack isn't right but ...
-        // required as authentication is done before Vhost selection
+        _logger.info("Initialising  PrincipalDatabase authentication manager.");
 
         Map<String, Class<? extends SaslServerFactory>> providerMap = new TreeMap<String, Class<? extends SaslServerFactory>>();
 
 
-        if (name == null || hostConfig == null)
-        {
-            initialiseAuthenticationMechanisms(providerMap, ApplicationRegistry.getInstance().getDatabaseManager().getDatabases());
-        }
-        else
-        {
-            String databaseName = hostConfig.getAuthenticationDatabase();
-
-            if (databaseName == null)
-            {
-
-                _default = ApplicationRegistry.getInstance().getAuthenticationManager();
-                return;
-            }
-            else
-            {
-                PrincipalDatabase database = ApplicationRegistry.getInstance().getDatabaseManager().getDatabases().get(databaseName);
-
-                if (database == null)
-                {
-                    throw new ConfigurationException("Requested database:" + databaseName + " was not found");
-                }
-
-                initialiseAuthenticationMechanisms(providerMap, database);
-            }
-        }
+        initialiseAuthenticationMechanisms(providerMap, ApplicationRegistry.getInstance().getDatabaseManager().getDatabases());
 
         if (providerMap.size() > 0)
         {
@@ -116,11 +88,9 @@ public class PrincipalDatabaseAuthenticationManager implements AuthenticationMan
         {
             _logger.warn("No additional SASL providers registered.");
         }
-
     }
 
-
-    private void initialiseAuthenticationMechanisms(Map<String, Class<? extends SaslServerFactory>> providerMap, Map<String, PrincipalDatabase> databases) throws Exception
+    private void initialiseAuthenticationMechanisms(Map<String, Class<? extends SaslServerFactory>> providerMap, Map<String, PrincipalDatabase> databases) 
     {
         if (databases.size() > 1)
         {
@@ -136,7 +106,7 @@ public class PrincipalDatabaseAuthenticationManager implements AuthenticationMan
         }
     }
 
-    private void initialiseAuthenticationMechanisms(Map<String, Class<? extends SaslServerFactory>> providerMap, PrincipalDatabase database) throws Exception
+    private void initialiseAuthenticationMechanisms(Map<String, Class<? extends SaslServerFactory>> providerMap, PrincipalDatabase database) 
     {
         if (database == null || database.getMechanisms().size() == 0)
         {
@@ -152,7 +122,7 @@ public class PrincipalDatabaseAuthenticationManager implements AuthenticationMan
 
     private void initialiseAuthenticationMechanism(String mechanism, AuthenticationProviderInitialiser initialiser,
                                                    Map<String, Class<? extends SaslServerFactory>> providerMap)
-            throws Exception
+            
     {
         if (_mechanisms == null)
         {
@@ -175,41 +145,17 @@ public class PrincipalDatabaseAuthenticationManager implements AuthenticationMan
 
     public String getMechanisms()
     {
-        if (_default != null)
-        {
-            // Use the default AuthenticationManager if present
-            return _default.getMechanisms();
-        }
-        else
-        {
-            return _mechanisms;
-        }
+        return _mechanisms;
     }
 
     public SaslServer createSaslServer(String mechanism, String localFQDN) throws SaslException
     {
-        if (_default != null)
-        {
-            // Use the default AuthenticationManager if present
-            return _default.createSaslServer(mechanism, localFQDN);
-        }
-        else
-        {
-            return Sasl.createSaslServer(mechanism, "AMQP", localFQDN, _serverCreationProperties.get(mechanism),
-                                         _callbackHandlerMap.get(mechanism));
-        }
-
+        return Sasl.createSaslServer(mechanism, "AMQP", localFQDN, _serverCreationProperties.get(mechanism),
+                                     _callbackHandlerMap.get(mechanism));
     }
 
     public AuthenticationResult authenticate(SaslServer server, byte[] response)
     {
-        // Use the default AuthenticationManager if present
-        if (_default != null)
-        {
-            return _default.authenticate(server, response);
-        }
-
-
         try
         {
             // Process response from the client
@@ -232,6 +178,7 @@ public class PrincipalDatabaseAuthenticationManager implements AuthenticationMan
 
     public void close()
     {
+        _mechanisms = null;
         Security.removeProvider(PROVIDER_NAME);
     }
 }
diff --git a/qpid/java/broker/src/main/java/org/apache/qpid/server/virtualhost/VirtualHostImpl.java b/qpid/java/broker/src/main/java/org/apache/qpid/server/virtualhost/VirtualHostImpl.java
index 8ce4121b97..33c713c62a 100644
--- a/qpid/java/broker/src/main/java/org/apache/qpid/server/virtualhost/VirtualHostImpl.java
+++ b/qpid/java/broker/src/main/java/org/apache/qpid/server/virtualhost/VirtualHostImpl.java
@@ -101,7 +101,7 @@ public class VirtualHostImpl implements VirtualHost
 
     private AMQBrokerManagerMBean _brokerMBean;
 
-    private AuthenticationManager _authenticationManager;
+    private final AuthenticationManager _authenticationManager;
 
     private SecurityManager _securityManager;
 
@@ -248,7 +248,7 @@ public class VirtualHostImpl implements VirtualHost
 			initialiseMessageStore(hostConfig);
         }
 		
-        _authenticationManager = new PrincipalDatabaseAuthenticationManager(_name, _configuration);
+        _authenticationManager = ApplicationRegistry.getInstance().getAuthenticationManager();
 
         _brokerMBean = new AMQBrokerManagerMBean(_virtualHostMBean);
         _brokerMBean.register();
diff --git a/qpid/java/broker/src/test/java/org/apache/qpid/server/configuration/VirtualHostConfigurationTest.java b/qpid/java/broker/src/test/java/org/apache/qpid/server/configuration/VirtualHostConfigurationTest.java
index 917755e8a5..593119041d 100644
--- a/qpid/java/broker/src/test/java/org/apache/qpid/server/configuration/VirtualHostConfigurationTest.java
+++ b/qpid/java/broker/src/test/java/org/apache/qpid/server/configuration/VirtualHostConfigurationTest.java
@@ -20,6 +20,8 @@
 package org.apache.qpid.server.configuration;
 
 
+import org.apache.commons.configuration.ConfigurationException;
+import org.apache.commons.configuration.XMLConfiguration;
 import org.apache.qpid.framing.AMQShortString;
 import org.apache.qpid.server.queue.AMQPriorityQueue;
 import org.apache.qpid.server.queue.AMQQueue;
@@ -203,5 +205,29 @@ public class VirtualHostConfigurationTest extends InternalBrokerBaseCase
 
      }
 
+     /**
+      * Tests that the old element security.authentication.name is rejected.  This element
+      * was never supported properly as authentication  is performed before the virtual host
+      * is considered.
+      */
+     public void testSecurityAuthenticationNameRejected() throws Exception
+     {
+         getConfigXml().addProperty("virtualhosts.virtualhost.testSecurityAuthenticationNameRejected.security.authentication.name",
+                 "testdb");
+         
+         try
+         {
+             super.createBroker();
+             fail("Exception not thrown");
+         }
+         catch(ConfigurationException ce)
+         {
+             assertEquals("Incorrect error message",
+                          "Validation error : security/authentication/name is no longer a supported element within the configuration xml." +
+                          " It appears in virtual host definition : " + getName(),
+                          ce.getMessage());
+         }
+     }
+
 
 }
diff --git a/qpid/java/broker/src/test/java/org/apache/qpid/server/security/auth/manager/PrincipalDatabaseAuthenticationManagerTest.java b/qpid/java/broker/src/test/java/org/apache/qpid/server/security/auth/manager/PrincipalDatabaseAuthenticationManagerTest.java
new file mode 100644
index 0000000000..f51ce0b6c6
--- /dev/null
+++ b/qpid/java/broker/src/test/java/org/apache/qpid/server/security/auth/manager/PrincipalDatabaseAuthenticationManagerTest.java
@@ -0,0 +1,209 @@
+/*
+ *
+ * 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.security.Provider;
+import java.security.Security;
+
+import javax.security.sasl.SaslException;
+import javax.security.sasl.SaslServer;
+
+import org.apache.qpid.server.security.auth.AuthenticationResult;
+import org.apache.qpid.server.security.auth.AuthenticationResult.AuthenticationStatus;
+import org.apache.qpid.server.util.InternalBrokerBaseCase;
+
+/**
+ *
+ * Tests the public methods of PrincipalDatabaseAuthenticationManager.
+ *
+ */
+public class PrincipalDatabaseAuthenticationManagerTest extends InternalBrokerBaseCase
+{
+    private PrincipalDatabaseAuthenticationManager _manager = null;
+    
+    /**
+     * @see org.apache.qpid.server.util.InternalBrokerBaseCase#tearDown()
+     */
+    @Override
+    public void tearDown() throws Exception
+    {
+        super.tearDown();
+        if (_manager != null)
+        {
+            _manager.close();
+        }
+    }
+
+    /**
+     * @see org.apache.qpid.server.util.InternalBrokerBaseCase#setUp()
+     */
+    @Override
+    public void setUp() throws Exception
+    {
+        super.setUp();
+        
+        _manager = new PrincipalDatabaseAuthenticationManager();
+    }
+
+    /**
+     * Tests that the PDAM registers SASL mechanisms correctly with the runtime.
+     */
+    public void testRegisteredMechanisms() throws Exception
+    {
+        assertNotNull(_manager.getMechanisms());
+        // relies on those mechanisms attached to PropertiesPrincipalDatabaseManager
+        assertEquals("PLAIN CRAM-MD5", _manager.getMechanisms());
+    
+        Provider qpidProvider = Security.getProvider(PrincipalDatabaseAuthenticationManager.PROVIDER_NAME);
+        assertNotNull(qpidProvider);
+    }
+
+    /**
+     * Tests that the SASL factory method createSaslServer correctly
+     * returns a non-null implementation.
+     */
+    public void testSaslMechanismCreation() throws Exception
+    {
+        SaslServer server = _manager.createSaslServer("CRAM-MD5", "localhost");
+        assertNotNull(server);
+        // Merely tests the creation of the mechanism. Mechanisms themselves are tested
+        // by their own tests.
+    }
+    
+    /**
+     * 
+     * Tests that the authenticate method correctly interprets an
+     * authentication success.
+     * 
+     */
+    public void testAuthenticationSuccess() throws Exception
+    {
+        SaslServer testServer = createTestSaslServer(true, false);
+        
+        AuthenticationResult result = _manager.authenticate(testServer, "12345".getBytes());
+        assertEquals(AuthenticationStatus.SUCCESS, result.status);
+    }
+
+    /**
+     * 
+     * Tests that the authenticate method correctly interprets an
+     * authentication not complete.
+     * 
+     */
+    public void testAuthenticationNotCompleted() throws Exception
+    {
+        SaslServer testServer = createTestSaslServer(false, false);
+        
+        AuthenticationResult result = _manager.authenticate(testServer, "12345".getBytes());
+        assertEquals(AuthenticationStatus.CONTINUE, result.status);
+    }
+
+    /**
+     * 
+     * Tests that the authenticate method correctly interprets an
+     * authentication error.
+     * 
+     */
+    public void testAuthenticationError() throws Exception
+    {
+        SaslServer testServer = createTestSaslServer(false, true);
+        
+        AuthenticationResult result = _manager.authenticate(testServer, "12345".getBytes());
+        assertEquals(AuthenticationStatus.ERROR, result.status);
+    }
+    
+    /**
+     * Tests the ability to de-register the provider.
+     */
+    public void testClose() throws Exception
+    {
+        assertEquals("PLAIN CRAM-MD5", _manager.getMechanisms());
+        assertNotNull(Security.getProvider(PrincipalDatabaseAuthenticationManager.PROVIDER_NAME));
+        
+        _manager.close();
+        
+        // Check provider has been removed.
+        assertNull(_manager.getMechanisms());
+        assertNull(Security.getProvider(PrincipalDatabaseAuthenticationManager.PROVIDER_NAME));
+        _manager = null;
+    }
+
+    /**
+     * Test SASL implementation used to test the authenticate() method.
+     */
+    private SaslServer createTestSaslServer(final boolean complete, final boolean throwSaslException)
+    {
+        return new SaslServer()
+        {
+
+            @Override
+            public String getMechanismName()
+            {
+                return null;
+            }
+
+            @Override
+            public byte[] evaluateResponse(byte[] response) throws SaslException
+            {
+                if (throwSaslException)
+                {
+                    throw new SaslException("Mocked exception");
+                }
+                return null;
+            }
+
+            @Override
+            public boolean isComplete()
+            {
+                return complete;
+            }
+
+            @Override
+            public String getAuthorizationID()
+            {
+                return null;
+            }
+
+            @Override
+            public byte[] unwrap(byte[] incoming, int offset, int len) throws SaslException
+            {
+                return null;
+            }
+
+            @Override
+            public byte[] wrap(byte[] outgoing, int offset, int len) throws SaslException
+            {
+                return null;
+            }
+
+            @Override
+            public Object getNegotiatedProperty(String propName)
+            {
+                return null;
+            }
+
+            @Override
+            public void dispose() throws SaslException
+            {
+            }
+        };
+    }
+}