From ba76ff8b7cd128383c86aeeacf12d1001670eec4 Mon Sep 17 00:00:00 2001 From: Luke Granger-Brown <git@lukegb.com> Date: Wed, 21 Sep 2022 03:15:38 +0100 Subject: [PATCH] Add support for usernames and groups Change-Id: I3ba8527f66216d08e555a6ac4451fe0d1e090de5 --- .../codeowners/backend/CodeOwnerResolver.java | 120 ++++++++++++++++-- .../FindOwnersCodeOwnerConfigParser.java | 3 +- ...AbstractFileBasedCodeOwnerBackendTest.java | 2 +- .../backend/CodeOwnerResolverTest.java | 87 ++++++++++++- .../FindOwnersCodeOwnerConfigParserTest.java | 32 ++++- 5 files changed, 230 insertions(+), 14 deletions(-) diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolver.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolver.java index 07894ced..40943659 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.ExternalIds; 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; @@ -103,6 +113,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 +125,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; @@ -133,7 +147,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; @@ -144,6 +160,8 @@ public class CodeOwnerResolver { this.codeOwnerMetrics = codeOwnerMetrics; this.unresolvedImportFormatter = unresolvedImportFormatter; this.transientCodeOwnerCache = transientCodeOwnerCache; + this.groupBackend = groupBackend; + this.context = context; } /** @@ -357,6 +375,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 +425,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()); @@ -438,7 +506,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 +521,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 +534,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 +643,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 +658,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 +689,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 +906,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( -- 2.37.3