diff options
| author | Edwin Kempin <ekempin@google.com> | 2025-11-13 14:03:42 +0000 |
|---|---|---|
| committer | Paladox none <thomasmulhall410@yahoo.com> | 2025-11-14 04:46:22 -0800 |
| commit | bec979235742ffffa018c744a90ed20923430e59 (patch) | |
| tree | e67629cdd3eeefa3c91cdce4b7cbcf4105ea4dbf | |
| parent | 93d721be3553d95be4877c804af5e3d1f86d6f6a (diff) | |
DefaultRefFilter: Allow admins to always see all branches and tagsupstream/stable-3.13
With change I44d97259f we added special logic that allows admins to see
all branches, tags and changes without having explicit read permission.
When doing this we missed some methods that are being used by
DefaultRefFilter.
Due to this admins couldn't
1. git fetch refs without having explicit read permissions (although
they could see these refs through the REST API)
2. create branches on commits that are (only) reachable from branches
for which the admin didn't have explicit read permissions (although
they could see these branches and hence the commits in them through
the REST API).
This change fixes this and adds tests for 1. (in RefAdvertisementIT) and
2. (in CreateBranchIT).
The private ProjectControl#canPerformOnAllRefs(String permission,
Set<String> ignore) method was only called with permission=Read. Since
the special logic for admins only applies for Read, I dropped the
permission parameter from this method and renamed it to
canPerformReadOnAllRefs. That seems easier than checking on the
permission name inside the method to know if the admin shortcut applies.
Some tests that checked that branches, tags, changes are no longer
visible when the Read access right is removed/blocked needed to be
changed to use a non-admin user, since admins can now always see all
branches, tags and changes.
Bug: Google b/458738958
Release-Notes: Allow admins to see all refs on fetch and when creating branches
Change-Id: Icd3d9cff9fe4f6f85d4f0be5fd47fd0516d2a729
Signed-off-by: Edwin Kempin <ekempin@google.com>
(cherry picked from commit 710b768a1dcd36a09d08b784fbc5913ac8830477)
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); |