commit 29ace6c38ac513f7ec56ca425230d5712c081043
Author: Luke Granger-Brown <git@lukegb.com>
Date: Wed Sep 21 03:15:38 2022 +0100
Add support for usernames and groups
Change-Id: I3ba8527f66216d08e555a6ac4451fe0d1e090de5
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolver.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolver.java
index 70009591..6dc596c9 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolver.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolver.java
@@ -17,6 +17,8 @@ package com.google.gerrit.plugins.codeowners.backend;
import static com.google.common.base.Preconditions.checkState;
import static com.google.common.collect.ImmutableMap.toImmutableMap;
import static com.google.common.collect.ImmutableSet.toImmutableSet;
+import static com.google.common.collect.ImmutableSetMultimap.flatteningToImmutableSetMultimap;
+import static com.google.common.collect.ImmutableSetMultimap.toImmutableSetMultimap;
import static com.google.gerrit.plugins.codeowners.backend.CodeOwnersInternalServerErrorException.newInternalServerError;
import static java.util.Objects.requireNonNull;
@@ -25,6 +27,7 @@ import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableMultimap;
import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.ImmutableSetMultimap;
import com.google.common.collect.Iterables;
import com.google.common.collect.Streams;
import com.google.common.flogger.FluentLogger;
@@ -33,17 +36,24 @@ import com.google.gerrit.entities.Project;
import com.google.gerrit.metrics.Timer0;
import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginConfiguration;
import com.google.gerrit.plugins.codeowners.metrics.CodeOwnerMetrics;
+import com.google.gerrit.server.AnonymousUser;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.account.AccountCache;
import com.google.gerrit.server.account.AccountControl;
import com.google.gerrit.server.account.AccountState;
+import com.google.gerrit.server.account.GroupBackend;
+import com.google.gerrit.server.account.GroupBackends;
+import com.google.gerrit.server.account.InternalGroupBackend;
import com.google.gerrit.server.account.externalids.ExternalId;
import com.google.gerrit.server.account.externalids.ExternalIdCache;
import com.google.gerrit.server.permissions.GlobalPermission;
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
+import com.google.gerrit.server.util.RequestContext;
+import com.google.gerrit.server.util.ThreadLocalRequestContext;
import com.google.inject.Inject;
+import com.google.inject.OutOfScopeException;
import com.google.inject.Provider;
import java.io.IOException;
import java.nio.file.Path;
@@ -102,6 +112,8 @@ public class CodeOwnerResolver {
@VisibleForTesting public static final String ALL_USERS_WILDCARD = "*";
+ public static final String GROUP_PREFIX = "group:";
+
private final CodeOwnersPluginConfiguration codeOwnersPluginConfiguration;
private final PermissionBackend permissionBackend;
private final Provider<CurrentUser> currentUser;
@@ -112,6 +124,8 @@ public class CodeOwnerResolver {
private final CodeOwnerMetrics codeOwnerMetrics;
private final UnresolvedImportFormatter unresolvedImportFormatter;
private final TransientCodeOwnerCache transientCodeOwnerCache;
+ private final InternalGroupBackend groupBackend;
+ private final ThreadLocalRequestContext context;
// Enforce visibility by default.
private boolean enforceVisibility = true;
@@ -132,7 +146,9 @@ public class CodeOwnerResolver {
PathCodeOwners.Factory pathCodeOwnersFactory,
CodeOwnerMetrics codeOwnerMetrics,
UnresolvedImportFormatter unresolvedImportFormatter,
- TransientCodeOwnerCache transientCodeOwnerCache) {
+ TransientCodeOwnerCache transientCodeOwnerCache,
+ InternalGroupBackend groupBackend,
+ ThreadLocalRequestContext context) {
this.codeOwnersPluginConfiguration = codeOwnersPluginConfiguration;
this.permissionBackend = permissionBackend;
this.currentUser = currentUser;
@@ -143,6 +159,8 @@ public class CodeOwnerResolver {
this.codeOwnerMetrics = codeOwnerMetrics;
this.unresolvedImportFormatter = unresolvedImportFormatter;
this.transientCodeOwnerCache = transientCodeOwnerCache;
+ this.groupBackend = groupBackend;
+ this.context = context;
}
/**
@@ -361,6 +379,12 @@ public class CodeOwnerResolver {
"cannot resolve code owner email %s: no account with this email exists",
CodeOwnerResolver.ALL_USERS_WILDCARD));
}
+ if (codeOwnerReference.email().startsWith(GROUP_PREFIX)) {
+ return OptionalResultWithMessages.createEmpty(
+ String.format(
+ "cannot resolve code owner email %s: this is a group",
+ codeOwnerReference.email()));
+ }
ImmutableList.Builder<String> messageBuilder = ImmutableList.builder();
AtomicBoolean ownedByAllUsers = new AtomicBoolean(false);
@@ -405,9 +429,53 @@ public class CodeOwnerResolver {
ImmutableMultimap<CodeOwnerReference, CodeOwnerAnnotation> annotations) {
requireNonNull(codeOwnerReferences, "codeOwnerReferences");
+ ImmutableSet<String> groupsToResolve =
+ codeOwnerReferences.stream()
+ .map(CodeOwnerReference::email)
+ .filter(ref -> ref.startsWith(GROUP_PREFIX))
+ .map(ref -> ref.substring(GROUP_PREFIX.length()))
+ .collect(toImmutableSet());
+
+ // When we call GroupBackends.findExactSuggestion we need to ensure that we
+ // have a user in context. This is because the suggestion backend is
+ // likely to want to try to check that we can actually see the group it's
+ // returning (which we also check for explicitly, because I have trust
+ // issues).
+ RequestContext oldCtx = context.getContext();
+ // Check if we have a user in the context at all...
+ try {
+ oldCtx.getUser();
+ } catch (OutOfScopeException | NullPointerException e) {
+ // Nope.
+ RequestContext newCtx = () -> {
+ return new AnonymousUser();
+ };
+ context.setContext(newCtx);
+ }
+ ImmutableSetMultimap<String, CodeOwner> resolvedGroups = null;
+ try {
+ resolvedGroups =
+ groupsToResolve.stream()
+ .map(groupName -> GroupBackends.findExactSuggestion(groupBackend, groupName))
+ .filter(groupRef -> groupRef != null)
+ .filter(groupRef -> groupBackend.isVisibleToAll(groupRef.getUUID()))
+ .map(groupRef -> groupBackend.get(groupRef.getUUID()))
+ .collect(flatteningToImmutableSetMultimap(
+ groupRef -> GROUP_PREFIX + groupRef.getName(),
+ groupRef -> accountCache
+ .get(ImmutableSet.copyOf(groupRef.getMembers()))
+ .values().stream()
+ .map(accountState -> CodeOwner.create(accountState.account().id()))));
+ } finally {
+ context.setContext(oldCtx);
+ }
+ ImmutableSetMultimap<CodeOwner, String> usersToGroups =
+ resolvedGroups.inverse();
+
ImmutableSet<String> emailsToResolve =
codeOwnerReferences.stream()
.map(CodeOwnerReference::email)
+ .filter(ref -> !ref.startsWith(GROUP_PREFIX))
.filter(filterOutAllUsersWildCard(ownedByAllUsers))
.collect(toImmutableSet());
@@ -442,7 +510,8 @@ public class CodeOwnerResolver {
ImmutableMap<String, CodeOwner> codeOwnersByEmail =
accountsByEmail.map(mapToCodeOwner()).collect(toImmutableMap(Pair::key, Pair::value));
- if (codeOwnersByEmail.keySet().size() < emailsToResolve.size()) {
+ if (codeOwnersByEmail.keySet().size() < emailsToResolve.size() ||
+ resolvedGroups.keySet().size() < groupsToResolve.size()) {
hasUnresolvedCodeOwners.set(true);
}
@@ -456,7 +525,9 @@ public class CodeOwnerResolver {
cachedCodeOwnersByEmail.entrySet().stream()
.filter(e -> e.getValue().isPresent())
.map(e -> Pair.of(e.getKey(), e.getValue().get()));
- Streams.concat(newlyResolvedCodeOwnersStream, cachedCodeOwnersStream)
+ Stream<Pair<String, CodeOwner>> resolvedGroupsCodeOwnersStream =
+ resolvedGroups.entries().stream().map(e -> Pair.of(e.getKey(), e.getValue()));
+ Streams.concat(Streams.concat(newlyResolvedCodeOwnersStream, cachedCodeOwnersStream), resolvedGroupsCodeOwnersStream)
.forEach(
p -> {
ImmutableSet.Builder<CodeOwnerAnnotation> annotationBuilder = ImmutableSet.builder();
@@ -467,6 +538,12 @@ public class CodeOwnerResolver {
annotationBuilder.addAll(
annotations.get(CodeOwnerReference.create(ALL_USERS_WILDCARD)));
+ // annotations for the groups this user is in apply as well
+ for (String group : usersToGroups.get(p.value())) {
+ annotationBuilder.addAll(
+ annotations.get(CodeOwnerReference.create(group)));
+ }
+
if (!codeOwnersWithAnnotations.containsKey(p.value())) {
codeOwnersWithAnnotations.put(p.value(), new HashSet<>());
}
@@ -570,7 +647,7 @@ public class CodeOwnerResolver {
}
messages.add(String.format("email %s has no domain", email));
- return false;
+ return true; // TVL: we allow domain-less strings which are treated as usernames.
}
/**
@@ -585,11 +662,29 @@ public class CodeOwnerResolver {
*/
private ImmutableMap<String, Collection<ExternalId>> lookupExternalIds(
ImmutableList.Builder<String> messages, ImmutableSet<String> emails) {
+ String[] actualEmails = emails.stream()
+ .filter(email -> email.contains("@"))
+ .toArray(String[]::new);
+ ImmutableSet<String> usernames = emails.stream()
+ .filter(email -> !email.contains("@"))
+ .collect(ImmutableSet.toImmutableSet());
try {
- ImmutableMap<String, Collection<ExternalId>> extIdsByEmail =
- externalIdCache.byEmails(emails.toArray(new String[0])).asMap();
+ ImmutableMap<String, Collection<ExternalId>> extIds =
+ new ImmutableMap.Builder<String, Collection<ExternalId>>()
+ .putAll(externalIdCache.byEmails(actualEmails).asMap())
+ .putAll(externalIdCache.allByAccount().entries().stream()
+ .map(entry -> entry.getValue())
+ .filter(externalId ->
+ externalId.key().scheme() != null &&
+ externalId.key().isScheme(ExternalId.SCHEME_USERNAME) &&
+ usernames.contains(externalId.key().id()))
+ .collect(toImmutableSetMultimap(
+ externalId -> externalId.key().id(),
+ externalId -> externalId))
+ .asMap())
+ .build();
emails.stream()
- .filter(email -> !extIdsByEmail.containsKey(email))
+ .filter(email -> !extIds.containsKey(email))
.forEach(
email -> {
transientCodeOwnerCache.cacheNonResolvable(email);
@@ -598,7 +693,7 @@ public class CodeOwnerResolver {
"cannot resolve code owner email %s: no account with this email exists",
email));
});
- return extIdsByEmail;
+ return extIds;
} catch (IOException e) {
throw newInternalServerError(
String.format("cannot resolve code owner emails: %s", emails), e);
@@ -815,6 +910,15 @@ public class CodeOwnerResolver {
user != null ? user.getLoggableName() : currentUser.get().getLoggableName()));
return true;
}
+ if (!email.contains("@")) {
+ // the email is the username of the account, or a group, or something else.
+ messages.add(
+ String.format(
+ "account %s is visible to user %s",
+ accountState.account().id(),
+ user != null ? user.getLoggableName() : currentUser.get().getLoggableName()));
+ return true;
+ }
if (user != null) {
if (user.hasEmailAddress(email)) {
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/findowners/FindOwnersCodeOwnerConfigParser.java b/java/com/google/gerrit/plugins/codeowners/backend/findowners/FindOwnersCodeOwnerConfigParser.java
index 5f350998..7977ba55 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/findowners/FindOwnersCodeOwnerConfigParser.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/findowners/FindOwnersCodeOwnerConfigParser.java
@@ -149,7 +149,8 @@ public class FindOwnersCodeOwnerConfigParser implements CodeOwnerConfigParser {
private static final String EOL = "[\\s]*(#.*)?$"; // end-of-line
private static final String GLOB = "[^\\s,=]+"; // a file glob
- private static final String EMAIL_OR_STAR = "([^\\s<>@,]+@[^\\s<>@#,]+|\\*)";
+ // Also allows usernames, and group:$GROUP_NAME.
+ private static final String EMAIL_OR_STAR = "([^\\s<>@,]+@[^\\s<>@#,]+?|\\*|[a-zA-Z0-9_\\-]+|group:[a-zA-Z0-9_\\-]+)";
private static final String EMAIL_LIST =
"(" + EMAIL_OR_STAR + "(" + COMMA + EMAIL_OR_STAR + ")*)";
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/AbstractFileBasedCodeOwnerBackendTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/AbstractFileBasedCodeOwnerBackendTest.java
index 7ec92959..59cf7e05 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/AbstractFileBasedCodeOwnerBackendTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/AbstractFileBasedCodeOwnerBackendTest.java
@@ -424,7 +424,7 @@ public abstract class AbstractFileBasedCodeOwnerBackendTest extends AbstractCode
.commit()
.parent(head)
.message("Add invalid test code owner config")
- .add(JgitPath.of(codeOwnerConfigKey.filePath(getFileName())).get(), "INVALID"));
+ .add(JgitPath.of(codeOwnerConfigKey.filePath(getFileName())).get(), "INVALID!"));
}
// Try to update the code owner config.
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolverTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolverTest.java
index 6171aca9..37699012 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolverTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolverTest.java
@@ -24,8 +24,10 @@ import com.google.gerrit.acceptance.TestAccount;
import com.google.gerrit.acceptance.TestMetricMaker;
import com.google.gerrit.acceptance.config.GerritConfig;
import com.google.gerrit.acceptance.testsuite.account.AccountOperations;
+import com.google.gerrit.acceptance.testsuite.group.GroupOperations;
import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations;
import com.google.gerrit.entities.Account;
+import com.google.gerrit.entities.AccountGroup;
import com.google.gerrit.plugins.codeowners.acceptance.AbstractCodeOwnersTest;
import com.google.gerrit.server.ServerInitiated;
import com.google.gerrit.server.account.AccountsUpdate;
@@ -51,6 +53,7 @@ public class CodeOwnerResolverTest extends AbstractCodeOwnersTest {
@Inject private RequestScopeOperations requestScopeOperations;
@Inject @ServerInitiated private Provider<AccountsUpdate> accountsUpdate;
@Inject private AccountOperations accountOperations;
+ @Inject private GroupOperations groupOperations;
@Inject private ExternalIdNotes.Factory externalIdNotesFactory;
@Inject private TestMetricMaker testMetricMaker;
@Inject private ExternalIdFactory externalIdFactory;
@@ -112,6 +115,18 @@ public class CodeOwnerResolverTest extends AbstractCodeOwnersTest {
.contains(String.format("account %s is visible to user %s", admin.id(), admin.username()));
}
+ @Test
+ public void resolveCodeOwnerReferenceForUsername() throws Exception {
+ OptionalResultWithMessages<CodeOwner> result =
+ codeOwnerResolverProvider
+ .get()
+ .resolveWithMessages(CodeOwnerReference.create(admin.username()));
+ assertThat(result.get()).hasAccountIdThat().isEqualTo(admin.id());
+ assertThat(result)
+ .hasMessagesThat()
+ .contains(String.format("account %s is visible to user %s", admin.id(), admin.username()));
+ }
+
@Test
public void cannotResolveCodeOwnerReferenceForStarAsEmail() throws Exception {
OptionalResultWithMessages<CodeOwner> result =
@@ -127,6 +142,18 @@ public class CodeOwnerResolverTest extends AbstractCodeOwnersTest {
CodeOwnerResolver.ALL_USERS_WILDCARD));
}
+ @Test
+ public void cannotResolveCodeOwnerReferenceForGroup() throws Exception {
+ OptionalResultWithMessages<CodeOwner> result =
+ codeOwnerResolverProvider
+ .get()
+ .resolveWithMessages(CodeOwnerReference.create("group:Administrators"));
+ assertThat(result).isEmpty();
+ assertThat(result)
+ .hasMessagesThat()
+ .contains("cannot resolve code owner email group:Administrators: this is a group");
+ }
+
@Test
public void resolveCodeOwnerReferenceForAmbiguousEmailIfOtherAccountIsInactive()
throws Exception {
@@ -397,6 +424,64 @@ public class CodeOwnerResolverTest extends AbstractCodeOwnersTest {
assertThat(result.hasUnresolvedCodeOwners()).isFalse();
}
+ @Test
+ public void resolvePathCodeOwnersWhenNonVisibleGroupIsUsed() throws Exception {
+ CodeOwnerConfig codeOwnerConfig =
+ CodeOwnerConfig.builder(CodeOwnerConfig.Key.create(project, "master", "/"), TEST_REVISION)
+ .addCodeOwnerSet(
+ CodeOwnerSet.createWithoutPathExpressions("group:Administrators"))
+ .build();
+
+ CodeOwnerResolverResult result =
+ codeOwnerResolverProvider
+ .get()
+ .resolvePathCodeOwners(codeOwnerConfig, Paths.get("/README.md"));
+ assertThat(result.codeOwnersAccountIds()).isEmpty();
+ assertThat(result.ownedByAllUsers()).isFalse();
+ assertThat(result.hasUnresolvedCodeOwners()).isTrue();
+ }
+
+ @Test
+ public void resolvePathCodeOwnersWhenVisibleGroupIsUsed() throws Exception {
+ AccountGroup.UUID createdGroupUUID = groupOperations
+ .newGroup()
+ .name("VisibleGroup")
+ .visibleToAll(true)
+ .addMember(admin.id())
+ .create();
+
+ CodeOwnerConfig codeOwnerConfig =
+ CodeOwnerConfig.builder(CodeOwnerConfig.Key.create(project, "master", "/"), TEST_REVISION)
+ .addCodeOwnerSet(
+ CodeOwnerSet.createWithoutPathExpressions("group:VisibleGroup"))
+ .build();
+
+ CodeOwnerResolverResult result =
+ codeOwnerResolverProvider
+ .get()
+ .resolvePathCodeOwners(codeOwnerConfig, Paths.get("/README.md"));
+ assertThat(result.codeOwnersAccountIds()).containsExactly(admin.id());
+ assertThat(result.ownedByAllUsers()).isFalse();
+ assertThat(result.hasUnresolvedCodeOwners()).isFalse();
+ }
+
+ @Test
+ public void resolvePathCodeOwnersWhenUsernameIsUsed() throws Exception {
+ CodeOwnerConfig codeOwnerConfig =
+ CodeOwnerConfig.builder(CodeOwnerConfig.Key.create(project, "master", "/"), TEST_REVISION)
+ .addCodeOwnerSet(
+ CodeOwnerSet.createWithoutPathExpressions(admin.username()))
+ .build();
+
+ CodeOwnerResolverResult result =
+ codeOwnerResolverProvider
+ .get()
+ .resolvePathCodeOwners(codeOwnerConfig, Paths.get("/README.md"));
+ assertThat(result.codeOwnersAccountIds()).containsExactly(admin.id());
+ assertThat(result.ownedByAllUsers()).isFalse();
+ assertThat(result.hasUnresolvedCodeOwners()).isFalse();
+ }
+
@Test
public void resolvePathCodeOwnersNonResolvableCodeOwnersAreFilteredOut() throws Exception {
CodeOwnerConfig codeOwnerConfig =
@@ -655,7 +740,7 @@ public class CodeOwnerResolverTest extends AbstractCodeOwnersTest {
"domain example.com of email foo@example.org@example.com is allowed");
assertIsEmailDomainAllowed(
"foo@example.org", false, "domain example.org of email foo@example.org is not allowed");
- assertIsEmailDomainAllowed("foo", false, "email foo has no domain");
+ assertIsEmailDomainAllowed("foo", true, "email foo has no domain");
assertIsEmailDomainAllowed(
"foo@example.com@example.org",
false,
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/findowners/FindOwnersCodeOwnerConfigParserTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/findowners/FindOwnersCodeOwnerConfigParserTest.java
index 260e635e..7aab99d0 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/findowners/FindOwnersCodeOwnerConfigParserTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/findowners/FindOwnersCodeOwnerConfigParserTest.java
@@ -158,16 +158,42 @@ public class FindOwnersCodeOwnerConfigParserTest extends AbstractCodeOwnerConfig
codeOwnerConfigParser.parse(
TEST_REVISION,
CodeOwnerConfig.Key.create(project, "master", "/"),
- getCodeOwnerConfig(EMAIL_1, "INVALID", "NOT_AN_EMAIL", EMAIL_2)));
+ getCodeOwnerConfig(EMAIL_1, "INVALID!", "NOT!AN_EMAIL", EMAIL_2)));
assertThat(exception.getFullMessage(FindOwnersBackend.CODE_OWNER_CONFIG_FILE_NAME))
.isEqualTo(
String.format(
"invalid code owner config file '/OWNERS' (project = %s, branch = master):\n"
- + " invalid line: INVALID\n"
- + " invalid line: NOT_AN_EMAIL",
+ + " invalid line: INVALID!\n"
+ + " invalid line: NOT!AN_EMAIL",
project));
}
+ @Test
+ public void codeOwnerConfigWithUsernames() throws Exception {
+ assertParseAndFormat(
+ getCodeOwnerConfig(EMAIL_1, "USERNAME", EMAIL_2),
+ codeOwnerConfig ->
+ assertThat(codeOwnerConfig)
+ .hasCodeOwnerSetsThat()
+ .onlyElement()
+ .hasCodeOwnersEmailsThat()
+ .containsExactly(EMAIL_1, "USERNAME", EMAIL_2),
+ getCodeOwnerConfig(EMAIL_1, "USERNAME", EMAIL_2));
+ }
+
+ @Test
+ public void codeOwnerConfigWithGroups() throws Exception {
+ assertParseAndFormat(
+ getCodeOwnerConfig(EMAIL_1, "group:tvl-employees", EMAIL_2),
+ codeOwnerConfig ->
+ assertThat(codeOwnerConfig)
+ .hasCodeOwnerSetsThat()
+ .onlyElement()
+ .hasCodeOwnersEmailsThat()
+ .containsExactly(EMAIL_1, "group:tvl-employees", EMAIL_2),
+ getCodeOwnerConfig(EMAIL_1, "group:tvl-employees", EMAIL_2));
+ }
+
@Test
public void codeOwnerConfigWithComment() throws Exception {
assertParseAndFormat(
