improve logging and small fixes for active directory authentication, see #28

plugins/scm-activedirectory-auth-plugin/src/main/java/sonia/scm/activedirectory/auth/ActiveDirectoryAuthenticationHandler.java

@@ -38,18 +38,14 @@ package sonia.scm.activedirectory.auth;
 import com.google.inject.Singleton;
 
 import com4j.COM4J;
-import com4j.Com4jObject;
 import com4j.ComException;
 import com4j.ExecutionException;
 import com4j.Variant;
 
 import com4j.typelibs.activeDirectory.IADs;
-import com4j.typelibs.activeDirectory.IADsGroup;
 import com4j.typelibs.activeDirectory.IADsOpenDSObject;
 import com4j.typelibs.activeDirectory.IADsUser;
 import com4j.typelibs.ado20.ClassFactory;
-import com4j.typelibs.ado20.Field;
-import com4j.typelibs.ado20.Fields;
 import com4j.typelibs.ado20._Command;
 import com4j.typelibs.ado20._Connection;
 import com4j.typelibs.ado20._Recordset;
@@ -70,11 +66,8 @@ import sonia.scm.web.security.AuthenticationResult;
 
 import java.io.IOException;
 
-import java.util.Collection;
 import java.util.HashMap;
 import java.util.Map;
-import java.util.Set;
-import java.util.TreeSet;
 
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
@@ -177,7 +170,13 @@ public class ActiveDirectoryAuthenticationHandler
       IADs rootDSE = COM4J.getObject(IADs.class, "LDAP://RootDSE", null);
 
       defaultNamingContext = (String) rootDSE.get("defaultNamingContext");
-      logger.info("Active Directory domain is " + defaultNamingContext);
+
+      if (logger.isInfoEnabled())
+      {
+        logger.info("Active Directory default domain is {}",
+                    defaultNamingContext);
+      }
+
       con = ClassFactory.createConnection();
       con.provider("ADsDSOObject");
       con.open("Active Directory Provider", "" /* default */, "" /* default */,
@@ -218,22 +217,30 @@ public class ActiveDirectoryAuthenticationHandler
   protected String getDnOfUserOrGroup(String domainContext,
           String userOrGroupname)
   {
-    String dn;
+    String dn = null;
-    _Command cmd = ClassFactory.createCommand();
 
-    cmd.activeConnection(con);
+    try
-    cmd.commandText("<LDAP://" + domainContext + ">;(sAMAccountName="
-                    + userOrGroupname + ");distinguishedName;subTree");
-
-    _Recordset rs = cmd.execute(null, Variant.MISSING, -1 /* default */);
-
-    if (!rs.eof())
     {
-      dn = getString(rs, "distinguishedName");
+      _Command cmd = ClassFactory.createCommand();
+
+      cmd.activeConnection(con);
+      cmd.commandText("<LDAP://" + domainContext + ">;(sAMAccountName="
+                      + userOrGroupname + ");distinguishedName;subTree");
+
+      _Recordset rs = cmd.execute(null, Variant.MISSING, -1 /* default */);
+
+      if (!rs.eof())
+      {
+        dn = ActiveDirectoryUtil.getString(rs, "distinguishedName");
+      }
+      else
+      {
+        dn = null;    // No such user or group
+      }
     }
-    else
+    catch (ComException ex)
     {
-      dn = null;    // No such user or group
+      logger.error("could not search for dn", ex);
     }
 
     return dn;
@@ -252,35 +259,34 @@ public class ActiveDirectoryAuthenticationHandler
    */
   private AuthenticationResult authenticate(String username, String password)
   {
+    AuthenticationResult result = AuthenticationResult.NOT_FOUND;
+
     if (!canDoNativeAuth())
     {
-      return null;
+      return result;
     }
 
     if (con == null)
     {
-      return null;
+      return result;
     }
 
-    AuthenticationResult result;
+    String host = Util.EMPTY_STRING;
-    String host = "";
-    String internalName = username;
     String domainContext = defaultNamingContext;
     int index = username.indexOf("\\");
+    ActiveDirectoryDomain add = null;
 
     if (index > 0)
     {
       String domain = username.substring(0, index);
 
       username = username.substring(index + 1);
-      internalName = domain.toLowerCase().concat("/").concat(username);
+      add = domainMap.get(domain.toUpperCase());
 
-      ActiveDirectoryDomain d = domainMap.get(domain.toUpperCase());
+      if (add != null)
-
-      if (d != null)
       {
-        domainContext = d.getDomainContext();
+        domainContext = add.getDomainContext();
-        host = Util.nonNull(d.getHost());
+        host = Util.nonNull(add.getHost());
       }
       else if (logger.isWarnEnabled())
       {
@@ -296,10 +302,38 @@ public class ActiveDirectoryAuthenticationHandler
 
     String dn = getDnOfUserOrGroup(domainContext, username);
 
-    if (logger.isDebugEnabled())
+    if (Util.isNotEmpty(dn))
     {
-      logger.debug("found user at {}", dn);
+      if (logger.isDebugEnabled())
+      {
+        logger.debug("found user at {}", dn);
+      }
+
+      result = authenticate(add, host, dn, password);
     }
+    else if (logger.isDebugEnabled())
+    {
+      logger.debug("could not find dn of user {}", username);
+    }
+
+    return result;
+  }
+
+  /**
+   * Method description
+   *
+   *
+   * @param add
+   * @param host
+   * @param dn
+   * @param password
+   *
+   * @return
+   */
+  private AuthenticationResult authenticate(ActiveDirectoryDomain add,
+          String host, String dn, String password)
+  {
+    AuthenticationResult result = AuthenticationResult.FAILED;
 
     // now we got the DN of the user
     IADsOpenDSObject dso = COM4J.getObject(IADsOpenDSObject.class, "LDAP:",
@@ -307,43 +341,91 @@ public class ActiveDirectoryAuthenticationHandler
 
     try
     {
-      if (Util.isNotEmpty(host))
+      String ldapUrl = createLdapUrl(host, dn);
+
+      if (logger.isDebugEnabled())
       {
-        host = host.concat("/");
+        logger.debug("use {} ldap url for authentication", ldapUrl);
       }
 
-      IADsUser usr = dso.openDSObject("LDAP://".concat(host).concat(dn), dn,
+      IADsUser usr = dso.openDSObject(ldapUrl, dn, password,
-                                      password,
                                       0).queryInterface(IADsUser.class);
 
       if (usr != null)
       {
+        String name = getName(add, usr);
+
         if (!usr.accountDisabled())
         {
-          User user = new User(internalName, usr.fullName(),
+          if (logger.isDebugEnabled())
-                               usr.emailAddress());
+          {
+            logger.debug("ad user \"{}\" successfully authenticated", name);
+          }
+
+          User user = new User(name, usr.fullName(), usr.emailAddress());
 
           user.setType(TYPE);
-          result = new AuthenticationResult(user, getGroups(usr));
+          result = new AuthenticationResult(user,
+                                            ActiveDirectoryUtil.getGroups(usr));
         }
         else
-        {    // Account disabled
+        {
+          if (logger.isWarnEnabled())
+          {
+            logger.warn("ad user \"{}\" is disabled", name);
+          }
+
+          // Account disabled
           result = AuthenticationResult.FAILED;
         }
       }
       else
-      {      // the user name was in fact a group
+      {
+        if (logger.isWarnEnabled())
+        {
+          logger.warn("could not authenticate ad user with dn \"{}\"", dn);
+        }
+
+        // the user name was in fact a group
         result = AuthenticationResult.NOT_FOUND;
       }
     }
     catch (ComException e)
     {
+      if (logger.isTraceEnabled())
+      {
+        logger.trace("ad authentication failed", e);
+      }
+
       result = AuthenticationResult.FAILED;
     }
 
     return result;
   }
 
+  /**
+   * Method description
+   *
+   *
+   * @param host
+   * @param dn
+   *
+   * @return
+   */
+  private String createLdapUrl(String host, String dn)
+  {
+    String ldapUrl = "LDAP://";
+
+    if (Util.isNotEmpty(host))
+    {
+      ldapUrl = ldapUrl.concat(host).concat("/");
+    }
+
+    ldapUrl = ldapUrl.concat(dn);
+
+    return ldapUrl;
+  }
+
   /**
    * Method description
    *
@@ -368,9 +450,9 @@ public class ActiveDirectoryAuthenticationHandler
 
         while (!rs.eof())
         {
-          String cn = getString(rs, "cn");
+          String cn = ActiveDirectoryUtil.getString(rs, "cn");
-          String dn = getString(rs, "ncname");
+          String dn = ActiveDirectoryUtil.getString(rs, "ncname");
-          String host = getFirstString(rs, "dnsRoot");
+          String host = ActiveDirectoryUtil.getFirstString(rs, "dnsRoot");
 
           if (Util.isNotEmpty(cn) && Util.isNotEmpty(dn))
           {
@@ -400,104 +482,28 @@ public class ActiveDirectoryAuthenticationHandler
   //~--- get methods ----------------------------------------------------------
 
   /**
-   * Get the first String of a multivalue recordset item
+   * Method description
-   *
-   *
-   * @param rs
-   * @param item
-   *
-   * @return the first item of a recordset
-   */
-  private String getFirstString(_Recordset rs, String item)
-  {
-    String result = null;
-    Object[] values = (Object[]) getObject(rs, item);
-
-    if (Util.isNotEmpty(values))
-    {
-      Object value = values[0];
-
-      if (value != null)
-      {
-        result = value.toString();
-      }
-    }
-
-    return result;
-  }
-
-  /**
-   * Fetch all groupnames of a user
    *
    *
+   * @param add
    * @param usr
    *
    * @return
    */
-  private Collection<String> getGroups(IADsUser usr)
+  private String getName(ActiveDirectoryDomain add, IADsUser usr)
   {
-    Set<String> groups = new TreeSet<String>();
+    Object nameObj = usr.get("sAMAccountName");
 
-    for (Com4jObject g : usr.groups())
+    AssertUtil.assertIsNotNull(nameObj);
+
+    String name = nameObj.toString();
+
+    if ((add != null) && Util.isNotEmpty(add.getDomain()))
     {
-      IADsGroup grp = g.queryInterface(IADsGroup.class);
+      name = add.getDomain().concat("\\").concat(name);
-
-      // cut "CN=" and make that the role name
-      String groupName = grp.name().substring(3);
-
-      groups.add(groupName);
     }
 
-    return groups;
+    return name;
-  }
-
-  /**
-   * Method description
-   *
-   *
-   * @param rs
-   * @param item
-   *
-   * @return
-   */
-  private Object getObject(_Recordset rs, String item)
-  {
-    Object value = null;
-    Fields fields = rs.fields();
-
-    if (fields != null)
-    {
-      Field field = fields.item(item);
-
-      if (field != null)
-      {
-        value = field.value();
-      }
-    }
-
-    return value;
-  }
-
-  /**
-   * Method description
-   *
-   *
-   * @param rs
-   * @param item
-   *
-   * @return
-   */
-  private String getString(_Recordset rs, String item)
-  {
-    String result = null;
-    Object value = getObject(rs, item);
-
-    if (value != null)
-    {
-      result = value.toString();
-    }
-
-    return result;
   }
 
   //~--- fields ---------------------------------------------------------------

plugins/scm-activedirectory-auth-plugin/src/main/java/sonia/scm/activedirectory/auth/ActiveDirectoryUtil.java

@@ -0,0 +1,161 @@
+/**
+ * Copyright (c) 2010, Sebastian Sdorra
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions are met:
+ *
+ * 1. Redistributions of source code must retain the above copyright notice,
+ *    this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright notice,
+ *    this list of conditions and the following disclaimer in the documentation
+ *    and/or other materials provided with the distribution.
+ * 3. Neither the name of SCM-Manager; nor the names of its
+ *    contributors may be used to endorse or promote products derived from this
+ *    software without specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
+ * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+ * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
+ * DISCLAIMED.  IN NO EVENT SHALL THE REGENTS OR CONTRIBUTORS BE LIABLE FOR ANY
+ * DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
+ * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
+ * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON
+ * ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
+ * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ *
+ * http://bitbucket.org/sdorra/scm-manager
+ *
+ */
+
+
+
+package sonia.scm.activedirectory.auth;
+
+//~--- non-JDK imports --------------------------------------------------------
+
+import com4j.Com4jObject;
+
+import com4j.typelibs.activeDirectory.IADsGroup;
+import com4j.typelibs.activeDirectory.IADsUser;
+import com4j.typelibs.ado20.Field;
+import com4j.typelibs.ado20.Fields;
+import com4j.typelibs.ado20._Recordset;
+
+import sonia.scm.util.Util;
+
+//~--- JDK imports ------------------------------------------------------------
+
+import java.util.Collection;
+import java.util.Set;
+import java.util.TreeSet;
+
+/**
+ *
+ * @author sdorra
+ */
+public class ActiveDirectoryUtil
+{
+
+  /**
+   * Get the first String of a multivalue recordset item
+   *
+   *
+   * @param rs
+   * @param item
+   *
+   * @return the first item of a recordset
+   */
+  public static String getFirstString(_Recordset rs, String item)
+  {
+    String result = null;
+    Object[] values = (Object[]) getObject(rs, item);
+
+    if (Util.isNotEmpty(values))
+    {
+      Object value = values[0];
+
+      if (value != null)
+      {
+        result = value.toString();
+      }
+    }
+
+    return result;
+  }
+
+  /**
+   * Fetch all groupnames of a user
+   *
+   *
+   * @param usr
+   *
+   * @return
+   */
+  public static Collection<String> getGroups(IADsUser usr)
+  {
+    Set<String> groups = new TreeSet<String>();
+
+    for (Com4jObject g : usr.groups())
+    {
+      IADsGroup grp = g.queryInterface(IADsGroup.class);
+
+      // cut "CN=" and make that the role name
+      String groupName = grp.name().substring(3);
+
+      groups.add(groupName);
+    }
+
+    return groups;
+  }
+
+  /**
+   * Method description
+   *
+   *
+   * @param rs
+   * @param item
+   *
+   * @return
+   */
+  public static Object getObject(_Recordset rs, String item)
+  {
+    Object value = null;
+    Fields fields = rs.fields();
+
+    if (fields != null)
+    {
+      Field field = fields.item(item);
+
+      if (field != null)
+      {
+        value = field.value();
+      }
+    }
+
+    return value;
+  }
+
+  /**
+   * Method description
+   *
+   *
+   * @param rs
+   * @param item
+   *
+   * @return
+   */
+  public static String getString(_Recordset rs, String item)
+  {
+    String result = null;
+    Object value = getObject(rs, item);
+
+    if (value != null)
+    {
+      result = value.toString();
+    }
+
+    return result;
+  }
+}

scm-core/src/main/java/sonia/scm/user/UserAllreadyExistException.java

@@ -33,6 +33,10 @@
 
 package sonia.scm.user;
 
+//~--- non-JDK imports --------------------------------------------------------
+
+import sonia.scm.util.Util;
+
 /**
  *
  * @author Sebastian Sdorra
@@ -42,4 +46,25 @@ public class UserAllreadyExistException extends UserException
 
   /** Field description */
   private static final long serialVersionUID = 9182294539718090814L;
+
+  //~--- constructors ---------------------------------------------------------
+
+  /**
+   * Constructs ...
+   *
+   */
+  public UserAllreadyExistException() {}
+
+  /**
+   * Constructs ...
+   *
+   *
+   * @param username
+   * @since 1.5
+   */
+  public UserAllreadyExistException(String username)
+  {
+    super("user \"".concat(Util.nonNull(username)).concat(
+      "\" allready exists"));
+  }
 }

scm-webapp/src/main/java/sonia/scm/user/xml/XmlUserManager.java

@@ -35,7 +35,6 @@ package sonia.scm.user.xml;
 
 //~--- non-JDK imports --------------------------------------------------------
 
-import com.google.common.collect.Collections2;
 import com.google.inject.Inject;
 import com.google.inject.Provider;
 import com.google.inject.Singleton;
@@ -71,8 +70,6 @@ import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.Comparator;
-import java.util.Iterator;
-import java.util.LinkedList;
 import java.util.List;
 
 import javax.xml.bind.JAXBContext;
@@ -176,7 +173,7 @@ public class XmlUserManager extends AbstractUserManager
 
     if (userDB.contains(user.getName()))
     {
-      throw new UserAllreadyExistException();
+      throw new UserAllreadyExistException(user.getName());
     }
 
     String type = user.getType();

scm-webapp/src/main/java/sonia/scm/web/security/BasicSecurityContext.java

@@ -158,7 +158,7 @@ public class BasicSecurityContext implements WebSecurityContext
       catch (Exception ex)
       {
         user = null;
-        logger.error(ex.getMessage(), ex);
+        logger.error("authentication failed", ex);
       }
     }