Fix: User could get around restrictions by reverting a commit

The Gerrit server may enforce several restrictions on the
commit message (change-id needed, signed-off-by, etc).
The user could get around these restrictions by reverting
a commit message using the UI.

Now the "Revert" feature is using the validation code
refactored out from ReceiveCommits, and correct validation
of the commit message is done before Gerrit accepts the new
commit.

Change-Id: I4700c2d31d4b7e263c793e3430841b29dbd69657

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 {
-        (IdentifiedUser) control.getCurrentUser(),
+      CommitValidators commitValidators =
-        Strings.emptyToNull(input.message),
+          commitValidatorsFactory.create(control.getRefControl(), new NoSshInfo(), git);
-        dbProvider.get(),
+
-        revertedSenderFactory, hooks, gitManager,
+      Change.Id revertedChangeId =
-        patchSetInfoFactory, replication, myIdent);
+          ChangeUtil.revert(control.getRefControl(), change.currentPatchSetId(),
-    return json.format(revertedChangeId);
+              (IdentifiedUser) control.getCurrentUser(),
+              commitValidators,
+              Strings.emptyToNull(input.message), dbProvider.get(),
+              revertedSenderFactory, hooks, git, patchSetInfoFactory,
+              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;