From de3662444a742a189213e4de6dc75c02e90de19c Mon Sep 17 00:00:00 2001 From: Marcin Czech Date: Tue, 27 Aug 2019 08:57:33 +0100 Subject: [PATCH] Fix issue with querying inactive user changes Introduce the custom account activity predicate to allow customise the way of resolving accounts during the search. Current implementation filters out inactive users during the change data query but changes from inactive user should be visible. Bug: Issue 11367 Change-Id: Ib8cfcf3e7c4f21a0207f801c111c5359716bda9c --- .../server/account/AccountResolver.java | 19 ++++++++--- .../query/change/ChangeQueryBuilder.java | 18 +++++++++-- .../acceptance/api/accounts/AccountIT.java | 18 +++++++++++ .../server/account/AccountResolverTest.java | 32 ++++++++++++++++++- 4 files changed, 80 insertions(+), 7 deletions(-) diff --git a/java/com/google/gerrit/server/account/AccountResolver.java b/java/com/google/gerrit/server/account/AccountResolver.java index 2ac7147dbc..f9945b5046 100644 --- a/java/com/google/gerrit/server/account/AccountResolver.java +++ b/java/com/google/gerrit/server/account/AccountResolver.java @@ -516,7 +516,12 @@ public class AccountResolver { * @throws IOException if an error occurs. */ public Result resolve(String input) throws ConfigInvalidException, IOException { - return searchImpl(input, searchers, visibilitySupplier()); + return searchImpl(input, searchers, visibilitySupplier(), accountActivityPredicate()); + } + + public Result resolve(String input, Predicate accountActivityPredicate) + throws ConfigInvalidException, IOException { + return searchImpl(input, searchers, visibilitySupplier(), accountActivityPredicate); } /** @@ -544,18 +549,24 @@ public class AccountResolver { */ @Deprecated public Result resolveByNameOrEmail(String input) throws ConfigInvalidException, IOException { - return searchImpl(input, nameOrEmailSearchers, visibilitySupplier()); + return searchImpl( + input, nameOrEmailSearchers, visibilitySupplier(), accountActivityPredicate()); } private Supplier> visibilitySupplier() { return () -> accountControlFactory.get()::canSee; } + private Predicate accountActivityPredicate() { + return (AccountState accountState) -> accountState.getAccount().isActive(); + } + @VisibleForTesting Result searchImpl( String input, ImmutableList> searchers, - Supplier> visibilitySupplier) + Supplier> visibilitySupplier, + Predicate accountActivityPredicate) throws ConfigInvalidException, IOException { visibilitySupplier = Suppliers.memoize(visibilitySupplier::get); List inactive = new ArrayList<>(); @@ -576,7 +587,7 @@ public class AccountResolver { // Keep track of all inactive candidates discovered by any searchers. If we end up short- // circuiting, the inactive list will be discarded. List active = new ArrayList<>(); - results.forEach(a -> (a.getAccount().isActive() ? active : inactive).add(a)); + results.forEach(a -> (accountActivityPredicate.test(a) ? active : inactive).add(a)); list = active; } else { list = results.collect(toImmutableList()); diff --git a/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java b/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java index c0fe1e2a98..d306e54a21 100644 --- a/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java +++ b/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java @@ -54,6 +54,7 @@ import com.google.gerrit.server.StarredChangesUtil; import com.google.gerrit.server.account.AccountCache; import com.google.gerrit.server.account.AccountResolver; import com.google.gerrit.server.account.AccountResolver.UnresolvableAccountException; +import com.google.gerrit.server.account.AccountState; import com.google.gerrit.server.account.GroupBackend; import com.google.gerrit.server.account.GroupBackends; import com.google.gerrit.server.account.GroupMembers; @@ -1002,7 +1003,7 @@ public class ChangeQueryBuilder extends QueryBuilder owner(String who) throws QueryParseException, IOException, ConfigInvalidException { - return owner(parseAccount(who)); + return owner(parseAccount(who, (AccountState s) -> true)); } private Predicate owner(Set who) { @@ -1025,7 +1026,7 @@ public class ChangeQueryBuilder extends QueryBuilder assignee(String who) throws QueryParseException, IOException, ConfigInvalidException { - return assignee(parseAccount(who)); + return assignee(parseAccount(who, (AccountState s) -> true)); } private Predicate assignee(Set who) { @@ -1351,6 +1352,19 @@ public class ChangeQueryBuilder extends QueryBuilder parseAccount( + String who, java.util.function.Predicate activityFilter) + throws QueryParseException, IOException, ConfigInvalidException { + try { + return args.accountResolver.resolve(who, activityFilter).asNonEmptyIdSet(); + } catch (UnresolvableAccountException e) { + if (e.isSelf()) { + throw new QueryRequiresAuthException(e.getMessage(), e); + } + throw new QueryParseException(e.getMessage(), e); + } + } + private GroupReference parseGroup(String group) throws QueryParseException { GroupReference g = GroupBackends.findBestSuggestion(args.groupBackend, group); if (g == null) { diff --git a/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java b/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java index 7f8ed767b4..6247739e50 100644 --- a/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java +++ b/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java @@ -558,6 +558,24 @@ public class AccountIT extends AbstractDaemonTest { accountIndexedCounter.assertReindexOf(user); } + @Test + public void shouldAllowQueryByEmailForInactiveUser() throws Exception { + Account.Id activatableAccountId = + accountOperations.newAccount().inactive().preferredEmail("foo@activatable.com").create(); + accountIndexedCounter.assertReindexOf(activatableAccountId, 1); + + gApi.changes().query("owner:foo@activatable.com").get(); + } + + @Test + public void shouldAllowQueryByUserNameForInactiveUser() throws Exception { + Account.Id activatableAccountId = + accountOperations.newAccount().inactive().username("foo").create(); + accountIndexedCounter.assertReindexOf(activatableAccountId, 1); + + gApi.changes().query("owner:foo").get(); + } + @Test public void validateAccountActivation() throws Exception { Account.Id activatableAccountId = diff --git a/javatests/com/google/gerrit/server/account/AccountResolverTest.java b/javatests/com/google/gerrit/server/account/AccountResolverTest.java index 5f662e9725..6f074dde02 100644 --- a/javatests/com/google/gerrit/server/account/AccountResolverTest.java +++ b/javatests/com/google/gerrit/server/account/AccountResolverTest.java @@ -162,6 +162,23 @@ public class AccountResolverTest extends GerritBaseTests { assertThat(filteredInactiveIds(result)).isEmpty(); } + @Test + public void shouldUseCustomAccountActivityPredicate() throws Exception { + TestSearcher searcher1 = new TestSearcher("foo", false, newInactiveAccount(1)); + searcher1.setCallerShouldFilterOutInactiveCandidates(); + TestSearcher searcher2 = new TestSearcher("f.*", false, newInactiveAccount(2)); + searcher2.setCallerShouldFilterOutInactiveCandidates(); + ImmutableList> searchers = ImmutableList.of(searcher1, searcher2); + + Result result = search("foo", searchers, allVisible(), (a) -> true); + // Searchers always short-circuit when finding a non-empty result list, + // and this one didn't filter out inactive results, + // so the second searcher never ran. + assertThat(result.asIdSet()).containsExactlyElementsIn(ids(1)); + assertThat(getOnlyElement(result.asList()).getAccount().isActive()).isFalse(); + assertThat(filteredInactiveIds(result)).isEmpty(); + } + @Test public void filterInactiveEventuallyFindingResults() throws Exception { TestSearcher searcher1 = new TestSearcher("foo", false, newInactiveAccount(1)); @@ -324,7 +341,16 @@ public class AccountResolverTest extends GerritBaseTests { ImmutableList> searchers, Supplier> visibilitySupplier) throws Exception { - return newAccountResolver().searchImpl(input, searchers, visibilitySupplier); + return search(input, searchers, visibilitySupplier, activityPrediate()); + } + + private Result search( + String input, + ImmutableList> searchers, + Supplier> visibilitySupplier, + Predicate activityPredicate) + throws Exception { + return newAccountResolver().searchImpl(input, searchers, visibilitySupplier, activityPredicate); } private static AccountResolver newAccountResolver() { @@ -350,6 +376,10 @@ public class AccountResolverTest extends GerritBaseTests { return () -> a -> true; } + private Predicate activityPrediate() { + return (AccountState accountState) -> accountState.getAccount().isActive(); + } + private static Supplier> only(int... ids) { ImmutableSet idSet = Arrays.stream(ids).mapToObj(Account.Id::new).collect(toImmutableSet());