From 257ca2aaf49145c95f4008fa350adbf3305857b9 Mon Sep 17 00:00:00 2001 From: Shawn Pearce Date: Sat, 29 Apr 2017 11:32:14 -0700 Subject: [PATCH 1/4] Hide RefControl.canSubmit, adding UPDATE_BY_SUBMIT Test for submit on refs/for/refs/heads/master by checking for UPDATE_BY_SUBMIT on refs/heads/master with the PermissionBackend. The UPDATE_BY_SUBMIT strategy allows a user to create changes and then immediately update the destination branch during push, without waiting for review workflow. Using this permission still requires the caller to have CREATE_CHANGE permission, which is checked earlier. Change-Id: I59f945adf1920ac12489bf8bbe892cde93550033 --- .../google/gerrit/acceptance/git/SubmitOnPushIT.java | 4 ++-- .../com/google/gerrit/server/git/ReceiveCommits.java | 11 +++++++---- .../gerrit/server/permissions/RefPermission.java | 12 +++++++++++- .../com/google/gerrit/server/project/RefControl.java | 5 ++++- 4 files changed, 24 insertions(+), 8 deletions(-) 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/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/permissions/RefPermission.java b/gerrit-server/src/main/java/com/google/gerrit/server/permissions/RefPermission.java index 127603fc12..4c738bffe4 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 @@ -30,7 +30,17 @@ public enum RefPermission { FORGE_SERVER(Permission.FORGE_SERVER), 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, post-submit review is possible. + */ + UPDATE_BY_SUBMIT; private final String name; 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..88caa9eb37 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 @@ -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 @@ -734,6 +734,9 @@ public class RefControl { case CREATE_CHANGE: return canUpload(); + case UPDATE_BY_SUBMIT: + return projectControl.controlForRef("refs/for/" + getRefName()).canSubmit(true); + case BYPASS_REVIEW: return canForgeAuthor() && canForgeCommitter() From f081209063f7edd82ea9752475b789caba81ea38 Mon Sep 17 00:00:00 2001 From: Shawn Pearce Date: Sat, 29 Apr 2017 11:49:39 -0700 Subject: [PATCH 2/4] Add RefPermission.MERGE to check creating merge commits Hide canUploadMerges() and instead use MERGE permission on the ref. If the caller has MERGE they can upload merge commits, or create merges with the REST API. Change-Id: I2db15347d66c559b0487bf192a8245c2a2e69de7 --- .../git/validators/CommitValidators.java | 28 ++++++++++++------- .../server/permissions/RefPermission.java | 3 +- .../gerrit/server/project/RefControl.java | 6 +++- 3 files changed, 25 insertions(+), 12 deletions(-) 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 c2b5947155..a293ed1c0c 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))); } @@ -408,21 +408,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/RefPermission.java b/gerrit-server/src/main/java/com/google/gerrit/server/permissions/RefPermission.java index 4c738bffe4..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,6 +28,7 @@ public enum RefPermission { FORGE_AUTHOR(Permission.FORGE_AUTHOR), FORGE_COMMITTER(Permission.FORGE_COMMITTER), FORGE_SERVER(Permission.FORGE_SERVER), + MERGE, BYPASS_REVIEW, /** Create a change to code review a commit. */ @@ -38,7 +39,7 @@ public enum RefPermission { * *

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, post-submit review is possible. + * for each commit, automatic server side rebase, and post-update review are enabled. */ UPDATE_BY_SUBMIT; 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 88caa9eb37..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) @@ -725,12 +725,16 @@ 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(); From 510ed0a274bc51ff798256e34d28610314ce8ec4 Mon Sep 17 00:00:00 2001 From: Shawn Pearce Date: Sat, 29 Apr 2017 12:03:23 -0700 Subject: [PATCH 3/4] Add ProjectPermission.CREATE_CHANGE for cherry picks The cherry pick UI action needs to know if the user can create a change in any branch of the repository. Add CREATE_CHANGE to ProjectPermission which (like CREATE_REF) only offers an educated guess about whether or not the user has creation permission. Actual CREATE_CHANGE is evaluated again during the cherry pick action itself, after the user has selected a specific branch. Change-Id: I8fe33e418515cf029d1995f0c0b2288f9ee15f30 --- .../google/gerrit/server/change/CherryPick.java | 10 ++++++++-- .../server/permissions/ProjectPermission.java | 17 ++++++++++++++++- .../gerrit/server/project/ProjectControl.java | 4 +++- 3 files changed, 27 insertions(+), 4 deletions(-) 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/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/project/ProjectControl.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java index 44a3405c2a..196dcef695 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 @@ -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"); } From 946b5a737d2936fa71ec8e9bf5b476747b671514 Mon Sep 17 00:00:00 2001 From: Shawn Pearce Date: Sat, 29 Apr 2017 12:09:41 -0700 Subject: [PATCH 4/4] Hide ProjectControl.isHidden This is now only used by the PermissionBackend.ForProject glue code, and can be marked private. Change-Id: Ic2fb4d24dde3d2876c5245cb2f3578ea943d75ab --- .../java/com/google/gerrit/server/project/ProjectControl.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 196dcef695..74780afd0c 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); }