Fixed ActiveDirectory LDAP group support. Allows recursive group searching.

Issue: 283
Change-Id: I3196a8e9f5c08dedccd05d2de10c55042933e427

3 files changed, 84 insertions, 14 deletions

diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt
index 487a9fc78a..b17b8a2c30 100644
--- a/Documentation/config-gerrit.txt
+++ b/Documentation/config-gerrit.txt
@@ -599,6 +599,14 @@ Gerrit will populate it only from the LDAP data.
 +
 Default is `uid`, a common value for most servers.
 
+[[ldap.accountMemberField]ldap.accountMemberField::
++
+_(Optional)_ Name of an attribute on the user account object which
+contains the groups the user is part of. Typically used for ActiveDirectory
+servers.
++
+Default is `memberOf`, to correspond with ActiveDirectory's default.
+
 [[ldap.groupBase]]ldap.groupBase::
 +
 Root of the tree containing all group objects.  This is typically
diff --git a/src/main/java/com/google/gerrit/server/ldap/LdapQuery.java b/src/main/java/com/google/gerrit/server/ldap/LdapQuery.java
index 4951c14bf2..fcb0ed4748 100644
--- a/src/main/java/com/google/gerrit/server/ldap/LdapQuery.java
+++ b/src/main/java/com/google/gerrit/server/ldap/LdapQuery.java
@@ -26,6 +26,7 @@ import java.util.Set;
 import javax.naming.NamingEnumeration;
 import javax.naming.NamingException;
 import javax.naming.directory.Attribute;
+import javax.naming.directory.BasicAttribute;
 import javax.naming.directory.DirContext;
 import javax.naming.directory.SearchControls;
 import javax.naming.directory.SearchResult;
@@ -78,45 +79,55 @@ class LdapQuery {
   }
 
   class Result {
-    private final String dn;
-    private final Map<String, String> atts = new HashMap<String, String>();
+    private final Map<String, Attribute> atts = new HashMap<String, Attribute>();
+    private final Map<String, String> attsSingle = new HashMap<String, String>();
 
     Result(final SearchResult sr) throws NamingException {
-      dn = sr.getNameInNamespace();
       if (returnAttributes != null) {
         for (final String attName : returnAttributes) {
           final Attribute a = sr.getAttributes().get(attName);
           if (a != null && a.size() > 0) {
-            atts.put(attName, String.valueOf(a.get(0)));
+            atts.put(attName, a);
+            attsSingle.put(attName, String.valueOf(a.get(0)));
           }
         }
       } else {
         NamingEnumeration<? extends Attribute> e = sr.getAttributes().getAll();
         while (e.hasMoreElements()) {
           final Attribute a = e.nextElement();
+          atts.put(a.getID(), a);
           if (a.size() == 1) {
-            atts.put(a.getID(), String.valueOf(a.get(0)));
+            attsSingle.put(a.getID(), String.valueOf(a.get(0)));
           }
         }
       }
-      atts.put("dn", dn);
+      atts.put("dn", new BasicAttribute("dn", sr.getNameInNamespace()));
+      attsSingle.put("dn", sr.getNameInNamespace());
     }
 
-    String getDN() {
-      return dn;
+    String getDN() throws NamingException {
+      return get("dn");
     }
 
-    String get(final String attName) {
+    String get(final String attName) throws NamingException {
+      return String.valueOf(atts.get(attName).get(0));
+    }
+
+    Attribute getAll(final String attName) {
       return atts.get(attName);
     }
 
     Map<String, String> map() {
-      return Collections.unmodifiableMap(atts);
+      return Collections.unmodifiableMap(attsSingle);
     }
 
     @Override
     public String toString() {
-      return getDN();
+      try {
+          return getDN();
+      } catch (NamingException e) {
+          return "";
+      }
     }
   }
 }
diff --git a/src/main/java/com/google/gerrit/server/ldap/LdapRealm.java b/src/main/java/com/google/gerrit/server/ldap/LdapRealm.java
index 8d5af24357..5f66f0bdd1 100644
--- a/src/main/java/com/google/gerrit/server/ldap/LdapRealm.java
+++ b/src/main/java/com/google/gerrit/server/ldap/LdapRealm.java
@@ -56,8 +56,11 @@ import java.util.Set;
 
 import javax.naming.Context;
 import javax.naming.NamingException;
+import javax.naming.NamingEnumeration;
 import javax.naming.directory.DirContext;
 import javax.naming.directory.InitialDirContext;
+import javax.naming.directory.Attribute;
+import javax.naming.directory.Attributes;
 
 @Singleton
 class LdapRealm implements Realm {
@@ -76,6 +79,7 @@ class LdapRealm implements Realm {
   private final ParamertizedString accountFullName;
   private final ParamertizedString accountEmailAddress;
   private final ParamertizedString accountSshUserName;
+  private final String accountMemberField;
   private final List<LdapQuery> accountQueryList;
   private final SelfPopulatingCache<String, Account.Id> usernameCache;
 
@@ -157,6 +161,10 @@ class LdapRealm implements Realm {
     if (accountSshUserName != null) {
       accountAtts.addAll(accountSshUserName.getParameterNames());
     }
+    accountMemberField = reqdef(config, "accountMemberField", "memberOf");
+    if (accountMemberField != null) {
+      accountAtts.add(accountMemberField);
+    }
     for (final String name : groupMemberQueryList.get(0).getParameters()) {
       if (!USERNAME.equals(name)) {
         groupNeedsAccount = true;
@@ -355,12 +363,13 @@ class LdapRealm implements Realm {
   private Set<AccountGroup.Id> queryForGroups(final DirContext ctx,
       final String username, LdapQuery.Result account) throws NamingException,
       AccountException {
+    if (account == null) {
+      account = findAccount(ctx, username);
+    }
+
     final HashMap<String, String> params = new HashMap<String, String>();
     params.put(USERNAME, username);
     if (groupNeedsAccount) {
-      if (account == null) {
-        account = findAccount(ctx, username);
-      }
       for (final String name : groupMemberQueryList.get(0).getParameters()) {
         params.put(name, account.get(name));
       }
@@ -376,6 +385,17 @@ class LdapRealm implements Realm {
         }
       }
     }
+    NamingEnumeration groups = account.getAll(accountMemberField).getAll();
+    while (groups.hasMore()) {
+       final String dn = (String) groups.next();
+
+       for (String cn : groupsFor(ctx, dn)) {
+           AccountGroup group = groupCache.lookup(cn);
+           if (null != group && isLdapGroup(group)) {
+               actual.add(group.getId());
+           }
+       }
+    }
     if (actual.isEmpty()) {
       return Collections.emptySet();
     } else {
@@ -383,6 +403,37 @@ class LdapRealm implements Realm {
     }
   }
 
+  /*
+   * We store the group names because LDAP doesn't support a query
+   * that will tell us whether a user is part of a parent group
+   */
+  private ArrayList<String> groupsFor(DirContext ctx, String dn)
+    throws NamingException {
+      ArrayList<String> out = new ArrayList<String>();
+      String parentDn;
+
+      try {
+          // add the current item's name
+          String attNames[] = { groupName, accountMemberField };
+          Attributes groupAtts = ctx.getAttributes(dn, attNames);
+          String cn = (String) groupAtts.get(groupName).get();
+          out.add(cn);
+
+          // get its parents
+          Attribute parentAttrs = ctx.getAttributes(dn).get(accountMemberField);
+          if (parentAttrs != null) {
+              NamingEnumeration parents = parentAttrs.getAll();
+              while (parents.hasMore()) {
+                  parentDn = String.valueOf(parents.next());
+                  out.addAll(groupsFor(ctx, parentDn));
+              }
+          }
+      } catch (NamingException e) {
+          log.warn("Could not find dn", e);
+      }
+      return out;
+  }
+
   private boolean isLdapGroup(final AccountGroup group) {
     return group.isAutomaticMembership()
         && !authConfig.getRegisteredGroups().contains(group.getId());