opendev/gerrit
Code Issues Proposed changes

Fix nested group expansion

Browse Source 
When an internal group contains an external group the
universal group backend needs to redispatch any check
to other implementations. Support this inside of the
internal group backend by expanding the query UUIDs
and caching results.

Change-Id: I4e3d163fdaafa8a6df76bd933ca358a4f84d855b
This commit is contained in:
Shawn Pearce
2013-01-17 16:42:36 -08:00
parent 55117d37db
commit 8f07bbbd90
6 changed files with 158 additions and 60 deletions
Download Patch File Download Diff File Expand all files Collapse all files

14
gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/account/GroupAdminServiceImpl.java
View File

@@ -223,7 +223,8 @@ class GroupAdminServiceImpl extends BaseServiceImplementation implements
public GroupDetail run(ReviewDb db) throws OrmException, Failure,
NoSuchGroupException {
final GroupControl control = groupControlFactory.validateFor(groupId);
if (groupCache.get(groupId).getType() != AccountGroup.Type.INTERNAL) {
AccountGroup group = groupCache.get(groupId);
if (group.getType() != AccountGroup.Type.INTERNAL) {
throw new Failure(new NameAlreadyUsedException());
}
@@ -244,7 +245,8 @@ class GroupAdminServiceImpl extends BaseServiceImplementation implements
Collections.singleton(new AccountGroupIncludeByUuidAudit(m,
getAccountId())));
db.accountGroupIncludesByUuid().insert(Collections.singleton(m));
groupIncludeCache.evictInclude(incGroupUUID);
groupIncludeCache.evictMemberIn(incGroupUUID);
groupIncludeCache.evictMembersOf(group.getGroupUUID());
}
return groupDetailFactory.create(groupId).call();
@@ -313,7 +315,8 @@ class GroupAdminServiceImpl extends BaseServiceImplementation implements
public VoidResult run(final ReviewDb db) throws OrmException,
NoSuchGroupException, Failure {
final GroupControl control = groupControlFactory.validateFor(groupId);
if (groupCache.get(groupId).getType() != AccountGroup.Type.INTERNAL) {
AccountGroup group = groupCache.get(groupId);
if (group.getType() != AccountGroup.Type.INTERNAL) {
throw new Failure(new NameAlreadyUsedException());
}
@@ -353,7 +356,10 @@ class GroupAdminServiceImpl extends BaseServiceImplementation implements
}
}
for (AccountGroup.UUID uuid : groupsToEvict) {
groupIncludeCache.evictInclude(uuid);
groupIncludeCache.evictMemberIn(uuid);
}
if (!groupsToEvict.isEmpty()) {
groupIncludeCache.evictMembersOf(group.getGroupUUID());
}
return VoidResult.INSTANCE;
}

11
gerrit-server/src/main/java/com/google/gerrit/server/account/GroupIncludeCache.java
View File

@@ -16,12 +16,17 @@ package com.google.gerrit.server.account;
import com.google.gerrit.reviewdb.client.AccountGroup;
import java.util.Collection;
import java.util.Set;
/** Tracks group inclusions in memory for efficient access. */
public interface GroupIncludeCache {
public Collection<AccountGroup.UUID> getByInclude(AccountGroup.UUID groupId);
/** @return groups directly a member of the passed group. */
public Set<AccountGroup.UUID> membersOf(AccountGroup.UUID group);
public void evictInclude(AccountGroup.UUID groupId);
/** @return any groups the passed group belongs to. */
public Set<AccountGroup.UUID> memberIn(AccountGroup.UUID groupId);
public void evictMembersOf(AccountGroup.UUID groupId);
public void evictMemberIn(AccountGroup.UUID groupId);
}

75
gerrit-server/src/main/java/com/google/gerrit/server/account/GroupIncludeCacheImpl.java
View File

@@ -32,7 +32,6 @@ import com.google.inject.name.Named;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import java.util.Collection;
import java.util.Collections;
import java.util.List;
import java.util.Set;
@@ -44,6 +43,7 @@ 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";
public static Module module() {
return new CacheModule() {
@@ -52,7 +52,12 @@ public class GroupIncludeCacheImpl implements GroupIncludeCache {
cache(BYINCLUDE_NAME,
AccountGroup.UUID.class,
new TypeLiteral<Set<AccountGroup.UUID>>() {})
.loader(ByIncludeLoader.class);
.loader(MemberInLoader.class);
cache(MEMBERS_NAME,
AccountGroup.UUID.class,
new TypeLiteral<Set<AccountGroup.UUID>>() {})
.loader(MembersOfLoader.class);
bind(GroupIncludeCacheImpl.class);
bind(GroupIncludeCache.class).to(GroupIncludeCacheImpl.class);
@@ -60,35 +65,83 @@ public class GroupIncludeCacheImpl implements GroupIncludeCache {
};
}
private final LoadingCache<AccountGroup.UUID, Set<AccountGroup.UUID>> byInclude;
private final LoadingCache<AccountGroup.UUID, Set<AccountGroup.UUID>> membersOf;
private final LoadingCache<AccountGroup.UUID, Set<AccountGroup.UUID>> memberIn;
@Inject
GroupIncludeCacheImpl(
@Named(BYINCLUDE_NAME) LoadingCache<AccountGroup.UUID, Set<AccountGroup.UUID>> byInclude) {
this.byInclude = byInclude;
@Named(MEMBERS_NAME) LoadingCache<AccountGroup.UUID, Set<AccountGroup.UUID>> membersOf,
@Named(BYINCLUDE_NAME) LoadingCache<AccountGroup.UUID, Set<AccountGroup.UUID>> memberIn) {
this.membersOf = membersOf;
this.memberIn = memberIn;
}
public Collection<AccountGroup.UUID> getByInclude(AccountGroup.UUID groupId) {
public Set<AccountGroup.UUID> membersOf(AccountGroup.UUID groupId) {
try {
return byInclude.get(groupId);
return membersOf.get(groupId);
} catch (ExecutionException e) {
log.warn("Cannot load members of group", e);
return Collections.emptySet();
}
}
public Set<AccountGroup.UUID> memberIn(AccountGroup.UUID groupId) {
try {
return memberIn.get(groupId);
} catch (ExecutionException e) {
log.warn("Cannot load included groups", e);
return Collections.emptySet();
}
}
public void evictInclude(AccountGroup.UUID groupId) {
public void evictMembersOf(AccountGroup.UUID groupId) {
if (groupId != null) {
byInclude.invalidate(groupId);
membersOf.invalidate(groupId);
}
}
static class ByIncludeLoader extends
public void evictMemberIn(AccountGroup.UUID groupId) {
if (groupId != null) {
memberIn.invalidate(groupId);
}
}
static class MembersOfLoader extends
CacheLoader<AccountGroup.UUID, Set<AccountGroup.UUID>> {
private final SchemaFactory<ReviewDb> schema;
@Inject
ByIncludeLoader(final SchemaFactory<ReviewDb> sf) {
MembersOfLoader(final SchemaFactory<ReviewDb> sf) {
schema = sf;
}
@Override
public Set<AccountGroup.UUID> load(AccountGroup.UUID key) throws Exception {
final ReviewDb db = schema.open();
try {
List<AccountGroup> group = db.accountGroups().byUUID(key).toList();
if (group.size() != 1) {
return Collections.emptySet();
}
Set<AccountGroup.UUID> ids = Sets.newHashSet();
for (AccountGroupIncludeByUuid agi : db.accountGroupIncludesByUuid()
.byGroup(group.get(0).getId())) {
ids.add(agi.getIncludeUUID());
}
return ImmutableSet.copyOf(ids);
} finally {
db.close();
}
}
}
static class MemberInLoader extends
CacheLoader<AccountGroup.UUID, Set<AccountGroup.UUID>> {
private final SchemaFactory<ReviewDb> schema;
@Inject
MemberInLoader(final SchemaFactory<ReviewDb> sf) {
schema = sf;
}

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

@@ -14,36 +14,49 @@
package com.google.gerrit.server.account;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Lists;
import com.google.common.collect.Maps;
import com.google.common.collect.Sets;
import com.google.gerrit.reviewdb.client.AccountGroup;
import com.google.gerrit.server.IdentifiedUser;
import com.google.inject.Inject;
import com.google.inject.assistedinject.Assisted;
import java.util.Collections;
import java.util.Queue;
import java.util.List;
import java.util.Map;
import java.util.Set;
/**
* Creates a GroupMembership checker for the internal group system, which
* starts with the seed groups and includes all child groups.
* Group membership checker for the internal group system.
* <p>
* Groups the user is directly a member of are pulled from the in-memory
* AccountCache by way of the IdentifiedUser. Transitive group memberhips are
* resolved on demand starting from the requested group and looking for a path
* to a group the user is a member of. Other group backends are supported by
* recursively invoking the universal GroupMembership.
*/
public class IncludingGroupMembership implements GroupMembership {
public interface Factory {
IncludingGroupMembership create(Iterable<AccountGroup.UUID> groupIds);
IncludingGroupMembership create(IdentifiedUser user);
}
private final GroupIncludeCache groupIncludeCache;
private final Set<AccountGroup.UUID> includes;
private final Queue<AccountGroup.UUID> groupQueue;
private final GroupIncludeCache includeCache;
private final IdentifiedUser user;
private final Map<AccountGroup.UUID, Boolean> memberOf;
private Set<AccountGroup.UUID> knownGroups;
@Inject
IncludingGroupMembership(
GroupIncludeCache groupIncludeCache,
@Assisted Iterable<AccountGroup.UUID> seedGroups) {
this.groupIncludeCache = groupIncludeCache;
this.includes = Sets.newHashSet(seedGroups);
this.groupQueue = Lists.newLinkedList(seedGroups);
IncludingGroupMembership(GroupIncludeCache includeCache,
@Assisted IdentifiedUser user) {
this.includeCache = includeCache;
this.user = user;
Set<AccountGroup.UUID> groups = user.state().getInternalGroups();
memberOf = Maps.newHashMapWithExpectedSize(groups.size());
for (AccountGroup.UUID g : groups) {
memberOf.put(g, true);
}
}
@Override
@@ -51,23 +64,40 @@ public class IncludingGroupMembership implements GroupMembership {
if (id == null) {
return false;
}
if (includes.contains(id)) {
return true;
}
return findIncludedGroup(Collections.singleton(id));
Boolean b = memberOf.get(id);
return b != null ? b : containsAnyOf(ImmutableSet.of(id));
}
@Override
public boolean containsAnyOf(Iterable<AccountGroup.UUID> ids) {
Set<AccountGroup.UUID> query = Sets.newHashSet();
for (AccountGroup.UUID groupId : ids) {
if (includes.contains(groupId)) {
public boolean containsAnyOf(Iterable<AccountGroup.UUID> queryIds) {
// Prefer lookup of a cached result over expanding includes.
boolean tryExpanding = false;
for (AccountGroup.UUID id : queryIds) {
Boolean b = memberOf.get(id);
if (b == null) {
tryExpanding = true;
} else if (b) {
return true;
}
query.add(groupId);
}
return findIncludedGroup(query);
if (tryExpanding) {
for (AccountGroup.UUID id : queryIds) {
if (memberOf.containsKey(id)) {
// Membership was earlier proven to be false.
continue;
}
memberOf.put(id, false);
if (search(includeCache.membersOf(id))) {
memberOf.put(id, true);
return true;
}
}
}
return false;
}
@Override
@@ -81,25 +111,31 @@ public class IncludingGroupMembership implements GroupMembership {
return r;
}
private boolean findIncludedGroup(Set<AccountGroup.UUID> query) {
boolean found = false;
while (!found && !groupQueue.isEmpty()) {
AccountGroup.UUID id = groupQueue.remove();
private boolean search(Set<AccountGroup.UUID> ids) {
return user.getEffectiveGroups().containsAnyOf(ids);
}
for (final AccountGroup.UUID groupId : groupIncludeCache.getByInclude(id)) {
if (includes.add(groupId)) {
groupQueue.add(groupId);
found |= query.contains(groupId);
private ImmutableSet<AccountGroup.UUID> computeKnownGroups() {
Set<AccountGroup.UUID> direct = user.state().getInternalGroups();
Set<AccountGroup.UUID> r = Sets.newHashSet(direct);
List<AccountGroup.UUID> q = Lists.newArrayList(r);
while (!q.isEmpty()) {
AccountGroup.UUID id = q.remove(q.size() - 1);
for (AccountGroup.UUID g : includeCache.memberIn(id)) {
if (r.add(g)) {
q.add(g);
memberOf.put(g, true);
}
}
}
return found;
return ImmutableSet.copyOf(r);
}
@Override
public Set<AccountGroup.UUID> getKnownGroups() {
findIncludedGroup(Collections.<AccountGroup.UUID>emptySet()); // find all
return Sets.newHashSet(includes);
if (knownGroups == null) {
knownGroups = computeKnownGroups();
}
return knownGroups;
}
}

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

@@ -28,9 +28,7 @@ import com.google.inject.Singleton;
import java.util.Collection;
/**
* Implementation of GroupBackend for the internal group system.
*/
/** Implementation of GroupBackend for the internal group system. */
@Singleton
public class InternalGroupBackend implements GroupBackend {
private static final Function<AccountGroup, GroupReference> ACT_GROUP_TO_GROUP_REF =
@@ -45,7 +43,6 @@ public class InternalGroupBackend implements GroupBackend {
private final GroupCache groupCache;
private final IncludingGroupMembership.Factory groupMembershipFactory;
@Inject
InternalGroupBackend(GroupControl.Factory groupControlFactory,
GroupCache groupCache,
@@ -89,6 +86,6 @@ public class InternalGroupBackend implements GroupBackend {
@Override
public GroupMembership membershipsOf(IdentifiedUser user) {
return groupMembershipFactory.create(user.state().getInternalGroups());
return groupMembershipFactory.create(user);
}
}

3
gerrit-server/src/main/java/com/google/gerrit/server/account/PerformCreateGroup.java
View File

@@ -128,6 +128,7 @@ public class PerformCreateGroup {
if (initialGroups != null) {
addGroups(groupId, initialGroups);
groupIncludeCache.evictMembersOf(uuid);
}
groupCache.onCreateGroup(nameKey);
@@ -177,7 +178,7 @@ public class PerformCreateGroup {
db.accountGroupIncludesByUuidAudit().insert(includesAudit);
for (AccountGroup.UUID uuid : groups) {
groupIncludeCache.evictInclude(uuid);
groupIncludeCache.evictMemberIn(uuid);
}
}
}