diff options
9 files changed, 110 insertions, 10 deletions
diff --git a/java/com/google/gerrit/server/permissions/ProjectControl.java b/java/com/google/gerrit/server/permissions/ProjectControl.java index b08b38304c..328d01e94a 100644 --- a/java/com/google/gerrit/server/permissions/ProjectControl.java +++ b/java/com/google/gerrit/server/permissions/ProjectControl.java @@ -186,8 +186,7 @@ public class ProjectControl { boolean allRefsAreVisible(Set<String> ignore) { return user.isInternalUser() - || (!getProject().getNameKey().equals(allUsersName) - && canPerformOnAllRefs(Permission.READ, ignore)); + || (!getProject().getNameKey().equals(allUsersName) && canPerformReadOnAllRefs(ignore)); } /** Can the user run upload pack? */ @@ -297,20 +296,25 @@ public class ProjectControl { return null; } - private boolean canPerformOnAllRefs(String permission, Set<String> ignore) { + private boolean canPerformReadOnAllRefs(Set<String> ignore) { try (TraceTimer timer = TraceContext.newTimer( - "ProjectControl#canPerformOnAllRefs", + "ProjectControl#canPerformReadOnAllRefs", Metadata.builder().projectName(getProject().getName()).build())) { + // Admins should be able to read all refs. + if (isAdmin()) { + return true; + } + boolean canPerform = false; - Set<String> patterns = allRefPatterns(permission); + Set<String> patterns = allRefPatterns(Permission.READ); if (patterns.contains(ALL)) { // Only possible if granted on the pattern that // matches every possible reference. Check all // patterns also have the permission. // for (String pattern : patterns) { - if (controlForRef(pattern).canPerform(permission)) { + if (controlForRef(pattern).canPerform(Permission.READ)) { canPerform = true; } else if (ignore.contains(pattern)) { continue; diff --git a/java/com/google/gerrit/server/permissions/RefVisibilityControl.java b/java/com/google/gerrit/server/permissions/RefVisibilityControl.java index c5313de366..756e922d2b 100644 --- a/java/com/google/gerrit/server/permissions/RefVisibilityControl.java +++ b/java/com/google/gerrit/server/permissions/RefVisibilityControl.java @@ -68,6 +68,11 @@ public class RefVisibilityControl { if (!RefNames.isGerritRef(refName)) { // This is not a special Gerrit ref and not a NoteDb ref. Likely, it's just a ref under // refs/heads or another ref the user created. Apply the regular permissions with inheritance. + + // Admins should be able to read all branches (including 'refs/meta/config'). + if (projectControl.isAdmin()) { + return true; + } return projectControl.controlForRef(refName).hasReadPermissionOnRef(false); } diff --git a/javatests/com/google/gerrit/acceptance/api/project/CommitIT.java b/javatests/com/google/gerrit/acceptance/api/project/CommitIT.java index 1c349d8de3..d58f51ad2c 100644 --- a/javatests/com/google/gerrit/acceptance/api/project/CommitIT.java +++ b/javatests/com/google/gerrit/acceptance/api/project/CommitIT.java @@ -144,6 +144,9 @@ public class CommitIT extends AbstractDaemonTest { createBranch(BranchNameKey.create(change.project(), "branch-1")); createBranch(BranchNameKey.create(change.project(), "branch-2")); + // Use a non-admin user, since admins can always see all branches. + requestScopeOperations.setApiUser(user.id()); + assertThat(getIncludedIn(change.project(), currentPatchSet.commitId()).branches) .containsExactly("master", "branch-1", "branch-2"); @@ -203,6 +206,9 @@ public class CommitIT extends AbstractDaemonTest { TagInfo tagOnCommitInOther = gApi.projects().name(project.get()).tag(tagInput.ref).create(tagInput).get(); + // Use a non-admin user, since admins can always see all tags. + requestScopeOperations.setApiUser(user.id()); + // Verify that both tags are returned when retrieving the refs that include the commit in the // master branch. assertThat(getIncludedIn(project, currentPatchSet.commit()).tags) diff --git a/javatests/com/google/gerrit/acceptance/api/project/ProjectIT.java b/javatests/com/google/gerrit/acceptance/api/project/ProjectIT.java index 3db8ea1072..c5f51238b4 100644 --- a/javatests/com/google/gerrit/acceptance/api/project/ProjectIT.java +++ b/javatests/com/google/gerrit/acceptance/api/project/ProjectIT.java @@ -1216,6 +1216,8 @@ public class ProjectIT extends AbstractDaemonTest { createBranch(BranchNameKey.create(project, "non-visible-branch")); blockReadPermission(nonVisibleBranch); + // Use a non-admin user, since admins can always see all branches. + requestScopeOperations.setApiUser(user.id()); assertThat( getCommitsIncludedInRefs( change.getCommit().getName(), Arrays.asList(R_HEADS_MASTER, nonVisibleBranch))) @@ -1231,6 +1233,8 @@ public class ProjectIT extends AbstractDaemonTest { // on master so that tag-with-change becomes non-visible blockReadPermission(R_HEADS_MASTER); + // Use a non-admin user, since admins can always see all tags. + requestScopeOperations.setApiUser(user.id()); assertThat( getCommitsIncludedInRefs( change.getCommit().getName(), Arrays.asList(R_HEADS_MASTER, nonVisibleTag))) diff --git a/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java b/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java index 140b26bff6..bfa5130043 100644 --- a/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java +++ b/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java @@ -1811,17 +1811,23 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest { public void pushToNonVisibleBranchIsRejected() throws Exception { String master = "refs/heads/master"; + // Do the test as a non-admin user, since admins can always see all branches, even if read is + // blocked. + TestRepository<InMemoryRepository> userRepo = cloneProject(project, user); + userRepo.branch("HEAD").commit().message("New Commit 1").insertChangeId().create(); + projectOperations .project(project) .forUpdate() + .add(allow(Permission.PUSH).ref(master).group(REGISTERED_USERS)) + .add(allow(Permission.FORGE_COMMITTER).ref(master).group(REGISTERED_USERS)) .add(block(Permission.READ).ref(master).group(REGISTERED_USERS)) .update(); - testRepo.branch("HEAD").commit().message("New Commit 1").insertChangeId().create(); // Since the branch is not visible to the caller, the command tries to create the ref resulting // in the command being rejected because the ref already exists. assertPushRejected( - pushHead(testRepo, master), + pushHead(userRepo, master), master, "Cannot create ref 'refs/heads/master' because it already exists."); @@ -1831,8 +1837,8 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest { .add(allow(Permission.READ).ref(master).group(REGISTERED_USERS)) .update(); - testRepo.branch("HEAD").commit().message("New Commit 2").insertChangeId().create(); - assertPushOk(pushHead(testRepo, master), master); + userRepo.branch("HEAD").commit().message("New Commit 2").insertChangeId().create(); + assertPushOk(pushHead(userRepo, master), master); } @Test diff --git a/javatests/com/google/gerrit/acceptance/git/RefAdvertisementIT.java b/javatests/com/google/gerrit/acceptance/git/RefAdvertisementIT.java index 73d312fc3f..969c98f660 100644 --- a/javatests/com/google/gerrit/acceptance/git/RefAdvertisementIT.java +++ b/javatests/com/google/gerrit/acceptance/git/RefAdvertisementIT.java @@ -21,6 +21,7 @@ import static com.google.common.truth.TruthJUnit.assume; import static com.google.gerrit.acceptance.GitUtil.fetch; import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allow; import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allowCapability; +import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.block; import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.deny; import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.permissionKey; import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS; @@ -331,6 +332,34 @@ public class RefAdvertisementIT extends AbstractDaemonTest { } @Test + public void uploadPackAllRefsVisibleToAdminWithoutExplicitReadPermission() throws Exception { + // Remove read access + projectOperations + .project(project) + .forUpdate() + .add(block(Permission.READ).ref("refs/*").group(REGISTERED_USERS)) + .update(); + + // Admins can always read all branches, tags and changes. + assertUploadPackRefsAsAdmin( + "HEAD", + psRef1, + metaRef1, + psRef2, + metaRef2, + psRef3, + metaRef3, + psRef4, + metaRef4, + "refs/heads/branch", + "refs/heads/master", + RefNames.REFS_CONFIG, + "refs/tags/branch-tag", + "refs/tags/master-tag", + "refs/tags/tree-tag"); + } + + @Test public void grantReadOnRefsTagsIsNoOp() throws Exception { projectOperations .project(project) @@ -1520,6 +1549,15 @@ public class RefAdvertisementIT extends AbstractDaemonTest { assertRefs(project, user, true, expectedRefs); } + /** + * Assert that refs seen by an admin user match the expected refs. + * + * @param expectedRefs expected refs. + */ + private void assertUploadPackRefsAsAdmin(String... expectedRefs) throws Exception { + assertRefs(project, admin, true, expectedRefs); + } + private void assertRefs( Project.NameKey project, TestAccount user, boolean disableDb, String... expectedRefs) throws Exception { diff --git a/javatests/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java b/javatests/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java index 4cb1733d76..81e5593751 100644 --- a/javatests/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java +++ b/javatests/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java @@ -2301,6 +2301,8 @@ public class CreateChangeIT extends AbstractDaemonTest { public void createChangeWithSourceBranch() throws Exception { changeInTwoBranches("branchA", "a.txt", "branchB", "b.txt"); + requestScopeOperations.setApiUser(user.id()); + // create a merge change from branchA to master in gerrit ChangeInput in = new ChangeInput(); in.project = project.get(); diff --git a/javatests/com/google/gerrit/acceptance/rest/project/CreateBranchIT.java b/javatests/com/google/gerrit/acceptance/rest/project/CreateBranchIT.java index 1159ae7dd5..089e90196c 100644 --- a/javatests/com/google/gerrit/acceptance/rest/project/CreateBranchIT.java +++ b/javatests/com/google/gerrit/acceptance/rest/project/CreateBranchIT.java @@ -683,6 +683,37 @@ public class CreateBranchIT extends AbstractDaemonTest { () -> gApi.projects().name(project.get()).branch(branchInput.ref).create(branchInput)); } + @Test + public void createBranchAsAdminIsPossibleWithoutExplicitReadPermission() throws Exception { + PushOneCommit push = pushFactory.create(admin.newIdent(), testRepo); + PushOneCommit.Result result = push.to("refs/heads/foo"); + result.assertOkStatus(); + RevCommit fooCommit = result.getCommit(); + + // Remove read access + projectOperations + .project(project) + .forUpdate() + .add(block(Permission.READ).ref("refs/heads/*").group(REGISTERED_USERS)) + .update(); + + BranchInput branchInput = new BranchInput(); + branchInput.ref = "bar"; + branchInput.revision = fooCommit.name(); + BranchInfo created = + gApi.projects().name(project.get()).branch(branchInput.ref).create(branchInput).get(); + assertThat(created.ref).isEqualTo(RefNames.fullName(branchInput.ref)); + assertThat(created.revision).isEqualTo(fooCommit.name()); + assertThat(projectOperations.project(project).getHead(branchInput.ref)).isEqualTo(fooCommit); + + branchInput.ref = "baz"; + branchInput.revision = "refs/heads/foo"; + created = gApi.projects().name(project.get()).branch(branchInput.ref).create(branchInput).get(); + assertThat(created.ref).isEqualTo(RefNames.fullName(branchInput.ref)); + assertThat(created.revision).isEqualTo(fooCommit.name()); + assertThat(projectOperations.project(project).getHead(branchInput.ref)).isEqualTo(fooCommit); + } + private void blockCreateReference() throws Exception { projectOperations .project(project) diff --git a/javatests/com/google/gerrit/acceptance/rest/project/TagsIT.java b/javatests/com/google/gerrit/acceptance/rest/project/TagsIT.java index d535abde8a..c019822e04 100644 --- a/javatests/com/google/gerrit/acceptance/rest/project/TagsIT.java +++ b/javatests/com/google/gerrit/acceptance/rest/project/TagsIT.java @@ -246,6 +246,10 @@ public class TagsIT extends AbstractDaemonTest { assertThat(result.ref).isEqualTo(R_TAGS + tag2.ref); assertThat(result.revision).isEqualTo(tag2.revision); + // Do the test as a non-admin user, since admins can always see all tags, even if read on all + // branches is blocked. + requestScopeOperations.setApiUser(user.id()); + List<TagInfo> tags = getTags().get(); assertThat(tags).hasSize(2); assertThat(tags.get(0).ref).isEqualTo(R_TAGS + tag1.ref); |