opendev/gerrit
Code Issues Proposed changes

Support managing visible external groups within internal groups

Browse Source 
Unfortunately GroupControl.isVisible() does not correctly enforce
rules for other GroupBackend implementations. A backend may need to
use a different check than "caller is a member of" to determine if
the user can see the group. GroupsCollection cannot always delegate
to GroupControl.isVisible() when checking an identifier for a nested
included group.

GroupBackend implementations are supposed to implement their own
visibility checks as part of the suggest() method. The internal
backend already does this, routing through GroupControl to verify
the caller can see the group. Other backends used on well-known
servers also perform their own access checks, whose results are
being blocked by GroupControl.isVisible() because it doesn't have
the correct visibility information.

Allow parse(String) to return the matching GroupDescription. Check
visibility for the caller inside of the collection's parse method,
where its known the target group must already be an internal group.

Change-Id: I88b09de1e70177137badd7b9b37c202ab5ae89e1
This commit is contained in:
Shawn Pearce
2013-02-07 21:11:14 -08:00
parent b39b746f40
commit 3e4009b9f4
6 changed files with 64 additions and 62 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

@@ -45,7 +45,7 @@ public class GroupControl {
if (group == null) {
throw new NoSuchGroupException(groupId);
}
return new GroupControl(user.get(), group);
return controlFor(GroupDescriptions.forAccountGroup(group));
}
public GroupControl controlFor(final AccountGroup.UUID groupId)
@@ -54,10 +54,14 @@ public class GroupControl {
if (group == null) {
throw new NoSuchGroupException(groupId);
}
return new GroupControl(user.get(), group);
return controlFor(group);
}
public GroupControl controlFor(final AccountGroup group) {
public GroupControl controlFor(AccountGroup group) {
return controlFor(GroupDescriptions.forAccountGroup(group));
}
public GroupControl controlFor(GroupDescription.Basic group) {
return new GroupControl(user.get(), group);
}
@@ -89,10 +93,6 @@ public class GroupControl {
group = gd;
}
GroupControl(CurrentUser who, AccountGroup ag) {
this(who, GroupDescriptions.forAccountGroup(ag));
}
public GroupDescription.Basic getGroup() {
return group;
}

40
gerrit-server/src/main/java/com/google/gerrit/server/group/AddIncludedGroups.java
View File

@@ -18,6 +18,7 @@ import com.google.common.base.Strings;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.Lists;
import com.google.common.collect.Maps;
import com.google.gerrit.common.data.GroupDescription;
import com.google.gerrit.common.errors.NoSuchGroupException;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.BadRequestException;
@@ -94,28 +95,31 @@ public class AddIncludedGroups implements RestModifyView<GroupResource, Input> {
Account.Id me = ((IdentifiedUser) control.getCurrentUser()).getAccountId();
for (String includedGroup : input.groups) {
GroupDescription.Basic d;
try {
GroupResource includedGroupResource = groupsCollection.get().parse(includedGroup);
if (!control.canAddGroup(includedGroupResource.getGroupUUID())) {
throw new AuthException(String.format("Cannot add group: %s",
includedGroupResource.getName()));
}
if (!newIncludedGroups.containsKey(includedGroupResource.getGroupUUID())) {
AccountGroupIncludeByUuid.Key agiKey =
new AccountGroupIncludeByUuid.Key(group.getId(),
includedGroupResource.getGroupUUID());
AccountGroupIncludeByUuid agi = db.accountGroupIncludesByUuid().get(agiKey);
if (agi == null) {
agi = new AccountGroupIncludeByUuid(agiKey);
newIncludedGroups.put(includedGroupResource.getGroupUUID(), agi);
newIncludedGroupsAudits.add(new AccountGroupIncludeByUuidAudit(agi, me));
}
}
result.add(new GroupInfo(includedGroupResource.getGroup()));
d = groupsCollection.get().parse(includedGroup);
} catch (ResourceNotFoundException e) {
badRequest.addError(new NoSuchGroupException(includedGroup));
continue;
}
if (!control.canAddGroup(d.getGroupUUID())) {
throw new AuthException(String.format("Cannot add group: %s",
d.getName()));
}
if (!newIncludedGroups.containsKey(d.getGroupUUID())) {
AccountGroupIncludeByUuid.Key agiKey =
new AccountGroupIncludeByUuid.Key(group.getId(),
d.getGroupUUID());
AccountGroupIncludeByUuid agi = db.accountGroupIncludesByUuid().get(agiKey);
if (agi == null) {
agi = new AccountGroupIncludeByUuid(agiKey);
newIncludedGroups.put(d.getGroupUUID(), agi);
newIncludedGroupsAudits.add(new AccountGroupIncludeByUuidAudit(agi, me));
}
}
result.add(new GroupInfo(d));
}
badRequest.failOnError();

5
gerrit-server/src/main/java/com/google/gerrit/server/group/CreateGroup.java
View File

@@ -17,6 +17,7 @@ package com.google.gerrit.server.group;
import com.google.common.base.Objects;
import com.google.common.base.Strings;
import com.google.gerrit.common.data.GlobalCapability;
import com.google.gerrit.common.data.GroupDescription;
import com.google.gerrit.common.data.GroupDescriptions;
import com.google.gerrit.common.errors.NameAlreadyUsedException;
import com.google.gerrit.common.errors.PermissionDeniedException;
@@ -107,8 +108,8 @@ class CreateGroup implements RestModifyView<TopLevelResource, Input> {
private AccountGroup.Id owner(Input input) throws BadRequestException {
if (input.ownerId != null) {
try {
GroupResource rsrc = groups.parse(Url.decode(input.ownerId));
AccountGroup owner = GroupDescriptions.toAccountGroup(rsrc.getGroup());
GroupDescription.Basic d = groups.parse(Url.decode(input.ownerId));
AccountGroup owner = GroupDescriptions.toAccountGroup(d);
if (owner == null) {
throw new BadRequestException("ownerId must be internal group");
}

27
gerrit-server/src/main/java/com/google/gerrit/server/group/DeleteIncludedGroups.java
View File

@@ -17,6 +17,7 @@ package com.google.gerrit.server.group;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.Lists;
import com.google.common.collect.Maps;
import com.google.gerrit.common.data.GroupDescription;
import com.google.gerrit.common.errors.NoSuchGroupException;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.BadRequestException;
@@ -74,24 +75,24 @@ public class DeleteIncludedGroups implements RestModifyView<GroupResource, Input
final BadRequestHandler badRequest = new BadRequestHandler("removing included groups");
for (final String includedGroup : input.groups) {
GroupDescription.Basic d;
try {
final GroupResource includedGroupResource = groupsCollection.get().parse(includedGroup);
if (!control.canRemoveGroup(includedGroupResource.getGroupUUID())) {
throw new AuthException(String.format("Cannot delete group: %s",
includedGroupResource.getName()));
}
final AccountGroupIncludeByUuid g =
includedGroups.remove(includedGroupResource.getGroupUUID());
if (g != null) {
toRemove.add(g);
}
d = groupsCollection.get().parse(includedGroup);
} catch (ResourceNotFoundException e) {
badRequest.addError(new NoSuchGroupException(includedGroup));
continue;
}
if (!control.canRemoveGroup(d.getGroupUUID())) {
throw new AuthException(String.format("Cannot delete group: %s",
d.getName()));
}
AccountGroupIncludeByUuid g = includedGroups.remove(d.getGroupUUID());
if (g != null) {
toRemove.add(g);
}
}
badRequest.failOnError();
if (!toRemove.isEmpty()) {

36
gerrit-server/src/main/java/com/google/gerrit/server/group/GroupsCollection.java
View File

@@ -14,6 +14,7 @@
package com.google.gerrit.server.group;
import com.google.gerrit.common.data.GroupDescription;
import com.google.gerrit.common.data.GroupReference;
import com.google.gerrit.common.errors.NoSuchGroupException;
import com.google.gerrit.extensions.registration.DynamicMap;
@@ -81,23 +82,28 @@ public class GroupsCollection implements
} else if(!(user instanceof IdentifiedUser)) {
throw new ResourceNotFoundException(id);
}
return parse(id.get());
GroupControl ctl = groupControlFactory.controlFor(parse(id.get()));
if (!ctl.isVisible()) {
throw new ResourceNotFoundException(id);
}
return new GroupResource(ctl);
}
GroupResource parse(String id) throws ResourceNotFoundException {
try {
AccountGroup.UUID uuid = new AccountGroup.UUID(id);
if (groupBackend.handles(uuid)) {
return check(id, groupControlFactory.controlFor(uuid));
GroupDescription.Basic parse(String id) throws ResourceNotFoundException {
AccountGroup.UUID uuid = new AccountGroup.UUID(id);
if (groupBackend.handles(uuid)) {
GroupDescription.Basic d = groupBackend.get(uuid);
if (d != null) {
return d;
}
} catch (NoSuchGroupException noSuchGroup) {
}
// Might be a legacy AccountGroup.Id.
if (id.matches("^[1-9][0-9]*$")) {
try {
AccountGroup.Id legacyId = AccountGroup.Id.parse(id);
return check(id, groupControlFactory.controlFor(legacyId));
return groupControlFactory.controlFor(legacyId).getGroup();
} catch (IllegalArgumentException invalidId) {
} catch (NoSuchGroupException e) {
}
@@ -106,23 +112,15 @@ public class GroupsCollection implements
// Might be a group name, be nice and accept unique names.
GroupReference ref = GroupBackends.findExactSuggestion(groupBackend, id);
if (ref != null) {
try {
return check(id, groupControlFactory.controlFor(ref.getUUID()));
} catch (NoSuchGroupException e) {
GroupDescription.Basic d = groupBackend.get(ref.getUUID());
if (d != null) {
return d;
}
}
throw new ResourceNotFoundException(id);
}
private static GroupResource check(String id, GroupControl ctl)
throws ResourceNotFoundException {
if (ctl.isVisible()) {
return new GroupResource(ctl);
}
throw new ResourceNotFoundException(id);
}
@SuppressWarnings("unchecked")
@Override
public CreateGroup create(TopLevelResource root, IdString name) {

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

@@ -63,9 +63,7 @@ public class IncludedGroupsCollection implements
throw new ResourceNotFoundException(id);
}
GroupDescription.Basic member =
groupsCollection.get().parse(id.get()).getGroup();
GroupDescription.Basic member = groupsCollection.get().parse(id.get());
if (isMember(parent, member)
&& resource.getControl().canSeeGroup(member.getGroupUUID())) {
return new IncludedGroupResource(resource, member);