opendev/gerrit
Code Issues Proposed changes

Revert "Prevent mutation of permissions on refs/groups through pushes"

Browse Source 
As discussed in change Id18851f503 group owners are no longer defined by
permissions on refs/groups and hence we don't need to prevent
modifications to permissions on this namespace.

This reverts commit 22f6c58aa0.

Change-Id: I2e7d4450c9db14e789e8e9a0d795ae8a66ee83b6
Signed-off-by: Edwin Kempin <ekempin@google.com>
This commit is contained in:
Edwin Kempin
2017-11-13 11:38:59 +01:00
parent 0a9b72de6a
commit 36f971b004
6 changed files with 9 additions and 136 deletions
Download Patch File Download Diff File Expand all files Collapse all files

3
java/com/google/gerrit/server/change/ChangeInserter.java
View File

@@ -569,8 +569,7 @@ public class ChangeInserter implements InsertChangeOp {
new Branch.NameKey(ctx.getProject(), refName),
ctx.getIdentifiedUser(),
new NoSshInfo(),
ctx.getRevWalk(),
projectState)
ctx.getRevWalk())
.validate(event);
}
} catch (CommitValidationException e) {

3
java/com/google/gerrit/server/change/PatchSetInserter.java
View File

@@ -344,8 +344,7 @@ public class PatchSetInserter implements BatchUpdateOp {
origNotes.getChange().getDest(),
ctx.getIdentifiedUser(),
new NoSshInfo(),
ctx.getRevWalk(),
projectCache.checkedGet(ctx.getProject()))
ctx.getRevWalk())
.validate(event);
} catch (CommitValidationException e) {
throw new ResourceConflictException(e.getFullMessage());

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

@@ -21,11 +21,9 @@ import static java.util.stream.Collectors.toList;
import com.google.common.base.CharMatcher;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.Sets;
import com.google.gerrit.common.FooterConstants;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.common.PageLinks;
import com.google.gerrit.common.data.AccessSection;
import com.google.gerrit.extensions.api.config.ConsistencyCheckInfo.ConsistencyProblemInfo;
import com.google.gerrit.extensions.registration.DynamicSet;
import com.google.gerrit.extensions.restapi.AuthException;
@@ -60,9 +58,7 @@ import java.net.MalformedURLException;
import java.net.URL;
import java.util.ArrayList;
import java.util.Collections;
import java.util.HashSet;
import java.util.List;
import java.util.Set;
import java.util.regex.Pattern;
import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.lib.Config;
@@ -137,7 +133,7 @@ public class CommitValidators {
new SignedOffByValidator(user, perm, projectState),
new ChangeIdValidator(
projectState, user, canonicalWebUrl, installCommitMsgHookCommand, sshInfo),
new ConfigValidator(branch, user, rw, allUsers, allProjects, projectState),
new ConfigValidator(branch, user, rw, allUsers, allProjects),
new BannedCommitsValidator(rejectCommits),
new PluginCommitValidationListener(pluginValidators),
new ExternalIdUpdateListener(allUsers, externalIdsConsistencyChecker),
@@ -150,8 +146,7 @@ public class CommitValidators {
Branch.NameKey branch,
IdentifiedUser user,
SshInfo sshInfo,
RevWalk rw,
ProjectState projectState)
RevWalk rw)
throws IOException {
return new CommitValidators(
ImmutableList.of(
@@ -165,7 +160,7 @@ public class CommitValidators {
canonicalWebUrl,
installCommitMsgHookCommand,
sshInfo),
new ConfigValidator(branch, user, rw, allUsers, allProjects, projectState),
new ConfigValidator(branch, user, rw, allUsers, allProjects),
new PluginCommitValidationListener(pluginValidators),
new ExternalIdUpdateListener(allUsers, externalIdsConsistencyChecker),
new AccountCommitValidator(allUsers, accountValidator),
@@ -371,21 +366,18 @@ public class CommitValidators {
private final RevWalk rw;
private final AllUsersName allUsers;
private final AllProjectsName allProjects;
private final ProjectState projectState;
public ConfigValidator(
Branch.NameKey branch,
IdentifiedUser user,
RevWalk rw,
AllUsersName allUsers,
AllProjectsName allProjects,
ProjectState projectState) {
AllProjectsName allProjects) {
this.branch = branch;
this.user = user;
this.rw = rw;
this.allUsers = allUsers;
this.allProjects = allProjects;
this.projectState = projectState;
this.allUsers = allUsers;
}
@Override
@@ -404,29 +396,6 @@ public class CommitValidators {
}
throw new ConfigInvalidException("invalid project configuration");
}
if (allUsers.equals(receiveEvent.project.getNameKey())
|| allProjects.equals(receiveEvent.project.getNameKey())) {
// Check if the new config modifies any access sections for refs/groups/. These are
// managed by Gerrit and modifications are not allowed.
Set<AccessSection> diff =
Sets.symmetricDifference(
new HashSet<>(projectState.getConfig().getAccessSections()),
new HashSet<>(cfg.getAccessSections()));
boolean modifiesGroupsAccessSection =
diff.stream()
.filter(as -> as.getName().startsWith(RefNames.REFS_GROUPS))
.findAny()
.isPresent();
if (modifiesGroupsAccessSection) {
addError("Invalid project configuration:", messages);
addError(
String.format(
" permissions on %s are managed by gerrit and cannot be modified",
RefNames.REFS_GROUPS),
messages);
throw new ConfigInvalidException("invalid project configuration");
}
}
if (allUsers.equals(receiveEvent.project.getNameKey())
&& !allProjects.equals(cfg.getProject().getParent(allProjects))) {
addError("Invalid project configuration:", messages);

2
java/com/google/gerrit/server/project/SetAccessUtil.java
View File

@@ -139,7 +139,7 @@ public class SetAccessUtil {
&& section.getName().startsWith(RefNames.REFS_GROUPS)) {
throw new BadRequestException(
String.format(
"permissions on %s are managed by gerrit and cannot be modified",
"Permissions on %s is managed by Gerrit and cannot be modified",
RefNames.REFS_GROUPS));
}
}

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

@@ -25,7 +25,6 @@ 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.ImmutableMap;
import com.google.common.collect.Iterables;
import com.google.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.acceptance.GerritConfig;
@@ -34,10 +33,8 @@ import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.acceptance.Sandboxed;
import com.google.gerrit.acceptance.TestAccount;
import com.google.gerrit.common.TimeUtil;
import com.google.gerrit.common.data.AccessSection;
import com.google.gerrit.common.data.GroupReference;
import com.google.gerrit.common.data.Permission;
import com.google.gerrit.common.data.PermissionRule;
import com.google.gerrit.extensions.api.changes.ReviewInput;
import com.google.gerrit.extensions.api.groups.GroupApi;
import com.google.gerrit.extensions.api.groups.GroupInput;
@@ -60,7 +57,6 @@ 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.git.ProjectConfig;
import com.google.gerrit.server.group.InternalGroup;
import com.google.gerrit.server.group.SystemGroupBackend;
import com.google.gerrit.server.group.db.Groups;
@@ -824,96 +820,6 @@ public class GroupsIT extends AbstractDaemonTest {
pushToGroupBranchForReviewAndSubmit(project, null);
}
@Test
public void pushGroupsAccessSectionChangeToAllUsersFails() throws Exception {
TestRepository<InMemoryRepository> repo = cloneProject(allUsers, RefNames.REFS_CONFIG);
String config =
gApi.projects()
.name(allUsers.get())
.branch(RefNames.REFS_CONFIG)
.file(ProjectConfig.PROJECT_CONFIG)
.asString();
Config cfg = new Config();
cfg.fromText(config);
cfg.setString("access", RefNames.REFS_GROUPS + "foo", "push", "group Registered Users");
config = cfg.toText();
PushOneCommit.Result r =
pushFactory
.create(db, admin.getIdent(), repo, "Subject", ProjectConfig.PROJECT_CONFIG, config)
.to(RefNames.REFS_CONFIG);
r.assertErrorStatus("invalid project configuration");
r.assertMessage("permissions on refs/groups/ are managed by gerrit and cannot be modified");
}
@Test
public void pushNonGroupsAccessSectionChangeToAllUsersSucceeds() throws Exception {
// Add an access section for refs/groups manually to see that mutation other data does not
// trigger a validation error.
ProjectConfig projectConfig = projectCache.checkedGet(allUsers).getConfig();
AccessSection as = new AccessSection(RefNames.REFS_GROUPS + "foo");
Permission perm = new Permission("push");
perm.add(new PermissionRule(systemGroupBackend.getGroup(ANONYMOUS_USERS)));
as.addPermission(perm);
projectConfig.replace(as);
saveProjectConfig(allUsers, projectConfig);
TestRepository<InMemoryRepository> repo = cloneProject(allUsers, RefNames.REFS_CONFIG);
String config =
gApi.projects()
.name(allUsers.get())
.branch(RefNames.REFS_CONFIG)
.file(ProjectConfig.PROJECT_CONFIG)
.asString();
assertThat(config).contains("[access \"refs/groups/foo\"]");
Config cfg = new Config();
cfg.fromText(config);
cfg.setString("access", RefNames.REFS_CHANGES + "foo", "push", "group Registered Users");
config = cfg.toText();
PushOneCommit.Result r =
pushFactory
.create(db, admin.getIdent(), repo, "Subject", ProjectConfig.PROJECT_CONFIG, config)
.to(RefNames.REFS_CONFIG);
r.assertOkStatus();
}
@Test
public void pushGroupsAccessSectionChangeToCustomProjectSucceeds() throws Exception {
TestRepository<InMemoryRepository> repo = cloneProject(project, RefNames.REFS_CONFIG);
String config =
gApi.projects()
.name(project.get())
.branch(RefNames.REFS_CONFIG)
.file(ProjectConfig.PROJECT_CONFIG)
.asString();
Config cfg = new Config();
cfg.fromText(config);
cfg.setString("access", RefNames.REFS_GROUPS + "foo", "push", "group Registered Users");
config = cfg.toText();
PushOneCommit.Result r =
pushFactory
.create(
db,
admin.getIdent(),
repo,
"Subject",
ImmutableMap.of(
"groups",
"global:Registered-Users\tRegistered Users",
ProjectConfig.PROJECT_CONFIG,
config))
.to(RefNames.REFS_CONFIG);
r.assertOkStatus();
}
@Test
public void pushCustomInheritanceForAllUsersFails() throws Exception {
TestRepository<InMemoryRepository> repo = cloneProject(allUsers, RefNames.REFS_CONFIG);

2
javatests/com/google/gerrit/acceptance/rest/project/AccessIT.java
View File

@@ -531,7 +531,7 @@ public class AccessIT extends AbstractDaemonTest {
accessInput.add.put(RefNames.REFS_GROUPS + "*", createDefaultAccessSectionInfo());
exception.expect(BadRequestException.class);
exception.expectMessage(
"permissions on refs/groups/ are managed by gerrit and cannot be modified");
"Permissions on refs/groups/ is managed by Gerrit and cannot be modified");
gApi.projects().name(allProjects.get()).access(accessInput);
}