opendev/gerrit
Code Issues Proposed changes

Perform user.getEffectiveGroups() less eagerly

Browse Source 
Most of the time we used the following pattern to check if a user is
a member of one of the given groups:

  user.getEffectiveGroups().containsAnyOf(groups)

The getEffectiveGroups used to eagerly fetch all groups where this user
belongs to from all registered group backends. This included also
recursive expansion of all LDAP groups this user is a member of. Even
if the set of given groups was consisting of only local Gerrit groups
the code above would still first fetch from all known group backends.
This was particularly bad when using LDAP for user authentication but
not using the LDAP groups otherwise. In large corporate setups a user
could be a member of hundreds of LDAP groups which would all
unnecessarily be fetched just to verify if the user is a member of a
local group.

Introduce the:

  GroupBacked.memberOfAny(user, groups)

to enable a more lazy implementation. The UniversalGroupBackend will
first partition the groups based on their group backends and then only
fetch groups from the involved backends. For an LDAP based Gerrit
instance which doesn't make use of LDAP groups this would effectively
avoid fetching of the LDAP groups for this user.

In our corporate setup this reduces the data transfered between the LDAP
server and a Gerrit instance from 250KB down to 6KB, per user.

Change-Id: I6e3027381cbf4cace454fa0cb9bfc725a2f452fa
This commit is contained in:
Saša Živkov
2015-02-09 13:20:28 +01:00
committed by Edwin Kempin
parent c81291fde0
commit f7569d0cb2
18 changed files with 115 additions and 28 deletions
Download Patch File Download Diff File Expand all files Collapse all files

7
gerrit-server/src/main/java/com/google/gerrit/server/AnonymousUser.java
View File

@@ -14,6 +14,8 @@
package com.google.gerrit.server;
import com.google.common.collect.Iterables;
import com.google.gerrit.reviewdb.client.AccountGroup;
import com.google.gerrit.reviewdb.client.AccountProjectWatch;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.server.account.CapabilityControl;
@@ -38,6 +40,11 @@ public class AnonymousUser extends CurrentUser {
return new ListGroupMembership(Collections.singleton(SystemGroupBackend.ANONYMOUS_USERS));
}
@Override
public boolean memberOfAny(Iterable<AccountGroup.UUID> ids) {
return Iterables.contains(ids, SystemGroupBackend.ANONYMOUS_USERS);
}
@Override
public Set<Change.Id> getStarredChanges() {
return Collections.emptySet();

10
gerrit-server/src/main/java/com/google/gerrit/server/CurrentUser.java
View File

@@ -14,12 +14,14 @@
package com.google.gerrit.server;
import com.google.gerrit.reviewdb.client.AccountGroup;
import com.google.gerrit.reviewdb.client.AccountProjectWatch;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.server.account.CapabilityControl;
import com.google.gerrit.server.account.GroupMembership;
import com.google.inject.servlet.RequestScoped;
import java.util.Arrays;
import java.util.Collection;
import java.util.Set;
@@ -77,6 +79,14 @@ public abstract class CurrentUser {
*/
public abstract GroupMembership getEffectiveGroups();
public boolean memberOfAny(Iterable<AccountGroup.UUID> ids) {
return getEffectiveGroups().containsAnyOf(ids);
}
public boolean memberOf(AccountGroup.UUID id) {
return memberOfAny(Arrays.asList(id));
}
/** Set of changes starred by this user. */
public abstract Set<Change.Id> getStarredChanges();

6
gerrit-server/src/main/java/com/google/gerrit/server/IdentifiedUser.java
View File

@@ -23,6 +23,7 @@ import com.google.gerrit.reviewdb.client.AccountDiffPreference;
import com.google.gerrit.reviewdb.client.AccountProjectWatch;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.StarredChange;
import com.google.gerrit.reviewdb.client.AccountGroup.UUID;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.account.AccountCache;
import com.google.gerrit.server.account.AccountState;
@@ -324,6 +325,11 @@ public class IdentifiedUser extends CurrentUser {
return effectiveGroups;
}
@Override
public boolean memberOfAny(Iterable<UUID> ids) {
return groupBackend.memberOfAny(this, ids);
}
@Override
public Set<Change.Id> getStarredChanges() {
if (starredChanges == null) {

6
gerrit-server/src/main/java/com/google/gerrit/server/InternalUser.java
View File

@@ -16,6 +16,7 @@ package com.google.gerrit.server;
import com.google.gerrit.reviewdb.client.AccountProjectWatch;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.AccountGroup.UUID;
import com.google.gerrit.server.account.CapabilityControl;
import com.google.gerrit.server.account.GroupMembership;
import com.google.inject.Inject;
@@ -49,6 +50,11 @@ public class InternalUser extends CurrentUser {
return GroupMembership.EMPTY;
}
@Override
public boolean memberOfAny(Iterable<UUID> ids) {
return false;
}
@Override
public Set<Change.Id> getStarredChanges() {
return Collections.emptySet();

6
gerrit-server/src/main/java/com/google/gerrit/server/PeerDaemonUser.java
View File

@@ -16,6 +16,7 @@ package com.google.gerrit.server;
import com.google.gerrit.reviewdb.client.AccountProjectWatch;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.AccountGroup.UUID;
import com.google.gerrit.server.account.CapabilityControl;
import com.google.gerrit.server.account.GroupMembership;
import com.google.inject.Inject;
@@ -49,6 +50,11 @@ public class PeerDaemonUser extends CurrentUser {
return GroupMembership.EMPTY;
}
@Override
public boolean memberOfAny(Iterable<UUID> ids) {
return false;
}
@Override
public Set<Change.Id> getStarredChanges() {
return Collections.emptySet();

26
gerrit-server/src/main/java/com/google/gerrit/server/account/AbstractGroupBackend.java Normal file
View File

@@ -0,0 +1,26 @@
// 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.account;
import com.google.gerrit.reviewdb.client.AccountGroup.UUID;
import com.google.gerrit.server.IdentifiedUser;
public abstract class AbstractGroupBackend implements GroupBackend {
@Override
public boolean memberOfAny(IdentifiedUser user, Iterable<UUID> ids) {
return membershipsOf(user).containsAnyOf(ids);
}
}

15
gerrit-server/src/main/java/com/google/gerrit/server/account/CapabilityControl.java
View File

@@ -183,10 +183,9 @@ public class CapabilityControl {
// the 'CI Servers' actually use the BATCH queue while everyone else gets
// to use the INTERACTIVE queue without additional grants.
//
GroupMembership groups = user.getEffectiveGroups();
boolean batch = false;
for (PermissionRule r : capabilities.priority) {
if (match(groups, r)) {
if (user.memberOf(r.getGroup().getUUID())) {
switch (r.getAction()) {
case INTERACTIVE:
if (!SystemGroupBackend.isAnonymousOrRegistered(r.getGroup())) {
@@ -265,9 +264,8 @@ public class CapabilityControl {
return rules;
}
GroupMembership groups = user.getEffectiveGroups();
if (rules.size() == 1) {
if (!match(groups, rules.get(0))) {
if (!user.memberOf(rules.get(0).getGroup().getUUID())) {
rules = Collections.emptyList();
}
effective.put(permissionName, rules);
@@ -276,7 +274,7 @@ public class CapabilityControl {
List<PermissionRule> mine = new ArrayList<>(rules.size());
for (PermissionRule rule : rules) {
if (match(groups, rule)) {
if (user.memberOf(rule.getGroup().getUUID())) {
mine.add(rule);
}
}
@@ -304,11 +302,6 @@ public class CapabilityControl {
return rule.getGroup().getUUID();
}
});
return user.getEffectiveGroups().containsAnyOf(ids);
}
private static boolean match(GroupMembership groups,
PermissionRule rule) {
return groups.contains(rule.getGroup().getUUID());
return user.memberOfAny(ids);
}
}

8
gerrit-server/src/main/java/com/google/gerrit/server/account/GroupBackend.java
View File

@@ -50,4 +50,12 @@ public interface GroupBackend {
/** @return the group membership checker for the backend. */
GroupMembership membershipsOf(IdentifiedUser user);
/**
* Check if the user is member of any of the given groups.
*
* @return <code>true</code> if the user is a member of at least one of the
* given groups, <code>false</code> otherwise
*/
boolean memberOfAny(IdentifiedUser user, Iterable<AccountGroup.UUID> ids);
}

4
gerrit-server/src/main/java/com/google/gerrit/server/account/GroupControl.java
View File

@@ -133,7 +133,7 @@ public class GroupControl {
*/
return (accountGroup != null && accountGroup.isVisibleToAll())
|| user instanceof InternalUser
|| user.getEffectiveGroups().contains(group.getGroupUUID())
|| user.memberOf(group.getGroupUUID())
|| isOwner()
|| user.getCapabilities().canAdministrateServer();
}
@@ -144,7 +144,7 @@ public class GroupControl {
isOwner = false;
} else if (isOwner == null) {
AccountGroup.UUID ownerUUID = accountGroup.getOwnerGroupUUID();
isOwner = getCurrentUser().getEffectiveGroups().contains(ownerUUID)
isOwner = getCurrentUser().memberOf(ownerUUID)
|| getCurrentUser().getCapabilities().canAdministrateServer();
}
return isOwner;

2
gerrit-server/src/main/java/com/google/gerrit/server/account/IncludingGroupMembership.java
View File

@@ -112,7 +112,7 @@ public class IncludingGroupMembership implements GroupMembership {
}
private boolean search(Set<AccountGroup.UUID> ids) {
return user.getEffectiveGroups().containsAnyOf(ids);
return user.memberOfAny(ids);
}
private ImmutableSet<AccountGroup.UUID> computeKnownGroups() {

2
gerrit-server/src/main/java/com/google/gerrit/server/account/InternalGroupBackend.java
View File

@@ -31,7 +31,7 @@ import java.util.Collection;
/** Implementation of GroupBackend for the internal group system. */
@Singleton
public class InternalGroupBackend implements GroupBackend {
public class InternalGroupBackend extends AbstractGroupBackend {
private static final Function<AccountGroup, GroupReference> ACT_GROUP_TO_GROUP_REF =
new Function<AccountGroup, GroupReference>() {
@Override

23
gerrit-server/src/main/java/com/google/gerrit/server/account/UniversalGroupBackend.java
View File

@@ -19,6 +19,7 @@ import static com.google.gerrit.server.account.GroupBackends.GROUP_REF_NAME_COMP
import com.google.common.collect.ArrayListMultimap;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Iterables;
import com.google.common.collect.LinkedListMultimap;
import com.google.common.collect.Multimap;
import com.google.common.collect.Sets;
import com.google.gerrit.common.Nullable;
@@ -26,6 +27,7 @@ import com.google.gerrit.common.data.GroupDescription;
import com.google.gerrit.common.data.GroupReference;
import com.google.gerrit.extensions.registration.DynamicSet;
import com.google.gerrit.reviewdb.client.AccountGroup;
import com.google.gerrit.reviewdb.client.AccountGroup.UUID;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.project.ProjectControl;
import com.google.inject.Inject;
@@ -43,7 +45,7 @@ import java.util.Set;
* set of GroupBackends.
*/
@Singleton
public class UniversalGroupBackend implements GroupBackend {
public class UniversalGroupBackend extends AbstractGroupBackend {
private static final Logger log =
LoggerFactory.getLogger(UniversalGroupBackend.class);
@@ -95,6 +97,25 @@ public class UniversalGroupBackend implements GroupBackend {
return new UniversalGroupMembership(user);
}
@Override
public boolean memberOfAny(IdentifiedUser user, Iterable<AccountGroup.UUID> ids) {
Multimap<GroupBackend, AccountGroup.UUID> groups = LinkedListMultimap.create();
for (AccountGroup.UUID uuid : ids) {
for (GroupBackend g : backends) {
if (g.handles(uuid)) {
groups.put(g, uuid);
}
}
}
for (Map.Entry<GroupBackend, Collection<UUID>> e : groups.asMap().entrySet()) {
if (e.getKey().memberOfAny(user, e.getValue())) {
return true;
}
}
return false;
}
private class UniversalGroupMembership implements GroupMembership {
private final Map<GroupBackend, GroupMembership> memberships;

4
gerrit-server/src/main/java/com/google/gerrit/server/auth/ldap/LdapGroupBackend.java
View File

@@ -29,7 +29,7 @@ import com.google.gerrit.reviewdb.client.AccountExternalId;
import com.google.gerrit.reviewdb.client.AccountGroup;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.account.GroupBackend;
import com.google.gerrit.server.account.AbstractGroupBackend;
import com.google.gerrit.server.account.GroupMembership;
import com.google.gerrit.server.account.ListGroupMembership;
import com.google.gerrit.server.auth.ldap.Helper.LdapSchema;
@@ -59,7 +59,7 @@ import javax.security.auth.login.LoginException;
/**
* Implementation of GroupBackend for the LDAP group system.
*/
public class LdapGroupBackend implements GroupBackend {
public class LdapGroupBackend extends AbstractGroupBackend {
private static final Logger log = LoggerFactory.getLogger(LdapGroupBackend.class);
private static final String LDAP_NAME = "ldap/";

4
gerrit-server/src/main/java/com/google/gerrit/server/group/SystemGroupBackend.java
View File

@@ -22,7 +22,7 @@ import com.google.gerrit.common.data.GroupDescription;
import com.google.gerrit.common.data.GroupReference;
import com.google.gerrit.reviewdb.client.AccountGroup;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.account.GroupBackend;
import com.google.gerrit.server.account.AbstractGroupBackend;
import com.google.gerrit.server.account.GroupMembership;
import com.google.gerrit.server.account.ListGroupMembership;
import com.google.gerrit.server.project.ProjectControl;
@@ -36,7 +36,7 @@ import java.util.Map;
import java.util.SortedMap;
import java.util.TreeMap;
public class SystemGroupBackend implements GroupBackend {
public class SystemGroupBackend extends AbstractGroupBackend {
/** Common UUID assigned to the "Anonymous Users" group. */
public static final AccountGroup.UUID ANONYMOUS_USERS =
new AccountGroup.UUID("global:Anonymous-Users");

10
gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java
View File

@@ -34,7 +34,6 @@ import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.InternalUser;
import com.google.gerrit.server.account.GroupMembership;
import com.google.gerrit.server.change.IncludedInResolver;
import com.google.gerrit.server.config.CanonicalWebUrl;
import com.google.gerrit.server.config.GitReceivePackGroups;
@@ -299,8 +298,7 @@ public class ProjectControl {
private boolean isDeclaredOwner() {
if (declaredOwner == null) {
GroupMembership effectiveGroups = user.getEffectiveGroups();
declaredOwner = effectiveGroups.containsAnyOf(state.getAllOwners());
declaredOwner = user.memberOfAny(state.getAllOwners());
}
return declaredOwner;
}
@@ -374,11 +372,11 @@ public class ProjectControl {
}
}
if (iUser.getEffectiveGroups().containsAnyOf(okGroupIds)) {
if (iUser.memberOfAny(okGroupIds)) {
return Capable.OK;
}
if (iUser.getEffectiveGroups().containsAnyOf(missingInfoGroupIds)) {
if (iUser.memberOfAny(missingInfoGroupIds)) {
final StringBuilder msg = new StringBuilder();
for (ContributorAgreement ca : contributorAgreements) {
if (ca.isRequireContactInformation()) {
@@ -512,7 +510,7 @@ public class ProjectControl {
} else if (SystemGroupBackend.CHANGE_OWNER.equals(uuid)) {
return isChangeOwner;
} else {
return user.getEffectiveGroups().contains(uuid);
return user.memberOf(uuid);
}
}

2
gerrit-server/src/main/java/com/google/gerrit/server/query/change/EqualsLabelPredicate.java
View File

@@ -131,7 +131,7 @@ class EqualsLabelPredicate extends IndexPredicate<ChangeData> {
return false;
}
if (group != null && !reviewer.getEffectiveGroups().contains(group)) {
if (group != null && !reviewer.memberOf(group)) {
return false;
}

2
gerrit-server/src/main/java/com/google/gerrit/server/query/change/OwnerinPredicate.java
View File

@@ -47,7 +47,7 @@ class OwnerinPredicate extends OperatorPredicate<ChangeData> {
}
final IdentifiedUser owner = userFactory.create(dbProvider,
change.getOwner());
return owner.getEffectiveGroups().contains(uuid);
return owner.memberOf(uuid);
}
@Override

6
gerrit-server/src/test/java/com/google/gerrit/server/project/Util.java
View File

@@ -29,6 +29,7 @@ import com.google.gerrit.reviewdb.client.AccountGroup;
import com.google.gerrit.reviewdb.client.AccountProjectWatch;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.client.AccountGroup.UUID;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.rules.PrologEnvironment;
import com.google.gerrit.rules.RulesCache;
@@ -356,6 +357,11 @@ public class Util {
return groups;
}
@Override
public boolean memberOfAny(Iterable<UUID> ids) {
return getEffectiveGroups().containsAnyOf(ids);
}
@Override
public String getUserName() {
return username;