Allow uploading changes to group refs

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 maintain task config [1] in group refs
which allows users to collaborate and review changes on group specific
task configs.

[1] https://gerrit.googlesource.com/plugins/task/+/refs/heads/stable-3.5/src/main/resources/Documentation/task.md

Release-Notes: users can now upload changes to group refs except changes to group files
Change-Id: I993f7a2d9e2db73744bb13313d058917322768d1

3 files changed, 46 insertions, 12 deletions

diff --git a/java/com/google/gerrit/server/git/validators/MergeValidators.java b/java/com/google/gerrit/server/git/validators/MergeValidators.java
index 6b145cac4a..325de5b99c 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.PluginConfig;
 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 @@ public class MergeValidators {
     }
 
     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,30 @@ public class MergeValidators {
         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 maintain task config in group refs which
+      // allows users to collaborate and review changes on group specific task configs.
+      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 c18718628e..a00d5299b9 100644
--- a/java/com/google/gerrit/server/group/db/GroupConfig.java
+++ b/java/com/google/gerrit/server/group/db/GroupConfig.java
@@ -19,7 +19,6 @@ import static com.google.common.collect.ImmutableSet.toImmutableSet;
 import static java.util.Objects.requireNonNull;
 import static java.util.stream.Collectors.joining;
 
-import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Splitter;
 import com.google.common.base.Strings;
 import com.google.common.collect.ImmutableSet;
@@ -89,9 +88,9 @@ import org.eclipse.jgit.revwalk.RevSort;
  * doesn't have any members or subgroups.
  */
 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";
+  public static final String GROUP_CONFIG_FILE = "group.config";
+  public static final String MEMBERS_FILE = "members";
+  public static final String SUBGROUPS_FILE = "subgroups";
   private static final Pattern LINE_SEPARATOR_PATTERN = Pattern.compile("\\R");
 
   /**
diff --git a/javatests/com/google/gerrit/acceptance/api/group/GroupsIT.java b/javatests/com/google/gerrit/acceptance/api/group/GroupsIT.java
index 4cbc36b581..61b7c0ccb9 100644
--- a/javatests/com/google/gerrit/acceptance/api/group/GroupsIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/group/GroupsIT.java
@@ -1265,16 +1265,24 @@ public class GroupsIT extends AbstractDaemonTest {
   }
 
   @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
@@ -1554,7 +1562,8 @@ public class GroupsIT extends AbstractDaemonTest {
   }
 
   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()
@@ -1572,7 +1581,7 @@ public class GroupsIT extends AbstractDaemonTest {
 
     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);
@@ -1581,7 +1590,7 @@ public class GroupsIT extends AbstractDaemonTest {
     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();
     }