QPID-4513: improve client handling of discovery that its SASL Provider has already been registered when it attemts to do so, as occurs in cases with multiple classloaders.

Verifies if the previously registered Provider matches the new Provider trying to be registerered; accepts it if it does, logs a warning if it doesnt (and logs the properties at debug to aid discovering why), and now only logs the error if we cant determine either way (rather than all the time as it did previously). Also corrects and clarifies some of the other existing logging to make it clearer.

Work by Alex (orudyy) and myself.

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

2 files changed, 64 insertions, 10 deletions

diff --git a/qpid/java/client/src/main/java/org/apache/qpid/client/security/DynamicSaslRegistrar.java b/qpid/java/client/src/main/java/org/apache/qpid/client/security/DynamicSaslRegistrar.java
index 9198903408..b43229292f 100644
--- a/qpid/java/client/src/main/java/org/apache/qpid/client/security/DynamicSaslRegistrar.java
+++ b/qpid/java/client/src/main/java/org/apache/qpid/client/security/DynamicSaslRegistrar.java
@@ -28,8 +28,10 @@ import org.apache.qpid.util.FileUtils;
 import javax.security.sasl.SaslClientFactory;
 import java.io.IOException;
 import java.io.InputStream;
+import java.security.Provider;
 import java.security.Security;
 import java.util.Enumeration;
+import java.util.HashMap;
 import java.util.Map;
 import java.util.Properties;
 import java.util.TreeMap;
@@ -67,10 +69,10 @@ public class DynamicSaslRegistrar
     }
 
     /** Reads the properties file, and creates a dynamic security provider to register the SASL implementations with. */
-    public static void registerSaslProviders()
+    public static ProviderRegistrationResult registerSaslProviders()
     {
         _logger.debug("public static void registerSaslProviders(): called");
-
+        ProviderRegistrationResult result = ProviderRegistrationResult.FAILED;
         // Open the SASL properties file, using the default name is one is not specified.
         String filename = System.getProperty(FILE_PROPERTY);
         InputStream is =
@@ -89,22 +91,45 @@ public class DynamicSaslRegistrar
             if (factories.size() > 0)
             {
                 // Ensure we are used before the defaults
-                if (Security.insertProviderAt(new JCAProvider(factories), 1) == -1)
+                JCAProvider qpidProvider = new JCAProvider(factories);
+                if (Security.insertProviderAt(qpidProvider, 1) == -1)
                 {
-                    _logger.error("Unable to load custom SASL providers.");
+                    Provider registeredProvider = findProvider(JCAProvider.QPID_CLIENT_SASL_PROVIDER_NAME);
+                    if (registeredProvider == null)
+                    {
+                        result = ProviderRegistrationResult.FAILED;
+                        _logger.error("Unable to load custom SASL providers.");
+                    }
+                    else if (registeredProvider.equals(qpidProvider))
+                    {
+                        result = ProviderRegistrationResult.EQUAL_ALREADY_REGISTERED;
+                        _logger.debug("Custom SASL provider is already registered with equal properties.");
+                    }
+                    else
+                    {
+                        result = ProviderRegistrationResult.DIFFERENT_ALREADY_REGISTERED;
+                        _logger.warn("Custom SASL provider was already registered with different properties.");
+                        if (_logger.isDebugEnabled())
+                        {
+                            _logger.debug("Custom SASL provider " + registeredProvider + " properties: " + new HashMap<Object, Object>(registeredProvider));
+                        }
+                    }
                 }
                 else
                 {
+                    result = ProviderRegistrationResult.SUCCEEDED;
                     _logger.info("Additional SASL providers successfully registered.");
                 }
             }
             else
             {
-                _logger.warn("No additional SASL providers registered.");
+                result = ProviderRegistrationResult.NO_SASL_FACTORIES;
+                _logger.warn("No additional SASL factories found to register.");
             }
         }
         catch (IOException e)
         {
+            result = ProviderRegistrationResult.FAILED;
             _logger.error("Error reading properties: " + e, e);
         }
         finally
@@ -122,6 +147,22 @@ public class DynamicSaslRegistrar
                 }
             }
         }
+        return result;
+    }
+
+    static Provider findProvider(String name)
+    {
+        Provider[] providers = Security.getProviders();
+        Provider registeredProvider = null;
+        for (Provider provider : providers)
+        {
+            if (name.equals(provider.getName()))
+            {
+                registeredProvider = provider;
+                break;
+            }
+        }
+        return registeredProvider;
     }
 
     /**
@@ -158,15 +199,24 @@ public class DynamicSaslRegistrar
                     continue;
                 }
 
-                _logger.debug("Registering class "+ clazz.getName() +" for mechanism "+mechanism);
+                _logger.debug("Found class "+ clazz.getName() +" for mechanism "+mechanism);
                 factoriesToRegister.put(mechanism, (Class<? extends SaslClientFactory>) clazz);
             }
             catch (Exception ex)
             {
-                _logger.error("Error instantiating SaslClientFactory calss " + className + " - skipping");
+                _logger.error("Error instantiating SaslClientFactory class " + className + " - skipping");
             }
         }
 
         return factoriesToRegister;
     }
+
+    public static enum ProviderRegistrationResult
+    {
+        SUCCEEDED,
+        EQUAL_ALREADY_REGISTERED,
+        DIFFERENT_ALREADY_REGISTERED,
+        NO_SASL_FACTORIES,
+        FAILED;
+    }
 }
diff --git a/qpid/java/client/src/main/java/org/apache/qpid/client/security/JCAProvider.java b/qpid/java/client/src/main/java/org/apache/qpid/client/security/JCAProvider.java
index 4a91f805f6..c9bcaf0d15 100644
--- a/qpid/java/client/src/main/java/org/apache/qpid/client/security/JCAProvider.java
+++ b/qpid/java/client/src/main/java/org/apache/qpid/client/security/JCAProvider.java
@@ -39,6 +39,11 @@ import java.util.Map;
  */
 public class JCAProvider extends Provider
 {
+    static final String QPID_CLIENT_SASL_PROVIDER_NAME = "AMQSASLProvider-Client";
+    static final String QPID_CLIENT_SASL_PROVIDER_INFO = "A JCA provider that registers all "
+                                                       + "AMQ SASL providers that want to be registered";
+    static final double QPID_CLIENT_SASL_PROVIDER_VERSION = 1.0;
+
     private static final Logger log = LoggerFactory.getLogger(JCAProvider.class);
 
     /**
@@ -48,8 +53,7 @@ public class JCAProvider extends Provider
      */
     public JCAProvider(Map<String, Class<? extends SaslClientFactory>> providerMap)
     {
-        super("AMQSASLProvider-Client", 1.0, "A JCA provider that registers all "
-            + "AMQ SASL providers that want to be registered");
+        super(QPID_CLIENT_SASL_PROVIDER_NAME, QPID_CLIENT_SASL_PROVIDER_VERSION, QPID_CLIENT_SASL_PROVIDER_INFO);
         register(providerMap);
     }
 
@@ -63,7 +67,7 @@ public class JCAProvider extends Provider
         for (Map.Entry<String, Class<? extends SaslClientFactory>> me : providerMap.entrySet())
         {
             put( "SaslClientFactory."+me.getKey(), me.getValue().getName());
-            log.debug("Registered SASL Client factory for " + me.getKey() + " as " + me.getValue().getName());
+            log.debug("Recording SASL Client factory for " + me.getKey() + " as " + me.getValue().getName());
         }
     }
 }