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)
diff --git a/java/com/google/gerrit/acceptance/BUILD b/java/com/google/gerrit/acceptance/BUILD
index d6dff8f..d03d0310c 100644
--- a/java/com/google/gerrit/acceptance/BUILD
+++ b/java/com/google/gerrit/acceptance/BUILD
@@ -65,6 +65,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 12d8c93..2d9c798 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 @@
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 e36dbfc..0ffdb1c 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 @@
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 @@
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 @@
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 a7a1795..05987bb 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 b00cadb..49edd4c 100644
--- a/java/com/google/gerrit/testing/InMemoryModule.java
+++ b/java/com/google/gerrit/testing/InMemoryModule.java
@@ -20,6 +20,8 @@
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 @@
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 cbdc138..74929ad 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.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 @@
@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 @@
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 @@
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 802bf54..a7226ee 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",
