From daf8bd4f90b5ea36890c61b8b32a860356bee907 Mon Sep 17 00:00:00 2001 From: Jonathan Nieder Date: Tue, 1 Oct 2013 15:06:14 -0700 Subject: [PATCH] Revert "Fix: Permission.PUSH_MERGE checking for push bypassing Gerrit" This reverts commit f6159bed862ce09d8e801fd02d50ec3476f53634. Broke direct push of merges for all projects that followed documentation and included a 'Push Merge Commit' ACL prefixed with "refs/for/". The fix for the 'Push Merge Commit' handling looks good, but it should be accompanied by a migration that adjusts existing configuration. Change-Id: Id2a25551d8e9953cd1e678086d491133f5f2d987 --- Documentation/access-control.txt | 11 ++++++----- .../gerrit/server/git/ReceiveCommits.java | 3 +-- .../git/validators/CommitValidators.java | 18 ++++-------------- .../gerrit/server/project/RefControl.java | 12 +----------- .../google/gerrit/server/util/MagicBranch.java | 5 ----- 5 files changed, 12 insertions(+), 37 deletions(-) diff --git a/Documentation/access-control.txt b/Documentation/access-control.txt index a002caa40e..db806ccac7 100644 --- a/Documentation/access-control.txt +++ b/Documentation/access-control.txt @@ -609,11 +609,12 @@ push to happen. Some projects wish to restrict merges to being created by Gerrit. By granting `Push` without `Push Merge Commit`, the only merges that enter the system will be those created by Gerrit. -For pushing a merge commit for review, the reference name connected to -a `Push Merge Commit` entry must be prefixed with `refs/for/`, -for example `refs/for/refs/heads/BRANCH`. For pushing a merge commit -directly into the repository bypassing review, the reference name -must be `refs/heads/BRANCH`. +The reference name connected to a `Push Merge Commit` entry must always +be prefixed with `refs/for/`, for example `refs/for/refs/heads/BRANCH`. +This applies even for an entry that complements a `Push` entry for +`refs/heads/BRANCH` that allows direct pushes of non-merge commits, and +the intention of the `Push Merge Commit` entry is to allow direct pushes +of merge commits. [[category_push_annotated]] 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 0f4c9d1de7..1c47902f1b 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 @@ -2002,8 +2002,7 @@ public class ReceiveCommits { if (ctl.canForgeAuthor() && ctl.canForgeCommitter() && ctl.canForgeGerritServerIdentity() - && (MagicBranch.isForReview(cmd.getRefName()) ? ctl.canUploadMerges() - : ctl.canPushMerges()) + && ctl.canUploadMerges() && !projectControl.getProjectState().isUseSignedOffBy() && Iterables.isEmpty(rejectCommits) && !GitRepositoryManager.REF_CONFIG.equals(ctl.getRefName()) 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 f4810decaf..6175367683 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 @@ -354,21 +354,11 @@ public class CommitValidators { @Override public List onCommitReceived( CommitReceivedEvent receiveEvent) throws CommitValidationException { - if (receiveEvent.commit.getParentCount() > 1) { - if (MagicBranch.isForReview(receiveEvent.command.getRefName())) { - if (!refControl.canUploadMerges()) { - throw new CommitValidationException( - "you are not allowed to upload merges for review"); - } - } else { - if (!refControl.canPushMerges()) { - throw new CommitValidationException( - "you are not allowed to push merges directly bypassing code review"); - } - } + if (receiveEvent.commit.getParentCount() > 1 + && !refControl.canUploadMerges()) { + throw new CommitValidationException("you are not allowed to upload merges"); } - - return Collections. emptyList(); + return Collections.emptyList(); } } 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 2efdc0f98d..08987893b4 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 @@ -147,17 +147,7 @@ public class RefControl { && canWrite(); } - /** - * @return true if this user can push merge commits directly bypassing code - * review to this ref. - */ - public boolean canPushMerges() { - return projectControl.controlForRef(getRefName()).canPerform( - Permission.PUSH_MERGE) - && canWrite(); - } - - /** @return true if this user can upload merge commits to this ref for review */ + /** @return true if this user can submit merge patch sets to this ref */ public boolean canUploadMerges() { return projectControl.controlForRef("refs/for/" + getRefName()) .canPerform(Permission.PUSH_MERGE) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/util/MagicBranch.java b/gerrit-server/src/main/java/com/google/gerrit/server/util/MagicBranch.java index 55527307e1..60844eed66 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/util/MagicBranch.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/util/MagicBranch.java @@ -51,11 +51,6 @@ public final class MagicBranch { || refName.startsWith(NEW_CHANGE); } - /** Checks if the supplied ref name is a magic branch for review */ - public static boolean isForReview(String refName) { - return isMagicBranch(refName); - } - /** Returns the ref name prefix for a magic branch, null if the branch is not magic */ public static String getMagicRefNamePrefix(String refName) { if (refName.startsWith(NEW_DRAFT_CHANGE)) {