QPID-7934: [Java Broker] [Model [ACL] Ensure that RuleBasedAccessControlProvider reports changes in the rule set to the control point (Broker/VirtualHost)
* Added new ACO#postSetAttributes that is fired once all attributes specified by a setAttributes call have completed their change. * Refactored AbstractCommonRuleBasedAccessControlProvider implement the new method to update the controller's state * Corrected AbstractVirtualHost to install access control listeners consistently on the recovery and restart paths. Project: http://git-wip-us.apache.org/repos/asf/qpid-broker-j/repo Commit: http://git-wip-us.apache.org/repos/asf/qpid-broker-j/commit/d4d408f2 Tree: http://git-wip-us.apache.org/repos/asf/qpid-broker-j/tree/d4d408f2 Diff: http://git-wip-us.apache.org/repos/asf/qpid-broker-j/diff/d4d408f2 Branch: refs/heads/master Commit: d4d408f27fed8f5cb9c3180b3d0167be069ae690 Parents: 54f8074 Author: Keith Wall <[email protected]> Authored: Fri Sep 29 11:09:43 2017 +0100 Committer: Keith Wall <[email protected]> Committed: Fri Sep 29 13:30:43 2017 +0100 ---------------------------------------------------------------------- .../server/model/AbstractConfiguredObject.java | 17 ++++- .../server/virtualhost/AbstractVirtualHost.java | 72 +++++++++----------- .../singleton/AbstractConfiguredObjectTest.java | 27 ++++++++ .../testmodels/singleton/TestSingleton.java | 2 +- .../testmodels/singleton/TestSingletonImpl.java | 20 ++++++ ...actCommonRuleBasedAccessControlProvider.java | 9 +-- ...irtualHostAccessControlProviderRestTest.java | 59 ++++++++++++---- 7 files changed, 148 insertions(+), 58 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/qpid-broker-j/blob/d4d408f2/broker-core/src/main/java/org/apache/qpid/server/model/AbstractConfiguredObject.java ---------------------------------------------------------------------- diff --git a/broker-core/src/main/java/org/apache/qpid/server/model/AbstractConfiguredObject.java b/broker-core/src/main/java/org/apache/qpid/server/model/AbstractConfiguredObject.java index fb0a7fb..4b15161 100644 --- a/broker-core/src/main/java/org/apache/qpid/server/model/AbstractConfiguredObject.java +++ b/broker-core/src/main/java/org/apache/qpid/server/model/AbstractConfiguredObject.java @@ -1930,7 +1930,7 @@ public abstract class AbstractConfiguredObject<X extends ConfiguredObject<X>> im } } - protected boolean changeAttribute(final String name, final Object desired) + private boolean changeAttribute(final String name, final Object desired) { synchronized (_attributes) { @@ -2390,6 +2390,10 @@ public abstract class AbstractConfiguredObject<X extends ConfiguredObject<X>> im doSync(setAttributesAsync(attributes)); } + protected void postSetAttributes(final Set<String> actualUpdatedAttributes) + { + } + protected final ChainedListenableFuture<Void> doAfter(ListenableFuture<?> first, final Runnable second) { return doAfter(getTaskExecutor(), first, second); @@ -2799,6 +2803,7 @@ public abstract class AbstractConfiguredObject<X extends ConfiguredObject<X>> im protected void changeAttributes(final Map<String, Object> attributes) { Collection<String> names = getAttributeNames(); + Set<String> updatedAttributes = new HashSet<>(attributes.size()); try { bulkChangeStart(); @@ -2812,13 +2817,21 @@ public abstract class AbstractConfiguredObject<X extends ConfiguredObject<X>> im if (changeAttribute(attributeName, desired)) { attributeSet(attributeName, expected, desired); + updatedAttributes.add(attributeName); } } } } finally { - bulkChangeEnd(); + try + { + postSetAttributes(updatedAttributes); + } + finally + { + bulkChangeEnd(); + } } } http://git-wip-us.apache.org/repos/asf/qpid-broker-j/blob/d4d408f2/broker-core/src/main/java/org/apache/qpid/server/virtualhost/AbstractVirtualHost.java ---------------------------------------------------------------------- diff --git a/broker-core/src/main/java/org/apache/qpid/server/virtualhost/AbstractVirtualHost.java b/broker-core/src/main/java/org/apache/qpid/server/virtualhost/AbstractVirtualHost.java index f5580b3..230638c 100644 --- a/broker-core/src/main/java/org/apache/qpid/server/virtualhost/AbstractVirtualHost.java +++ b/broker-core/src/main/java/org/apache/qpid/server/virtualhost/AbstractVirtualHost.java @@ -142,7 +142,6 @@ import org.apache.qpid.server.txn.AutoCommitTransaction; import org.apache.qpid.server.txn.DtxRegistry; import org.apache.qpid.server.txn.LocalTransaction; import org.apache.qpid.server.txn.ServerTransaction; -import org.apache.qpid.server.util.Action; import org.apache.qpid.server.util.HousekeepingExecutor; import org.apache.qpid.server.util.Strings; @@ -439,6 +438,11 @@ public abstract class AbstractVirtualHost<X extends AbstractVirtualHost<X>> exte { super.postResolveChildren(); addChangeListener(_accessControlProviderListener); + Collection<VirtualHostAccessControlProvider> accessControlProviders = getChildren(VirtualHostAccessControlProvider.class); + if (!accessControlProviders.isEmpty()) + { + accessControlProviders.forEach(child -> child.addChangeListener(_accessControlProviderListener)); + } } private void validateNodeAutoCreationPolicy(final NodeAutoCreationPolicy policy) @@ -2624,53 +2628,43 @@ public abstract class AbstractVirtualHost<X extends AbstractVirtualHost<X>> exte // Transitioning to STOPPED will have closed all our children. Now we are transition // back to ACTIVE, we need to recover and re-open them. - getDurableConfigurationStore().reload(new ConfiguredObjectRecordHandler() - { - - @Override - public void handle(final ConfiguredObjectRecord record) - { - records.add(record); - } - - }); + getDurableConfigurationStore().reload(records::add); new GenericRecoverer(this).recover(records, false); + final Collection<VirtualHostAccessControlProvider> accessControlProviders = getChildren(VirtualHostAccessControlProvider.class); + if (!accessControlProviders.isEmpty()) + { + accessControlProviders.forEach(child -> child.addChangeListener(_accessControlProviderListener)); + } + final List<ListenableFuture<Void>> childOpenFutures = new ArrayList<>(); - Subject.doAs(getSubjectWithAddedSystemRights(), new PrivilegedAction<Object>() + Subject.doAs(getSubjectWithAddedSystemRights(), (PrivilegedAction<Object>) () -> { - @Override - public Object run() - { - applyToChildren(new Action<ConfiguredObject<?>>() - { - @Override - public void performAction(final ConfiguredObject<?> child) - { - final ListenableFuture<Void> childOpenFuture = child.openAsync(); - childOpenFutures.add(childOpenFuture); - - addFutureCallback(childOpenFuture, new FutureCallback<Void>() - { - @Override - public void onSuccess(final Void result) + applyToChildren(child -> { - } + final ListenableFuture<Void> childOpenFuture = child.openAsync(); + childOpenFutures.add(childOpenFuture); - @Override - public void onFailure(final Throwable t) - { - _logger.error("Exception occurred while opening {} : {}", - child.getClass().getSimpleName(), child.getName(), t); - } + addFutureCallback(childOpenFuture, new FutureCallback<Void>() + { + @Override + public void onSuccess(final Void result) + { + updateAccessControl(); + } - }, getTaskExecutor()); - } - }); - return null; - } + @Override + public void onFailure(final Throwable t) + { + _logger.error("Exception occurred while opening {} : {}", + child.getClass().getSimpleName(), child.getName(), t); + } + + }, getTaskExecutor()); + }); + return null; }); ListenableFuture<List<Void>> combinedFuture = Futures.allAsList(childOpenFutures); http://git-wip-us.apache.org/repos/asf/qpid-broker-j/blob/d4d408f2/broker-core/src/test/java/org/apache/qpid/server/model/testmodels/singleton/AbstractConfiguredObjectTest.java ---------------------------------------------------------------------- diff --git a/broker-core/src/test/java/org/apache/qpid/server/model/testmodels/singleton/AbstractConfiguredObjectTest.java b/broker-core/src/test/java/org/apache/qpid/server/model/testmodels/singleton/AbstractConfiguredObjectTest.java index 2de5b50..e29ac7f 100644 --- a/broker-core/src/test/java/org/apache/qpid/server/model/testmodels/singleton/AbstractConfiguredObjectTest.java +++ b/broker-core/src/test/java/org/apache/qpid/server/model/testmodels/singleton/AbstractConfiguredObjectTest.java @@ -28,11 +28,14 @@ import java.util.Date; import java.util.HashMap; import java.util.LinkedHashMap; import java.util.Map; +import java.util.Set; import java.util.UUID; import java.util.concurrent.atomic.AtomicInteger; import javax.security.auth.Subject; +import com.google.common.collect.Sets; + import org.apache.qpid.server.configuration.IllegalConfigurationException; import org.apache.qpid.server.model.AbstractConfigurationChangeListener; import org.apache.qpid.server.model.AbstractConfiguredObject; @@ -1084,6 +1087,30 @@ public class AbstractConfiguredObjectTest extends QpidTestCase assertEquals("Unexpected recovered object updated time", object.getLastUpdatedTime(), recovered.getLastUpdatedTime()); } + public void testPostSetAttributesReportsChanges() + { + final String objectName = "myName"; + + Map<String, Object> attributes = new HashMap<>(); + attributes.put(TestSingleton.NAME, objectName); + + TestSingleton object = _model.getObjectFactory().create(TestSingleton.class, + attributes, null); + + assertEquals(objectName, object.getName()); + + + object.setAttributes(Collections.emptyMap()); + assertTrue("Unexpected member of update set for empty update", object.takeLastReportedSetAttributes().isEmpty()); + + Map<String, Object> update = new HashMap<>(); + update.put(TestSingleton.NAME, objectName); + update.put(TestSingleton.DESCRIPTION, "an update"); + + object.setAttributes(update); + assertEquals("Unexpected member of update set", Sets.newHashSet(TestSingleton.DESCRIPTION), + object.takeLastReportedSetAttributes()); + } private Subject createTestAuthenticatedSubject(final String username) { return new Subject(true, http://git-wip-us.apache.org/repos/asf/qpid-broker-j/blob/d4d408f2/broker-core/src/test/java/org/apache/qpid/server/model/testmodels/singleton/TestSingleton.java ---------------------------------------------------------------------- diff --git a/broker-core/src/test/java/org/apache/qpid/server/model/testmodels/singleton/TestSingleton.java b/broker-core/src/test/java/org/apache/qpid/server/model/testmodels/singleton/TestSingleton.java index 08d6c1b..34a9ab7 100644 --- a/broker-core/src/test/java/org/apache/qpid/server/model/testmodels/singleton/TestSingleton.java +++ b/broker-core/src/test/java/org/apache/qpid/server/model/testmodels/singleton/TestSingleton.java @@ -126,5 +126,5 @@ public interface TestSingleton<X extends TestSingleton<X>> extends ConfiguredObj <T> T doAsSystem(PrivilegedAction<T> action); - + Set<String> takeLastReportedSetAttributes(); } http://git-wip-us.apache.org/repos/asf/qpid-broker-j/blob/d4d408f2/broker-core/src/test/java/org/apache/qpid/server/model/testmodels/singleton/TestSingletonImpl.java ---------------------------------------------------------------------- diff --git a/broker-core/src/test/java/org/apache/qpid/server/model/testmodels/singleton/TestSingletonImpl.java b/broker-core/src/test/java/org/apache/qpid/server/model/testmodels/singleton/TestSingletonImpl.java index d9f444b..4a9821a 100644 --- a/broker-core/src/test/java/org/apache/qpid/server/model/testmodels/singleton/TestSingletonImpl.java +++ b/broker-core/src/test/java/org/apache/qpid/server/model/testmodels/singleton/TestSingletonImpl.java @@ -20,14 +20,19 @@ package org.apache.qpid.server.model.testmodels.singleton; import java.security.Principal; import java.security.PrivilegedAction; +import java.util.ArrayDeque; import java.util.Collections; import java.util.Date; +import java.util.Deque; +import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Set; import javax.security.auth.Subject; +import com.google.common.collect.Sets; + import org.apache.qpid.server.configuration.updater.CurrentThreadTaskExecutor; import org.apache.qpid.server.configuration.updater.TaskExecutor; import org.apache.qpid.server.model.AbstractConfiguredObject; @@ -110,6 +115,8 @@ public class TestSingletonImpl extends AbstractConfiguredObject<TestSingletonImp @ManagedAttributeField private String _attrWithDefaultFromContextMaterializeInit; + private Deque<HashSet<String>> _lastReportedSetAttributes = new ArrayDeque<>(); + @ManagedObjectFactoryConstructor public TestSingletonImpl(final Map<String, Object> attributes) { @@ -261,4 +268,17 @@ public class TestSingletonImpl extends AbstractConfiguredObject<TestSingletonImp { return Subject.doAs(SYSTEM_SUBJECT, action); } + + @Override + protected void postSetAttributes(final Set<String> actualUpdatedAttributes) + { + super.postSetAttributes(actualUpdatedAttributes); + _lastReportedSetAttributes.add(Sets.newHashSet(actualUpdatedAttributes)); + } + + @Override + public Set<String> takeLastReportedSetAttributes() + { + return _lastReportedSetAttributes.removeFirst(); + } } http://git-wip-us.apache.org/repos/asf/qpid-broker-j/blob/d4d408f2/broker-plugins/access-control/src/main/java/org/apache/qpid/server/security/access/plugins/AbstractCommonRuleBasedAccessControlProvider.java ---------------------------------------------------------------------- diff --git a/broker-plugins/access-control/src/main/java/org/apache/qpid/server/security/access/plugins/AbstractCommonRuleBasedAccessControlProvider.java b/broker-plugins/access-control/src/main/java/org/apache/qpid/server/security/access/plugins/AbstractCommonRuleBasedAccessControlProvider.java index 3b92c65..315f297 100644 --- a/broker-plugins/access-control/src/main/java/org/apache/qpid/server/security/access/plugins/AbstractCommonRuleBasedAccessControlProvider.java +++ b/broker-plugins/access-control/src/main/java/org/apache/qpid/server/security/access/plugins/AbstractCommonRuleBasedAccessControlProvider.java @@ -30,6 +30,7 @@ import java.util.ArrayList; import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.Set; import org.apache.qpid.server.logging.EventLoggerProvider; import org.apache.qpid.server.model.CommonAccessControlProvider; @@ -65,11 +66,11 @@ abstract class AbstractCommonRuleBasedAccessControlProvider<X extends AbstractCo } @Override - protected void changeAttributes(final Map<String, Object> attributes) + protected void postSetAttributes(final Set<String> actualUpdatedAttributes) { - super.changeAttributes(attributes); - if(attributes.containsKey(RuleBasedVirtualHostAccessControlProvider.DEFAULT_RESULT) || attributes.containsKey( - RuleBasedVirtualHostAccessControlProvider.RULES)) + super.postSetAttributes(actualUpdatedAttributes); + if (actualUpdatedAttributes.contains(RuleBasedVirtualHostAccessControlProvider.DEFAULT_RESULT) + || actualUpdatedAttributes.contains(RuleBasedVirtualHostAccessControlProvider.RULES)) { recreateAccessController(); } http://git-wip-us.apache.org/repos/asf/qpid-broker-j/blob/d4d408f2/systests/src/test/java/org/apache/qpid/systest/rest/acl/VirtualHostAccessControlProviderRestTest.java ---------------------------------------------------------------------- diff --git a/systests/src/test/java/org/apache/qpid/systest/rest/acl/VirtualHostAccessControlProviderRestTest.java b/systests/src/test/java/org/apache/qpid/systest/rest/acl/VirtualHostAccessControlProviderRestTest.java index 3b669b1..c37b1f2 100644 --- a/systests/src/test/java/org/apache/qpid/systest/rest/acl/VirtualHostAccessControlProviderRestTest.java +++ b/systests/src/test/java/org/apache/qpid/systest/rest/acl/VirtualHostAccessControlProviderRestTest.java @@ -28,19 +28,21 @@ import static org.apache.qpid.server.security.access.config.ObjectType.*; import java.util.Collections; import java.util.HashMap; +import java.util.List; import java.util.Map; +import java.util.stream.Collectors; import javax.servlet.http.HttpServletResponse; import org.apache.qpid.server.model.ConfiguredObject; import org.apache.qpid.server.model.Queue; import org.apache.qpid.server.model.VirtualHostAccessControlProvider; -import org.apache.qpid.server.security.access.plugins.RuleOutcome; import org.apache.qpid.server.security.access.config.LegacyOperation; import org.apache.qpid.server.security.access.config.ObjectProperties; import org.apache.qpid.server.security.access.config.ObjectType; import org.apache.qpid.server.security.access.plugins.AclRule; import org.apache.qpid.server.security.access.plugins.RuleBasedVirtualHostAccessControlProvider; +import org.apache.qpid.server.security.access.plugins.RuleOutcome; import org.apache.qpid.systest.rest.QpidRestTestCase; import org.apache.qpid.test.utils.TestBrokerConfiguration; @@ -58,6 +60,7 @@ public class VirtualHostAccessControlProviderRestTest extends QpidRestTestCase private String _queueUrl; private String _queueName; + private String _virtualHostRuleProviderUrl; @Override public void setUp() throws Exception @@ -65,6 +68,7 @@ public class VirtualHostAccessControlProviderRestTest extends QpidRestTestCase super.setUp(); _queueName = getTestName(); _queueUrl = "queue/test/test/" + _queueName; + _virtualHostRuleProviderUrl = VirtualHostAccessControlProvider.class.getSimpleName().toLowerCase() + "/test/test/rules"; getRestTestHelper().setUsernameAndPassword(ADMIN, ADMIN); final Map<String, Object> attributes = new HashMap<>(); @@ -85,7 +89,7 @@ public class VirtualHostAccessControlProviderRestTest extends QpidRestTestCase }; attributes.put(RuleBasedVirtualHostAccessControlProvider.RULES, rules); - getRestTestHelper().submitRequest(VirtualHostAccessControlProvider.class.getSimpleName().toLowerCase() + "/test/test/rules", "PUT", attributes); + getRestTestHelper().submitRequest(_virtualHostRuleProviderUrl, "PUT", attributes); } @@ -150,36 +154,67 @@ public class VirtualHostAccessControlProviderRestTest extends QpidRestTestCase assertCreateQueueDenied(USER6); } - public void assertCreateAndDeleteQueueSucceeds(final String username) throws Exception + @SuppressWarnings("unchecked") + public void testUpdateVirtualHostRule() throws Exception + { + // Denied by virtualhost rule + assertCreateQueueDenied(USER1); + + Map<String, Object> providerDetails = getRestTestHelper().getJsonAsMap(_virtualHostRuleProviderUrl); + + List<Map<String, Object>> currentRules = ((List<Map<String, Object>>) providerDetails.get(RuleBasedVirtualHostAccessControlProvider.RULES)); + + List<Map<String, Object>> filteredRulesWithoutUser1 = currentRules.stream() + .filter(rule -> + !rule.get("identity").equals(USER1)) + .collect(Collectors.toList()); + + Map<String, Object> update = Collections.singletonMap(RuleBasedVirtualHostAccessControlProvider.RULES, filteredRulesWithoutUser1); + getRestTestHelper().setUsernameAndPassword(ADMIN, ADMIN); + getRestTestHelper().submitRequest(_virtualHostRuleProviderUrl, "PUT", update, HttpServletResponse.SC_OK); + + // Now allowed by the rule at the broker + assertCreateQueueAllowed(USER1); + } + + private void assertCreateAndDeleteQueueSucceeds(final String username) throws Exception { getRestTestHelper().setUsernameAndPassword(username, username); int responseCode = createQueue(); - assertEquals("Queue creation should be allowed", 201, responseCode); + assertEquals("Queue creation should be allowed", HttpServletResponse.SC_CREATED, responseCode); assertQueueExists(); responseCode = getRestTestHelper().submitRequest(_queueUrl, "DELETE"); - assertEquals("Queue deletion should be allowed", 200, responseCode); + assertEquals("Queue deletion should be allowed", HttpServletResponse.SC_OK, responseCode); assertQueueDoesNotExist(); } - public void assertCreateQueueDenied(String username) throws Exception + private void assertCreateQueueDenied(String username) throws Exception { getRestTestHelper().setUsernameAndPassword(username, username); int responseCode = createQueue(); - assertEquals("Queue creation should be denied", 403, responseCode); + assertEquals("Queue creation should be denied", HttpServletResponse.SC_FORBIDDEN, responseCode); assertQueueDoesNotExist(); } + private void assertCreateQueueAllowed(String username) throws Exception + { + getRestTestHelper().setUsernameAndPassword(username, username); + + int responseCode = createQueue(); + assertEquals("Queue creation should be allowed", HttpServletResponse.SC_CREATED, responseCode); + } + private int createQueue() throws Exception { - Map<String, Object> attributes = new HashMap<String, Object>(); + Map<String, Object> attributes = new HashMap<>(); attributes.put(Queue.NAME, _queueName); return getRestTestHelper().submitRequest(_queueUrl, "PUT", attributes); @@ -208,10 +243,10 @@ public class VirtualHostAccessControlProviderRestTest extends QpidRestTestCase private LegacyOperation _operation; private RuleOutcome _outcome; - public TestAclRule(final String identity, - final ObjectType objectType, - final LegacyOperation operation, - final RuleOutcome outcome) + TestAclRule(final String identity, + final ObjectType objectType, + final LegacyOperation operation, + final RuleOutcome outcome) { _identity = identity; _objectType = objectType; --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
