From c3bd56f6e5f34ad26908ffadb1a55bffcd037b09 Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Wed, 25 Oct 2017 13:33:17 -0400 Subject: [PATCH] CreateRefControl: Pass explicit user When used from ReceiveCommits, the injected Provider always returns an anonymous user. While this behavior may not be intended, it at least is not new, dating back at least to 2.14. The simplest fix to avoid depending on the injected provider is to pass an explicit user. Other approaches (e.g. introducing a factory) may result in cleaner code overall, but are more invasive and thus less suitable for a stable branch. I eyeballed the diff of ReceiveCommits between stable-2.14 and stable-2.15 to see if there were any other permission checks which might be implicitly depending on the Provider, and I did not find any. CreateRefControl is kind of a one-off, and the remainder of the checks are handled by the PermissionBackend that is initialized in the ReceiveCommits constructor with the correct user. Bug: Issue 7385 Change-Id: I60dc41be1a91f26e1dfc3493997e72b837b881b0 --- .../server/git/receive/ReceiveCommits.java | 5 +++- .../gerrit/server/project/CreateBranch.java | 2 +- .../server/project/CreateRefControl.java | 24 ++++++++++++------- 3 files changed, 20 insertions(+), 11 deletions(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/receive/ReceiveCommits.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/receive/ReceiveCommits.java index bb84425bea..3e025525fb 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/receive/ReceiveCommits.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/receive/ReceiveCommits.java @@ -154,6 +154,7 @@ import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; import com.google.inject.Provider; import com.google.inject.assistedinject.Assisted; +import com.google.inject.util.Providers; import java.io.IOException; import java.io.StringWriter; import java.util.ArrayList; @@ -1014,7 +1015,9 @@ class ReceiveCommits { Branch.NameKey branch = new Branch.NameKey(project.getName(), cmd.getRefName()); try { - createRefControl.checkCreateRef(rp.getRepository(), branch, obj); + // Must pass explicit user instead of injecting a provider into CreateRefControl, since + // Provider within ReceiveCommits will always return anonymous. + createRefControl.checkCreateRef(Providers.of(user), rp.getRepository(), branch, obj); } catch (AuthException denied) { reject(cmd, "prohibited by Gerrit: " + denied.getMessage()); return; diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/CreateBranch.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/CreateBranch.java index f15571cf65..0a648d9a71 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/project/CreateBranch.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/CreateBranch.java @@ -121,7 +121,7 @@ public class CreateBranch implements RestModifyView user; @Inject - CreateRefControl( - PermissionBackend permissionBackend, ProjectCache projectCache, Provider user) { + CreateRefControl(PermissionBackend permissionBackend, ProjectCache projectCache) { this.permissionBackend = permissionBackend; this.projectCache = projectCache; - this.user = user; } /** * Checks whether the {@link CurrentUser} can create a new Git ref. * + * @param user the user performing the operation * @param repo repository on which user want to create * @param branch the branch the new {@link RevObject} should be created on * @param object the object the user will start the reference with * @throws AuthException if creation is denied; the message explains the denial. * @throws PermissionBackendException on failure of permission checks. */ - public void checkCreateRef(Repository repo, Branch.NameKey branch, RevObject object) + public void checkCreateRef( + Provider user, + Repository repo, + Branch.NameKey branch, + RevObject object) throws AuthException, PermissionBackendException, NoSuchProjectException, IOException { ProjectState ps = projectCache.checkedGet(branch.getParentKey()); if (ps == null) { @@ -73,7 +75,7 @@ public class CreateRefControl { PermissionBackend.ForRef perm = permissionBackend.user(user).ref(branch); if (object instanceof RevCommit) { perm.check(RefPermission.CREATE); - checkCreateCommit(repo, (RevCommit) object, ps, perm); + checkCreateCommit(user, repo, (RevCommit) object, ps, perm); } else if (object instanceof RevTag) { RevTag tag = (RevTag) object; try (RevWalk rw = new RevWalk(repo)) { @@ -93,9 +95,9 @@ public class CreateRefControl { RevObject target = tag.getObject(); if (target instanceof RevCommit) { - checkCreateCommit(repo, (RevCommit) target, ps, perm); + checkCreateCommit(user, repo, (RevCommit) target, ps, perm); } else { - checkCreateRef(repo, branch, target); + checkCreateRef(user, repo, branch, target); } // If the tag has a PGP signature, allow a lower level of permission @@ -116,7 +118,11 @@ public class CreateRefControl { * new commit to the repository. */ private void checkCreateCommit( - Repository repo, RevCommit commit, ProjectState projectState, PermissionBackend.ForRef forRef) + Provider user, + Repository repo, + RevCommit commit, + ProjectState projectState, + PermissionBackend.ForRef forRef) throws AuthException, PermissionBackendException { try { // If the user has update (push) permission, they can create the ref regardless