ARTEMIS-1872 Improving Security Checks on AMQP Protocol

Also improving test coverage on SecureConfigurationTest
This commit will fix JMSConnectionWithSecurityTest.

artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/broker/AMQPSessionCallback.java

@@ -248,7 +248,7 @@ public class AMQPSessionCallback implements SessionCallback {
       try {
          serverSession.createQueue(address, queueName, routingType, filter, true, false);
       } catch (ActiveMQSecurityException se) {
-         throw ActiveMQAMQPProtocolMessageBundle.BUNDLE.securityErrorCreatingConsumer(se.getMessage());
+         throw ActiveMQAMQPProtocolMessageBundle.BUNDLE.securityErrorCreatingTempDestination(se.getMessage());
       }
    }
 

artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/proton/ProtonServerReceiverContext.java

@@ -29,6 +29,7 @@ import org.apache.activemq.artemis.protocol.amqp.broker.AMQPSessionCallback;
 import org.apache.activemq.artemis.protocol.amqp.exceptions.ActiveMQAMQPException;
 import org.apache.activemq.artemis.protocol.amqp.exceptions.ActiveMQAMQPInternalErrorException;
 import org.apache.activemq.artemis.protocol.amqp.exceptions.ActiveMQAMQPNotFoundException;
+import org.apache.activemq.artemis.protocol.amqp.exceptions.ActiveMQAMQPSecurityException;
 import org.apache.activemq.artemis.protocol.amqp.logger.ActiveMQAMQPProtocolMessageBundle;
 import org.apache.activemq.artemis.protocol.amqp.sasl.PlainSASLResult;
 import org.apache.activemq.artemis.protocol.amqp.sasl.SASLResult;
@@ -108,6 +109,8 @@ public class ProtonServerReceiverContext extends ProtonInitializable implements
 
             try {
                sessionSPI.createTemporaryQueue(address, defRoutingType);
+            } catch (ActiveMQAMQPSecurityException e) {
+               throw e;
             } catch (ActiveMQSecurityException e) {
                throw ActiveMQAMQPProtocolMessageBundle.BUNDLE.securityErrorCreatingTempDestination(e.getMessage());
             } catch (Exception e) {

tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/server/SecureConfigurationTest.java

@@ -25,10 +25,13 @@ import org.apache.activemq.artemis.jms.client.ActiveMQConnectionFactory;
 import org.apache.activemq.artemis.jms.server.config.impl.FileJMSConfiguration;
 import org.apache.activemq.artemis.spi.core.security.ActiveMQJAASSecurityManager;
 import org.apache.activemq.artemis.spi.core.security.jaas.InVMLoginModule;
+import org.apache.activemq.artemis.tests.integration.IntegrationTestLogger;
 import org.apache.activemq.artemis.tests.util.ActiveMQTestBase;
 import org.apache.qpid.jms.JmsConnectionFactory;
+import org.junit.After;
 import org.junit.Assert;
 import org.junit.Assume;
+import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.Parameterized;
@@ -58,24 +61,30 @@ public class SecureConfigurationTest extends ActiveMQTestBase {
    @Parameterized.Parameter(0)
    public String protocol;
 
+   ActiveMQServer server;
+
+   @Before
+   public void startSever() throws Exception {
+      server = getActiveMQServer("multicast_topic.xml");
+      server.start();
+   }
+
+   @After
+   public void stopServer() throws Exception {
+      try {
+         if (server != null) {
+            server.stop();
+         }
+      } catch (Throwable e) {
+         e.printStackTrace();
+      }
+   }
+
    @Test
    public void testSecureSharedDurableSubscriber() throws Exception {
       //This is because OpenWire does not support JMS 2.0
       Assume.assumeFalse(protocol.equals("OPENWIRE"));
-
+      ConnectionFactory connectionFactory = getConnectionFactory("b", "b");
-      ActiveMQServer server = getActiveMQServer("multicast_topic.xml");
-      try {
-         server.start();
-         internal_testSecureSharedDurableSubscriber(getConnectionFactory("b", "b"));
-      } finally {
-         try {
-            server.stop();
-         } catch (Exception e) {
-         }
-      }
-   }
-
-   private void internal_testSecureSharedDurableSubscriber(ConnectionFactory connectionFactory) throws JMSException {
       String message = "blah";
 
       //Expect to be able to create subscriber on pre-defined/existing queue.
@@ -101,20 +110,7 @@ public class SecureConfigurationTest extends ActiveMQTestBase {
    public void testSecureSharedSubscriber() throws Exception {
       //This is because OpenWire does not support JMS 2.0
       Assume.assumeFalse(protocol.equals("OPENWIRE"));
-
+      ConnectionFactory connectionFactory = getConnectionFactory("b", "b");
-      ActiveMQServer server = getActiveMQServer("multicast_topic.xml");
-      try {
-         server.start();
-         internal_testSecureSharedSubscriber(getConnectionFactory("b", "b"));
-      } finally {
-         try {
-            server.stop();
-         } catch (Exception e) {
-         }
-      }
-   }
-
-   private void internal_testSecureSharedSubscriber(ConnectionFactory connectionFactory) throws JMSException {
       String message = "blah";
 
       //Expect to be able to create subscriber on pre-defined/existing queue.
@@ -138,19 +134,7 @@ public class SecureConfigurationTest extends ActiveMQTestBase {
 
    @Test
    public void testSecureDurableSubscriber() throws Exception {
-      ActiveMQServer server = getActiveMQServer("multicast_topic.xml");
+      ConnectionFactory connectionFactory = getConnectionFactory("b", "b");
-      try {
-         server.start();
-         internal_testSecureDurableSubscriber(getConnectionFactory("b", "b"));
-      } finally {
-         try {
-            server.stop();
-         } catch (Exception e) {
-         }
-      }
-   }
-
-   private void internal_testSecureDurableSubscriber(ConnectionFactory connectionFactory) throws JMSException {
       String message = "blah";
 
       //Expect to be able to create subscriber on pre-defined/existing queue.
@@ -177,8 +161,31 @@ public class SecureConfigurationTest extends ActiveMQTestBase {
       } catch (JMSSecurityException j) {
          //Expected exception
       }
+
+      Connection connection = null;
+
+      try {
+         connection = connectionFactory.createConnection();
+         Session session = connection.createSession(false, Session.AUTO_ACKNOWLEDGE);
+
+         try {
+            session.createTemporaryQueue();
+            Assert.fail("Security exception expected, but did not occur, excepetion expected as not permissioned to create a temporary queue");
+         } catch (JMSSecurityException jmsse) {
+            IntegrationTestLogger.LOGGER.info("Client should have thrown a JMSSecurityException but only threw JMSException");
+         } catch (JMSException e) {
+            e.printStackTrace();
+            Assert.fail("thrown a JMSEXception instead of a JMSSEcurityException");
          }
 
+         // Should not be fatal
+         assertNotNull(connection.createSession(false, Session.AUTO_ACKNOWLEDGE));
+      } finally {
+         connection.close();
+      }
+   }
+
+
    private ConnectionFactory getConnectionFactory(String user, String password) {
       switch (protocol) {
          case "CORE": return getActiveMQConnectionFactory(user, password);

tests/integration-tests/src/test/resources/multicast_topic.xml

@@ -85,13 +85,13 @@ under the License.
 
       <security-settings>
          <security-setting match="#">
-            <permission type="createNonDurableQueue" roles="a,b"/>
+            <permission type="createNonDurableQueue" roles="a"/>
-            <permission type="deleteNonDurableQueue" roles="a,b"/>
+            <permission type="deleteNonDurableQueue" roles="a"/>
-            <permission type="createDurableQueue" roles="a,b"/>
+            <permission type="createDurableQueue" roles="a"/>
-            <permission type="deleteDurableQueue" roles="a,b"/>
+            <permission type="deleteDurableQueue" roles="a"/>
             <permission type="browse" roles="a"/>
-            <permission type="send" roles="a,b"/>
+            <permission type="send" roles="a"/>
-            <permission type="consume" roles="a,b" />
+            <permission type="consume" roles="a" />
             <!-- we need this otherwise ./artemis data imp wouldn't work -->
             <permission type="manage" roles="a"/>
          </security-setting>