[GitHub] [activemq-artemis] gemmellr commented on a diff in pull request #4099: ARTEMIS-3692 support temporary queue namespace w/security-settings

[GitBox <gi...@apache.org>]

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.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org