Merge "Fix: User could get around restrictions by reverting a commit"

gerrit-server/src/main/java/com/google/gerrit/server/ChangeUtil.java

@@ -31,9 +31,9 @@ import com.google.gerrit.server.events.CommitReceivedEvent;
 import com.google.gerrit.server.extensions.events.GitReferenceUpdated;
 import com.google.gerrit.server.git.GitRepositoryManager;
 import com.google.gerrit.server.git.MergeOp;
-import com.google.gerrit.server.mail.CommitMessageEditedSender;
 import com.google.gerrit.server.git.validators.CommitValidationException;
 import com.google.gerrit.server.git.validators.CommitValidators;
+import com.google.gerrit.server.mail.CommitMessageEditedSender;
 import com.google.gerrit.server.mail.RevertedSender;
 import com.google.gerrit.server.patch.PatchSetInfoFactory;
 import com.google.gerrit.server.patch.PatchSetInfoNotAvailableException;
@@ -47,7 +47,6 @@ import com.google.gwtorm.server.OrmException;
 
 import org.eclipse.jgit.errors.IncorrectObjectTypeException;
 import org.eclipse.jgit.errors.MissingObjectException;
-import org.eclipse.jgit.errors.RepositoryNotFoundException;
 import org.eclipse.jgit.lib.CommitBuilder;
 import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.ObjectInserter;
@@ -192,14 +191,17 @@ public class ChangeUtil {
     db.patchSetAncestors().insert(toInsert);
   }
 
-  public static Change.Id revert(final PatchSet.Id patchSetId,
+  public static Change.Id revert(final RefControl refControl,
-      final IdentifiedUser user, final String message, final ReviewDb db,
+      final PatchSet.Id patchSetId, final IdentifiedUser user,
+      final CommitValidators commitValidators,
+      final String message, final ReviewDb db,
       final RevertedSender.Factory revertedSenderFactory,
-      final ChangeHooks hooks, GitRepositoryManager gitManager,
+      final ChangeHooks hooks, Repository git,
       final PatchSetInfoFactory patchSetInfoFactory,
-      final GitReferenceUpdated replication, PersonIdent myIdent)
+      final GitReferenceUpdated replication, PersonIdent myIdent,
-      throws NoSuchChangeException, EmailException, OrmException,
+      String canonicalWebUrl) throws NoSuchChangeException, EmailException,
-      MissingObjectException, IncorrectObjectTypeException, IOException {
+      OrmException, MissingObjectException, IncorrectObjectTypeException,
+      IOException, InvalidChangeOperationException {
     final Change.Id changeId = patchSetId.getParentKey();
     final PatchSet patch = db.patchSets().get(patchSetId);
     if (patch == null) {
@@ -207,13 +209,6 @@ public class ChangeUtil {
     }
     final Change changeToRevert = db.changes().get(changeId);
 
-    final Repository git;
-    try {
-      git = gitManager.openRepository(changeToRevert.getProject());
-    } catch (RepositoryNotFoundException e) {
-      throw new NoSuchChangeException(changeId, e);
-    }
-
     final RevWalk revWalk = new RevWalk(git);
     try {
       RevCommit commitToRevert =
@@ -260,6 +255,18 @@ public class ChangeUtil {
       ps.setUploader(change.getOwner());
       ps.setRevision(new RevId(revertCommit.name()));
 
+      CommitReceivedEvent commitReceivedEvent =
+          new CommitReceivedEvent(new ReceiveCommand(ObjectId.zeroId(),
+              revertCommit.getId(), ps.getRefName()), refControl
+              .getProjectControl().getProject(), refControl.getRefName(),
+              revertCommit, user);
+
+      try {
+        commitValidators.validateForRevertCommits(commitReceivedEvent);
+      } catch (CommitValidationException e) {
+        throw new InvalidChangeOperationException(e.getMessage());
+      }
+
       change.setCurrentPatchSet(patchSetInfoFactory.get(revertCommit, ps.getId()));
       ChangeUtil.updated(change);
 
@@ -305,7 +312,6 @@ public class ChangeUtil {
       return change.getId();
     } finally {
       revWalk.release();
-      git.close();
     }
   }
 

gerrit-server/src/main/java/com/google/gerrit/server/change/Revert.java

@@ -17,6 +17,7 @@ package com.google.gerrit.server.change;
 import com.google.common.base.Strings;
 import com.google.gerrit.common.ChangeHooks;
 import com.google.gerrit.extensions.restapi.AuthException;
+import com.google.gerrit.extensions.restapi.BadRequestException;
 import com.google.gerrit.extensions.restapi.ResourceConflictException;
 import com.google.gerrit.extensions.restapi.RestModifyView;
 import com.google.gerrit.reviewdb.client.Change;
@@ -26,25 +27,34 @@ import com.google.gerrit.server.ChangeUtil;
 import com.google.gerrit.server.GerritPersonIdent;
 import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.change.Revert.Input;
+import com.google.gerrit.server.config.CanonicalWebUrl;
 import com.google.gerrit.server.extensions.events.GitReferenceUpdated;
 import com.google.gerrit.server.git.GitRepositoryManager;
+import com.google.gerrit.server.git.validators.CommitValidators;
 import com.google.gerrit.server.mail.RevertedSender;
 import com.google.gerrit.server.patch.PatchSetInfoFactory;
 import com.google.gerrit.server.project.ChangeControl;
+import com.google.gerrit.server.project.InvalidChangeOperationException;
+import com.google.gerrit.server.ssh.NoSshInfo;
 import com.google.inject.Inject;
 import com.google.inject.Provider;
 
 import org.eclipse.jgit.lib.PersonIdent;
+import org.eclipse.jgit.lib.Repository;
+
+import javax.annotation.Nullable;
 
 public class Revert implements RestModifyView<ChangeResource, Input> {
   private final ChangeHooks hooks;
   private final RevertedSender.Factory revertedSenderFactory;
+  private final CommitValidators.Factory commitValidatorsFactory;
   private final Provider<ReviewDb> dbProvider;
   private final ChangeJson json;
   private final GitRepositoryManager gitManager;
   private final PersonIdent myIdent;
   private final PatchSetInfoFactory patchSetInfoFactory;
   private final GitReferenceUpdated replication;
+  private final String canonicalWebUrl;
 
   public static class Input {
     public String message;
@@ -53,20 +63,24 @@ public class Revert implements RestModifyView<ChangeResource, Input> {
   @Inject
   Revert(ChangeHooks hooks,
       RevertedSender.Factory revertedSenderFactory,
+      final CommitValidators.Factory commitValidatorsFactory,
       Provider<ReviewDb> dbProvider,
       ChangeJson json,
       GitRepositoryManager gitManager,
       final PatchSetInfoFactory patchSetInfoFactory,
       final GitReferenceUpdated replication,
-      @GerritPersonIdent final PersonIdent myIdent) {
+      @GerritPersonIdent final PersonIdent myIdent,
+      @CanonicalWebUrl @Nullable final String canonicalWebUrl) {
     this.hooks = hooks;
     this.revertedSenderFactory = revertedSenderFactory;
+    this.commitValidatorsFactory = commitValidatorsFactory;
     this.dbProvider = dbProvider;
     this.json = json;
     this.gitManager = gitManager;
     this.myIdent = myIdent;
     this.replication = replication;
     this.patchSetInfoFactory = patchSetInfoFactory;
+    this.canonicalWebUrl = canonicalWebUrl;
   }
 
   @Override
@@ -75,8 +89,7 @@ public class Revert implements RestModifyView<ChangeResource, Input> {
   }
 
   @Override
-  public Object apply(ChangeResource req, Input input)
+  public Object apply(ChangeResource req, Input input) throws Exception {
-      throws Exception {
     ChangeControl control = req.getControl();
     Change change = req.getChange();
     if (!control.canAddPatchSet()) {
@@ -85,14 +98,25 @@ public class Revert implements RestModifyView<ChangeResource, Input> {
       throw new ResourceConflictException("change is " + status(change));
     }
 
-    Change.Id revertedChangeId = ChangeUtil.revert(
+    final Repository git = gitManager.openRepository(control.getProject().getNameKey());
-        change.currentPatchSetId(),
+    try {
+      CommitValidators commitValidators =
+          commitValidatorsFactory.create(control.getRefControl(), new NoSshInfo(), git);
+
+      Change.Id revertedChangeId =
+          ChangeUtil.revert(control.getRefControl(), change.currentPatchSetId(),
               (IdentifiedUser) control.getCurrentUser(),
-        Strings.emptyToNull(input.message),
+              commitValidators,
-        dbProvider.get(),
+              Strings.emptyToNull(input.message), dbProvider.get(),
-        revertedSenderFactory, hooks, gitManager,
+              revertedSenderFactory, hooks, git, patchSetInfoFactory,
-        patchSetInfoFactory, replication, myIdent);
+              replication, myIdent, canonicalWebUrl);
+
       return json.format(revertedChangeId);
+    } catch (InvalidChangeOperationException e) {
+      throw new BadRequestException(e.getMessage());
+    } finally {
+      git.close();
+    }
   }
 
   private static String status(Change change) {

gerrit-server/src/main/java/com/google/gerrit/server/git/validators/CommitValidators.java

@@ -117,6 +117,36 @@ public class CommitValidators {
     return messages;
   }
 
+  public List<CommitValidationMessage> validateForRevertCommits(
+      CommitReceivedEvent receiveEvent) throws CommitValidationException {
+
+    List<CommitValidationListener> validators =
+        new LinkedList<CommitValidationListener>();
+
+    validators.add(new UploadMergesPermissionValidator(refControl));
+    validators.add(new AmendedGerritMergeCommitValidationListener(
+        refControl, gerritIdent));
+    validators.add(new AuthorUploaderValidator(refControl, canonicalWebUrl));
+    validators.add(new SignedOffByValidator(refControl, canonicalWebUrl));
+    validators.add(new ChangeIdValidator(refControl, canonicalWebUrl, sshInfo));
+    validators.add(new ConfigValidator(refControl, repo));
+    validators.add(new PluginCommitValidationListener(commitValidationListeners));
+
+    List<CommitValidationMessage> messages =
+        new LinkedList<CommitValidationMessage>();
+
+    try {
+      for (CommitValidationListener commitValidator : validators) {
+        messages.addAll(commitValidator.onCommitReceived(receiveEvent));
+      }
+    } catch (CommitValidationException e) {
+      // Keep the old messages (and their order) in case of an exception
+      messages.addAll(e.getMessages());
+      throw new CommitValidationException(e.getMessage(), messages);
+    }
+    return messages;
+  }
+
   public static class ChangeIdValidator implements CommitValidationListener {
     private final RefControl refControl;
     private final String canonicalWebUrl;