Merge changes Ifa58dcf6,I96ac1894,I4c333619,I74df9064,I6eced365, ... into stable-2.10 * changes: Fix LDAP authentication for the RFC2307 server type Lazily lookup LDAP group memberships Configurable ldap.fetchMemberOfEagerly to optimize LDAP login Improve LDAP login times, transfer 40x less data. Reduce number of LDAP queries when having multiple accountBases Fix LDAP connection pool configuration. Improve method and variable names related to group inclusion caches Honor ldap.connectTimeout also without connection pooling
diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt index 14d39a5..c2221df 100644 --- a/Documentation/config-gerrit.txt +++ b/Documentation/config-gerrit.txt
@@ -2272,6 +2272,16 @@ 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 @@ -2399,73 +2409,29 @@ [[ldap.connectTimeout]]ldap.connectTimeout:: + -_(Optional)_ Specify how long to wait for a pooled connection. -This is also used to specify a timeout period for establishment -of the LDAP connection. +_(Optional)_ Timeout period for establishment of an LDAP connection. + The value is in the usual time-unit format like "1 s", "100 ms", etc... + By default there is no timeout and Gerrit will wait indefinitely. -[[ldap.poolAuthentication]]ldap.poolAuthentication:: -+ -_(Optional)_ A list of space-separated authentication types of -connections that may be pooled. Valid types are "none", "simple", -and "DIGEST-MD5". -+ -Default is "none simple". +[[ldap-connection-pooling]] +==== LDAP Connection Pooling +Once LDAP connection pooling is enabled by setting the link:#ldap.useConnectionPooling[ +ldap.useConnectionPooling] configuration property to `true`, the connection pool +can be configured using JVM system properties as explained in the +link:http://docs.oracle.com/javase/7/docs/technotes/guides/jndi/jndi-ldap.html#POOL[ +Java SE Documentation]. -[[ldap.poolDebug]]ldap.poolDebug:: -+ -_(Optional)_ A string that indicates the level of debug output -to produce. Valid values are "fine" (trace connection creation -and removal) and "all" (all debugging information). +For standalone Gerrit (running with the embedded Jetty), JVM system properties +are specified in the link:#container[container section]: -[[ldap.poolInitsize]]ldap.poolInitsize:: -+ -_(Optional)_ The string representation of an integer that -represents the number of connections per connection identity -to create when initially creating a connection for the identity. -+ -Default is 1. - -[[ldap.poolMaxsize]]ldap.poolMaxsize:: -+ -_(Optional)_ The string representation of an integer that -represents the maximum number of connections per connection -identity that can be maintained concurrently. -+ -Default is 0, means that there is no maximum size: A request for -a pooled connection will use an existing pooled idle connection -or a newly created pooled connection. - -[[ldap.poolPrefsize]]ldap.poolPrefsize:: -+ -_(Optional)_ The string representation of an integer that -represents the preferred number of connections per connection -identity that should be maintained concurrently. -+ -Default is 0, means that there is no preferred size: A request -for a pooled connection will result in a newly created connection -only if no idle ones are available. - -[[ldap.poolProtocol]]ldap.poolProtocol:: -+ -_(Optional)_ A list of space-separated protocol types of -connections that may be pooled. Valid types are "plain" and "ssl". -+ -Default is "plain". - -[[ldap.poolTimeout]]ldap.poolTimeout:: -+ -_(Optional)_ Specify how long an idle connection may remain -in the pool without being closed and removed from the pool. -+ -The value is in the usual time-unit format like "1 s", "100 ms", -etc... -+ -By default there is no timeout. +---- + javaOptions = -Dcom.sun.jndi.ldap.connect.pool.maxsize=20 + javaOptions = -Dcom.sun.jndi.ldap.connect.pool.prefsize=10 + javaOptions = -Dcom.sun.jndi.ldap.connect.pool.timeout=300000 +---- [[mimetype]] === Section mimetype
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupIncludeCache.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupIncludeCache.java index d130243..6ba6bf0 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupIncludeCache.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupIncludeCache.java
@@ -21,14 +21,14 @@ /** Tracks group inclusions in memory for efficient access. */ public interface GroupIncludeCache { /** @return groups directly a member of the passed group. */ - public Set<AccountGroup.UUID> membersOf(AccountGroup.UUID group); + public Set<AccountGroup.UUID> subgroupsOf(AccountGroup.UUID group); /** @return any groups the passed group belongs to. */ - public Set<AccountGroup.UUID> memberIn(AccountGroup.UUID groupId); + public Set<AccountGroup.UUID> parentGroupsOf(AccountGroup.UUID groupId); /** @return set of any UUIDs that are not internal groups. */ public Set<AccountGroup.UUID> allExternalMembers(); - public void evictMembersOf(AccountGroup.UUID groupId); - public void evictMemberIn(AccountGroup.UUID groupId); + public void evictSubgroupsOf(AccountGroup.UUID groupId); + public void evictParentGroupsOf(AccountGroup.UUID groupId); }
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupIncludeCacheImpl.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupIncludeCacheImpl.java index 37d407c..c982aa5 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupIncludeCacheImpl.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupIncludeCacheImpl.java
@@ -42,23 +42,23 @@ public class GroupIncludeCacheImpl implements GroupIncludeCache { private static final Logger log = LoggerFactory .getLogger(GroupIncludeCacheImpl.class); - private static final String BYINCLUDE_NAME = "groups_byinclude"; - private static final String MEMBERS_NAME = "groups_members"; + private static final String PARENT_GROUPS_NAME = "groups_byinclude"; + private static final String SUBGROUPS_NAME = "groups_members"; private static final String EXTERNAL_NAME = "groups_external"; public static Module module() { return new CacheModule() { @Override protected void configure() { - cache(BYINCLUDE_NAME, + cache(PARENT_GROUPS_NAME, AccountGroup.UUID.class, new TypeLiteral<Set<AccountGroup.UUID>>() {}) - .loader(MemberInLoader.class); + .loader(parentGroupsLoader.class); - cache(MEMBERS_NAME, + cache(SUBGROUPS_NAME, AccountGroup.UUID.class, new TypeLiteral<Set<AccountGroup.UUID>>() {}) - .loader(MembersOfLoader.class); + .loader(subgroupsLoader.class); cache(EXTERNAL_NAME, String.class, @@ -71,24 +71,24 @@ }; } - private final LoadingCache<AccountGroup.UUID, Set<AccountGroup.UUID>> membersOf; - private final LoadingCache<AccountGroup.UUID, Set<AccountGroup.UUID>> memberIn; + private final LoadingCache<AccountGroup.UUID, Set<AccountGroup.UUID>> subgroups; + private final LoadingCache<AccountGroup.UUID, Set<AccountGroup.UUID>> parentGroups; private final LoadingCache<String, Set<AccountGroup.UUID>> external; @Inject GroupIncludeCacheImpl( - @Named(MEMBERS_NAME) LoadingCache<AccountGroup.UUID, Set<AccountGroup.UUID>> membersOf, - @Named(BYINCLUDE_NAME) LoadingCache<AccountGroup.UUID, Set<AccountGroup.UUID>> memberIn, + @Named(SUBGROUPS_NAME) LoadingCache<AccountGroup.UUID, Set<AccountGroup.UUID>> subgroups, + @Named(PARENT_GROUPS_NAME) LoadingCache<AccountGroup.UUID, Set<AccountGroup.UUID>> parentGroups, @Named(EXTERNAL_NAME) LoadingCache<String, Set<AccountGroup.UUID>> external) { - this.membersOf = membersOf; - this.memberIn = memberIn; + this.subgroups = subgroups; + this.parentGroups = parentGroups; this.external = external; } @Override - public Set<AccountGroup.UUID> membersOf(AccountGroup.UUID groupId) { + public Set<AccountGroup.UUID> subgroupsOf(AccountGroup.UUID groupId) { try { - return membersOf.get(groupId); + return subgroups.get(groupId); } catch (ExecutionException e) { log.warn("Cannot load members of group", e); return Collections.emptySet(); @@ -96,9 +96,9 @@ } @Override - public Set<AccountGroup.UUID> memberIn(AccountGroup.UUID groupId) { + public Set<AccountGroup.UUID> parentGroupsOf(AccountGroup.UUID groupId) { try { - return memberIn.get(groupId); + return parentGroups.get(groupId); } catch (ExecutionException e) { log.warn("Cannot load included groups", e); return Collections.emptySet(); @@ -106,16 +106,16 @@ } @Override - public void evictMembersOf(AccountGroup.UUID groupId) { + public void evictSubgroupsOf(AccountGroup.UUID groupId) { if (groupId != null) { - membersOf.invalidate(groupId); + subgroups.invalidate(groupId); } } @Override - public void evictMemberIn(AccountGroup.UUID groupId) { + public void evictParentGroupsOf(AccountGroup.UUID groupId) { if (groupId != null) { - memberIn.invalidate(groupId); + parentGroups.invalidate(groupId); if (!AccountGroup.isInternalGroup(groupId)) { external.invalidate(EXTERNAL_NAME); @@ -133,12 +133,12 @@ } } - static class MembersOfLoader extends + static class subgroupsLoader extends CacheLoader<AccountGroup.UUID, Set<AccountGroup.UUID>> { private final SchemaFactory<ReviewDb> schema; @Inject - MembersOfLoader(final SchemaFactory<ReviewDb> sf) { + subgroupsLoader(final SchemaFactory<ReviewDb> sf) { schema = sf; } @@ -163,12 +163,12 @@ } } - static class MemberInLoader extends + static class parentGroupsLoader extends CacheLoader<AccountGroup.UUID, Set<AccountGroup.UUID>> { private final SchemaFactory<ReviewDb> schema; @Inject - MemberInLoader(final SchemaFactory<ReviewDb> sf) { + parentGroupsLoader(final SchemaFactory<ReviewDb> sf) { schema = sf; }
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/IncludingGroupMembership.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/IncludingGroupMembership.java index 2e278c3..b8a67ff 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/IncludingGroupMembership.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/IncludingGroupMembership.java
@@ -90,7 +90,7 @@ } memberOf.put(id, false); - if (search(includeCache.membersOf(id))) { + if (search(includeCache.subgroupsOf(id))) { memberOf.put(id, true); return true; } @@ -131,7 +131,7 @@ while (!q.isEmpty()) { AccountGroup.UUID id = q.remove(q.size() - 1); - for (AccountGroup.UUID g : includeCache.memberIn(id)) { + for (AccountGroup.UUID g : includeCache.parentGroupsOf(id)) { if (g != null && r.add(g)) { q.add(g); memberOf.put(g, true);
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/PerformCreateGroup.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/PerformCreateGroup.java index a54a97b..86c840b 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/PerformCreateGroup.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/PerformCreateGroup.java
@@ -116,7 +116,7 @@ if (createGroupArgs.initialGroups != null) { addGroups(groupId, createGroupArgs.initialGroups); - groupIncludeCache.evictMembersOf(uuid); + groupIncludeCache.evictSubgroupsOf(uuid); } groupCache.onCreateGroup(createGroupArgs.getGroup()); @@ -162,7 +162,7 @@ db.accountGroupByIdAud().insert(includesAudit); for (AccountGroup.UUID uuid : groups) { - groupIncludeCache.evictMemberIn(uuid); + groupIncludeCache.evictParentGroupsOf(uuid); } } }
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 cc61695..f8ecf25 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
@@ -60,43 +60,7 @@ @Singleton class Helper { static final String LDAP_UUID = "ldap:"; - static private Map<String, String> getPoolProperties(Config config) { - if (LdapRealm.optional(config, "useConnectionPooling", false)) { - Map<String, String> r = Maps.newHashMap(); - r.put("com.sun.jndi.ldap.connect.pool", "true"); - - String connectTimeout = LdapRealm.optional(config, "connectTimeout"); - String poolDebug = LdapRealm.optional(config, "poolDebug"); - String poolTimeout = LdapRealm.optional(config, "poolTimeout"); - - if (connectTimeout != null) { - r.put("com.sun.jndi.ldap.connect.timeout", Long.toString(ConfigUtil - .getTimeUnit(connectTimeout, 0, TimeUnit.MILLISECONDS))); - } - r.put("com.sun.jndi.ldap.connect.pool.authentication", - LdapRealm.optional(config, "poolAuthentication", "none simple")); - if (poolDebug != null) { - r.put("com.sun.jndi.ldap.connect.pool.debug", poolDebug); - } - r.put("com.sun.jndi.ldap.connect.pool.initsize", - String.valueOf(LdapRealm.optional(config, "poolInitsize", 1))); - r.put("com.sun.jndi.ldap.connect.pool.maxsize", - String.valueOf(LdapRealm.optional(config, "poolMaxsize", 0))); - r.put("com.sun.jndi.ldap.connect.pool.prefsize", - String.valueOf(LdapRealm.optional(config, "poolPrefsize", 0))); - r.put("com.sun.jndi.ldap.connect.pool.protocol", - LdapRealm.optional(config, "poolProtocol", "plain")); - if (poolTimeout != null) { - r.put("com.sun.jndi.ldap.connect.pool.timeout", Long - .toString(ConfigUtil.getTimeUnit(poolTimeout, 0, - TimeUnit.MILLISECONDS))); - } - return r; - } - return null; - } - - private final Cache<String, ImmutableSet<String>> groupsByInclude; + private final Cache<String, ImmutableSet<String>> parentGroups; private final Config config; private final String server; private final String username; @@ -105,13 +69,14 @@ private final boolean sslVerify; private final String authentication; private volatile LdapSchema ldapSchema; - private final String readTimeOutMillis; - private final Map<String, String> connectionPoolConfig; + private final String readTimeoutMillis; + private final String connectTimeoutMillis; + private final boolean useConnectionPooling; @Inject Helper(@GerritServerConfig final Config config, - @Named(LdapModule.GROUPS_BYINCLUDE_CACHE) - Cache<String, ImmutableSet<String>> groupsByInclude) { + @Named(LdapModule.PARENT_GROUPS_CACHE) + Cache<String, ImmutableSet<String>> parentGroups) { this.config = config; this.server = LdapRealm.optional(config, "server"); this.username = LdapRealm.optional(config, "username"); @@ -120,16 +85,25 @@ this.sslVerify = config.getBoolean("ldap", "sslverify", true); this.authentication = LdapRealm.optional(config, "authentication", "simple"); - String timeout = LdapRealm.optional(config, "readTimeout"); - if (timeout != null) { - readTimeOutMillis = - Long.toString(ConfigUtil.getTimeUnit(timeout, 0, + String readTimeout = LdapRealm.optional(config, "readTimeout"); + if (readTimeout != null) { + readTimeoutMillis = + Long.toString(ConfigUtil.getTimeUnit(readTimeout, 0, TimeUnit.MILLISECONDS)); } else { - readTimeOutMillis = null; + readTimeoutMillis = null; } - this.groupsByInclude = groupsByInclude; - this.connectionPoolConfig = getPoolProperties(config); + String connectTimeout = LdapRealm.optional(config, "connectTimeout"); + if (connectTimeout != null) { + connectTimeoutMillis = + Long.toString(ConfigUtil.getTimeUnit(connectTimeout, 0, + TimeUnit.MILLISECONDS)); + } else { + connectTimeoutMillis = null; + } + this.parentGroups = parentGroups; + this.useConnectionPooling = + LdapRealm.optional(config, "useConnectionPooling", false); } private Properties createContextProperties() { @@ -140,17 +114,20 @@ Class<? extends SSLSocketFactory> factory = BlindSSLSocketFactory.class; env.put("java.naming.ldap.factory.socket", factory.getName()); } - if (readTimeOutMillis != null) { - env.put("com.sun.jndi.ldap.read.timeout", readTimeOutMillis); + if (readTimeoutMillis != null) { + env.put("com.sun.jndi.ldap.read.timeout", readTimeoutMillis); + } + if (connectTimeoutMillis != null) { + env.put("com.sun.jndi.ldap.connect.timeout", connectTimeoutMillis); + } + if (useConnectionPooling) { + env.put("com.sun.jndi.ldap.connect.pool", "true"); } return env; } DirContext open() throws NamingException, LoginException { final Properties env = createContextProperties(); - if (connectionPoolConfig != null) { - env.putAll(connectionPoolConfig); - } env.put(Context.SECURITY_AUTHENTICATION, authentication); env.put(Context.REFERRAL, referral); if ("GSSAPI".equals(authentication)) { @@ -210,27 +187,28 @@ 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); - final List<LdapQuery.Result> res = new ArrayList<>(); - for (LdapQuery accountQuery : schema.accountQueryList) { - res.addAll(accountQuery.query(ctx, params)); + List<LdapQuery> accountQueryList; + if (fetchMemberOf && schema.type.accountMemberField() != null) { + accountQueryList = schema.accountWithMemberOfQueryList; + } else { + accountQueryList = schema.accountQueryList; } - switch (res.size()) { - case 0: - throw new NoSuchUserException(username); - - case 1: + for (LdapQuery accountQuery : accountQueryList) { + List<LdapQuery.Result> res = accountQuery.query(ctx, params); + if (res.size() == 1) { return res.get(0); - - default: + } else if (res.size() > 1) { throw new AccountException("Duplicate users: " + username); + } } + throw new NoSuchUserException(username); } Set<AccountGroup.UUID> queryForGroups(final DirContext ctx, @@ -244,7 +222,7 @@ 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"); @@ -265,9 +243,9 @@ } 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"); @@ -303,14 +281,15 @@ private void recursivelyExpandGroups(final Set<String> groupDNs, final LdapSchema schema, final DirContext ctx, final String groupDN) { if (groupDNs.add(groupDN) && schema.accountMemberField != null) { - ImmutableSet<String> cachedGroupDNs = groupsByInclude.getIfPresent(groupDN); - if (cachedGroupDNs == null) { + ImmutableSet<String> cachedParentsDNs = parentGroups.getIfPresent(groupDN); + if (cachedParentsDNs == null) { // Recursively identify the groups it is a member of. ImmutableSet.Builder<String> dns = ImmutableSet.builder(); try { final Name compositeGroupName = new CompositeName().add(groupDN); final Attribute in = - ctx.getAttributes(compositeGroupName).get(schema.accountMemberField); + ctx.getAttributes(compositeGroupName, schema.accountMemberFieldArray) + .get(schema.accountMemberField); if (in != null) { final NamingEnumeration<?> groups = in.getAll(); try { @@ -323,10 +302,10 @@ } catch (NamingException e) { LdapRealm.log.warn("Could not find group " + groupDN, e); } - cachedGroupDNs = dns.build(); - groupsByInclude.put(groupDN, cachedGroupDNs); + cachedParentsDNs = dns.build(); + parentGroups.put(groupDN, cachedParentsDNs); } - for (String dn : cachedGroupDNs) { + for (String dn : cachedParentsDNs) { recursivelyExpandGroups(groupDNs, schema, ctx, dn); } } @@ -339,7 +318,9 @@ final ParameterizedString accountEmailAddress; final ParameterizedString accountSshUserName; final String accountMemberField; + final String[] accountMemberFieldArray; final List<LdapQuery> accountQueryList; + final List<LdapQuery> accountWithMemberOfQueryList; final List<String> groupBases; final SearchScope groupScope; @@ -351,6 +332,7 @@ type = discoverLdapType(ctx); groupMemberQueryList = new ArrayList<>(); accountQueryList = new ArrayList<>(); + accountWithMemberOfQueryList = new ArrayList<>(); final Set<String> accountAtts = new HashSet<>(); @@ -403,15 +385,24 @@ accountMemberField = LdapRealm.optdef(config, "accountMemberField", type.accountMemberField()); if (accountMemberField != null) { - accountAtts.add(accountMemberField); + accountMemberFieldArray = new String[] {accountMemberField}; + } else { + accountMemberFieldArray = null; } final SearchScope accountScope = LdapRealm.scope(config, "accountScope"); 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()) { @@ -419,6 +410,13 @@ "No variables in ldap.accountPattern"); } accountQueryList.add(accountQuery); + + if (accountWithMemberOfAtts != null) { + LdapQuery accountWithMemberOfQuery = + new LdapQuery(accountBase, accountScope, new ParameterizedString( + accountPattern), accountWithMemberOfAtts); + accountWithMemberOfQueryList.add(accountWithMemberOfQuery); + } } }
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/auth/ldap/LdapAuthBackend.java b/gerrit-server/src/main/java/com/google/gerrit/server/auth/ldap/LdapAuthBackend.java index eb6249c..1d90f0c 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/auth/ldap/LdapAuthBackend.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/auth/ldap/LdapAuthBackend.java
@@ -83,7 +83,7 @@ } 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
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/auth/ldap/LdapGroupBackend.java b/gerrit-server/src/main/java/com/google/gerrit/server/auth/ldap/LdapGroupBackend.java index 7731b7d..6cdce8d 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/auth/ldap/LdapGroupBackend.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/auth/ldap/LdapGroupBackend.java
@@ -31,7 +31,6 @@ import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.account.GroupBackend; import com.google.gerrit.server.account.GroupMembership; -import com.google.gerrit.server.account.ListGroupMembership; import com.google.gerrit.server.auth.ldap.Helper.LdapSchema; import com.google.gerrit.server.project.ProjectCache; import com.google.gerrit.server.project.ProjectControl; @@ -60,7 +59,7 @@ * Implementation of GroupBackend for the LDAP group system. */ public class LdapGroupBackend implements GroupBackend { - private static final Logger log = LoggerFactory.getLogger(LdapGroupBackend.class); + static final Logger log = LoggerFactory.getLogger(LdapGroupBackend.class); private static final String LDAP_NAME = "ldap/"; private static final String GROUPNAME = "groupname"; @@ -185,21 +184,7 @@ if (id == null) { return GroupMembership.EMPTY; } - - try { - final Set<AccountGroup.UUID> groups = membershipCache.get(id); - return new ListGroupMembership(groups) { - @Override - public Set<AccountGroup.UUID> getKnownGroups() { - Set<AccountGroup.UUID> g = Sets.newHashSet(groups); - g.retainAll(projectCache.guessRelevantGroupUUIDs()); - return g; - } - }; - } catch (ExecutionException e) { - log.warn(String.format("Cannot lookup membershipsOf %s in LDAP", id), e); - return GroupMembership.EMPTY; - } + return new LdapGroupMembership(membershipCache, projectCache, id); } private static String findId(final Collection<AccountExternalId> ids) {
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/auth/ldap/LdapGroupMembership.java b/gerrit-server/src/main/java/com/google/gerrit/server/auth/ldap/LdapGroupMembership.java new file mode 100644 index 0000000..7853320 --- /dev/null +++ b/gerrit-server/src/main/java/com/google/gerrit/server/auth/ldap/LdapGroupMembership.java
@@ -0,0 +1,76 @@ +// Copyright (C) 2015 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.gerrit.server.auth.ldap; + +import com.google.common.cache.LoadingCache; +import com.google.gerrit.reviewdb.client.AccountGroup; +import com.google.gerrit.server.account.GroupMembership; +import com.google.gerrit.server.account.ListGroupMembership; +import com.google.gerrit.server.project.ProjectCache; + +import java.util.HashSet; +import java.util.Set; +import java.util.concurrent.ExecutionException; + +class LdapGroupMembership implements GroupMembership { + private final LoadingCache<String, Set<AccountGroup.UUID>> membershipCache; + private final ProjectCache projectCache; + private final String id; + private GroupMembership membership; + + LdapGroupMembership( + LoadingCache<String, Set<AccountGroup.UUID>> membershipCache, + ProjectCache projectCache, + String id) { + this.membershipCache = membershipCache; + this.projectCache = projectCache; + this.id = id; + } + + @Override + public boolean contains(AccountGroup.UUID groupId) { + return get().contains(groupId); + } + + @Override + public boolean containsAnyOf(Iterable<AccountGroup.UUID> groupIds) { + return get().containsAnyOf(groupIds); + } + + @Override + public Set<AccountGroup.UUID> intersection(Iterable<AccountGroup.UUID> groupIds) { + return get().intersection(groupIds); + } + + @Override + public Set<AccountGroup.UUID> getKnownGroups() { + Set<AccountGroup.UUID> g = new HashSet<>(get().getKnownGroups()); + g.retainAll(projectCache.guessRelevantGroupUUIDs()); + return g; + } + + private synchronized GroupMembership get() { + if (membership == null) { + try { + membership = new ListGroupMembership(membershipCache.get(id)); + } catch (ExecutionException e) { + LdapGroupBackend.log.warn(String.format( + "Cannot lookup membershipsOf %s in LDAP", id), e); + membership = GroupMembership.EMPTY; + } + } + return membership; + } +}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/auth/ldap/LdapModule.java b/gerrit-server/src/main/java/com/google/gerrit/server/auth/ldap/LdapModule.java index 88cf45b..eaaafd6 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/auth/ldap/LdapModule.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/auth/ldap/LdapModule.java
@@ -33,7 +33,7 @@ static final String USERNAME_CACHE = "ldap_usernames"; static final String GROUP_CACHE = "ldap_groups"; static final String GROUP_EXIST_CACHE = "ldap_group_existence"; - static final String GROUPS_BYINCLUDE_CACHE = "ldap_groups_byinclude"; + static final String PARENT_GROUPS_CACHE = "ldap_groups_byinclude"; @Override @@ -55,7 +55,7 @@ .expireAfterWrite(1, HOURS) .loader(LdapRealm.ExistenceLoader.class); - cache(GROUPS_BYINCLUDE_CACHE, + cache(PARENT_GROUPS_CACHE, String.class, new TypeLiteral<ImmutableSet<String>>() {}) .expireAfterWrite(1, HOURS);
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/auth/ldap/LdapRealm.java b/gerrit-server/src/main/java/com/google/gerrit/server/auth/ldap/LdapRealm.java index 0f22d2c..7b79add 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/auth/ldap/LdapRealm.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/auth/ldap/LdapRealm.java
@@ -69,6 +69,7 @@ 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 @@ 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 @@ } 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 @@ // 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 {
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/group/AddIncludedGroups.java b/gerrit-server/src/main/java/com/google/gerrit/server/group/AddIncludedGroups.java index 614138a..163b335 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/group/AddIncludedGroups.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/group/AddIncludedGroups.java
@@ -128,9 +128,9 @@ db.get().accountGroupByIdAud().insert(newIncludedGroupsAudits); db.get().accountGroupById().insert(newIncludedGroups.values()); for (AccountGroupById agi : newIncludedGroups.values()) { - groupIncludeCache.evictMemberIn(agi.getIncludeUUID()); + groupIncludeCache.evictParentGroupsOf(agi.getIncludeUUID()); } - groupIncludeCache.evictMembersOf(group.getGroupUUID()); + groupIncludeCache.evictSubgroupsOf(group.getGroupUUID()); } return result;
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/group/DeleteIncludedGroups.java b/gerrit-server/src/main/java/com/google/gerrit/server/group/DeleteIncludedGroups.java index 555744e..8cc1a4a 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/group/DeleteIncludedGroups.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/group/DeleteIncludedGroups.java
@@ -91,9 +91,9 @@ writeAudits(toRemove); db.get().accountGroupById().delete(toRemove); for (final AccountGroupById g : toRemove) { - groupIncludeCache.evictMemberIn(g.getIncludeUUID()); + groupIncludeCache.evictParentGroupsOf(g.getIncludeUUID()); } - groupIncludeCache.evictMembersOf(internalGroup.getGroupUUID()); + groupIncludeCache.evictSubgroupsOf(internalGroup.getGroupUUID()); } return Response.none();
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/ProjectWatch.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/ProjectWatch.java index 3d16372..872b84a 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/ProjectWatch.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/ProjectWatch.java
@@ -166,7 +166,7 @@ for (AccountGroupMember m : db.accountGroupMembers().byGroup(ig.getId())) { matching.accounts.add(m.getAccountId()); } - for (AccountGroup.UUID m : args.groupIncludes.membersOf(uuid)) { + for (AccountGroup.UUID m : args.groupIncludes.subgroupsOf(uuid)) { if (seen.add(m)) { q.add(m); }