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@1459695 13f79535-47bb-0310-9956-ffa450edef68

10 files changed, 108 insertions, 55 deletions

diff --git a/qpid/java/broker-plugins/management-http/src/main/java/resources/js/qpid/common/util.js b/qpid/java/broker-plugins/management-http/src/main/java/resources/js/qpid/common/util.js
index 6f3049ec0d..fea67d5942 100644
--- a/qpid/java/broker-plugins/management-http/src/main/java/resources/js/qpid/common/util.js
+++ b/qpid/java/broker-plugins/management-http/src/main/java/resources/js/qpid/common/util.js
@@ -23,17 +23,21 @@ define(["dojo/_base/xhr",
         "dojo/_base/event",
         "dojo/_base/json",
         "dojo/_base/lang",
+        "dojo/dom-construct",
+        "dojo/dom-geometry",
+        "dojo/window",
         "dijit/Dialog",
         "dijit/form/Form",
         "dijit/form/Button",
         "dijit/form/RadioButton",
         "dijit/form/CheckBox",
         "dojox/layout/TableContainer",
+        "dojox/layout/ScrollPane",
         "dojox/validate/us",
         "dojox/validate/web",
         "dojo/domReady!"
         ],
-       function (xhr, event, json, lang) {
+       function (xhr, event, json, lang, dom, geometry, win) {
            var util = {};
            if (Array.isArray) {
                util.isArray = function (object) {
@@ -147,8 +151,16 @@ define(["dojo/_base/xhr",
               });
               var submitButton = new dijit.form.Button({label: "Submit", type: "submit"});
               var form = new dijit.form.Form();
-              form.domNode.appendChild(layout.domNode);
-              form.domNode.appendChild(submitButton.domNode);
+
+              var dialogContent = dom.create("div");
+              var dialogContentArea = dom.create("div", { "class": "dijitDialogPaneContentArea"});
+              var dialogActionBar = dom.create("div", { "class": "dijitDialogPaneActionBar"} );
+              dialogContent.appendChild(dialogContentArea);
+              dialogContent.appendChild(dialogActionBar);
+              dialogContentArea.appendChild(layout.domNode)
+              dialogActionBar.appendChild(submitButton.domNode);
+              form.domNode.appendChild(dialogContent);
+
               var widgets = {};
               var requiredFor ={};
               for(var i in attributeWidgetFactories)
@@ -170,10 +182,6 @@ define(["dojo/_base/xhr",
               {
                 var dependent = requiredFor[widgetName];
                 var widget = widgets[widgetName];
-                if (widget.value)
-                {
-                  dependent.set("required", true);
-                }
                 if (widget)
                 {
                   widget.dependent = dependent;
@@ -185,7 +193,7 @@ define(["dojo/_base/xhr",
               var setAttributesDialog = new dijit.Dialog({
                  title: dialogTitle,
                  content: form,
-                 style: "width: 600px"
+                 style: "width: 600px; max-height: 80%"
              });
              form.on("submit", function(e)
              {
@@ -204,7 +212,7 @@ define(["dojo/_base/xhr",
                        {
                          values[ propName ] = widget.checked;
                        }
-                       else if (value != "" || (widget.initialValue && value != widget.initialValue))
+                       else if (value != widget.initialValue)
                        {
                          values[ propName ] = value ? value: null;
                        }
@@ -239,8 +247,17 @@ define(["dojo/_base/xhr",
                }
              });
              form.connectChildren(true);
+             setAttributesDialog.startup();
+             setAttributesDialog.on("show", function(){
+               var data = geometry.position(layout.domNode);
+               var maxHeight = win.getBox().h * 0.6;
+               if (data.h > maxHeight)
+               {
+                 dialogContentArea.style.height = maxHeight + "px";
+                 dialogContentArea.style.overflow= "auto";
+               }
+             })
              setAttributesDialog.show();
-
            };
 
            return util;
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 9b06a2b499..3290c827c9 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
@@ -47,7 +47,7 @@ public class AuthenticationProviderRecoverer implements ConfiguredObjectRecovere
         AuthenticationProvider authenticationProvider = _authenticationProviderFactory.create(
                 configurationEntry.getId(),
                 broker,
-                attributes, null);
+                attributes);
 
         return authenticationProvider;
     }
diff --git a/qpid/java/broker/src/main/java/org/apache/qpid/server/configuration/startup/BrokerRecoverer.java b/qpid/java/broker/src/main/java/org/apache/qpid/server/configuration/startup/BrokerRecoverer.java
index b76717802c..0d7be75a0b 100644
--- a/qpid/java/broker/src/main/java/org/apache/qpid/server/configuration/startup/BrokerRecoverer.java
+++ b/qpid/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/qpid/java/broker/src/main/java/org/apache/qpid/server/model/AuthenticationProvider.java b/qpid/java/broker/src/main/java/org/apache/qpid/server/model/AuthenticationProvider.java
index 2e5c3a0cc7..61ceae9cf0 100644
--- a/qpid/java/broker/src/main/java/org/apache/qpid/server/model/AuthenticationProvider.java
+++ b/qpid/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/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 a6c5a74765..a7204d991c 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
@@ -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/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 721282fb9c..fbdc2e42b5 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
@@ -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/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 fd275b23bd..7e4282f4ee 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
@@ -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/qpid/java/broker/src/main/java/org/apache/qpid/server/security/group/GroupPrincipalAccessor.java b/qpid/java/broker/src/main/java/org/apache/qpid/server/security/group/GroupPrincipalAccessor.java
index d549b76aab..1b8cdc91bc 100644
--- a/qpid/java/broker/src/main/java/org/apache/qpid/server/security/group/GroupPrincipalAccessor.java
+++ b/qpid/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/qpid/java/broker/src/test/java/org/apache/qpid/server/configuration/startup/BrokerRecovererTest.java b/qpid/java/broker/src/test/java/org/apache/qpid/server/configuration/startup/BrokerRecovererTest.java
index 8e0bfdaeb7..79dcf0cac4 100644
--- a/qpid/java/broker/src/test/java/org/apache/qpid/server/configuration/startup/BrokerRecovererTest.java
+++ b/qpid/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/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 585fecae83..9bf80bb87e 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
@@ -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());