opendev/gerrit
Code Issues Proposed changes

Configurable ldap.fetchMemberOfEagerly to optimize LDAP login

Browse Source 
Only query for all groups where the user belongs to if the
fetchMemberOfEagerly=true. Since querying for LDAP group membership
also performs recursive group lookup, this may save a lot of LDAP
requests and traffic on user login. Gerrit instances which use LDAP
for user authentication but otherwise rely on local Gerrit groups
will want to set the fetchMemberOfEagerly=false.

NOTE: Even if we avoid fetching LDAP group membership eagerly on user
login, we just postpone querying of the LDAP group membership until the
UI asks for all capabilities for this user. This happens immediately
after the login. This issue is addressed in the follow up change.

Change-Id: I4c33361976d814788dceb58a67c2027b9fd3e8d1
This commit is contained in:
Saša Živkov
2015-02-04 17:19:20 +01:00
parent 386b419e76
commit 723e69cf67
4 changed files with 50 additions and 12 deletions
Download Patch File Download Diff File Expand all files Collapse all files

10
Documentation/config-gerrit.txt
View File

@@ -2272,6 +2272,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

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

@@ -187,13 +187,20 @@ import javax.security.auth.login.LoginException;
return ldapSchema;
}
LdapQuery.Result findAccount(final Helper.LdapSchema schema,
final DirContext ctx, final String username) throws NamingException,
AccountException {
LdapQuery.Result findAccount(Helper.LdapSchema schema,
DirContext ctx, String username, boolean fetchMemberOf)
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);
@@ -215,7 +222,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");
@@ -236,9 +243,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");
@@ -313,6 +320,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;
@@ -324,6 +332,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<>();
@@ -377,7 +386,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;
}
@@ -386,8 +394,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()) {
@@ -395,6 +410,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);
}
}
}

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

@@ -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

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

@@ -69,6 +69,7 @@ public class LdapRealm implements Realm {
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;
@@ -96,6 +97,8 @@ public class LdapRealm implements Realm {
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) {
@@ -216,7 +219,8 @@ public class LdapRealm implements Realm {
}
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
@@ -245,7 +249,9 @@ public class LdapRealm implements Realm {
// 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 {