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

10 files changed, 192 insertions, 33 deletions

diff --git a/qpid/java/broker-plugins/management-http/src/main/java/resources/js/qpid/management/Broker.js b/qpid/java/broker-plugins/management-http/src/main/java/resources/js/qpid/management/Broker.js
index a27e43d1b1..230f148d4c 100644
--- a/qpid/java/broker-plugins/management-http/src/main/java/resources/js/qpid/management/Broker.js
+++ b/qpid/java/broker-plugins/management-http/src/main/java/resources/js/qpid/management/Broker.js
@@ -357,11 +357,25 @@ define(["dojo/_base/xhr",
                             var deleteProviderButton = query(".deleteAuthenticationProvider", contentPane.containerNode)[0];
                             connect.connect(registry.byNode(deleteProviderButton), "onClick",
                                     function(evt){
+                                        var warning = "";
+                                        var data = that.brokerUpdater.authenticationProvidersGrid.grid.selection.getSelected();
+                                        if(data.length && data.length > 0)
+                                        {
+                                          for(var i = 0; i<data.length; i++)
+                                          {
+                                              if (data[i].type.indexOf("File") != -1)
+                                              {
+                                                warning = "NOTE: provider deletion will also remove the password file on disk.\n\n"
+                                                break;
+                                              }
+                                          }
+                                        }
+
                                         util.deleteGridSelections(
                                                 that.brokerUpdater,
                                                 that.brokerUpdater.authenticationProvidersGrid.grid,
                                                 "rest/authenticationprovider",
-                                                "Are you sure you want to delete authentication provider");
+                                                warning + "Are you sure you want to delete authentication provider");
                                 }
                             );
 
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());
     }
 
     /**
diff --git a/qpid/java/systests/src/main/java/org/apache/qpid/systest/rest/AuthenticationProviderRestTest.java b/qpid/java/systests/src/main/java/org/apache/qpid/systest/rest/AuthenticationProviderRestTest.java
index 09408572d7..97c75d8d80 100644
--- a/qpid/java/systests/src/main/java/org/apache/qpid/systest/rest/AuthenticationProviderRestTest.java
+++ b/qpid/java/systests/src/main/java/org/apache/qpid/systest/rest/AuthenticationProviderRestTest.java
@@ -169,6 +169,52 @@ public class AuthenticationProviderRestTest extends QpidRestTestCase
         assertEquals("Unexpected number of providers", 0, providerDetails.size());
     }
 
+    public void testCreateAndDeletePasswordAuthenticationProviderWithNonExistingFile() throws Exception
+    {
+        stopBroker();
+        getBrokerConfiguration().setSaved(false);
+        getBrokerConfiguration().removeObjectConfiguration(TestBrokerConfiguration.ENTRY_NAME_AUTHENTICATION_PROVIDER);
+        getBrokerConfiguration().setObjectAttribute(TestBrokerConfiguration.ENTRY_NAME_AMQP_PORT, Port.AUTHENTICATION_PROVIDER, ANONYMOUS_AUTHENTICATION_PROVIDER);
+        getBrokerConfiguration().setObjectAttribute(TestBrokerConfiguration.ENTRY_NAME_HTTP_PORT, Port.AUTHENTICATION_PROVIDER, ANONYMOUS_AUTHENTICATION_PROVIDER);
+
+        startBroker();
+
+        File file = new File(TMP_FOLDER + File.separator + getTestName());
+        if (file.exists())
+        {
+            file.delete();
+        }
+        assertFalse("File " + file.getAbsolutePath() + " should not exist", file.exists());
+
+        // create provider
+        String providerName = "test-provider";
+        Map<String, Object> attributes = new HashMap<String, Object>();
+        attributes.put(AuthenticationProvider.NAME, providerName);
+        attributes.put(AuthenticationProvider.TYPE, PlainPasswordFileAuthenticationManagerFactory.PROVIDER_TYPE);
+        attributes.put(PlainPasswordFileAuthenticationManagerFactory.ATTRIBUTE_PATH, file.getAbsolutePath());
+
+        int responseCode = getRestTestHelper().submitRequest("/rest/authenticationprovider/" + providerName, "PUT", attributes);
+        assertEquals("Password provider was not created", 201, responseCode);
+
+
+        Map<String, Object> providerDetails = getRestTestHelper().getJsonAsSingletonList("/rest/authenticationprovider/" + providerName);
+        assertNotNull("Providers details cannot be null", providerDetails);
+        assertEquals("Unexpected name", providerName, providerDetails.get(AuthenticationProvider.NAME));
+        assertEquals("Unexpected type", PlainPasswordFileAuthenticationManagerFactory.PROVIDER_TYPE, providerDetails.get(AuthenticationProvider.TYPE));
+        assertEquals("Unexpected path", file.getAbsolutePath(), providerDetails.get(PlainPasswordFileAuthenticationManagerFactory.ATTRIBUTE_PATH));
+
+        assertTrue("User file should be created", file.exists());
+
+        responseCode = getRestTestHelper().submitRequest("/rest/authenticationprovider/" + providerName , "DELETE", null);
+        assertEquals("Unexpected response code for provider deletion", 200, responseCode);
+
+        List<Map<String, Object>> providers = getRestTestHelper().getJsonAsList("/rest/authenticationprovider/" + providerName);
+        assertNotNull("Providers details cannot be null", providers);
+        assertEquals("Unexpected number of providers", 0, providers.size());
+
+        assertFalse("File " + file.getAbsolutePath() + " should be deleted", file.exists());
+    }
+
     private void assertProvider(boolean managesPrincipals, String type, Map<String, Object> provider)
     {
         Asserts.assertAttributesPresent(provider, AuthenticationProvider.AVAILABLE_ATTRIBUTES,