QPID-4661: Improve broker attribute editing UI to avoid sending of unmodified attributes, improve UI look-&-feel, improve broker attribute changing functionality to avoid unnecessary modifications

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

9 files changed, 81 insertions, 45 deletions

diff --git a/java/broker/src/main/java/org/apache/qpid/server/configuration/startup/AuthenticationProviderRecoverer.java b/java/broker/src/main/java/org/apache/qpid/server/configuration/startup/AuthenticationProviderRecoverer.java
index 9b06a2b499..3290c827c9 100644
--- a/java/broker/src/main/java/org/apache/qpid/server/configuration/startup/AuthenticationProviderRecoverer.java
+++ b/java/broker/src/main/java/org/apache/qpid/server/configuration/startup/AuthenticationProviderRecoverer.java
@@ -47,7 +47,7 @@ public class AuthenticationProviderRecoverer implements ConfiguredObjectRecovere
         AuthenticationProvider authenticationProvider = _authenticationProviderFactory.create(
                 configurationEntry.getId(),
                 broker,
-                attributes, null);
+                attributes);
 
         return authenticationProvider;
     }
diff --git a/java/broker/src/main/java/org/apache/qpid/server/configuration/startup/BrokerRecoverer.java b/java/broker/src/main/java/org/apache/qpid/server/configuration/startup/BrokerRecoverer.java
index b76717802c..0d7be75a0b 100644
--- a/java/broker/src/main/java/org/apache/qpid/server/configuration/startup/BrokerRecoverer.java
+++ b/java/broker/src/main/java/org/apache/qpid/server/configuration/startup/BrokerRecoverer.java
@@ -18,7 +18,6 @@ import org.apache.qpid.server.model.adapter.BrokerAdapter;
 import org.apache.qpid.server.model.adapter.PortFactory;
 import org.apache.qpid.server.configuration.store.StoreConfigurationChangeListener;
 import org.apache.qpid.server.configuration.updater.TaskExecutor;
-import org.apache.qpid.server.security.group.GroupPrincipalAccessor;
 import org.apache.qpid.server.stats.StatisticsGatherer;
 import org.apache.qpid.server.virtualhost.VirtualHostRegistry;
 
@@ -103,12 +102,6 @@ public class BrokerRecoverer implements ConfiguredObjectRecoverer<Broker>
         }
         broker.setDefaultAuthenticationProvider(defaultAuthenticationProvider);
 
-        GroupPrincipalAccessor groupPrincipalAccessor = new GroupPrincipalAccessor(broker.getGroupProviders());
-        for (AuthenticationProvider authenticationProvider : authenticationProviders)
-        {
-            authenticationProvider.setGroupAccessor(groupPrincipalAccessor);
-        }
-
         Collection<Port> ports = broker.getPorts();
         for (Port port : ports)
         {
diff --git a/java/broker/src/main/java/org/apache/qpid/server/model/AuthenticationProvider.java b/java/broker/src/main/java/org/apache/qpid/server/model/AuthenticationProvider.java
index 2e5c3a0cc7..61ceae9cf0 100644
--- a/java/broker/src/main/java/org/apache/qpid/server/model/AuthenticationProvider.java
+++ b/java/broker/src/main/java/org/apache/qpid/server/model/AuthenticationProvider.java
@@ -25,7 +25,6 @@ import java.util.Collection;
 import java.util.Collections;
 
 import org.apache.qpid.server.security.SubjectCreator;
-import org.apache.qpid.server.security.group.GroupPrincipalAccessor;
 
 public interface AuthenticationProvider extends ConfiguredObject
 {
@@ -67,5 +66,4 @@ public interface AuthenticationProvider extends ConfiguredObject
      */
     SubjectCreator getSubjectCreator();
 
-    void setGroupAccessor(GroupPrincipalAccessor groupPrincipalAccessor);
 }
diff --git a/java/broker/src/main/java/org/apache/qpid/server/model/adapter/AuthenticationProviderAdapter.java b/java/broker/src/main/java/org/apache/qpid/server/model/adapter/AuthenticationProviderAdapter.java
index a6c5a74765..a7204d991c 100644
--- a/java/broker/src/main/java/org/apache/qpid/server/model/adapter/AuthenticationProviderAdapter.java
+++ b/java/broker/src/main/java/org/apache/qpid/server/model/adapter/AuthenticationProviderAdapter.java
@@ -36,6 +36,7 @@ import javax.security.auth.login.AccountNotFoundException;
 import org.apache.log4j.Logger;
 import org.apache.qpid.server.model.AuthenticationProvider;
 import org.apache.qpid.server.model.Broker;
+import org.apache.qpid.server.model.ConfigurationChangeListener;
 import org.apache.qpid.server.model.ConfiguredObject;
 import org.apache.qpid.server.model.IllegalStateTransitionException;
 import org.apache.qpid.server.model.IntegrityViolationException;
@@ -67,8 +68,6 @@ public abstract class AuthenticationProviderAdapter<T extends AuthenticationMana
     protected T _authManager;
     protected final Broker _broker;
 
-    private GroupPrincipalAccessor _groupAccessor;
-
     protected Collection<String> _supportedAttributes;
     Map<String, AuthenticationManagerFactory> _factories;
 
@@ -238,10 +237,6 @@ public abstract class AuthenticationProviderAdapter<T extends AuthenticationMana
         }
         else if(desiredState == State.ACTIVE)
         {
-            if (_groupAccessor == null)
-            {
-                throw new IllegalStateTransitionException("Cannot transit into ACTIVE state with null group accessor!");
-            }
             _authManager.initialise();
             return true;
         }
@@ -256,12 +251,7 @@ public abstract class AuthenticationProviderAdapter<T extends AuthenticationMana
     @Override
     public SubjectCreator getSubjectCreator()
     {
-        return new SubjectCreator(_authManager, _groupAccessor);
-    }
-
-    public void setGroupAccessor(GroupPrincipalAccessor groupAccessor)
-    {
-        _groupAccessor = groupAccessor;
+        return new SubjectCreator(_authManager, new GroupPrincipalAccessor(_broker.getGroupProviders()));
     }
 
     @Override
@@ -343,7 +333,6 @@ public abstract class AuthenticationProviderAdapter<T extends AuthenticationMana
         }
 
 
-
     }
 
     public static class PrincipalDatabaseAuthenticationManagerAdapter
diff --git a/java/broker/src/main/java/org/apache/qpid/server/model/adapter/AuthenticationProviderFactory.java b/java/broker/src/main/java/org/apache/qpid/server/model/adapter/AuthenticationProviderFactory.java
index 721282fb9c..fbdc2e42b5 100644
--- a/java/broker/src/main/java/org/apache/qpid/server/model/adapter/AuthenticationProviderFactory.java
+++ b/java/broker/src/main/java/org/apache/qpid/server/model/adapter/AuthenticationProviderFactory.java
@@ -33,7 +33,6 @@ import org.apache.qpid.server.plugin.AuthenticationManagerFactory;
 import org.apache.qpid.server.plugin.QpidServiceLoader;
 import org.apache.qpid.server.security.auth.manager.AuthenticationManager;
 import org.apache.qpid.server.security.auth.manager.PrincipalDatabaseAuthenticationManager;
-import org.apache.qpid.server.security.group.GroupPrincipalAccessor;
 import org.apache.qpid.server.model.adapter.AuthenticationProviderAdapter.PrincipalDatabaseAuthenticationManagerAdapter;
 import org.apache.qpid.server.model.adapter.AuthenticationProviderAdapter.SimpleAuthenticationProviderAdapter;
 
@@ -58,9 +57,8 @@ public class AuthenticationProviderFactory
      * <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.
-     * @param groupPrincipalAccessor TODO
      */
-    public AuthenticationProvider create(UUID id, Broker broker, Map<String, Object> attributes, GroupPrincipalAccessor groupPrincipalAccessor)
+    public AuthenticationProvider create(UUID id, Broker broker, Map<String, Object> attributes)
     {
         for (AuthenticationManagerFactory factory : _factories)
         {
@@ -77,7 +75,6 @@ public class AuthenticationProviderFactory
                 {
                     authenticationProvider = new SimpleAuthenticationProviderAdapter(id, broker, manager, attributes, factory.getAttributeNames());
                 }
-                authenticationProvider.setGroupAccessor(groupPrincipalAccessor);
                 return authenticationProvider;
             }
         }
diff --git a/java/broker/src/main/java/org/apache/qpid/server/model/adapter/BrokerAdapter.java b/java/broker/src/main/java/org/apache/qpid/server/model/adapter/BrokerAdapter.java
index fd275b23bd..7e4282f4ee 100644
--- a/java/broker/src/main/java/org/apache/qpid/server/model/adapter/BrokerAdapter.java
+++ b/java/broker/src/main/java/org/apache/qpid/server/model/adapter/BrokerAdapter.java
@@ -63,7 +63,6 @@ import org.apache.qpid.server.security.auth.manager.Base64MD5PasswordFileAuthent
 import org.apache.qpid.server.security.auth.manager.PlainPasswordFileAuthenticationManagerFactory;
 import org.apache.qpid.server.security.group.FileGroupManager;
 import org.apache.qpid.server.security.group.GroupManager;
-import org.apache.qpid.server.security.group.GroupPrincipalAccessor;
 import org.apache.qpid.server.security.SecurityManager;
 import org.apache.qpid.server.security.SubjectCreator;
 import org.apache.qpid.server.stats.StatisticsGatherer;
@@ -209,6 +208,14 @@ public class BrokerAdapter extends AbstractAdapter implements Broker, Configurat
      */
     private void createBrokerChildrenFromAttributes()
     {
+        createGroupProvider();
+        createKeyStore();
+        createTrustStore();
+        createPeerStore();
+    }
+
+    private void createGroupProvider()
+    {
         String groupFile = (String) getAttribute(GROUP_FILE);
         if (groupFile != null)
         {
@@ -222,6 +229,10 @@ public class BrokerAdapter extends AbstractAdapter implements Broker, Configurat
         {
             _groupProviders.remove(DEFAULT_GROUP_PROFIDER_NAME);
         }
+    }
+
+    private void createKeyStore()
+    {
         Map<String, Object> actualAttributes = getActualAttributes();
         String keyStorePath = (String) getAttribute(KEY_STORE_PATH);
         if (keyStorePath != null)
@@ -240,6 +251,11 @@ public class BrokerAdapter extends AbstractAdapter implements Broker, Configurat
         {
             _keyStores.remove(_defaultKeyStoreId);
         }
+    }
+
+    private void createTrustStore()
+    {
+        Map<String, Object> actualAttributes = getActualAttributes();
         String trustStorePath = (String) getAttribute(TRUST_STORE_PATH);
         if (trustStorePath != null)
         {
@@ -257,6 +273,11 @@ public class BrokerAdapter extends AbstractAdapter implements Broker, Configurat
         {
             _trustStores.remove(_defaultTrustStoreId);
         }
+    }
+
+    private void createPeerStore()
+    {
+        Map<String, Object> actualAttributes = getActualAttributes();
         String peerStorePath = (String) getAttribute(PEER_STORE_PATH);
         UUID peerStoreId = UUIDGenerator.generateBrokerChildUUID(TrustStore.class.getSimpleName(), DEFAULT_PEER_STORE_NAME);
         if (peerStorePath != null)
@@ -539,11 +560,7 @@ public class BrokerAdapter extends AbstractAdapter implements Broker, Configurat
 
             }
 
-            // it's cheap to create the groupPrincipalAccessor on the fly
-            GroupPrincipalAccessor groupPrincipalAccessor = new GroupPrincipalAccessor(_groupProviders.values());
-
-            authenticationProvider = _authenticationProviderFactory.create(UUID.randomUUID(), this, attributes,
-                    groupPrincipalAccessor);
+            authenticationProvider = _authenticationProviderFactory.create(UUID.randomUUID(), this, attributes);
             addAuthenticationProvider(authenticationProvider);
         }
         authenticationProvider.setDesiredState(State.INITIALISING, State.ACTIVE);
@@ -1033,15 +1050,63 @@ public class BrokerAdapter extends AbstractAdapter implements Broker, Configurat
         //TODO: Add management mode check
         Map<String, Object> convertedAttributes = MapValueConverter.convert(attributes, ATTRIBUTE_TYPES);
         validateAttributes(convertedAttributes);
-        super.changeAttributes(convertedAttributes);
+
+        boolean keyStoreChanged = false;
+        boolean trustStoreChanged = false;
+        boolean peerStoreChanged = false;
+        Collection<String> names = AVAILABLE_ATTRIBUTES;
+        for (String name : names)
+        {
+            if (attributes.containsKey(name))
+            {
+                Object desired = attributes.get(name);
+                Object expected = getAttribute(name);
+                if (changeAttribute(name, expected, desired))
+                {
+                    if (GROUP_FILE.equals(name))
+                    {
+                        createGroupProvider();
+                    }
+                    else if (DEFAULT_AUTHENTICATION_PROVIDER.equals(name))
+                    {
+                        if (!_defaultAuthenticationProvider.getName().equals(desired))
+                        {
+                            _defaultAuthenticationProvider = findAuthenticationProviderByName((String)desired);
+                        }
+                    }
+                    else if (KEY_STORE_PATH.equals(name) || KEY_STORE_PASSWORD.equals(name) || KEY_STORE_CERT_ALIAS.equals(name))
+                    {
+                        keyStoreChanged = true;
+                    }
+                    else if (TRUST_STORE_PATH.equals(name) || TRUST_STORE_PASSWORD.equals(name))
+                    {
+                        trustStoreChanged = true;
+                    }
+                    else if (PEER_STORE_PATH.equals(name) || PEER_STORE_PASSWORD.equals(name))
+                    {
+                        peerStoreChanged = true;
+                    }
+                    attributeSet(name, expected, desired);
+                }
+            }
+        }
 
         // the calls below are not thread safe but they should be fine in a management mode
         // as there will be no user connected
-        createBrokerChildrenFromAttributes();
-        String defaultProviderName = (String)getAttribute(DEFAULT_AUTHENTICATION_PROVIDER);
-        if (!_defaultAuthenticationProvider.getName().equals(defaultProviderName))
+        // The new keystore/trustore/peerstore will be only used with new ports
+        // At the moment we cannot restart ports with new keystore/trustore/peerstore
+
+        if (keyStoreChanged)
+        {
+            createKeyStore();
+        }
+        if (trustStoreChanged)
+        {
+            createTrustStore();
+        }
+        if (peerStoreChanged)
         {
-            _defaultAuthenticationProvider = findAuthenticationProviderByName(defaultProviderName);
+            createPeerStore();
         }
     }
 
diff --git a/java/broker/src/main/java/org/apache/qpid/server/security/group/GroupPrincipalAccessor.java b/java/broker/src/main/java/org/apache/qpid/server/security/group/GroupPrincipalAccessor.java
index d549b76aab..1b8cdc91bc 100644
--- a/java/broker/src/main/java/org/apache/qpid/server/security/group/GroupPrincipalAccessor.java
+++ b/java/broker/src/main/java/org/apache/qpid/server/security/group/GroupPrincipalAccessor.java
@@ -25,8 +25,6 @@ import java.util.HashSet;
 import java.util.Set;
 
 import org.apache.qpid.server.model.GroupProvider;
-import org.apache.qpid.server.model.adapter.GroupProviderAdapter;
-
 
 public class GroupPrincipalAccessor
 {
diff --git a/java/broker/src/test/java/org/apache/qpid/server/configuration/startup/BrokerRecovererTest.java b/java/broker/src/test/java/org/apache/qpid/server/configuration/startup/BrokerRecovererTest.java
index 8e0bfdaeb7..79dcf0cac4 100644
--- a/java/broker/src/test/java/org/apache/qpid/server/configuration/startup/BrokerRecovererTest.java
+++ b/java/broker/src/test/java/org/apache/qpid/server/configuration/startup/BrokerRecovererTest.java
@@ -53,7 +53,6 @@ import org.apache.qpid.server.model.VirtualHost;
 import org.apache.qpid.server.model.adapter.AuthenticationProviderFactory;
 import org.apache.qpid.server.model.adapter.PortFactory;
 import org.apache.qpid.server.configuration.updater.TaskExecutor;
-import org.apache.qpid.server.security.group.GroupPrincipalAccessor;
 import org.apache.qpid.server.stats.StatisticsGatherer;
 import org.apache.qpid.server.virtualhost.VirtualHostRegistry;
 
@@ -300,9 +299,6 @@ public class BrokerRecovererTest extends TestCase
         assertNotNull(broker);
         assertEquals("Unexpected number of authentication providers", 2, broker.getAuthenticationProviders().size());
 
-        //verify that a GroupAcessor was added to the AuthenticationProviders
-        verify(_authenticationProvider1).setGroupAccessor(any(GroupPrincipalAccessor.class));
-        verify(authenticationProvider2).setGroupAccessor(any(GroupPrincipalAccessor.class));
     }
 
     public void testCreateBrokerWithGroupProvider()
diff --git a/java/broker/src/test/java/org/apache/qpid/server/model/adapter/AuthenticationProviderFactoryTest.java b/java/broker/src/test/java/org/apache/qpid/server/model/adapter/AuthenticationProviderFactoryTest.java
index 585fecae83..9bf80bb87e 100644
--- a/java/broker/src/test/java/org/apache/qpid/server/model/adapter/AuthenticationProviderFactoryTest.java
+++ b/java/broker/src/test/java/org/apache/qpid/server/model/adapter/AuthenticationProviderFactoryTest.java
@@ -74,7 +74,7 @@ public class AuthenticationProviderFactoryTest extends TestCase
         when(authenticationManagerFactory.createInstance(attributes)).thenReturn(authenticationManager);
 
         AuthenticationProviderFactory providerFactory = new AuthenticationProviderFactory(authManagerFactoryServiceLoader);
-        AuthenticationProvider provider = providerFactory.create(id, broker, attributes, null);
+        AuthenticationProvider provider = providerFactory.create(id, broker, attributes);
 
         assertNotNull("Provider is not created", provider);
         assertEquals("Unexpected ID", id, provider.getId());