diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/SubmitOnPushIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/SubmitOnPushIT.java index 6c5dabdd3e..74412abbe7 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/SubmitOnPushIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/SubmitOnPushIT.java @@ -154,7 +154,7 @@ public class SubmitOnPushIT extends AbstractDaemonTest { @Test public void submitOnPushNotAllowed_Error() throws Exception { PushOneCommit.Result r = pushTo("refs/for/master%submit"); - r.assertErrorStatus("submit not allowed"); + r.assertErrorStatus("update by submit not permitted"); } @Test @@ -166,7 +166,7 @@ public class SubmitOnPushIT extends AbstractDaemonTest { push( "refs/for/master%submit", PushOneCommit.SUBJECT, "a.txt", "other content", r.getChangeId()); - r.assertErrorStatus("submit not allowed"); + r.assertErrorStatus("update by submit not permitted"); } @Test diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/CherryPick.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/CherryPick.java index ae937c96d2..b4867c44ed 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/CherryPick.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/CherryPick.java @@ -26,6 +26,7 @@ import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.git.IntegrationException; import com.google.gerrit.server.permissions.PermissionBackend; import com.google.gerrit.server.permissions.PermissionBackendException; +import com.google.gerrit.server.permissions.ProjectPermission; import com.google.gerrit.server.permissions.RefPermission; import com.google.gerrit.server.project.InvalidChangeOperationException; import com.google.gerrit.server.project.NoSuchChangeException; @@ -99,10 +100,15 @@ public class CherryPick } @Override - public UiAction.Description getDescription(RevisionResource resource) { + public UiAction.Description getDescription(RevisionResource rsrc) { return new UiAction.Description() .setLabel("Cherry Pick") .setTitle("Cherry pick change to a different branch") - .setVisible(resource.getControl().getProjectControl().canUpload() && resource.isCurrent()); + .setVisible( + rsrc.isCurrent() + && permissionBackend + .user(user) + .project(rsrc.getProject()) + .testOrFalse(ProjectPermission.CREATE_CHANGE)); } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java index e0dbaab01e..a8e4b23ac2 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java @@ -1545,10 +1545,13 @@ public class ReceiveCommits { return; } - if (magicBranch.submit - && !projectControl.controlForRef(MagicBranch.NEW_CHANGE + ref).canSubmit(true)) { - reject(cmd, "submit not allowed"); - return; + if (magicBranch.submit) { + try { + permissions.ref(ref).check(RefPermission.UPDATE_BY_SUBMIT); + } catch (AuthException e) { + reject(cmd, e.getMessage()); + return; + } } RevWalk walk = rp.getRevWalk(); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/CommitValidators.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/CommitValidators.java index ff16ab6f7c..ff755ead31 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/CommitValidators.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/CommitValidators.java @@ -111,7 +111,7 @@ public class CommitValidators { IdentifiedUser user = refctl.getUser().asIdentifiedUser(); return new CommitValidators( ImmutableList.of( - new UploadMergesPermissionValidator(refctl), + new UploadMergesPermissionValidator(perm), new AmendedGerritMergeCommitValidationListener(perm, gerritIdent), new AuthorUploaderValidator(user, perm, canonicalWebUrl), new CommitterUploaderValidator(user, perm, canonicalWebUrl), @@ -128,7 +128,7 @@ public class CommitValidators { IdentifiedUser user = refctl.getUser().asIdentifiedUser(); return new CommitValidators( ImmutableList.of( - new UploadMergesPermissionValidator(refctl), + new UploadMergesPermissionValidator(perm), new AmendedGerritMergeCommitValidationListener(perm, gerritIdent), new AuthorUploaderValidator(user, perm, canonicalWebUrl), new SignedOffByValidator(user, perm, refctl.getProjectControl().getProjectState()), @@ -155,7 +155,7 @@ public class CommitValidators { // formats, so we play it safe and exclude them. return new CommitValidators( ImmutableList.of( - new UploadMergesPermissionValidator(refControl), + new UploadMergesPermissionValidator(perm), new AuthorUploaderValidator(user, perm, canonicalWebUrl), new CommitterUploaderValidator(user, perm, canonicalWebUrl))); } @@ -407,21 +407,29 @@ public class CommitValidators { } } - /** Require permission to upload merges. */ + /** Require permission to upload merge commits. */ public static class UploadMergesPermissionValidator implements CommitValidationListener { - private final RefControl refControl; + private final PermissionBackend.ForRef perm; - public UploadMergesPermissionValidator(RefControl refControl) { - this.refControl = refControl; + public UploadMergesPermissionValidator(PermissionBackend.ForRef perm) { + this.perm = perm; } @Override public List onCommitReceived(CommitReceivedEvent receiveEvent) throws CommitValidationException { - if (receiveEvent.commit.getParentCount() > 1 && !refControl.canUploadMerges()) { - throw new CommitValidationException("you are not allowed to upload merges"); + if (receiveEvent.commit.getParentCount() <= 1) { + return Collections.emptyList(); + } + try { + perm.check(RefPermission.MERGE); + return Collections.emptyList(); + } catch (AuthException e) { + throw new CommitValidationException("you are not allowed to upload merges"); + } catch (PermissionBackendException e) { + log.error("cannot check MERGE", e); + throw new CommitValidationException("internal auth error"); } - return Collections.emptyList(); } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/permissions/ProjectPermission.java b/gerrit-server/src/main/java/com/google/gerrit/server/permissions/ProjectPermission.java index 098b07a439..3078437202 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/permissions/ProjectPermission.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/permissions/ProjectPermission.java @@ -47,7 +47,22 @@ public enum ProjectPermission { * .check(RefPermission.CREATE); * */ - CREATE_REF; + CREATE_REF, + + /** + * Can create at least one change in the project. + * + *

This project level permission only validates the user may create a change for some branch + * within the project. The exact reference name must be checked at creation: + * + *

permissionBackend
+   *    .user(user)
+   *    .project(proj)
+   *    .ref(ref)
+   *    .check(RefPermission.CREATE_CHANGE);
+   * 
+ */ + CREATE_CHANGE; private final String name; diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/permissions/RefPermission.java b/gerrit-server/src/main/java/com/google/gerrit/server/permissions/RefPermission.java index 127603fc12..e03272b156 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/permissions/RefPermission.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/permissions/RefPermission.java @@ -28,9 +28,20 @@ public enum RefPermission { FORGE_AUTHOR(Permission.FORGE_AUTHOR), FORGE_COMMITTER(Permission.FORGE_COMMITTER), FORGE_SERVER(Permission.FORGE_SERVER), + MERGE, BYPASS_REVIEW, - CREATE_CHANGE; + /** Create a change to code review a commit. */ + CREATE_CHANGE, + + /** + * Creates changes, then also immediately submits them during {@code push}. + * + *

This is similar to {@link #UPDATE} except it constructs changes first, then submits them + * according to the submit strategy, which may include cherry-pick or rebase. By creating changes + * for each commit, automatic server side rebase, and post-update review are enabled. + */ + UPDATE_BY_SUBMIT; private final String name; diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java index 252118c832..57c933fecc 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java @@ -245,7 +245,7 @@ public class ProjectControl { } /** Returns whether the project is hidden. */ - public boolean isHidden() { + private boolean isHidden() { return getProject().getState().equals(com.google.gerrit.extensions.client.ProjectState.HIDDEN); } @@ -253,7 +253,7 @@ public class ProjectControl { return (canPerformOnAnyRef(Permission.CREATE) || isOwnerAnyRef()); } - public boolean canUpload() { + private boolean canCreateChanges() { for (SectionMatcher matcher : access()) { AccessSection section = matcher.section; if (section.getName().startsWith("refs/for/")) { @@ -591,6 +591,8 @@ public class ProjectControl { case CREATE_REF: return canAddRefs(); + case CREATE_CHANGE: + return canCreateChanges(); } throw new PermissionBackendException(perm + " unsupported"); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/RefControl.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/RefControl.java index c9e4989d9d..fa5720ffeb 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/project/RefControl.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/RefControl.java @@ -165,7 +165,7 @@ public class RefControl { } /** @return true if this user can submit merge patch sets to this ref */ - public boolean canUploadMerges() { + private boolean canUploadMerges() { return projectControl .controlForRef("refs/for/" + getRefName()) .canPerform(Permission.PUSH_MERGE) @@ -178,7 +178,7 @@ public class RefControl { } /** @return true if this user can submit patch sets to this ref */ - public boolean canSubmit(boolean isChangeOwner) { + boolean canSubmit(boolean isChangeOwner) { if (RefNames.REFS_CONFIG.equals(refName)) { // Always allow project owners to submit configuration changes. // Submitting configuration changes modifies the access control @@ -725,15 +725,22 @@ public class RefControl { return canUpdate(); case FORCE_UPDATE: return canForceUpdate(); + case FORGE_AUTHOR: return canForgeAuthor(); case FORGE_COMMITTER: return canForgeCommitter(); case FORGE_SERVER: return canForgeGerritServerIdentity(); + case MERGE: + return canUploadMerges(); + case CREATE_CHANGE: return canUpload(); + case UPDATE_BY_SUBMIT: + return projectControl.controlForRef("refs/for/" + getRefName()).canSubmit(true); + case BYPASS_REVIEW: return canForgeAuthor() && canForgeCommitter()