diff --git a/java/com/google/gerrit/reviewdb/client/RefNames.java b/java/com/google/gerrit/reviewdb/client/RefNames.java index 1839d1192a..995b0203ce 100644 --- a/java/com/google/gerrit/reviewdb/client/RefNames.java +++ b/java/com/google/gerrit/reviewdb/client/RefNames.java @@ -224,6 +224,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 ed86a922f0..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()); @@ -118,22 +125,29 @@ 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; 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<>(); @@ -143,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)) { @@ -156,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) { @@ -180,6 +204,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); + } } } @@ -327,15 +357,17 @@ 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) { 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); @@ -364,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/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/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/api/group/GroupsIT.java b/javatests/com/google/gerrit/acceptance/api/group/GroupsIT.java index c090558ae9..631f96ef2a 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 { @@ -935,6 +942,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( diff --git a/javatests/com/google/gerrit/acceptance/git/RefAdvertisementIT.java b/javatests/com/google/gerrit/acceptance/git/RefAdvertisementIT.java index 1ac135eb44..4b0dbfe26a 100644 --- a/javatests/com/google/gerrit/acceptance/git/RefAdvertisementIT.java +++ b/javatests/com/google/gerrit/acceptance/git/RefAdvertisementIT.java @@ -21,18 +21,23 @@ 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; +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; @@ -53,11 +58,12 @@ 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; +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; @@ -75,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; @@ -88,19 +95,32 @@ 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(); } 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) { @@ -451,23 +471,114 @@ 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.REFS_USERS_SELF, + RefNames.refsUsers(user.id), + RefNames.refsUsers(admin.id)); + } + } finally { + removeGlobalCapabilities(REGISTERED_USERS, GlobalCapability.ACCESS_DATABASE); + } + } + + @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); 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); } } @@ -477,17 +588,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); } } @@ -553,11 +658,12 @@ 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, + RefNames.REFS_CONFIG); 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"); } @@ -565,7 +671,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(); @@ -587,6 +692,22 @@ 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 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()); + } + /** * Assert that refs seen by a non-admin user match expected. * @@ -644,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); + } } 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);