opendev/gerrit
Code Issues Proposed changes

Merge changes I2da2eabd,Iefa35b7e

Browse Source 
* changes:
  SSH: set-project-parent is no longer admin exclusive
  New option: receive.allowProjectOwnersToChangeParent
This commit is contained in:
Dave Borowitz 2018-08-28 20:04:25 +00:00 committed by Gerrit Code Review
commit a7a33cb437
9 changed files with 153 additions and 93 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

6
Documentation/cmd-index.txt
View File

@ -85,6 +85,9 @@ link:cmd-set-head.html[gerrit set-head]::
link:cmd-set-project.html[gerrit set-project]::
Change a project's settings.
link:cmd-set-project-parent.html[gerrit set-project-parent]::
Change the project permissions are inherited from.
link:cmd-set-reviewers.html[gerrit set-reviewers]::
Add or remove reviewers on a change.
@ -178,9 +181,6 @@ link:cmd-set-account.html[gerrit set-account]::
link:cmd-set-members.html[gerrit set-members]::
Set group members.
link:cmd-set-project-parent.html[gerrit set-project-parent]::
Change the project permissions are inherited from.
link:cmd-show-caches.html[gerrit show-caches]::
Display current cache statistics.

6
Documentation/cmd-set-project-parent.txt
View File

@ -20,7 +20,11 @@ default this is `All-Projects`. This command sets
the project to inherit through another one.
== ACCESS
Caller must be a member of the privileged 'Administrators' group.
Caller must be a member of the privileged 'Administrators' group
or, if
link:config-gerrit.html#receive.allowProjectOwnersToChangeParent[receive.allowProjectOwnersToChangeParent]
is enabled, be a project owner of the projects that is getting their
parent updated.
== SCRIPTING
This command is intended to be used in scripts.

11
Documentation/config-gerrit.txt
View File

@ -3670,6 +3670,17 @@ Setting to false to skip the check can decrease latency during push.
+
Default is true.
[[receive.allowProjectOwnersToChangeParent]]receive.allowProjectOwnersToChangeParent::
+
If true, Gerrit will allow project owners to change the parent of a project.
+
By default only Gerrit administrators are allowed to change the parent
of a project. By allowing project owners to change parents, it may
allow the owner to circumvent certain enforced rules (like important
BLOCK rules).
+
Default is false.
[[receive.checkReferencedObjectsAreReachable]]receive.checkReferencedObjectsAreReachable::
+
If set to true, Gerrit will validate that all referenced objects that

27
java/com/google/gerrit/server/git/receive/ReceiveCommits.java
View File

@ -361,6 +361,7 @@ class ReceiveCommits {
private final ReceivePack receivePack;
// Immutable fields derived from constructor arguments.
private final boolean allowProjectOwnersToChangeParent;
private final boolean allowPushToRefsChanges;
private final LabelTypes labelTypes;
private final NoteMap rejectCommits;
@ -505,6 +506,9 @@ class ReceiveCommits {
updateGroups = new ArrayList<>();
validCommits = new HashSet<>();
this.allowProjectOwnersToChangeParent =
cfg.getBoolean("receive", "allowProjectOwnersToChangeParent", false);
// Other settings populated during processing.
newChangeForAllNotInTarget =
projectState.is(BooleanProjectConfig.CREATE_NEW_CHANGE_FOR_ALL_NOT_IN_TARGET);
@ -1113,11 +1117,24 @@ class ReceiveCommits {
}
} else {
if (!oldParent.equals(newParent)) {
try {
permissionBackend.user(user).check(GlobalPermission.ADMINISTRATE_SERVER);
} catch (AuthException e) {
reject(cmd, "invalid project configuration: only Gerrit admin can set parent");
return;
if (allowProjectOwnersToChangeParent) {
try {
permissionBackend
.user(user)
.project(project.getNameKey())
.check(ProjectPermission.WRITE_CONFIG);
} catch (AuthException e) {
reject(
cmd, "invalid project configuration: only project owners can set parent");
return;
}
} else {
try {
permissionBackend.user(user).check(GlobalPermission.ADMINISTRATE_SERVER);
} catch (AuthException e) {
reject(cmd, "invalid project configuration: only Gerrit admin can set parent");
return;
}
}
}

41
java/com/google/gerrit/server/git/validators/MergeValidators.java
View File

@ -32,12 +32,14 @@ import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.account.AccountProperties;
import com.google.gerrit.server.config.AllProjectsName;
import com.google.gerrit.server.config.AllUsersName;
import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.config.PluginConfig;
import com.google.gerrit.server.config.ProjectConfigEntry;
import com.google.gerrit.server.git.CodeReviewCommit;
import com.google.gerrit.server.permissions.GlobalPermission;
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.permissions.ProjectPermission;
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.project.ProjectConfig;
import com.google.gerrit.server.project.ProjectState;
@ -48,6 +50,7 @@ import com.google.inject.Provider;
import java.io.IOException;
import java.util.List;
import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.lib.Config;
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.revwalk.RevWalk;
@ -114,12 +117,17 @@ public class MergeValidators {
"Change contains a project configuration that changes the parent"
+ " project.\n"
+ "The change must be submitted by a Gerrit administrator.";
private static final String SET_BY_OWNER =
"Change contains a project configuration that changes the parent"
+ " project.\n"
+ "The change must be submitted by a Gerrit administrator or the project owner.";
private final AllProjectsName allProjectsName;
private final AllUsersName allUsersName;
private final ProjectCache projectCache;
private final PermissionBackend permissionBackend;
private final DynamicMap<ProjectConfigEntry> pluginConfigEntries;
private final boolean allowProjectOwnersToChangeParent;
public interface Factory {
ProjectConfigValidator create();
@ -131,12 +139,15 @@ public class MergeValidators {
AllUsersName allUsersName,
ProjectCache projectCache,
PermissionBackend permissionBackend,
DynamicMap<ProjectConfigEntry> pluginConfigEntries) {
DynamicMap<ProjectConfigEntry> pluginConfigEntries,
@GerritServerConfig Config config) {
this.allProjectsName = allProjectsName;
this.allUsersName = allUsersName;
this.projectCache = projectCache;
this.permissionBackend = permissionBackend;
this.pluginConfigEntries = pluginConfigEntries;
this.allowProjectOwnersToChangeParent =
config.getBoolean("receive", "allowProjectOwnersToChangeParent", false);
}
@Override
@ -162,13 +173,27 @@ public class MergeValidators {
}
} else {
if (!oldParent.equals(newParent)) {
try {
permissionBackend.user(caller).check(GlobalPermission.ADMINISTRATE_SERVER);
} catch (AuthException e) {
throw new MergeValidationException(SET_BY_ADMIN);
} catch (PermissionBackendException e) {
logger.atWarning().withCause(e).log("Cannot check ADMINISTRATE_SERVER");
throw new MergeValidationException("validation unavailable");
if (!allowProjectOwnersToChangeParent) {
try {
permissionBackend.user(caller).check(GlobalPermission.ADMINISTRATE_SERVER);
} catch (AuthException e) {
throw new MergeValidationException(SET_BY_ADMIN);
} catch (PermissionBackendException e) {
logger.atWarning().withCause(e).log("Cannot check ADMINISTRATE_SERVER");
throw new MergeValidationException("validation unavailable");
}
} else {
try {
permissionBackend
.user(caller)
.project(destProject.getNameKey())
.check(ProjectPermission.WRITE_CONFIG);
} catch (AuthException e) {
throw new MergeValidationException(SET_BY_OWNER);
} catch (PermissionBackendException e) {
logger.atWarning().withCause(e).log("Cannot check WRITE_CONFIG");
throw new MergeValidationException("validation unavailable");
}
}
if (allUsersName.equals(destProject.getNameKey())
&& !allProjectsName.equals(newParent)) {

15
java/com/google/gerrit/server/restapi/project/SetParent.java
View File

@ -30,10 +30,12 @@ import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.config.AllProjectsName;
import com.google.gerrit.server.config.AllUsersName;
import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.git.meta.MetaDataUpdate;
import com.google.gerrit.server.permissions.GlobalPermission;
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.permissions.ProjectPermission;
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.project.ProjectConfig;
import com.google.gerrit.server.project.ProjectResource;
@ -43,6 +45,7 @@ import com.google.inject.Singleton;
import java.io.IOException;
import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.errors.RepositoryNotFoundException;
import org.eclipse.jgit.lib.Config;
@Singleton
public class SetParent implements RestModifyView<ProjectResource, ParentInput> {
@ -51,6 +54,7 @@ public class SetParent implements RestModifyView<ProjectResource, ParentInput> {
private final MetaDataUpdate.Server updateFactory;
private final AllProjectsName allProjects;
private final AllUsersName allUsers;
private final boolean allowProjectOwnersToChangeParent;
@Inject
SetParent(
@ -58,12 +62,15 @@ public class SetParent implements RestModifyView<ProjectResource, ParentInput> {
PermissionBackend permissionBackend,
MetaDataUpdate.Server updateFactory,
AllProjectsName allProjects,
AllUsersName allUsers) {
AllUsersName allUsers,
@GerritServerConfig Config config) {
this.cache = cache;
this.permissionBackend = permissionBackend;
this.updateFactory = updateFactory;
this.allProjects = allProjects;
this.allUsers = allUsers;
this.allowProjectOwnersToChangeParent =
config.getBoolean("receive", "allowProjectOwnersToChangeParent", false);
}
@Override
@ -114,7 +121,11 @@ public class SetParent implements RestModifyView<ProjectResource, ParentInput> {
throws AuthException, ResourceConflictException, UnprocessableEntityException,
PermissionBackendException, BadRequestException {
if (checkIfAdmin) {
permissionBackend.user(user).check(GlobalPermission.ADMINISTRATE_SERVER);
if (allowProjectOwnersToChangeParent) {
permissionBackend.user(user).project(project).check(ProjectPermission.WRITE_CONFIG);
} else {
permissionBackend.user(user).check(GlobalPermission.ADMINISTRATE_SERVER);
}
}
if (project.equals(allUsers) && !allProjects.get().equals(newParent)) {

2
java/com/google/gerrit/sshd/commands/DefaultCommandModule.java
View File

@ -114,7 +114,7 @@ public class DefaultCommandModule extends CommandModule {
command(gerrit, SetMembersCommand.class);
command(gerrit, CreateBranchCommand.class);
command(gerrit, SetAccountCommand.class);
command(gerrit, AdminSetParent.class);
command(gerrit, SetParentCommand.class);
command(testSubmit, TestSubmitRuleCommand.class);
command(testSubmit, TestSubmitTypeCommand.class);

101
java/com/google/gerrit/sshd/commands/AdminSetParent.java → java/com/google/gerrit/sshd/commands/SetParentCommand.java
View File

@ -16,41 +16,36 @@ package com.google.gerrit.sshd.commands;
import static java.util.stream.Collectors.toList;
import com.google.common.flogger.FluentLogger;
import com.google.gerrit.common.data.GlobalCapability;
import com.google.gerrit.extensions.annotations.RequiresCapability;
import com.google.gerrit.extensions.api.projects.ParentInput;
import com.google.gerrit.extensions.common.ProjectInfo;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.server.config.AllProjectsName;
import com.google.gerrit.server.git.meta.MetaDataUpdate;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.project.ProjectConfig;
import com.google.gerrit.server.project.ProjectResource;
import com.google.gerrit.server.project.ProjectState;
import com.google.gerrit.server.restapi.project.ListChildProjects;
import com.google.gerrit.server.restapi.project.SetParent;
import com.google.gerrit.sshd.CommandMetaData;
import com.google.gerrit.sshd.SshCommand;
import com.google.inject.Inject;
import java.io.IOException;
import java.util.ArrayList;
import java.util.Collections;
import java.util.HashSet;
import java.util.List;
import java.util.Set;
import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.errors.RepositoryNotFoundException;
import org.kohsuke.args4j.Argument;
import org.kohsuke.args4j.Option;
@RequiresCapability(GlobalCapability.ADMINISTRATE_SERVER)
@CommandMetaData(
name = "set-project-parent",
description = "Change the project permissions are inherited from")
final class AdminSetParent extends SshCommand {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
final class SetParentCommand extends SshCommand {
@Option(
name = "--parent",
aliases = {"-p"},
@ -80,14 +75,18 @@ final class AdminSetParent extends SshCommand {
@Inject private ProjectCache projectCache;
@Inject private MetaDataUpdate.User metaDataUpdateFactory;
@Inject private AllProjectsName allProjectsName;
@Inject private ListChildProjects listChildProjects;
@Inject private SetParent setParent;
private Project.NameKey newParentKey;
private static ParentInput parentInput(String parent) {
ParentInput input = new ParentInput();
input.parent = parent;
return input;
}
@Override
protected void run() throws Failure {
if (oldParent == null && children.isEmpty()) {
@ -100,25 +99,9 @@ final class AdminSetParent extends SshCommand {
}
final StringBuilder err = new StringBuilder();
final Set<Project.NameKey> grandParents = new HashSet<>();
grandParents.add(allProjectsName);
if (newParent != null) {
newParentKey = newParent.getProject().getNameKey();
// Catalog all grandparents of the "parent", we want to
// catch a cycle in the parent pointers before it occurs.
//
Project.NameKey gp = newParent.getProject().getParent();
while (gp != null && grandParents.add(gp)) {
final ProjectState s = projectCache.get(gp);
if (s != null) {
gp = s.getProject().getParent();
} else {
break;
}
}
}
final List<Project.NameKey> childProjects =
@ -135,47 +118,19 @@ final class AdminSetParent extends SshCommand {
for (Project.NameKey nameKey : childProjects) {
final String name = nameKey.get();
if (allProjectsName.equals(nameKey)) {
// Don't allow the wild card project to have a parent.
//
err.append("error: Cannot set parent of '").append(name).append("'\n");
continue;
}
if (grandParents.contains(nameKey) || nameKey.equals(newParentKey)) {
// Try to avoid creating a cycle in the parent pointers.
//
err.append("error: Cycle exists between '")
.append(name)
.append("' and '")
.append(newParentKey != null ? newParentKey.get() : allProjectsName.get())
.append("'\n");
continue;
}
try (MetaDataUpdate md = metaDataUpdateFactory.create(nameKey)) {
ProjectConfig config = ProjectConfig.read(md);
config.getProject().setParentName(newParentKey);
md.setMessage(
"Inherit access from "
+ (newParentKey != null ? newParentKey.get() : allProjectsName.get())
+ "\n");
config.commit(md);
} catch (RepositoryNotFoundException notFound) {
err.append("error: Project ").append(name).append(" not found\n");
} catch (IOException | ConfigInvalidException e) {
final String msg = "Cannot update project " + name;
logger.atSevere().withCause(e).log(msg);
err.append("error: ").append(msg).append("\n");
}
ProjectState project = projectCache.get(nameKey);
try {
projectCache.evict(nameKey);
} catch (IOException e) {
final String msg = "Cannot reindex project: " + name;
logger.atSevere().withCause(e).log(msg);
err.append("error: ").append(msg).append("\n");
setParent.apply(new ProjectResource(project, user), parentInput(newParentKey.get()));
} catch (AuthException e) {
err.append("error: insuffient access rights to change parent of '")
.append(name)
.append("'\n");
} catch (ResourceConflictException | ResourceNotFoundException | BadRequestException e) {
err.append("error: ").append(e.getMessage()).append("'\n");
} catch (UnprocessableEntityException | IOException e) {
throw new Failure(1, "failure in request", e);
} catch (PermissionBackendException e) {
throw new Failure(1, "permissions unavailable", e);
}
}

37
javatests/com/google/gerrit/acceptance/api/project/SetParentIT.java
View File

@ -17,13 +17,16 @@ package com.google.gerrit.acceptance.api.project;
import static com.google.common.truth.Truth.assertThat;
import com.google.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.acceptance.GerritConfig;
import com.google.gerrit.acceptance.NoHttpd;
import com.google.gerrit.common.data.Permission;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.server.config.AllProjectsNameProvider;
import com.google.gerrit.server.group.SystemGroupBackend;
import org.junit.Test;
@NoHttpd
@ -37,6 +40,40 @@ public class SetParentIT extends AbstractDaemonTest {
gApi.projects().name(project.get()).parent(parent);
}
@Test
@GerritConfig(name = "receive.allowProjectOwnersToChangeParent", value = "true")
public void setParentNotAllowedForNonOwners() throws Exception {
String parent = createProject("parent", null, true).get();
setApiUser(user);
exception.expect(AuthException.class);
gApi.projects().name(project.get()).parent(parent);
}
@Test
@GerritConfig(name = "receive.allowProjectOwnersToChangeParent", value = "true")
public void setParentAllowedByAdminWhenAllowProjectOwnersEnabled() throws Exception {
String parent = createProject("parent", null, true).get();
gApi.projects().name(project.get()).parent(parent);
assertThat(gApi.projects().name(project.get()).parent()).isEqualTo(parent);
// When the parent name is not explicitly set, it should be
// set to "All-Projects".
gApi.projects().name(project.get()).parent(null);
assertThat(gApi.projects().name(project.get()).parent())
.isEqualTo(AllProjectsNameProvider.DEFAULT);
}
@Test
@GerritConfig(name = "receive.allowProjectOwnersToChangeParent", value = "true")
public void setParentAllowedForOwners() throws Exception {
String parent = createProject("parent", null, true).get();
setApiUser(user);
grant(project, "refs/*", Permission.OWNER, false, SystemGroupBackend.REGISTERED_USERS);
gApi.projects().name(project.get()).parent(parent);
assertThat(gApi.projects().name(project.get()).parent()).isEqualTo(parent);
}
@Test
public void setParent() throws Exception {
String parent = createProject("parent", null, true).get();