Merge branch 'stable-2.16' into stable-3.0

* stable-2.16:
  ChangeQueryBuilder: Throw error on ambiguous visibleto by display name

Change-Id: I12897851d2769b0e9bf400f4feac1ce140d0be80

3 files changed, 47 insertions, 9 deletions

diff --git a/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java b/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
index 8d8f9ba97a..129dcaca0e 100644
--- a/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
+++ b/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
@@ -14,7 +14,6 @@
 
 package com.google.gerrit.server.query.change;
 
-import static com.google.common.collect.ImmutableList.toImmutableList;
 import static com.google.gerrit.reviewdb.client.Change.CHANGE_ID_PATTERN;
 import static com.google.gerrit.server.account.AccountResolver.isSelf;
 import static com.google.gerrit.server.query.change.ChangeData.asChanges;
@@ -24,6 +23,7 @@ import static java.util.stream.Collectors.toSet;
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Enums;
 import com.google.common.base.Splitter;
+import com.google.common.collect.Iterables;
 import com.google.common.collect.Lists;
 import com.google.common.primitives.Ints;
 import com.google.gerrit.common.data.GroupDescription;
@@ -957,18 +957,23 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData, ChangeQueryBuil
     if (isSelf(who)) {
       return isVisible();
     }
+    Set<Account.Id> accounts = null;
     try {
-      return Predicate.or(
-          parseAccount(who).stream()
-              .map(a -> visibleto(args.userFactory.create(a)))
-              .collect(toImmutableList()));
+      accounts = parseAccount(who);
     } catch (QueryParseException e) {
       if (e instanceof QueryRequiresAuthException) {
         throw e;
       }
-      // Otherwise continue: if it's not an account, maybe it's a group?
+    }
+    if (accounts != null) {
+      if (accounts.size() == 1) {
+        return visibleto(args.userFactory.create(Iterables.getOnlyElement(accounts)));
+      } else if (accounts.size() > 1) {
+        throw error(String.format("\"%s\" resolves to multiple accounts", who));
+      }
     }
 
+    // If its not an account, maybe its a group?
     Collection<GroupReference> suggestions = args.groupBackend.suggest(who, null);
     if (!suggestions.isEmpty()) {
       HashSet<AccountGroup.UUID> ids = new HashSet<>();
diff --git a/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java b/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
index 1087d5c4c9..33ec540520 100644
--- a/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
+++ b/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
@@ -1853,16 +1853,43 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests {
     assertQuery(q + " visibleto:self", change2, change1);
 
     // Second user cannot see first user's private change
-    Account.Id user2 = createAccount("anotheruser");
+    Account.Id user2 = createAccount("user2");
     assertQuery(q + " visibleto:" + user2.get(), change1);
-    assertQuery(q + " visibleto:anotheruser", change1);
+    assertQuery(q + " visibleto:user2", change1);
 
     String g1 = createGroup("group1", "Administrators");
-    gApi.groups().id(g1).addMembers("anotheruser");
+    gApi.groups().id(g1).addMembers("user2");
     assertQuery(q + " visibleto:" + g1, change1);
 
     requestContext.setContext(newRequestContext(user2));
     assertQuery("is:visible", change1);
+
+    Account.Id user3 = createAccount("user3");
+
+    // Explicitly authenticate user2 and user3 so that display name gets set
+    AuthRequest authRequest = AuthRequest.forUser("user2");
+    authRequest.setDisplayName("Another User");
+    authRequest.setEmailAddress("user2@example.com");
+    accountManager.authenticate(authRequest);
+    authRequest = AuthRequest.forUser("user3");
+    authRequest.setDisplayName("Another User");
+    authRequest.setEmailAddress("user3@example.com");
+    accountManager.authenticate(authRequest);
+
+    // Switch to user3
+    requestContext.setContext(newRequestContext(user3));
+    Change change3 = insert(repo, newChange(repo), user3);
+    Change change4 = insert(repo, newChangePrivate(repo), user3);
+
+    // User3 can see both their changes and the first user's change
+    assertQuery(q + " visibleto:" + user3.get(), change4, change3, change1);
+
+    // User2 cannot see user3's private change
+    assertQuery(q + " visibleto:" + user2.get(), change3, change1);
+
+    // Query as user3 by display name matching user2 and user3; bad request
+    assertFailingQuery(
+        q + " visibleto:\"Another User\"", "\"Another User\" resolves to multiple accounts");
   }
 
   @Test
diff --git a/javatests/com/google/gerrit/server/query/change/LuceneQueryChangesTest.java b/javatests/com/google/gerrit/server/query/change/LuceneQueryChangesTest.java
index 2ea198f6d8..122b6326dc 100644
--- a/javatests/com/google/gerrit/server/query/change/LuceneQueryChangesTest.java
+++ b/javatests/com/google/gerrit/server/query/change/LuceneQueryChangesTest.java
@@ -46,6 +46,12 @@ public class LuceneQueryChangesTest extends AbstractQueryChangesTest {
   }
 
   @Override
+  @Test
+  public void visible() throws Exception {
+    super.visible();
+  }
+
+  @Override
   protected Injector createInjector() {
     Config luceneConfig = new Config(config);
     InMemoryModule.setDefaults(luceneConfig);