Support named destinations on group refs
By adding support for named destinations on group refs allows users
to collaborate and review changes on centrally maintained named
destinations. This is very helpful for Teams which require maintaining
named destinations for CI systems in a common location.
Also, only block submit to group refs when group files are being
updated, i.e. group.config, subgroups and members. This allows the
users to submit changes made to named destinations on group refs.
Update documentation and tests accordingly.
Release-Notes: Named destinations are now supported on group refs
Change-Id: I04ae140335b8906bc500b234effff3b398b36686
diff --git a/Documentation/config-groups.txt b/Documentation/config-groups.txt
index 4abb223..01e6141 100644
--- a/Documentation/config-groups.txt
+++ b/Documentation/config-groups.txt
@@ -93,8 +93,10 @@
== Pushing to group refs
-Validation on push for changes to the group ref is not implemented, so
-pushes are rejected. Pushes that bypass Gerrit should be avoided since
+Users can push changes to `refs/for/refs/groups/*`, but submit is rejected
+for changes which update group files (i.e. group.config, members, subgroups).
+It is possible for users to upload and submit changes on the named destination
+files in a group ref. Pushes that bypass Gerrit should be avoided since
the names, IDs and UUIDs must be internally consistent between all the
branches involved. In addition, group references should not be created
or deleted manually either. If you attempt any of these actions
diff --git a/Documentation/user-named-destinations.txt b/Documentation/user-named-destinations.txt
index a1ab258..cee562b 100644
--- a/Documentation/user-named-destinations.txt
+++ b/Documentation/user-named-destinations.txt
@@ -1,15 +1,19 @@
= Gerrit Code Review - Named Destinations
-[[user-named-destinations]]
-== User Named Destinations
-It is possible to define named destination sets on a user level.
+[[user-or-group-named-destinations]]
+== User Or Group Named Destinations
+It is possible to define named destination sets on a user or group level.
To do this, define the named destination sets in files named after
each destination set in the `destinations` directory of the user's
-account ref in the `All-Users` project. The user's account ref is
-based on the user's account id which is an integer. The account
-refs are sharded by the last two digits (`+nn+`) in the refname,
-leading to refs of the format `+refs/users/nn/accountid+`. The
-user's destination files are a 2 column tab delimited file. Each
+or group's account ref in the `All-Users` project. The user's account ref is
+based on the user's account id which is an integer. The user account refs
+are sharded by the last two digits (`+nn+`) in the refname, leading to refs
+of the format `+refs/users/nn/accountid+`. Similarly, the group's ref is
+based on the group id which is a UUID. The group refs are sharded
+by the first 2 characters of the group UUID, leading to a refs of the
+format `+refs/groups/cc/groupid+`.
+
+The destination files are a 2 column tab delimited file. Each
row in a destination file represents a single destination in the
named set. The left column represents the ref of the destination,
and the right column represents the project of the destination.
diff --git a/Documentation/user-search.txt b/Documentation/user-search.txt
index 67b8d75..b0c4a53 100644
--- a/Documentation/user-search.txt
+++ b/Documentation/user-search.txt
@@ -128,11 +128,13 @@
as a change number such as 15183, or a Change-Id from the Change-Id footer.
[[destination]]
-destination:'[name=]NAME[,user=USER]'::
+destination:'[name=]NAME[,user=USER|,group=GROUP]'::
+
-Changes which match the specified USER's destination named 'NAME'. If 'USER'
-is unspecified, the current user is used. The named destinations can be
-publicly accessible by other users.
+Changes which match the specified USER's or GROUP's destination named 'NAME'.
+If 'USER' is unspecified, the current user is used. The named destinations can
+be publicly accessible by other users.
+The value may be wrapped in double quotes to include spaces. For example,
+`destination:"myreviews,group=My Group"`
(see link:user-named-destinations.html[Named Destinations]).
[[owner]]
diff --git a/java/com/google/gerrit/server/account/VersionedAccountDestinations.java b/java/com/google/gerrit/server/account/VersionedAccountDestinations.java
index f1cf9fe..41a02a9 100644
--- a/java/com/google/gerrit/server/account/VersionedAccountDestinations.java
+++ b/java/com/google/gerrit/server/account/VersionedAccountDestinations.java
@@ -15,20 +15,19 @@
package com.google.gerrit.server.account;
import com.google.common.flogger.FluentLogger;
-import com.google.gerrit.entities.Account;
-import com.google.gerrit.entities.RefNames;
+import com.google.gerrit.entities.BranchNameKey;
import com.google.gerrit.server.git.meta.VersionedMetaData;
import java.io.IOException;
import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.lib.CommitBuilder;
import org.eclipse.jgit.lib.FileMode;
-/** User configured named destinations. */
+/** User or Group configured named destinations. */
public class VersionedAccountDestinations extends VersionedMetaData {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
- public static VersionedAccountDestinations forUser(Account.Id id) {
- return new VersionedAccountDestinations(RefNames.refsUsers(id));
+ public static VersionedAccountDestinations forBranch(BranchNameKey branch) {
+ return new VersionedAccountDestinations(branch.branch());
}
private final String ref;
diff --git a/java/com/google/gerrit/server/git/validators/MergeValidators.java b/java/com/google/gerrit/server/git/validators/MergeValidators.java
index 40ce671..514dee1 100644
--- a/java/com/google/gerrit/server/git/validators/MergeValidators.java
+++ b/java/com/google/gerrit/server/git/validators/MergeValidators.java
@@ -36,6 +36,7 @@
import com.google.gerrit.server.config.ProjectConfigEntry;
import com.google.gerrit.server.git.CodeReviewCommit;
import com.google.gerrit.server.git.CodeReviewCommit.CodeReviewRevWalk;
+import com.google.gerrit.server.group.db.GroupConfig;
import com.google.gerrit.server.permissions.GlobalPermission;
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
@@ -343,10 +344,12 @@
}
private final AllUsersName allUsersName;
+ private final ChangeData.Factory changeDataFactory;
@Inject
- public GroupMergeValidator(AllUsersName allUsersName) {
+ public GroupMergeValidator(AllUsersName allUsersName, ChangeData.Factory changeDataFactory) {
this.allUsersName = allUsersName;
+ this.changeDataFactory = changeDataFactory;
}
@Override
@@ -365,7 +368,29 @@
return;
}
- throw new MergeValidationException("group update not allowed");
+ // Update to group files is not supported because there are no validations
+ // on the changes being done to these files, without which the group data
+ // might get corrupted. Thus don't allow merges into All-Users group refs
+ // which updates group files (i.e., group.config, members and subgroups).
+ // But it is still useful to allow users to update files apart from group
+ // files. For example, users can upload named destinations into group refs.
+ ChangeData cd =
+ changeDataFactory.create(destProject.getProject().getNameKey(), patchSetId.changeId());
+ try {
+ if (cd.currentFilePaths().contains(GroupConfig.GROUP_CONFIG_FILE)
+ || cd.currentFilePaths().contains(GroupConfig.MEMBERS_FILE)
+ || cd.currentFilePaths().contains(GroupConfig.SUBGROUPS_FILE)) {
+ throw new MergeValidationException(
+ String.format(
+ "update to group files (%s, %s, %s) not allowed",
+ GroupConfig.GROUP_CONFIG_FILE,
+ GroupConfig.MEMBERS_FILE,
+ GroupConfig.SUBGROUPS_FILE));
+ }
+ } catch (StorageException e) {
+ logger.atSevere().withCause(e).log("Cannot validate group update");
+ throw new MergeValidationException("group validation unavailable", e);
+ }
}
}
diff --git a/java/com/google/gerrit/server/group/db/GroupConfig.java b/java/com/google/gerrit/server/group/db/GroupConfig.java
index 4f2c049..3470a6c 100644
--- a/java/com/google/gerrit/server/group/db/GroupConfig.java
+++ b/java/com/google/gerrit/server/group/db/GroupConfig.java
@@ -90,8 +90,8 @@
*/
public class GroupConfig extends VersionedMetaData {
@VisibleForTesting public static final String GROUP_CONFIG_FILE = "group.config";
- @VisibleForTesting static final String MEMBERS_FILE = "members";
- @VisibleForTesting static final String SUBGROUPS_FILE = "subgroups";
+ @VisibleForTesting public static final String MEMBERS_FILE = "members";
+ @VisibleForTesting public static final String SUBGROUPS_FILE = "subgroups";
private static final Pattern LINE_SEPARATOR_PATTERN = Pattern.compile("\\R");
/**
diff --git a/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java b/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
index 711b4ec..60378b1 100644
--- a/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
+++ b/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
@@ -36,6 +36,7 @@
import com.google.gerrit.entities.Address;
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.PatchSet;
import com.google.gerrit.entities.Project;
@@ -500,7 +501,7 @@
protected final Arguments args;
protected Map<String, String> hasOperandAliases = Collections.emptyMap();
- private final Map<Account.Id, DestinationList> destinationListByAccount = new HashMap<>();
+ private final Map<BranchNameKey, DestinationList> destinationListByBranch = new HashMap<>();
private final Map<Account.Id, QueryList> queryListByAccount = new HashMap<>();
private static final Splitter RULE_SPLITTER = Splitter.on("=");
@@ -1486,11 +1487,16 @@
@Operator
public Predicate<ChangeData> destination(String value) throws QueryParseException {
- // [name=]<name>[,user=<user>] || [user=<user>,][name=]<name>
+ // [name=]<name>[,user=<user>|,group=<group>] || [group=<group>,|user=<user>,][name=]<name>
PredicateArgs inputArgs = new PredicateArgs(value);
String name = null;
Account.Id account = null;
+ GroupDescription.Internal group = null;
+ if (inputArgs.keyValue.containsKey(ARG_ID_USER)
+ && inputArgs.keyValue.containsKey(ARG_ID_GROUP)) {
+ throw new QueryParseException("User and group arguments are mutually exclusive");
+ }
// [name=]<name>
if (inputArgs.keyValue.containsKey(ARG_ID_NAME)) {
name = inputArgs.keyValue.get(ARG_ID_NAME).value();
@@ -1514,7 +1520,23 @@
account = self();
}
- Set<BranchNameKey> destinations = getDestinationList(account).getDestinations(name);
+ // [,group=<group>]
+ if (inputArgs.keyValue.containsKey(ARG_ID_GROUP)) {
+ AccountGroup.UUID groupId =
+ parseGroup(inputArgs.keyValue.get(ARG_ID_GROUP).value()).getUUID();
+ GroupDescription.Basic backendGroup = args.groupBackend.get(groupId);
+ if (!(backendGroup instanceof GroupDescription.Internal)) {
+ throw error(backendGroup.getName() + " is not an Internal group");
+ }
+ group = (GroupDescription.Internal) backendGroup;
+ }
+
+ BranchNameKey branch = BranchNameKey.create(args.allUsersName, RefNames.refsUsers(account));
+ if (group != null) {
+ branch = BranchNameKey.create(args.allUsersName, RefNames.refsGroups(group.getGroupUUID()));
+ }
+ Set<BranchNameKey> destinations = getDestinationList(branch).getDestinations(name);
+
if (destinations != null && !destinations.isEmpty()) {
return new BranchSetIndexPredicate(FIELD_DESTINATION + ":" + value, destinations);
}
@@ -1527,19 +1549,19 @@
throw new QueryParseException("Unknown named destination: " + name);
}
- protected DestinationList getDestinationList(Account.Id account)
+ protected DestinationList getDestinationList(BranchNameKey branch)
throws ConfigInvalidException, RepositoryNotFoundException, IOException {
- DestinationList dl = destinationListByAccount.get(account);
+ DestinationList dl = destinationListByBranch.get(branch);
if (dl == null) {
- dl = loadDestinationList(account);
- destinationListByAccount.put(account, dl);
+ dl = loadDestinationList(branch);
+ destinationListByBranch.put(branch, dl);
}
return dl;
}
- protected DestinationList loadDestinationList(Account.Id account)
+ protected DestinationList loadDestinationList(BranchNameKey branch)
throws ConfigInvalidException, RepositoryNotFoundException, IOException {
- VersionedAccountDestinations d = VersionedAccountDestinations.forUser(account);
+ VersionedAccountDestinations d = VersionedAccountDestinations.forBranch(branch);
try (Repository git = args.repoManager.openRepository(args.allUsersName)) {
d.load(args.allUsersName, git);
}
diff --git a/javatests/com/google/gerrit/acceptance/api/group/GroupsIT.java b/javatests/com/google/gerrit/acceptance/api/group/GroupsIT.java
index 9456a31..6dbbe9a 100644
--- a/javatests/com/google/gerrit/acceptance/api/group/GroupsIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/group/GroupsIT.java
@@ -1285,16 +1285,24 @@
}
@Test
- public void pushToGroupBranchForReviewForAllUsersRepoIsRejectedOnSubmit() throws Throwable {
+ public void pushToGroupBranchForReviewForAllUsersRepoIsRejectedOnSubmitForGroupFiles()
+ throws Throwable {
+ String error = "update to group files (group.config, members, subgroups) not allowed";
pushToGroupBranchForReviewAndSubmit(
- allUsers, RefNames.refsGroups(adminGroupUuid()), "group update not allowed");
+ allUsers, RefNames.refsGroups(adminGroupUuid()), "group.config", error);
+ pushToGroupBranchForReviewAndSubmit(
+ allUsers, RefNames.refsGroups(adminGroupUuid()), "members", error);
+ pushToGroupBranchForReviewAndSubmit(
+ allUsers, RefNames.refsGroups(adminGroupUuid()), "subgroups", error);
+ pushToGroupBranchForReviewAndSubmit(
+ allUsers, RefNames.refsGroups(adminGroupUuid()), "destinations/myreviews", null);
}
@Test
public void pushToGroupBranchForReviewForNonAllUsersRepoAndSubmit() throws Throwable {
String groupRef = RefNames.refsGroups(adminGroupUuid());
createBranch(project, groupRef);
- pushToGroupBranchForReviewAndSubmit(project, groupRef, null);
+ pushToGroupBranchForReviewAndSubmit(project, groupRef, "group.config", null);
}
@Test
@@ -1576,7 +1584,8 @@
}
private void pushToGroupBranchForReviewAndSubmit(
- Project.NameKey project, String groupRef, String expectedError) throws Throwable {
+ Project.NameKey project, String groupRef, String fileName, String expectedError)
+ throws Throwable {
projectOperations
.project(project)
.forUpdate()
@@ -1594,7 +1603,7 @@
PushOneCommit.Result r =
pushFactory
- .create(admin.newIdent(), repo, "Update group config", "group.config", "some content")
+ .create(admin.newIdent(), repo, "Update group config", fileName, "some content")
.to(MagicBranch.NEW_CHANGE + groupRef);
r.assertOkStatus();
assertThat(r.getChange().change().getDest().branch()).isEqualTo(groupRef);
@@ -1603,7 +1612,7 @@
ThrowingRunnable submit = () -> gApi.changes().id(r.getChangeId()).current().submit();
if (expectedError != null) {
Throwable thrown = assertThrows(ResourceConflictException.class, submit);
- assertThat(thrown).hasMessageThat().contains("group update not allowed");
+ assertThat(thrown).hasMessageThat().contains(expectedError);
} else {
submit.run();
}
diff --git a/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java b/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
index 8fb9a77..693bb47 100644
--- a/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
+++ b/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
@@ -3853,7 +3853,7 @@
@GerritConfig(name = "accounts.visibility", value = "NONE")
@Test
- public void userDestination() throws Exception {
+ public void namedDestination() throws Exception {
createProject("repo1");
Change change1 = insert("repo1", newChange("repo1"));
createProject("repo2");
@@ -3863,6 +3863,8 @@
.hasMessageThat()
.isEqualTo("Unknown named destination: foo");
+ String group = "test-group";
+ AccountGroup.UUID groupId = groupOperations.newGroup().name(group).create();
Account.Id anotherUserId =
accountManager.authenticate(authRequestFactory.createForUser("anotheruser")).getAccountId();
String destination1 = "refs/heads/master\trepo1";
@@ -3906,6 +3908,13 @@
Ref anotherUserRef = allUsers.getRepository().exactRef(anotherRefsUsers);
assertThat(userRef).isNotNull();
assertThat(anotherUserRef).isNotNull();
+
+ String groupRef = RefNames.refsGroups(groupId);
+ allUsers.branch(groupRef).commit().add("destinations/destination1", destination1).create();
+ allUsers.branch(groupRef).commit().add("destinations/destination2", destination2).create();
+ allUsers.branch(groupRef).commit().add("destinations/destination3", destination3).create();
+ allUsers.branch(groupRef).commit().add("destinations/destination4", destination4).create();
+ assertThat(allUsers.getRepository().exactRef(groupRef)).isNotNull();
}
assertQuery("destination:destination1", change1);
@@ -3931,6 +3940,24 @@
assertThatQueryException("destination:destination3,user=" + userId)
.hasMessageThat()
.isEqualTo(String.format("Account '%s' not found", userId));
+
+ // Group destinations
+ requestContext.setContext(newRequestContext(userId));
+ assertThatQueryException("destination:non-existent-dest,group=" + group)
+ .hasMessageThat()
+ .isEqualTo("Unknown named destination: non-existent-dest");
+ assertThatQueryException("destination:destination1,group=non-existent-group")
+ .hasMessageThat()
+ .isEqualTo("Group non-existent-group not found");
+ assertThatQueryException("destination:destination1,group=" + group + ",user=" + userId)
+ .hasMessageThat()
+ .isEqualTo("User and group arguments are mutually exclusive");
+
+ assertQuery("destination:destination1,group=" + group, change1);
+ assertQuery("destination:name=destination1,group=" + group, change1);
+ assertQuery("destination:group=" + group + ",destination2", change2);
+ assertQuery("destination:group=" + group + ",name=destination3", change2, change1);
+ assertQuery("destination:destination4,group=" + group);
}
@GerritConfig(name = "accounts.visibility", value = "NONE")
