opendev/gerrit
Code Issues Proposed changes

Disallow updates to group branches by direct push or submit

Browse Source 
While the migration of groups to NoteDb is not fully done yet we must
ensure that the group data in NoteDb stays in sync with the group data
in ReviewDb. This is why we must prevent updates to the NoteDb group
branches by direct push or by push for review + submit.

Change-Id: I9b9c43876755a1aa4c37204b0569529c37b0ad37
Signed-off-by: Edwin Kempin <ekempin@google.com>
This commit is contained in:
Edwin Kempin
2017-11-06 12:36:33 +01:00
parent c40a55128e
commit 4f742db3ec
6 changed files with 208 additions and 6 deletions
Download Patch File Download Diff File Expand all files Collapse all files

6
java/com/google/gerrit/acceptance/PushOneCommit.java
View File

@@ -313,16 +313,18 @@ public class PushOneCommit {
commitBuilder.message(subject).author(i).committer(new PersonIdent(i, testRepo.getDate()));
}
public void setParents(List<RevCommit> parents) throws Exception {
public PushOneCommit setParents(List<RevCommit> parents) throws Exception {
commitBuilder.noParents();
for (RevCommit p : parents) {
commitBuilder.parent(p);
}
return this;
}
public void setParent(RevCommit parent) throws Exception {
public PushOneCommit setParent(RevCommit parent) throws Exception {
commitBuilder.noParents();
commitBuilder.parent(parent);
return this;
}
public Result to(String ref) throws Exception {

3
java/com/google/gerrit/reviewdb/client/RefNames.java
View File

@@ -82,6 +82,9 @@ public class RefNames {
/** Preference settings for a user {@code refs/users} */
public static final String REFS_USERS = "refs/users/";
/** NoteDb ref for a group {@code refs/groups} */
public static final String REFS_GROUPS = "refs/groups/";
/** Draft inline comments of a user on a change */
public static final String REFS_DRAFT_COMMENTS = "refs/draft-comments/";

2
java/com/google/gerrit/server/config/GerritGlobalModule.java
View File

@@ -126,6 +126,7 @@ import com.google.gerrit.server.git.validators.CommitValidationListener;
import com.google.gerrit.server.git.validators.MergeValidationListener;
import com.google.gerrit.server.git.validators.MergeValidators;
import com.google.gerrit.server.git.validators.MergeValidators.AccountMergeValidator;
import com.google.gerrit.server.git.validators.MergeValidators.GroupMergeValidator;
import com.google.gerrit.server.git.validators.MergeValidators.ProjectConfigValidator;
import com.google.gerrit.server.git.validators.OnSubmitValidationListener;
import com.google.gerrit.server.git.validators.OnSubmitValidators;
@@ -395,6 +396,7 @@ public class GerritGlobalModule extends FactoryModule {
factory(AbandonOp.Factory.class);
factory(AccountMergeValidator.Factory.class);
factory(GroupMergeValidator.Factory.class);
factory(RefOperationValidators.Factory.class);
factory(OnSubmitValidators.Factory.class);
factory(MergeValidators.Factory.class);

35
java/com/google/gerrit/server/git/validators/CommitValidators.java
View File

@@ -133,7 +133,8 @@ public class CommitValidators {
new BannedCommitsValidator(rejectCommits),
new PluginCommitValidationListener(pluginValidators),
new ExternalIdUpdateListener(allUsers, externalIdsConsistencyChecker),
new AccountCommitValidator(allUsers, accountValidator)));
new AccountCommitValidator(allUsers, accountValidator),
new GroupCommitValidator(allUsers)));
}
public CommitValidators forGerritCommits(
@@ -158,7 +159,8 @@ public class CommitValidators {
new ConfigValidator(branch, user, rw, allUsers),
new PluginCommitValidationListener(pluginValidators),
new ExternalIdUpdateListener(allUsers, externalIdsConsistencyChecker),
new AccountCommitValidator(allUsers, accountValidator)));
new AccountCommitValidator(allUsers, accountValidator),
new GroupCommitValidator(allUsers)));
}
public CommitValidators forMergedCommits(PermissionBackend.ForRef perm, IdentifiedUser user) {
@@ -758,6 +760,35 @@ public class CommitValidators {
}
}
/** Rejects updates to group branches. */
public static class GroupCommitValidator implements CommitValidationListener {
private final AllUsersName allUsers;
public GroupCommitValidator(AllUsersName allUsers) {
this.allUsers = allUsers;
}
@Override
public List<CommitValidationMessage> onCommitReceived(CommitReceivedEvent receiveEvent)
throws CommitValidationException {
// Groups are stored inside 'refs/groups/' refs inside the 'All-Users' repository.
if (!allUsers.equals(receiveEvent.project.getNameKey())) {
return Collections.emptyList();
}
if (receiveEvent.command.getRefName().startsWith(MagicBranch.NEW_CHANGE)) {
// no validation on push for review, will be checked on submit by
// MergeValidators.GroupMergeValidator
return Collections.emptyList();
}
if (receiveEvent.command.getRefName().startsWith(RefNames.REFS_GROUPS)) {
throw new CommitValidationException("group update not allowed");
}
return Collections.emptyList();
}
}
private static CommitValidationMessage invalidEmail(
RevCommit c,
String type,

39
java/com/google/gerrit/server/git/validators/MergeValidators.java
View File

@@ -58,6 +58,7 @@ public class MergeValidators {
private final DynamicSet<MergeValidationListener> mergeValidationListeners;
private final ProjectConfigValidator.Factory projectConfigValidatorFactory;
private final AccountMergeValidator.Factory accountValidatorFactory;
private final GroupMergeValidator.Factory groupValidatorFactory;
public interface Factory {
MergeValidators create();
@@ -67,10 +68,12 @@ public class MergeValidators {
MergeValidators(
DynamicSet<MergeValidationListener> mergeValidationListeners,
ProjectConfigValidator.Factory projectConfigValidatorFactory,
AccountMergeValidator.Factory accountValidatorFactory) {
AccountMergeValidator.Factory accountValidatorFactory,
GroupMergeValidator.Factory groupValidatorFactory) {
this.mergeValidationListeners = mergeValidationListeners;
this.projectConfigValidatorFactory = projectConfigValidatorFactory;
this.accountValidatorFactory = accountValidatorFactory;
this.groupValidatorFactory = groupValidatorFactory;
}
public void validatePreMerge(
@@ -85,7 +88,8 @@ public class MergeValidators {
ImmutableList.of(
new PluginMergeValidationListener(mergeValidationListeners),
projectConfigValidatorFactory.create(),
accountValidatorFactory.create());
accountValidatorFactory.create(),
groupValidatorFactory.create());
for (MergeValidationListener validator : validators) {
validator.onPreMerge(repo, commit, destProject, destBranch, patchSetId, caller);
@@ -284,4 +288,35 @@ public class MergeValidators {
}
}
}
public static class GroupMergeValidator implements MergeValidationListener {
public interface Factory {
GroupMergeValidator create();
}
private final AllUsersName allUsersName;
@Inject
public GroupMergeValidator(AllUsersName allUsersName) {
this.allUsersName = allUsersName;
}
@Override
public void onPreMerge(
Repository repo,
CodeReviewCommit commit,
ProjectState destProject,
Branch.NameKey destBranch,
PatchSet.Id patchSetId,
IdentifiedUser caller)
throws MergeValidationException {
// Groups are stored inside 'refs/groups/' refs inside the 'All-Users' repository.
if (!allUsersName.equals(destProject.getNameKey())
|| !destBranch.get().startsWith(RefNames.REFS_GROUPS)) {
return;
}
throw new MergeValidationException("group update not allowed");
}
}
}

129
javatests/com/google/gerrit/acceptance/api/group/GroupsIT.java
View File

@@ -15,18 +15,24 @@
package com.google.gerrit.acceptance.api.group;
import static com.google.common.truth.Truth.assertThat;
import static com.google.gerrit.acceptance.GitUtil.fetch;
import static com.google.gerrit.acceptance.api.group.GroupAssert.assertGroupInfo;
import static com.google.gerrit.acceptance.rest.account.AccountAssert.assertAccountInfos;
import static com.google.gerrit.server.group.SystemGroupBackend.ANONYMOUS_USERS;
import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS;
import static java.util.stream.Collectors.toList;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.Iterables;
import com.google.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.acceptance.GerritConfig;
import com.google.gerrit.acceptance.NoHttpd;
import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.acceptance.TestAccount;
import com.google.gerrit.common.TimeUtil;
import com.google.gerrit.common.data.GroupReference;
import com.google.gerrit.common.data.Permission;
import com.google.gerrit.extensions.api.changes.ReviewInput;
import com.google.gerrit.extensions.api.groups.GroupApi;
import com.google.gerrit.extensions.api.groups.GroupInput;
import com.google.gerrit.extensions.api.groups.Groups.ListRequest;
@@ -45,14 +51,19 @@ import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
import com.google.gerrit.extensions.restapi.Url;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.AccountGroup;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.client.RefNames;
import com.google.gerrit.server.account.GroupIncludeCache;
import com.google.gerrit.server.config.AllUsersName;
import com.google.gerrit.server.group.InternalGroup;
import com.google.gerrit.server.group.ServerInitiated;
import com.google.gerrit.server.group.SystemGroupBackend;
import com.google.gerrit.server.group.db.Groups;
import com.google.gerrit.server.group.db.GroupsUpdate;
import com.google.gerrit.server.util.MagicBranch;
import com.google.inject.Inject;
import com.google.inject.Provider;
import java.io.IOException;
import java.sql.Timestamp;
import java.util.ArrayList;
import java.util.Arrays;
@@ -60,6 +71,16 @@ import java.util.Collection;
import java.util.Collections;
import java.util.List;
import java.util.Map;
import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
import org.eclipse.jgit.junit.TestRepository;
import org.eclipse.jgit.lib.CommitBuilder;
import org.eclipse.jgit.lib.Constants;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.ObjectInserter;
import org.eclipse.jgit.lib.PersonIdent;
import org.eclipse.jgit.lib.RefUpdate;
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.revwalk.RevWalk;
import org.junit.Test;
@NoHttpd
@@ -67,6 +88,7 @@ public class GroupsIT extends AbstractDaemonTest {
@Inject @ServerInitiated private Provider<GroupsUpdate> groupsUpdateProvider;
@Inject private Groups groups;
@Inject private GroupIncludeCache groupIncludeCache;
@Inject private AllUsersName allUsers;
@Test
public void systemGroupCanBeRetrievedFromIndex() throws Exception {
@@ -737,6 +759,113 @@ public class GroupsIT extends AbstractDaemonTest {
gApi.groups().id(group.id).index();
}
@Test
public void pushToGroupBranchIsRejectedForAllUsersRepo() throws Exception {
pushToGroupBranch(allUsers, "group update not allowed");
}
@Test
public void pushToGroupBranchForNonAllUsersRepo() throws Exception {
pushToGroupBranch(project, null);
}
private void pushToGroupBranch(Project.NameKey project, String expectedError) throws Exception {
grant(project, RefNames.REFS_GROUPS + "*", Permission.CREATE, false, REGISTERED_USERS);
grant(project, RefNames.REFS_GROUPS + "*", Permission.PUSH, false, REGISTERED_USERS);
TestRepository<InMemoryRepository> repo = cloneProject(project);
// create new branch
PushOneCommit.Result r =
pushFactory
.create(
db, admin.getIdent(), repo, "Update group config", "group.config", "some content")
.setParents(ImmutableList.of())
.to(RefNames.REFS_GROUPS + name("foo"));
if (expectedError != null) {
r.assertErrorStatus(expectedError);
} else {
r.assertOkStatus();
}
// update existing branch
String groupRefName = RefNames.REFS_GROUPS + name("bar");
createGroupBranch(project, groupRefName);
fetch(repo, groupRefName + ":groupRef");
repo.reset("groupRef");
r =
pushFactory
.create(
db, admin.getIdent(), repo, "Update group config", "group.config", "some content")
.to(groupRefName);
if (expectedError != null) {
r.assertErrorStatus(expectedError);
} else {
r.assertOkStatus();
}
}
@Test
public void pushToGroupBranchForReviewForAllUsersRepoIsRejectedOnSubmit() throws Exception {
pushToGroupBranchForReviewAndSubmit(allUsers, "group update not allowed");
}
@Test
public void pushToGroupBranchForReviewForNonAllUsersRepoAndSubmit() throws Exception {
pushToGroupBranchForReviewAndSubmit(project, null);
}
private void pushToGroupBranchForReviewAndSubmit(Project.NameKey project, String expectedError)
throws Exception {
grantLabel(
"Code-Review", -2, 2, project, RefNames.REFS_GROUPS + "*", false, REGISTERED_USERS, false);
grant(project, RefNames.REFS_GROUPS + "*", Permission.SUBMIT, false, REGISTERED_USERS);
String groupRefName = RefNames.REFS_GROUPS + name("foo");
createGroupBranch(project, groupRefName);
TestRepository<InMemoryRepository> repo = cloneProject(project);
fetch(repo, groupRefName + ":groupRef");
repo.reset("groupRef");
PushOneCommit.Result r =
pushFactory
.create(
db, admin.getIdent(), repo, "Update group config", "group.config", "some content")
.to(MagicBranch.NEW_CHANGE + groupRefName);
r.assertOkStatus();
assertThat(r.getChange().change().getDest().get()).isEqualTo(groupRefName);
gApi.changes().id(r.getChangeId()).current().review(ReviewInput.approve());
if (expectedError != null) {
exception.expect(ResourceConflictException.class);
exception.expectMessage("group update not allowed");
}
gApi.changes().id(r.getChangeId()).current().submit();
}
private void createGroupBranch(Project.NameKey project, String ref) throws IOException {
try (Repository r = repoManager.openRepository(project);
ObjectInserter oi = r.newObjectInserter();
RevWalk rw = new RevWalk(r)) {
ObjectId emptyTree = oi.insert(Constants.OBJ_TREE, new byte[] {});
PersonIdent ident = new PersonIdent(serverIdent.get(), TimeUtil.nowTs());
CommitBuilder cb = new CommitBuilder();
cb.setTreeId(emptyTree);
cb.setCommitter(ident);
cb.setAuthor(ident);
cb.setMessage("Create group");
ObjectId emptyCommit = oi.insert(cb);
oi.flush();
RefUpdate updateRef = r.updateRef(ref);
updateRef.setExpectedOldObjectId(ObjectId.zeroId());
updateRef.setNewObjectId(emptyCommit);
assertThat(updateRef.update(rw)).isEqualTo(RefUpdate.Result.NEW);
}
}
private void assertAuditEvent(
GroupAuditEventInfo info,
Type expectedType,