QPID-4596: Call AuthenticationManager#onCreate() only when authentication provider is created

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

8 files changed, 131 insertions, 32 deletions

diff --git a/qpid/java/broker/src/main/java/org/apache/qpid/server/configuration/startup/AuthenticationProviderRecoverer.java b/qpid/java/broker/src/main/java/org/apache/qpid/server/configuration/startup/AuthenticationProviderRecoverer.java
index 81aff002d1..2bf0cdbab3 100644
--- a/qpid/java/broker/src/main/java/org/apache/qpid/server/configuration/startup/AuthenticationProviderRecoverer.java
+++ b/qpid/java/broker/src/main/java/org/apache/qpid/server/configuration/startup/AuthenticationProviderRecoverer.java
@@ -44,10 +44,7 @@ public class AuthenticationProviderRecoverer implements ConfiguredObjectRecovere
     {
         Broker broker = RecovererHelper.verifyOnlyBrokerIsParent(parents);
         Map<String, Object> attributes = configurationEntry.getAttributes();
-        AuthenticationProvider authenticationProvider = _authenticationProviderFactory.create(
-                configurationEntry.getId(),
-                broker,
-                attributes);
+        AuthenticationProvider authenticationProvider = _authenticationProviderFactory.recover(configurationEntry.getId(), attributes, broker);
 
         return authenticationProvider;
     }
diff --git a/qpid/java/broker/src/main/java/org/apache/qpid/server/model/adapter/AuthenticationProviderAdapter.java b/qpid/java/broker/src/main/java/org/apache/qpid/server/model/adapter/AuthenticationProviderAdapter.java
index 8d601460ad..24f4757a18 100644
--- a/qpid/java/broker/src/main/java/org/apache/qpid/server/model/adapter/AuthenticationProviderAdapter.java
+++ b/qpid/java/broker/src/main/java/org/apache/qpid/server/model/adapter/AuthenticationProviderAdapter.java
@@ -253,10 +253,11 @@ public abstract class AuthenticationProviderAdapter<T extends AuthenticationMana
     @Override
     protected void changeAttributes(Map<String, Object> attributes)
     {
-        AuthenticationManager manager = validateAttributes(attributes);
+        Map<String, Object> effectiveAttributes = super.generateEffectiveAttributes(attributes);
+        AuthenticationManager manager = validateAttributes(effectiveAttributes);
         manager.initialise();
         _authManager = (T)manager;
-        String type = (String)attributes.get(AuthenticationManagerFactory.ATTRIBUTE_TYPE);
+        String type = (String)effectiveAttributes.get(AuthenticationManagerFactory.ATTRIBUTE_TYPE);
         AuthenticationManagerFactory managerFactory = _factories.get(type);
         _supportedAttributes = createSupportedAttributes(managerFactory.getAttributeNames());
         super.changeAttributes(attributes);
diff --git a/qpid/java/broker/src/main/java/org/apache/qpid/server/model/adapter/AuthenticationProviderFactory.java b/qpid/java/broker/src/main/java/org/apache/qpid/server/model/adapter/AuthenticationProviderFactory.java
index 353e9f83bf..4dfcaab8ef 100644
--- a/qpid/java/broker/src/main/java/org/apache/qpid/server/model/adapter/AuthenticationProviderFactory.java
+++ b/qpid/java/broker/src/main/java/org/apache/qpid/server/model/adapter/AuthenticationProviderFactory.java
@@ -62,6 +62,24 @@ public class AuthenticationProviderFactory
      */
     public AuthenticationProvider create(UUID id, Broker broker, Map<String, Object> attributes)
     {
+        AuthenticationProviderAdapter<?> provider = createAuthenticationProvider(id, broker, attributes);
+        provider.getAuthManager().onCreate();
+        return provider;
+    }
+
+    /**
+     * Recovers {@link AuthenticationProvider} for given ID, attributes and {@link Broker}.
+     * <p>
+     * The configured {@link AuthenticationManagerFactory}'s are used to try to create the {@link AuthenticationProvider}.
+     * The first non-null instance is returned. The factories are used in non-deterministic order.
+     */
+    public AuthenticationProvider recover(UUID id, Map<String, Object> attributes, Broker broker)
+    {
+        return createAuthenticationProvider(id, broker, attributes);
+    }
+
+    private AuthenticationProviderAdapter<?> createAuthenticationProvider(UUID id, Broker broker, Map<String, Object> attributes)
+    {
         for (AuthenticationManagerFactory factory : _factories)
         {
             AuthenticationManager manager = factory.createInstance(attributes);
@@ -80,18 +98,15 @@ public class AuthenticationProviderFactory
                     {
                         if (provider instanceof PasswordCredentialManagingAuthenticationProvider)
                         {
-                            throw new IllegalConfigurationException("An authentication provider which can manage users alredy exists ["
+                            throw new IllegalConfigurationException("An authentication provider which can manage users already exists ["
                                     + provider.getName() + "]. Only one instance is allowed.");
                         }
                     }
-
-                    manager.onCreate();
                     authenticationProvider = new PrincipalDatabaseAuthenticationManagerAdapter(id, broker,
                             (PrincipalDatabaseAuthenticationManager) manager, attributes, factory.getAttributeNames());
                 }
                 else
                 {
-                    manager.onCreate();
                     authenticationProvider = new SimpleAuthenticationProviderAdapter(id, broker, manager, attributes, factory.getAttributeNames());
                 }
                 return authenticationProvider;
diff --git a/qpid/java/broker/src/main/java/org/apache/qpid/server/model/adapter/BrokerAdapter.java b/qpid/java/broker/src/main/java/org/apache/qpid/server/model/adapter/BrokerAdapter.java
index c715f1989c..ad75aba132 100644
--- a/qpid/java/broker/src/main/java/org/apache/qpid/server/model/adapter/BrokerAdapter.java
+++ b/qpid/java/broker/src/main/java/org/apache/qpid/server/model/adapter/BrokerAdapter.java
@@ -536,13 +536,9 @@ public class BrokerAdapter extends AbstractAdapter implements Broker, Configurat
 
     private AuthenticationProvider createAuthenticationProvider(Map<String, Object> attributes)
     {
-        AuthenticationProvider authenticationProvider = null;
-        synchronized (_authenticationProviders)
-        {
-            authenticationProvider = _authenticationProviderFactory.create(UUID.randomUUID(), this, attributes);
-            addAuthenticationProvider(authenticationProvider);
-        }
+        AuthenticationProvider authenticationProvider = _authenticationProviderFactory.create(UUID.randomUUID(), this, attributes);
         authenticationProvider.setDesiredState(State.INITIALISING, State.ACTIVE);
+        addAuthenticationProvider(authenticationProvider);
         return authenticationProvider;
     }
 
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 9647499783..788dfbe204 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,6 +21,7 @@
 package org.apache.qpid.server.security.auth.manager;
 
 import java.io.File;
+import java.io.FileNotFoundException;
 import java.io.IOException;
 import java.security.Principal;
 import org.apache.log4j.Logger;
@@ -79,6 +80,19 @@ public class PrincipalDatabaseAuthenticationManager implements AuthenticationMan
 
     public void initialise()
     {
+        try
+        {
+            _principalDatabase.open(new File(_passwordFile));
+        }
+        catch (FileNotFoundException e)
+        {
+            throw new IllegalConfigurationException("Exception opening password database: " + e.getMessage(), e);
+        }
+        catch (IOException e)
+        {
+            throw new IllegalConfigurationException("Cannot use password database at :" + _passwordFile, e);
+        }
+
         final Map<String, Class<? extends SaslServerFactory>> providerMap = new TreeMap<String, Class<? extends SaslServerFactory>>();
 
         initialiseAuthenticationMechanisms(providerMap, _principalDatabase);
@@ -224,8 +238,6 @@ public class PrincipalDatabaseAuthenticationManager implements AuthenticationMan
             {
                 throw new IllegalConfigurationException("Cannot read password file" + _passwordFile + ". Check permissions.");
             }
-
-            _principalDatabase.open(passwordFile);
         }
         catch (IOException e)
         {
diff --git a/qpid/java/broker/src/test/java/org/apache/qpid/server/model/adapter/AuthenticationProviderFactoryTest.java b/qpid/java/broker/src/test/java/org/apache/qpid/server/model/adapter/AuthenticationProviderFactoryTest.java
index eb721d93a0..4a76c1837c 100644
--- a/qpid/java/broker/src/test/java/org/apache/qpid/server/model/adapter/AuthenticationProviderFactoryTest.java
+++ b/qpid/java/broker/src/test/java/org/apache/qpid/server/model/adapter/AuthenticationProviderFactoryTest.java
@@ -23,6 +23,8 @@ package org.apache.qpid.server.model.adapter;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
 import static org.mockito.Mockito.any;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.never;
 
 import java.util.Collections;
 import java.util.HashMap;
@@ -45,20 +47,42 @@ public class AuthenticationProviderFactoryTest extends TestCase
 
     public void testCreatePasswordCredentialManagingAuthenticationProvider()
     {
-        AuthenticationProvider provider = testForFactory(mock(PrincipalDatabaseAuthenticationManager.class));
+        AuthenticationManager am = mock(PrincipalDatabaseAuthenticationManager.class);
+        AuthenticationProvider provider = testForFactory(am, true);
         assertTrue("The created provider should match the factory's AuthenticationManager type",
                 provider instanceof PasswordCredentialManagingAuthenticationProvider);
+        verify(am).onCreate();
     }
 
     public void testCreateNonPasswordCredentialManagingAuthenticationProvider()
     {
-        AuthenticationProvider provider = testForFactory(mock(AuthenticationManager.class));
+        AuthenticationManager am = mock(AuthenticationManager.class);
+        AuthenticationProvider provider = testForFactory(am, true);
         assertFalse("The created provider should match the factory's AuthenticationManager type",
                 provider instanceof PasswordCredentialManagingAuthenticationProvider);
+        verify(am).onCreate();
+    }
+
+    public void testRecoverPasswordCredentialManagingAuthenticationProvider()
+    {
+        AuthenticationManager am = mock(PrincipalDatabaseAuthenticationManager.class);
+        AuthenticationProvider provider = testForFactory(am, false);
+        assertTrue("The created provider should match the factory's AuthenticationManager type",
+                provider instanceof PasswordCredentialManagingAuthenticationProvider);
+        verify(am, never()).onCreate();
+    }
+
+    public void testRecoverNonPasswordCredentialManagingAuthenticationProvider()
+    {
+        AuthenticationManager am = mock(AuthenticationManager.class);
+        AuthenticationProvider provider = testForFactory(am, false);
+        assertFalse("The created provider should match the factory's AuthenticationManager type",
+                provider instanceof PasswordCredentialManagingAuthenticationProvider);
+        verify(am, never()).onCreate();
     }
 
     @SuppressWarnings("unchecked")
-    private AuthenticationProvider testForFactory(AuthenticationManager authenticationManager)
+    private AuthenticationProvider testForFactory(AuthenticationManager authenticationManager, boolean create)
     {
         UUID id = UUID.randomUUID();
         Map<String, Object> attributes = new HashMap<String, Object>();
@@ -73,7 +97,16 @@ public class AuthenticationProviderFactoryTest extends TestCase
         when(authenticationManagerFactory.createInstance(attributes)).thenReturn(authenticationManager);
 
         AuthenticationProviderFactory providerFactory = new AuthenticationProviderFactory(authManagerFactoryServiceLoader);
-        AuthenticationProvider provider = providerFactory.create(id, broker, attributes);
+
+        AuthenticationProvider provider = null;
+        if (create)
+        {
+            provider = providerFactory.create(id, broker, attributes);
+        }
+        else
+        {
+            provider = providerFactory.recover(id, attributes, broker);
+        }
 
         assertNotNull("Provider is not created", provider);
         assertEquals("Unexpected ID", id, provider.getId());
diff --git a/qpid/java/broker/src/test/java/org/apache/qpid/server/security/auth/manager/PlainPasswordFileAuthenticationManagerFactoryTest.java b/qpid/java/broker/src/test/java/org/apache/qpid/server/security/auth/manager/PlainPasswordFileAuthenticationManagerFactoryTest.java
index d428f8b211..cc11a94db8 100644
--- a/qpid/java/broker/src/test/java/org/apache/qpid/server/security/auth/manager/PlainPasswordFileAuthenticationManagerFactoryTest.java
+++ b/qpid/java/broker/src/test/java/org/apache/qpid/server/security/auth/manager/PlainPasswordFileAuthenticationManagerFactoryTest.java
@@ -20,7 +20,6 @@
 package org.apache.qpid.server.security.auth.manager;
 
 import java.io.File;
-import java.io.FileNotFoundException;
 import java.util.HashMap;
 import java.util.Map;
 
@@ -62,14 +61,11 @@ public class PlainPasswordFileAuthenticationManagerFactoryTest extends  TestCase
         _configuration.put(AbstractPrincipalDatabaseAuthManagerFactory.ATTRIBUTE_TYPE, PlainPasswordFileAuthenticationManagerFactory.PROVIDER_TYPE);
         _configuration.put(AbstractPrincipalDatabaseAuthManagerFactory.ATTRIBUTE_PATH, _emptyPasswordFile.getAbsolutePath());
 
-        try
-        {
-            _factory.createInstance(_configuration);
-        }
-        catch (RuntimeException re)
-        {
-            assertTrue(re.getCause() instanceof FileNotFoundException);
-        }
+        AuthenticationManager manager = _factory.createInstance(_configuration);
+
+        assertNotNull(manager);
+        assertTrue(manager instanceof PrincipalDatabaseAuthenticationManager);
+        assertTrue(((PrincipalDatabaseAuthenticationManager)manager).getPrincipalDatabase() instanceof PlainPasswordFilePrincipalDatabase);
     }
 
     public void testReturnsNullWhenNoConfig() throws Exception
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
index 8025907e41..b505b361fd 100644
--- 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
@@ -25,10 +25,14 @@ import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
 
 import java.io.File;
+import java.io.FileNotFoundException;
+import java.io.FileOutputStream;
+import java.security.Principal;
 import java.security.Provider;
 import java.security.Security;
 import java.util.Collections;
 import java.util.HashMap;
+import java.util.List;
 import java.util.Map;
 
 import javax.security.auth.callback.CallbackHandler;
@@ -36,9 +40,11 @@ import javax.security.sasl.SaslException;
 import javax.security.sasl.SaslServer;
 import javax.security.sasl.SaslServerFactory;
 
+import org.apache.qpid.server.configuration.IllegalConfigurationException;
 import org.apache.qpid.server.security.auth.AuthenticationResult;
 import org.apache.qpid.server.security.auth.AuthenticationResult.AuthenticationStatus;
 import org.apache.qpid.server.security.auth.UsernamePrincipal;
+import org.apache.qpid.server.security.auth.database.PlainPasswordFilePrincipalDatabase;
 import org.apache.qpid.server.security.auth.database.PrincipalDatabase;
 import org.apache.qpid.server.security.auth.sasl.AuthenticationProviderInitialiser;
 import org.apache.qpid.server.security.auth.sasl.UsernamePasswordInitialiser;
@@ -121,8 +127,51 @@ public class PrincipalDatabaseAuthenticationManagerTest extends QpidTestCase
 
         usernamePasswordInitialiser.initialise(_principalDatabase);
 
-        _manager = new PrincipalDatabaseAuthenticationManager(_principalDatabase, null);
+        _manager = new PrincipalDatabaseAuthenticationManager(_principalDatabase, _passwordFileLocation);
+        _manager.initialise();
+    }
+
+    public void testInitialiseWhenPasswordFileNotFound() throws Exception
+    {
+        _principalDatabase = new PlainPasswordFilePrincipalDatabase();
+        _manager = new PrincipalDatabaseAuthenticationManager(_principalDatabase, _passwordFileLocation);
+
+        try
+        {
+            _manager.initialise();
+            fail("Initialisiation should fail when users file does not exist");
+        }
+        catch (IllegalConfigurationException e)
+        {
+            assertTrue(e.getCause() instanceof FileNotFoundException);
+        }
+    }
+
+    public void testInitialiseWhenPasswordFileExists() throws Exception
+    {
+        _principalDatabase = new PlainPasswordFilePrincipalDatabase();
+        _manager = new PrincipalDatabaseAuthenticationManager(_principalDatabase, _passwordFileLocation);
+
+        File f = new File(_passwordFileLocation);
+        f.createNewFile();
+        FileOutputStream fos = null;
+        try
+        {
+            fos = new FileOutputStream(f);
+            fos.write("admin:admin".getBytes());
+        }
+        finally
+        {
+            if (fos != null)
+            {
+                fos.close();
+            }
+        }
         _manager.initialise();
+        List<Principal> users = _principalDatabase.getUsers();
+        assertEquals("Unexpected uses size", 1, users.size());
+        Principal p = _principalDatabase.getUser("admin");
+        assertEquals("Unexpected principal name", "admin", p.getName());
     }
 
     /**