* QPID-486 Choose strongest SASL Mechanism first

* Remove unnecessary wrapping of AMQExceptions on connection failure


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

8 files changed, 97 insertions, 83 deletions

diff --git a/dotnet/Qpid.Client.Tests/Security/CallbackHandlerRegistryTests.cs b/dotnet/Qpid.Client.Tests/Security/CallbackHandlerRegistryTests.cs
index e18fa15c20..9dac69a483 100644
--- a/dotnet/Qpid.Client.Tests/Security/CallbackHandlerRegistryTests.cs
+++ b/dotnet/Qpid.Client.Tests/Security/CallbackHandlerRegistryTests.cs
@@ -39,6 +39,17 @@ namespace Qpid.Client.Tests.Security
          Assert.IsNotNull(handlerType);

          Assert.AreEqual(typeof(TestCallbackHandler), handlerType);

       }

+

+      [Test]

+      public void MechanimsInOrder()

+      {

+         CallbackHandlerRegistry registry = CallbackHandlerRegistry.Instance;

+         Assert.AreEqual(4, registry.Mechanisms.Length);

+         Assert.AreEqual("TEST", registry.Mechanisms[0]);

+         Assert.AreEqual("EXTERNAL", registry.Mechanisms[1]);

+         Assert.AreEqual("CRAM-MD5", registry.Mechanisms[2]);

+         Assert.AreEqual("PLAIN", registry.Mechanisms[3]);

+      }

    } // class CallbackRegistryHandlerTests

 

    public class TestCallbackHandler : IAMQCallbackHandler

diff --git a/dotnet/Qpid.Client.Tests/connection/ConnectionTest.cs b/dotnet/Qpid.Client.Tests/connection/ConnectionTest.cs
index bcfbb3da20..d597b013bb 100644
--- a/dotnet/Qpid.Client.Tests/connection/ConnectionTest.cs
+++ b/dotnet/Qpid.Client.Tests/connection/ConnectionTest.cs
@@ -28,12 +28,15 @@ namespace Qpid.Client.Tests.Connection
     [TestFixture]
     public class ConnectionTest
     {
+        private AmqBrokerInfo _broker = 
+           new AmqBrokerInfo("amqp", "localhost", 5672, false);
+        
         [Test]
         public void SimpleConnection()
         {
             IConnectionInfo connectionInfo = new QpidConnectionInfo();
             connectionInfo.VirtualHost = "test";
-            connectionInfo.AddBrokerInfo(new AmqBrokerInfo("amqp", "localhost", 5672, false));
+            connectionInfo.AddBrokerInfo(_broker);
             using (IConnection connection = new AMQConnection(connectionInfo))
             {
                 Console.WriteLine("connection = " + connection);
@@ -41,62 +44,29 @@ namespace Qpid.Client.Tests.Connection
         }
 
         [Test]
+        [ExpectedException(typeof(AMQAuthenticationException))]
         public void PasswordFailureConnection()
         {
             IConnectionInfo connectionInfo = new QpidConnectionInfo();
             connectionInfo.VirtualHost = "test";
             connectionInfo.Password = "rubbish";
-            connectionInfo.AddBrokerInfo(new AmqBrokerInfo());
-            try
-            {
-                using (IConnection connection = new AMQConnection(connectionInfo))
-                {
-                    Console.WriteLine("connection = " + connection);
-                    // wrong
-                    Assert.Fail("Authentication succeeded but should've failed");
-                }
-            } 
-            catch (AMQException e)
+            connectionInfo.AddBrokerInfo(_broker);
+
+            using (IConnection connection = new AMQConnection(connectionInfo))
             {
-                if (!(e.InnerException is AMQAuthenticationException))
-                {
-                    Assert.Fail("Expected AMQAuthenticationException!");
-                }
-           }
+                 Console.WriteLine("connection = " + connection);
+                 // wrong
+                 Assert.Fail("Authentication succeeded but should've failed");
+            }
+        }
+
+        [Test]
+        [ExpectedException(typeof(AMQConnectionException))]
+        public void connectionFailure()
+        {
+            string url = "amqp://guest:guest@clientid/testpath?brokerlist='tcp://localhost:5673?retries='0''";
+            new AMQConnection(QpidConnectionInfo.FromUrl(url));
+            Assert.Fail("Connection should not be established");
         }
-//
-//        [Test]
-//        public void connectionFailure()
-//        {
-//            try
-//            {
-//                new AMQConnection("amqp://guest:guest@clientid/testpath?brokerlist='tcp://localhost:5673?retries='0''");
-//                Assert.fail("Connection should not be established");
-//            }
-//            catch (AMQException amqe)
-//            {
-//                if (!(amqe instanceof AMQConnectionException))
-//                {
-//                    Assert.fail("Correct exception not thrown");
-//                }
-//            }
-//        }
-//
-//        [Test]
-//        public void unresolvedHostFailure()
-//        {
-//            try
-//            {
-//                new AMQConnection("amqp://guest:guest@clientid/testpath?brokerlist='tcp://rubbishhost:5672?retries='0''");
-//                Assert.fail("Connection should not be established");
-//            }
-//            catch (AMQException amqe)
-//            {
-//                if (!(amqe instanceof AMQUnresolvedAddressException))
-//                {
-//                    Assert.fail("Correct exception not thrown");
-//                }
-//            }
-//        }
     }
 }
diff --git a/dotnet/Qpid.Client/Client/AMQConnection.cs b/dotnet/Qpid.Client/Client/AMQConnection.cs
index da8498f938..4498ba3a32 100644
--- a/dotnet/Qpid.Client/Client/AMQConnection.cs
+++ b/dotnet/Qpid.Client/Client/AMQConnection.cs
@@ -157,7 +157,10 @@ namespace Qpid.Client
             
             if (!_failoverPolicy.FailoverAllowed())
             {
-                throw new AMQConnectionException("Unable to connect", lastException);
+                if ( lastException is AMQException )
+                   throw lastException;
+                else
+                   throw new AMQConnectionException("Unable to connect", lastException);
             }
 
             // TODO: this needs to be redone so that we are not spinning.
diff --git a/dotnet/Qpid.Client/Client/Configuration/AuthenticationConfigurationSectionHandler.cs b/dotnet/Qpid.Client/Client/Configuration/AuthenticationConfigurationSectionHandler.cs
index 54ee2c6660..0d1fb73c31 100644
--- a/dotnet/Qpid.Client/Client/Configuration/AuthenticationConfigurationSectionHandler.cs
+++ b/dotnet/Qpid.Client/Client/Configuration/AuthenticationConfigurationSectionHandler.cs
@@ -37,7 +37,7 @@ namespace Qpid.Client.Configuration
       public object Create(object parent, object configContext, System.Xml.XmlNode section)

       {

          NameValueSectionHandler handler = new NameValueSectionHandler();

-         IDictionary schemes = new Hashtable();

+         OrderedHashTable schemes = new OrderedHashTable();

 

          NameValueCollection options = (NameValueCollection)

             handler.Create(parent, configContext, section);

@@ -52,7 +52,7 @@ namespace Qpid.Client.Configuration
                if ( !typeof(IAMQCallbackHandler).IsAssignableFrom(type) )

                   throw new ConfigurationException(string.Format("Type '{0}' does not implement IAMQCallbackHandler", key));

 

-               schemes[key] = type;

+               schemes.Add(key, type);

             }

          }

 

@@ -61,4 +61,24 @@ namespace Qpid.Client.Configuration
 

    } // class AuthenticationConfigurationSectionHandler

 

+   public class OrderedHashTable : Hashtable

+   {

+      private ArrayList _keys = new ArrayList();

+

+      public IList OrderedKeys

+      {

+         get { return _keys; }

+      }

+

+      public override void Add(object key, object value)

+      {

+         base.Add(key, value);

+         _keys.Add(key);

+      }

+      public override void Remove(object key)

+      {

+         base.Remove(key);

+         _keys.Remove(key);

+      }

+   }

 } // namespace Qpid.Client.Configuration

diff --git a/dotnet/Qpid.Client/Client/Handler/ConnectionStartMethodHandler.cs b/dotnet/Qpid.Client/Client/Handler/ConnectionStartMethodHandler.cs
index 1815bea152..99ee7e2587 100644
--- a/dotnet/Qpid.Client/Client/Handler/ConnectionStartMethodHandler.cs
+++ b/dotnet/Qpid.Client/Client/Handler/ConnectionStartMethodHandler.cs
@@ -103,14 +103,7 @@ namespace Qpid.Client.Handler
 
        private string ChooseMechanism(string mechanisms)
        {
-           foreach ( string mech in mechanisms.Split(' ') )
-           {
-               if ( CallbackHandlerRegistry.Instance.IsSupportedMechanism(mech) )
-               {
-                   return mech;
-               }
-           }
-           return null;
+          return CallbackHandlerRegistry.Instance.ChooseMechanism(mechanisms);
        }
 
        private byte[] DoAuthentication(string selectedMechanism, AMQProtocolSession ps)
diff --git a/dotnet/Qpid.Client/Client/Security/CallbackHandlerRegistry.cs b/dotnet/Qpid.Client/Client/Security/CallbackHandlerRegistry.cs
index 944d21ad92..546bcec35a 100644
--- a/dotnet/Qpid.Client/Client/Security/CallbackHandlerRegistry.cs
+++ b/dotnet/Qpid.Client/Client/Security/CallbackHandlerRegistry.cs
@@ -70,7 +70,7 @@ namespace Qpid.Client.Security
    {

       private static CallbackHandlerRegistry _instance = 

          new CallbackHandlerRegistry();

-      private IDictionary _mechanism2HandlerMap;

+      private OrderedHashTable _mechanism2HandlerMap;

       private string[] _mechanisms;

 

       public static CallbackHandlerRegistry Instance

@@ -85,12 +85,12 @@ namespace Qpid.Client.Security
 

       private CallbackHandlerRegistry()

       {

-         _mechanism2HandlerMap = (IDictionary)

+         _mechanism2HandlerMap = (OrderedHashTable)

             ConfigurationSettings.GetConfig("qpid.client/authentication");

 

          // configure default options if not available

          if ( _mechanism2HandlerMap == null )

-            _mechanism2HandlerMap = new Hashtable();

+            _mechanism2HandlerMap = new OrderedHashTable();

 

          if ( !_mechanism2HandlerMap.Contains(ExternalSaslClient.Mechanism) )

             _mechanism2HandlerMap.Add(ExternalSaslClient.Mechanism, typeof(UsernamePasswordCallbackHandler));

@@ -99,8 +99,8 @@ namespace Qpid.Client.Security
          if ( !_mechanism2HandlerMap.Contains(PlainSaslClient.Mechanism) )

             _mechanism2HandlerMap.Add(PlainSaslClient.Mechanism, typeof(UsernamePasswordCallbackHandler));

 

-         _mechanisms = new string[_mechanism2HandlerMap.Keys.Count];

-         _mechanism2HandlerMap.Keys.CopyTo(_mechanisms, 0);

+         _mechanisms = new string[_mechanism2HandlerMap.Count];

+         _mechanism2HandlerMap.OrderedKeys.CopyTo(_mechanisms, 0);

       }

 

       public bool IsSupportedMechanism(string mechanism)

@@ -108,6 +108,17 @@ namespace Qpid.Client.Security
          return _mechanism2HandlerMap.Contains(mechanism);

       }

 

+      public string ChooseMechanism(string mechanisms)

+      {

+         IList mechs = mechanisms.Split(' ');

+         foreach ( string supportedMech in _mechanisms )

+         {

+            if ( mechs.Contains(supportedMech) )

+               return supportedMech;

+         }

+         return null;

+      }

+

       public Type GetCallbackHandler(string mechanism)

       {

          return (Type)_mechanism2HandlerMap[mechanism];

diff --git a/dotnet/Qpid.Sasl.Tests/SaslTests.cs b/dotnet/Qpid.Sasl.Tests/SaslTests.cs
index 52e07d0041..8d576c175a 100644
--- a/dotnet/Qpid.Sasl.Tests/SaslTests.cs
+++ b/dotnet/Qpid.Sasl.Tests/SaslTests.cs
@@ -112,6 +112,17 @@ namespace Qpid.Sasl.Tests
          Assert.IsInstanceOfType(typeof(TestSaslClient), client);

       }

 

+      [Test]

+      public void ChoosesStrongerMechanism()

+      {

+         Hashtable props = new Hashtable();

+         string[] mechanisms = new string[] { "PLAIN", "OTHER", "CRAM-MD5" };

+         ISaslClient client = Sasl.CreateClient(mechanisms, "", "", "", props, this);

+

+         Assert.IsNotNull(client);

+         Assert.IsInstanceOfType(typeof(CramMD5SaslClient), client);

+      }

+

 

       void ISaslCallbackHandler.Handle(ISaslCallback[] callbacks)

       {

diff --git a/dotnet/Qpid.Sasl/DefaultClientFactory.cs b/dotnet/Qpid.Sasl/DefaultClientFactory.cs
index 44df9c7ee2..e6387a1e29 100644
--- a/dotnet/Qpid.Sasl/DefaultClientFactory.cs
+++ b/dotnet/Qpid.Sasl/DefaultClientFactory.cs
@@ -74,22 +74,17 @@ namespace Qpid.Sasl
          IDictionary props, ISaslCallbackHandler handler

          )

       {

-         foreach ( string mech in mechanisms )

-         {

-            switch ( mech )

-            {

-            case PlainSaslClient.Mechanism:

-               return new PlainSaslClient(authorizationId, props, handler);

-            case CramMD5SaslClient.Mechanism:

-               return new CramMD5SaslClient(authorizationId, props, handler);

-            case AnonymousSaslClient.Mechanism:

-               return new AnonymousSaslClient(authorizationId, props, handler);

-            case DigestSaslClient.Mechanism:

-               return new DigestSaslClient(authorizationId, serverName, protocol, props, handler);

-            case ExternalSaslClient.Mechanism:

-               return new ExternalSaslClient(authorizationId, props, handler);

-            }

-         }

+         IList mechs = mechanisms;

+         if ( mechs.Contains(ExternalSaslClient.Mechanism) )

+            return new ExternalSaslClient(authorizationId, props, handler);

+         if ( mechs.Contains(DigestSaslClient.Mechanism) )

+            return new DigestSaslClient(authorizationId, serverName, protocol, props, handler);

+         if ( mechs.Contains(CramMD5SaslClient.Mechanism) )

+            return new CramMD5SaslClient(authorizationId, props, handler);

+         if ( mechs.Contains(PlainSaslClient.Mechanism) )

+            return new PlainSaslClient(authorizationId, props, handler);

+         if ( mechs.Contains(AnonymousSaslClient.Mechanism) )

+            return new AnonymousSaslClient(authorizationId, props, handler);

          // unknown mechanism

          return null;

       }