From 7ed07a74efceb3767ed8896c749eb7fdcf7a843d Mon Sep 17 00:00:00 2001 From: Gustaf Lundh Date: Mon, 20 Aug 2018 15:02:15 +0200 Subject: [PATCH] SSH: set-project-parent is no longer admin exclusive Config option "recieve.allowProjectOwnersToChangeParent" may allow project owners to reparent a project. We fix this by delegating the actual project re-parenting to the SetParent REST Api endpoint, which already have the neccessary permission checks in place. This also allows us to remove a lot of reduntant input sanity checks from the ssh command. Change-Id: I2da2eabd1f3bbaebcdfa5ba86524297181867282 --- Documentation/cmd-index.txt | 6 +- Documentation/cmd-set-project-parent.txt | 6 +- .../sshd/commands/DefaultCommandModule.java | 2 +- ...inSetParent.java => SetParentCommand.java} | 101 +++++------------- 4 files changed, 37 insertions(+), 78 deletions(-) rename java/com/google/gerrit/sshd/commands/{AdminSetParent.java => SetParentCommand.java} (66%) diff --git a/Documentation/cmd-index.txt b/Documentation/cmd-index.txt index 236240115f..05a8b9a980 100644 --- a/Documentation/cmd-index.txt +++ b/Documentation/cmd-index.txt @@ -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. diff --git a/Documentation/cmd-set-project-parent.txt b/Documentation/cmd-set-project-parent.txt index 6e2328c256..ec5a5c629b 100644 --- a/Documentation/cmd-set-project-parent.txt +++ b/Documentation/cmd-set-project-parent.txt @@ -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. diff --git a/java/com/google/gerrit/sshd/commands/DefaultCommandModule.java b/java/com/google/gerrit/sshd/commands/DefaultCommandModule.java index 0e36d5322a..1c857e4c28 100644 --- a/java/com/google/gerrit/sshd/commands/DefaultCommandModule.java +++ b/java/com/google/gerrit/sshd/commands/DefaultCommandModule.java @@ -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); diff --git a/java/com/google/gerrit/sshd/commands/AdminSetParent.java b/java/com/google/gerrit/sshd/commands/SetParentCommand.java similarity index 66% rename from java/com/google/gerrit/sshd/commands/AdminSetParent.java rename to java/com/google/gerrit/sshd/commands/SetParentCommand.java index 67ed098005..56ee371786 100644 --- a/java/com/google/gerrit/sshd/commands/AdminSetParent.java +++ b/java/com/google/gerrit/sshd/commands/SetParentCommand.java @@ -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 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 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); } }