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
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 513c4ab..b12750d 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 @@
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 c1d5cdc..983549d 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 @@
}
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 6daa2c8..3c6fd71 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 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<String, Ref> 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 @@
"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<String, Ref> unfilteredRefs) throws IOException {
+ VisibleRefFilter filter =
+ new VisibleRefFilter(tagCache, changeCache, repo, this, db, true);
+ Map<String, Ref> 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 66bb9b7..f13b411 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 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 @@
/** 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 @@
* @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 @@
} 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 @@
}
}
+ private boolean isMergedIntoBranchOrTag(ReviewDb db, RevWalk rw,
+ RevCommit commit) {
+ try {
+ Repository repo = projectControl.openRepository();
+ try {
+ Map<String, Ref> 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.
