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
diff --git a/java/com/google/gerrit/acceptance/BUILD b/java/com/google/gerrit/acceptance/BUILD
index 9d237af..3c7ec2b 100644
--- a/java/com/google/gerrit/acceptance/BUILD
+++ b/java/com/google/gerrit/acceptance/BUILD
@@ -63,6 +63,7 @@
"//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 0000000..d4f1175
--- /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 b0e270c..2f063f6 100644
--- a/java/com/google/gerrit/server/group/testing/TestGroupBackend.java
+++ b/java/com/google/gerrit/server/group/testing/TestGroupBackend.java
@@ -103,6 +103,13 @@
memberships.put(user, membership);
}
+ /**
+ * Remove a 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 b8c02af..03cdfaa 100644
--- a/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
+++ b/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
@@ -1291,7 +1291,8 @@
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);
}
@@ -1314,7 +1315,8 @@
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);
}
@@ -1687,6 +1689,22 @@
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 861fa00..e5234fe 100644
--- a/java/com/google/gerrit/testing/BUILD
+++ b/java/com/google/gerrit/testing/BUILD
@@ -15,6 +15,7 @@
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 936b448..e86fd09 100644
--- a/java/com/google/gerrit/testing/InMemoryModule.java
+++ b/java/com/google/gerrit/testing/InMemoryModule.java
@@ -21,6 +21,8 @@
import com.google.common.base.Strings;
import com.google.common.collect.ImmutableSet;
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;
@@ -279,6 +281,7 @@
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 75c90f2..fe451c4 100644
--- a/javatests/com/google/gerrit/acceptance/BUILD
+++ b/javatests/com/google/gerrit/acceptance/BUILD
@@ -5,6 +5,7 @@
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 fbf9c87..153c62c 100644
--- a/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
+++ b/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
@@ -51,6 +51,7 @@
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.common.RawInputUtil;
@@ -58,6 +59,7 @@
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;
@@ -204,6 +206,7 @@
@Inject protected AuthRequest.Factory authRequestFactory;
@Inject protected ExternalIdFactory externalIdFactory;
@Inject protected ProjectOperations projectOperations;
+ @Inject protected GroupOperations groupOperations;
@Inject private ProjectConfig.Factory projectConfigFactory;
@@ -783,6 +786,41 @@
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:\"" + TestGroupBackend.PREFIX + 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
@@ -799,6 +837,37 @@
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:\"" + TestGroupBackend.PREFIX + 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 32a646e..57a3c4b 100644
--- a/javatests/com/google/gerrit/server/query/change/BUILD
+++ b/javatests/com/google/gerrit/server/query/change/BUILD
@@ -19,6 +19,7 @@
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",
