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

3 files changed, 204 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());
         }
     }
 }
diff --git a/qpid/java/client/src/test/java/org/apache/qpid/client/security/DynamicSaslRegistrarTest.java b/qpid/java/client/src/test/java/org/apache/qpid/client/security/DynamicSaslRegistrarTest.java
new file mode 100644
index 0000000000..4281984212
--- /dev/null
+++ b/qpid/java/client/src/test/java/org/apache/qpid/client/security/DynamicSaslRegistrarTest.java
@@ -0,0 +1,140 @@
+/*
+ *
+ * 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.client.security;
+
+import java.io.File;
+import java.security.Provider;
+import java.security.Security;
+
+import org.apache.qpid.client.security.DynamicSaslRegistrar.ProviderRegistrationResult;
+import org.apache.qpid.test.utils.QpidTestCase;
+import org.apache.qpid.test.utils.TestFileUtils;
+
+public class DynamicSaslRegistrarTest extends QpidTestCase
+{
+    private Provider _registeredProvider;
+
+    public void setUp() throws Exception
+    {
+        super.setUp();
+
+        //If the client provider is already registered, remove it for the duration of the test
+        _registeredProvider = DynamicSaslRegistrar.findProvider(JCAProvider.QPID_CLIENT_SASL_PROVIDER_NAME);
+        if (_registeredProvider != null)
+        {
+            Security.removeProvider(JCAProvider.QPID_CLIENT_SASL_PROVIDER_NAME);
+        }
+    }
+
+    public void tearDown() throws Exception
+    {
+        //Remove any provider left behind by the test.
+        Security.removeProvider(JCAProvider.QPID_CLIENT_SASL_PROVIDER_NAME);
+        try
+        {
+            //If the client provider was already registered before the test, restore it.
+            if (_registeredProvider != null)
+            {
+                Security.insertProviderAt(_registeredProvider, 1);
+            }
+        }
+        finally
+        {
+            super.tearDown();
+        }
+    }
+
+    public void testRegisterDefaultProvider()
+    {
+        assertNull("Provider should not yet be registered", DynamicSaslRegistrar.findProvider(JCAProvider.QPID_CLIENT_SASL_PROVIDER_NAME));
+
+        ProviderRegistrationResult firstRegistrationResult = DynamicSaslRegistrar.registerSaslProviders();
+        assertEquals("Unexpected registration result", ProviderRegistrationResult.SUCCEEDED, firstRegistrationResult);
+        assertNotNull("Providers should now be registered", DynamicSaslRegistrar.findProvider(JCAProvider.QPID_CLIENT_SASL_PROVIDER_NAME));
+    }
+
+    public void testRegisterDefaultProviderTwice()
+    {
+        assertNull("Provider should not yet be registered", DynamicSaslRegistrar.findProvider(JCAProvider.QPID_CLIENT_SASL_PROVIDER_NAME));
+
+        DynamicSaslRegistrar.registerSaslProviders();
+        assertNotNull("Providers should now be registered", DynamicSaslRegistrar.findProvider(JCAProvider.QPID_CLIENT_SASL_PROVIDER_NAME));
+
+        ProviderRegistrationResult result = DynamicSaslRegistrar.registerSaslProviders();
+        assertEquals("Unexpected registration result when trying to re-register", ProviderRegistrationResult.EQUAL_ALREADY_REGISTERED, result);
+        assertNotNull("Providers should still be registered", DynamicSaslRegistrar.findProvider(JCAProvider.QPID_CLIENT_SASL_PROVIDER_NAME));
+    }
+
+    @SuppressWarnings("serial")
+    public void testRegisterDefaultProviderWhenAnotherIsAlreadyPresentWithDifferentFactories()
+    {
+        assertNull("Provider should not be registered", DynamicSaslRegistrar.findProvider(JCAProvider.QPID_CLIENT_SASL_PROVIDER_NAME));
+
+        //Add a test provider with the same name, version, info as the default client provider, but with different factory properties (none).
+        Provider testProvider = new Provider(JCAProvider.QPID_CLIENT_SASL_PROVIDER_NAME,
+                                             JCAProvider.QPID_CLIENT_SASL_PROVIDER_VERSION,
+                                             JCAProvider.QPID_CLIENT_SASL_PROVIDER_INFO){};
+        Security.addProvider(testProvider);
+        assertSame("Test provider should be registered", testProvider, DynamicSaslRegistrar.findProvider(JCAProvider.QPID_CLIENT_SASL_PROVIDER_NAME));
+
+        //Try to register the default provider now that another with the same name etc (but different factories)
+        //is already registered, expect it not to be registered as a result.
+        ProviderRegistrationResult result = DynamicSaslRegistrar.registerSaslProviders();
+        assertEquals("Unexpected registration result", ProviderRegistrationResult.DIFFERENT_ALREADY_REGISTERED, result);
+
+        //Verify the test provider is still registered
+        assertSame("Test provider should still be registered", testProvider, DynamicSaslRegistrar.findProvider(JCAProvider.QPID_CLIENT_SASL_PROVIDER_NAME));
+    }
+
+    public void testRegisterWithNoFactories()
+    {
+        File emptyTempFile = TestFileUtils.createTempFile(this);
+
+        assertNull("Provider should not be registered", DynamicSaslRegistrar.findProvider(JCAProvider.QPID_CLIENT_SASL_PROVIDER_NAME));
+
+        //Adjust the location of the properties file to point at an empty file, so no factories are found to register.
+        setTestSystemProperty("amq.dynamicsaslregistrar.properties", emptyTempFile.getPath());
+
+        //Try to register the default provider, expect it it not to be registered because there were no factories.
+        ProviderRegistrationResult result = DynamicSaslRegistrar.registerSaslProviders();
+        assertEquals("Unexpected registration result", ProviderRegistrationResult.NO_SASL_FACTORIES, result);
+
+        assertNull("Provider should not be registered", DynamicSaslRegistrar.findProvider(JCAProvider.QPID_CLIENT_SASL_PROVIDER_NAME));
+    }
+
+    public void testRegisterWithMissingFileGetsDefault()
+    {
+        //Create a temp file and then delete it, such that we get a path which doesn't exist
+        File tempFile = TestFileUtils.createTempFile(this);
+        assertTrue("Failed to delete file", tempFile.delete());
+
+        assertNull("Provider should not be registered", DynamicSaslRegistrar.findProvider(JCAProvider.QPID_CLIENT_SASL_PROVIDER_NAME));
+
+        //Adjust the location of the properties file to point at non-existent file.
+        setTestSystemProperty("amq.dynamicsaslregistrar.properties", tempFile.getPath());
+
+        //Try to register the default provider, expect it to fall back to the default in the jar and succeed.
+        ProviderRegistrationResult result = DynamicSaslRegistrar.registerSaslProviders();
+        assertEquals("Unexpected registration result", ProviderRegistrationResult.SUCCEEDED, result);
+
+        assertNotNull("Provider should be registered", DynamicSaslRegistrar.findProvider(JCAProvider.QPID_CLIENT_SASL_PROVIDER_NAME));
+    }
+}