QPID-5873: [Java Broker] Allow ACL rules to be applied to VirtualHostNode objects

* ACL rules using the new operation VIRTUALHOSTNODE apply to VHN model objects.
* ACL rules using the operation VIRTUALHOST apply to VH model objects for CREATE, UPDATE and DELETE.  This
  is a change from previous version where BROKER operation permission was required.
* For HA, VIRTUALHOSTNODE permission is required to perform updates on RemoteReplicationNodes.

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

7 files changed, 284 insertions, 24 deletions

diff --git a/java/broker-core/src/main/java/org/apache/qpid/server/model/adapter/BrokerAdapter.java b/java/broker-core/src/main/java/org/apache/qpid/server/model/adapter/BrokerAdapter.java
index 77eda0dc67..2aba33104a 100644
--- a/java/broker-core/src/main/java/org/apache/qpid/server/model/adapter/BrokerAdapter.java
+++ b/java/broker-core/src/main/java/org/apache/qpid/server/model/adapter/BrokerAdapter.java
@@ -895,9 +895,19 @@ public class BrokerAdapter extends AbstractConfiguredObject<BrokerAdapter> imple
     protected <C extends ConfiguredObject> void authoriseCreateChild(Class<C> childClass, Map<String, Object> attributes,
             ConfiguredObject... otherParents) throws AccessControlException
     {
-        if (!_securityManager.authoriseConfiguringBroker(String.valueOf(attributes.get(NAME)), childClass, Operation.CREATE))
+        if (childClass == VirtualHostNode.class)
         {
-            throw new AccessControlException("Creation of new broker level entity is denied");
+            _securityManager.authoriseVirtualHostNode(String.valueOf(attributes.get(NAME)), Operation.CREATE);
+
+        }
+        else
+        {
+            if (!_securityManager.authoriseConfiguringBroker(String.valueOf(attributes.get(NAME)),
+                                                             childClass,
+                                                             Operation.CREATE))
+            {
+                throw new AccessControlException("Creation of new broker level entity is denied");
+            }
         }
     }
 
diff --git a/java/broker-core/src/main/java/org/apache/qpid/server/security/SecurityManager.java b/java/broker-core/src/main/java/org/apache/qpid/server/security/SecurityManager.java
index a8a59a317c..2f7acba91d 100755
--- a/java/broker-core/src/main/java/org/apache/qpid/server/security/SecurityManager.java
+++ b/java/broker-core/src/main/java/org/apache/qpid/server/security/SecurityManager.java
@@ -25,6 +25,7 @@ import static org.apache.qpid.server.security.access.ObjectType.METHOD;
 import static org.apache.qpid.server.security.access.ObjectType.QUEUE;
 import static org.apache.qpid.server.security.access.ObjectType.USER;
 import static org.apache.qpid.server.security.access.ObjectType.VIRTUALHOST;
+import static org.apache.qpid.server.security.access.ObjectType.VIRTUALHOSTNODE;
 import static org.apache.qpid.server.security.access.Operation.*;
 
 import java.security.AccessControlException;
@@ -242,9 +243,24 @@ public class SecurityManager implements ConfigurationChangeListener
         }
     }
 
-    public void authoriseCreateConnection(final AMQConnectionModel connection)
+    public void authoriseVirtualHostNode(final String virtualHostNodeName, final Operation operation)
+    {
+        if(!checkAllPlugins(new AccessCheck()
+        {
+            Result allowed(AccessControl plugin)
+            {
+                ObjectProperties properties = new ObjectProperties(virtualHostNodeName);
+                return plugin.authorise(operation, VIRTUALHOSTNODE, properties);
+            }
+        }))
+        {
+            throw new AccessControlException(operation + " permission denied for " + VIRTUALHOSTNODE
+                                             + " : " + virtualHostNodeName);
+        }
+    }
+
+    public void authoriseVirtualHost(final String virtualHostName, final Operation operation)
     {
-        final String virtualHostName = connection.getVirtualHostName();
         if(!checkAllPlugins(new AccessCheck()
         {
             Result allowed(AccessControl plugin)
@@ -252,10 +268,24 @@ public class SecurityManager implements ConfigurationChangeListener
                 // We put the name into the properties under both name and virtualhost_name so the user may express predicates using either.
                 ObjectProperties properties = new ObjectProperties(virtualHostName);
                 properties.put(Property.VIRTUALHOST_NAME, virtualHostName);
-                return plugin.authorise(Operation.ACCESS, VIRTUALHOST, properties);
+                return plugin.authorise(operation, VIRTUALHOST, properties);
             }
         }))
         {
+            throw new AccessControlException(operation + " permission denied for " + VIRTUALHOST
+                                             + " : " + virtualHostName);
+        }
+    }
+
+    public void authoriseCreateConnection(final AMQConnectionModel connection)
+    {
+        String virtualHostName = connection.getVirtualHostName();
+        try
+        {
+            authoriseVirtualHost(virtualHostName, Operation.ACCESS);
+        }
+        catch (AccessControlException ace)
+        {
             throw new AccessControlException("Permission denied: " + virtualHostName);
         }
     }
diff --git a/java/broker-core/src/main/java/org/apache/qpid/server/security/access/ObjectType.java b/java/broker-core/src/main/java/org/apache/qpid/server/security/access/ObjectType.java
index 9016205d1c..88b1a6b727 100644
--- a/java/broker-core/src/main/java/org/apache/qpid/server/security/access/ObjectType.java
+++ b/java/broker-core/src/main/java/org/apache/qpid/server/security/access/ObjectType.java
@@ -42,7 +42,8 @@ import java.util.Set;
 public enum ObjectType
 {
     ALL(Operation.ALL),
-    VIRTUALHOST(Operation.ALL, ACCESS),
+    VIRTUALHOSTNODE(Operation.ALL, CREATE, DELETE, UPDATE),
+    VIRTUALHOST(Operation.ALL, ACCESS, CREATE, DELETE, UPDATE),
     MANAGEMENT(Operation.ALL, ACCESS),
     QUEUE(Operation.ALL, CREATE, DELETE, PURGE, CONSUME, UPDATE),
     EXCHANGE(Operation.ALL, ACCESS, CREATE, DELETE, BIND, UNBIND, PUBLISH, UPDATE),
diff --git a/java/broker-core/src/main/java/org/apache/qpid/server/virtualhost/AbstractVirtualHost.java b/java/broker-core/src/main/java/org/apache/qpid/server/virtualhost/AbstractVirtualHost.java
index 3c63c5b869..fa6da37d45 100644
--- a/java/broker-core/src/main/java/org/apache/qpid/server/virtualhost/AbstractVirtualHost.java
+++ b/java/broker-core/src/main/java/org/apache/qpid/server/virtualhost/AbstractVirtualHost.java
@@ -318,20 +318,18 @@ public abstract class AbstractVirtualHost<X extends AbstractVirtualHost<X>> exte
     {
         if(desiredState == State.DELETED)
         {
-            if (!_broker.getSecurityManager().authoriseConfiguringBroker(getName(), org.apache.qpid.server.model.VirtualHost.class, Operation.DELETE))
-            {
-                throw new AccessControlException("Deletion of virtual host is denied");
-            }
+            _broker.getSecurityManager().authoriseVirtualHost(getName(), Operation.DELETE);
+        }
+        else
+        {
+            _broker.getSecurityManager().authoriseVirtualHost(getName(), Operation.UPDATE);
         }
     }
 
     @Override
     protected void authoriseSetAttributes(ConfiguredObject<?> modified, Set<String> attributes) throws AccessControlException
     {
-        if (!_broker.getSecurityManager().authoriseConfiguringBroker(getName(), org.apache.qpid.server.model.VirtualHost.class, Operation.UPDATE))
-        {
-            throw new AccessControlException("Setting of virtual host attributes is denied");
-        }
+        _broker.getSecurityManager().authoriseVirtualHost(getName(), Operation.UPDATE);
     }
 
     public Collection<Connection> getConnections()
diff --git a/java/broker-core/src/main/java/org/apache/qpid/server/virtualhostnode/AbstractVirtualHostNode.java b/java/broker-core/src/main/java/org/apache/qpid/server/virtualhostnode/AbstractVirtualHostNode.java
index 0cb69e4cd8..b21fcd704f 100644
--- a/java/broker-core/src/main/java/org/apache/qpid/server/virtualhostnode/AbstractVirtualHostNode.java
+++ b/java/broker-core/src/main/java/org/apache/qpid/server/virtualhostnode/AbstractVirtualHostNode.java
@@ -197,20 +197,36 @@ public abstract class AbstractVirtualHostNode<X extends AbstractVirtualHostNode<
     {
         if(desiredState == State.DELETED)
         {
-            if (!_broker.getSecurityManager().authoriseConfiguringBroker(getName(), VirtualHostNode.class, Operation.DELETE))
-            {
-                throw new AccessControlException("Deletion of virtual host node is denied");
-            }
+            _broker.getSecurityManager().authoriseVirtualHostNode(getName(), Operation.DELETE);
+        }
+        else
+        {
+            _broker.getSecurityManager().authoriseVirtualHostNode(getName(), Operation.UPDATE);
         }
     }
 
     @Override
-    protected void authoriseSetAttributes(ConfiguredObject<?> modified, Set<String> attributes) throws AccessControlException
+    protected <C extends ConfiguredObject> void authoriseCreateChild(final Class<C> childClass,
+                                                                     final Map<String, Object> attributes,
+                                                                     final ConfiguredObject... otherParents)
+            throws AccessControlException
     {
-        if (!_broker.getSecurityManager().authoriseConfiguringBroker(getName(), VirtualHostNode.class, Operation.UPDATE))
+        if (childClass == VirtualHost.class)
         {
-            throw new AccessControlException("Setting of virtual host node attributes is denied");
+            _broker.getSecurityManager().authoriseVirtualHost(String.valueOf(attributes.get(VirtualHost.NAME)),
+                                                              Operation.CREATE);
+
         }
+        else
+        {
+            super.authoriseCreateChild(childClass, attributes, otherParents);
+        }
+    }
+
+    @Override
+    protected void authoriseSetAttributes(ConfiguredObject<?> modified, Set<String> attributes) throws AccessControlException
+    {
+        _broker.getSecurityManager().authoriseVirtualHostNode(getName(), Operation.UPDATE);
     }
 
     private void closeConfigurationStore()
diff --git a/java/broker-core/src/test/java/org/apache/qpid/server/model/VirtualHostTest.java b/java/broker-core/src/test/java/org/apache/qpid/server/model/VirtualHostTest.java
index b69af13c13..d12f0bc5f9 100644
--- a/java/broker-core/src/test/java/org/apache/qpid/server/model/VirtualHostTest.java
+++ b/java/broker-core/src/test/java/org/apache/qpid/server/model/VirtualHostTest.java
@@ -22,12 +22,15 @@ package org.apache.qpid.server.model;
 
 import static org.mockito.Matchers.argThat;
 import static org.mockito.Matchers.eq;
+import static org.mockito.Mockito.doThrow;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.never;
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.verifyNoMoreInteractions;
 import static org.mockito.Mockito.when;
 
+import java.security.AccessControlException;
 import java.util.HashMap;
 import java.util.Map;
 import java.util.UUID;
@@ -36,6 +39,8 @@ import org.mockito.ArgumentMatcher;
 
 import org.apache.qpid.server.configuration.updater.CurrentThreadTaskExecutor;
 import org.apache.qpid.server.configuration.updater.TaskExecutor;
+import org.apache.qpid.server.security.SecurityManager;
+import org.apache.qpid.server.security.access.Operation;
 import org.apache.qpid.server.store.ConfiguredObjectRecord;
 import org.apache.qpid.server.store.DurableConfigurationStore;
 import org.apache.qpid.server.util.BrokerTestHelper;
@@ -44,6 +49,7 @@ import org.apache.qpid.test.utils.QpidTestCase;
 
 public class VirtualHostTest extends QpidTestCase
 {
+    private final SecurityManager _mockSecurityManager = mock(SecurityManager.class);
     private Broker _broker;
     private TaskExecutor _taskExecutor;
     private VirtualHostNode<?> _virtualHostNode;
@@ -180,6 +186,82 @@ public class VirtualHostTest extends QpidTestCase
         verify(_configStore, never()).create(matchesRecord(queue.getId(), queue.getType()));
     }
 
+    // ***************  VH Access Control Tests  ***************
+
+    public void testUpdateDeniedByACL()
+    {
+        when(_broker.getSecurityManager()).thenReturn(_mockSecurityManager);
+
+        String virtualHostName = getName();
+        VirtualHost<?,?,?> virtualHost = createVirtualHost(virtualHostName);
+
+        doThrow(new AccessControlException("mocked ACL exception")).when(_mockSecurityManager).authoriseVirtualHost(
+                virtualHostName,
+                Operation.UPDATE);
+
+        assertNull(virtualHost.getDescription());
+
+        try
+        {
+            virtualHost.setAttribute(VirtualHost.DESCRIPTION, null, "My description");
+            fail("Exception not thrown");
+        }
+        catch (AccessControlException ace)
+        {
+            // PASS
+        }
+
+        verify(_configStore, never()).update(eq(false), matchesRecord(virtualHost.getId(), virtualHost.getType()));
+    }
+
+    public void testStopDeniedByACL()
+    {
+        when(_broker.getSecurityManager()).thenReturn(_mockSecurityManager);
+
+        String virtualHostName = getName();
+        VirtualHost<?,?,?> virtualHost = createVirtualHost(virtualHostName);
+
+        doThrow(new AccessControlException("mocked ACL exception")).when(_mockSecurityManager).authoriseVirtualHost(
+                virtualHostName,
+                Operation.UPDATE);
+
+        try
+        {
+            virtualHost.stop();
+            fail("Exception not thrown");
+        }
+        catch (AccessControlException ace)
+        {
+            // PASS
+        }
+
+        verify(_configStore, never()).update(eq(false), matchesRecord(virtualHost.getId(), virtualHost.getType()));
+    }
+
+    public void testDeleteDeniedByACL()
+    {
+        when(_broker.getSecurityManager()).thenReturn(_mockSecurityManager);
+
+        String virtualHostName = getName();
+        VirtualHost<?,?,?> virtualHost = createVirtualHost(virtualHostName);
+
+        doThrow(new AccessControlException("mocked ACL exception")).when(_mockSecurityManager).authoriseVirtualHost(
+                virtualHostName,
+                Operation.DELETE);
+
+        try
+        {
+            virtualHost.delete();
+            fail("Exception not thrown");
+        }
+        catch (AccessControlException ace)
+        {
+            // PASS
+        }
+
+        verify(_configStore, never()).remove(matchesRecord(virtualHost.getId(), virtualHost.getType()));
+    }
+
     private VirtualHost<?,?,?> createVirtualHost(final String virtualHostName)
     {
         Map<String, Object> attributes = new HashMap<>();
diff --git a/java/broker-core/src/test/java/org/apache/qpid/server/virtualhostnode/AbstractStandardVirtualHostNodeTest.java b/java/broker-core/src/test/java/org/apache/qpid/server/virtualhostnode/AbstractStandardVirtualHostNodeTest.java
index ea66911a89..d7c35ba6a8 100644
--- a/java/broker-core/src/test/java/org/apache/qpid/server/virtualhostnode/AbstractStandardVirtualHostNodeTest.java
+++ b/java/broker-core/src/test/java/org/apache/qpid/server/virtualhostnode/AbstractStandardVirtualHostNodeTest.java
@@ -21,9 +21,11 @@
 package org.apache.qpid.server.virtualhostnode;
 
 import static org.apache.qpid.server.virtualhostnode.AbstractStandardVirtualHostNode.*;
+import static org.mockito.Mockito.doThrow;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
 
+import java.security.AccessControlException;
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.Map;
@@ -39,6 +41,8 @@ import org.apache.qpid.server.model.State;
 import org.apache.qpid.server.model.SystemContext;
 import org.apache.qpid.server.model.VirtualHost;
 import org.apache.qpid.server.model.VirtualHostNode;
+import org.apache.qpid.server.security.SecurityManager;
+import org.apache.qpid.server.security.access.Operation;
 import org.apache.qpid.server.store.ConfiguredObjectRecord;
 import org.apache.qpid.server.store.DurableConfigurationStore;
 import org.apache.qpid.server.store.NullMessageStore;
@@ -53,7 +57,8 @@ public class AbstractStandardVirtualHostNodeTest extends QpidTestCase
     private static final String TEST_VIRTUAL_HOST_NODE_NAME = "testNode";
     private static final String TEST_VIRTUAL_HOST_NAME = "testVirtualHost";
 
-    private UUID _nodeId = UUID.randomUUID();
+    private final UUID _nodeId = UUID.randomUUID();
+    private final SecurityManager _mockSecurityManager = mock(SecurityManager.class);
     private Broker<?> _broker;
     private TaskExecutor _taskExecutor;
 
@@ -109,7 +114,6 @@ public class AbstractStandardVirtualHostNodeTest extends QpidTestCase
         assertEquals("Unexpected virtual host id", virtualHostId, virtualHost.getId());
     }
 
-
     /**
      *  Tests activating a virtualhostnode with a config store which does not specify
      *  a virtualhost.  Checks no virtualhost is created.
@@ -128,7 +132,6 @@ public class AbstractStandardVirtualHostNodeTest extends QpidTestCase
 
         VirtualHost<?, ?, ?> virtualHost = node.getVirtualHost();
         assertNull("Virtual host should not be automatically created", virtualHost);
-
     }
 
     /**
@@ -233,6 +236,125 @@ public class AbstractStandardVirtualHostNodeTest extends QpidTestCase
         assertEquals("Unexpected virtual host id", virtualHostId, virtualHost.getId());
     }
 
+    public void testStopStartVHN() throws Exception
+    {
+        DurableConfigurationStore configStore = configStoreThatProducesNoRecords();
+
+        Map<String, Object> nodeAttributes = new HashMap<>();
+        nodeAttributes.put(VirtualHostNode.NAME, TEST_VIRTUAL_HOST_NODE_NAME);
+        nodeAttributes.put(VirtualHostNode.ID, _nodeId);
+
+        VirtualHostNode<?> node = new TestVirtualHostNode(_broker, nodeAttributes, configStore);
+        node.open();
+        node.start();
+
+        assertEquals("Unexpected virtual host node state", State.ACTIVE, node.getState());
+
+        node.stop();
+        assertEquals("Unexpected virtual host node state after stop", State.STOPPED, node.getState());
+
+        node.start();
+        assertEquals("Unexpected virtual host node state after start", State.ACTIVE, node.getState());
+    }
+
+
+    // ***************  VHN Access Control Tests  ***************
+
+    public void testUpdateVHNDeniedByACL() throws Exception
+    {
+        when(_broker.getSecurityManager()).thenReturn(_mockSecurityManager);
+
+        DurableConfigurationStore configStore = configStoreThatProducesNoRecords();
+
+        Map<String, Object> nodeAttributes = new HashMap<>();
+        nodeAttributes.put(VirtualHostNode.NAME, TEST_VIRTUAL_HOST_NODE_NAME);
+        nodeAttributes.put(VirtualHostNode.ID, _nodeId);
+
+        VirtualHostNode<?> node = new TestVirtualHostNode(_broker, nodeAttributes, configStore);
+        node.open();
+        node.start();
+
+        doThrow(new AccessControlException("mocked ACL exception")).when(_mockSecurityManager).authoriseVirtualHostNode(
+                TEST_VIRTUAL_HOST_NODE_NAME,
+                Operation.UPDATE);
+
+        assertNull(node.getDescription());
+        try
+        {
+            node.setAttribute(VirtualHostNode.DESCRIPTION, null, "My virtualhost node");
+            fail("Exception not throws");
+        }
+        catch (AccessControlException ace)
+        {
+            // PASS
+        }
+        assertNull("Description unexpected updated", node.getDescription());
+    }
+
+    public void testDeleteVHNDeniedByACL() throws Exception
+    {
+        SecurityManager mockSecurityManager = mock(SecurityManager.class);
+        when(_broker.getSecurityManager()).thenReturn(mockSecurityManager);
+
+        DurableConfigurationStore configStore = configStoreThatProducesNoRecords();
+
+        Map<String, Object> nodeAttributes = new HashMap<>();
+        nodeAttributes.put(VirtualHostNode.NAME, TEST_VIRTUAL_HOST_NODE_NAME);
+        nodeAttributes.put(VirtualHostNode.ID, _nodeId);
+
+        VirtualHostNode<?> node = new TestVirtualHostNode(_broker, nodeAttributes, configStore);
+        node.open();
+        node.start();
+
+        doThrow(new AccessControlException("mocked ACL exception")).when(mockSecurityManager).authoriseVirtualHostNode(
+                TEST_VIRTUAL_HOST_NODE_NAME,
+                Operation.DELETE);
+
+        try
+        {
+            node.delete();
+            fail("Exception not throws");
+        }
+        catch (AccessControlException ace)
+        {
+            // PASS
+        }
+
+        assertEquals("Virtual host node state changed unexpectedly", State.ACTIVE, node.getState());
+    }
+
+    public void testStopVHNDeniedByACL() throws Exception
+    {
+        SecurityManager mockSecurityManager = mock(SecurityManager.class);
+        when(_broker.getSecurityManager()).thenReturn(mockSecurityManager);
+
+        DurableConfigurationStore configStore = configStoreThatProducesNoRecords();
+
+        Map<String, Object> nodeAttributes = new HashMap<>();
+        nodeAttributes.put(VirtualHostNode.NAME, TEST_VIRTUAL_HOST_NODE_NAME);
+        nodeAttributes.put(VirtualHostNode.ID, _nodeId);
+
+        VirtualHostNode<?> node = new TestVirtualHostNode(_broker, nodeAttributes, configStore);
+        node.open();
+        node.start();
+
+        doThrow(new AccessControlException("mocked ACL exception")).when(mockSecurityManager).authoriseVirtualHostNode(
+                TEST_VIRTUAL_HOST_NODE_NAME,
+                Operation.UPDATE);
+
+        try
+        {
+            node.stop();
+            fail("Exception not throws");
+        }
+        catch (AccessControlException ace)
+        {
+            // PASS
+        }
+
+        assertEquals("Virtual host node state changed unexpectedly", State.ACTIVE, node.getState());
+    }
+
     private ConfiguredObjectRecord createVirtualHostConfiguredObjectRecord(UUID virtualHostId)
     {
         Map<String, Object> virtualHostAttributes = new HashMap<>();
@@ -263,6 +385,7 @@ public class AbstractStandardVirtualHostNodeTest extends QpidTestCase
             }
         };
     }
+
     private NullMessageStore configStoreThatProducesNoRecords()
     {
         return configStoreThatProduces(null);