From 0d2ecb8f0e3036ce0a59e6ab52ef49d74abf9198 Mon Sep 17 00:00:00 2001 From: Sebastian Sdorra Date: Thu, 23 Jun 2011 09:37:55 +0200 Subject: [PATCH] improve logging and small fixes for active directory authentication, see #28 --- .../ActiveDirectoryAuthenticationHandler.java | 270 +++++++++--------- .../auth/ActiveDirectoryUtil.java | 161 +++++++++++ .../scm/user/UserAllreadyExistException.java | 25 ++ .../sonia/scm/user/xml/XmlUserManager.java | 5 +- .../web/security/BasicSecurityContext.java | 2 +- 5 files changed, 326 insertions(+), 137 deletions(-) create mode 100644 plugins/scm-activedirectory-auth-plugin/src/main/java/sonia/scm/activedirectory/auth/ActiveDirectoryUtil.java diff --git a/plugins/scm-activedirectory-auth-plugin/src/main/java/sonia/scm/activedirectory/auth/ActiveDirectoryAuthenticationHandler.java b/plugins/scm-activedirectory-auth-plugin/src/main/java/sonia/scm/activedirectory/auth/ActiveDirectoryAuthenticationHandler.java index bc4cadf48e..8c0a715013 100644 --- a/plugins/scm-activedirectory-auth-plugin/src/main/java/sonia/scm/activedirectory/auth/ActiveDirectoryAuthenticationHandler.java +++ b/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; - _Command cmd = ClassFactory.createCommand(); + String dn = null; - cmd.activeConnection(con); - cmd.commandText(";(sAMAccountName=" - + userOrGroupname + ");distinguishedName;subTree"); - - _Recordset rs = cmd.execute(null, Variant.MISSING, -1 /* default */); - - if (!rs.eof()) + try { - dn = getString(rs, "distinguishedName"); + _Command cmd = ClassFactory.createCommand(); + + cmd.activeConnection(con); + cmd.commandText(";(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 = ""; - String internalName = username; + String host = Util.EMPTY_STRING; 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 (d != null) + if (add != null) { - domainContext = d.getDomainContext(); - host = Util.nonNull(d.getHost()); + domainContext = add.getDomainContext(); + 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, - password, + IADsUser usr = dso.openDSObject(ldapUrl, dn, password, 0).queryInterface(IADsUser.class); if (usr != null) { + String name = getName(add, usr); + if (!usr.accountDisabled()) { - User user = new User(internalName, usr.fullName(), - usr.emailAddress()); + if (logger.isDebugEnabled()) + { + 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 dn = getString(rs, "ncname"); - String host = getFirstString(rs, "dnsRoot"); + String cn = ActiveDirectoryUtil.getString(rs, "cn"); + String dn = ActiveDirectoryUtil.getString(rs, "ncname"); + 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 - * - * - * @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 + * Method description * * + * @param add * @param usr * * @return */ - private Collection getGroups(IADsUser usr) + private String getName(ActiveDirectoryDomain add, IADsUser usr) { - Set groups = new TreeSet(); + 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); - - // cut "CN=" and make that the role name - String groupName = grp.name().substring(3); - - groups.add(groupName); + name = add.getDomain().concat("\\").concat(name); } - return groups; - } - - /** - * 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; + return name; } //~--- fields --------------------------------------------------------------- diff --git a/plugins/scm-activedirectory-auth-plugin/src/main/java/sonia/scm/activedirectory/auth/ActiveDirectoryUtil.java b/plugins/scm-activedirectory-auth-plugin/src/main/java/sonia/scm/activedirectory/auth/ActiveDirectoryUtil.java new file mode 100644 index 0000000000..6b607861c3 --- /dev/null +++ b/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 getGroups(IADsUser usr) + { + Set groups = new TreeSet(); + + 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; + } +} diff --git a/scm-core/src/main/java/sonia/scm/user/UserAllreadyExistException.java b/scm-core/src/main/java/sonia/scm/user/UserAllreadyExistException.java index 67f14afa97..a8e847b9a8 100644 --- a/scm-core/src/main/java/sonia/scm/user/UserAllreadyExistException.java +++ b/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")); + } } diff --git a/scm-webapp/src/main/java/sonia/scm/user/xml/XmlUserManager.java b/scm-webapp/src/main/java/sonia/scm/user/xml/XmlUserManager.java index ee71d3fa8f..8531dbde6b 100644 --- a/scm-webapp/src/main/java/sonia/scm/user/xml/XmlUserManager.java +++ b/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(); diff --git a/scm-webapp/src/main/java/sonia/scm/web/security/BasicSecurityContext.java b/scm-webapp/src/main/java/sonia/scm/web/security/BasicSecurityContext.java index b2078e7160..733eefe0f7 100644 --- a/scm-webapp/src/main/java/sonia/scm/web/security/BasicSecurityContext.java +++ b/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); } }