Merge changes I9b9967ae,I90c2cd62
* changes:
AccountResolver: Remove callerMayAssumeCandidatesAreVisible() method
AccountResolver: Fix account visibility checking
diff --git a/java/com/google/gerrit/server/account/AccountResolver.java b/java/com/google/gerrit/server/account/AccountResolver.java
index d2417d7..703693d 100644
--- a/java/com/google/gerrit/server/account/AccountResolver.java
+++ b/java/com/google/gerrit/server/account/AccountResolver.java
@@ -230,10 +230,6 @@
return true;
}
- default boolean callerMayAssumeCandidatesAreVisible() {
- return false;
- }
-
/**
* Searches can be done on behalf of either the current user or another provided user. The
* results of some searchers, such as BySelf, are affected by the context user.
@@ -312,11 +308,6 @@
}
@Override
- public boolean callerMayAssumeCandidatesAreVisible() {
- return true;
- }
-
- @Override
public boolean requiresContextUser() {
return true;
}
@@ -457,33 +448,19 @@
}
}
- private class ByFullName implements Searcher<AccountState> {
- boolean allowSkippingVisibilityCheck = true;
-
+ private class ByFullName extends StringSearcher {
ByFullName() {
super();
}
- ByFullName(boolean allowSkippingVisibilityCheck) {
- this();
- this.allowSkippingVisibilityCheck = allowSkippingVisibilityCheck;
+ @Override
+ protected boolean matches(String input) {
+ return true;
}
@Override
- public boolean callerMayAssumeCandidatesAreVisible() {
- return allowSkippingVisibilityCheck;
- }
-
- @Override
- public Optional<AccountState> tryParse(String input) {
- List<AccountState> results =
- accountQueryProvider.get().enforceVisibility(true).byFullName(input);
- return results.size() == 1 ? Optional.of(results.get(0)) : Optional.empty();
- }
-
- @Override
- public Stream<AccountState> search(AccountState input) {
- return Stream.of(input);
+ public Stream<AccountState> search(String input) {
+ return accountQueryProvider.get().byFullName(input).stream();
}
@Override
@@ -493,22 +470,10 @@
}
private class ByDefaultSearch extends StringSearcher {
- boolean allowSkippingVisibilityCheck = true;
-
ByDefaultSearch() {
super();
}
- ByDefaultSearch(boolean allowSkippingVisibilityCheck) {
- this();
- this.allowSkippingVisibilityCheck = allowSkippingVisibilityCheck;
- }
-
- @Override
- public boolean callerMayAssumeCandidatesAreVisible() {
- return allowSkippingVisibilityCheck;
- }
-
@Override
public boolean requiresContextUser() {
return true;
@@ -533,8 +498,7 @@
} catch (PermissionBackendException e) {
// remains false
}
- return accountQueryProvider.get().enforceVisibility(true)
- .byDefault(input, canSeeSecondaryEmails).stream();
+ return accountQueryProvider.get().byDefault(input, canSeeSecondaryEmails).stream();
}
@Override
@@ -562,18 +526,6 @@
.addAll(nameOrEmailSearchers)
.build();
- private final ImmutableList<Searcher<?>> forcedVisibilitySearchers =
- ImmutableList.of(
- new ByNameAndEmail(),
- new ByEmail(),
- new FromRealm(),
- new ByFullName(false),
- new ByDefaultSearch(false),
- new BySelf(),
- new ByExactAccountId(),
- new ByParenthesizedAccountId(),
- new ByUsername());
-
private final AccountCache accountCache;
private final AccountControl.Factory accountControlFactory;
private final Emails emails;
@@ -670,25 +622,21 @@
*
* @param asUser user to resolve the users by.
* @param input input string.
- * @param forceVisibilityCheck whether to force all searchers to check for visibility.
* @return a result describing matching accounts. Never null even if the result set is empty.
* @throws ConfigInvalidException if an error occurs.
* @throws IOException if an error occurs.
*/
- public Result resolveAsUser(CurrentUser asUser, String input, boolean forceVisibilityCheck)
+ public Result resolveAsUser(CurrentUser asUser, String input)
throws ConfigInvalidException, IOException {
- return resolveAsUser(asUser, input, AccountResolver::isActive, forceVisibilityCheck);
+ return resolveAsUser(asUser, input, AccountResolver::isActive);
}
public Result resolveAsUser(
- CurrentUser asUser,
- String input,
- Predicate<AccountState> accountActivityPredicate,
- boolean forceVisibilityCheck)
+ CurrentUser asUser, String input, Predicate<AccountState> accountActivityPredicate)
throws ConfigInvalidException, IOException {
return searchImpl(
input,
- forceVisibilityCheck ? forcedVisibilitySearchers : searchers,
+ searchers,
asUser,
new ProvidedUserCanSeePredicate(asUser),
accountActivityPredicate);
@@ -826,9 +774,9 @@
}
Stream<AccountState> results = maybeResults.get();
- if (!searcher.callerMayAssumeCandidatesAreVisible()) {
- results = results.filter(visibilitySupplier.get());
- }
+ // Filter out non-visible results, except if it's the BySelf searcher. Since users can always
+ // see themselves checking the visibility is not needed for the BySelf searcher.
+ results = searcher instanceof BySelf ? results : results.filter(visibilitySupplier.get());
List<AccountState> list;
if (searcher.callerShouldFilterOutInactiveCandidates()) {
diff --git a/java/com/google/gerrit/server/mail/send/ProjectWatch.java b/java/com/google/gerrit/server/mail/send/ProjectWatch.java
index cbf47c5..f4c211d 100644
--- a/java/com/google/gerrit/server/mail/send/ProjectWatch.java
+++ b/java/com/google/gerrit/server/mail/send/ProjectWatch.java
@@ -253,7 +253,6 @@
qb = WatcherChangeQueryBuilder.asUser(args.queryBuilder.get(), user);
p = qb.isVisible();
}
- qb.forceAccountVisibilityCheck();
if (filter != null) {
Predicate<ChangeData> filterPredicate = qb.parse(filter);
diff --git a/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java b/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
index b83b010..622c4bf 100644
--- a/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
+++ b/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
@@ -488,7 +488,6 @@
private final Arguments args;
protected Map<String, String> hasOperandAliases = Collections.emptyMap();
private Map<Account.Id, DestinationList> destinationListByAccount = new HashMap<>();
- private boolean forceAccountVisibilityCheck = false;
private static final Splitter RULE_SPLITTER = Splitter.on("=");
private static final Splitter PLUGIN_SPLITTER = Splitter.on("_");
@@ -519,11 +518,6 @@
return args;
}
- /** Whether to force account visibility check when searching for changes by account(s). */
- public void forceAccountVisibilityCheck() {
- forceAccountVisibilityCheck = true;
- }
-
@Operator
public Predicate<ChangeData> age(String value) {
return new AgePredicate(value);
@@ -1710,9 +1704,7 @@
private Set<Account.Id> parseAccount(String who)
throws QueryParseException, IOException, ConfigInvalidException {
try {
- return args.accountResolver
- .resolveAsUser(args.getUser(), who, forceAccountVisibilityCheck)
- .asNonEmptyIdSet();
+ return args.accountResolver.resolveAsUser(args.getUser(), who).asNonEmptyIdSet();
} catch (UnresolvableAccountException e) {
if (e.isSelf()) {
throw new QueryRequiresAuthException(e.getMessage(), e);
@@ -1726,7 +1718,7 @@
throws QueryParseException, IOException, ConfigInvalidException {
try {
return args.accountResolver
- .resolveAsUser(args.getUser(), who, activityFilter, forceAccountVisibilityCheck)
+ .resolveAsUser(args.getUser(), who, activityFilter)
.asNonEmptyIdSet();
} catch (UnresolvableAccountException e) {
if (e.isSelf()) {
diff --git a/javatests/com/google/gerrit/acceptance/server/account/AccountResolverIT.java b/javatests/com/google/gerrit/acceptance/server/account/AccountResolverIT.java
index 0cfa0f8..6f3aa15 100644
--- a/javatests/com/google/gerrit/acceptance/server/account/AccountResolverIT.java
+++ b/javatests/com/google/gerrit/acceptance/server/account/AccountResolverIT.java
@@ -26,11 +26,13 @@
import com.google.gerrit.acceptance.config.GerritConfig;
import com.google.gerrit.acceptance.testsuite.account.AccountOperations;
import com.google.gerrit.acceptance.testsuite.account.TestAccount;
+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.extensions.common.AccountVisibility;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.ServerInitiated;
+import com.google.gerrit.server.account.AccountControl;
import com.google.gerrit.server.account.AccountResolver;
import com.google.gerrit.server.account.AccountResolver.Result;
import com.google.gerrit.server.account.AccountResolver.UnresolvableAccountException;
@@ -56,7 +58,9 @@
@Inject @ServerInitiated private Provider<AccountsUpdate> accountsUpdateProvider;
@Inject private AccountOperations accountOperations;
@Inject private AccountResolver accountResolver;
+ @Inject private AccountControl.Factory accountControlFactory;
@Inject private Provider<CurrentUser> self;
+ @Inject private GroupOperations groupOperations;
@Inject private RequestScopeOperations requestScopeOperations;
@Inject private Sequences sequences;
@@ -365,6 +369,37 @@
assertThat(resolve("doe")).containsExactly(id2);
}
+ @Test
+ @GerritConfig(name = "accounts.visibility", value = "SAME_GROUP")
+ public void resolveAsUser_byFullName_accountThatIsNotVisibleToCurrentUserIsFound()
+ throws Exception {
+ Account.Id currentUser = accountOperations.newAccount().create();
+ Account.Id resolveAsUser = accountOperations.newAccount().create();
+ Account.Id userToBeFound = accountOperations.newAccount().fullname("Somebodys Name").create();
+
+ // Create a group that contains resolveAsUser and userToBeFound, so that resolveAsUser can see
+ // userToBeFound.
+ groupOperations.newGroup().addMember(resolveAsUser).addMember(userToBeFound).create();
+
+ // Verify that resolveAsUser can see userToBeFound.
+ assertThat(canSee(resolveAsUser, userToBeFound)).isTrue();
+
+ // Verify that currentUser cannot see userToBeFound
+ assertThat(canSee(currentUser, userToBeFound)).isFalse();
+
+ // Resolving userToBeFound as resolveAsUser should work even if the currentUser cannot see
+ // userToBeFound.
+ requestScopeOperations.setApiUser(currentUser);
+ String input = accountOperations.account(userToBeFound).get().fullname().get();
+ assertThat(resolveAsUser(resolveAsUser, input)).containsExactly(userToBeFound);
+ }
+
+ private boolean canSee(Account.Id currentUser, Account.Id userToBeSeen) {
+ return accountControlFactory
+ .get(identifiedUserFactory.create(currentUser))
+ .canSee(userToBeSeen);
+ }
+
private ImmutableSet<Account.Id> resolve(Object input) throws Exception {
return resolveAsResult(input).asIdSet();
}
@@ -373,6 +408,16 @@
return accountResolver.resolve(input.toString());
}
+ private ImmutableSet<Account.Id> resolveAsUser(Account.Id resolveAsUser, Object input)
+ throws Exception {
+ return resolveAsUserAsResult(resolveAsUser, input).asIdSet();
+ }
+
+ private Result resolveAsUserAsResult(Account.Id resolveAsUser, Object input) throws Exception {
+ return accountResolver.resolveAsUser(
+ identifiedUserFactory.create(resolveAsUser), input.toString());
+ }
+
@SuppressWarnings("deprecation")
private ImmutableSet<Account.Id> resolveByNameOrEmail(Object input) throws Exception {
return accountResolver.resolveByNameOrEmail(input.toString()).asIdSet();
diff --git a/javatests/com/google/gerrit/server/account/AccountResolverTest.java b/javatests/com/google/gerrit/server/account/AccountResolverTest.java
index 37728f7..34f746a 100644
--- a/javatests/com/google/gerrit/server/account/AccountResolverTest.java
+++ b/javatests/com/google/gerrit/server/account/AccountResolverTest.java
@@ -43,7 +43,6 @@
private final String pattern;
private final boolean shortCircuit;
private final ImmutableList<AccountState> accounts;
- private boolean assumeVisible;
private boolean filterInactive;
private TestSearcher(String pattern, boolean shortCircuit, AccountState... accounts) {
@@ -68,15 +67,6 @@
}
@Override
- public boolean callerMayAssumeCandidatesAreVisible() {
- return assumeVisible;
- }
-
- void setCallerMayAssumeCandidatesAreVisible() {
- this.assumeVisible = true;
- }
-
- @Override
public boolean callerShouldFilterOutInactiveCandidates() {
return filterInactive;
}
@@ -139,17 +129,6 @@
}
@Test
- public void skipVisibilityCheck() throws Exception {
- TestSearcher searcher = new TestSearcher("foo", false, newAccount(1), newAccount(2));
- ImmutableList<Searcher<?>> searchers = ImmutableList.of(searcher);
-
- assertThat(search("foo", searchers, only(2)).asIdSet()).containsExactlyElementsIn(ids(2));
-
- searcher.setCallerMayAssumeCandidatesAreVisible();
- assertThat(search("foo", searchers, only(2)).asIdSet()).containsExactlyElementsIn(ids(1, 2));
- }
-
- @Test
public void dontFilterInactive() throws Exception {
ImmutableList<Searcher<?>> searchers =
ImmutableList.of(
