opendev/gerrit
Code Issues Proposed changes

DeleteRef: add missing permission check

Browse Source 
This commit adds the missing permission check when delete a single
branch through the "Delete Branches" endpoint. The permission
check is forgotten because DeleteRef#deleteSingleRef assumes the
caller has done it before. But "Delete Branches" didn't do that.

It's created just for this stable branch. For "master", it's fixed
by I6a7f48e4a9. Cherry-pick is hard because these classes have been
refactored to other packages.

Change-Id: I6431ffe5d2625c54dab23516680c7a9a81019108
This commit is contained in:
Changcheng Xiao 2018-07-25 13:35:10 +02:00
parent 1e75093ffe
commit a8aaf67b0a
2 changed files with 28 additions and 3 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

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

@ -27,6 +27,7 @@ import com.google.gerrit.acceptance.NoHttpd;
import com.google.gerrit.extensions.api.projects.BranchInput;
import com.google.gerrit.extensions.api.projects.DeleteBranchesInput;
import com.google.gerrit.extensions.api.projects.ProjectApi;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.reviewdb.client.RefNames;
@ -60,7 +61,24 @@ public class DeleteBranchesIT extends AbstractDaemonTest {
}
@Test
public void deleteBranchesForbidden() throws Exception {
public void deleteOneBranchWithoutPermissionForbidden() throws Exception {
ImmutableList<String> branchToDelete = ImmutableList.of("refs/heads/test-1");
DeleteBranchesInput input = new DeleteBranchesInput();
input.branches = branchToDelete;
setApiUser(user);
try {
project().deleteBranches(input);
fail("Expected AuthException");
} catch (AuthException e) {
assertThat(e).hasMessageThat().isEqualTo("delete not permitted for refs/heads/test-1");
}
setApiUser(admin);
assertBranches(BRANCHES);
}
@Test
public void deleteMultiBranchesWithoutPermissionForbidden() throws Exception {
DeleteBranchesInput input = new DeleteBranchesInput();
input.branches = BRANCHES;
setApiUser(user);

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

@ -19,6 +19,7 @@ import static java.util.stream.Collectors.toList;
import static org.eclipse.jgit.lib.Constants.R_TAGS;
import static org.eclipse.jgit.transport.ReceiveCommand.Type.DELETE;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.reviewdb.client.Branch;
import com.google.gerrit.server.IdentifiedUser;
@ -96,7 +97,7 @@ public class DeleteRef {
return this;
}
public void delete() throws OrmException, IOException, ResourceConflictException {
public void delete() throws OrmException, IOException, ResourceConflictException, AuthException {
if (!refsToDelete.isEmpty()) {
try (Repository r = repoManager.openRepository(resource.getNameKey())) {
if (refsToDelete.size() == 1) {
@ -108,11 +109,17 @@ public class DeleteRef {
}
}
private void deleteSingleRef(Repository r) throws IOException, ResourceConflictException {
private void deleteSingleRef(Repository r)
throws IOException, ResourceConflictException, AuthException {
String ref = refsToDelete.get(0);
if (prefix != null && !ref.startsWith(prefix)) {
ref = prefix + ref;
}
if (!resource.getControl().controlForRef(ref).canDelete()) {
throw new AuthException("delete not permitted for " + ref);
}
RefUpdate.Result result;
RefUpdate u = r.updateRef(ref);
u.setExpectedOldObjectId(r.exactRef(ref).getObjectId());