opendev/gerrit
Code Issues Proposed changes

ChangeQueryBuilder: Throw error on ambiguous visibleto by display name

Browse Source 
If the given account identifier resolved to more than one user, the
visibleto would only use the first user from the result. This was OK
for queries by username or account ID because they always resolve to
either no user or exactly one user. However when querying by display
name it is possible that multiple users will be returned, and in this
case it was incorrect to only use the first one.

Fix it to instead throw an error when multiple accounts are resolved,
and extend the tests to cover this use case.

This has been broken since change I87e58dda4 which was introduced in
2012 and was discovered by Sonar Lint which reported the following
message on the broken code:

  Loops with at most one iteration should be refactored (squid:S1751)

  A loop with at most one iteration is equivalent to the use of an if
  statement to conditionally execute one piece of code. No developer
  expects to find such a use of a loop statement. If the initial
  intention of the author was really to conditionally execute one piece
  of code, an if statement should be used instead.

  At worst that was not the initial intention of the author and so the
  body of the loop should be fixed to use the nested return, break or
  throw statements in a more appropriate way.

Change-Id: Ibb16a70e6cc61c22516f5d9ac3e1b6b461c94dc2
This commit is contained in:
David Pursehouse
2020-02-20 11:03:02 +09:00
parent cde3214294
commit fc0ebdad73
2 changed files with 35 additions and 9 deletions
Download Patch File Download Diff File Expand all files Collapse all files

11
java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
View File

@@ -22,6 +22,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;
@@ -913,12 +914,10 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData> {
return isVisible();
}
Set<Account.Id> m = args.accountResolver.findAll(who);
if (!m.isEmpty()) {
List<Predicate<ChangeData>> p = Lists.newArrayListWithCapacity(m.size());
for (Account.Id id : m) {
return visibleto(args.userFactory.create(id));
}
return Predicate.or(p);
if (m.size() == 1) {
return visibleto(args.userFactory.create(Iterables.getOnlyElement(m)));
} else if (m.size() > 1) {
throw error(String.format("\"%s\" resolves to multiple accounts", who));
}
// If its not an account, maybe its a group?

33
javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
View File

@@ -1710,16 +1710,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