opendev/gerrit
Code Issues Proposed changes

Merge changes Ic857fc10,Ibc1c742a,If2760542

Browse Source 
* changes:
  ReceiveCommits: Fix error message when Force Push is required
  PushPermissionsIT: Add tests for remaining checks
  PushPermissionsIT: Add many more tests
This commit is contained in:
Dave Borowitz
2018-04-23 14:21:00 +00:00
committed by Gerrit Code Review
parent 0195b9690a 0e1dab6977
commit 712f0ea28e
3 changed files with 320 additions and 17 deletions
Download Patch File Download Diff File Expand all files Collapse all files

6
java/com/google/gerrit/server/git/receive/ReceiveCommits.java
View File

@@ -36,7 +36,6 @@ import static org.eclipse.jgit.lib.Constants.R_HEADS;
import static org.eclipse.jgit.transport.ReceiveCommand.Result.NOT_ATTEMPTED;
import static org.eclipse.jgit.transport.ReceiveCommand.Result.OK;
import static org.eclipse.jgit.transport.ReceiveCommand.Result.REJECTED_MISSING_OBJECT;
import static org.eclipse.jgit.transport.ReceiveCommand.Result.REJECTED_NONFASTFORWARD;
import static org.eclipse.jgit.transport.ReceiveCommand.Result.REJECTED_OTHER_REASON;
import com.google.common.base.Function;
@@ -498,7 +497,7 @@ class ReceiveCommits {
// Collections populated during processing.
actualCommands = new ArrayList<>();
errors = LinkedListMultimap.create();
errors = MultimapBuilder.linkedHashKeys().arrayListValues().build();
messages = new ArrayList<>();
pushOptions = LinkedListMultimap.create();
replaceByChange = new LinkedHashMap<>();
@@ -1228,8 +1227,7 @@ class ReceiveCommits {
}
actualCommands.add(cmd);
} else {
cmd.setResult(
REJECTED_NONFASTFORWARD, " need '" + PermissionRule.FORCE_PUSH + "' privilege.");
cmd.setResult(REJECTED_OTHER_REASON, "need '" + PermissionRule.FORCE_PUSH + "' privilege.");
}
}

2
javatests/com/google/gerrit/acceptance/git/ForcePushIT.java
View File

@@ -51,7 +51,7 @@ public class ForcePushIT extends AbstractDaemonTest {
pushFactory.create(db, admin.getIdent(), testRepo, "change2", "b.txt", "content");
push2.setForce(true);
PushOneCommit.Result r2 = push2.to("refs/heads/master");
r2.assertErrorStatus("non-fast forward");
r2.assertErrorStatus("need 'Force Push' privilege.");
}
@Test

329
javatests/com/google/gerrit/acceptance/git/PushPermissionsIT.java
View File

@@ -14,35 +14,72 @@
package com.google.gerrit.acceptance.git;
import static com.google.common.truth.Truth.assertThat;
import static com.google.common.truth.Truth.assertWithMessage;
import static com.google.common.truth.Truth.assert_;
import static com.google.gerrit.git.testing.PushResultSubject.assertThat;
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.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.common.data.AccessSection;
import com.google.gerrit.common.data.GlobalCapability;
import com.google.gerrit.common.data.Permission;
import com.google.gerrit.extensions.api.changes.ReviewInput;
import com.google.gerrit.extensions.client.InheritableBoolean;
import com.google.gerrit.extensions.client.ProjectState;
import com.google.gerrit.extensions.common.ChangeInput;
import com.google.gerrit.reviewdb.client.BooleanProjectConfig;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.server.project.ProjectConfig;
import com.google.gerrit.server.project.testing.Util;
import java.util.Arrays;
import java.util.function.Consumer;
import org.eclipse.jgit.api.PushCommand;
import org.eclipse.jgit.junit.TestRepository;
import org.eclipse.jgit.lib.BlobBasedConfig;
import org.eclipse.jgit.lib.Config;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.transport.PushResult;
import org.eclipse.jgit.transport.RefSpec;
import org.eclipse.jgit.transport.TrackingRefUpdate;
import org.junit.Before;
import org.junit.Test;
public class PushPermissionsIT extends AbstractDaemonTest {
@Before
public void setUp() throws Exception {
// Remove all push-related permissions, so they can be added back individually by test methods.
try (ProjectConfigUpdate u = updateProject(allProjects)) {
removeAllBranchPermissions(u.getConfig(), Permission.PUSH);
removeAllBranchPermissions(u.getConfig(), Permission.CREATE);
removeAllBranchPermissions(u.getConfig(), Permission.DELETE);
removeAllBranchPermissions(u.getConfig(), Permission.PUSH_MERGE);
ProjectConfig cfg = u.getConfig();
cfg.getProject()
.setBooleanConfig(BooleanProjectConfig.REQUIRE_CHANGE_ID, InheritableBoolean.FALSE);
// Remove push-related permissions, so they can be added back individually by test methods.
removeAllBranchPermissions(
cfg,
Permission.ADD_PATCH_SET,
Permission.CREATE_REVIEW,
Permission.CREATE,
Permission.DELETE,
Permission.PUSH,
Permission.PUSH_MERGE,
Permission.SUBMIT);
removeAllGlobalCapabilities(cfg, GlobalCapability.ADMINISTRATE_SERVER);
// Include some auxiliary permissions.
Util.allow(cfg, Permission.FORGE_AUTHOR, REGISTERED_USERS, "refs/*");
Util.allow(cfg, Permission.FORGE_COMMITTER, REGISTERED_USERS, "refs/*");
u.save();
}
}
@Test
public void noDirectPushPermissions() throws Exception {
public void fastForwardUpdateDenied() throws Exception {
testRepo.branch("HEAD").commit().create();
PushResult r = push("HEAD:refs/heads/master");
assertThat(r)
@@ -59,22 +96,290 @@ public class PushPermissionsIT extends AbstractDaemonTest {
assertThat(r).hasProcessed(ImmutableMap.of("refs", 1));
}
private static void removeAllBranchPermissions(ProjectConfig cfg, String permission) {
@Test
public void nonFastForwardUpdateDenied() throws Exception {
ObjectId commit = testRepo.commit().create();
PushResult r = push("+" + commit.name() + ":refs/heads/master");
assertThat(r).onlyRef("refs/heads/master").isRejected("need 'Force Push' privilege.");
assertThat(r).hasNoMessages();
// TODO(dborowitz): Why does this not mention refs?
assertThat(r).hasProcessed(ImmutableMap.of());
}
@Test
public void deleteDenied() throws Exception {
PushResult r = push(":refs/heads/master");
assertThat(r).onlyRef("refs/heads/master").isRejected("cannot delete references");
assertThat(r)
.hasMessages(
"Branch refs/heads/master:",
"You need 'Delete Reference' rights or 'Push' rights with the ",
"'Force Push' flag set to delete references.",
"User: admin",
"Please read the documentation and contact an administrator",
"if you feel the configuration is incorrect");
assertThat(r).hasProcessed(ImmutableMap.of("refs", 1));
}
@Test
public void createDenied() throws Exception {
testRepo.branch("HEAD").commit().create();
PushResult r = push("HEAD:refs/heads/newbranch");
assertThat(r)
.onlyRef("refs/heads/newbranch")
.isRejected("prohibited by Gerrit: create not permitted for refs/heads/newbranch");
assertThat(r).hasNoMessages();
assertThat(r).hasProcessed(ImmutableMap.of("refs", 1));
}
@Test
public void groupRefsByMessage() throws Exception {
try (Repository repo = repoManager.openRepository(project)) {
TestRepository<?> tr = new TestRepository<>(repo);
tr.branch("foo").commit().create();
tr.branch("bar").commit().create();
}
testRepo.branch("HEAD").commit().create();
PushResult r = push(":refs/heads/foo", ":refs/heads/bar", "HEAD:refs/heads/master");
assertThat(r).ref("refs/heads/foo").isRejected("cannot delete references");
assertThat(r).ref("refs/heads/bar").isRejected("cannot delete references");
assertThat(r)
.ref("refs/heads/master")
.isRejected("prohibited by Gerrit: ref update access denied");
assertThat(r)
.hasMessages(
"Branches refs/heads/foo, refs/heads/bar:",
"You need 'Delete Reference' rights or 'Push' rights with the ",
"'Force Push' flag set to delete references.",
"Branch refs/heads/master:",
"You are not allowed to perform this operation.",
"To push into this reference you need 'Push' rights.",
"User: admin",
"Please read the documentation and contact an administrator",
"if you feel the configuration is incorrect");
}
@Test
public void readOnlyProjectRejectedBeforeTestingPermissions() throws Exception {
try (Repository repo = repoManager.openRepository(project)) {
try (ProjectConfigUpdate u = updateProject(project)) {
u.getConfig().getProject().setState(ProjectState.READ_ONLY);
u.save();
}
}
PushResult r = push(":refs/heads/master");
assertThat(r)
.onlyRef("refs/heads/master")
.isRejected("prohibited by Gerrit: project state does not permit write");
assertThat(r).hasNoMessages();
assertThat(r).hasProcessed(ImmutableMap.of("refs", 1));
}
@Test
public void refsMetaConfigUpdateRequiresProjectOwner() throws Exception {
grant(project, "refs/meta/config", Permission.PUSH, false, REGISTERED_USERS);
forceFetch("refs/meta/config");
ObjectId commit = testRepo.branch("refs/meta/config").commit().create();
PushResult r = push(commit.name() + ":refs/meta/config");
assertThat(r)
.onlyRef("refs/meta/config")
// ReceiveCommits theoretically has a different message when a WRITE_CONFIG check fails, but
// it never gets there, since DefaultPermissionBackend special-cases refs/meta/config and
// denies UPDATE if the user is not a project owner.
.isRejected("prohibited by Gerrit: ref update access denied");
assertThat(r)
.hasMessages(
"Branch refs/meta/config:",
"You are not allowed to perform this operation.",
"Configuration changes can only be pushed by project owners",
"who also have 'Push' rights on refs/meta/config",
"User: admin",
"Please read the documentation and contact an administrator",
"if you feel the configuration is incorrect");
assertThat(r).hasProcessed(ImmutableMap.of("refs", 1));
grant(project, "refs/*", Permission.OWNER, false, REGISTERED_USERS);
// Re-fetch refs/meta/config from the server because the grant changed it, and we want a
// fast-forward.
forceFetch("refs/meta/config");
commit = testRepo.branch("refs/meta/config").commit().create();
assertThat(push(commit.name() + ":refs/meta/config")).onlyRef("refs/meta/config").isOk();
}
@Test
public void createChangeDenied() throws Exception {
testRepo.branch("HEAD").commit().create();
PushResult r = push("HEAD:refs/for/master");
assertThat(r)
.onlyRef("refs/for/master")
.isRejected("create change not permitted for refs/heads/master");
assertThat(r)
.hasMessages(
"Branch refs/heads/master:",
"You need 'Create Review' rights to upload code review requests.",
"Verify that you are pushing to the right branch.",
"User: admin",
"Please read the documentation and contact an administrator",
"if you feel the configuration is incorrect");
assertThat(r).hasProcessed(ImmutableMap.of("refs", 1));
}
@Test
public void updateBySubmitDenied() throws Exception {
grant(project, "refs/for/refs/heads/*", Permission.PUSH, false, REGISTERED_USERS);
ObjectId commit = testRepo.branch("HEAD").commit().create();
assertThat(push("HEAD:refs/for/master")).onlyRef("refs/for/master").isOk();
gApi.changes().id(commit.name()).current().review(ReviewInput.approve());
PushResult r = push("HEAD:refs/for/master%submit");
assertThat(r)
.onlyRef("refs/for/master%submit")
.isRejected("update by submit not permitted for refs/heads/master");
assertThat(r).hasNoMessages();
assertThat(r).hasProcessed(ImmutableMap.of("refs", 1));
}
@Test
public void addPatchSetDenied() throws Exception {
grant(project, "refs/for/refs/heads/*", Permission.PUSH, false, REGISTERED_USERS);
setApiUser(user);
ChangeInput ci = new ChangeInput();
ci.project = project.get();
ci.branch = "master";
ci.subject = "A change";
Change.Id id = new Change.Id(gApi.changes().create(ci).get()._number);
setApiUser(admin);
ObjectId ps1Id = forceFetch(new PatchSet.Id(id, 1).toRefName());
ObjectId ps2Id = testRepo.amend(ps1Id).add("file", "content").create();
PushResult r = push(ps2Id.name() + ":refs/for/master");
assertThat(r)
.onlyRef("refs/for/master")
.isRejected("cannot add patch set to " + id.get() + ".");
assertThat(r).hasNoMessages();
assertThat(r).hasProcessed(ImmutableMap.of("refs", 1));
}
@Test
public void skipValidationDenied() throws Exception {
grant(project, "refs/heads/*", Permission.PUSH, false, REGISTERED_USERS);
testRepo.branch("HEAD").commit().create();
PushResult r =
push(c -> c.setPushOptions(ImmutableList.of("skip-validation")), "HEAD:refs/heads/master");
assertThat(r)
.onlyRef("refs/heads/master")
.isRejected("skip validation not permitted for refs/heads/master");
assertThat(r).hasNoMessages();
assertThat(r).hasProcessed(ImmutableMap.of("refs", 1));
}
@Test
public void accessDatabaseForNoteDbDenied() throws Exception {
grant(project, "refs/heads/*", Permission.PUSH, false, REGISTERED_USERS);
testRepo.branch("HEAD").commit().create();
PushResult r =
push(
c -> c.setPushOptions(ImmutableList.of("notedb=allow")),
"HEAD:refs/changes/34/1234/meta");
// Same rejection message regardless of whether NoteDb is actually enabled.
assertThat(r)
.onlyRef("refs/changes/34/1234/meta")
.isRejected("NoteDb update requires access database permission");
assertThat(r).hasNoMessages();
assertThat(r).hasProcessed(ImmutableMap.of("refs", 1));
}
@Test
public void administrateServerForUpdateParentDenied() throws Exception {
grant(project, "refs/meta/config", Permission.PUSH, false, REGISTERED_USERS);
grant(project, "refs/*", Permission.OWNER, false, REGISTERED_USERS);
String project2 = name("project2");
gApi.projects().create(project2);
ObjectId oldId = forceFetch("refs/meta/config");
Config cfg = new BlobBasedConfig(null, testRepo.getRepository(), oldId, "project.config");
cfg.setString("access", null, "inheritFrom", project2);
ObjectId newId =
testRepo.branch("refs/meta/config").commit().add("project.config", cfg.toText()).create();
PushResult r = push(newId.name() + ":refs/meta/config");
assertThat(r)
.onlyRef("refs/meta/config")
.isRejected("invalid project configuration: only Gerrit admin can set parent");
assertThat(r).hasNoMessages();
assertThat(r).hasProcessed(ImmutableMap.of("refs", 1));
}
private static void removeAllBranchPermissions(ProjectConfig cfg, String... permissions) {
cfg.getAccessSections()
.stream()
.filter(s -> s.getName().startsWith("refs/heads/") || s.getName().equals("refs/*"))
.forEach(s -> s.removePermission(permission));
.filter(
s ->
s.getName().startsWith("refs/heads/")
|| s.getName().startsWith("refs/for/")
|| s.getName().equals("refs/*"))
.forEach(s -> Arrays.stream(permissions).forEach(s::removePermission));
}
private static void removeAllGlobalCapabilities(ProjectConfig cfg, String... capabilities) {
Arrays.stream(capabilities)
.forEach(
c ->
cfg.getAccessSection(AccessSection.GLOBAL_CAPABILITIES, true)
.getPermission(c, true)
.getRules()
.clear());
}
private PushResult push(String... refSpecs) throws Exception {
Iterable<PushResult> results =
return push(c -> {}, refSpecs);
}
private PushResult push(Consumer<PushCommand> setUp, String... refSpecs) throws Exception {
PushCommand cmd =
testRepo
.git()
.push()
.setRemote("origin")
.setRefSpecs(Arrays.stream(refSpecs).map(RefSpec::new).collect(toList()))
.call();
.setRefSpecs(Arrays.stream(refSpecs).map(RefSpec::new).collect(toList()));
setUp.accept(cmd);
Iterable<PushResult> results = cmd.call();
assertWithMessage("expected 1 PushResult").that(results).hasSize(1);
return results.iterator().next();
}
private ObjectId forceFetch(String ref) throws Exception {
TrackingRefUpdate u =
testRepo.git().fetch().setRefSpecs("+" + ref + ":" + ref).call().getTrackingRefUpdate(ref);
assertThat(u).isNotNull();
switch (u.getResult()) {
case NEW:
case FAST_FORWARD:
case FORCED:
break;
case IO_FAILURE:
case LOCK_FAILURE:
case NOT_ATTEMPTED:
case NO_CHANGE:
case REJECTED:
case REJECTED_CURRENT_BRANCH:
case REJECTED_MISSING_OBJECT:
case REJECTED_OTHER_REASON:
case RENAMED:
default:
assert_().fail("fetch failed to update local %s: %s", ref, u.getResult());
break;
}
return u.getNewObjectId();
}
}