QPID-1394 : Registration of JCAProvider is incorrect in client and broker SASL configurations

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

7 files changed, 93 insertions, 45 deletions

diff --git a/java/08ExcludeList-nonvm b/java/08ExcludeList-nonvm
index 4694178f92..b77b06e3ac 100644
--- a/java/08ExcludeList-nonvm
+++ b/java/08ExcludeList-nonvm
@@ -26,7 +26,7 @@ org.apache.qpid.test.client.failover.FailoverTest#*
 // InVM Broker tests awaiting resolution of QPID-1103
 org.apache.qpid.test.client.timeouts.SyncWaitDelayTest#*
 org.apache.qpid.test.client.timeouts.SyncWaitTimeoutDelayTest#*
-org.apache.qpid.server.security.acl.SimpleACLTest#
+org.apache.qpid.server.security.acl.SimpleACLTest#*
 
 // Those tests are written against the 0.10 path
 org.apache.qpid.test.unit.message.UTF8Test#*
diff --git a/java/broker/src/main/java/org/apache/qpid/server/security/auth/manager/PrincipalDatabaseAuthenticationManager.java b/java/broker/src/main/java/org/apache/qpid/server/security/auth/manager/PrincipalDatabaseAuthenticationManager.java
index 7fbb68e861..2cbbdc85ff 100644
--- a/java/broker/src/main/java/org/apache/qpid/server/security/auth/manager/PrincipalDatabaseAuthenticationManager.java
+++ b/java/broker/src/main/java/org/apache/qpid/server/security/auth/manager/PrincipalDatabaseAuthenticationManager.java
@@ -23,9 +23,7 @@ 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.commons.configuration.SubsetConfiguration;
 import org.apache.qpid.server.registry.ApplicationRegistry;
-import org.apache.qpid.server.virtualhost.VirtualHost;
 import org.apache.qpid.server.security.auth.manager.AuthenticationManager;
 import org.apache.qpid.server.security.auth.database.PrincipalDatabase;
 import org.apache.qpid.server.security.auth.sasl.JCAProvider;
@@ -59,6 +57,8 @@ 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, Configuration hostConfig) throws Exception
     {
@@ -101,10 +101,15 @@ public class PrincipalDatabaseAuthenticationManager implements AuthenticationMan
         if (providerMap.size() > 0)
         {
             // Ensure we are used before the defaults
-            if (Security.insertProviderAt(new JCAProvider(providerMap), 1) == -1)
+            if (Security.insertProviderAt(new JCAProvider(PROVIDER_NAME, providerMap), 1) == -1)
             {
-                _logger.warn("Unable to set order of providers.");
+                _logger.error("Unable to load custom SASL providers. Qpid custom SASL authenticators unavailable.");
             }
+            else
+            {
+                _logger.info("Additional SASL providers successfully registered.");
+            }
+
         }
         else
         {
diff --git a/java/broker/src/main/java/org/apache/qpid/server/security/auth/sasl/JCAProvider.java b/java/broker/src/main/java/org/apache/qpid/server/security/auth/sasl/JCAProvider.java
index fd4ad86055..d6a09d8217 100644
--- a/java/broker/src/main/java/org/apache/qpid/server/security/auth/sasl/JCAProvider.java
+++ b/java/broker/src/main/java/org/apache/qpid/server/security/auth/sasl/JCAProvider.java
@@ -28,12 +28,11 @@ import javax.security.sasl.SaslServerFactory;
 
 public final class JCAProvider extends Provider
 {
-    public JCAProvider(Map<String, Class<? extends SaslServerFactory>> providerMap)
+    public JCAProvider(String name, Map<String, Class<? extends SaslServerFactory>> providerMap)
     {
-        super("AMQSASLProvider", 1.0, "A JCA provider that registers all " +
+        super(name, 1.0, "A JCA provider that registers all " +
               "AMQ SASL providers that want to be registered");
         register(providerMap);
-        //Security.addProvider(this);
     }
 
     private void register(Map<String, Class<? extends SaslServerFactory>> providerMap)
diff --git a/java/client/src/main/java/org/apache/qpid/client/security/CallbackHandlerRegistry.properties b/java/client/src/main/java/org/apache/qpid/client/security/CallbackHandlerRegistry.properties
index 89ee8337f8..1fcfde3579 100644
--- a/java/client/src/main/java/org/apache/qpid/client/security/CallbackHandlerRegistry.properties
+++ b/java/client/src/main/java/org/apache/qpid/client/security/CallbackHandlerRegistry.properties
@@ -18,4 +18,5 @@
 #
 CallbackHandler.CRAM-MD5-HASHED=org.apache.qpid.client.security.UsernameHashedPasswordCallbackHandler
 CallbackHandler.CRAM-MD5=org.apache.qpid.client.security.UsernamePasswordCallbackHandler
+CallbackHandler.AMQPLAIN=org.apache.qpid.client.security.UsernamePasswordCallbackHandler
 CallbackHandler.PLAIN=org.apache.qpid.client.security.UsernamePasswordCallbackHandler
diff --git a/java/client/src/main/java/org/apache/qpid/client/security/DynamicSaslRegistrar.java b/java/client/src/main/java/org/apache/qpid/client/security/DynamicSaslRegistrar.java
index 803b34b7fa..2b4261b4b7 100644
--- a/java/client/src/main/java/org/apache/qpid/client/security/DynamicSaslRegistrar.java
+++ b/java/client/src/main/java/org/apache/qpid/client/security/DynamicSaslRegistrar.java
@@ -85,8 +85,19 @@ public class DynamicSaslRegistrar
 
             if (factories.size() > 0)
             {
-                Security.insertProviderAt(new JCAProvider(factories), 0);
-                _logger.debug("Dynamic SASL provider added as a security provider");
+                // Ensure we are used before the defaults
+                if (Security.insertProviderAt(new JCAProvider(factories), 1) == -1)
+                {
+                    _logger.error("Unable to load custom SASL providers.");
+                }
+                else
+                {
+                    _logger.info("Additional SASL providers successfully registered.");
+                }
+            }
+            else
+            {
+                _logger.warn("No additional SASL providers registered.");
             }
         }
         catch (IOException e)
@@ -185,6 +196,7 @@ public class DynamicSaslRegistrar
                     continue;
                 }
 
+                _logger.debug("Registering class "+ clazz.getName() +" for mechanism "+mechanism);
                 factoriesToRegister.put(mechanism, (Class<? extends SaslClientFactory>) clazz);
             }
             catch (Exception ex)
diff --git a/java/client/src/main/java/org/apache/qpid/client/security/JCAProvider.java b/java/client/src/main/java/org/apache/qpid/client/security/JCAProvider.java
index 5a2c5ac5c1..828d26ed0d 100644
--- a/java/client/src/main/java/org/apache/qpid/client/security/JCAProvider.java
+++ b/java/client/src/main/java/org/apache/qpid/client/security/JCAProvider.java
@@ -26,6 +26,7 @@ import org.slf4j.LoggerFactory;
 import javax.security.sasl.SaslClientFactory;
 
 import java.security.Provider;
+import java.security.Security;
 import java.util.Map;
 
 /**
@@ -49,10 +50,10 @@ public class JCAProvider extends Provider
      */
     public JCAProvider(Map<String, Class<? extends SaslClientFactory>> providerMap)
     {
-        super("AMQSASLProvider", 1.0, "A JCA provider that registers all "
+        super("AMQSASLProvider-Client", 1.0, "A JCA provider that registers all "
             + "AMQ SASL providers that want to be registered");
         register(providerMap);
-        // Security.addProvider(this);
+//        Security.addProvider(this);
     }
 
     /**
@@ -64,7 +65,7 @@ public class JCAProvider extends Provider
     {
         for (Map.Entry<String, Class<? extends SaslClientFactory>> me : providerMap.entrySet())
         {
-            put("SaslClientFactory." + me.getKey(), me.getValue().getName());
+            put( "SaslClientFactory."+me.getKey(), me.getValue().getName());
             log.debug("Registered SASL Client factory for " + me.getKey() + " as " + me.getValue().getName());
         }
     }
diff --git a/java/systests/src/main/java/org/apache/qpid/server/security/acl/SimpleACLTest.java b/java/systests/src/main/java/org/apache/qpid/server/security/acl/SimpleACLTest.java
index 1740d37e38..e6c9f43ffb 100644
--- a/java/systests/src/main/java/org/apache/qpid/server/security/acl/SimpleACLTest.java
+++ b/java/systests/src/main/java/org/apache/qpid/server/security/acl/SimpleACLTest.java
@@ -4,7 +4,7 @@
  *  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
+*  "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
@@ -62,6 +62,9 @@ public class SimpleACLTest extends QpidTestCase implements ConnectionListener
 
         ConfigurationFileApplicationRegistry config = new ConfigurationFileApplicationRegistry(defaultaclConfigFile);
 
+        // This is a bit evil it should be updated with QPID-1103
+        config.getConfiguration().setProperty("management.enabled", "false");
+
         ApplicationRegistry.initialise(config, 1);
 
         TransportConnection.createVMBroker(1);
@@ -69,8 +72,8 @@ public class SimpleACLTest extends QpidTestCase implements ConnectionListener
 
     public void tearDown()
     {
-        ApplicationRegistry.remove(1);
         TransportConnection.killAllVMBrokers();
+        ApplicationRegistry.remove(1);
     }
 
     public String createConnectionString(String username, String password, String broker)
@@ -83,7 +86,7 @@ public class SimpleACLTest extends QpidTestCase implements ConnectionListener
     {
         try
         {
-            Connection conn = new AMQConnection(createConnectionString("client", "guest", BROKER));
+            Connection conn = createConnection("client", "guest");
 
             Session sesh = conn.createSession(true, Session.SESSION_TRANSACTED);
 
@@ -104,7 +107,7 @@ public class SimpleACLTest extends QpidTestCase implements ConnectionListener
     {
         try
         {
-            Connection conn = new AMQConnection(createConnectionString("guest", "guest", BROKER));
+            Connection conn = createConnection("guest", "guest");
 
             //Attempt to do do things to test connection.
             Session sesh = conn.createSession(true, Session.SESSION_TRANSACTED);
@@ -126,7 +129,7 @@ public class SimpleACLTest extends QpidTestCase implements ConnectionListener
     {
         try
         {
-            Connection conn = new AMQConnection(createConnectionString("client", "guest", BROKER));
+            Connection conn = createConnection("client", "guest");
 
             Session sesh = conn.createSession(false, Session.AUTO_ACKNOWLEDGE);
 
@@ -146,7 +149,7 @@ public class SimpleACLTest extends QpidTestCase implements ConnectionListener
     {
         try
         {
-            Connection conn = new AMQConnection(createConnectionString("client", "guest", BROKER));
+            Connection conn = createConnection("client", "guest");
 
             //Prevent Failover
             ((AMQConnection) conn).setConnectionListener(this);
@@ -173,7 +176,7 @@ public class SimpleACLTest extends QpidTestCase implements ConnectionListener
     {
         try
         {
-            Connection conn = new AMQConnection(createConnectionString("client", "guest", BROKER));
+            Connection conn = createConnection("client", "guest");
 
             Session sesh = conn.createSession(false, Session.AUTO_ACKNOWLEDGE);
 
@@ -195,7 +198,7 @@ public class SimpleACLTest extends QpidTestCase implements ConnectionListener
     {
         try
         {
-            Connection conn = new AMQConnection(createConnectionString("client", "guest", BROKER));
+            Connection conn = createConnection("client", "guest");
 
             Session sesh = conn.createSession(false, Session.AUTO_ACKNOWLEDGE);
 
@@ -217,7 +220,7 @@ public class SimpleACLTest extends QpidTestCase implements ConnectionListener
     {
         try
         {
-            Connection conn = new AMQConnection(createConnectionString("client", "guest", BROKER));
+            Connection conn = createConnection("client", "guest");
 
             ((AMQConnection) conn).setConnectionListener(this);
 
@@ -244,7 +247,7 @@ public class SimpleACLTest extends QpidTestCase implements ConnectionListener
     {
         try
         {
-            Connection conn = new AMQConnection(createConnectionString("client", "guest", BROKER));
+            Connection conn = createConnection("client", "guest");
 
             ((AMQConnection) conn).setConnectionListener(this);
 
@@ -274,7 +277,7 @@ public class SimpleACLTest extends QpidTestCase implements ConnectionListener
     {
         try
         {
-            Connection conn = new AMQConnection(createConnectionString("client", "guest", BROKER));
+            Connection conn = createConnection("client", "guest");
 
             ((AMQConnection) conn).setConnectionListener(this);
 
@@ -319,7 +322,7 @@ public class SimpleACLTest extends QpidTestCase implements ConnectionListener
     {
         try
         {
-            Connection conn = new AMQConnection(createConnectionString("server", "guest", BROKER));
+            Connection conn = createConnection("server", "guest");
 
             Session sesh = conn.createSession(false, Session.AUTO_ACKNOWLEDGE);
 
@@ -338,8 +341,8 @@ public class SimpleACLTest extends QpidTestCase implements ConnectionListener
     public void testServerConsumeFromNamedQueueInvalid() throws AMQException, URLSyntaxException
     {
         try
-        {
-            Connection conn = new AMQConnection(createConnectionString("client", "guest", BROKER));
+        {                                                      
+            Connection conn = createConnection("client", "guest");
 
             Session sesh = conn.createSession(false, Session.AUTO_ACKNOWLEDGE);
 
@@ -364,10 +367,7 @@ public class SimpleACLTest extends QpidTestCase implements ConnectionListener
     {
         try
         {
-            Connection conn = new AMQConnection(createConnectionString("server", "guest", BROKER));
-
-            //Prevent Failover
-            ((AMQConnection) conn).setConnectionListener(this);
+            Connection conn = createConnection("server","guest");
 
             Session sesh = conn.createSession(false, Session.AUTO_ACKNOWLEDGE);
 
@@ -387,11 +387,30 @@ public class SimpleACLTest extends QpidTestCase implements ConnectionListener
         }
     }
 
+    private Connection createConnection(String username, String password) throws AMQException
+    {
+        AMQConnection connection = null;
+        try
+        {
+            connection = new AMQConnection(createConnectionString(username, password, BROKER));
+        }
+        catch (URLSyntaxException e)
+        {
+            // This should never happen as we generate the URLs.
+            fail(e.getMessage());
+        }
+
+        //Prevent Failover
+        connection.setConnectionListener(this);
+
+        return (Connection)connection;
+    }
+
     public void testServerCreateNamedQueueValid() throws JMSException, URLSyntaxException
     {
         try
         {
-            Connection conn = new AMQConnection(createConnectionString("server", "guest", BROKER));
+            Connection conn = createConnection("server", "guest");
 
             Session sesh = conn.createSession(false, Session.AUTO_ACKNOWLEDGE);
 
@@ -412,7 +431,7 @@ public class SimpleACLTest extends QpidTestCase implements ConnectionListener
     {
         try
         {
-            Connection conn = new AMQConnection(createConnectionString("server", "guest", BROKER));
+            Connection conn = createConnection("server", "guest");
 
             Session sesh = conn.createSession(false, Session.AUTO_ACKNOWLEDGE);
 
@@ -434,7 +453,7 @@ public class SimpleACLTest extends QpidTestCase implements ConnectionListener
     {
         try
         {
-            Connection conn = new AMQConnection(createConnectionString("server", "guest", BROKER));
+            Connection conn = createConnection("server", "guest");
 
             Session session = conn.createSession(false, Session.AUTO_ACKNOWLEDGE);
 
@@ -457,24 +476,25 @@ public class SimpleACLTest extends QpidTestCase implements ConnectionListener
 
     public void testServerCreateAutoDeleteQueueInvalid() throws JMSException, URLSyntaxException, AMQException
     {
+        Connection connection = null;
         try
         {
-            Connection conn = new AMQConnection(createConnectionString("server", "guest", BROKER));
+            connection = createConnection("server", "guest");
 
-            Session sesh = conn.createSession(false, Session.AUTO_ACKNOWLEDGE);
+            Session session = connection.createSession(false, Session.AUTO_ACKNOWLEDGE);
 
-            conn.start();
+            connection.start();
 
-            ((AMQSession) sesh).createQueue(new AMQShortString("again_ensure_auto_delete_queue_for_temporary"),
+            ((AMQSession) session).createQueue(new AMQShortString("again_ensure_auto_delete_queue_for_temporary"),
                                             true, false, false);
 
             fail("Test failed as creation succeded.");
-            //conn will be automatically closed
+            //connection will be automatically closed
         }
         catch (AMQAuthenticationException amqe)
         {
             assertEquals("Incorrect error code thrown", 403, amqe.getErrorCode().getCode());
-        }
+        }       
     }
 
     /**
@@ -488,7 +508,7 @@ public class SimpleACLTest extends QpidTestCase implements ConnectionListener
     public void testServerPublishUsingTransactionSuccess() throws AMQException, URLSyntaxException, JMSException
     {
         //Set up the Server
-        Connection serverConnection = new AMQConnection(createConnectionString("server", "guest", BROKER));
+        Connection serverConnection = createConnection("server", "guest");
 
         ((AMQConnection) serverConnection).setConnectionListener(this);
 
@@ -501,7 +521,7 @@ public class SimpleACLTest extends QpidTestCase implements ConnectionListener
         serverConnection.start();
 
         //Set up the consumer
-        Connection clientConnection = new AMQConnection(createConnectionString("client", "guest", BROKER));
+        Connection clientConnection = createConnection("client", "guest");
 
         //Send a test mesage
         Session clientSession = clientConnection.createSession(false, Session.AUTO_ACKNOWLEDGE);
@@ -542,26 +562,36 @@ public class SimpleACLTest extends QpidTestCase implements ConnectionListener
             //Send the message using a transaction as this will allow us to retrieve any errors that occur on the broker.
             serverSession.commit();
 
-            serverConnection.close();
+
 
             //Ensure Response is received.
             Message clientResponseMsg = clientResponse.receive(2000);
             assertNotNull("Client did not receive response message,", clientResponseMsg);
             assertEquals("Incorrect message received", "Response", ((TextMessage) clientResponseMsg).getText());
 
-            clientConnection.close();
         }
         catch (Exception e)
         {
             fail("Test publish failed:" + e);
         }
+        finally
+        {
+            try
+            {
+                serverConnection.close();
+            }
+            finally
+            {
+                clientConnection.close();
+            }
+        }
     }
 
     public void testServerPublishInvalidQueueSuccess() throws AMQException, URLSyntaxException, JMSException
     {
         try
         {
-            Connection conn = new AMQConnection(createConnectionString("server", "guest", BROKER));
+            Connection conn = createConnection("server", "guest");
 
             ((AMQConnection) conn).setConnectionListener(this);