Merge "Ignore account visibility when parsing Reviewers Collection"

java/com/google/gerrit/server/account/AccountResolver.java

@@ -516,12 +516,22 @@ public class AccountResolver {
    * @throws IOException if an error occurs.
    */
   public Result resolve(String input) throws ConfigInvalidException, IOException {
-    return searchImpl(input, searchers, visibilitySupplier(), accountActivityPredicate());
+    return searchImpl(input, searchers, visibilitySupplierCanSee(), accountActivityPredicate());
   }
 
   public Result resolve(String input, Predicate<AccountState> accountActivityPredicate)
       throws ConfigInvalidException, IOException {
-    return searchImpl(input, searchers, visibilitySupplier(), accountActivityPredicate);
+    return searchImpl(input, searchers, visibilitySupplierCanSee(), accountActivityPredicate);
+  }
+
+  public Result resolveIgnoreVisibility(String input) throws ConfigInvalidException, IOException {
+    return searchImpl(input, searchers, visibilitySupplierAll(), accountActivityPredicate());
+  }
+
+  public Result resolveIgnoreVisibility(
+      String input, Predicate<AccountState> accountActivityPredicate)
+      throws ConfigInvalidException, IOException {
+    return searchImpl(input, searchers, visibilitySupplierAll(), accountActivityPredicate);
   }
 
   /**
@@ -550,13 +560,23 @@ public class AccountResolver {
   @Deprecated
   public Result resolveByNameOrEmail(String input) throws ConfigInvalidException, IOException {
     return searchImpl(
-        input, nameOrEmailSearchers, visibilitySupplier(), accountActivityPredicate());
+        input, nameOrEmailSearchers, visibilitySupplierCanSee(), accountActivityPredicate());
   }
 
-  private Supplier<Predicate<AccountState>> visibilitySupplier() {
+  private Supplier<Predicate<AccountState>> visibilitySupplierCanSee() {
     return () -> accountControlFactory.get()::canSee;
   }
 
+  private Supplier<Predicate<AccountState>> visibilitySupplierAll() {
+    return () -> all();
+  }
+
+  private Predicate<AccountState> all() {
+    return accountState -> {
+      return true;
+    };
+  }
+
   private Predicate<AccountState> accountActivityPredicate() {
     return (AccountState accountState) -> accountState.account().isActive();
   }

java/com/google/gerrit/server/restapi/change/Reviewers.java

@@ -21,12 +21,11 @@ import com.google.gerrit.extensions.restapi.ChildCollection;
 import com.google.gerrit.extensions.restapi.IdString;
 import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
 import com.google.gerrit.extensions.restapi.RestView;
-import com.google.gerrit.extensions.restapi.TopLevelResource;
 import com.google.gerrit.mail.Address;
 import com.google.gerrit.server.ApprovalsUtil;
+import com.google.gerrit.server.account.AccountResolver;
 import com.google.gerrit.server.change.ChangeResource;
 import com.google.gerrit.server.change.ReviewerResource;
-import com.google.gerrit.server.restapi.account.AccountsCollection;
 import com.google.inject.Inject;
 import com.google.inject.Singleton;
 import java.io.IOException;
@@ -37,22 +36,22 @@ import org.eclipse.jgit.errors.ConfigInvalidException;
 public class Reviewers implements ChildCollection<ChangeResource, ReviewerResource> {
   private final DynamicMap<RestView<ReviewerResource>> views;
   private final ApprovalsUtil approvalsUtil;
-  private final AccountsCollection accounts;
   private final ReviewerResource.Factory resourceFactory;
   private final ListReviewers list;
+  private final AccountResolver accountResolver;
 
   @Inject
   Reviewers(
       ApprovalsUtil approvalsUtil,
-      AccountsCollection accounts,
       ReviewerResource.Factory resourceFactory,
       DynamicMap<RestView<ReviewerResource>> views,
-      ListReviewers list) {
+      ListReviewers list,
+      AccountResolver accountResolver) {
     this.approvalsUtil = approvalsUtil;
-    this.accounts = accounts;
     this.resourceFactory = resourceFactory;
     this.views = views;
     this.list = list;
+    this.accountResolver = accountResolver;
   }
 
   @Override
@@ -68,22 +67,18 @@ public class Reviewers implements ChildCollection<ChangeResource, ReviewerResour
   @Override
   public ReviewerResource parse(ChangeResource rsrc, IdString id)
       throws ResourceNotFoundException, AuthException, IOException, ConfigInvalidException {
-    Address address = Address.tryParse(id.get());
-
-    Account.Id accountId = null;
     try {
-      accountId = accounts.parse(TopLevelResource.INSTANCE, id).getUser().getAccountId();
-    } catch (ResourceNotFoundException e) {
-      if (address == null) {
-        throw e;
-      }
-    }
-    // See if the id exists as a reviewer for this change
-    if (accountId != null && fetchAccountIds(rsrc).contains(accountId)) {
-      return resourceFactory.create(rsrc, accountId);
-    }
 
-    // See if the address exists as a reviewer on the change
+      AccountResolver.Result result = accountResolver.resolveIgnoreVisibility(id.get());
+      if (fetchAccountIds(rsrc).contains(result.asUniqueUser().getAccountId())) {
+        return resourceFactory.create(rsrc, result.asUniqueUser().getAccountId());
+      }
+    } catch (AccountResolver.UnresolvableAccountException e) {
+      if (e.isSelf()) {
+        throw new AuthException(e.getMessage(), e);
+      }
+    }
+    Address address = Address.tryParse(id.get());
     if (address != null && rsrc.getNotes().getReviewersByEmail().all().contains(address)) {
       return new ReviewerResource(rsrc, address);
     }

javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java

@@ -2204,7 +2204,7 @@ public class ChangeIT extends AbstractDaemonTest {
         gApi.changes().id(r.getChangeId()).reviewer(admin.id().toString()).votes();
 
     assertThat(m).hasSize(1);
-    assertThat(m).containsEntry("Code-Review", Short.valueOf((short) 2));
+    assertThat(m).containsExactly("Code-Review", Short.valueOf((short) 2));
 
     requestScopeOperations.setApiUser(user.id());
     gApi.changes().id(r.getChangeId()).revision(r.getCommit().name()).review(ReviewInput.dislike());
@@ -2212,7 +2212,26 @@ public class ChangeIT extends AbstractDaemonTest {
     m = gApi.changes().id(r.getChangeId()).reviewer(user.id().toString()).votes();
 
     assertThat(m).hasSize(1);
-    assertThat(m).containsEntry("Code-Review", Short.valueOf((short) -1));
+    assertThat(m).containsExactly("Code-Review", Short.valueOf((short) -1));
+  }
+
+  @Test
+  @GerritConfig(name = "accounts.visibility", value = "NONE")
+  public void listVotesEvenWhenAccountsAreNotVisible() throws Exception {
+    PushOneCommit.Result r = createChange();
+    gApi.changes().id(r.getChangeId()).revision(r.getCommit().name()).review(ReviewInput.approve());
+
+    requestScopeOperations.setApiUser(user.id());
+
+    // check finding by address works
+    Map<String, Short> m = gApi.changes().id(r.getChangeId()).reviewer(admin.email()).votes();
+    assertThat(m).hasSize(1);
+    assertThat(m).containsEntry("Code-Review", Short.valueOf((short) 2));
+
+    // check finding by id works
+    m = gApi.changes().id(r.getChangeId()).reviewer(admin.id().toString()).votes();
+    assertThat(m).hasSize(1);
+    assertThat(m).containsEntry("Code-Review", Short.valueOf((short) 2));
   }
 
   @Test