From 4ee1c512ef988404e157f28d9646a626dc468031 Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Tue, 14 Nov 2017 15:00:30 +0100 Subject: [PATCH 1/6] VisibleRefFilter: Don't treat refs/meta/external-ids as meta ref VisibleRefFilter has a setting to filter out meta refs. AsyncReceiveCommits uses this setting to filter out meta refs and thus prevent that they are updated by push. For refs/meta/external-ids it should be possible to push updates if the 'Access Database' capbility is granted. This allows administrators to fix external IDs manually. Hence VisibleRefFilter shouldn't treat refs/meta/external-ids as meta ref (similar to how refs/meta/config is also not treated as meta ref). At the moment pushing to refs/meta/external-ids only works if the calling user has read permissions on all refs of All-Users because in this case VisibleRefFilter has a shortcut to return all refs and filtering of meta refs is not done. The tests for pushing updates to refs/meta/external-ids only work due to this shortcut. However in a follow-up change this shortcut will be removed for All-Users. Change-Id: Id69bc3160375c4c8c060eb099e4c210fbe016123 Signed-off-by: Edwin Kempin --- java/com/google/gerrit/server/git/VisibleRefFilter.java | 4 +--- .../google/gerrit/acceptance/git/RefAdvertisementIT.java | 6 +++--- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/java/com/google/gerrit/server/git/VisibleRefFilter.java b/java/com/google/gerrit/server/git/VisibleRefFilter.java index ed86a922f0..728f1e3fc2 100644 --- a/java/com/google/gerrit/server/git/VisibleRefFilter.java +++ b/java/com/google/gerrit/server/git/VisibleRefFilter.java @@ -327,9 +327,7 @@ public class VisibleRefFilter extends AbstractAdvertiseRefsHook { } private boolean isMetadata(String name) { - return name.startsWith(REFS_CHANGES) - || RefNames.isRefsEdit(name) - || (projectState.isAllUsers() && name.equals(RefNames.REFS_EXTERNAL_IDS)); + return name.startsWith(REFS_CHANGES) || RefNames.isRefsEdit(name); } private static boolean isTag(Ref ref) { diff --git a/javatests/com/google/gerrit/acceptance/git/RefAdvertisementIT.java b/javatests/com/google/gerrit/acceptance/git/RefAdvertisementIT.java index 1ac135eb44..470f5c36f9 100644 --- a/javatests/com/google/gerrit/acceptance/git/RefAdvertisementIT.java +++ b/javatests/com/google/gerrit/acceptance/git/RefAdvertisementIT.java @@ -553,11 +553,11 @@ public class RefAdvertisementIT extends AbstractDaemonTest { RefNames.refsUsers(admin.id), RefNames.refsUsers(user.id), RefNames.REFS_SEQUENCES + Sequences.NAME_ACCOUNTS, - RefNames.REFS_SEQUENCES + Sequences.NAME_GROUPS); + RefNames.REFS_SEQUENCES + Sequences.NAME_GROUPS, + RefNames.REFS_EXTERNAL_IDS); List expectedMetaRefs = - new ArrayList<>( - ImmutableList.of(RefNames.REFS_EXTERNAL_IDS, mr.getPatchSetId().toRefName())); + new ArrayList<>(ImmutableList.of(mr.getPatchSetId().toRefName())); if (NoteDbMode.get() != NoteDbMode.OFF) { expectedMetaRefs.add(changeRefPrefix(mr.getChange().getId()) + "meta"); } From 4a3dd3ad2e8c3ec8c5661429f3f9114bdc25f6a4 Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Tue, 14 Nov 2017 13:49:28 +0100 Subject: [PATCH 2/6] VisibleRefFilter: Skip fast advertising of all refs for All-Users All-Users contains special refs that should not be visible even if the user has read permissions. E.g. user branches or starred changes refs are only visible to the owning user. This special logic was not executed when the calling user had read permissions for all refs of the project. This means users who had read permissions for all refs of All-Users could see all user branches, although they should have seen only the own user branch. Fixing this shows that several tests relied on this broken behaviour. Those tests are fixed wither by making the test user only access the own user branch or by granting the 'Access Database' capability that allows to see all user branches. RefAdvertisementIT now removes all read permissions for All-Users in the setup so that this issue would have been discovered. Change-Id: Id8453cfd325dda8263ca639cd0f809cdaea61df3 Signed-off-by: Edwin Kempin --- .../gerrit/server/git/VisibleRefFilter.java | 10 ++++--- .../acceptance/api/accounts/AccountIT.java | 29 ++++++++++--------- .../acceptance/git/RefAdvertisementIT.java | 16 +++++++--- .../acceptance/rest/account/ExternalIdIT.java | 12 ++++++++ 4 files changed, 46 insertions(+), 21 deletions(-) diff --git a/java/com/google/gerrit/server/git/VisibleRefFilter.java b/java/com/google/gerrit/server/git/VisibleRefFilter.java index 728f1e3fc2..3dbfdccc1a 100644 --- a/java/com/google/gerrit/server/git/VisibleRefFilter.java +++ b/java/com/google/gerrit/server/git/VisibleRefFilter.java @@ -118,10 +118,12 @@ public class VisibleRefFilter extends AbstractAdvertiseRefsHook { PermissionBackend.WithUser withUser = permissionBackend.user(user); PermissionBackend.ForProject forProject = withUser.project(projectState.getNameKey()); - if (checkProjectPermission(forProject, ProjectPermission.READ)) { - return refs; - } else if (checkProjectPermission(forProject, ProjectPermission.READ_NO_CONFIG)) { - return fastHideRefsMetaConfig(refs); + if (!projectState.isAllUsers()) { + if (checkProjectPermission(forProject, ProjectPermission.READ)) { + return refs; + } else if (checkProjectPermission(forProject, ProjectPermission.READ_NO_CONFIG)) { + return fastHideRefsMetaConfig(refs); + } } Account.Id userId; diff --git a/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java b/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java index ce4d77bc42..293a438ead 100644 --- a/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java +++ b/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java @@ -918,11 +918,11 @@ public class AccountIT extends AbstractDaemonTest { @Test public void pushAccountConfigWithPrefEmailThatDoesNotExistAsExtIdToUserBranchForReviewAndSubmit() throws Exception { - TestAccount foo = accountCreator.create(name("foo")); + TestAccount foo = accountCreator.create(name("foo"), name("foo") + "@example.com", "Foo"); String userRef = RefNames.refsUsers(foo.id); accountIndexedCounter.clear(); - TestRepository allUsersRepo = cloneProject(allUsers); + TestRepository allUsersRepo = cloneProject(allUsers, foo); fetch(allUsersRepo, userRef + ":userRef"); allUsersRepo.reset("userRef"); @@ -934,7 +934,7 @@ public class AccountIT extends AbstractDaemonTest { pushFactory .create( db, - admin.getIdent(), + foo.getIdent(), allUsersRepo, "Update account config", AccountConfig.ACCOUNT_CONFIG, @@ -1058,7 +1058,10 @@ public class AccountIT extends AbstractDaemonTest { } @Test + @Sandboxed public void pushAccountConfigToUserBranchForReviewDeactivateOtherAccount() throws Exception { + allowGlobalCapabilities(REGISTERED_USERS, GlobalCapability.ACCESS_DATABASE); + TestAccount foo = accountCreator.create(name("foo")); assertThat(gApi.accounts().id(foo.id.get()).getActive()).isTrue(); String userRef = RefNames.refsUsers(foo.id); @@ -1229,18 +1232,15 @@ public class AccountIT extends AbstractDaemonTest { @Test public void pushAccountConfigToUserBranchInvalidPreferredEmailButNotChanged() throws Exception { - TestAccount foo = accountCreator.create(name("foo")); + TestAccount foo = accountCreator.create(name("foo"), name("foo") + "@example.com", "Foo"); String userRef = RefNames.refsUsers(foo.id); String noEmail = "no.email"; accountsUpdate.create().update(foo.id, a -> a.setPreferredEmail(noEmail)); accountIndexedCounter.clear(); - InternalGroup adminGroup = - groupCache.get(new AccountGroup.NameKey("Administrators")).orElse(null); - grant(allUsers, userRef, Permission.PUSH, false, adminGroup.getGroupUUID()); - - TestRepository allUsersRepo = cloneProject(allUsers); + grant(allUsers, userRef, Permission.PUSH, false, REGISTERED_USERS); + TestRepository allUsersRepo = cloneProject(allUsers, foo); fetch(allUsersRepo, userRef + ":userRef"); allUsersRepo.reset("userRef"); @@ -1251,7 +1251,7 @@ public class AccountIT extends AbstractDaemonTest { pushFactory .create( db, - admin.getIdent(), + foo.getIdent(), allUsersRepo, "Update account config", AccountConfig.ACCOUNT_CONFIG, @@ -1268,7 +1268,7 @@ public class AccountIT extends AbstractDaemonTest { @Test public void pushAccountConfigToUserBranchIfPreferredEmailDoesNotExistAsExtId() throws Exception { - TestAccount foo = accountCreator.create(name("foo")); + TestAccount foo = accountCreator.create(name("foo"), name("foo") + "@example.com", "Foo"); String userRef = RefNames.refsUsers(foo.id); accountIndexedCounter.clear(); @@ -1276,7 +1276,7 @@ public class AccountIT extends AbstractDaemonTest { groupCache.get(new AccountGroup.NameKey("Administrators")).orElse(null); grant(allUsers, userRef, Permission.PUSH, false, adminGroup.getGroupUUID()); - TestRepository allUsersRepo = cloneProject(allUsers); + TestRepository allUsersRepo = cloneProject(allUsers, foo); fetch(allUsersRepo, userRef + ":userRef"); allUsersRepo.reset("userRef"); @@ -1287,7 +1287,7 @@ public class AccountIT extends AbstractDaemonTest { pushFactory .create( db, - admin.getIdent(), + foo.getIdent(), allUsersRepo, "Update account config", AccountConfig.ACCOUNT_CONFIG, @@ -1326,7 +1326,10 @@ public class AccountIT extends AbstractDaemonTest { } @Test + @Sandboxed public void pushAccountConfigToUserBranchDeactivateOtherAccount() throws Exception { + allowGlobalCapabilities(REGISTERED_USERS, GlobalCapability.ACCESS_DATABASE); + TestAccount foo = accountCreator.create(name("foo")); assertThat(gApi.accounts().id(foo.id.get()).getActive()).isTrue(); String userRef = RefNames.refsUsers(foo.id); diff --git a/javatests/com/google/gerrit/acceptance/git/RefAdvertisementIT.java b/javatests/com/google/gerrit/acceptance/git/RefAdvertisementIT.java index 470f5c36f9..59eeb8ddc4 100644 --- a/javatests/com/google/gerrit/acceptance/git/RefAdvertisementIT.java +++ b/javatests/com/google/gerrit/acceptance/git/RefAdvertisementIT.java @@ -93,14 +93,22 @@ public class RefAdvertisementIT extends AbstractDaemonTest { } private void setUpPermissions() throws Exception { - // Remove read permissions for all users besides admin. This method is - // idempotent, so is safe to call on every test setup. + // Remove read permissions for all users besides admin. This method is idempotent, so is safe + // to call on every test setup. ProjectConfig pc = projectCache.checkedGet(allProjects).getConfig(); for (AccessSection sec : pc.getAccessSections()) { sec.removePermission(Permission.READ); } Util.allow(pc, Permission.READ, admins, "refs/*"); saveProjectConfig(allProjects, pc); + + // Remove all read permissions on All-Users. This method is idempotent, so is safe to call on + // every test setup. + pc = projectCache.checkedGet(allUsers).getConfig(); + for (AccessSection sec : pc.getAccessSections()) { + sec.removePermission(Permission.READ); + } + saveProjectConfig(allUsers, pc); } private static String changeRefPrefix(Change.Id id) { @@ -554,7 +562,8 @@ public class RefAdvertisementIT extends AbstractDaemonTest { RefNames.refsUsers(user.id), RefNames.REFS_SEQUENCES + Sequences.NAME_ACCOUNTS, RefNames.REFS_SEQUENCES + Sequences.NAME_GROUPS, - RefNames.REFS_EXTERNAL_IDS); + RefNames.REFS_EXTERNAL_IDS, + RefNames.REFS_CONFIG); List expectedMetaRefs = new ArrayList<>(ImmutableList.of(mr.getPatchSetId().toRefName())); @@ -565,7 +574,6 @@ public class RefAdvertisementIT extends AbstractDaemonTest { List expectedAllRefs = new ArrayList<>(expectedNonMetaRefs); expectedAllRefs.addAll(expectedMetaRefs); - setApiUser(user); try (Repository repo = repoManager.openRepository(allUsers)) { Map all = repo.getAllRefs(); diff --git a/javatests/com/google/gerrit/acceptance/rest/account/ExternalIdIT.java b/javatests/com/google/gerrit/acceptance/rest/account/ExternalIdIT.java index 4cc15d6baa..15e6a3510b 100644 --- a/javatests/com/google/gerrit/acceptance/rest/account/ExternalIdIT.java +++ b/javatests/com/google/gerrit/acceptance/rest/account/ExternalIdIT.java @@ -273,6 +273,8 @@ public class ExternalIdIT extends AbstractDaemonTest { @Test public void pushToExternalIdsBranch() throws Exception { + allowGlobalCapabilities(REGISTERED_USERS, GlobalCapability.ACCESS_DATABASE); + TestRepository allUsersRepo = cloneProject(allUsers); fetch(allUsersRepo, RefNames.REFS_EXTERNAL_IDS + ":" + RefNames.REFS_EXTERNAL_IDS); allUsersRepo.reset(RefNames.REFS_EXTERNAL_IDS); @@ -296,6 +298,8 @@ public class ExternalIdIT extends AbstractDaemonTest { @Test public void pushToExternalIdsBranchRejectsExternalIdWithoutAccountId() throws Exception { + allowGlobalCapabilities(REGISTERED_USERS, GlobalCapability.ACCESS_DATABASE); + TestRepository allUsersRepo = cloneProject(allUsers); fetch(allUsersRepo, RefNames.REFS_EXTERNAL_IDS + ":" + RefNames.REFS_EXTERNAL_IDS); allUsersRepo.reset(RefNames.REFS_EXTERNAL_IDS); @@ -312,6 +316,8 @@ public class ExternalIdIT extends AbstractDaemonTest { @Test public void pushToExternalIdsBranchRejectsExternalIdWithKeyThatDoesntMatchTheNoteId() throws Exception { + allowGlobalCapabilities(REGISTERED_USERS, GlobalCapability.ACCESS_DATABASE); + TestRepository allUsersRepo = cloneProject(allUsers); fetch(allUsersRepo, RefNames.REFS_EXTERNAL_IDS + ":" + RefNames.REFS_EXTERNAL_IDS); allUsersRepo.reset(RefNames.REFS_EXTERNAL_IDS); @@ -327,6 +333,8 @@ public class ExternalIdIT extends AbstractDaemonTest { @Test public void pushToExternalIdsBranchRejectsExternalIdWithInvalidConfig() throws Exception { + allowGlobalCapabilities(REGISTERED_USERS, GlobalCapability.ACCESS_DATABASE); + TestRepository allUsersRepo = cloneProject(allUsers); fetch(allUsersRepo, RefNames.REFS_EXTERNAL_IDS + ":" + RefNames.REFS_EXTERNAL_IDS); allUsersRepo.reset(RefNames.REFS_EXTERNAL_IDS); @@ -342,6 +350,8 @@ public class ExternalIdIT extends AbstractDaemonTest { @Test public void pushToExternalIdsBranchRejectsExternalIdWithEmptyNote() throws Exception { + allowGlobalCapabilities(REGISTERED_USERS, GlobalCapability.ACCESS_DATABASE); + TestRepository allUsersRepo = cloneProject(allUsers); fetch(allUsersRepo, RefNames.REFS_EXTERNAL_IDS + ":" + RefNames.REFS_EXTERNAL_IDS); allUsersRepo.reset(RefNames.REFS_EXTERNAL_IDS); @@ -380,6 +390,8 @@ public class ExternalIdIT extends AbstractDaemonTest { private void testPushToExternalIdsBranchRejectsInvalidExternalId(ExternalId invalidExtId) throws Exception { + allowGlobalCapabilities(REGISTERED_USERS, GlobalCapability.ACCESS_DATABASE); + TestRepository allUsersRepo = cloneProject(allUsers); fetch(allUsersRepo, RefNames.REFS_EXTERNAL_IDS + ":" + RefNames.REFS_EXTERNAL_IDS); allUsersRepo.reset(RefNames.REFS_EXTERNAL_IDS); From 6e0fd8e1077cc81d05421de0c4fbdd03804bfa20 Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Mon, 13 Nov 2017 14:12:32 +0100 Subject: [PATCH 3/6] Test advertisement of user refs To see a user ref the user must be owner of the user ref and have read permissions on it. Read permissions don't allow to see user branches of other users. To see user refs of other users the 'Access Database' capability is required. With 'Access Database' read permissions are not required. Change-Id: Ib960df33094980cdf8f3db2939e729d86fde06a7 Signed-off-by: Edwin Kempin --- .../acceptance/git/RefAdvertisementIT.java | 67 ++++++++++++++----- 1 file changed, 50 insertions(+), 17 deletions(-) diff --git a/javatests/com/google/gerrit/acceptance/git/RefAdvertisementIT.java b/javatests/com/google/gerrit/acceptance/git/RefAdvertisementIT.java index 59eeb8ddc4..6231400eb3 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.gerrit.acceptance.GitUtil.fetch; import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS; import static java.util.stream.Collectors.toList; +import com.google.common.base.Predicates; import com.google.common.collect.ImmutableList; import com.google.gerrit.acceptance.AbstractDaemonTest; import com.google.gerrit.acceptance.AcceptanceTestRequestScope; @@ -56,8 +57,8 @@ import java.util.ArrayList; import java.util.Collections; import java.util.List; import java.util.Map; +import java.util.function.Predicate; import org.eclipse.jgit.api.Git; -import org.eclipse.jgit.api.LsRemoteCommand; import org.eclipse.jgit.junit.TestRepository; import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.PersonIdent; @@ -459,23 +460,49 @@ public class RefAdvertisementIT extends AbstractDaemonTest { assertThat(getReceivePackRefs().additionalHaves()).containsExactly(obj(c4, 1)); } + @Test + public void advertisedReferencesDontShowUserBranchWithoutRead() throws Exception { + TestRepository userTestRepository = cloneProject(allUsers, user); + try (Git git = userTestRepository.git()) { + assertThat(getUserRefs(git)).isEmpty(); + } + } + + @Test + public void advertisedReferencesOmitUserBranchesOfOtherUsers() throws Exception { + allow(allUsersName, RefNames.REFS_USERS + "*", Permission.READ, REGISTERED_USERS); + TestRepository userTestRepository = cloneProject(allUsers, user); + try (Git git = userTestRepository.git()) { + assertThat(getUserRefs(git)) + .containsExactly(RefNames.REFS_USERS_SELF, RefNames.refsUsers(user.id)); + } + } + + @Test + public void advertisedReferencesIncludeAllUserBranchesWithAccessDatabase() throws Exception { + allowGlobalCapabilities(REGISTERED_USERS, GlobalCapability.ACCESS_DATABASE); + try { + TestRepository userTestRepository = cloneProject(allUsers, user); + try (Git git = userTestRepository.git()) { + assertThat(getUserRefs(git)) + .containsExactly(RefNames.refsUsers(user.id), RefNames.refsUsers(admin.id)); + } + } finally { + removeGlobalCapabilities(REGISTERED_USERS, GlobalCapability.ACCESS_DATABASE); + } + } + @Test public void advertisedReferencesOmitPrivateChangesOfOtherUsers() throws Exception { allow("refs/heads/master", Permission.READ, REGISTERED_USERS); TestRepository userTestRepository = cloneProject(project, user); try (Git git = userTestRepository.git()) { - LsRemoteCommand lsRemoteCommand = git.lsRemote(); String change3RefName = c3.currentPatchSet().getRefName(); - - List initialRefNames = - lsRemoteCommand.call().stream().map(Ref::getName).collect(toList()); - assertWithMessage("Precondition violated").that(initialRefNames).contains(change3RefName); + assertWithMessage("Precondition violated").that(getRefs(git)).contains(change3RefName); gApi.changes().id(c3.getId().get()).setPrivate(true, null); - - List refNames = lsRemoteCommand.call().stream().map(Ref::getName).collect(toList()); - assertThat(refNames).doesNotContain(change3RefName); + assertThat(getRefs(git)).doesNotContain(change3RefName); } } @@ -485,17 +512,11 @@ public class RefAdvertisementIT extends AbstractDaemonTest { TestRepository userTestRepository = cloneProject(project, user); try (Git git = userTestRepository.git()) { - LsRemoteCommand lsRemoteCommand = git.lsRemote(); String change3RefName = c3.currentPatchSet().getRefName(); - - List initialRefNames = - lsRemoteCommand.call().stream().map(Ref::getName).collect(toList()); - assertWithMessage("Precondition violated").that(initialRefNames).contains(change3RefName); + assertWithMessage("Precondition violated").that(getRefs(git)).contains(change3RefName); gApi.changes().id(c3.getId().get()).setPrivate(true, null); - - List refNames = lsRemoteCommand.call().stream().map(Ref::getName).collect(toList()); - assertThat(refNames).contains(change3RefName); + assertThat(getRefs(git)).contains(change3RefName); } } @@ -595,6 +616,18 @@ public class RefAdvertisementIT extends AbstractDaemonTest { } } + private List getRefs(Git git) throws Exception { + return getRefs(git, Predicates.alwaysTrue()); + } + + private List getUserRefs(Git git) throws Exception { + return getRefs(git, RefNames::isRefsUsers); + } + + private List getRefs(Git git, Predicate predicate) throws Exception { + return git.lsRemote().call().stream().map(Ref::getName).filter(predicate).collect(toList()); + } + /** * Assert that refs seen by a non-admin user match expected. * From 314a2b7e9e0b44294774f4429e06d919a0426d55 Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Tue, 14 Nov 2017 09:26:45 +0100 Subject: [PATCH 4/6] Advertise refs/users/self if user branch is visible due to Access Database The Access Database capability allows to see all user branches without read permissions. However if read permissions were missing refs/users/self was not advertised. If the target of refs/users/self is advertised refs/users/self should be included as well. Change-Id: I113bf46084a80aa645a13b793ba24bdb6ae17bdb Signed-off-by: Edwin Kempin --- .../com/google/gerrit/server/git/VisibleRefFilter.java | 10 ++++++++++ .../gerrit/acceptance/git/RefAdvertisementIT.java | 5 ++++- 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/java/com/google/gerrit/server/git/VisibleRefFilter.java b/java/com/google/gerrit/server/git/VisibleRefFilter.java index 3dbfdccc1a..b1eea1d434 100644 --- a/java/com/google/gerrit/server/git/VisibleRefFilter.java +++ b/java/com/google/gerrit/server/git/VisibleRefFilter.java @@ -182,6 +182,12 @@ public class VisibleRefFilter extends AbstractAdvertiseRefsHook { // symbolic we want the control around the final target. If its // not symbolic then getLeaf() is a no-op returning ref itself. result.put(name, ref); + } else if (isRefsUsersSelf(ref)) { + // viewMetadata allows to see all account refs, hence refs/users/self should be included as + // well + if (viewMetadata) { + result.put(name, ref); + } } } @@ -336,6 +342,10 @@ public class VisibleRefFilter extends AbstractAdvertiseRefsHook { return ref.getLeaf().getName().startsWith(Constants.R_TAGS); } + private static boolean isRefsUsersSelf(Ref ref) { + return ref.getName().startsWith(REFS_USERS_SELF); + } + private boolean canReadRef(String ref) { try { perm.ref(ref).check(RefPermission.READ); diff --git a/javatests/com/google/gerrit/acceptance/git/RefAdvertisementIT.java b/javatests/com/google/gerrit/acceptance/git/RefAdvertisementIT.java index 6231400eb3..a6698ef2f4 100644 --- a/javatests/com/google/gerrit/acceptance/git/RefAdvertisementIT.java +++ b/javatests/com/google/gerrit/acceptance/git/RefAdvertisementIT.java @@ -485,7 +485,10 @@ public class RefAdvertisementIT extends AbstractDaemonTest { TestRepository userTestRepository = cloneProject(allUsers, user); try (Git git = userTestRepository.git()) { assertThat(getUserRefs(git)) - .containsExactly(RefNames.refsUsers(user.id), RefNames.refsUsers(admin.id)); + .containsExactly( + RefNames.REFS_USERS_SELF, + RefNames.refsUsers(user.id), + RefNames.refsUsers(admin.id)); } } finally { removeGlobalCapabilities(REGISTERED_USERS, GlobalCapability.ACCESS_DATABASE); From 27ef42eb0bb5387495702f1c997c8146bdac0c6a Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Mon, 13 Nov 2017 14:17:31 +0100 Subject: [PATCH 5/6] Advertise group refs to group owners Group refs are only visible to group owners that have read permissions on the ref. To be a group owner the user must either be member of the owner group or have the 'Administrate Server' capability. Read permissions don't allow non-group-owners to see the group refs. To see group refs as non-group-owner the 'Access Database' capability is required. With 'Access Database' read permissions are not needed. This is consistent with the handling of user branches. To see a user ref the user must be the owning user and have read permissions. To see user branches of other users the 'Access Database' capability is needed. By default All-Projects grants read access on 'refs/*' to 'Anonymous Users'. Hence by inheritance all users have read access on the group branches in All-Users, but these read permissions are only effective for group owners and users that have the 'Access Database' capability. A follow-up change may make the read access more explicit by granting read permissions to 'Registered Users' on 'refs/groups/*' in All-Users by default (for existing sites this may be done by a schema migration). To check whether the calling user owns a group ref we need to know the owner group and check if the user is a member of this owner group. Fortunately all data that we need for this is available via caches. IdentifiedUser caches the effective groups of the calling user and the owner group can be retrieved via the group cache. This means to decide which group refs should be advertised we need to access all groups via the group cache. This only performs well if the group cache is large enough to hold entries for all groups. Change Ieef38b9dda made the group cache unlimited by default and documented that the group cache should be large enough to hold entries for all groups. Also ls-remote calls to All-Users are relatively rare and hence performance of the group ref advertisement is not that critical. Change-Id: I864c66970d48473d33694f4c33d3d6695123be88 Signed-off-by: Edwin Kempin --- .../gerrit/reviewdb/client/RefNames.java | 4 + .../gerrit/server/account/GroupControl.java | 1 + .../gerrit/server/git/VisibleRefFilter.java | 41 +++++++- .../acceptance/git/RefAdvertisementIT.java | 93 +++++++++++++++++++ 4 files changed, 134 insertions(+), 5 deletions(-) diff --git a/java/com/google/gerrit/reviewdb/client/RefNames.java b/java/com/google/gerrit/reviewdb/client/RefNames.java index 8e9bc344e5..9624ad1443 100644 --- a/java/com/google/gerrit/reviewdb/client/RefNames.java +++ b/java/com/google/gerrit/reviewdb/client/RefNames.java @@ -221,6 +221,10 @@ public class RefNames { return ref.startsWith(REFS_USERS); } + public static boolean isRefsGroups(String ref) { + return ref.startsWith(REFS_GROUPS); + } + static Integer parseShardedRefPart(String name) { if (name == null) { return null; diff --git a/java/com/google/gerrit/server/account/GroupControl.java b/java/com/google/gerrit/server/account/GroupControl.java index 16655abd06..119dc5b16f 100644 --- a/java/com/google/gerrit/server/account/GroupControl.java +++ b/java/com/google/gerrit/server/account/GroupControl.java @@ -144,6 +144,7 @@ public class GroupControl { return isOwner; } + // Keep this logic in sync with VisibleRefFilter#isOwner(...). if (group instanceof GroupDescription.Internal) { AccountGroup.UUID ownerUUID = ((GroupDescription.Internal) group).getOwnerGroupUUID(); isOwner = getUser().getEffectiveGroups().contains(ownerUUID) || canAdministrateServer(); diff --git a/java/com/google/gerrit/server/git/VisibleRefFilter.java b/java/com/google/gerrit/server/git/VisibleRefFilter.java index b1eea1d434..9ea0a63489 100644 --- a/java/com/google/gerrit/server/git/VisibleRefFilter.java +++ b/java/com/google/gerrit/server/git/VisibleRefFilter.java @@ -14,6 +14,7 @@ package com.google.gerrit.server.git; +import static com.google.common.base.Preconditions.checkNotNull; import static com.google.gerrit.reviewdb.client.RefNames.REFS_CACHE_AUTOMERGE; import static com.google.gerrit.reviewdb.client.RefNames.REFS_CHANGES; import static com.google.gerrit.reviewdb.client.RefNames.REFS_CONFIG; @@ -23,6 +24,7 @@ import static java.util.stream.Collectors.toMap; import com.google.gerrit.common.Nullable; import com.google.gerrit.extensions.restapi.AuthException; import com.google.gerrit.reviewdb.client.Account; +import com.google.gerrit.reviewdb.client.AccountGroup; import com.google.gerrit.reviewdb.client.Branch; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.Project; @@ -30,6 +32,8 @@ import com.google.gerrit.reviewdb.client.RefNames; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.IdentifiedUser; +import com.google.gerrit.server.account.GroupCache; +import com.google.gerrit.server.group.InternalGroup; import com.google.gerrit.server.notedb.ChangeNotes; import com.google.gerrit.server.notedb.ChangeNotes.Factory.ChangeNotesResult; import com.google.gerrit.server.permissions.ChangePermission; @@ -75,6 +79,7 @@ public class VisibleRefFilter extends AbstractAdvertiseRefsHook { @Nullable private final SearchingChangeCacheImpl changeCache; private final Provider db; private final Provider user; + private final GroupCache groupCache; private final PermissionBackend permissionBackend; private final PermissionBackend.ForProject perm; private final ProjectState projectState; @@ -90,6 +95,7 @@ public class VisibleRefFilter extends AbstractAdvertiseRefsHook { @Nullable SearchingChangeCacheImpl changeCache, Provider db, Provider user, + GroupCache groupCache, PermissionBackend permissionBackend, @Assisted ProjectState projectState, @Assisted Repository git) { @@ -98,6 +104,7 @@ public class VisibleRefFilter extends AbstractAdvertiseRefsHook { this.changeCache = changeCache; this.db = db; this.user = user; + this.groupCache = groupCache; this.permissionBackend = permissionBackend; this.perm = permissionBackend.user(user).database(db).project(projectState.getProject().getNameKey()); @@ -126,16 +133,21 @@ public class VisibleRefFilter extends AbstractAdvertiseRefsHook { } } - Account.Id userId; boolean viewMetadata; + boolean isAdmin; + Account.Id userId; + IdentifiedUser identifiedUser; if (user.get().isIdentifiedUser()) { viewMetadata = withUser.testOrFalse(GlobalPermission.ACCESS_DATABASE); - IdentifiedUser u = user.get().asIdentifiedUser(); - userId = u.getAccountId(); + isAdmin = withUser.testOrFalse(GlobalPermission.ADMINISTRATE_SERVER); + identifiedUser = user.get().asIdentifiedUser(); + userId = identifiedUser.getAccountId(); userEditPrefix = RefNames.refsEditPrefix(userId); } else { - userId = null; viewMetadata = false; + isAdmin = false; + userId = null; + identifiedUser = null; } Map result = new HashMap<>(); @@ -145,6 +157,7 @@ public class VisibleRefFilter extends AbstractAdvertiseRefsHook { String name = ref.getName(); Change.Id changeId; Account.Id accountId; + AccountGroup.UUID accountGroupUuid; if (name.startsWith(REFS_CACHE_AUTOMERGE) || (!showMetadata && isMetadata(name))) { continue; } else if (RefNames.isRefsEdit(name)) { @@ -158,10 +171,19 @@ public class VisibleRefFilter extends AbstractAdvertiseRefsHook { result.put(name, ref); } } else if ((accountId = Account.Id.fromRef(name)) != null) { - // Account ref is visible only to corresponding account. + // Account ref is visible only to the corresponding account. if (viewMetadata || (accountId.equals(userId) && canReadRef(name))) { result.put(name, ref); } + } else if ((accountGroupUuid = AccountGroup.UUID.fromRef(name)) != null) { + // Group ref is visible only to the corresponding owner group. + InternalGroup group = groupCache.get(accountGroupUuid).orElse(null); + if (viewMetadata + || (group != null + && isGroupOwner(group, identifiedUser, isAdmin) + && canReadRef(name))) { + result.put(name, ref); + } } else if (isTag(ref)) { // If its a tag, consider it later. if (ref.getObjectId() != null) { @@ -374,4 +396,13 @@ public class VisibleRefFilter extends AbstractAdvertiseRefsHook { } return true; } + + private boolean isGroupOwner( + InternalGroup group, @Nullable IdentifiedUser user, boolean isAdmin) { + checkNotNull(group); + + // Keep this logic in sync with GroupControl#isOwner(). + return isAdmin + || (user != null && user.getEffectiveGroups().contains(group.getOwnerGroupUUID())); + } } diff --git a/javatests/com/google/gerrit/acceptance/git/RefAdvertisementIT.java b/javatests/com/google/gerrit/acceptance/git/RefAdvertisementIT.java index a6698ef2f4..4b0dbfe26a 100644 --- a/javatests/com/google/gerrit/acceptance/git/RefAdvertisementIT.java +++ b/javatests/com/google/gerrit/acceptance/git/RefAdvertisementIT.java @@ -25,15 +25,19 @@ import com.google.common.base.Predicates; import com.google.common.collect.ImmutableList; import com.google.gerrit.acceptance.AbstractDaemonTest; import com.google.gerrit.acceptance.AcceptanceTestRequestScope; +import com.google.gerrit.acceptance.GerritConfig; import com.google.gerrit.acceptance.NoHttpd; import com.google.gerrit.acceptance.PushOneCommit; import com.google.gerrit.acceptance.Sandboxed; import com.google.gerrit.acceptance.TestAccount; +import com.google.gerrit.common.Nullable; import com.google.gerrit.common.data.AccessSection; import com.google.gerrit.common.data.GlobalCapability; import com.google.gerrit.common.data.Permission; import com.google.gerrit.extensions.api.changes.DraftInput; +import com.google.gerrit.extensions.api.groups.GroupInput; import com.google.gerrit.extensions.api.projects.BranchInput; +import com.google.gerrit.extensions.restapi.RestApiException; import com.google.gerrit.reviewdb.client.AccountGroup; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.Patch; @@ -54,6 +58,7 @@ import com.google.gerrit.testing.NoteDbMode; import com.google.gerrit.testing.TestChanges; import com.google.inject.Inject; import java.util.ArrayList; +import java.util.Arrays; import java.util.Collections; import java.util.List; import java.util.Map; @@ -76,6 +81,7 @@ public class RefAdvertisementIT extends AbstractDaemonTest { @Inject private AllUsersName allUsersName; private AccountGroup.UUID admins; + private AccountGroup.UUID nonInteractiveUsers; private ChangeData c1; private ChangeData c2; @@ -89,6 +95,11 @@ public class RefAdvertisementIT extends AbstractDaemonTest { @Before public void setUp() throws Exception { admins = groupCache.get(new AccountGroup.NameKey("Administrators")).orElse(null).getGroupUUID(); + nonInteractiveUsers = + groupCache + .get(new AccountGroup.NameKey("Non-Interactive Users")) + .orElse(null) + .getGroupUUID(); setUpPermissions(); setUpChanges(); } @@ -495,6 +506,68 @@ public class RefAdvertisementIT extends AbstractDaemonTest { } } + @Test + @Sandboxed + @GerritConfig(name = "user.writeGroupsToNoteDb", value = "true") + public void advertisedReferencesDontShowGroupBranchToOwnerWithoutRead() throws Exception { + createSelfOwnedGroup("Foos", user); + TestRepository userTestRepository = cloneProject(allUsers, user); + try (Git git = userTestRepository.git()) { + assertThat(getGroupRefs(git)).isEmpty(); + } + } + + @Test + @Sandboxed + @GerritConfig(name = "user.writeGroupsToNoteDb", value = "true") + public void advertisedReferencesOmitGroupBranchesOfNonOwnedGroups() throws Exception { + allow(allUsersName, RefNames.REFS_GROUPS + "*", Permission.READ, REGISTERED_USERS); + AccountGroup.UUID users = createGroup("Users", admins, user); + AccountGroup.UUID foos = createGroup("Foos", users); + AccountGroup.UUID bars = createSelfOwnedGroup("Bars", user); + TestRepository userTestRepository = cloneProject(allUsers, user); + try (Git git = userTestRepository.git()) { + assertThat(getGroupRefs(git)) + .containsExactly(RefNames.refsGroups(foos), RefNames.refsGroups(bars)); + } + } + + @Test + @Sandboxed + @GerritConfig(name = "user.writeGroupsToNoteDb", value = "true") + public void advertisedReferencesIncludeAllGroupBranchesWithAccessDatabase() throws Exception { + allowGlobalCapabilities(REGISTERED_USERS, GlobalCapability.ACCESS_DATABASE); + AccountGroup.UUID users = createGroup("Users", admins); + TestRepository userTestRepository = cloneProject(allUsers, user); + try (Git git = userTestRepository.git()) { + assertThat(getGroupRefs(git)) + .containsExactly( + RefNames.refsGroups(admins), + RefNames.refsGroups(nonInteractiveUsers), + RefNames.refsGroups(users)); + } + } + + @Test + @GerritConfig(name = "user.writeGroupsToNoteDb", value = "true") + public void advertisedReferencesIncludeAllGroupBranchesForAdmins() throws Exception { + allow(allUsersName, RefNames.REFS_GROUPS + "*", Permission.READ, REGISTERED_USERS); + allowGlobalCapabilities(REGISTERED_USERS, GlobalCapability.ADMINISTRATE_SERVER); + try { + AccountGroup.UUID users = createGroup("Users", admins); + TestRepository userTestRepository = cloneProject(allUsers, user); + try (Git git = userTestRepository.git()) { + assertThat(getGroupRefs(git)) + .containsExactly( + RefNames.refsGroups(admins), + RefNames.refsGroups(nonInteractiveUsers), + RefNames.refsGroups(users)); + } + } finally { + removeGlobalCapabilities(REGISTERED_USERS, GlobalCapability.ADMINISTRATE_SERVER); + } + } + @Test public void advertisedReferencesOmitPrivateChangesOfOtherUsers() throws Exception { allow("refs/heads/master", Permission.READ, REGISTERED_USERS); @@ -627,6 +700,10 @@ public class RefAdvertisementIT extends AbstractDaemonTest { return getRefs(git, RefNames::isRefsUsers); } + private List getGroupRefs(Git git) throws Exception { + return getRefs(git, RefNames::isRefsGroups); + } + private List getRefs(Git git, Predicate predicate) throws Exception { return git.lsRemote().call().stream().map(Ref::getName).filter(predicate).collect(toList()); } @@ -688,4 +765,20 @@ public class RefAdvertisementIT extends AbstractDaemonTest { assertWithMessage("%s not found in %s", psId, cd.patchSets()).that(ps).isNotNull(); return ObjectId.fromString(ps.getRevision().get()); } + + private AccountGroup.UUID createSelfOwnedGroup(String name, TestAccount... members) + throws RestApiException { + return createGroup(name, null, members); + } + + private AccountGroup.UUID createGroup( + String name, @Nullable AccountGroup.UUID ownerGroup, TestAccount... members) + throws RestApiException { + GroupInput groupInput = new GroupInput(); + groupInput.name = name(name); + groupInput.ownerId = ownerGroup != null ? ownerGroup.get() : null; + groupInput.members = + Arrays.stream(members).map(m -> String.valueOf(m.id.get())).collect(toList()); + return new AccountGroup.UUID(gApi.groups().create(groupInput).get().id); + } } From a0a603ed37c250a1ee3efa00060c1c6927fc2a81 Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Tue, 14 Nov 2017 11:04:35 +0100 Subject: [PATCH 6/6] Grant read access on group branches To allow group owners to see the group refs in All-Users they need to have read permissions on the group refs. Assign these read permissions explicitly on All-Users and don't rely on inherited read permissions. By default on All-Projects anonymous users have read permissions for all refs but if admins change this the group refs should still be accessible by the group owners. Assigning read permissions on the group refs for all users is okay since VisibleRefFilter ignores read permissions on group refs for non-group-owners. This change assigns the read permissions on the group refs for new sites via AllUsersCreator. In addition it contains a schema migration to also assign these permissions for existing sites. Change-Id: Iab4dc3c1be375e212f1faddcfbd11400b758e518 Signed-off-by: Edwin Kempin --- .../gerrit/server/schema/AllUsersCreator.java | 6 ++ .../gerrit/server/schema/SchemaVersion.java | 2 +- .../gerrit/server/schema/Schema_164.java | 85 +++++++++++++++++++ .../gerrit/acceptance/api/group/GroupsIT.java | 39 +++++++++ 4 files changed, 131 insertions(+), 1 deletion(-) create mode 100644 java/com/google/gerrit/server/schema/Schema_164.java diff --git a/java/com/google/gerrit/server/schema/AllUsersCreator.java b/java/com/google/gerrit/server/schema/AllUsersCreator.java index 3049e86ce2..42c86c1f72 100644 --- a/java/com/google/gerrit/server/schema/AllUsersCreator.java +++ b/java/com/google/gerrit/server/schema/AllUsersCreator.java @@ -112,6 +112,12 @@ public class AllUsersCreator { grant(config, defaults, Permission.CREATE, admin); } + // Grant read permissions on the group branches to all users. + // This allows group owners to see the group refs. VisibleRefFilter ensures that read + // permissions for non-group-owners are ignored. + AccessSection groups = config.getAccessSection(RefNames.REFS_GROUPS + "*", true); + grant(config, groups, Permission.READ, false, true, registered); + config.commit(md); } } diff --git a/java/com/google/gerrit/server/schema/SchemaVersion.java b/java/com/google/gerrit/server/schema/SchemaVersion.java index 2cb57a81f8..780b70a0b0 100644 --- a/java/com/google/gerrit/server/schema/SchemaVersion.java +++ b/java/com/google/gerrit/server/schema/SchemaVersion.java @@ -35,7 +35,7 @@ import java.util.concurrent.TimeUnit; /** A version of the database schema. */ public abstract class SchemaVersion { /** The current schema version. */ - public static final Class C = Schema_163.class; + public static final Class C = Schema_164.class; public static int getBinaryVersion() { return guessVersion(C); diff --git a/java/com/google/gerrit/server/schema/Schema_164.java b/java/com/google/gerrit/server/schema/Schema_164.java new file mode 100644 index 0000000000..fca0ae8825 --- /dev/null +++ b/java/com/google/gerrit/server/schema/Schema_164.java @@ -0,0 +1,85 @@ +// Copyright (C) 2017 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.gerrit.server.schema; + +import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS; +import static com.google.gerrit.server.schema.AclUtil.grant; + +import com.google.gerrit.common.data.AccessSection; +import com.google.gerrit.common.data.Permission; +import com.google.gerrit.reviewdb.client.RefNames; +import com.google.gerrit.reviewdb.server.ReviewDb; +import com.google.gerrit.server.GerritPersonIdent; +import com.google.gerrit.server.config.AllUsersName; +import com.google.gerrit.server.extensions.events.GitReferenceUpdated; +import com.google.gerrit.server.git.GitRepositoryManager; +import com.google.gerrit.server.git.MetaDataUpdate; +import com.google.gerrit.server.git.ProjectConfig; +import com.google.gerrit.server.group.SystemGroupBackend; +import com.google.gwtorm.server.OrmException; +import com.google.inject.Inject; +import com.google.inject.Provider; +import java.io.IOException; +import java.sql.SQLException; +import org.eclipse.jgit.errors.ConfigInvalidException; +import org.eclipse.jgit.lib.PersonIdent; +import org.eclipse.jgit.lib.Repository; + +/** Grant read on group branches */ +public class Schema_164 extends SchemaVersion { + private static final String COMMIT_MSG = "Grant read permissions on group branches"; + + private final GitRepositoryManager repoManager; + private final AllUsersName allUsersName; + private final SystemGroupBackend systemGroupBackend; + private final PersonIdent serverUser; + + @Inject + Schema_164( + Provider prior, + GitRepositoryManager repoManager, + AllUsersName allUsersName, + SystemGroupBackend systemGroupBackend, + @GerritPersonIdent PersonIdent serverUser) { + super(prior); + this.repoManager = repoManager; + this.allUsersName = allUsersName; + this.systemGroupBackend = systemGroupBackend; + this.serverUser = serverUser; + } + + @Override + protected void migrateData(ReviewDb db, UpdateUI ui) throws OrmException, SQLException { + try (Repository git = repoManager.openRepository(allUsersName); + MetaDataUpdate md = new MetaDataUpdate(GitReferenceUpdated.DISABLED, allUsersName, git)) { + md.getCommitBuilder().setAuthor(serverUser); + md.getCommitBuilder().setCommitter(serverUser); + md.setMessage(COMMIT_MSG); + + ProjectConfig config = ProjectConfig.read(md); + AccessSection groups = config.getAccessSection(RefNames.REFS_GROUPS + "*", true); + grant( + config, + groups, + Permission.READ, + false, + true, + systemGroupBackend.getGroup(REGISTERED_USERS)); + config.commit(md); + } catch (IOException | ConfigInvalidException e) { + throw new OrmException("Failed to grant read permissions on group branches", e); + } + } +} diff --git a/javatests/com/google/gerrit/acceptance/api/group/GroupsIT.java b/javatests/com/google/gerrit/acceptance/api/group/GroupsIT.java index ae3e63534c..93f3d326d7 100644 --- a/javatests/com/google/gerrit/acceptance/api/group/GroupsIT.java +++ b/javatests/com/google/gerrit/acceptance/api/group/GroupsIT.java @@ -33,8 +33,12 @@ import com.google.gerrit.acceptance.PushOneCommit; import com.google.gerrit.acceptance.Sandboxed; import com.google.gerrit.acceptance.TestAccount; import com.google.gerrit.common.TimeUtil; +import com.google.gerrit.common.data.AccessSection; +import com.google.gerrit.common.data.GroupDescription; import com.google.gerrit.common.data.GroupReference; import com.google.gerrit.common.data.Permission; +import com.google.gerrit.common.data.PermissionRule; +import com.google.gerrit.common.data.PermissionRule.Action; import com.google.gerrit.extensions.api.changes.ReviewInput; import com.google.gerrit.extensions.api.groups.GroupApi; import com.google.gerrit.extensions.api.groups.GroupInput; @@ -56,7 +60,9 @@ import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.AccountGroup; import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.reviewdb.client.RefNames; +import com.google.gerrit.server.account.GroupBackend; import com.google.gerrit.server.account.GroupIncludeCache; +import com.google.gerrit.server.git.ProjectConfig; import com.google.gerrit.server.group.InternalGroup; import com.google.gerrit.server.group.SystemGroupBackend; import com.google.gerrit.server.group.db.Groups; @@ -100,6 +106,7 @@ public class GroupsIT extends AbstractDaemonTest { @Inject private Groups groups; @Inject private GroupIncludeCache groupIncludeCache; + @Inject private GroupBackend groupBackend; @Test public void systemGroupCanBeRetrievedFromIndex() throws Exception { @@ -884,6 +891,38 @@ public class GroupsIT extends AbstractDaemonTest { } } + @Test + public void defaultPermissionsOnGroupBranches() throws Exception { + assertPermission( + allUsers, RefNames.REFS_GROUPS + "*", Permission.READ, groupRef(REGISTERED_USERS)); + } + + private GroupReference groupRef(AccountGroup.UUID groupUuid) { + GroupDescription.Basic groupDescription = groupBackend.get(groupUuid); + return new GroupReference(groupDescription.getGroupUUID(), groupDescription.getName()); + } + + private void assertPermission( + Project.NameKey project, String ref, String permission, GroupReference groupReference) + throws IOException { + ProjectConfig cfg = projectCache.checkedGet(project).getConfig(); + AccessSection accessSection = cfg.getAccessSection(ref); + assertThat(accessSection).isNotNull(); + Permission readPermission = accessSection.getPermission(permission); + assertThat(readPermission).isNotNull(); + assertThat(readPermission.getName()).isEqualTo(permission); + assertThat(readPermission.getExclusiveGroup()).isTrue(); + assertThat(readPermission.getLabel()).isNull(); + + PermissionRule rule = readPermission.getRule(groupReference); + assertThat(rule).isNotNull(); + assertThat(rule.getGroup()).isEqualTo(groupReference); + assertThat(rule.getAction()).isEqualTo(Action.ALLOW); + assertThat(rule.getForce()).isFalse(); + assertThat(rule.getMin()).isEqualTo(0); + assertThat(rule.getMax()).isEqualTo(0); + } + private void pushToGroupBranchForReviewAndSubmit(Project.NameKey project, String expectedError) throws Exception { grantLabel(