Fix ownerin/uploaderin for internal groups that include external groups

The ownerin/uploaderin are post filter predicates, except for internal
groups where the query is rewritten as an OR-query of owner/uploader
predicates for all the group members as a performance optimisation. The
problem is that this optimization doesn't work for internal groups that
contain external groups as sub groups. This is because we can't list
members of external groups and hence the rewritten query doesn't have
predicates for the members of the included external groups and hence
changes for them are not matched. To fix this we skip the optimisation
for internal groups that contain external groups as sub groups and
instead use the post filter predicate, same as for external groups.

Release-Notes: Fixed ownerin/uploaderin for internal groups that include external groups
Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: I5cee8556be2306b2d486185fded37a0fccdf05ed
Bug: Google b/267526103
(cherry picked from commit d28408229a47aef761de668e515b7f2ab5aaaa6f)

9 files changed, 123 insertions, 2 deletions

diff --git a/java/com/google/gerrit/acceptance/BUILD b/java/com/google/gerrit/acceptance/BUILD
index d6dff8f00a..d03d0310c3 100644
--- a/java/com/google/gerrit/acceptance/BUILD
+++ b/java/com/google/gerrit/acceptance/BUILD
@@ -65,6 +65,7 @@ TEST_DEPS = [
     "//java/com/google/gerrit/pgm/util",
     "//java/com/google/gerrit/truth",
     "//java/com/google/gerrit/acceptance/config",
+    "//java/com/google/gerrit/acceptance/testsuite/group",
     "//java/com/google/gerrit/acceptance/testsuite/project",
     "//java/com/google/gerrit/server/fixes/testing",
     "//java/com/google/gerrit/server/data",
diff --git a/java/com/google/gerrit/acceptance/testsuite/group/BUILD b/java/com/google/gerrit/acceptance/testsuite/group/BUILD
new file mode 100644
index 0000000000..d4f117586a
--- /dev/null
+++ b/java/com/google/gerrit/acceptance/testsuite/group/BUILD
@@ -0,0 +1,25 @@
+load("@rules_java//java:defs.bzl", "java_library")
+
+package(default_testonly = 1)
+
+java_library(
+    name = "group",
+    srcs = glob(["*.java"]),
+    visibility = ["//visibility:public"],
+    deps = [
+        "//java/com/google/gerrit/acceptance:function",
+        "//java/com/google/gerrit/common:annotations",
+        "//java/com/google/gerrit/common:server",
+        "//java/com/google/gerrit/entities",
+        "//java/com/google/gerrit/exceptions",
+        "//java/com/google/gerrit/extensions:api",
+        "//java/com/google/gerrit/server",
+        "//lib:guava",
+        "//lib:jgit",
+        "//lib:jgit-junit",
+        "//lib/auto:auto-value",
+        "//lib/auto:auto-value-annotations",
+        "//lib/commons:lang3",
+        "//lib/guice",
+    ],
+)
diff --git a/java/com/google/gerrit/server/group/testing/TestGroupBackend.java b/java/com/google/gerrit/server/group/testing/TestGroupBackend.java
index 12d8c936d6..2d9c798dda 100644
--- a/java/com/google/gerrit/server/group/testing/TestGroupBackend.java
+++ b/java/com/google/gerrit/server/group/testing/TestGroupBackend.java
@@ -102,6 +102,11 @@ public class TestGroupBackend implements GroupBackend {
     memberships.put(user, membership);
   }
 
+  /** Remove the memberships of the given user. No-op if the user does not have any memberships. */
+  public void removeMembershipsOf(Account.Id user) {
+    memberships.remove(user);
+  }
+
   @Override
   public boolean handles(AccountGroup.UUID uuid) {
     if (uuid != null) {
diff --git a/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java b/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
index e36dbfc0ba..0ffdb1c124 100644
--- a/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
+++ b/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
@@ -1246,7 +1246,8 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData, ChangeQueryBuil
 
     AccountGroup.UUID groupId = g.getUUID();
     GroupDescription.Basic groupDescription = args.groupBackend.get(groupId);
-    if (!(groupDescription instanceof GroupDescription.Internal)) {
+    if (!(groupDescription instanceof GroupDescription.Internal)
+        || containsExernalSubGroups((GroupDescription.Internal) groupDescription)) {
       return new OwnerinPredicate(args.userFactory, groupId);
     }
 
@@ -1271,7 +1272,8 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData, ChangeQueryBuil
 
     AccountGroup.UUID groupId = g.getUUID();
     GroupDescription.Basic groupDescription = args.groupBackend.get(groupId);
-    if (!(groupDescription instanceof GroupDescription.Internal)) {
+    if (!(groupDescription instanceof GroupDescription.Internal)
+        || containsExernalSubGroups((GroupDescription.Internal) groupDescription)) {
       return new UploaderinPredicate(args.userFactory, groupId);
     }
 
@@ -1645,6 +1647,22 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData, ChangeQueryBuil
     return Predicate.and(predicates);
   }
 
+  private boolean containsExernalSubGroups(GroupDescription.Internal internalGroup)
+      throws IOException {
+    for (AccountGroup.UUID subGroupUuid : internalGroup.getSubgroups()) {
+      GroupDescription.Basic subGroupDescription = args.groupBackend.get(subGroupUuid);
+      if (!(subGroupDescription instanceof GroupDescription.Internal)) {
+        return true;
+      }
+      boolean containsExernalSubGroups =
+          containsExernalSubGroups((GroupDescription.Internal) subGroupDescription);
+      if (containsExernalSubGroups) {
+        return true;
+      }
+    }
+    return false;
+  }
+
   private Set<Account.Id> getMembers(AccountGroup.UUID g) throws IOException {
     Set<Account.Id> accounts;
     Set<Account.Id> allMembers =
diff --git a/java/com/google/gerrit/testing/BUILD b/java/com/google/gerrit/testing/BUILD
index a7a1795efd..05987bbdd3 100644
--- a/java/com/google/gerrit/testing/BUILD
+++ b/java/com/google/gerrit/testing/BUILD
@@ -15,6 +15,7 @@ java_library(
     runtime_deps = ["//java/com/google/gerrit/index/testing"],
     deps = [
         "//java/com/google/gerrit/acceptance/config",
+        "//java/com/google/gerrit/acceptance/testsuite/group",
         "//java/com/google/gerrit/acceptance/testsuite/project",
         "//java/com/google/gerrit/auth",
         "//java/com/google/gerrit/common:annotations",
diff --git a/java/com/google/gerrit/testing/InMemoryModule.java b/java/com/google/gerrit/testing/InMemoryModule.java
index b00cadb7a7..49edd4c4d4 100644
--- a/java/com/google/gerrit/testing/InMemoryModule.java
+++ b/java/com/google/gerrit/testing/InMemoryModule.java
@@ -20,6 +20,8 @@ import static com.google.inject.Scopes.SINGLETON;
 
 import com.google.common.base.Strings;
 import com.google.common.util.concurrent.ListeningExecutorService;
+import com.google.gerrit.acceptance.testsuite.group.GroupOperations;
+import com.google.gerrit.acceptance.testsuite.group.GroupOperationsImpl;
 import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
 import com.google.gerrit.acceptance.testsuite.project.ProjectOperationsImpl;
 import com.google.gerrit.auth.AuthModule;
@@ -270,6 +272,7 @@ public class InMemoryModule extends FactoryModule {
     install(new ConfigExperimentFeaturesModule());
 
     bind(ProjectOperations.class).to(ProjectOperationsImpl.class);
+    bind(GroupOperations.class).to(GroupOperationsImpl.class);
     bind(TestGroupBackend.class).in(SINGLETON);
     DynamicSet.bind(binder(), GroupBackend.class).to(TestGroupBackend.class);
   }
diff --git a/javatests/com/google/gerrit/acceptance/BUILD b/javatests/com/google/gerrit/acceptance/BUILD
index 75c90f249e..fe451c4be0 100644
--- a/javatests/com/google/gerrit/acceptance/BUILD
+++ b/javatests/com/google/gerrit/acceptance/BUILD
@@ -5,6 +5,7 @@ junit_tests(
     srcs = glob(["**/*.java"]),
     deps = [
         "//java/com/google/gerrit/acceptance:lib",
+        "//java/com/google/gerrit/acceptance/testsuite/group",
         "//java/com/google/gerrit/server/util/time",
         "//java/com/google/gerrit/testing:gerrit-test-util",
         "//java/com/google/gerrit/truth",
diff --git a/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java b/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
index cbdc138748..74929adbb8 100644
--- a/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
+++ b/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
@@ -46,12 +46,14 @@ import com.google.gerrit.acceptance.ExtensionRegistry;
 import com.google.gerrit.acceptance.ExtensionRegistry.Registration;
 import com.google.gerrit.acceptance.FakeSubmitRule;
 import com.google.gerrit.acceptance.config.GerritConfig;
+import com.google.gerrit.acceptance.testsuite.group.GroupOperations;
 import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
 import com.google.gerrit.common.Nullable;
 import com.google.gerrit.entities.Account;
 import com.google.gerrit.entities.AccountGroup;
 import com.google.gerrit.entities.BranchNameKey;
 import com.google.gerrit.entities.Change;
+import com.google.gerrit.entities.GroupDescription;
 import com.google.gerrit.entities.GroupReference;
 import com.google.gerrit.entities.LabelId;
 import com.google.gerrit.entities.LabelType;
@@ -198,6 +200,7 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests {
   @Inject protected AuthRequest.Factory authRequestFactory;
   @Inject protected ExternalIdFactory externalIdFactory;
   @Inject protected ProjectOperations projectOperations;
+  @Inject protected GroupOperations groupOperations;
 
   @Inject private ProjectConfig.Factory projectConfigFactory;
 
@@ -739,6 +742,38 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests {
     assertQuery("ownerin:\"Registered Users\"", change3, change2, change1);
     assertQuery("ownerin:\"Registered Users\" project:repo", change3, change2, change1);
     assertQuery("ownerin:\"Registered Users\" status:merged", change3);
+
+    GroupDescription.Basic externalGroup = testGroupBackend.create("External Group");
+    try {
+      testGroupBackend.setMembershipsOf(
+          user2, new ListGroupMembership(ImmutableList.of(externalGroup.getGroupUUID())));
+
+      assertQuery("ownerin:\"" + "testbackend:" + externalGroup.getName() + "\"", change3, change2);
+
+      String nameOfGroupThatContainsExternalGroupAsSubgroup = "test-group-1";
+      AccountGroup.UUID uuidOfGroupThatContainsExternalGroupAsSubgroup =
+          groupOperations
+              .newGroup()
+              .name(nameOfGroupThatContainsExternalGroupAsSubgroup)
+              .addSubgroup(externalGroup.getGroupUUID())
+              .create();
+      assertQuery(
+          "ownerin:\"" + nameOfGroupThatContainsExternalGroupAsSubgroup + "\"", change3, change2);
+
+      String nameOfGroupThatContainsExternalGroupAsSubSubgroup = "test-group-2";
+      groupOperations
+          .newGroup()
+          .name(nameOfGroupThatContainsExternalGroupAsSubSubgroup)
+          .addSubgroup(uuidOfGroupThatContainsExternalGroupAsSubgroup)
+          .create();
+      assertQuery(
+          "ownerin:\"" + nameOfGroupThatContainsExternalGroupAsSubSubgroup + "\"",
+          change3,
+          change2);
+    } finally {
+      testGroupBackend.removeMembershipsOf(user2);
+      testGroupBackend.remove(externalGroup.getGroupUUID());
+    }
   }
 
   @Test
@@ -753,6 +788,37 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests {
     CurrentUser user2CurrentUser = userFactory.create(user2);
     newPatchSet(repo, change1, user2CurrentUser);
     assertQuery("uploaderin:Administrators");
+    assertQuery("uploaderin:\"Registered Users\"", change1);
+
+    GroupDescription.Basic externalGroup = testGroupBackend.create("External Group");
+    try {
+      testGroupBackend.setMembershipsOf(
+          user2, new ListGroupMembership(ImmutableList.of(externalGroup.getGroupUUID())));
+
+      assertQuery("uploaderin:\"" + "testbackend:" + externalGroup.getName() + "\"", change1);
+
+      String nameOfGroupThatContainsExternalGroupAsSubgroup = "test-group-1";
+      AccountGroup.UUID uuidOfGroupThatContainsExternalGroupAsSubgroup =
+          groupOperations
+              .newGroup()
+              .name(nameOfGroupThatContainsExternalGroupAsSubgroup)
+              .addSubgroup(externalGroup.getGroupUUID())
+              .create();
+      assertQuery("uploaderin:\"" + nameOfGroupThatContainsExternalGroupAsSubgroup + "\"", change1);
+
+      String nameOfGroupThatContainsExternalGroupAsSubSubgroup = "test-group-2";
+      groupOperations
+          .newGroup()
+          .name(nameOfGroupThatContainsExternalGroupAsSubSubgroup)
+          .addSubgroup(uuidOfGroupThatContainsExternalGroupAsSubgroup)
+          .create();
+      assertQuery(
+          "uploaderin:\"" + nameOfGroupThatContainsExternalGroupAsSubSubgroup + "\"", change1);
+
+    } finally {
+      testGroupBackend.removeMembershipsOf(user2);
+      testGroupBackend.remove(externalGroup.getGroupUUID());
+    }
   }
 
   @Test
diff --git a/javatests/com/google/gerrit/server/query/change/BUILD b/javatests/com/google/gerrit/server/query/change/BUILD
index 802bf5480f..a7226eeb1f 100644
--- a/javatests/com/google/gerrit/server/query/change/BUILD
+++ b/javatests/com/google/gerrit/server/query/change/BUILD
@@ -19,6 +19,7 @@ java_library(
     deps = [
         "//java/com/google/gerrit/acceptance:lib",
         "//java/com/google/gerrit/acceptance/config",
+        "//java/com/google/gerrit/acceptance/testsuite/group",
         "//java/com/google/gerrit/acceptance/testsuite/project",
         "//java/com/google/gerrit/common:annotations",
         "//java/com/google/gerrit/common:server",