Ignore account visibility when processing reviewers
The reviewers plugin allows project owners to configure a list
of accounts and groups that should be added as reviewers. The logic
that actually adds reviewers is triggered e.g. on change updates.
We used to check account visibility against the calling user (the user
who updated the change) which conceptually makes no sense because
the project owner provided this list. Next to the conceptual mistake
that we get a good number of bug reports for, checking account
visibility here is also very slow. We saw traces where it took
10s of seconds.
This commit makes checking account visibility dependent on what we do:
- For suggesting reviewers, we keep the visibility check
- For processing updates, we drop it
Google-Bug-Id: b/267202984
Change-Id: I39023f74e7cfbdab24b7abd3df6518e0b2a4b209
diff --git a/src/main/java/com/googlesource/gerrit/plugins/reviewers/ReviewerSuggest.java b/src/main/java/com/googlesource/gerrit/plugins/reviewers/ReviewerSuggest.java
index 06a365f..088d304 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/reviewers/ReviewerSuggest.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/reviewers/ReviewerSuggest.java
@@ -62,7 +62,7 @@
try {
Set<String> reviewers = util.findReviewers(changeId.get(), sections);
if (!reviewers.isEmpty()) {
- return resolver.resolve(reviewers, project, changeId.get(), null).stream()
+ return resolver.resolve(reviewers, project, changeId.get(), null, false).stream()
.map(a -> suggestedReviewer(a))
.collect(toSet());
}
diff --git a/src/main/java/com/googlesource/gerrit/plugins/reviewers/Reviewers.java b/src/main/java/com/googlesource/gerrit/plugins/reviewers/Reviewers.java
index 865c289..aaf7c36 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/reviewers/Reviewers.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/reviewers/Reviewers.java
@@ -110,11 +110,15 @@
/* Remove all reviewer identifiers (account-ids, group-ids) from ccs that are present in reviewers.
* Further filtering of individual accounts is done in AddReviewers after the ids have been resolved into Accounts. */
ccs.removeAll(reviewers);
+ // This listener is called after a revision was created to add reviewers
+ // according to the configs that project owners provided. Respecting
+ // account visibility here by checking if the caller (e.g. the user adding
+ // a new revision) can see the reviewer to be added does not make sense.
final AddReviewers addReviewers =
addReviewersFactory.create(
c,
- resolver.resolve(reviewers, projectName, changeNumber, uploader),
- resolver.resolve(ccs, projectName, changeNumber, uploader));
+ resolver.resolve(reviewers, projectName, changeNumber, uploader, true),
+ resolver.resolve(ccs, projectName, changeNumber, uploader, true));
workQueue.submit(addReviewers);
} catch (QueryParseException e) {
logger.atWarning().log(
diff --git a/src/main/java/com/googlesource/gerrit/plugins/reviewers/ReviewersResolver.java b/src/main/java/com/googlesource/gerrit/plugins/reviewers/ReviewersResolver.java
index ea7d8e1..9454455 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/reviewers/ReviewersResolver.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/reviewers/ReviewersResolver.java
@@ -67,6 +67,7 @@
* @param project the project name
* @param changeNumber the change Id
* @param uploader account to use to look up groups, or null if groups are not needed
+ * @param ignoreAccountVisibility if account visibiltiy should be ignored
* @return set of {@link com.google.gerrit.entities.Account.Id}s.
*/
@VisibleForTesting
@@ -74,10 +75,12 @@
Set<String> names,
Project.NameKey project,
int changeNumber,
- @Nullable AccountInfo uploader) {
+ @Nullable AccountInfo uploader,
+ boolean ignoreAccountVisibility) {
Set<Account.Id> reviewers = Sets.newHashSetWithExpectedSize(names.size());
for (String name : names) {
- if (resolveAccount(project, changeNumber, uploader, reviewers, name)) {
+ if (resolveAccount(
+ project, changeNumber, uploader, reviewers, name, ignoreAccountVisibility)) {
continue;
}
@@ -91,9 +94,13 @@
int changeNumber,
@Nullable AccountInfo uploader,
Set<Account.Id> reviewers,
- String accountName) {
+ String accountName,
+ boolean ignoreAccountVisibility) {
try {
- AccountResolver.Result result = accountResolver.resolve(accountName);
+ AccountResolver.Result result =
+ ignoreAccountVisibility
+ ? accountResolver.resolveIgnoreVisibility(accountName)
+ : accountResolver.resolve(accountName);
if (result.asList().size() == 1) {
Account.Id id = result.asList().get(0).account().id();
if (uploader == null || id.get() != uploader._accountId) {
diff --git a/src/test/java/com/googlesource/gerrit/plugins/reviewers/ReviewersIT.java b/src/test/java/com/googlesource/gerrit/plugins/reviewers/ReviewersIT.java
index 65e72e5..e6fb981 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/reviewers/ReviewersIT.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/reviewers/ReviewersIT.java
@@ -35,7 +35,6 @@
import com.google.gerrit.extensions.client.ReviewerState;
import com.google.gerrit.extensions.common.ChangeInfo;
import java.util.Set;
-
import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
import org.eclipse.jgit.junit.TestRepository;
import org.junit.Test;
diff --git a/src/test/java/com/googlesource/gerrit/plugins/reviewers/ReviewersResolverIT.java b/src/test/java/com/googlesource/gerrit/plugins/reviewers/ReviewersResolverIT.java
index 607c9f5..76d5b2b 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/reviewers/ReviewersResolverIT.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/reviewers/ReviewersResolverIT.java
@@ -20,7 +20,9 @@
import com.google.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.acceptance.NoHttpd;
import com.google.gerrit.acceptance.TestAccount;
+import com.google.gerrit.acceptance.config.GerritConfig;
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.inject.Inject;
import java.util.Collections;
@@ -30,7 +32,7 @@
@NoHttpd
public class ReviewersResolverIT extends AbstractDaemonTest {
-
+ @Inject private RequestScopeOperations requestScopeOperations;
@Inject private GroupOperations groupOperations;
@Inject private ReviewersResolver resolver;
private int change;
@@ -47,7 +49,8 @@
Collections.singleton(user.email()),
project,
change,
- gApi.accounts().id(user.id().get()).get());
+ gApi.accounts().id(user.id().get()).get(),
+ false);
assertThat(reviewers).isEmpty();
}
@@ -58,11 +61,37 @@
ImmutableSet.of(user.email(), admin.email()),
project,
change,
- gApi.accounts().id(admin.id().get()).get());
+ gApi.accounts().id(admin.id().get()).get(),
+ false);
assertThat(reviewers).containsExactly(user.id());
}
@Test
+ @GerritConfig(name = "accounts.visibility", value = "NONE")
+ public void accountResolveIgnoreVisibility() throws Exception {
+ requestScopeOperations.setApiUser(user.id());
+
+ Set<Account.Id> reviewers =
+ resolver.resolve(
+ ImmutableSet.of(user.email(), admin.email()),
+ project,
+ change,
+ gApi.accounts().id(user.id().get()).get(),
+ false);
+
+ assertThat(reviewers).isEmpty();
+
+ reviewers =
+ resolver.resolve(
+ ImmutableSet.of(user.email(), admin.email()),
+ project,
+ change,
+ gApi.accounts().id(user.id().get()).get(),
+ true);
+ assertThat(reviewers).containsExactly(admin.id());
+ }
+
+ @Test
public void accountGroupResolve() throws Exception {
String group1 = "group1";
groupOperations.newGroup().name(group1).create();
@@ -81,7 +110,8 @@
ImmutableSet.of(system.email(), group1, group2),
project,
change,
- gApi.accounts().id(admin.id().get()).get());
+ gApi.accounts().id(admin.id().get()).get(),
+ false);
assertThat(reviewers).containsExactly(system.id(), foo.id(), bar.id(), baz.id(), qux.id());
}
