diff options
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(); } |