opendev/gerrit
Code Issues Proposed changes

CreateChange: add a visibility check for the target branch

Browse Source 
Currently, it's possible for a user to create a change on a branch
they can't see. When this happens, the created change is not
accessible by the user (WAI) but this gives the user a way to probe
whether a branch exists or not.

This commit fixes this issue by adding a "READ" permisssion check for
the input branch and verifies that if the user doesn't have permission
to see the target branch, we always return 404 no matter the branch
exists or not.

BTW, the existing #createChangeOnInvisibleBranchFails test failed
to catch up this because the 404 it verifies is thrown by
the java API ChangesImpl#create when it tries to create a ChangeInfo
for the created change.

Change-Id: I5c8e3334e37b7215c86c08c5172a8e3b4a69d0c6
This commit is contained in:
Changcheng Xiao
2019-01-24 17:04:20 +01:00
parent 1865b12967
commit a0ef8f6095
2 changed files with 31 additions and 10 deletions
Download Patch File Download Diff File Expand all files Collapse all files

7
java/com/google/gerrit/server/restapi/change/CreateChange.java
View File

@@ -30,6 +30,7 @@ import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.MethodNotAllowedException;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
import com.google.gerrit.extensions.restapi.Response;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.extensions.restapi.TopLevelResource;
@@ -196,6 +197,12 @@ public class CreateChange
Project.NameKey project = rsrc.getNameKey();
String refName = RefNames.fullName(input.branch);
try {
permissionBackend.currentUser().project(project).ref(refName).check(RefPermission.READ);
} catch (AuthException e) {
throw new ResourceNotFoundException(String.format("ref %s not found", refName));
}
permissionBackend
.currentUser()
.project(project)

34
javatests/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java
View File

@@ -267,16 +267,6 @@ public class CreateChangeIT extends AbstractDaemonTest {
in, UnprocessableEntityException.class, "Base change not found: " + in.baseChange);
}
@Test
public void createChangeOnInvisibleBranchFails() throws Exception {
changeInTwoBranches("invisible-branch", "a.txt", "branchB", "b.txt");
block(project, "refs/heads/invisible-branch", READ, REGISTERED_USERS);
ChangeInput in = newChangeInput(ChangeStatus.NEW);
in.branch = "invisible-branch";
assertCreateFails(in, ResourceNotFoundException.class, "");
}
@Test
public void noteDbCommit() throws Exception {
ChangeInfo c = assertCreateSucceeds(newChangeInput(ChangeStatus.NEW));
@@ -456,6 +446,30 @@ public class CreateChangeIT extends AbstractDaemonTest {
assertThat(revInfo.commit.message).isEqualTo(input.message + "\n");
}
@Test
public void createChangeOnExistingBranchNotPermitted() throws Exception {
createBranch(new Branch.NameKey(project, "foo"));
blockRead("refs/heads/*");
requestScopeOperations.setApiUser(user.id);
ChangeInput input = newChangeInput(ChangeStatus.NEW);
input.branch = "foo";
assertCreateFails(input, ResourceNotFoundException.class, "ref refs/heads/foo not found");
}
@Test
public void createChangeOnNonExistingBranchNotPermitted() throws Exception {
blockRead("refs/heads/*");
requestScopeOperations.setApiUser(user.id);
ChangeInput input = newChangeInput(ChangeStatus.NEW);
input.branch = "foo";
// sets this option to be true to make sure permission check happened before this option could
// be considered.
input.newBranch = true;
assertCreateFails(input, ResourceNotFoundException.class, "ref refs/heads/foo not found");
}
private ChangeInput newChangeInput(ChangeStatus status) {
ChangeInput in = new ChangeInput();
in.project = project.get();