tvl-depot/third_party/gerrit_plugins/code-owners/using-usernames.patch

diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolver.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolver.java
index 07894ced..6a528b50 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;
@@ -38,6 +41,9 @@ 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.ExternalIds;
 import com.google.gerrit.server.permissions.GlobalPermission;
@@ -103,6 +109,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;
@@ -113,6 +121,7 @@ public class CodeOwnerResolver {
   private final CodeOwnerMetrics codeOwnerMetrics;
   private final UnresolvedImportFormatter unresolvedImportFormatter;
   private final TransientCodeOwnerCache transientCodeOwnerCache;
+  private final InternalGroupBackend groupBackend;
 
   // Enforce visibility by default.
   private boolean enforceVisibility = true;
@@ -133,7 +142,8 @@ public class CodeOwnerResolver {
       PathCodeOwners.Factory pathCodeOwnersFactory,
       CodeOwnerMetrics codeOwnerMetrics,
       UnresolvedImportFormatter unresolvedImportFormatter,
-      TransientCodeOwnerCache transientCodeOwnerCache) {
+      TransientCodeOwnerCache transientCodeOwnerCache,
+      InternalGroupBackend groupBackend) {
     this.codeOwnersPluginConfiguration = codeOwnersPluginConfiguration;
     this.permissionBackend = permissionBackend;
     this.currentUser = currentUser;
@@ -144,6 +154,7 @@ public class CodeOwnerResolver {
     this.codeOwnerMetrics = codeOwnerMetrics;
     this.unresolvedImportFormatter = unresolvedImportFormatter;
     this.transientCodeOwnerCache = transientCodeOwnerCache;
+    this.groupBackend = groupBackend;
   }
 
   /**
@@ -357,6 +368,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);
@@ -401,9 +418,32 @@ 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());
+
+    ImmutableSetMultimap<String, CodeOwner> 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()))));
+    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());
 
@@ -438,7 +478,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);
     }
 
@@ -452,7 +493,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();
@@ -463,6 +506,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<>());
               }
@@ -566,7 +615,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.
   }
 
   /**
@@ -581,11 +630,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 =
-          externalIds.byEmails(emails.toArray(new String[0])).asMap();
+      ImmutableMap<String, Collection<ExternalId>> extIds =
+          new ImmutableMap.Builder<String, Collection<ExternalId>>()
+              .putAll(externalIds.byEmails(actualEmails).asMap())
+              .putAll(externalIds.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);
@@ -594,7 +661,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);
@@ -811,6 +878,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 b32c3b5e..6b0f0cf8 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 {
@@ -391,6 +418,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 =
@@ -649,7 +734,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(