Fix LabelPredicate group matching for included external groups

The same problem fixed for the ownerin and uploaderin predicates in
Change I5cee8556be23 applies to the LabelPredicate when a group arg is
given. Share the approach and code from Change I5cee8556be23 to check if
an internal group includes any external groups.

Change-Id: I6526299e8eb92fbab44869f72889fb1595ae284f
Release-Notes: Fixed label:...,group= for internal groups that include external groups

3 files changed, 57 insertions, 13 deletions

diff --git a/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java b/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
index 5b6f044ae7..c090aa068a 100644
--- a/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
+++ b/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
@@ -1241,9 +1241,7 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData, ChangeQueryBuil
   public Predicate<ChangeData> ownerin(String group) throws QueryParseException, IOException {
     GroupReference g = parseGroup(group);
     AccountGroup.UUID groupId = g.getUUID();
-    GroupDescription.Basic groupDescription = args.groupBackend.get(groupId);
-    if (!(groupDescription instanceof GroupDescription.Internal)
-        || containsExternalSubGroups((GroupDescription.Internal) groupDescription)) {
+    if (isOrContainsExternalGroup(args.groupBackend, groupId)) {
       return new OwnerinPredicate(args.userFactory, groupId);
     }
 
@@ -1263,9 +1261,7 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData, ChangeQueryBuil
 
     GroupReference g = parseGroup(group);
     AccountGroup.UUID groupId = g.getUUID();
-    GroupDescription.Basic groupDescription = args.groupBackend.get(groupId);
-    if (!(groupDescription instanceof GroupDescription.Internal)
-        || containsExternalSubGroups((GroupDescription.Internal) groupDescription)) {
+    if (isOrContainsExternalGroup(args.groupBackend, groupId)) {
       return new UploaderinPredicate(args.userFactory, groupId);
     }
 
@@ -1636,13 +1632,28 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData, ChangeQueryBuil
     return Predicate.and(predicates);
   }
 
-  private boolean containsExternalSubGroups(GroupDescription.Internal internalGroup) {
+  protected static boolean isOrContainsExternalGroup(
+      GroupBackend groupBackend, AccountGroup.UUID groupId) {
+    if (groupId != null) {
+      GroupDescription.Basic groupDescription = groupBackend.get(groupId);
+      if (!(groupDescription instanceof GroupDescription.Internal)
+          || containsExternalSubGroups(
+              groupBackend, (GroupDescription.Internal) groupDescription)) {
+        return true;
+      }
+    }
+    return false;
+  }
+
+  protected static boolean containsExternalSubGroups(
+      GroupBackend groupBackend, GroupDescription.Internal internalGroup) {
     for (AccountGroup.UUID subGroupUuid : internalGroup.getSubgroups()) {
-      GroupDescription.Basic subGroupDescription = args.groupBackend.get(subGroupUuid);
+      GroupDescription.Basic subGroupDescription = groupBackend.get(subGroupUuid);
       if (!(subGroupDescription instanceof GroupDescription.Internal)) {
         return true;
       }
-      if (containsExternalSubGroups((GroupDescription.Internal) subGroupDescription)) {
+      if (containsExternalSubGroups(
+          groupBackend, (GroupDescription.Internal) subGroupDescription)) {
         return true;
       }
     }
diff --git a/java/com/google/gerrit/server/query/change/LabelPredicate.java b/java/com/google/gerrit/server/query/change/LabelPredicate.java
index cbf5df434f..f50cd04771 100644
--- a/java/com/google/gerrit/server/query/change/LabelPredicate.java
+++ b/java/com/google/gerrit/server/query/change/LabelPredicate.java
@@ -23,6 +23,7 @@ import com.google.gerrit.index.query.Predicate;
 import com.google.gerrit.index.query.RangeUtil;
 import com.google.gerrit.index.query.RangeUtil.Range;
 import com.google.gerrit.server.IdentifiedUser;
+import com.google.gerrit.server.account.GroupBackend;
 import com.google.gerrit.server.permissions.PermissionBackend;
 import com.google.gerrit.server.project.ProjectCache;
 import com.google.gerrit.server.util.LabelVote;
@@ -40,6 +41,7 @@ public class LabelPredicate extends OrPredicate<ChangeData> {
     protected final String value;
     protected final Set<Account.Id> accounts;
     protected final AccountGroup.UUID group;
+    protected final GroupBackend groupBackend;
 
     protected Args(
         ProjectCache projectCache,
@@ -47,13 +49,15 @@ public class LabelPredicate extends OrPredicate<ChangeData> {
         IdentifiedUser.GenericFactory userFactory,
         String value,
         Set<Account.Id> accounts,
-        AccountGroup.UUID group) {
+        AccountGroup.UUID group,
+        GroupBackend groupBackend) {
       this.projectCache = projectCache;
       this.permissionBackend = permissionBackend;
       this.userFactory = userFactory;
       this.value = value;
       this.accounts = accounts;
       this.group = group;
+      this.groupBackend = groupBackend;
     }
   }
 
@@ -78,7 +82,14 @@ public class LabelPredicate extends OrPredicate<ChangeData> {
       AccountGroup.UUID group) {
     super(
         predicates(
-            new Args(a.projectCache, a.permissionBackend, a.userFactory, value, accounts, group)));
+            new Args(
+                a.projectCache,
+                a.permissionBackend,
+                a.userFactory,
+                value,
+                accounts,
+                group,
+                a.groupBackend)));
     this.value = value;
   }
 
@@ -147,7 +158,7 @@ public class LabelPredicate extends OrPredicate<ChangeData> {
   }
 
   protected static Predicate<ChangeData> equalsLabelPredicate(Args args, String label, int expVal) {
-    if (args.group != null && !args.group.isInternalGroup()) {
+    if (ChangeQueryBuilder.isOrContainsExternalGroup(args.groupBackend, args.group)) {
       // We can only get members of internal groups and negating an index search that doesn't
       // include the external group information leads to incorrect query results. Use a
       // PostFilterPredicate in this case instead.
@@ -164,7 +175,7 @@ public class LabelPredicate extends OrPredicate<ChangeData> {
   }
 
   protected static Predicate<ChangeData> magicLabelPredicate(Args args, MagicLabelVote mlv) {
-    if (args.group != null && !args.group.isInternalGroup()) {
+    if (ChangeQueryBuilder.isOrContainsExternalGroup(args.groupBackend, args.group)) {
       // We can only get members of internal groups and negating an index search that doesn't
       // include the external group information leads to incorrect query results. Use a
       // PostFilterPredicate in this case instead.
diff --git a/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java b/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
index 74929adbb8..bda17da2c3 100644
--- a/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
+++ b/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
@@ -1356,12 +1356,25 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests {
     // create group and add users
     AccountGroup.UUID external_group1 = AccountGroup.uuid("testbackend:group1");
     AccountGroup.UUID external_group2 = AccountGroup.uuid("testbackend:group2");
+    String nameOfGroupThatContainsExternalGroupAsSubgroup = "test-group-1";
+    String nameOfGroupThatContainsExternalGroupAsSubSubgroup = "test-group-2";
     testGroupBackend.create(external_group1);
     testGroupBackend.create(external_group2);
     testGroupBackend.setMembershipsOf(
         user1, new ListGroupMembership(ImmutableList.of(external_group1)));
     testGroupBackend.setMembershipsOf(
         user2, new ListGroupMembership(ImmutableList.of(external_group2)));
+    AccountGroup.UUID uuidOfGroupThatContainsExternalGroupAsSubgroup =
+        groupOperations
+            .newGroup()
+            .name(nameOfGroupThatContainsExternalGroupAsSubgroup)
+            .addSubgroup(external_group1)
+            .create();
+    groupOperations
+        .newGroup()
+        .name(nameOfGroupThatContainsExternalGroupAsSubSubgroup)
+        .addSubgroup(uuidOfGroupThatContainsExternalGroupAsSubgroup)
+        .create();
 
     Change change1 = insert(repo, newChange(repo), user1);
     Change change2 = insert(repo, newChange(repo), user1);
@@ -1380,6 +1393,10 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests {
 
     assertQuery("label:Code-Review=+1," + external_group1.get(), change1);
     assertQuery("label:Code-Review=+1,group=" + external_group1.get(), change1);
+    assertQuery(
+        "label:Code-Review=+1,group=" + nameOfGroupThatContainsExternalGroupAsSubgroup, change1);
+    assertQuery(
+        "label:Code-Review=+1,group=" + nameOfGroupThatContainsExternalGroupAsSubSubgroup, change1);
     assertQuery("label:Code-Review=+1,user=user1", change1);
     assertQuery("label:Code-Review=+1,user=user2");
     assertQuery("label:Code-Review=+1,group=" + external_group2.get());
@@ -1387,6 +1404,11 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests {
     // Negated operator tests
     assertQuery("-label:Code-Review=+1," + external_group1.get(), change2);
     assertQuery("-label:Code-Review=+1,group=" + external_group1.get(), change2);
+    assertQuery(
+        "-label:Code-Review=+1,group=" + nameOfGroupThatContainsExternalGroupAsSubgroup, change2);
+    assertQuery(
+        "-label:Code-Review=+1,group=" + nameOfGroupThatContainsExternalGroupAsSubSubgroup,
+        change2);
     assertQuery("-label:Code-Review=+1,user=user1", change2);
     assertQuery("-label:Code-Review=+1,group=" + external_group2.get(), change2, change1);
     assertQuery("-label:Code-Review=+1,user=user2", change2, change1);