From 50457503f2c5fd9bf6f5129056ef0cf99af6efcb Mon Sep 17 00:00:00 2001
From: Scott Dial <geekmug@gmail.com>
Date: Sun, 11 Aug 2013 16:52:51 -0400
Subject: [PATCH] Expand capabilities of ldap.groupMemberPattern

Previously, the pattern supported a limited set of variables that were
either accidentially available (due to their use in other queries) or
hard-coded (e.g., `username` is a special-case that was added).
Furthermore, the documentation made reference to being able to use
variables such as `${uidNumber}` even though they are not actually
supported (since `uidNumber` is normally never queried). Under the
default RFC 2307 configuration of LDAP, the only variables available
were `displayName, `mail`, `uid`, and `username` (It's noteworthy
that `username` was added as a special-case due to the default
`groupMemberPattern` containing `${username}` even though `username`
is substitued by Gerrit and not LDAP).

This changeset removes the artificial restrictions on the attributes
used in the `groupMemberPattern`. Any variable is assumed to
originate from the account, but `username` is still overridden and
provided by Gerrit (as before). This allows more expressive patterns,
which allows us to fix an outstanding bug in group matching. Prevously,
a user whose `gidNumber` matched the group's `gidNumber` would not have
been included in the group. This changeset updates the default
`groupMemberPattern` to account for this issue by adding the additional
case of `(gidNumber=${gidNumber}`.

Bug: Issue 2054
Change-Id: Iff3a14c569a10c1ef693b672f4710fb6f2f8d9a6
---
 Documentation/config-gerrit.txt                |  4 ++--
 .../google/gerrit/server/auth/ldap/Helper.java | 18 ++++++------------
 .../gerrit/server/auth/ldap/LdapType.java      |  2 +-
 3 files changed, 9 insertions(+), 15 deletions(-)

diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt
index a57c8a7751..ce42fe663f 100644
--- a/Documentation/config-gerrit.txt
+++ b/Documentation/config-gerrit.txt
@@ -1975,8 +1975,8 @@ corresponding attribute (in this case, `fooBarAttribute`) as read
 from the user's account object matched under `ldap.accountBase`.
 Attributes such as `${dn}` or `${uidNumber}` may be useful.
 +
-Default is `(memberUid=${username})` for RFC 2307,
-and unset (disabled) for Active Directory.
+Default is `(|(memberUid=${username})(gidNumber=${gidNumber}))` for
+RFC 2307, and unset (disabled) for Active Directory.
 
 [[ldap.groupName]]ldap.groupName::
 +
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/auth/ldap/Helper.java b/gerrit-server/src/main/java/com/google/gerrit/server/auth/ldap/Helper.java
index 0151dde40f..7d0ad24325 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/auth/ldap/Helper.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/auth/ldap/Helper.java
@@ -197,13 +197,11 @@ import javax.security.auth.login.LoginException;
     if (!schema.groupMemberQueryList.isEmpty()) {
       final HashMap<String, String> params = new HashMap<String, String>();
 
-      if (schema.groupNeedsAccount) {
-        if (account == null) {
-          account = findAccount(schema, ctx, username);
-        }
-        for (String name : schema.groupMemberQueryList.get(0).getParameters()) {
-          params.put(name, account.get(name));
-        }
+      if (account == null) {
+        account = findAccount(schema, ctx, username);
+      }
+      for (String name : schema.groupMemberQueryList.get(0).getParameters()) {
+        params.put(name, account.get(name));
       }
 
       params.put(LdapRealm.USERNAME, username);
@@ -286,7 +284,6 @@ import javax.security.auth.login.LoginException;
     final String accountMemberField;
     final List<LdapQuery> accountQueryList;
 
-    boolean groupNeedsAccount;
     final List<String> groupBases;
     final SearchScope groupScope;
     final ParameterizedString groupPattern;
@@ -321,10 +318,7 @@ import javax.security.auth.login.LoginException;
           }
 
           for (final String name : groupMemberQuery.getParameters()) {
-            if (!LdapRealm.USERNAME.equals(name)) {
-              groupNeedsAccount = true;
-              accountAtts.add(name);
-            }
+            accountAtts.add(name);
           }
 
           groupMemberQueryList.add(groupMemberQuery);
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/auth/ldap/LdapType.java b/gerrit-server/src/main/java/com/google/gerrit/server/auth/ldap/LdapType.java
index db5baebfc0..3c1b0d22b3 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/auth/ldap/LdapType.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/auth/ldap/LdapType.java
@@ -57,7 +57,7 @@ abstract class LdapType {
 
     @Override
     String groupMemberPattern() {
-      return "(memberUid=${username})";
+      return "(|(memberUid=${username})(gidNumber=${gidNumber}))";
     }
 
     @Override