https://issues.apache.org/jira/browse/AMQ-3182 - JAAS PropertiesLoginModule does not maintain internal validity state, so will commit in error after an invalid login attempt

https://issues.apache.org/jira/browse/AMQ-3183 - Set JMSXUserID value based on authenticated principal
Fixed up PropertiesLoginModule such that it maintains login state and only commits on success. Added attribute brokerService useAuthenticatedPrincipalForJMXUserID to indicate
that the first authenticated user principal should be used for the userName and hense by the userId broker when populateJMSXUserID is set. In the absense of a principal the
userName is unchanged.

git-svn-id: https://svn.apache.org/repos/asf/activemq/trunk@1071301 13f79535-47bb-0310-9956-ffa450edef68

activemq-core/src/main/java/org/apache/activemq/broker/BrokerService.java

@@ -113,6 +113,8 @@ public class BrokerService implements Service {
     private boolean enableStatistics = true;
     private boolean persistent = true;
     private boolean populateJMSXUserID;
+    private boolean useAuthenticatedPrincipalForJMXUserID;
+
     private boolean useShutdownHook = true;
     private boolean useLoggingForShutdownErrors;
     private boolean shutdownOnMasterFailure;
@@ -1882,7 +1884,9 @@ public class BrokerService implements Service {
         broker = new CompositeDestinationBroker(broker);
         broker = new TransactionBroker(broker, getPersistenceAdapter().createTransactionStore());
         if (isPopulateJMSXUserID()) {
-            broker = new UserIDBroker(broker);
+            UserIDBroker userIDBroker = new UserIDBroker(broker);
+            userIDBroker.setUseAuthenticatePrincipal(isUseAuthenticatedPrincipalForJMXUserID());
+            broker = userIDBroker;
         }
         if (isMonitorConnectionSplits()) {
             broker = new ConnectionSplitBroker(broker);
@@ -2338,4 +2342,12 @@ public class BrokerService implements Service {
     public void setBrokerId(String brokerId) {
         this.brokerId = new BrokerId(brokerId);
     }
+
+    public boolean isUseAuthenticatedPrincipalForJMXUserID() {
+        return useAuthenticatedPrincipalForJMXUserID;
+    }
+
+    public void setUseAuthenticatedPrincipalForJMXUserID(boolean useAuthenticatedPrincipalForJMXUserID) {
+        this.useAuthenticatedPrincipalForJMXUserID = useAuthenticatedPrincipalForJMXUserID;
+    }
 }

activemq-core/src/main/java/org/apache/activemq/broker/UserIDBroker.java

@@ -16,7 +16,10 @@
  */
 package org.apache.activemq.broker;
 
+import java.util.Set;
 import org.apache.activemq.command.Message;
+import org.apache.activemq.jaas.UserPrincipal;
+import org.apache.activemq.security.SecurityContext;
 
 /**
  * This broker filter will append the producer's user ID into the JMSXUserID header
@@ -27,7 +30,7 @@ import org.apache.activemq.command.Message;
  * 
  */
 public class UserIDBroker extends BrokerFilter {
-    
+    boolean useAuthenticatePrincipal = false;
     public UserIDBroker(Broker next) {
         super(next);
     }
@@ -35,7 +38,30 @@ public class UserIDBroker extends BrokerFilter {
     public void send(ProducerBrokerExchange producerExchange, Message messageSend) throws Exception {
         final ConnectionContext context = producerExchange.getConnectionContext();
         String userID = context.getUserName();
+        if (isUseAuthenticatePrincipal()) {
+            SecurityContext securityContext = context.getSecurityContext();
+            if (securityContext != null) {
+                Set<?> principals = securityContext.getPrincipals();
+                if (principals != null) {
+                    for (Object candidate : principals) {
+                        if (candidate instanceof UserPrincipal) {
+                            userID = ((UserPrincipal)candidate).getName();
+                            break;
+                        }
+                    }
+                }
+            }
+        }
         messageSend.setUserID(userID);
         super.send(producerExchange, messageSend);
     }
+
+
+    public boolean isUseAuthenticatePrincipal() {
+        return useAuthenticatePrincipal;
+    }
+
+    public void setUseAuthenticatePrincipal(boolean useAuthenticatePrincipal) {
+        this.useAuthenticatePrincipal = useAuthenticatePrincipal;
+    }
 }

activemq-core/src/main/java/org/apache/activemq/security/JaasDualAuthenticationBroker.java

@@ -112,11 +112,11 @@ public class JaasDualAuthenticationBroker extends BrokerFilter {
         } else {
             isSSL = false;
         }
+        super.removeConnection(context, info, error);
         if (isSSL) {
             this.sslBroker.removeConnection(context, info, error);
         } else {
             this.nonSslBroker.removeConnection(context, info, error);
         }
-        super.removeConnection(context, info, error);
     }
 }

activemq-core/src/test/java/org/apache/activemq/security/XBeanSecurityWithGuestTest.java

@@ -0,0 +1,125 @@
+/**
+ * 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.security;
+
+import java.net.URI;
+import javax.jms.Connection;
+import javax.jms.ConnectionFactory;
+import javax.jms.JMSException;
+import javax.jms.Message;
+import javax.jms.MessageConsumer;
+import javax.jms.Session;
+import javax.jms.TextMessage;
+import junit.framework.Test;
+import org.apache.activemq.ActiveMQConnection;
+import org.apache.activemq.ActiveMQConnectionFactory;
+import org.apache.activemq.CombinationTestSupport;
+import org.apache.activemq.JmsTestSupport;
+import org.apache.activemq.broker.BrokerFactory;
+import org.apache.activemq.broker.BrokerService;
+import org.apache.activemq.command.ActiveMQDestination;
+import org.apache.activemq.command.ActiveMQMessage;
+import org.apache.activemq.command.ActiveMQQueue;
+import org.apache.activemq.command.ActiveMQTopic;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class XBeanSecurityWithGuestTest extends JmsTestSupport {
+
+    private static final Logger LOG = LoggerFactory.getLogger(XBeanSecurityWithGuestTest.class);
+    public ActiveMQDestination destination;
+    
+    public static Test suite() {
+        return suite(XBeanSecurityWithGuestTest.class);
+    }
+    
+    public void testUserSendGoodPassword() throws JMSException {
+        Message m = doSend(false);
+        assertEquals("system", ((ActiveMQMessage)m).getUserID());
+        assertEquals("system", m.getStringProperty("JMSXUserID"));
+    }
+    
+    public void testUserSendWrongPassword() throws JMSException {
+        Message m = doSend(false);
+        // note brokerService.useAuthenticatedPrincipalForJMXUserID=true for this
+        assertEquals("guest", ((ActiveMQMessage)m).getUserID());
+        assertEquals("guest", m.getStringProperty("JMSXUserID"));
+    }
+
+    protected BrokerService createBroker() throws Exception {
+        return createBroker("org/apache/activemq/security/jaas-broker-guest.xml");
+    }
+
+    protected BrokerService createBroker(String uri) throws Exception {
+        LOG.info("Loading broker configuration from the classpath with URI: " + uri);
+        return BrokerFactory.createBroker(new URI("xbean:" + uri));
+    }
+
+    public Message doSend(boolean fail) throws JMSException {
+
+        Connection adminConnection = factory.createConnection("system", "manager");
+        connections.add(adminConnection);
+
+        adminConnection.start();
+        Session adminSession = adminConnection.createSession(false, Session.AUTO_ACKNOWLEDGE);
+        MessageConsumer consumer = adminSession.createConsumer(destination);
+
+        connections.remove(connection);
+        connection = (ActiveMQConnection)factory.createConnection(userName, password);
+        connections.add(connection);
+
+        Session session = connection.createSession(false, Session.AUTO_ACKNOWLEDGE);
+        try {
+            sendMessages(session, destination, 1);
+        } catch (JMSException e) {
+            // If test is expected to fail, the cause must only be a
+            // SecurityException
+            // otherwise rethrow the exception
+            if (!fail || !(e.getCause() instanceof SecurityException)) {
+                throw e;
+            }
+        }
+
+        Message m = consumer.receive(1000);
+        if (fail) {
+            assertNull(m);
+        } else {
+            assertNotNull(m);
+            assertEquals("0", ((TextMessage)m).getText());
+            assertNull(consumer.receiveNoWait());
+        }
+        return m;
+    }
+    
+    /**
+     * @see {@link CombinationTestSupport}
+     */
+    public void initCombosForTestUserSendGoodPassword() {
+        addCombinationValues("userName", new Object[] {"system"});
+        addCombinationValues("password", new Object[] {"manager"});
+        addCombinationValues("destination", new Object[] {new ActiveMQQueue("test"), new ActiveMQTopic("test")});
+    }
+    
+    /**
+     * @see {@link CombinationTestSupport}
+     */
+    public void initCombosForTestUserSendWrongPassword() {
+        addCombinationValues("userName", new Object[] {"system"});
+        addCombinationValues("password", new Object[] {"wrongpassword"});
+        addCombinationValues("destination", new Object[] {new ActiveMQQueue("GuestQueue")});
+    }
+}

activemq-core/src/test/resources/login.config

@@ -21,6 +21,17 @@ activemq-domain {
         org.apache.activemq.jaas.properties.group="org/apache/activemq/security/groups.properties";
 };
 
+activemq-guest-domain {
+    org.apache.activemq.jaas.PropertiesLoginModule sufficient
+        debug=true
+        org.apache.activemq.jaas.properties.user="org/apache/activemq/security/users.properties"
+        org.apache.activemq.jaas.properties.group="org/apache/activemq/security/groups.properties";
+    org.apache.activemq.jaas.GuestLoginModule sufficient
+       debug=true
+       org.apache.activemq.jaas.guest.user="guest"
+       org.apache.activemq.jaas.guest.group="guests";
+};
+
 cert-login {
     org.apache.activemq.jaas.TextFileCertificateLoginModule required
         debug=true

activemq-core/src/test/resources/org/apache/activemq/security/jaas-broker-guest.xml

@@ -0,0 +1,56 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+  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.
+-->
+
+<beans
+  xmlns="http://www.springframework.org/schema/beans"
+  xmlns:amq="http://activemq.apache.org/schema/core"
+  xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+  xsi:schemaLocation="http://www.springframework.org/schema/beans http://www.springframework.org/schema/beans/spring-beans-2.0.xsd
+  http://activemq.apache.org/schema/core http://activemq.apache.org/schema/core/activemq-core.xsd">
+
+  <bean class="org.springframework.beans.factory.config.PropertyPlaceholderConfigurer"/>
+
+  <broker useJmx="false" persistent="false" xmlns="http://activemq.apache.org/schema/core"
+          populateJMSXUserID="true"
+          useAuthenticatedPrincipalForJMXUserID="true">
+
+    <plugins>
+      <!--  use JAAS to authenticate using the login.config file on the classpath to configure JAAS -->
+      <jaasDualAuthenticationPlugin configuration="activemq-guest-domain" sslConfiguration="cert-login" />
+
+      <!--  lets configure a destination based authorization mechanism -->
+      <authorizationPlugin>
+        <map>
+          <authorizationMap>
+            <authorizationEntries>
+             <authorizationEntry queue="&gt;" read="admins" write="admins" admin="admins"/>
+             <authorizationEntry topic="&gt;" read="admins" write="admins" admin="admins"/>
+             <authorizationEntry queue="GuestQueue" read="admins" write="admins, guests" admin="admins"/>
+             <authorizationEntry topic="ActiveMQ.Advisory.&gt;" read="guests" write="guests" admin="guests"/>
+            </authorizationEntries>    
+          </authorizationMap>
+        </map>
+      </authorizationPlugin>
+    </plugins>
+    
+    <transportConnectors>
+		<transportConnector name="stomp"   uri="stomp://localhost:61613"/>
+	</transportConnectors>
+  </broker>
+
+</beans>

activemq-jaas/src/main/java/org/apache/activemq/jaas/PropertiesLoginModule.java

@@ -59,10 +59,12 @@ public class PropertiesLoginModule implements LoginModule {
     private String user;
     private Set<Principal> principals = new HashSet<Principal>();
     private File baseDir;
+    private boolean loginSucceeded;
 
     public void initialize(Subject subject, CallbackHandler callbackHandler, Map sharedState, Map options) {
         this.subject = subject;
         this.callbackHandler = callbackHandler;
+        loginSucceeded = false;
 
         if (System.getProperty("java.security.auth.login.config") != null) {
             baseDir = new File(System.getProperty("java.security.auth.login.config")).getParentFile();
@@ -121,15 +123,18 @@ public class PropertiesLoginModule implements LoginModule {
         if (!password.equals(new String(tmpPassword))) {
             throw new FailedLoginException("Password does not match");
         }
+        loginSucceeded = true;
         users.clear();
 
         if (debug) {
             LOG.debug("login " + user);
         }
-        return true;
+        return loginSucceeded;
     }
 
     public boolean commit() throws LoginException {
+        boolean result = loginSucceeded;
+        if (result) {
             principals.add(new UserPrincipal(user));
 
             for (Enumeration enumeration = groups.keys(); enumeration.hasMoreElements();) {
@@ -144,13 +149,15 @@ public class PropertiesLoginModule implements LoginModule {
             }
 
             subject.getPrincipals().addAll(principals);
+        }
 
+        // will whack loginSucceeded
         clear();
 
         if (debug) {
-            LOG.debug("commit");
+            LOG.debug("commit, result: " + result);
         }
-        return true;
+        return result;
     }
 
     public boolean abort() throws LoginException {
@@ -165,7 +172,7 @@ public class PropertiesLoginModule implements LoginModule {
     public boolean logout() throws LoginException {
         subject.getPrincipals().removeAll(principals);
         principals.clear();
-
+        clear();
         if (debug) {
             LOG.debug("logout");
         }
@@ -175,5 +182,6 @@ public class PropertiesLoginModule implements LoginModule {
     private void clear() {
         groups.clear();
         user = null;
+        loginSucceeded = false;
     }
 }