ARTEMIS-1821 LDAPLoginModule always returns true on commit()

artemis-server/src/main/java/org/apache/activemq/artemis/spi/core/security/jaas/LDAPLoginModule.java

@@ -181,15 +181,16 @@ public class LDAPLoginModule implements LoginModule {
 
    @Override
    public boolean logout() throws LoginException {
-      username = null;
+      clear();
       return true;
    }
 
    @Override
    public boolean commit() throws LoginException {
+      boolean result = userAuthenticated;
       Set<UserPrincipal> authenticatedUsers = subject.getPrincipals(UserPrincipal.class);
       Set<Principal> principals = subject.getPrincipals();
-      if (userAuthenticated) {
+      if (result) {
          principals.add(new UserPrincipal(username));
       }
 
@@ -210,12 +211,18 @@ public class LDAPLoginModule implements LoginModule {
       for (RolePrincipal gp : groups) {
          principals.add(gp);
       }
-      return true;
+      clear();
+      return result;
+   }
+
+   private void clear() {
+      username = null;
+      userAuthenticated = false;
    }
 
    @Override
    public boolean abort() throws LoginException {
-      username = null;
+      clear();
       return true;
    }
 

artemis-server/src/test/java/org/apache/activemq/artemis/core/security/jaas/LDAPLoginModuleTest.java

@@ -21,6 +21,7 @@ import javax.naming.NameClassPair;
 import javax.naming.NamingEnumeration;
 import javax.naming.directory.DirContext;
 import javax.naming.directory.InitialDirContext;
+import javax.security.auth.Subject;
 import javax.security.auth.callback.Callback;
 import javax.security.auth.callback.CallbackHandler;
 import javax.security.auth.callback.NameCallback;
@@ -28,10 +29,14 @@ import javax.security.auth.callback.PasswordCallback;
 import javax.security.auth.callback.UnsupportedCallbackException;
 import javax.security.auth.login.LoginContext;
 import javax.security.auth.login.LoginException;
+import javax.security.auth.spi.LoginModule;
 import java.io.IOException;
+import java.util.HashMap;
 import java.util.HashSet;
 import java.util.Hashtable;
 
+import org.apache.activemq.artemis.spi.core.security.jaas.JaasCallbackHandler;
+import org.apache.activemq.artemis.spi.core.security.jaas.LDAPLoginModule;
 import org.apache.directory.server.annotations.CreateLdapServer;
 import org.apache.directory.server.annotations.CreateTransport;
 import org.apache.directory.server.core.annotations.ApplyLdifFiles;
@@ -43,6 +48,7 @@ import org.junit.Test;
 import org.junit.runner.RunWith;
 
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
 
@@ -142,4 +148,18 @@ public class LDAPLoginModuleTest extends AbstractLdapTestUnit {
       }
       fail("Should have failed authenticating");
    }
+
+   @Test
+   public void testCommitOnFailedLogin() throws LoginException {
+      LoginModule loginModule = new LDAPLoginModule();
+      JaasCallbackHandler callbackHandler = new JaasCallbackHandler(null, null, null);
+
+      loginModule.initialize(new Subject(), callbackHandler, null, new HashMap<String, Object>());
+
+      // login should return false due to null username
+      assertFalse(loginModule.login());
+
+      // since login failed commit should return false as well
+      assertFalse(loginModule.commit());
+   }
 }