[
https://issues.apache.org/jira/browse/ARTEMIS-3692?focusedWorklogId=775499&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-775499
]
ASF GitHub Bot logged work on ARTEMIS-3692:
-------------------------------------------
Author: ASF GitHub Bot
Created on: 27/May/22 15:10
Start Date: 27/May/22 15:10
Worklog Time Spent: 10m
Work Description: gemmellr commented on code in PR #4099:
URL: https://github.com/apache/activemq-artemis/pull/4099#discussion_r883665970
##########
tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/amqp/JMSTemporaryDestinationWithSecurityTest.java:
##########
@@ -0,0 +1,239 @@
+/*
+ * 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.activemq.artemis.tests.integration.amqp;
+
+import javax.jms.Connection;
+import javax.jms.JMSSecurityException;
+import javax.jms.MessageConsumer;
+import javax.jms.MessageProducer;
+import javax.jms.Session;
+import javax.jms.TemporaryQueue;
+import javax.jms.TemporaryTopic;
+import javax.jms.TextMessage;
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.Map;
+import java.util.Set;
+
+import org.apache.activemq.artemis.core.security.Role;
+import org.apache.activemq.artemis.core.server.ActiveMQServer;
+import org.apache.activemq.artemis.core.server.Queue;
+import org.apache.activemq.artemis.core.settings.HierarchicalRepository;
+import
org.apache.activemq.artemis.spi.core.security.ActiveMQJAASSecurityManager;
+import org.apache.activemq.artemis.tests.util.RandomUtil;
+import org.apache.activemq.artemis.tests.util.Wait;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+
+@RunWith(value = Parameterized.class)
+public class JMSTemporaryDestinationWithSecurityTest extends
JMSClientTestSupport {
+
+ private SecureConnectionSupplier connectionSupplier;
+ private final String TEMP_QUEUE_NAMESPACE = RandomUtil.randomString();
+ private String createOnlyUser = "createonly";
+ private String createOnlyPass = "createonly";
+
+ @Parameterized.Parameters(name = "connectionSupplier={0}")
+ public static Collection getParameters() {
+ return Arrays.asList(new Object[][]{
+ {"AMQP"},
+ {"CORE"},
+ {"OPENWIRE"}
+ });
+ }
+
+ public JMSTemporaryDestinationWithSecurityTest(String connectionSupplier) {
+ switch (connectionSupplier) {
+ case "AMQP":
+ this.connectionSupplier = (username, password) ->
createConnection(username, password);
+ break;
+ case "CORE":
+ this.connectionSupplier = (username, password) ->
createCoreConnection(username, password);
+ break;
+ case "OPENWIRE":
+ this.connectionSupplier = (username, password) ->
createOpenWireConnection(username, password);
+ break;
+ }
+ }
+
+ @Override
+ protected boolean isSecurityEnabled() {
+ return true;
+ }
+
+ @Override
+ protected void enableSecurity(ActiveMQServer server, String...
securityMatches) {
+ super.enableSecurity(server, TEMP_QUEUE_NAMESPACE + ".#");
+
+ // add a new user/role who can create but not delete
+ ActiveMQJAASSecurityManager securityManager =
(ActiveMQJAASSecurityManager) server.getSecurityManager();
+ securityManager.getConfiguration().addUser(createOnlyUser,
createOnlyPass);
+ securityManager.getConfiguration().addRole(createOnlyUser, "createonly");
+ HierarchicalRepository<Set<Role>> securityRepository =
server.getSecurityRepository();
+ Set<Role> value = securityRepository.getMatch(TEMP_QUEUE_NAMESPACE +
".#");
+ value.add(new Role("createonly", false, false, true, false, true, false,
false, false, true, false));
+ }
+ @Override
+ protected void configureAMQPAcceptorParameters(Map<String, Object> params) {
+ params.put("supportAdvisory", "false");
+ }
+
+ @Override
+ protected String getConfiguredProtocols() {
+ return "AMQP,OPENWIRE,CORE";
+ }
+
+ @Override
+ protected void addConfiguration(ActiveMQServer server) {
+ super.addConfiguration(server);
+
server.getConfiguration().setTemporaryQueueNamespace(TEMP_QUEUE_NAMESPACE);
+ }
+
+ @Test(timeout = 60000)
+ public void testCreateTemporaryQueue() throws Throwable {
+ Connection connection = connectionSupplier.createConnection(fullUser,
fullPass);
+
+ Session session = connection.createSession(false,
Session.AUTO_ACKNOWLEDGE);
+ TemporaryQueue queue = session.createTemporaryQueue();
+ MessageProducer producer = session.createProducer(queue);
+
+ TextMessage message = session.createTextMessage();
+ message.setText("Message temporary");
+ producer.send(message);
+
+ MessageConsumer consumer = session.createConsumer(queue);
+ connection.start();
+
+ message = (TextMessage) consumer.receive(5000);
+
+ assertNotNull(message);
+ }
+
+ @Test(timeout = 60000)
+ public void testCreateTemporaryQueueNegative() throws Throwable {
+ Connection connection = connectionSupplier.createConnection(noprivUser,
noprivPass);
+
+ Session session = connection.createSession(false,
Session.AUTO_ACKNOWLEDGE);
+ try {
+ session.createTemporaryQueue();
+ fail();
+ } catch (JMSSecurityException e) {
+ // expected
+ }
+ }
+
+ @Test(timeout = 30000)
+ public void testDeleteTemporaryQueue() throws Exception {
+ Connection connection = connectionSupplier.createConnection(fullUser,
fullPass);
+
+ Session session = connection.createSession(false,
Session.AUTO_ACKNOWLEDGE);
+ final javax.jms.TemporaryQueue queue = session.createTemporaryQueue();
+ assertNotNull(queue);
+
+ Queue queueView = getProxyToQueue(queue.getQueueName());
+ assertNotNull(queueView);
+
+ queue.delete();
+
+ Wait.assertTrue("Temp Queue should be deleted.", () ->
getProxyToQueue(queue.getQueueName()) == null, 3000, 50);
+ }
+
+ @Test(timeout = 30000)
+ public void testDeleteTemporaryQueueNegative() throws Exception {
+ Connection connection =
connectionSupplier.createConnection(createOnlyUser, createOnlyPass);
+
+ Session session = connection.createSession(false,
Session.AUTO_ACKNOWLEDGE);
+ final javax.jms.TemporaryQueue queue = session.createTemporaryQueue();
+ assertNotNull(queue);
+
+ Queue queueView = getProxyToQueue(queue.getQueueName());
+ assertNotNull(queueView);
+
+ try {
+ queue.delete();
+ fail();
+ } catch (JMSSecurityException e) {
+ // expected
+ }
Review Comment:
What happens when the Connection closes, with the TemporaryQueue supposed to
be inherently tied to its lifecycle? Is it still deleted as it would be
expected? Does it barf and orphan it to clog up the broker?
Seems like it would be important to test that, if testing the rest of this
seemingly unusual 'cant delete what you create' setup.
##########
tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/amqp/JMSTemporaryDestinationWithSecurityTest.java:
##########
@@ -0,0 +1,239 @@
+/*
+ * 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.activemq.artemis.tests.integration.amqp;
+
+import javax.jms.Connection;
+import javax.jms.JMSSecurityException;
+import javax.jms.MessageConsumer;
+import javax.jms.MessageProducer;
+import javax.jms.Session;
+import javax.jms.TemporaryQueue;
+import javax.jms.TemporaryTopic;
+import javax.jms.TextMessage;
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.Map;
+import java.util.Set;
+
+import org.apache.activemq.artemis.core.security.Role;
+import org.apache.activemq.artemis.core.server.ActiveMQServer;
+import org.apache.activemq.artemis.core.server.Queue;
+import org.apache.activemq.artemis.core.settings.HierarchicalRepository;
+import
org.apache.activemq.artemis.spi.core.security.ActiveMQJAASSecurityManager;
+import org.apache.activemq.artemis.tests.util.RandomUtil;
+import org.apache.activemq.artemis.tests.util.Wait;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+
+@RunWith(value = Parameterized.class)
+public class JMSTemporaryDestinationWithSecurityTest extends
JMSClientTestSupport {
+
+ private SecureConnectionSupplier connectionSupplier;
+ private final String TEMP_QUEUE_NAMESPACE = RandomUtil.randomString();
+ private String createOnlyUser = "createonly";
+ private String createOnlyPass = "createonly";
+
+ @Parameterized.Parameters(name = "connectionSupplier={0}")
+ public static Collection getParameters() {
+ return Arrays.asList(new Object[][]{
+ {"AMQP"},
+ {"CORE"},
+ {"OPENWIRE"}
+ });
+ }
+
+ public JMSTemporaryDestinationWithSecurityTest(String connectionSupplier) {
+ switch (connectionSupplier) {
+ case "AMQP":
+ this.connectionSupplier = (username, password) ->
createConnection(username, password);
+ break;
+ case "CORE":
+ this.connectionSupplier = (username, password) ->
createCoreConnection(username, password);
+ break;
+ case "OPENWIRE":
+ this.connectionSupplier = (username, password) ->
createOpenWireConnection(username, password);
+ break;
+ }
+ }
+
+ @Override
+ protected boolean isSecurityEnabled() {
+ return true;
+ }
+
+ @Override
+ protected void enableSecurity(ActiveMQServer server, String...
securityMatches) {
+ super.enableSecurity(server, TEMP_QUEUE_NAMESPACE + ".#");
+
+ // add a new user/role who can create but not delete
+ ActiveMQJAASSecurityManager securityManager =
(ActiveMQJAASSecurityManager) server.getSecurityManager();
+ securityManager.getConfiguration().addUser(createOnlyUser,
createOnlyPass);
+ securityManager.getConfiguration().addRole(createOnlyUser, "createonly");
+ HierarchicalRepository<Set<Role>> securityRepository =
server.getSecurityRepository();
+ Set<Role> value = securityRepository.getMatch(TEMP_QUEUE_NAMESPACE +
".#");
+ value.add(new Role("createonly", false, false, true, false, true, false,
false, false, true, false));
+ }
+ @Override
+ protected void configureAMQPAcceptorParameters(Map<String, Object> params) {
+ params.put("supportAdvisory", "false");
+ }
+
+ @Override
+ protected String getConfiguredProtocols() {
+ return "AMQP,OPENWIRE,CORE";
+ }
+
+ @Override
+ protected void addConfiguration(ActiveMQServer server) {
+ super.addConfiguration(server);
+
server.getConfiguration().setTemporaryQueueNamespace(TEMP_QUEUE_NAMESPACE);
+ }
+
+ @Test(timeout = 60000)
+ public void testCreateTemporaryQueue() throws Throwable {
+ Connection connection = connectionSupplier.createConnection(fullUser,
fullPass);
+
+ Session session = connection.createSession(false,
Session.AUTO_ACKNOWLEDGE);
+ TemporaryQueue queue = session.createTemporaryQueue();
+ MessageProducer producer = session.createProducer(queue);
+
+ TextMessage message = session.createTextMessage();
+ message.setText("Message temporary");
+ producer.send(message);
+
+ MessageConsumer consumer = session.createConsumer(queue);
+ connection.start();
+
+ message = (TextMessage) consumer.receive(5000);
+
+ assertNotNull(message);
+ }
+
+ @Test(timeout = 60000)
+ public void testCreateTemporaryQueueNegative() throws Throwable {
+ Connection connection = connectionSupplier.createConnection(noprivUser,
noprivPass);
+
+ Session session = connection.createSession(false,
Session.AUTO_ACKNOWLEDGE);
+ try {
+ session.createTemporaryQueue();
+ fail();
+ } catch (JMSSecurityException e) {
+ // expected
+ }
+ }
+
+ @Test(timeout = 30000)
+ public void testDeleteTemporaryQueue() throws Exception {
+ Connection connection = connectionSupplier.createConnection(fullUser,
fullPass);
+
+ Session session = connection.createSession(false,
Session.AUTO_ACKNOWLEDGE);
+ final javax.jms.TemporaryQueue queue = session.createTemporaryQueue();
+ assertNotNull(queue);
+
+ Queue queueView = getProxyToQueue(queue.getQueueName());
+ assertNotNull(queueView);
+
+ queue.delete();
+
+ Wait.assertTrue("Temp Queue should be deleted.", () ->
getProxyToQueue(queue.getQueueName()) == null, 3000, 50);
Review Comment:
50ms seems a long time to wait between checks for a simple deletion after a
synchronous operation. 10?
##########
artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/proton/ProtonServerReceiverContext.java:
##########
@@ -271,7 +280,13 @@ public void close(boolean remoteLinkClose) throws
ActiveMQAMQPException {
org.apache.qpid.proton.amqp.messaging.Target target =
(org.apache.qpid.proton.amqp.messaging.Target) receiver.getRemoteTarget();
if (target != null && target.getDynamic() && (target.getExpiryPolicy()
== TerminusExpiryPolicy.LINK_DETACH || target.getExpiryPolicy() ==
TerminusExpiryPolicy.SESSION_END)) {
try {
-
sessionSPI.removeTemporaryQueue(SimpleString.toSimpleString(target.getAddress()));
+ if (Arrays.stream(target.getCapabilities()).anyMatch(c ->
c.equals(TEMP_QUEUE_CAPABILITY))) {
+
sessionSPI.removeTemporaryQueue(SimpleString.toSimpleString(target.getAddress()));
+ } else {
+
sessionSPI.removeTemporaryAddress(SimpleString.toSimpleString(target.getAddress()));
+ }
Review Comment:
This "Arrays.stream(target.getCapabilities()).anyMatch(c ->
c.equals(TEMP_QUEUE_CAPABILITY)" doesnt seem to align with the behaviour used
for creation, see getRoutingType() and its uses, where there are different
behaviours for cases there are no capabilities given.
##########
artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/proton/ProtonServerReceiverContext.java:
##########
@@ -127,7 +136,7 @@ public void initialize() throws Exception {
}
try {
- sessionSPI.check(address, CheckType.SEND,
connection.getSecurityAuth());
+ sessionSPI.check(address, CheckType.SEND,
connection.getSecurityAuth(),
Arrays.stream(target.getCapabilities()).anyMatch(c ->
c.equals(TEMP_QUEUE_CAPABILITY) || c.equals(TEMP_TOPIC_CAPABILITY)));
Review Comment:
What happens if the target didnt have any capability? Some clients (i.e
non-JMS ones) wont know or care about those, but just be establishing a
producer to whatever address they were told, e.g reply-to address from a
message they got.
(Even for JMS clients, anonymous producers, those created without an
destination/address, wont have the capabilities on the link target as the
destination isnt known; thats somewhat handled elsewhere by looking at an
annotation the JMS client adds, this branch here only happens for links that
had a specific address).
##########
tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/amqp/JMSTemporaryDestinationWithSecurityTest.java:
##########
@@ -0,0 +1,239 @@
+/*
+ * 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.activemq.artemis.tests.integration.amqp;
+
+import javax.jms.Connection;
+import javax.jms.JMSSecurityException;
+import javax.jms.MessageConsumer;
+import javax.jms.MessageProducer;
+import javax.jms.Session;
+import javax.jms.TemporaryQueue;
+import javax.jms.TemporaryTopic;
+import javax.jms.TextMessage;
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.Map;
+import java.util.Set;
+
+import org.apache.activemq.artemis.core.security.Role;
+import org.apache.activemq.artemis.core.server.ActiveMQServer;
+import org.apache.activemq.artemis.core.server.Queue;
+import org.apache.activemq.artemis.core.settings.HierarchicalRepository;
+import
org.apache.activemq.artemis.spi.core.security.ActiveMQJAASSecurityManager;
+import org.apache.activemq.artemis.tests.util.RandomUtil;
+import org.apache.activemq.artemis.tests.util.Wait;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+
+@RunWith(value = Parameterized.class)
+public class JMSTemporaryDestinationWithSecurityTest extends
JMSClientTestSupport {
+
+ private SecureConnectionSupplier connectionSupplier;
+ private final String TEMP_QUEUE_NAMESPACE = RandomUtil.randomString();
+ private String createOnlyUser = "createonly";
+ private String createOnlyPass = "createonly";
+
+ @Parameterized.Parameters(name = "connectionSupplier={0}")
+ public static Collection getParameters() {
+ return Arrays.asList(new Object[][]{
+ {"AMQP"},
+ {"CORE"},
+ {"OPENWIRE"}
Review Comment:
The non-AMQP tests shouldn't be in this package, they should go in the
respective core and openwire related package trees. 2/3rds of the tests here
dont use AMQP at all it seems, which just shouldnt be the case.
I'd suggest a subclass in each of the appropriate packages overriding
something from a base class to provide the appropriate ConnectionSupplier /
config. Or sticking this as-is in a specific cross-protocol handling package,
related to the functionality.
##########
docs/user-manual/en/security.md:
##########
@@ -179,6 +179,28 @@ FQQN) in the `match` of the `security-setting`, e.g.:
**Note:** Wildcard matching doesn't work in conjuction with FQQN. The explicit
goal of using FQQN here is to be *exact*.
+### Temporary Queues
+
+The `temporary-queue-namespace` value can be used with a `security-setting`
+match just as it can with an
[`address-setting`](address-model.md#temporary-queues).
+For example:
+
+```xml
+<temporary-queue-namespace>temp</temporary-queue-namespace>
+
+<security-settings>
+ <security-setting match="temp.#">
+ <permission type="createAddress" roles="myRole"/>
+ <permission type="createNonDurableQueue" roles="myRole"/>
+ <permission type="deleteAddress" roles="myRole"/>
+ <permission type="deleteNonDurableQueue" roles="myRole"/>
+ </security-setting>
+</security-settings>
+```
+
+Using this configuration any user in `myRole` can create and delete all the
+resources necessary to deal with temporary JMS queues or topics.
Review Comment:
maybe expand "but users without that role will be prevented from creating
them" for clarity? That extra bit being a key reason you would do this.
##########
artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/proton/ProtonServerReceiverContext.java:
##########
@@ -95,7 +100,11 @@ public void initialize() throws Exception {
defRoutingType = getRoutingType(target.getCapabilities(), address);
try {
- sessionSPI.createTemporaryQueue(address, defRoutingType);
+ if (defRoutingType == RoutingType.ANYCAST) {
+ sessionSPI.createTemporaryQueue(address, defRoutingType);
Review Comment:
It doesnt seem like the address generated just above this changed either,
though I notice it being manipulated elsewhere to add and remove the namespace.
Wouldnt it be simpler to have the broker-generated address actually include the
namespace its considered to be part of? Then the address used everywhere is
simply the same actual queue/address name.
Issue Time Tracking
-------------------
Worklog Id: (was: 775499)
Time Spent: 0.5h (was: 20m)
> Extend Functionality of Temporary Queue Namespace to Security Settings
> ----------------------------------------------------------------------
>
> Key: ARTEMIS-3692
> URL: https://issues.apache.org/jira/browse/ARTEMIS-3692
> Project: ActiveMQ Artemis
> Issue Type: Improvement
> Reporter: Kevin O'Neal
> Priority: Major
> Time Spent: 0.5h
> Remaining Estimate: 0h
>
> Currently the temporary-queue-namespace is only relevant for
> address-settings, not security-settings. Therefore, the only way to enforce
> security settings on temporary queues is to use the match "#".
--
This message was sent by Atlassian Jira
(v8.20.7#820007)
