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
diff --git a/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java b/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
index 5b6f044..c090aa0 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 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 @@
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 @@
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 cbf5df4..f50cd04 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.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 @@
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 @@
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 @@
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 @@
}
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 @@
}
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 74929ad..bda17da 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 @@
// 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 @@
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 @@
// 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);
