From 053f563502b82fd6e5b265c3e37cb8d828b0c81b Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Fri, 10 Nov 2017 10:18:10 +0100 Subject: [PATCH] Fix deletion of meta branches through Delete Branch API 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 --- .../rest/project/DeleteBranchIT.java | 77 ++++++++++++------- .../rest/project/DeleteBranchesIT.java | 31 +++++--- .../gerrit/server/project/DeleteRef.java | 5 +- 3 files changed, 73 insertions(+), 40 deletions(-) diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/DeleteBranchIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/DeleteBranchIT.java index c8f2fef23f..ce30cd50b2 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/DeleteBranchIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/DeleteBranchIT.java @@ -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(); } } diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/DeleteBranchesIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/DeleteBranchesIT.java index dc18a58588..73b20aa057 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/DeleteBranchesIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/DeleteBranchesIT.java @@ -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 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 branches) throws Exception { List 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.of()); + private void assertBranchesDeleted(List branches) throws Exception { + try (Repository repo = repoManager.openRepository(project)) { + for (String branch : branches) { + assertThat(repo.exactRef(branch)).isNull(); + } + } } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/DeleteRef.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/DeleteRef.java index 759f1d8170..3b4638f56a 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/project/DeleteRef.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/DeleteRef.java @@ -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));