QPID-6436: Address review comments and fix issues caused by ACL refactoring

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

9 files changed, 91 insertions, 34 deletions

diff --git a/qpid/java/bdbstore/src/main/java/org/apache/qpid/server/virtualhost/berkeleydb/BDBHAReplicaVirtualHostImpl.java b/qpid/java/bdbstore/src/main/java/org/apache/qpid/server/virtualhost/berkeleydb/BDBHAReplicaVirtualHostImpl.java
index 205ff57fab..30fff154bb 100644
--- a/qpid/java/bdbstore/src/main/java/org/apache/qpid/server/virtualhost/berkeleydb/BDBHAReplicaVirtualHostImpl.java
+++ b/qpid/java/bdbstore/src/main/java/org/apache/qpid/server/virtualhost/berkeleydb/BDBHAReplicaVirtualHostImpl.java
@@ -47,6 +47,7 @@ import org.apache.qpid.server.model.port.AmqpPort;
 import org.apache.qpid.server.protocol.AMQConnectionModel;
 import org.apache.qpid.server.protocol.LinkRegistry;
 import org.apache.qpid.server.queue.AMQQueue;
+import org.apache.qpid.server.security.SecurityManager;
 import org.apache.qpid.server.stats.StatisticsCounter;
 import org.apache.qpid.server.store.DurableConfigurationStore;
 import org.apache.qpid.server.store.MessageStore;
@@ -355,9 +356,9 @@ public class BDBHAReplicaVirtualHostImpl extends AbstractConfiguredObject<BDBHAR
     }
 
     @Override
-    public org.apache.qpid.server.security.SecurityManager getSecurityManager()
+    public SecurityManager getSecurityManager()
     {
-        return null;
+        return super.getSecurityManager();
     }
 
     @Override
diff --git a/qpid/java/broker-core/src/main/java/org/apache/qpid/server/security/SecurityManager.java b/qpid/java/broker-core/src/main/java/org/apache/qpid/server/security/SecurityManager.java
index 754f6074e3..e0eb083bd4 100755
--- a/qpid/java/broker-core/src/main/java/org/apache/qpid/server/security/SecurityManager.java
+++ b/qpid/java/broker-core/src/main/java/org/apache/qpid/server/security/SecurityManager.java
@@ -38,9 +38,7 @@ import java.util.concurrent.ConcurrentMap;
 
 import javax.security.auth.Subject;
 
-import org.apache.log4j.Logger;
 import org.apache.qpid.server.model.AccessControlProvider;
-import org.apache.qpid.server.model.AuthenticationProvider;
 import org.apache.qpid.server.model.Binding;
 import org.apache.qpid.server.model.Broker;
 import org.apache.qpid.server.model.ConfiguredObject;
@@ -50,17 +48,13 @@ import org.apache.qpid.server.model.Exchange;
 import org.apache.qpid.server.model.ExclusivityPolicy;
 import org.apache.qpid.server.model.Group;
 import org.apache.qpid.server.model.GroupMember;
-import org.apache.qpid.server.model.GroupProvider;
-import org.apache.qpid.server.model.KeyStore;
 import org.apache.qpid.server.model.LifetimePolicy;
 import org.apache.qpid.server.model.Model;
-import org.apache.qpid.server.model.Plugin;
-import org.apache.qpid.server.model.Port;
+import org.apache.qpid.server.model.PreferencesProvider;
 import org.apache.qpid.server.model.Queue;
 import org.apache.qpid.server.model.RemoteReplicationNode;
 import org.apache.qpid.server.model.Session;
 import org.apache.qpid.server.model.State;
-import org.apache.qpid.server.model.TrustStore;
 import org.apache.qpid.server.model.User;
 import org.apache.qpid.server.model.VirtualHost;
 import org.apache.qpid.server.model.VirtualHostAlias;
@@ -77,7 +71,6 @@ import org.apache.qpid.server.security.auth.TaskPrincipal;
 
 public class SecurityManager
 {
-    private static final Logger LOGGER = Logger.getLogger(SecurityManager.class);
 
     private static final Subject SYSTEM = new Subject(true,
                                                      Collections.singleton(new SystemPrincipal()),
@@ -273,7 +266,7 @@ public class SecurityManager
             return;
         }
 
-        if (Operation.CREATE == operation && configuredObject instanceof RemoteReplicationNode)
+        if (isAllowedOperation(operation, configuredObject))
         {
             // creation of remote replication node is out of control for user of this broker
             return;
@@ -283,9 +276,7 @@ public class SecurityManager
         ObjectType objectType = getACLObjectTypeManagingConfiguredObjectOfCategory(categoryClass);
         if (objectType == null)
         {
-            LOGGER.warn("Cannot determine object type for " + configuredObject.getName() + " of category "
-                    + categoryClass + ". Skipping ACL check...");
-            return;
+            throw new IllegalArgumentException("Cannot identify object type for category " + categoryClass );
         }
 
         ObjectProperties properties = getACLObjectProperties(configuredObject, operation);
@@ -316,6 +307,28 @@ public class SecurityManager
         }
     }
 
+    private boolean isAllowedOperation(Operation operation, ConfiguredObject<?> configuredObject)
+    {
+        if (configuredObject instanceof Session && (operation == Operation.CREATE || operation == Operation.UPDATE
+                || operation ==  Operation.DELETE))
+        {
+            return true;
+
+        }
+
+        if (configuredObject instanceof Consumer && (operation == Operation.UPDATE || operation ==  Operation.DELETE))
+        {
+            return true;
+        }
+
+        if (configuredObject instanceof Connection && (operation == Operation.UPDATE || operation ==  Operation.DELETE))
+        {
+            return true;
+        }
+
+        return false;
+    }
+
     private Model getModel()
     {
         return _aclProvidersParent.getModel();
@@ -351,7 +364,7 @@ public class SecurityManager
                 // CREATE GROUP MEMBER is transformed into UPDATE GROUP rule
                 return Operation.UPDATE;
             }
-            else if (isBrokerOrBrokerChild(category))
+            else if (isBrokerOrBrokerChildOrPreferencesProvider(category))
             {
                 // CREATE/UPDATE broker child is transformed into CONFIGURE BROKER rule
                 return Operation.CONFIGURE;
@@ -364,10 +377,11 @@ public class SecurityManager
                 // DELETE BINDING is transformed into UNBIND EXCHANGE rule
                 return Operation.UNBIND;
             }
-            else if (isBrokerOrBrokerChild(category))
+            else if (isBrokerOrBrokerChildOrPreferencesProvider(category))
             {
                 // DELETE broker child is transformed into CONFIGURE BROKER rule
                 return Operation.CONFIGURE;
+
             }
             else if (GroupMember.class.isAssignableFrom(category))
             {
@@ -378,16 +392,11 @@ public class SecurityManager
         return operation;
     }
 
-    private boolean isBrokerOrBrokerChild(Class<? extends ConfiguredObject> category)
+    private boolean isBrokerOrBrokerChildOrPreferencesProvider(Class<? extends ConfiguredObject> category)
     {
-        return Broker.class.isAssignableFrom(category)
-                || Port.class.isAssignableFrom(category)
-                || AuthenticationProvider.class.isAssignableFrom(category)
-                || AccessControlProvider.class.isAssignableFrom(category)
-                || GroupProvider.class.isAssignableFrom(category)
-                || KeyStore.class.isAssignableFrom(category)
-                || TrustStore.class.isAssignableFrom(category)
-                || Plugin.class.isAssignableFrom(category);
+        return Broker.class.isAssignableFrom(category) ||
+               PreferencesProvider.class.isAssignableFrom(category) ||
+               ( !VirtualHostNode.class.isAssignableFrom(category) && getModel().getChildTypes(Broker.class).contains(category));
     }
 
     private ObjectProperties getACLObjectProperties(ConfiguredObject<?> configuredObject, Operation configuredObjectOperation)
@@ -428,7 +437,7 @@ public class SecurityManager
             Queue<?> queue = (Queue<?>)configuredObject.getParent(Queue.class);
             setQueueProperties(queue, properties);
         }
-        else if (isBrokerOrBrokerChild(configuredObjectType))
+        else if (isBrokerOrBrokerChildOrPreferencesProvider(configuredObjectType))
         {
             String description = String.format("%s %s '%s'",
                     configuredObjectOperation == null? null : configuredObjectOperation.name().toLowerCase(),
@@ -474,7 +483,7 @@ public class SecurityManager
         {
             return ObjectType.VIRTUALHOSTNODE;
         }
-        else if (isBrokerOrBrokerChild(category))
+        else if (isBrokerOrBrokerChildOrPreferencesProvider(category))
         {
             return ObjectType.BROKER;
         }
diff --git a/qpid/java/broker-core/src/main/java/org/apache/qpid/server/virtualhostnode/RedirectingVirtualHostImpl.java b/qpid/java/broker-core/src/main/java/org/apache/qpid/server/virtualhostnode/RedirectingVirtualHostImpl.java
index cacc981e9b..917c2fd9a1 100644
--- a/qpid/java/broker-core/src/main/java/org/apache/qpid/server/virtualhostnode/RedirectingVirtualHostImpl.java
+++ b/qpid/java/broker-core/src/main/java/org/apache/qpid/server/virtualhostnode/RedirectingVirtualHostImpl.java
@@ -48,6 +48,7 @@ import org.apache.qpid.server.model.port.AmqpPort;
 import org.apache.qpid.server.protocol.AMQConnectionModel;
 import org.apache.qpid.server.protocol.LinkRegistry;
 import org.apache.qpid.server.queue.AMQQueue;
+import org.apache.qpid.server.security.SecurityManager;
 import org.apache.qpid.server.stats.StatisticsCounter;
 import org.apache.qpid.server.store.DurableConfigurationStore;
 import org.apache.qpid.server.store.MessageStore;
@@ -355,9 +356,9 @@ class RedirectingVirtualHostImpl
     }
 
     @Override
-    public org.apache.qpid.server.security.SecurityManager getSecurityManager()
+    public SecurityManager getSecurityManager()
     {
-        return null;
+        return super.getSecurityManager();
     }
 
     @Override
diff --git a/qpid/java/broker-core/src/test/java/org/apache/qpid/server/model/testmodels/TestSecurityManager.java b/qpid/java/broker-core/src/test/java/org/apache/qpid/server/model/testmodels/TestSecurityManager.java
new file mode 100644
index 0000000000..de2fb8fe74
--- /dev/null
+++ b/qpid/java/broker-core/src/test/java/org/apache/qpid/server/model/testmodels/TestSecurityManager.java
@@ -0,0 +1,40 @@
+/*
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+package org.apache.qpid.server.model.testmodels;
+
+
+import org.apache.qpid.server.model.ConfiguredObject;
+import org.apache.qpid.server.security.SecurityManager;
+import org.apache.qpid.server.security.access.Operation;
+
+public class TestSecurityManager extends SecurityManager
+{
+    public TestSecurityManager(ConfiguredObject<?> aclProvidersParent)
+    {
+        super(aclProvidersParent, false);
+    }
+
+    @Override
+    public void authorise(Operation operation, ConfiguredObject<?> configuredObject)
+    {
+        // noop
+    }
+}
diff --git a/qpid/java/broker-core/src/test/java/org/apache/qpid/server/model/testmodels/hierarchy/TestKitCarImpl.java b/qpid/java/broker-core/src/test/java/org/apache/qpid/server/model/testmodels/hierarchy/TestKitCarImpl.java
index 43dcecd6c8..bc60e0db68 100644
--- a/qpid/java/broker-core/src/test/java/org/apache/qpid/server/model/testmodels/hierarchy/TestKitCarImpl.java
+++ b/qpid/java/broker-core/src/test/java/org/apache/qpid/server/model/testmodels/hierarchy/TestKitCarImpl.java
@@ -25,6 +25,7 @@ import org.apache.qpid.server.model.AbstractConfiguredObject;
 import org.apache.qpid.server.model.ConfiguredObject;
 import org.apache.qpid.server.model.ManagedObject;
 import org.apache.qpid.server.model.ManagedObjectFactoryConstructor;
+import org.apache.qpid.server.model.testmodels.TestSecurityManager;
 import org.apache.qpid.server.security.SecurityManager;
 
 @ManagedObject( category = false,
@@ -39,7 +40,7 @@ public class TestKitCarImpl extends AbstractConfiguredObject<TestKitCarImpl>
     public TestKitCarImpl(final Map<String, Object> attributes)
     {
         super(parentsMap(), attributes, newTaskExecutor(), TestModel.getInstance());
-        _securityManager = new SecurityManager(this, false);
+        _securityManager = new TestSecurityManager(this);
     }
 
     @Override
diff --git a/qpid/java/broker-core/src/test/java/org/apache/qpid/server/model/testmodels/hierarchy/TestStandardCarImpl.java b/qpid/java/broker-core/src/test/java/org/apache/qpid/server/model/testmodels/hierarchy/TestStandardCarImpl.java
index 7582de2952..719e6315ac 100644
--- a/qpid/java/broker-core/src/test/java/org/apache/qpid/server/model/testmodels/hierarchy/TestStandardCarImpl.java
+++ b/qpid/java/broker-core/src/test/java/org/apache/qpid/server/model/testmodels/hierarchy/TestStandardCarImpl.java
@@ -29,6 +29,7 @@ import org.apache.qpid.server.configuration.updater.CurrentThreadTaskExecutor;
 import org.apache.qpid.server.model.AbstractConfiguredObject;
 import org.apache.qpid.server.model.ManagedObject;
 import org.apache.qpid.server.model.ManagedObjectFactoryConstructor;
+import org.apache.qpid.server.model.testmodels.TestSecurityManager;
 import org.apache.qpid.server.security.SecurityManager;
 
 @ManagedObject( category = false,
@@ -44,7 +45,7 @@ public class TestStandardCarImpl extends AbstractConfiguredObject<TestStandardCa
     public TestStandardCarImpl(final Map<String, Object> attributes)
     {
         super(parentsMap(), attributes, newTaskExecutor(), TestModel.getInstance());
-        _securityManager = new SecurityManager(this, false);
+        _securityManager = new TestSecurityManager(this);
     }
 
     private static CurrentThreadTaskExecutor newTaskExecutor()
diff --git a/qpid/java/broker-core/src/test/java/org/apache/qpid/server/model/testmodels/lifecycle/TestConfiguredObject.java b/qpid/java/broker-core/src/test/java/org/apache/qpid/server/model/testmodels/lifecycle/TestConfiguredObject.java
index 6935230aeb..f905a98729 100644
--- a/qpid/java/broker-core/src/test/java/org/apache/qpid/server/model/testmodels/lifecycle/TestConfiguredObject.java
+++ b/qpid/java/broker-core/src/test/java/org/apache/qpid/server/model/testmodels/lifecycle/TestConfiguredObject.java
@@ -38,6 +38,7 @@ import org.apache.qpid.server.model.ManagedObject;
 import org.apache.qpid.server.model.Model;
 import org.apache.qpid.server.model.State;
 import org.apache.qpid.server.model.StateTransition;
+import org.apache.qpid.server.model.testmodels.TestSecurityManager;
 import org.apache.qpid.server.plugin.ConfiguredObjectRegistration;
 import org.apache.qpid.server.security.SecurityManager;
 
@@ -78,7 +79,7 @@ public class TestConfiguredObject extends AbstractConfiguredObject
     {
         super(parents, attributes, taskExecutor, model);
         _opened = false;
-        _securityManager = new SecurityManager(this, false);
+        _securityManager = new TestSecurityManager(this);
     }
 
     @Override
diff --git a/qpid/java/broker-core/src/test/java/org/apache/qpid/server/model/testmodels/singleton/TestSingletonImpl.java b/qpid/java/broker-core/src/test/java/org/apache/qpid/server/model/testmodels/singleton/TestSingletonImpl.java
index 5de40042cc..794c2cfee0 100644
--- a/qpid/java/broker-core/src/test/java/org/apache/qpid/server/model/testmodels/singleton/TestSingletonImpl.java
+++ b/qpid/java/broker-core/src/test/java/org/apache/qpid/server/model/testmodels/singleton/TestSingletonImpl.java
@@ -24,9 +24,11 @@ import java.util.Set;
 import org.apache.qpid.server.configuration.updater.CurrentThreadTaskExecutor;
 import org.apache.qpid.server.configuration.updater.TaskExecutor;
 import org.apache.qpid.server.model.AbstractConfiguredObject;
+import org.apache.qpid.server.model.ConfiguredObject;
 import org.apache.qpid.server.model.ManagedAttributeField;
 import org.apache.qpid.server.model.ManagedObject;
 import org.apache.qpid.server.model.ManagedObjectFactoryConstructor;
+import org.apache.qpid.server.model.testmodels.TestSecurityManager;
 import org.apache.qpid.server.security.SecurityManager;
 
 @ManagedObject( category = false, type = TestSingletonImpl.TEST_SINGLETON_TYPE)
@@ -73,7 +75,7 @@ public class TestSingletonImpl extends AbstractConfiguredObject<TestSingletonImp
     public TestSingletonImpl(final Map<String, Object> attributes)
     {
         super(parentsMap(), attributes, newTaskExecutor(), TestModel.getInstance());
-        _securityManager = new SecurityManager(this, false);
+        _securityManager = new TestSecurityManager(this);
     }
 
     private static CurrentThreadTaskExecutor newTaskExecutor()
@@ -87,7 +89,7 @@ public class TestSingletonImpl extends AbstractConfiguredObject<TestSingletonImp
                              final TaskExecutor taskExecutor)
     {
         super(parentsMap(), attributes, taskExecutor);
-        _securityManager = new SecurityManager(this, false);
+        _securityManager = new TestSecurityManager(this);
     }
 
 
diff --git a/qpid/java/broker-core/src/test/java/org/apache/qpid/server/virtualhost/AbstractVirtualHostTest.java b/qpid/java/broker-core/src/test/java/org/apache/qpid/server/virtualhost/AbstractVirtualHostTest.java
index 1231f5393e..9a7f1fc9a7 100644
--- a/qpid/java/broker-core/src/test/java/org/apache/qpid/server/virtualhost/AbstractVirtualHostTest.java
+++ b/qpid/java/broker-core/src/test/java/org/apache/qpid/server/virtualhost/AbstractVirtualHostTest.java
@@ -66,6 +66,7 @@ public class AbstractVirtualHostTest extends QpidTestCase
         when(systemConfig.getEventLogger()).thenReturn(mock(EventLogger.class));
         Broker<?> broker = mock(Broker.class);
         when(broker.getParent(SystemConfig.class)).thenReturn(systemConfig);
+        when(broker.getModel()).thenReturn(BrokerModel.getInstance());
         when(broker.getSecurityManager()).thenReturn(new SecurityManager(broker, false));
 
         _taskExecutor = new TaskExecutorImpl();