You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@qpid.apache.org by kw...@apache.org on 2017/09/29 12:31:45 UTC
[2/2] qpid-broker-j git commit: QPID-7934: [Java Broker] [Model [ACL]
Ensure that RuleBasedAccessControlProvider reports changes in the rule set to
the control point (Broker/VirtualHost)
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 <kw...@apache.org>
Authored: Fri Sep 29 11:09:43 2017 +0100
Committer: Keith Wall <kw...@apache.org>
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: commits-unsubscribe@qpid.apache.org
For additional commands, e-mail: commits-help@qpid.apache.org