opendev/gerrit
Code Issues Proposed changes

Explicitly check READ permission when processing a git push

Browse Source 
Git pushes for review require PUSH (create change) permission. They
did also always require READ permission, though, that was not checked
explicitly. READ permission was required, because we'd only consider
refs that the user can read as 'uninteresting' when traversing the
pushed commit chain to find commits we want to turn into changes.

Not having READ permission  means, Gerrit finds way more commits than
are actually new and then checks if the user has FORGE_AUTHOR permission
because Gerrit thinks they are uploading a commit from someone else. That
would then fail and leave the user with a cryptic error message.

While we could fix that on it's own, it seems way more robust to just
also check for READ permission when processing a push.

Change-Id: Ic3b433799d4dae3a5b8ac61f42fa3d47fdbb3d68
This commit is contained in:
Patrick Hiesel
2020-08-25 15:31:19 +02:00
parent 86a4503ec7
commit c2d48fb765
3 changed files with 25 additions and 6 deletions
Download Patch File Download Diff File Expand all files Collapse all files

4
java/com/google/gerrit/server/git/receive/ReceiveCommits.java
View File

@@ -1839,7 +1839,9 @@ class ReceiveCommits {
magicBranch.perm = permissions.ref(ref);
Optional<AuthException> err =
checkRefPermission(magicBranch.perm, RefPermission.CREATE_CHANGE);
checkRefPermission(magicBranch.perm, RefPermission.READ)
.map(Optional::of)
.orElse(checkRefPermission(magicBranch.perm, RefPermission.CREATE_CHANGE));
if (err.isPresent()) {
rejectProhibited(cmd, err.get());
return;

10
javatests/com/google/gerrit/acceptance/api/change/QueryChangesIT.java
View File

@@ -153,16 +153,16 @@ public class QueryChangesIT extends AbstractDaemonTest {
// Create hidden project.
Project.NameKey hiddenProject = projectOperations.newProject().create();
TestRepository<InMemoryRepository> hiddenRepo = cloneProject(hiddenProject, admin);
// Create 2 hidden changes.
createChange(hiddenRepo);
createChange(hiddenRepo);
// Actually hide project
projectOperations
.project(hiddenProject)
.forUpdate()
.add(block(Permission.READ).ref("refs/*").group(REGISTERED_USERS))
.update();
TestRepository<InMemoryRepository> hiddenRepo = cloneProject(hiddenProject, admin);
// Create 2 hidden changes.
createChange(hiddenRepo);
createChange(hiddenRepo);
// Create a change query that matches all changes (visible and hidden changes).
// The index returns the changes ordered by last updated timestamp:

17
javatests/com/google/gerrit/acceptance/git/PushPermissionsIT.java
View File

@@ -17,6 +17,7 @@ package com.google.gerrit.acceptance.git;
import static com.google.common.truth.Truth.assertThat;
import static com.google.common.truth.Truth.assertWithMessage;
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.git.testing.PushResultSubject.assertThat;
import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS;
import static java.util.stream.Collectors.toList;
@@ -145,6 +146,22 @@ public class PushPermissionsIT extends AbstractDaemonTest {
assertThat(r).hasProcessed(ImmutableMap.of("refs", 1));
}
@Test
public void createDeniedIfUserCantRead() throws Exception {
projectOperations
.project(project)
.forUpdate()
.add(block(Permission.READ).ref("refs/*").group(REGISTERED_USERS))
.add(allow(Permission.PUSH).ref("refs/*").group(REGISTERED_USERS))
.update();
testRepo.branch("HEAD").commit().create();
PushResult r = push("HEAD:refs/for/master");
assertThat(r)
.onlyRef("refs/for/master")
.isRejected("prohibited by Gerrit: not permitted: read on refs/heads/master");
assertThat(r).hasProcessed(ImmutableMap.of("refs", 1));
}
@Test
public void groupRefsByMessage() throws Exception {
try (Repository repo = repoManager.openRepository(project);