opendev/gerrit
Code Issues Proposed changes

New option: receive.allowProjectOwnersToChangeParent

Browse Source 
If true, Gerrit will allow project owners to update project parent.

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).

Though for some organisations, this rule is not as beneficial and
introduces costs by increased lead times of project configuration
changes.

Change-Id: Iefa35b7efad3b22a456ab5c0162937b985a245a1
This commit is contained in:
Gustaf Lundh
2018-06-25 16:15:21 +02:00
parent 42e75dfea0
commit fd5fcf4058
5 changed files with 116 additions and 15 deletions
Download Patch File Download Diff File Expand all files Collapse all files

11
Documentation/config-gerrit.txt
View File

@@ -3667,6 +3667,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

@@ -360,6 +360,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;
@@ -519,6 +520,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);
@@ -1051,11 +1055,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)) {

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();