Allow to configure relevant groups
Guessing relevant groups is not very reliable since it depends on which
groups are mentioned in the ACLs and which projects are currently
cached. Due to this checking the account visibility often doesn't work
well for external group backends when account visibility is set to
'SAME_GROUP' or 'VISIBLE_GROUP'.
To make this more reliable we add a new configuration parameter that
allows to configure external groups that should always be considered as
relevant. E.g. at Google we could make use of this to configure a group
as relevant that contains all Googlers, so that Googlers can always see
all other Googlers.
Release-Notes: skip
Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: Ie89fb10e0bff3201a38169f31d04c7f9cd66f57c
diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt
index d5b247e..3a775a7 100644
--- a/Documentation/config-gerrit.txt
+++ b/Documentation/config-gerrit.txt
@@ -2761,6 +2761,39 @@
when this parameter is removed and the system group uses the default
name again.
+[[groups.relevantGroup]]groups.relevantGroup::
++
+UUID of an external group that should always be considered as relevant
+when checking whether an account is visible.
++
+This setting is only relevant for external group backends and only if
+the link:#accounts.visibility[account visibility] is set to
+`SAME_GROUP` or `VISIBLE_GROUP`.
++
+If the link:#accounts.visibility[account visibility] is set to
+`SAME_GROUP` or `VISIBLE_GROUP` users should see all accounts that are
+a member of a group that contains themselves or that is visible to
+them. Checking this would require getting all groups of the current
+user and all groups of the accounts for which the visibility is being
+checked, but since getting all groups that a user is a member of is
+expensive for external group backends Gerrit doesn't query these groups
+but instead guesses the relevant groups. Guessing relevant groups
+limits the inspected groups to all groups that are mentioned in the
+ACLs of the projects that are currently cached (i.e. all groups that
+are listed in the link:config-project-config.html#file-groups[groups]
+files of the cached projects). This is not very reliable since it
+depends on which groups are mentioned in the ACLs and which projects
+are currently cached. To make this more reliable this configuration
+parameter allows to configure external groups that should always be
+considered as relevant.
++
+As said this setting is only relevant for external group backends. In
+Gerrit core this is only the LDAP backend, but it may apply to further
+group backends that are added by plugins.
++
+This parameter may be added multiple times to specify multiple relevant
+groups.
+
[[has-operand-alias]]
=== Section has operand alias
diff --git a/java/com/google/gerrit/server/group/testing/TestGroupBackend.java b/java/com/google/gerrit/server/group/testing/TestGroupBackend.java
index d81992a..b0e270c 100644
--- a/java/com/google/gerrit/server/group/testing/TestGroupBackend.java
+++ b/java/com/google/gerrit/server/group/testing/TestGroupBackend.java
@@ -33,7 +33,7 @@
/** Implementation of GroupBackend for tests. */
public class TestGroupBackend implements GroupBackend {
- private static final String PREFIX = "testbackend:";
+ public static final String PREFIX = "testbackend:";
private final Map<AccountGroup.UUID, GroupDescription.Basic> groups = new HashMap<>();
private final Map<Account.Id, GroupMembership> memberships = new HashMap<>();
diff --git a/java/com/google/gerrit/server/project/ProjectCacheImpl.java b/java/com/google/gerrit/server/project/ProjectCacheImpl.java
index 67c031e..7fdd113 100644
--- a/java/com/google/gerrit/server/project/ProjectCacheImpl.java
+++ b/java/com/google/gerrit/server/project/ProjectCacheImpl.java
@@ -24,6 +24,7 @@
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.ImmutableSortedSet;
import com.google.common.collect.Sets;
+import com.google.common.collect.Streams;
import com.google.common.flogger.FluentLogger;
import com.google.common.hash.Hashing;
import com.google.common.util.concurrent.Futures;
@@ -54,6 +55,7 @@
import com.google.gerrit.server.config.AllProjectsConfigProvider;
import com.google.gerrit.server.config.AllProjectsName;
import com.google.gerrit.server.config.AllUsersName;
+import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.logging.Metadata;
import com.google.gerrit.server.logging.TraceContext;
@@ -67,6 +69,7 @@
import com.google.protobuf.ByteString;
import java.io.IOException;
import java.time.Duration;
+import java.util.Arrays;
import java.util.Objects;
import java.util.Optional;
import java.util.Set;
@@ -75,6 +78,7 @@
import java.util.concurrent.locks.ReentrantLock;
import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.errors.RepositoryNotFoundException;
+import org.eclipse.jgit.lib.Config;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.Ref;
import org.eclipse.jgit.lib.Repository;
@@ -158,6 +162,7 @@
};
}
+ private final Config config;
private final AllProjectsName allProjectsName;
private final AllUsersName allUsersName;
private final LoadingCache<Project.NameKey, CachedProjectConfig> inMemoryProjectCache;
@@ -169,13 +174,15 @@
@Inject
ProjectCacheImpl(
- final AllProjectsName allProjectsName,
- final AllUsersName allUsersName,
+ @GerritServerConfig Config config,
+ AllProjectsName allProjectsName,
+ AllUsersName allUsersName,
@Named(CACHE_NAME) LoadingCache<Project.NameKey, CachedProjectConfig> inMemoryProjectCache,
@Named(CACHE_LIST) LoadingCache<ListKey, ImmutableSortedSet<Project.NameKey>> list,
Provider<ProjectIndexer> indexer,
MetricMaker metricMaker,
ProjectState.Factory projectStateFactory) {
+ this.config = config;
this.allProjectsName = allProjectsName;
this.allUsersName = allUsersName;
this.inMemoryProjectCache = inMemoryProjectCache;
@@ -293,13 +300,16 @@
@Override
public Set<AccountGroup.UUID> guessRelevantGroupUUIDs() {
try (Timer0.Context ignored = guessRelevantGroupsLatency.start()) {
- return all().stream()
- .map(n -> inMemoryProjectCache.getIfPresent(n))
- .filter(Objects::nonNull)
- .flatMap(p -> p.getAllGroupUUIDs().stream())
- // getAllGroupUUIDs shouldn't really return null UUIDs, but harden
- // against them just in case there is a bug or corner case.
- .filter(id -> id != null && id.get() != null)
+ return Streams.concat(
+ Arrays.stream(config.getStringList("groups", /* subsection= */ null, "relevantGroup"))
+ .map(AccountGroup::uuid),
+ all().stream()
+ .map(n -> inMemoryProjectCache.getIfPresent(n))
+ .filter(Objects::nonNull)
+ .flatMap(p -> p.getAllGroupUUIDs().stream())
+ // getAllGroupUUIDs shouldn't really return null UUIDs, but harden
+ // against them just in case there is a bug or corner case.
+ .filter(id -> id != null && id.get() != null))
.collect(toSet());
}
}
diff --git a/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java b/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java
index ec2e751..111f8c9 100644
--- a/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java
@@ -78,6 +78,7 @@
import com.google.gerrit.acceptance.testsuite.account.TestSshKeys;
import com.google.gerrit.acceptance.testsuite.group.GroupOperations;
import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
+import com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate;
import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations;
import com.google.gerrit.acceptance.testsuite.request.SshSessionFactory;
import com.google.gerrit.common.Nullable;
@@ -130,10 +131,12 @@
import com.google.gerrit.httpd.CacheBasedWebSession;
import com.google.gerrit.server.ExceptionHook;
import com.google.gerrit.server.ServerInitiated;
+import com.google.gerrit.server.account.AccountControl;
import com.google.gerrit.server.account.AccountProperties;
import com.google.gerrit.server.account.AccountState;
import com.google.gerrit.server.account.AccountsUpdate;
import com.google.gerrit.server.account.Emails;
+import com.google.gerrit.server.account.GroupMembership;
import com.google.gerrit.server.account.VersionedAuthorizedKeys;
import com.google.gerrit.server.account.externalids.DuplicateExternalIdKeyException;
import com.google.gerrit.server.account.externalids.ExternalId;
@@ -144,6 +147,7 @@
import com.google.gerrit.server.config.AuthConfig;
import com.google.gerrit.server.extensions.events.GitReferenceUpdated;
import com.google.gerrit.server.git.meta.MetaDataUpdate;
+import com.google.gerrit.server.group.testing.TestGroupBackend;
import com.google.gerrit.server.index.account.AccountIndexer;
import com.google.gerrit.server.index.account.StalenessChecker;
import com.google.gerrit.server.notedb.Sequences;
@@ -175,6 +179,7 @@
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.concurrent.atomic.AtomicInteger;
import java.util.stream.Collectors;
+import java.util.stream.StreamSupport;
import javax.servlet.http.HttpServletResponse;
import org.apache.http.HttpResponse;
import org.apache.http.client.ClientProtocolException;
@@ -241,6 +246,7 @@
@Inject private ExternalIdKeyFactory externalIdKeyFactory;
@Inject private ExternalIdFactory externalIdFactory;
@Inject private AuthConfig authConfig;
+ @Inject private AccountControl.Factory accountControlFactory;
@Inject protected Emails emails;
@@ -3118,6 +3124,139 @@
.isNotEqualTo(updatedUserState.account().metaId());
}
+ @Test
+ @GerritConfig(name = "accounts.visibility", value = "SAME_GROUP")
+ public void accountsCanSeeEachOtherThroughASharedExternalGroupOnlyWhenTheGroupIsMentionedInAcls()
+ throws Exception {
+ TestAccount user2 = accountCreator.user2();
+
+ // user and user2 cannot see each other because they do not share a Gerrit internal group
+ assertThat(
+ accountControlFactory.get(identifiedUserFactory.create(user.id())).canSee(user2.id()))
+ .isFalse();
+ assertThat(
+ accountControlFactory.get(identifiedUserFactory.create(user2.id())).canSee(user.id()))
+ .isFalse();
+
+ // Configure an external group backend that has a single group that contains all users.
+ TestGroupBackend testGroupBackend = createTestGroupBackendWithAllUsersGroup("AllUsers");
+ try (ExtensionRegistry.Registration registration =
+ extensionRegistry.newRegistration().add(testGroupBackend)) {
+ // user and user2 cannot see each other although the external AllUsers group contains both
+ // users. That's because this group is not detected as relevant and hence its memberships are
+ // not checked.
+ assertThat(
+ accountControlFactory.get(identifiedUserFactory.create(user.id())).canSee(user2.id()))
+ .isFalse();
+ assertThat(
+ accountControlFactory.get(identifiedUserFactory.create(user2.id())).canSee(user.id()))
+ .isFalse();
+
+ // Add ACL for the external group.
+ projectOperations
+ .project(project)
+ .forUpdate()
+ .add(
+ TestProjectUpdate.allowLabel("Code-Review")
+ .range(0, 1)
+ .ref("refs/heads/*")
+ .group(AccountGroup.uuid(TestGroupBackend.PREFIX + "AllUsers"))
+ .build())
+ .update();
+
+ // user and user2 can now see each other because the external AllUsers group that contains
+ // both users is guessed as relevant now that permissions are assigned to this group.
+ assertThat(
+ accountControlFactory.get(identifiedUserFactory.create(user.id())).canSee(user2.id()))
+ .isTrue();
+ assertThat(
+ accountControlFactory.get(identifiedUserFactory.create(user2.id())).canSee(user.id()))
+ .isTrue();
+ }
+ }
+
+ @Test
+ @GerritConfig(name = "accounts.visibility", value = "SAME_GROUP")
+ @GerritConfig(name = "groups.relevantGroup", value = "testbackend:AllUsers")
+ public void accountsCanSeeEachOtherThroughASharedExternalGroupThatIsConfiguredAsRelevant()
+ throws Exception {
+ TestAccount user2 = accountCreator.user2();
+
+ // user and user2 cannot see each other because they do not share a Gerrit internal group
+ assertThat(
+ accountControlFactory.get(identifiedUserFactory.create(user.id())).canSee(user2.id()))
+ .isFalse();
+ assertThat(
+ accountControlFactory.get(identifiedUserFactory.create(user2.id())).canSee(user.id()))
+ .isFalse();
+
+ // Check that the configured relevant group is included into the guessed groups.
+ assertThat(projectCache.guessRelevantGroupUUIDs())
+ .contains(AccountGroup.uuid("testbackend:AllUsers"));
+
+ // Configure an external group backend that has a single group that contains all users.
+ TestGroupBackend testGroupBackend = createTestGroupBackendWithAllUsersGroup("AllUsers");
+ try (ExtensionRegistry.Registration registration =
+ extensionRegistry.newRegistration().add(testGroupBackend)) {
+ // user and user2 can see each other since the external AllUsers that contains both users has
+ // been configured as a relevant group.
+ assertThat(
+ accountControlFactory.get(identifiedUserFactory.create(user.id())).canSee(user2.id()))
+ .isTrue();
+ assertThat(
+ accountControlFactory.get(identifiedUserFactory.create(user2.id())).canSee(user.id()))
+ .isTrue();
+ }
+ }
+
+ private TestGroupBackend createTestGroupBackendWithAllUsersGroup(String nameOfAllUsersGroup)
+ throws IOException {
+ TestGroupBackend testGroupBackend = new TestGroupBackend();
+
+ AccountGroup.UUID allUsersGroupUuid =
+ testGroupBackend.create(nameOfAllUsersGroup).getGroupUUID();
+
+ GroupMembership testGroupMembership =
+ new GroupMembership() {
+ @Override
+ public Set<AccountGroup.UUID> intersection(Iterable<AccountGroup.UUID> groupUuids) {
+ return StreamSupport.stream(groupUuids.spliterator(), /* parallel= */ false)
+ .filter(this::contains)
+ .collect(toSet());
+ }
+
+ @Override
+ public Set<AccountGroup.UUID> getKnownGroups() {
+ // Typically for external group backends it's too expensive to query all groups that the
+ // user is a member of. Instead limit the group membership check to groups that are
+ // guessed to be relevant.
+ return projectCache.guessRelevantGroupUUIDs().stream()
+ // filter out groups of other group backends and groups of this group backend that
+ // don't exist
+ .filter(
+ uuid -> testGroupBackend.handles(uuid) && testGroupBackend.get(uuid) != null)
+ .collect(toImmutableSet());
+ }
+
+ @Override
+ public boolean containsAnyOf(Iterable<AccountGroup.UUID> groupUuids) {
+ return StreamSupport.stream(groupUuids.spliterator(), /* parallel= */ false)
+ .anyMatch(this::contains);
+ }
+
+ @Override
+ public boolean contains(AccountGroup.UUID groupUuid) {
+ return allUsersGroupUuid.equals(groupUuid);
+ }
+ };
+
+ accounts
+ .allIds()
+ .forEach(accountId -> testGroupBackend.setMembershipsOf(accountId, testGroupMembership));
+
+ return testGroupBackend;
+ }
+
private void assertExternalIds(Account.Id accountId, ImmutableSet<String> extIds)
throws Exception {
assertThat(
