AMQ-7137 - Implement abort() properly in the LoginModules. Also fix a bug in LdapLoginModule relating to Logout

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

@@ -49,10 +49,13 @@ public abstract class CertificateLoginModule extends PropertiesLoader implements
     private CallbackHandler callbackHandler;
     private Subject subject;
 
-    private X509Certificate certificates[];
     private String username;
     private Set<Principal> principals = new HashSet<Principal>();
 
+    /** the authentication status*/
+    private boolean succeeded = false;
+    private boolean commitSucceeded = false;
+
     /**
      * Overriding to allow for proper initialization. Standard JAAS.
      */
@@ -78,7 +81,7 @@ public abstract class CertificateLoginModule extends PropertiesLoader implements
         } catch (UnsupportedCallbackException uce) {
             throw new LoginException(uce.getMessage() + " Unable to obtain client certificates.");
         }
-        certificates = ((CertificateCallback)callbacks[0]).getCertificates();
+        X509Certificate[] certificates = ((CertificateCallback)callbacks[0]).getCertificates();
 
         username = getUserNameForCertificates(certificates);
         if (username == null) {
@@ -88,6 +91,7 @@ public abstract class CertificateLoginModule extends PropertiesLoader implements
         if (debug) {
             LOG.debug("Certificate for user: " + username);
         }
+        succeeded = true;
         return true;
     }
 
@@ -96,6 +100,15 @@ public abstract class CertificateLoginModule extends PropertiesLoader implements
      */
     @Override
     public boolean commit() throws LoginException {
+        if (debug) {
+            LOG.debug("commit");
+        }
+
+        if (!succeeded) {
+            clear();
+            return false;
+        }
+
         principals.add(new UserPrincipal(username));
 
         for (String group : getUserGroups(username)) {
@@ -104,11 +117,8 @@ public abstract class CertificateLoginModule extends PropertiesLoader implements
 
         subject.getPrincipals().addAll(principals);
 
-        clear();
+        username = null;
-
+        commitSucceeded = true;
-        if (debug) {
-            LOG.debug("commit");
-        }
         return true;
     }
 
@@ -117,11 +127,19 @@ public abstract class CertificateLoginModule extends PropertiesLoader implements
      */
     @Override
     public boolean abort() throws LoginException {
-        clear();
-
         if (debug) {
             LOG.debug("abort");
         }
+        if (!succeeded) {
+            return false;
+        } else if (succeeded && commitSucceeded) {
+            // we succeeded, but another required module failed
+            logout();
+        } else {
+            // our commit failed
+            clear();
+            succeeded = false;
+        }
         return true;
     }
 
@@ -131,11 +149,14 @@ public abstract class CertificateLoginModule extends PropertiesLoader implements
     @Override
     public boolean logout() {
         subject.getPrincipals().removeAll(principals);
-        principals.clear();
+        clear();
 
         if (debug) {
             LOG.debug("logout");
         }
+
+        succeeded = false;
+        commitSucceeded = false;
         return true;
     }
 
@@ -143,8 +164,8 @@ public abstract class CertificateLoginModule extends PropertiesLoader implements
      * Helper method.
      */
     private void clear() {
-        certificates = null;
         username = null;
+        principals.clear();
     }
 
     /**

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

@@ -54,7 +54,10 @@ public class GuestLoginModule implements LoginModule {
     private boolean credentialsInvalidate;
     private Set<Principal> principals = new HashSet<Principal>();
     private CallbackHandler callbackHandler;
-    private boolean loginSucceeded;
+
+    /** the authentication status*/
+    private boolean succeeded = false;
+    private boolean commitSucceeded = false;
 
     @Override
     public void initialize(Subject subject, CallbackHandler callbackHandler, Map sharedState, Map options) {
@@ -79,7 +82,7 @@ public class GuestLoginModule implements LoginModule {
 
     @Override
     public boolean login() throws LoginException {
-        loginSucceeded = true;
+        succeeded = true;
         if (credentialsInvalidate) {
             PasswordCallback passwordCallback = new PasswordCallback("Password: ", false);
             try {
@@ -88,7 +91,7 @@ public class GuestLoginModule implements LoginModule {
                      if (debug) {
                         LOG.debug("Guest login failing (credentialsInvalidate=true) on presence of a password");
                      }
-                     loginSucceeded = false;
+                     succeeded = false;
                      passwordCallback.clearPassword();
                  };
              } catch (IOException ioe) {
@@ -96,21 +99,24 @@ public class GuestLoginModule implements LoginModule {
              }
         }
         if (debug) {
-            LOG.debug("Guest login " + loginSucceeded);
+            LOG.debug("Guest login " + succeeded);
         }
-        return loginSucceeded;
+        return succeeded;
     }
 
     @Override
     public boolean commit() throws LoginException {
-        if (loginSucceeded) {
-            subject.getPrincipals().addAll(principals);
-        }
-
         if (debug) {
             LOG.debug("commit");
         }
-        return loginSucceeded;
+
+        if (!succeeded) {
+            return false;
+        }
+
+        subject.getPrincipals().addAll(principals);
+        commitSucceeded = true;
+        return true;
     }
 
     @Override
@@ -119,6 +125,15 @@ public class GuestLoginModule implements LoginModule {
         if (debug) {
             LOG.debug("abort");
         }
+        if (!succeeded) {
+            return false;
+        } else if (succeeded && commitSucceeded) {
+            // we succeeded, but another required module failed
+            logout();
+        } else {
+            // our commit failed
+            succeeded = false;
+        }
         return true;
     }
 
@@ -129,6 +144,9 @@ public class GuestLoginModule implements LoginModule {
         if (debug) {
             LOG.debug("logout");
         }
+
+        succeeded = false;
+        commitSucceeded = false;
         return true;
     }
 }

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

@@ -72,9 +72,13 @@ public class LDAPLoginModule implements LoginModule {
     private Subject subject;
     private CallbackHandler handler;  
     private LDAPLoginProperty [] config;
-    private String username;
+    private Principal user;
     private Set<GroupPrincipal> groups = new HashSet<GroupPrincipal>();
 
+    /** the authentication status*/
+    private boolean succeeded = false;
+    private boolean commitSucceeded = false;
+
     @Override
     public void initialize(Subject subject, CallbackHandler callbackHandler, Map sharedState, Map options) {
         this.subject = subject;
@@ -118,7 +122,7 @@ public class LDAPLoginModule implements LoginModule {
         
         String password;
         
-        username = ((NameCallback)callbacks[0]).getName();
+        String username = ((NameCallback)callbacks[0]).getName();
         if (username == null)
         	return false;
         	
@@ -130,28 +134,56 @@ public class LDAPLoginModule implements LoginModule {
         // authenticate will throw LoginException
         // in case of failed authentication
         authenticate(username, password);
+
+        user = new UserPrincipal(username);
+        succeeded = true;
         return true;
     }
 
     @Override
     public boolean logout() throws LoginException {
-        username = null;
+        subject.getPrincipals().remove(user);
+        subject.getPrincipals().removeAll(groups);
+
+        user = null;
+        groups.clear();
+
+        succeeded = false;
+        commitSucceeded = false;
         return true;
     }
 
     @Override
     public boolean commit() throws LoginException {
+        if (!succeeded) {
+            user = null;
+            groups.clear();
+            return false;
+        }
+
         Set<Principal> principals = subject.getPrincipals();
-        principals.add(new UserPrincipal(username));
+        principals.add(user);
         for (GroupPrincipal gp : groups) {
             principals.add(gp);
         }
+
+        commitSucceeded = true;
         return true;
     }
 
     @Override
     public boolean abort() throws LoginException {
-        username = null;
+        if (!succeeded) {
+            return false;
+        } else if (succeeded && commitSucceeded) {
+            // we succeeded, but another required module failed
+            logout();
+        } else {
+            // our commit failed
+            user = null;
+            groups.clear();
+            succeeded = false;
+        }
         return true;
     }
 

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

@@ -50,13 +50,16 @@ public class PropertiesLoginModule extends PropertiesLoader implements LoginModu
     private Map<String,Set<String>> groups;
     private String user;
     private final Set<Principal> principals = new HashSet<Principal>();
-    private boolean loginSucceeded;
+
+    /** the authentication status*/
+    private boolean succeeded = false;
+    private boolean commitSucceeded = false;
 
     @Override
     public void initialize(Subject subject, CallbackHandler callbackHandler, Map sharedState, Map options) {
         this.subject = subject;
         this.callbackHandler = callbackHandler;
-        loginSucceeded = false;
+        succeeded = false;
         init(options);
         users = load(USER_FILE_PROP_NAME, "user", options).getProps();
         groups = load(GROUP_FILE_PROP_NAME, "group", options).invertedPropertiesValuesMap();
@@ -91,63 +94,77 @@ public class PropertiesLoginModule extends PropertiesLoader implements LoginModu
         if (!password.equals(new String(tmpPassword))) {
             throw new FailedLoginException("Password does not match");
         }
-        loginSucceeded = true;
+        succeeded = true;
 
         if (debug) {
             LOG.debug("login " + user);
         }
-        return loginSucceeded;
+        return succeeded;
     }
 
     @Override
     public boolean commit() throws LoginException {
-        boolean result = loginSucceeded;
+        if (!succeeded) {
-        if (result) {
+            clear();
-            principals.add(new UserPrincipal(user));
+            if (debug) {
-
+                LOG.debug("commit, result: false");
-            Set<String> matchedGroups = groups.get(user);
-            if (matchedGroups != null) {
-                for (String entry : matchedGroups) {
-                    principals.add(new GroupPrincipal(entry));
-                }
             }
-
+            return false;
-            subject.getPrincipals().addAll(principals);
         }
 
-        // will whack loginSucceeded
+        principals.add(new UserPrincipal(user));
-        clear();
+
+        Set<String> matchedGroups = groups.get(user);
+        if (matchedGroups != null) {
+            for (String entry : matchedGroups) {
+                principals.add(new GroupPrincipal(entry));
+            }
+        }
+
+        subject.getPrincipals().addAll(principals);
 
         if (debug) {
-            LOG.debug("commit, result: " + result);
+            LOG.debug("commit, result: true");
         }
-        return result;
+
+        commitSucceeded = true;
+        return true;
     }
 
     @Override
     public boolean abort() throws LoginException {
-        clear();
-
         if (debug) {
             LOG.debug("abort");
         }
+        if (!succeeded) {
+            return false;
+        } else if (succeeded && commitSucceeded) {
+            // we succeeded, but another required module failed
+            logout();
+        } else {
+            // our commit failed
+            clear();
+            succeeded = false;
+        }
         return true;
     }
 
     @Override
     public boolean logout() throws LoginException {
         subject.getPrincipals().removeAll(principals);
-        principals.clear();
         clear();
         if (debug) {
             LOG.debug("logout");
         }
+
+        succeeded = false;
+        commitSucceeded = false;
         return true;
     }
 
     private void clear() {
         user = null;
-        loginSucceeded = false;
+        principals.clear();
     }
 
 }