Merge "Configurable ldap.fetchMemberOfEagerly to optimize LDAP login"

Documentation/config-gerrit.txt

@@ -2390,6 +2390,16 @@ Directory servers.
 Default is unset for RFC 2307 servers (disabled)
 and `memberOf` for Active Directory.
 
+[[ldap.fetchMemberOfEagerly]]ldap.fetchMemberOfEagerly::
++
+_(Optional)_ Whether to fetch the `memberOf` account attribute on
+login. Setups which use LDAP for user authentication but don't make
+use of the LDAP groups may benefit from setting this option to `false`
+as this will result in a much faster LDAP login.
++
+Default is unset for RFC 2307 servers (disabled) and `true` for
+Active Directory.
+
 [[ldap.groupBase]]ldap.groupBase::
 +
 Root of the tree containing all group objects.  This is typically

gerrit-server/src/main/java/com/google/gerrit/server/auth/ldap/Helper.java

@@ -185,13 +185,20 @@ import javax.security.auth.login.LoginException;
     return ldapSchema;
   }
 
-  LdapQuery.Result findAccount(final Helper.LdapSchema schema,
+  LdapQuery.Result findAccount(Helper.LdapSchema schema,
-      final DirContext ctx, final String username) throws NamingException,
+      DirContext ctx, String username, boolean fetchMemberOf)
-      AccountException {
+      throws NamingException, AccountException {
     final HashMap<String, String> params = new HashMap<>();
     params.put(LdapRealm.USERNAME, username);
 
-    for (LdapQuery accountQuery : schema.accountQueryList) {
+    List<LdapQuery> accountQueryList;
+    if (fetchMemberOf) {
+      accountQueryList = schema.accountWithMemberOfQueryList;
+    } else {
+      accountQueryList = schema.accountQueryList;
+    }
+
+    for (LdapQuery accountQuery : accountQueryList) {
       List<LdapQuery.Result> res = accountQuery.query(ctx, params);
       if (res.size() == 1) {
         return res.get(0);
@@ -213,7 +220,7 @@ import javax.security.auth.login.LoginException;
 
       if (account == null) {
         try {
-          account = findAccount(schema, ctx, username);
+          account = findAccount(schema, ctx, username, false);
         } catch (AccountException e) {
           LdapRealm.log.warn("Account " + username +
               " not found, assuming empty group membership");
@@ -234,9 +241,9 @@ import javax.security.auth.login.LoginException;
     }
 
     if (schema.accountMemberField != null) {
-      if (account == null) {
+      if (account == null || account.getAll(schema.accountMemberField) == null) {
         try {
-          account = findAccount(schema, ctx, username);
+          account = findAccount(schema, ctx, username, true);
         } catch (AccountException e) {
           LdapRealm.log.warn("Account " + username +
               " not found, assuming empty group membership");
@@ -311,6 +318,7 @@ import javax.security.auth.login.LoginException;
     final String accountMemberField;
     final String[] accountMemberFieldArray;
     final List<LdapQuery> accountQueryList;
+    final List<LdapQuery> accountWithMemberOfQueryList;
 
     final List<String> groupBases;
     final SearchScope groupScope;
@@ -322,6 +330,7 @@ import javax.security.auth.login.LoginException;
       type = discoverLdapType(ctx);
       groupMemberQueryList = new ArrayList<>();
       accountQueryList = new ArrayList<>();
+      accountWithMemberOfQueryList = new ArrayList<>();
 
       final Set<String> accountAtts = new HashSet<>();
 
@@ -375,7 +384,6 @@ import javax.security.auth.login.LoginException;
           LdapRealm.optdef(config, "accountMemberField", type.accountMemberField());
       if (accountMemberField != null) {
         accountMemberFieldArray = new String[] {accountMemberField};
-        accountAtts.add(accountMemberField);
       } else {
         accountMemberFieldArray = null;
       }
@@ -384,8 +392,15 @@ import javax.security.auth.login.LoginException;
       final String accountPattern =
           LdapRealm.reqdef(config, "accountPattern", type.accountPattern());
 
+      Set<String> accountWithMemberOfAtts;
+      if (accountMemberField != null) {
+        accountWithMemberOfAtts = new HashSet<>(accountAtts);
+        accountWithMemberOfAtts.add(accountMemberField);
+      } else {
+        accountWithMemberOfAtts = null;
+      }
       for (String accountBase : LdapRealm.requiredList(config, "accountBase")) {
-        final LdapQuery accountQuery =
+        LdapQuery accountQuery =
             new LdapQuery(accountBase, accountScope, new ParameterizedString(
                 accountPattern), accountAtts);
         if (accountQuery.getParameters().isEmpty()) {
@@ -393,6 +408,13 @@ import javax.security.auth.login.LoginException;
               "No variables in ldap.accountPattern");
         }
         accountQueryList.add(accountQuery);
+
+        if (accountWithMemberOfAtts != null) {
+          LdapQuery accountWithMemberOfQuery =
+              new LdapQuery(accountBase, accountScope, new ParameterizedString(
+                  accountPattern), accountWithMemberOfAtts);
+          accountWithMemberOfQueryList.add(accountWithMemberOfQuery);
+        }
       }
     }
 

gerrit-server/src/main/java/com/google/gerrit/server/auth/ldap/LdapAuthBackend.java

@@ -83,7 +83,7 @@ public class LdapAuthBackend implements AuthBackend {
       }
       try {
         final Helper.LdapSchema schema = helper.getSchema(ctx);
-        final LdapQuery.Result m = helper.findAccount(schema, ctx, username);
+        final LdapQuery.Result m = helper.findAccount(schema, ctx, username, false);
 
         if (authConfig.getAuthType() == AuthType.LDAP) {
           // We found the user account, but we need to verify

gerrit-server/src/main/java/com/google/gerrit/server/auth/ldap/LdapRealm.java

@@ -68,6 +68,7 @@ public class LdapRealm extends AbstractRealm {
   private final EmailExpander emailExpander;
   private final LoadingCache<String, Optional<Account.Id>> usernameCache;
   private final Set<Account.FieldName> readOnlyAccountFields;
+  private final boolean fetchMemberOfEagerly;
   private final Config config;
 
   private final LoadingCache<String, Set<AccountGroup.UUID>> membershipCache;
@@ -95,6 +96,8 @@ public class LdapRealm extends AbstractRealm {
     if (optdef(config, "accountSshUserName", "DEFAULT") != null) {
       readOnlyAccountFields.add(Account.FieldName.USER_NAME);
     }
+
+    fetchMemberOfEagerly = optional(config, "fetchMemberOfEagerly", true);
   }
 
   static SearchScope scope(final Config c, final String setting) {
@@ -215,7 +218,8 @@ public class LdapRealm extends AbstractRealm {
       }
       try {
         final Helper.LdapSchema schema = helper.getSchema(ctx);
-        final LdapQuery.Result m = helper.findAccount(schema, ctx, username);
+        final LdapQuery.Result m = helper.findAccount(schema, ctx, username,
+            fetchMemberOfEagerly);
 
         if (authConfig.getAuthType() == AuthType.LDAP && !who.isSkipAuthentication()) {
           // We found the user account, but we need to verify
@@ -244,7 +248,9 @@ public class LdapRealm extends AbstractRealm {
         // in the middle of authenticating the user, its likely we will
         // need to know what access rights they have soon.
         //
-        membershipCache.put(username, helper.queryForGroups(ctx, username, m));
+        if (fetchMemberOfEagerly) {
+          membershipCache.put(username, helper.queryForGroups(ctx, username, m));
+        }
         return who;
       } finally {
         try {