Merge "project-configuration: Fix old UI references" into stable-3.5
diff --git a/java/com/google/gerrit/server/git/validators/MergeValidators.java b/java/com/google/gerrit/server/git/validators/MergeValidators.java
index 6b145ca..325de5b 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,30 @@
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 c187186..a00d529 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 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 @@
* 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 4cbc36b..61b7c0c 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 @@
}
@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 @@
}
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 @@
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 @@
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();
}
