[AMQ-6787] release securty context on failure to addConnection subsequent to auth, resolve leak. fix and test

activemq-broker/src/main/java/org/apache/activemq/security/JaasAuthenticationBroker.java

@@ -63,16 +63,24 @@ public class JaasAuthenticationBroker extends AbstractAuthenticationBroker {
             // Set the TCCL since it seems JAAS needs it to find the login module classes.
             ClassLoader original = Thread.currentThread().getContextClassLoader();
             Thread.currentThread().setContextClassLoader(JaasAuthenticationBroker.class.getClassLoader());
-
+            SecurityContext securityContext = null;
             try {
-                SecurityContext s = authenticate(info.getUserName(), info.getPassword(), null);
-                context.setSecurityContext(s);
-                securityContexts.add(s);
+                securityContext = authenticate(info.getUserName(), info.getPassword(), null);
+                context.setSecurityContext(securityContext);
+                securityContexts.add(securityContext);
+                super.addConnection(context, info);
+            } catch (Exception error) {
+                if (securityContext != null) {
+                    securityContexts.remove(securityContext);
+                }
+                context.setSecurityContext(null);
+                throw error;
             } finally {
                 Thread.currentThread().setContextClassLoader(original);
             }
+        } else {
+            super.addConnection(context, info);
         }
-        super.addConnection(context, info);
     }
 
     @Override

activemq-unit-tests/src/test/java/org/apache/activemq/broker/StubBroker.java

@@ -17,6 +17,7 @@
 
 package org.apache.activemq.broker;
 
+import javax.jms.InvalidClientIDException;
 import java.util.LinkedList;
 import org.apache.activemq.command.ConnectionInfo;
 
@@ -47,6 +48,11 @@ public class StubBroker extends EmptyBroker {
     }
 
     public void addConnection(ConnectionContext context, ConnectionInfo info) throws Exception {
+        for (AddConnectionData data : addConnectionData) {
+            if (data.connectionInfo.getClientId() != null && data.connectionInfo.getClientId().equals(info.getClientId())) {
+                throw new InvalidClientIDException("ClientID already exists");
+            }
+        }
         addConnectionData.add(new AddConnectionData(context, info));
     }
 

activemq-unit-tests/src/test/java/org/apache/activemq/security/JaasAuthenticationBrokerTest.java

@@ -0,0 +1,97 @@
+/**
+ * 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 javax.jms.InvalidClientIDException;
+import javax.security.auth.login.AppConfigurationEntry;
+import javax.security.auth.login.Configuration;
+import java.util.HashMap;
+import java.util.concurrent.CopyOnWriteArrayList;
+
+import junit.framework.TestCase;
+
+import org.apache.activemq.broker.Broker;
+import org.apache.activemq.broker.ConnectionContext;
+import org.apache.activemq.broker.StubBroker;
+import org.apache.activemq.command.ConnectionInfo;
+
+public class JaasAuthenticationBrokerTest extends TestCase {
+    StubBroker receiveBroker;
+
+    JaasAuthenticationBroker authBroker;
+
+    ConnectionContext connectionContext;
+    ConnectionInfo connectionInfo;
+
+    CopyOnWriteArrayList<SecurityContext> visibleSecurityContexts;
+
+    class JaasAuthenticationBrokerTester extends JaasAuthenticationBroker {
+        public JaasAuthenticationBrokerTester(Broker next, String jassConfiguration) {
+            super(next, jassConfiguration);
+            visibleSecurityContexts = securityContexts;
+        }
+    }
+
+    @Override
+    protected void setUp() throws Exception {
+        receiveBroker = new StubBroker();
+
+        authBroker = new JaasAuthenticationBrokerTester(receiveBroker, "");
+
+        connectionContext = new ConnectionContext();
+        connectionInfo = new ConnectionInfo();
+    }
+
+    @Override
+    protected void tearDown() throws Exception {
+        super.tearDown();
+    }
+
+    private void setConfiguration(boolean loginShouldSucceed) {
+        HashMap<String, String> configOptions = new HashMap<String, String>();
+
+        configOptions.put(StubLoginModule.ALLOW_LOGIN_PROPERTY, loginShouldSucceed ? "true" : "false");
+        configOptions.put(StubLoginModule.USERS_PROPERTY, "");
+        configOptions.put(StubLoginModule.GROUPS_PROPERTY, "");
+        AppConfigurationEntry configEntry = new AppConfigurationEntry("org.apache.activemq.security.StubLoginModule", AppConfigurationEntry.LoginModuleControlFlag.REQUIRED, configOptions);
+
+        StubJaasConfiguration jaasConfig = new StubJaasConfiguration(configEntry);
+
+        Configuration.setConfiguration(jaasConfig);
+    }
+
+
+    public void testAddConnectionFailureOnDuplicateClientId() throws Exception {
+        setConfiguration(true);
+
+        connectionInfo.setClientId("CliIdX");
+        authBroker.addConnection(connectionContext, connectionInfo);
+        ConnectionContext secondContext = connectionContext.copy();
+        secondContext.setSecurityContext(null);
+        ConnectionInfo secondInfo = connectionInfo.copy();
+        try {
+            authBroker.addConnection(secondContext, secondInfo);
+            fail("Expect duplicate id");
+        } catch (InvalidClientIDException expected) {
+        }
+
+        assertEquals("one connection allowed.", 1, receiveBroker.addConnectionData.size());
+        assertEquals("one context .", 1, visibleSecurityContexts.size());
+
+    }
+}