From a0bcc9fcb9f3d5366543f282c431b43c5695e195 Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Wed, 6 Aug 2014 17:00:57 -0700 Subject: [PATCH] RefControl: Use consistent rev-walking logic in canCreate The intent of I4e580e2f was to allow creation of new branches along with pushing new commits only if the user has push permissions, but the implementation was buggy. For this change, we define "new commits" as anything not reachable from a visible branch or tag (which excludes, for example, commits of visible but not merged patch sets). We previously defined a more correct walk in ProjectControl for determining if a commit is merged into a visible branch, so reuse that logic from RefControl. Change-Id: I2e6181de5bf2a990ee58fb8e9d72b3370eba5cb8 --- .../gerrit/server/git/ReceiveCommits.java | 2 +- .../gerrit/server/project/CreateBranch.java | 2 +- .../gerrit/server/project/ProjectControl.java | 25 ++++++---- .../gerrit/server/project/RefControl.java | 46 +++++++++++++++---- 4 files changed, 55 insertions(+), 20 deletions(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java index 513c4abb46..b12750d305 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java @@ -992,7 +992,7 @@ public class ReceiveCommits { RefControl ctl = projectControl.controlForRef(cmd.getRefName()); rp.getRevWalk().reset(); - if (ctl.canCreate(db, rp.getRevWalk(), obj, allRefs.values().contains(obj))) { + if (ctl.canCreate(db, rp.getRevWalk(), obj)) { validateNewCommits(ctl, cmd); batch.addCommand(cmd); } else { 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 c1d5cdc2f5..983549d059 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 @@ -134,7 +134,7 @@ public class CreateBranch implements RestModifyView { } rw.reset(); - if (!refControl.canCreate(db.get(), rw, object, true)) { + if (!refControl.canCreate(db.get(), rw, object)) { throw new AuthException("Cannot create \"" + ref + "\""); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java index 6daa2c8bfb..3c6fd71a61 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java @@ -523,15 +523,9 @@ public class ProjectControl { public boolean canReadCommit(ReviewDb db, RevWalk rw, RevCommit commit) { try { - Repository repo = repoManager.openRepository(getProject().getNameKey()); + Repository repo = openRepository(); try { - VisibleRefFilter filter = - new VisibleRefFilter(tagCache, changeCache, repo, this, db, true); - Map visibleRefs = filter.filter(repo.getAllRefs(), true); - if (!visibleRefs.isEmpty() && IncludedInResolver.includedInOne( - repo, rw, commit, visibleRefs.values())) { - return true; - } + return isMergedIntoVisibleRef(repo, db, rw, commit, repo.getAllRefs()); } finally { repo.close(); } @@ -540,7 +534,20 @@ public class ProjectControl { "Cannot verify permissions to commit object %s in repository %s", commit.name(), getProject().getNameKey()); log.error(msg, e); + return false; } - return false; + } + + boolean isMergedIntoVisibleRef(Repository repo, ReviewDb db, RevWalk rw, + RevCommit commit, Map unfilteredRefs) throws IOException { + VisibleRefFilter filter = + new VisibleRefFilter(tagCache, changeCache, repo, this, db, true); + Map refs = filter.filter(unfilteredRefs, true); + return !refs.isEmpty() + && IncludedInResolver.includedInOne(repo, rw, commit, refs.values()); + } + + Repository openRepository() throws IOException { + return repoManager.openRepository(getProject().getNameKey()); } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/RefControl.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/RefControl.java index 66bb9b7d21..f13b4111df 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/project/RefControl.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/RefControl.java @@ -32,12 +32,16 @@ import com.google.gerrit.server.group.SystemGroupBackend; import dk.brics.automaton.RegExp; +import org.eclipse.jgit.lib.Constants; import org.eclipse.jgit.lib.PersonIdent; +import org.eclipse.jgit.lib.Ref; import org.eclipse.jgit.lib.Repository; import org.eclipse.jgit.revwalk.RevCommit; import org.eclipse.jgit.revwalk.RevObject; import org.eclipse.jgit.revwalk.RevTag; import org.eclipse.jgit.revwalk.RevWalk; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import java.io.IOException; import java.util.ArrayList; @@ -50,6 +54,8 @@ import java.util.Set; /** Manages access control for Git references (aka branches, tags). */ public class RefControl { + private static final Logger log = LoggerFactory.getLogger(RefControl.class); + private final ProjectControl projectControl; private final String refName; @@ -236,11 +242,9 @@ public class RefControl { * @param rw revision pool {@code object} was parsed in; must be reset before * calling this method. * @param object the object the user will start the reference with. - * @param existsOnServer the object exists on server or not. * @return {@code true} if the user specified can create a new Git ref */ - public boolean canCreate(ReviewDb db, RevWalk rw, RevObject object, - boolean existsOnServer) { + public boolean canCreate(ReviewDb db, RevWalk rw, RevObject object) { if (!canWrite()) { return false; } @@ -265,13 +269,15 @@ public class RefControl { } else if (!canPerform(Permission.CREATE)) { // No create permissions. return false; - } else if (!existsOnServer && canUpdate()) { - // If the object doesn't exist on the server, check that the user has - // push permissions. + } else if (canUpdate()) { + // If the user has push permissions, they can create the ref regardless + // of whether they are pushing any new objects along with the create. return true; - } else if (projectControl.canReadCommit(db, rw, (RevCommit) object)) { - // The object exists on the server and is readable by this user, so they - // do not require push permission create this ref. + } else if (isMergedIntoBranchOrTag(db, rw, (RevCommit) object)) { + // If the user has no push permissions, check whether the object is + // merged into a branch or tag readable by this user. If so, they are + // not effectively "pushing" more objects, so they can create the ref + // even if they don't have push permission. return true; } return false; @@ -313,6 +319,28 @@ public class RefControl { } } + private boolean isMergedIntoBranchOrTag(ReviewDb db, RevWalk rw, + RevCommit commit) { + try { + Repository repo = projectControl.openRepository(); + try { + Map refs = + repo.getRefDatabase().getRefs(Constants.R_HEADS); + refs.putAll(repo.getRefDatabase().getRefs(Constants.R_TAGS)); + return projectControl.isMergedIntoVisibleRef( + repo, db, rw, commit, refs); + } finally { + repo.close(); + } + } catch (IOException e) { + String msg = String.format( + "Cannot verify permissions to commit object %s in repository %s", + commit.name(), projectControl.getProject().getNameKey()); + log.error(msg, e); + } + return false; + } + /** * Determines whether the user can delete the Git ref controlled by this * object.