opendev/gerrit
Code Issues Proposed changes

Fix deletion of meta branches through Delete Branch API

Browse Source 
Trying to delete a branch that doesn't start with 'refs/heads/' failed
with a NullPointerException [1]. This was because DeleteRef prepended
the 'refs/heads/' prefix for all refs that didn't start with
'refs/heads/'. Hence a meta ref like 'refs/meta/foo' became
'refs/heads/refs/meta/foo' which didn't exist in the repository and
hence it couldn't be resolved to a SHA1. Normally the BranchesCollection
ensures that the branch on which the Delete Branch REST endpoint is
invoked exists, but since the BranchesCollection used the correct ref
name ('refs/meta/foo') it did find the branch.

The prefix in DeleteRef should really be a default prefix that is only
prepended to non-qualified refs (refs that don't start with 'refs/').

[1]
java.lang.NullPointerException
  at com.google.gerrit.server.project.DeleteRef.deleteSingleRef(DeleteRef.java:128)
  at com.google.gerrit.server.project.DeleteRef.delete(DeleteRef.java:112)
  at com.google.gerrit.server.project.DeleteBranch.apply(DeleteBranch.java:64)
  at com.google.gerrit.server.project.DeleteBranch.apply(DeleteBranch.java:1)
  at com.google.gerrit.httpd.restapi.RestApiServlet.service(RestApiServlet.java:405)
  ...

Change-Id: I581a69b5cf7abe917a1f394155d865d3eece523f
Signed-off-by: Edwin Kempin <ekempin@google.com>
This commit is contained in:
Edwin Kempin 2017-11-10 10:18:10 +01:00
parent d00515e898
commit 053f563502
3 changed files with 73 additions and 40 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

77
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/DeleteBranchIT.java
View File

@ -25,45 +25,46 @@ import com.google.gerrit.common.data.Permission;
import com.google.gerrit.extensions.api.projects.BranchApi;
import com.google.gerrit.extensions.api.projects.BranchInput;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.IdString;
import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
import com.google.gerrit.extensions.restapi.Url;
import com.google.gerrit.reviewdb.client.Branch;
import com.google.gerrit.reviewdb.client.RefNames;
import org.junit.Before;
import org.junit.Test;
public class DeleteBranchIT extends AbstractDaemonTest {
private Branch.NameKey branch;
private Branch.NameKey testBranch;
@Before
public void setUp() throws Exception {
project = createProject(name("p"));
branch = new Branch.NameKey(project, "test");
branch().create(new BranchInput());
testBranch = new Branch.NameKey(project, "test");
branch(testBranch).create(new BranchInput());
}
@Test
public void deleteBranch_Forbidden() throws Exception {
setApiUser(user);
assertDeleteForbidden();
assertDeleteForbidden(testBranch);
}
@Test
public void deleteBranchByAdmin() throws Exception {
assertDeleteSucceeds();
assertDeleteSucceeds(testBranch);
}
@Test
public void deleteBranchByProjectOwner() throws Exception {
grantOwner();
setApiUser(user);
assertDeleteSucceeds();
assertDeleteSucceeds(testBranch);
}
@Test
public void deleteBranchByAdminForcePushBlocked() throws Exception {
blockForcePush();
assertDeleteSucceeds();
assertDeleteSucceeds(testBranch);
}
@Test
@ -71,44 +72,57 @@ public class DeleteBranchIT extends AbstractDaemonTest {
grantOwner();
blockForcePush();
setApiUser(user);
assertDeleteForbidden();
assertDeleteForbidden(testBranch);
}
@Test
public void deleteBranchByUserWithForcePushPermission() throws Exception {
grantForcePush();
setApiUser(user);
assertDeleteSucceeds();
assertDeleteSucceeds(testBranch);
}
@Test
public void deleteBranchByUserWithDeletePermission() throws Exception {
grantDelete();
setApiUser(user);
assertDeleteSucceeds();
assertDeleteSucceeds(testBranch);
}
@Test
public void deleteBranchByRestWithoutRefsHeadsPrefix() throws Exception {
grantDelete();
String ref = branch.getShortName();
String ref = testBranch.getShortName();
assertThat(ref).doesNotMatch(R_HEADS);
assertDeleteByRestSucceeds(ref);
assertDeleteByRestSucceeds(testBranch, ref);
}
@Test
public void deleteBranchByRestWithEncodedFullName() throws Exception {
public void deleteBranchByRestWithFullName() throws Exception {
grantDelete();
assertDeleteByRestSucceeds(Url.encode(branch.get()));
assertDeleteByRestSucceeds(testBranch, testBranch.get());
}
@Test
public void deleteBranchByRestFailsWithUnencodedFullName() throws Exception {
grantDelete();
RestResponse r =
userRestSession.delete("/projects/" + project.get() + "/branches/" + branch.get());
userRestSession.delete("/projects/" + project.get() + "/branches/" + testBranch.get());
r.assertNotFound();
branch().get();
branch(testBranch).get();
}
@Test
public void deleteMetaBranch() throws Exception {
String metaRef = RefNames.REFS_META + "foo";
allow(metaRef, Permission.CREATE, REGISTERED_USERS);
allow(metaRef, Permission.PUSH, REGISTERED_USERS);
Branch.NameKey metaBranch = new Branch.NameKey(project, metaRef);
branch(metaBranch).create(new BranchInput());
grantDelete();
assertDeleteByRestSucceeds(metaBranch, metaRef);
}
private void blockForcePush() throws Exception {
@ -127,31 +141,36 @@ public class DeleteBranchIT extends AbstractDaemonTest {
allow("refs/*", Permission.OWNER, REGISTERED_USERS);
}
private BranchApi branch() throws Exception {
private BranchApi branch(Branch.NameKey branch) throws Exception {
return gApi.projects().name(branch.getParentKey().get()).branch(branch.get());
}
private void assertDeleteByRestSucceeds(String ref) throws Exception {
RestResponse r = userRestSession.delete("/projects/" + project.get() + "/branches/" + ref);
private void assertDeleteByRestSucceeds(Branch.NameKey branch, String ref) throws Exception {
RestResponse r =
userRestSession.delete(
"/projects/"
+ IdString.fromDecoded(project.get()).encoded()
+ "/branches/"
+ IdString.fromDecoded(ref).encoded());
r.assertNoContent();
exception.expect(ResourceNotFoundException.class);
branch().get();
branch(branch).get();
}
private void assertDeleteSucceeds() throws Exception {
assertThat(branch().get().canDelete).isTrue();
String branchRev = branch().get().revision;
branch().delete();
private void assertDeleteSucceeds(Branch.NameKey branch) throws Exception {
assertThat(branch(branch).get().canDelete).isTrue();
String branchRev = branch(branch).get().revision;
branch(branch).delete();
eventRecorder.assertRefUpdatedEvents(
project.get(), branch.get(), null, branchRev, branchRev, null);
exception.expect(ResourceNotFoundException.class);
branch().get();
branch(branch).get();
}
private void assertDeleteForbidden() throws Exception {
assertThat(branch().get().canDelete).isNull();
private void assertDeleteForbidden(Branch.NameKey branch) throws Exception {
assertThat(branch(branch).get().canDelete).isNull();
exception.expect(AuthException.class);
exception.expectMessage("delete not permitted");
branch().delete();
branch(branch).delete();
}
}

31
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/DeleteBranchesIT.java
View File

@ -15,15 +15,17 @@
package com.google.gerrit.acceptance.rest.project;
import static com.google.common.truth.Truth.assertThat;
import static com.google.gerrit.acceptance.rest.project.RefAssert.assertRefNames;
import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS;
import static java.util.stream.Collectors.toList;
import static org.eclipse.jgit.lib.Constants.R_HEADS;
import static org.eclipse.jgit.lib.Constants.R_REFS;
import static org.junit.Assert.fail;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.Lists;
import com.google.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.acceptance.NoHttpd;
import com.google.gerrit.common.data.Permission;
import com.google.gerrit.extensions.api.projects.BranchInput;
import com.google.gerrit.extensions.api.projects.DeleteBranchesInput;
import com.google.gerrit.extensions.api.projects.ProjectApi;
@ -32,6 +34,7 @@ import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.reviewdb.client.RefNames;
import java.util.HashMap;
import java.util.List;
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.revwalk.RevCommit;
import org.junit.Before;
import org.junit.Test;
@ -39,10 +42,12 @@ import org.junit.Test;
@NoHttpd
public class DeleteBranchesIT extends AbstractDaemonTest {
private static final ImmutableList<String> BRANCHES =
ImmutableList.of("refs/heads/test-1", "refs/heads/test-2", "test-3");
ImmutableList.of("refs/heads/test-1", "refs/heads/test-2", "test-3", "refs/meta/foo");
@Before
public void setUp() throws Exception {
allow("refs/*", Permission.CREATE, REGISTERED_USERS);
allow("refs/*", Permission.PUSH, REGISTERED_USERS);
for (String name : BRANCHES) {
project().branch(name).create(new BranchInput());
}
@ -55,7 +60,7 @@ public class DeleteBranchesIT extends AbstractDaemonTest {
DeleteBranchesInput input = new DeleteBranchesInput();
input.branches = BRANCHES;
project().deleteBranches(input);
assertBranchesDeleted();
assertBranchesDeleted(BRANCHES);
assertRefUpdatedEvents(initialRevisions);
}
@ -88,7 +93,7 @@ public class DeleteBranchesIT extends AbstractDaemonTest {
.hasMessageThat()
.isEqualTo(errorMessageForBranches(ImmutableList.of("refs/heads/does-not-exist")));
}
assertBranchesDeleted();
assertBranchesDeleted(BRANCHES);
}
@Test
@ -107,7 +112,7 @@ public class DeleteBranchesIT extends AbstractDaemonTest {
.hasMessageThat()
.isEqualTo(errorMessageForBranches(ImmutableList.of("refs/heads/does-not-exist")));
}
assertBranchesDeleted();
assertBranchesDeleted(BRANCHES);
}
@Test
@ -164,7 +169,7 @@ public class DeleteBranchesIT extends AbstractDaemonTest {
}
private String prefixRef(String ref) {
return ref.startsWith(R_HEADS) ? ref : R_HEADS + ref;
return ref.startsWith(R_REFS) ? ref : R_HEADS + ref;
}
private ProjectApi project() throws Exception {
@ -174,10 +179,18 @@ public class DeleteBranchesIT extends AbstractDaemonTest {
private void assertBranches(List<String> branches) throws Exception {
List<String> expected = Lists.newArrayList("HEAD", RefNames.REFS_CONFIG, "refs/heads/master");
expected.addAll(branches.stream().map(b -> prefixRef(b)).collect(toList()));
assertRefNames(expected, project().branches().get());
try (Repository repo = repoManager.openRepository(project)) {
for (String branch : expected) {
assertThat(repo.exactRef(branch)).isNotNull();
}
}
}
private void assertBranchesDeleted() throws Exception {
assertBranches(ImmutableList.<String>of());
private void assertBranchesDeleted(List<String> branches) throws Exception {
try (Repository repo = repoManager.openRepository(project)) {
for (String branch : branches) {
assertThat(repo.exactRef(branch)).isNull();
}
}
}
}

5
gerrit-server/src/main/java/com/google/gerrit/server/project/DeleteRef.java
View File

@ -16,6 +16,7 @@ package com.google.gerrit.server.project;
import static java.lang.String.format;
import static java.util.stream.Collectors.toList;
import static org.eclipse.jgit.lib.Constants.R_REFS;
import static org.eclipse.jgit.lib.Constants.R_TAGS;
import static org.eclipse.jgit.transport.ReceiveCommand.Type.DELETE;
@ -118,7 +119,7 @@ public class DeleteRef {
private void deleteSingleRef(Repository r) throws IOException, ResourceConflictException {
String ref = refsToDelete.get(0);
if (prefix != null && !ref.startsWith(prefix)) {
if (prefix != null && !ref.startsWith(R_REFS)) {
ref = prefix + ref;
}
RefUpdate.Result result;
@ -186,7 +187,7 @@ public class DeleteRef {
? refsToDelete
: refsToDelete
.stream()
.map(ref -> ref.startsWith(prefix) ? ref : prefix + ref)
.map(ref -> ref.startsWith(R_REFS) ? ref : prefix + ref)
.collect(toList());
for (String ref : refs) {
batchUpdate.addCommand(createDeleteCommand(resource, r, ref));