feat(gerrit): add code-owners plugin r/4921

This is the New Thing that is intended to replace the find-owners
and owners plugins.

In particular:

* It inserts a submit requirement rather than providing a Prolog
  predicate.
* The default OWNERS file formats are suspiciously Googley.
* It provides a neat UI for finding OWNERS and tracking approval
  state on a per-file basis.

When we fully migrate to using the code-owners plugin, a few
things will need to land, which I will likely do "offline"
directly to the Gerrit backing Git repos:

* Add the corresponding Gerrit config
* Replace OWNERS files depot-wide
* Add OWNERS files to the refs/meta/config branch
* Introduce the Owners-Override label, settable by depot-interventions

The enclosed patch adds two extra pieces of functionality that
we need in tvldepot but aren't upstream:

1. The ability to just specify usernames rather than email addresses
2. The ability to specify `group:GROUPNAME`, _as long as_ that group is
   visible to everyone. This is a restriction intended to avoid having
   the plugin just leak group membership.

Change-Id: I27d92b6cb7449af83030b9015f09a1571aa8452f
Reviewed-on: https://cl.tvl.fyi/c/depot/+/6664
Tested-by: BuildkiteCI
Reviewed-by: tazjin <tazjin@tvl.su>
Reviewed-by: sterni <sternenseemann@systemli.org>

3 files changed, 453 insertions, 7 deletions

diff --git a/third_party/gerrit_plugins/builder.nix b/third_party/gerrit_plugins/builder.nix
index 0b6501801c..9085975620 100644
--- a/third_party/gerrit_plugins/builder.nix
+++ b/third_party/gerrit_plugins/builder.nix
@@ -1,4 +1,4 @@
-{ depot, pkgs, ... }:
+{ depot, lib, pkgs, ... }:
 {
   buildGerritBazelPlugin =
     { name
@@ -8,7 +8,7 @@
         cp -R "${src}" "$out/plugins/${name}"
       ''
     , postPatch ? ""
-    ,
+    , patches ? [ ]
     }: ((depot.third_party.gerrit.override {
       name = "${name}.jar";
 
@@ -26,10 +26,14 @@
       installPhase = ''
         cp "bazel-bin/plugins/${name}/${name}.jar" "$out"
       '';
-      postPatch =
-        if super ? postPatch then ''
-          ${super.postPatch}
-          ${postPatch}
-        '' else postPatch;
+      postPatch = ''
+        ${super.postPatch or ""}
+        pushd "plugins/${name}"
+        ${lib.concatMapStringsSep "\n" (patch: ''
+          patch -p1 < ${patch}
+        '') patches}
+        popd
+        ${postPatch}
+      '';
     }));
 }
diff --git a/third_party/gerrit_plugins/code-owners/default.nix b/third_party/gerrit_plugins/code-owners/default.nix
new file mode 100644
index 0000000000..377a2a0471
--- /dev/null
+++ b/third_party/gerrit_plugins/code-owners/default.nix
@@ -0,0 +1,17 @@
+{ depot, pkgs, ... }@args:
+
+let
+  inherit (import ../builder.nix args) buildGerritBazelPlugin;
+in
+buildGerritBazelPlugin rec {
+  name = "code-owners";
+  depsOutputHash = "sha256:0fpv5yavgki5nv84lg5zykp6v7pv9xll1glmz5dwnz5z11axj4g9";
+  src = pkgs.fetchgit {
+    url = "https://gerrit.googlesource.com/plugins/code-owners";
+    rev = "6fdf3ce2e52904b35e2a5824a4197155c2c6b4e4";
+    sha256 = "sha256:17k6310py71wax3881mf3vsf9zas648j4xzs9h0d7migv5nzsdzs";
+  };
+  patches = [
+    ./using-usernames.patch
+  ];
+}
diff --git a/third_party/gerrit_plugins/code-owners/using-usernames.patch b/third_party/gerrit_plugins/code-owners/using-usernames.patch
new file mode 100644
index 0000000000..738e88f771
--- /dev/null
+++ b/third_party/gerrit_plugins/code-owners/using-usernames.patch
@@ -0,0 +1,425 @@
+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(