Reject CREATE ReceiveCommands if the ref already exists
When users push tags that are not visible to them in the remote repo,
this results in sending a "CREATE" ReceiveCommand to the Gerrit server
although the ref/tag already exists in remote.
The command is processed normally resulting in a LOCK_FAILURE exception
that is confusing to the callers and may lead any investigation to the
wrong direction.
In this change, we check on the existence of the ref for "CREATE"
commands and reject the command early with a meaningful error message.
Google-Bug-Id: b/172186467
Change-Id: Iee225423520390b72ae84df421da93c4691bfadf
diff --git a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
index 89c025a..82b4b0b 100644
--- a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
+++ b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
@@ -1431,6 +1431,12 @@
private void parseCreate(ReceiveCommand cmd)
throws PermissionBackendException, NoSuchProjectException, IOException {
try (TraceTimer traceTimer = newTimer("parseCreate")) {
+ if (repo.resolve(cmd.getRefName()) != null) {
+ reject(
+ cmd,
+ String.format("Cannot create ref '%s' because it already exists.", cmd.getRefName()));
+ return;
+ }
RevObject obj;
try {
obj = receivePack.getRevWalk().parseAny(cmd.getNewId());
diff --git a/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java b/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java
index 2253202..ba1e1a7 100644
--- a/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java
+++ b/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java
@@ -1427,6 +1427,34 @@
}
@Test
+ public void pushToNonVisibleBranchIsRejected() throws Exception {
+ String master = "refs/heads/master";
+
+ projectOperations
+ .project(project)
+ .forUpdate()
+ .add(block(Permission.READ).ref(master).group(REGISTERED_USERS))
+ .update();
+
+ testRepo.branch("HEAD").commit().message("New Commit 1").insertChangeId().create();
+ // Since the branch is not visible to the caller, the command tries to create the ref resulting
+ // in the command being rejected because the ref already exists.
+ assertPushRejected(
+ pushHead(testRepo, master),
+ master,
+ "Cannot create ref 'refs/heads/master' because it already exists.");
+
+ projectOperations
+ .project(project)
+ .forUpdate()
+ .add(allow(Permission.READ).ref(master).group(REGISTERED_USERS))
+ .update();
+
+ testRepo.branch("HEAD").commit().message("New Commit 2").insertChangeId().create();
+ assertPushOk(pushHead(testRepo, master), master);
+ }
+
+ @Test
public void pushSameCommitTwiceUsingMagicBranchBaseOption() throws Exception {
projectOperations
.project(project)
diff --git a/javatests/com/google/gerrit/acceptance/rest/project/AbstractPushTag.java b/javatests/com/google/gerrit/acceptance/rest/project/AbstractPushTag.java
index 531357a..793f256 100644
--- a/javatests/com/google/gerrit/acceptance/rest/project/AbstractPushTag.java
+++ b/javatests/com/google/gerrit/acceptance/rest/project/AbstractPushTag.java
@@ -22,6 +22,7 @@
import static com.google.gerrit.acceptance.rest.project.AbstractPushTag.TagType.ANNOTATED;
import static com.google.gerrit.acceptance.rest.project.AbstractPushTag.TagType.LIGHTWEIGHT;
import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allow;
+import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.block;
import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.permissionKey;
import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS;
@@ -29,6 +30,7 @@
import com.google.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.acceptance.GitUtil;
import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
+import com.google.gerrit.common.Nullable;
import com.google.gerrit.entities.Permission;
import com.google.gerrit.entities.RefNames;
import com.google.gerrit.testing.ConfigSuite;
@@ -191,33 +193,57 @@
pushTagDeletion(tagName, Status.OK);
}
+ @Test
+ public void pushToNonVisibleTagIsRejected() throws Exception {
+ allowTagCreation();
+ allowPushOnRefsTags();
+
+ String tagName = pushTagForExistingCommit(Status.OK);
+
+ removeReadFromRefsTags();
+ removeReadFromRefsHeads();
+
+ pushTag(
+ tagName,
+ /* newCommit= */ true,
+ /* force= */ false,
+ Status.REJECTED_OTHER_REASON,
+ /* expectedMessage= */ String.format(
+ "Cannot create ref '%s' because it already exists.", tagRef(tagName)));
+ }
+
private String pushTagForExistingCommit(Status expectedStatus) throws Exception {
- return pushTag(null, false, false, expectedStatus);
+ return pushTag(null, false, false, expectedStatus, /* expectedMessage= */ null);
}
private String pushTagForNewCommit(Status expectedStatus) throws Exception {
- return pushTag(null, true, false, expectedStatus);
+ return pushTag(null, true, false, expectedStatus, /* expectedMessage= */ null);
}
private void fastForwardTagToExistingCommit(String tagName, Status expectedStatus)
throws Exception {
- pushTag(tagName, false, false, expectedStatus);
+ pushTag(tagName, false, false, expectedStatus, /* expectedMessage= */ null);
}
private void fastForwardTagToNewCommit(String tagName, Status expectedStatus) throws Exception {
- pushTag(tagName, true, false, expectedStatus);
+ pushTag(tagName, true, false, expectedStatus, /* expectedMessage= */ null);
}
private void forceUpdateTagToExistingCommit(String tagName, Status expectedStatus)
throws Exception {
- pushTag(tagName, false, true, expectedStatus);
+ pushTag(tagName, false, true, expectedStatus, /* expectedMessage= */ null);
}
private void forceUpdateTagToNewCommit(String tagName, Status expectedStatus) throws Exception {
- pushTag(tagName, true, true, expectedStatus);
+ pushTag(tagName, true, true, expectedStatus, /* expectedMessage= */ null);
}
- private String pushTag(String tagName, boolean newCommit, boolean force, Status expectedStatus)
+ private String pushTag(
+ String tagName,
+ boolean newCommit,
+ boolean force,
+ Status expectedStatus,
+ @Nullable String expectedMessage)
throws Exception {
if (force) {
testRepo.reset(initialHead);
@@ -256,6 +282,9 @@
: GitUtil.pushTag(testRepo, tagName, !createTag);
RemoteRefUpdate refUpdate = r.getRemoteUpdate(tagRef);
assertWithMessage(tagType.name()).that(refUpdate.getStatus()).isEqualTo(expectedStatus);
+ if (expectedMessage != null) {
+ assertWithMessage(tagType.name()).that(refUpdate.getMessage()).isEqualTo(expectedMessage);
+ }
return tagName;
}
@@ -352,6 +381,22 @@
.update();
}
+ private void removeReadFromRefsTags() throws Exception {
+ projectOperations
+ .project(project)
+ .forUpdate()
+ .add(block(Permission.READ).ref("refs/tags/*").group(REGISTERED_USERS))
+ .update();
+ }
+
+ private void removeReadFromRefsHeads() throws Exception {
+ projectOperations
+ .project(project)
+ .forUpdate()
+ .add(block(Permission.READ).ref("refs/heads/*").group(REGISTERED_USERS))
+ .update();
+ }
+
private void commit(PersonIdent ident, String subject) throws Exception {
commitBuilder().ident(ident).message(subject + " (" + System.nanoTime() + ")").create();
}
