Revert "Fix: Permission.PUSH_MERGE checking for push bypassing Gerrit"
This reverts commit f6159bed86.
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
			
			
This commit is contained in:
		@@ -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
 | 
					by Gerrit. By granting `Push` without `Push Merge Commit`, the only
 | 
				
			||||||
merges that enter the system will be those created by Gerrit.
 | 
					merges that enter the system will be those created by Gerrit.
 | 
				
			||||||
 | 
					
 | 
				
			||||||
For pushing a merge commit for review, the reference name connected to
 | 
					The reference name connected to a `Push Merge Commit` entry must always
 | 
				
			||||||
a `Push Merge Commit` entry must be prefixed with `refs/for/`,
 | 
					be prefixed with `refs/for/`, for example `refs/for/refs/heads/BRANCH`.
 | 
				
			||||||
for example `refs/for/refs/heads/BRANCH`. For pushing a merge commit
 | 
					This applies even for an entry that complements a `Push` entry for
 | 
				
			||||||
directly into the repository bypassing review, the reference name
 | 
					`refs/heads/BRANCH` that allows direct pushes of non-merge commits, and
 | 
				
			||||||
must be `refs/heads/BRANCH`.
 | 
					the intention of the `Push Merge Commit` entry is to allow direct pushes
 | 
				
			||||||
 | 
					of merge commits.
 | 
				
			||||||
 | 
					
 | 
				
			||||||
 | 
					
 | 
				
			||||||
[[category_push_annotated]]
 | 
					[[category_push_annotated]]
 | 
				
			||||||
 
 | 
				
			|||||||
@@ -2002,8 +2002,7 @@ public class ReceiveCommits {
 | 
				
			|||||||
    if (ctl.canForgeAuthor()
 | 
					    if (ctl.canForgeAuthor()
 | 
				
			||||||
        && ctl.canForgeCommitter()
 | 
					        && ctl.canForgeCommitter()
 | 
				
			||||||
        && ctl.canForgeGerritServerIdentity()
 | 
					        && ctl.canForgeGerritServerIdentity()
 | 
				
			||||||
        && (MagicBranch.isForReview(cmd.getRefName()) ? ctl.canUploadMerges()
 | 
					        && ctl.canUploadMerges()
 | 
				
			||||||
            : ctl.canPushMerges())
 | 
					 | 
				
			||||||
        && !projectControl.getProjectState().isUseSignedOffBy()
 | 
					        && !projectControl.getProjectState().isUseSignedOffBy()
 | 
				
			||||||
        && Iterables.isEmpty(rejectCommits)
 | 
					        && Iterables.isEmpty(rejectCommits)
 | 
				
			||||||
        && !GitRepositoryManager.REF_CONFIG.equals(ctl.getRefName())
 | 
					        && !GitRepositoryManager.REF_CONFIG.equals(ctl.getRefName())
 | 
				
			||||||
 
 | 
				
			|||||||
@@ -354,21 +354,11 @@ public class CommitValidators {
 | 
				
			|||||||
    @Override
 | 
					    @Override
 | 
				
			||||||
    public List<CommitValidationMessage> onCommitReceived(
 | 
					    public List<CommitValidationMessage> onCommitReceived(
 | 
				
			||||||
        CommitReceivedEvent receiveEvent) throws CommitValidationException {
 | 
					        CommitReceivedEvent receiveEvent) throws CommitValidationException {
 | 
				
			||||||
      if (receiveEvent.commit.getParentCount() > 1) {
 | 
					      if (receiveEvent.commit.getParentCount() > 1
 | 
				
			||||||
        if (MagicBranch.isForReview(receiveEvent.command.getRefName())) {
 | 
					          && !refControl.canUploadMerges()) {
 | 
				
			||||||
          if (!refControl.canUploadMerges()) {
 | 
					        throw new CommitValidationException("you are not allowed to upload merges");
 | 
				
			||||||
            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");
 | 
					 | 
				
			||||||
          }
 | 
					 | 
				
			||||||
        }
 | 
					 | 
				
			||||||
      }
 | 
					      }
 | 
				
			||||||
 | 
					      return Collections.<CommitValidationMessage>emptyList();
 | 
				
			||||||
      return Collections.<CommitValidationMessage> emptyList();
 | 
					 | 
				
			||||||
    }
 | 
					    }
 | 
				
			||||||
  }
 | 
					  }
 | 
				
			||||||
 | 
					
 | 
				
			||||||
 
 | 
				
			|||||||
@@ -147,17 +147,7 @@ public class RefControl {
 | 
				
			|||||||
        && canWrite();
 | 
					        && canWrite();
 | 
				
			||||||
  }
 | 
					  }
 | 
				
			||||||
 | 
					
 | 
				
			||||||
  /**
 | 
					  /** @return true if this user can submit merge patch sets to this ref */
 | 
				
			||||||
   * @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 */
 | 
					 | 
				
			||||||
  public boolean canUploadMerges() {
 | 
					  public boolean canUploadMerges() {
 | 
				
			||||||
    return projectControl.controlForRef("refs/for/" + getRefName())
 | 
					    return projectControl.controlForRef("refs/for/" + getRefName())
 | 
				
			||||||
        .canPerform(Permission.PUSH_MERGE)
 | 
					        .canPerform(Permission.PUSH_MERGE)
 | 
				
			||||||
 
 | 
				
			|||||||
@@ -51,11 +51,6 @@ public final class MagicBranch {
 | 
				
			|||||||
        || refName.startsWith(NEW_CHANGE);
 | 
					        || 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, <code>null</code> if the branch is not magic */
 | 
					  /** Returns the ref name prefix for a magic branch, <code>null</code> if the branch is not magic */
 | 
				
			||||||
  public static String getMagicRefNamePrefix(String refName) {
 | 
					  public static String getMagicRefNamePrefix(String refName) {
 | 
				
			||||||
    if (refName.startsWith(NEW_DRAFT_CHANGE)) {
 | 
					    if (refName.startsWith(NEW_DRAFT_CHANGE)) {
 | 
				
			||||||
 
 | 
				
			|||||||
		Reference in New Issue
	
	Block a user