QPID-6306 : [Java Broker] Restrict broker to single ACL Provider at any given time

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

5 files changed, 53 insertions, 243 deletions

diff --git a/qpid/java/broker-core/src/main/java/org/apache/qpid/server/model/adapter/BrokerAdapter.java b/qpid/java/broker-core/src/main/java/org/apache/qpid/server/model/adapter/BrokerAdapter.java
index 5975d64f19..28eea21093 100644
--- a/qpid/java/broker-core/src/main/java/org/apache/qpid/server/model/adapter/BrokerAdapter.java
+++ b/qpid/java/broker-core/src/main/java/org/apache/qpid/server/model/adapter/BrokerAdapter.java
@@ -180,6 +180,14 @@ public class BrokerAdapter extends AbstractConfiguredObject<BrokerAdapter> imple
             deleted();
             throw new IllegalArgumentException(getClass().getSimpleName() + " must be durable");
         }
+
+        Collection<AccessControlProvider<?>> accessControlProviders = getAccessControlProviders();
+
+        if(accessControlProviders != null && accessControlProviders.size() > 1)
+        {
+            deleted();
+            throw new IllegalArgumentException("At most one AccessControlProvider can be defined");
+        }
     }
 
     @Override
@@ -242,14 +250,7 @@ public class BrokerAdapter extends AbstractConfiguredObject<BrokerAdapter> imple
             if (children != null) {
                 for (final ConfiguredObject<?> child : children) {
 
-                    if (child instanceof  AccessControlProvider)
-                    {
-                        addAccessControlProvider((AccessControlProvider)child);
-                    }
-                    else
-                    {
-                        child.addChangeListener(this);
-                    }
+                    child.addChangeListener(this);
 
                     if (child.getState() == State.ERRORED )
                     {
@@ -578,23 +579,18 @@ public class BrokerAdapter extends AbstractConfiguredObject<BrokerAdapter> imple
 
     private AccessControlProvider<?> createAccessControlProvider(final Map<String, Object> attributes)
     {
+        final Collection<AccessControlProvider<?>> currentProviders = getAccessControlProviders();
+        if(currentProviders != null && !currentProviders.isEmpty())
+        {
+            throw new IllegalConfigurationException("Cannot add a second AccessControlProvider");
+        }
         AccessControlProvider<?> accessControlProvider = (AccessControlProvider<?>) createChild(AccessControlProvider.class, attributes);
-        addAccessControlProvider(accessControlProvider);
+        accessControlProvider.addChangeListener(this);
 
         return accessControlProvider;
 
     }
 
-    private void addAccessControlProvider(final AccessControlProvider<?> accessControlProvider)
-    {
-        accessControlProvider.addChangeListener(this);
-        accessControlProvider.addChangeListener(_securityManager);
-        if(accessControlProvider.getState() == State.ACTIVE)
-        {
-            _securityManager.addPlugin(accessControlProvider.getAccessControl());
-        }
-    }
-
     private boolean deleteAccessControlProvider(AccessControlProvider<?> accessControlProvider)
     {
         accessControlProvider.removeChangeListener(this);
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 96bd9ee0d6..71878dcf72 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
@@ -44,7 +44,6 @@ import org.apache.qpid.server.consumer.ConsumerImpl;
 import org.apache.qpid.server.exchange.ExchangeImpl;
 import org.apache.qpid.server.model.AccessControlProvider;
 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.State;
 import org.apache.qpid.server.protocol.AMQConnectionModel;
@@ -57,18 +56,17 @@ import org.apache.qpid.server.security.access.OperationLoggingDetails;
 import org.apache.qpid.server.security.auth.AuthenticatedPrincipal;
 import org.apache.qpid.server.security.auth.TaskPrincipal;
 
-public class SecurityManager implements ConfigurationChangeListener
+public class SecurityManager
 {
     private static final Subject SYSTEM = new Subject(true,
                                                      Collections.singleton(new SystemPrincipal()),
                                                      Collections.emptySet(),
                                                      Collections.emptySet());
 
-    private final ConcurrentMap<String, AccessControl> _plugins = new ConcurrentHashMap<String, AccessControl>();
     private final boolean _managementMode;
     private final Broker<?> _broker;
 
-    private final ConcurrentMap<PublishAccessCheckCacheEntry, PublishAccessCheck> _publishAccessCheckCache = new ConcurrentHashMap<SecurityManager.PublishAccessCheckCacheEntry, SecurityManager.PublishAccessCheck>();
+    private final ConcurrentMap<PublishAccessCheckCacheEntry, PublishAccessCheck> _publishAccessCheckCache = new ConcurrentHashMap<PublishAccessCheckCacheEntry, SecurityManager.PublishAccessCheck>();
 
     public SecurityManager(Broker<?> broker, boolean managementMode)
     {
@@ -135,16 +133,6 @@ public class SecurityManager implements ConfigurationChangeListener
         return user;
     }
 
-    public void addPlugin(final AccessControl accessControl)
-    {
-
-        synchronized (_plugins)
-        {
-            String pluginTypeName = getPluginTypeName(accessControl);
-
-            _plugins.put(pluginTypeName, accessControl);
-        }
-    }
 
     private static final class SystemPrincipal implements Principal
     {
@@ -167,24 +155,31 @@ public class SecurityManager implements ConfigurationChangeListener
     private boolean checkAllPlugins(AccessCheck checker)
     {
         // If we are running as SYSTEM then no ACL checking
-        if(isSystemProcess())
+        if(isSystemProcess() || _managementMode)
         {
             return true;
         }
 
-        for (AccessControl plugin : _plugins.values())
+
+        Collection<AccessControlProvider<?>> accessControlProviders = _broker.getAccessControlProviders();
+        if(accessControlProviders != null && !accessControlProviders.isEmpty())
         {
-            Result remaining = checker.allowed(plugin);
-            if (remaining == Result.DEFER)
-            {
-                remaining = plugin.getDefault();
-            }
-            if (remaining == Result.DENIED)
+            AccessControlProvider<?> accessControlProvider = accessControlProviders.iterator().next();
+            if (accessControlProvider != null
+                && accessControlProvider.getState() == State.ACTIVE
+                && accessControlProvider.getAccessControl() != null)
             {
-                return false;
+                Result remaining = checker.allowed(accessControlProvider.getAccessControl());
+                if (remaining == Result.DEFER)
+                {
+                    remaining = accessControlProvider.getAccessControl().getDefault();
+                }
+                if (remaining == Result.DENIED)
+                {
+                    return false;
+                }
             }
         }
-
         // getting here means either allowed or abstained from all plugins
         return true;
     }
@@ -486,92 +481,6 @@ public class SecurityManager implements ConfigurationChangeListener
         }
     }
 
-    @Override
-    public void stateChanged(ConfiguredObject object, State oldState, State newState)
-    {
-        if(_managementMode)
-        {
-            //AccessControl is disabled in ManagementMode
-            return;
-        }
-
-        if(object instanceof AccessControlProvider)
-        {
-            if(newState == State.ACTIVE)
-            {
-                synchronized (_plugins)
-                {
-                    AccessControl accessControl = ((AccessControlProvider)object).getAccessControl();
-                    String pluginTypeName = getPluginTypeName(accessControl);
-
-                    _plugins.put(pluginTypeName, accessControl);
-                }
-            }
-            else if(newState == State.DELETED)
-            {
-                synchronized (_plugins)
-                {
-                    AccessControl control = ((AccessControlProvider)object).getAccessControl();
-                    String pluginTypeName = getPluginTypeName(control);
-
-                    // Remove the type->control mapping for this type key only if the
-                    // given control is actually referred to.
-                    if(_plugins.containsValue(control))
-                    {
-                        // If we are removing this control, check if another of the same
-                        // type already exists on the broker and use it in instead.
-                        AccessControl other = null;
-                        Collection<AccessControlProvider<?>> providers = _broker.getAccessControlProviders();
-                        for(AccessControlProvider p : providers)
-                        {
-                            if(p == object || p.getState() != State.ACTIVE)
-                            {
-                                //we don't count ourself as another
-                                continue;
-                            }
-
-                            AccessControl ac = p.getAccessControl();
-                            if(pluginTypeName.equals(getPluginTypeName(ac)))
-                            {
-                                other = ac;
-                                break;
-                            }
-                        }
-
-                        if(other != null)
-                        {
-                            //Another control of this type was found, use it instead
-                            _plugins.replace(pluginTypeName, control, other);
-                        }
-                        else
-                        {
-                            //No other was found, remove the type entirely
-                            _plugins.remove(pluginTypeName);
-                        }
-                    }
-                }
-            }
-        }
-    }
-
-    @Override
-    public void childAdded(ConfiguredObject object, ConfiguredObject child)
-    {
-        // no op
-    }
-
-    @Override
-    public void childRemoved(ConfiguredObject object, ConfiguredObject child)
-    {
-        // no op
-    }
-
-    @Override
-    public void attributeSet(ConfiguredObject object, String attributeName, Object oldAttributeValue, Object newAttributeValue)
-    {
-        // no op
-    }
-
     public boolean authoriseConfiguringBroker(String configuredObjectName, Class<? extends ConfiguredObject> configuredObjectType, Operation configuredObjectOperation)
     {
         String description = String.format("%s %s '%s'",
diff --git a/qpid/java/broker-core/src/test/java/org/apache/qpid/server/security/SecurityManagerTest.java b/qpid/java/broker-core/src/test/java/org/apache/qpid/server/security/SecurityManagerTest.java
index 15d4cba278..54bd69120b 100644
--- a/qpid/java/broker-core/src/test/java/org/apache/qpid/server/security/SecurityManagerTest.java
+++ b/qpid/java/broker-core/src/test/java/org/apache/qpid/server/security/SecurityManagerTest.java
@@ -30,6 +30,7 @@ import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 
 import java.security.AccessControlException;
+import java.util.Collections;
 
 import org.apache.qpid.server.binding.BindingImpl;
 import org.apache.qpid.server.consumer.ConsumerImpl;
@@ -68,11 +69,13 @@ public class SecurityManagerTest extends QpidTestCase
 
         AccessControlProvider<?> aclProvider = mock(AccessControlProvider.class);
         when(aclProvider.getAccessControl()).thenReturn(_accessControl);
+        when(aclProvider.getState()).thenReturn(State.ACTIVE);
 
         when(_virtualHost.getName()).thenReturn(TEST_VIRTUAL_HOST);
 
-        _securityManager = new SecurityManager(mock(Broker.class), false);
-        _securityManager.stateChanged(aclProvider, State.UNINITIALIZED, State.ACTIVE);
+        Broker broker = mock(Broker.class);
+        when(broker.getAccessControlProviders()).thenReturn(Collections.singleton(aclProvider));
+        _securityManager = new SecurityManager(broker, false);
     }
 
     public void testAuthoriseCreateBinding()
diff --git a/qpid/java/systests/src/test/java/org/apache/qpid/systest/rest/AccessControlProviderRestTest.java b/qpid/java/systests/src/test/java/org/apache/qpid/systest/rest/AccessControlProviderRestTest.java
index 4140c9c12c..0dda8e077b 100644
--- a/qpid/java/systests/src/test/java/org/apache/qpid/systest/rest/AccessControlProviderRestTest.java
+++ b/qpid/java/systests/src/test/java/org/apache/qpid/systest/rest/AccessControlProviderRestTest.java
@@ -125,34 +125,28 @@ public class AccessControlProviderRestTest extends QpidRestTestCase
 
     public void testReplaceAccessControlProvider() throws Exception
     {
-        String accessControlProviderName1 = getTestName() + "1";
 
-        //verify that the access control provider doesn't exist, and
-        //in doing so implicitly verify that the 'denied' user can
-        //actually currently connect because no ACL is in effect yet
-        getRestTestHelper().setUsernameAndPassword(DENIED_USER, DENIED_USER);
-        assertAccessControlProviderExistence(accessControlProviderName1, false);
 
         //create the access control provider using the 'allowed' user
         getRestTestHelper().setUsernameAndPassword(ALLOWED_USER, ALLOWED_USER);
-        int responseCode = createAccessControlProvider(accessControlProviderName1, _aclFileContent1);
+        int responseCode = createAccessControlProvider(getTestName(), _aclFileContent1);
         assertEquals("Access control provider creation should be allowed", 201, responseCode);
 
         //verify it exists with the 'allowed' user
-        assertAccessControlProviderExistence(accessControlProviderName1, true);
+        assertAccessControlProviderExistence(getTestName(), true);
 
         //verify the 'denied' and 'other' user can no longer access the management
         //interface due to the just-created ACL file now preventing them
         getRestTestHelper().setUsernameAndPassword(DENIED_USER, DENIED_USER);
-        assertCanAccessManagementInterface(accessControlProviderName1, false);
+        assertCanAccessManagementInterface(getTestName(), false);
         getRestTestHelper().setUsernameAndPassword(OTHER_USER, OTHER_USER);
-        assertCanAccessManagementInterface(accessControlProviderName1, false);
+        assertCanAccessManagementInterface(getTestName(), false);
 
         //create the replacement access control provider using the 'allowed' user.
         String accessControlProviderName2 = getTestName() + "2";
         getRestTestHelper().setUsernameAndPassword(ALLOWED_USER, ALLOWED_USER);
-        responseCode = createAccessControlProvider(accessControlProviderName2, _aclFileContent2);
-        assertEquals("Access control provider creation should be allowed", 201, responseCode);
+        responseCode = createAccessControlProvider(getTestName(), _aclFileContent2);
+        assertEquals("Access control provider creation should be allowed", 200, responseCode);
 
         //Verify that it took effect immediately, replacing the first access control provider
 
@@ -162,11 +156,6 @@ public class AccessControlProviderRestTest extends QpidRestTestCase
         getRestTestHelper().setUsernameAndPassword(OTHER_USER, OTHER_USER);
         assertCanAccessManagementInterface(accessControlProviderName2, true);
 
-        //remove the original access control provider using the 'allowed' user
-        getRestTestHelper().setUsernameAndPassword(ALLOWED_USER, ALLOWED_USER);
-        responseCode = getRestTestHelper().submitRequest("accesscontrolprovider/" + accessControlProviderName1, "DELETE");
-        assertEquals("Access control provider deletion should be allowed", 200, responseCode);
-        assertAccessControlProviderExistence(accessControlProviderName1, false);
 
         //verify the 'denied' user still can't access the management interface, the 'other' user still can, thus
         //confirming that the second access control provider is still in effect
@@ -177,61 +166,6 @@ public class AccessControlProviderRestTest extends QpidRestTestCase
     }
 
 
-    public void testAddAndRemoveSecondAccessControlProviderReinstatesOriginal() throws Exception
-    {
-        String accessControlProviderName1 = getTestName() + "1";
-
-        //verify that the access control provider doesn't exist, and
-        //in doing so implicitly verify that the 'denied' user can
-        //actually currently connect because no ACL is in effect yet
-        getRestTestHelper().setUsernameAndPassword(DENIED_USER, DENIED_USER);
-        assertAccessControlProviderExistence(accessControlProviderName1, false);
-
-        //create the access control provider using the 'allowed' user
-        getRestTestHelper().setUsernameAndPassword(ALLOWED_USER, ALLOWED_USER);
-        int responseCode = createAccessControlProvider(accessControlProviderName1, _aclFileContent1);
-        assertEquals("Access control provider creation should be allowed", 201, responseCode);
-
-        //verify it exists with the 'allowed' user
-        assertAccessControlProviderExistence(accessControlProviderName1, true);
-
-        //verify the 'denied' and 'other' user can no longer access the management
-        //interface due to the just-created ACL file now preventing them
-        getRestTestHelper().setUsernameAndPassword(DENIED_USER, DENIED_USER);
-        assertCanAccessManagementInterface(accessControlProviderName1, false);
-        getRestTestHelper().setUsernameAndPassword(OTHER_USER, OTHER_USER);
-        assertCanAccessManagementInterface(accessControlProviderName1, false);
-
-        //create the replacement access control provider using the 'allowed' user.
-        String accessControlProviderName2 = getTestName() + "2";
-        getRestTestHelper().setUsernameAndPassword(ALLOWED_USER, ALLOWED_USER);
-        responseCode = createAccessControlProvider(accessControlProviderName2, _aclFileContent2);
-        assertEquals("Access control provider creation should be allowed", 201, responseCode);
-
-        //Verify that it took effect immediately, replacing the first access control provider
-
-        //verify the 'denied' user still can't access the management interface, but the 'other' user now CAN.
-        getRestTestHelper().setUsernameAndPassword(DENIED_USER, DENIED_USER);
-        assertCanAccessManagementInterface(accessControlProviderName2, false);
-        getRestTestHelper().setUsernameAndPassword(OTHER_USER, OTHER_USER);
-        assertCanAccessManagementInterface(accessControlProviderName2, true);
-
-        //remove the second access control provider using the 'allowed' user
-        getRestTestHelper().setUsernameAndPassword(ALLOWED_USER, ALLOWED_USER);
-        responseCode = getRestTestHelper().submitRequest("accesscontrolprovider/" + accessControlProviderName2, "DELETE");
-        assertEquals("Access control provider deletion should be allowed", 200, responseCode);
-        assertAccessControlProviderExistence(accessControlProviderName2, false);
-
-        //verify the 'denied' user still can't access the management interface, the
-        //'other' now CANT again, the 'allowed' still can, thus confirming that the
-        //first access control provider is now in effect once again
-        getRestTestHelper().setUsernameAndPassword(DENIED_USER, DENIED_USER);
-        assertCanAccessManagementInterface(accessControlProviderName2, false);
-        getRestTestHelper().setUsernameAndPassword(OTHER_USER, OTHER_USER);
-        assertCanAccessManagementInterface(accessControlProviderName2, false);
-        getRestTestHelper().setUsernameAndPassword(ALLOWED_USER, ALLOWED_USER);
-        assertCanAccessManagementInterface(accessControlProviderName2, true);
-    }
 
     public void testRemovalOfAccessControlProviderInErrorStateUsingManagementMode() throws Exception
     {
diff --git a/qpid/java/systests/src/test/java/org/apache/qpid/systest/rest/acl/BrokerACLTest.java b/qpid/java/systests/src/test/java/org/apache/qpid/systest/rest/acl/BrokerACLTest.java
index e40add449e..86ebf11575 100644
--- a/qpid/java/systests/src/test/java/org/apache/qpid/systest/rest/acl/BrokerACLTest.java
+++ b/qpid/java/systests/src/test/java/org/apache/qpid/systest/rest/acl/BrokerACLTest.java
@@ -714,19 +714,6 @@ public class BrokerACLTest extends QpidRestTestCase
 
     /* === AccessControlProvider === */
 
-    public void testCreateAccessControlProviderAllowed() throws Exception
-    {
-        getRestTestHelper().setUsernameAndPassword(ALLOWED_USER, ALLOWED_USER);
-
-        String accessControlProviderName = getTestName();
-
-        assertAccessControlProviderExistence(accessControlProviderName, false);
-
-        int responseCode = createAccessControlProvider(accessControlProviderName);
-        assertEquals("Access control provider creation should be allowed", 201, responseCode);
-
-        assertAccessControlProviderExistence(accessControlProviderName, true);
-    }
 
     public void testCreateAccessControlProviderDenied() throws Exception
     {
@@ -746,18 +733,13 @@ public class BrokerACLTest extends QpidRestTestCase
     {
         getRestTestHelper().setUsernameAndPassword(ALLOWED_USER, ALLOWED_USER);
 
-        String accessControlProviderName = getTestName();
-
-        assertAccessControlProviderExistence(accessControlProviderName, false);
-
-        int responseCode = createAccessControlProvider(accessControlProviderName);
-        assertEquals("Access control provider creation should be allowed", 201, responseCode);
+        String accessControlProviderName = TestBrokerConfiguration.ENTRY_NAME_ACL_FILE;
 
         assertAccessControlProviderExistence(accessControlProviderName, true);
 
         getRestTestHelper().setUsernameAndPassword(DENIED_USER, DENIED_USER);
 
-        responseCode = getRestTestHelper().submitRequest("accesscontrolprovider/" + accessControlProviderName, "DELETE");
+        int responseCode = getRestTestHelper().submitRequest("accesscontrolprovider/" + accessControlProviderName, "DELETE");
         assertEquals("Access control provider deletion should be denied", 403, responseCode);
 
         assertAccessControlProviderExistence(accessControlProviderName, true);
@@ -767,16 +749,12 @@ public class BrokerACLTest extends QpidRestTestCase
     {
         getRestTestHelper().setUsernameAndPassword(ALLOWED_USER, ALLOWED_USER);
 
-        String accessControlProviderName = getTestName();
-
-        assertAccessControlProviderExistence(accessControlProviderName, false);
+        String accessControlProviderName = TestBrokerConfiguration.ENTRY_NAME_ACL_FILE;
 
-        int responseCode = createAccessControlProvider(accessControlProviderName);
-        assertEquals("Access control provider creation should be allowed", 201, responseCode);
 
         assertAccessControlProviderExistence(accessControlProviderName, true);
 
-        responseCode = getRestTestHelper().submitRequest("accesscontrolprovider/" + accessControlProviderName, "DELETE");
+        int responseCode = getRestTestHelper().submitRequest("accesscontrolprovider/" + accessControlProviderName, "DELETE");
         assertEquals("Access control provider deletion should be allowed", 200, responseCode);
 
         assertAccessControlProviderExistence(accessControlProviderName, false);
@@ -786,12 +764,7 @@ public class BrokerACLTest extends QpidRestTestCase
     {
         getRestTestHelper().setUsernameAndPassword(ALLOWED_USER, ALLOWED_USER);
 
-        String accessControlProviderName = getTestName();
-
-        assertAccessControlProviderExistence(accessControlProviderName, false);
-
-        int responseCode = createAccessControlProvider(accessControlProviderName);
-        assertEquals("Access control provider creation should be allowed", 201, responseCode);
+        String accessControlProviderName = TestBrokerConfiguration.ENTRY_NAME_ACL_FILE;
 
         assertAccessControlProviderExistence(accessControlProviderName, true);
 
@@ -800,7 +773,7 @@ public class BrokerACLTest extends QpidRestTestCase
         Map<String, Object> attributes = new HashMap<String, Object>();
         attributes.put(AccessControlProvider.NAME, accessControlProviderName);
         attributes.put(FileBasedGroupProvider.PATH, aclFile.getAbsolutePath());
-        responseCode = getRestTestHelper().submitRequest("accesscontrolprovider/" + accessControlProviderName, "PUT", attributes);
+        int responseCode = getRestTestHelper().submitRequest("accesscontrolprovider/" + accessControlProviderName, "PUT", attributes);
         assertEquals("Setting of access control provider attributes should be allowed", 200, responseCode);
     }
 
@@ -808,12 +781,7 @@ public class BrokerACLTest extends QpidRestTestCase
     {
         getRestTestHelper().setUsernameAndPassword(ALLOWED_USER, ALLOWED_USER);
 
-        String accessControlProviderName = getTestName();
-
-        assertAccessControlProviderExistence(accessControlProviderName, false);
-
-        int responseCode = createAccessControlProvider(accessControlProviderName);
-        assertEquals("Access control provider creation should be allowed", 201, responseCode);
+        String accessControlProviderName = TestBrokerConfiguration.ENTRY_NAME_ACL_FILE;
 
         assertAccessControlProviderExistence(accessControlProviderName, true);
 
@@ -823,7 +791,7 @@ public class BrokerACLTest extends QpidRestTestCase
         attributes.put(GroupProvider.NAME, accessControlProviderName);
         attributes.put(GroupProvider.TYPE, FileBasedGroupProviderImpl.GROUP_FILE_PROVIDER_TYPE);
         attributes.put(FileBasedGroupProvider.PATH, "/path/to/file");
-        responseCode = getRestTestHelper().submitRequest("accesscontrolprovider/" + accessControlProviderName, "PUT", attributes);
+        int responseCode = getRestTestHelper().submitRequest("accesscontrolprovider/" + accessControlProviderName, "PUT", attributes);
         assertEquals("Setting of access control provider attributes should be denied", 403, responseCode);
     }