Allow CommitValidationListener to ignore 'skip validation' push option

This is a reworked version of the change that was done on stable-2.15,
adjusted to work with refactoring that was done on stable-2.16.

Gerrit allows users to skip validation of commits on push. Prior to
version 2.15, validation was implicitly skipped when the following
criteria were met:

- The user has the 'push merges', 'forge author', 'forge committer',
  and 'forge server identity' permissions
- The project does not require the 'Signed-off-by' footer

This was originally introduced to improve performance on initial push
of content into a new project, for example when a project is imported
from an external source.

Since changes I80ad47852 and I012e1ea42, introduced in 2.15, skipping
validation is only done when the user (with the permissions) explicitly
specifies the 'skip-validation' push option.

Add a new method on the CommitValidationListener interface, allowing the
validator implementation to override the 'skip-validation' option so
that it gets invoked for all commits. This is useful in companies where
specific validations must be performed for all incoming commits, for
example for security or auditing purposes, regardless of their origin.

For backwards compatibility the new method has a default implementation
that returns false, so that validation is skipped by default as before,
and existing implementations don't need to be adjusted. Validation is
only invoked for the implementations that override the method to return
'true'; other validators are skipped as before.

Change-Id: I7ba197eac4b8edad7a87f1d3d961948f988db60f
Signed-off-by: Dariusz Luksza <dariusz@luksza.org>
Signed-off-by: David Pursehouse <dpursehouse@collab.net>

java/com/google/gerrit/extensions/registration/DynamicSet.java

@@ -36,6 +36,8 @@ import java.util.Iterator;
 import java.util.NoSuchElementException;
 import java.util.concurrent.CopyOnWriteArrayList;
 import java.util.concurrent.atomic.AtomicReference;
+import java.util.stream.Stream;
+import java.util.stream.StreamSupport;
 
 /**
  * A set of members that can be modified as plugins reload.
@@ -291,6 +293,10 @@ public class DynamicSet<T> implements Iterable<T> {
     return new ReloadableHandle(ref, key, ref.get());
   }
 
+  public Stream<T> stream() {
+    return StreamSupport.stream(spliterator(), false);
+  }
+
   private class ReloadableHandle implements ReloadableRegistrationHandle<T> {
     private final AtomicReference<Extension<T>> ref;
     private final Key<T> key;

java/com/google/gerrit/server/git/receive/BranchCommitValidator.java

@@ -90,6 +90,29 @@ public class BranchCommitValidator {
       NoteMap rejectCommits,
       @Nullable Change change)
       throws IOException {
+    return validCommit(objectReader, cmd, commit, isMerged, messages, rejectCommits, change, false);
+  }
+
+  /**
+   * Validates a single commit. If the commit does not validate, the command is rejected.
+   *
+   * @param objectReader the object reader to use.
+   * @param cmd the ReceiveCommand executing the push.
+   * @param commit the commit being validated.
+   * @param isMerged whether this is a merge commit created by magicBranch --merge option
+   * @param change the change for which this is a new patchset.
+   * @param skipValidation whether 'skip-validation' was requested.
+   */
+  public boolean validCommit(
+      ObjectReader objectReader,
+      ReceiveCommand cmd,
+      RevCommit commit,
+      boolean isMerged,
+      List<ValidationMessage> messages,
+      NoteMap rejectCommits,
+      @Nullable Change change,
+      boolean skipValidation)
+      throws IOException {
     try (CommitReceivedEvent receiveEvent =
         new CommitReceivedEvent(cmd, project, branch.get(), objectReader, commit, user)) {
       CommitValidators validators;
@@ -105,7 +128,8 @@ public class BranchCommitValidator {
                 sshInfo,
                 rejectCommits,
                 receiveEvent.revWalk,
-                change);
+                change,
+                skipValidation);
       }
 
       for (CommitValidationMessage m : validators.validate(receiveEvent)) {

java/com/google/gerrit/server/git/receive/ReceiveCommits.java

@@ -3013,10 +3013,12 @@ class ReceiveCommits {
    */
   private void validateRegularPushCommits(Branch.NameKey branch, ReceiveCommand cmd)
       throws PermissionBackendException {
-    if (!RefNames.REFS_CONFIG.equals(cmd.getRefName())
+    boolean skipValidation =
-        && !(MagicBranch.isMagicBranch(cmd.getRefName())
+        !RefNames.REFS_CONFIG.equals(cmd.getRefName())
-            || NEW_PATCHSET_PATTERN.matcher(cmd.getRefName()).matches())
+            && !(MagicBranch.isMagicBranch(cmd.getRefName())
-        && pushOptions.containsKey(PUSH_OPTION_SKIP_VALIDATION)) {
+                || NEW_PATCHSET_PATTERN.matcher(cmd.getRefName()).matches())
+            && pushOptions.containsKey(PUSH_OPTION_SKIP_VALIDATION);
+    if (skipValidation) {
       if (projectState.is(BooleanProjectConfig.USE_SIGNED_OFF_BY)) {
         reject(cmd, "requireSignedOffBy prevents option " + PUSH_OPTION_SKIP_VALIDATION);
         return;
@@ -3031,11 +3033,8 @@ class ReceiveCommits {
       if (!Iterables.isEmpty(rejectCommits)) {
         reject(cmd, "reject-commits prevents " + PUSH_OPTION_SKIP_VALIDATION);
       }
-      logger.atFine().log("Short-circuiting new commit validation");
-      return;
     }
 
-    BranchCommitValidator validator = commitValidatorFactory.create(projectState, branch, user);
     RevWalk walk = receivePack.getRevWalk();
     walk.reset();
     walk.sort(RevSort.NONE);
@@ -3050,7 +3049,7 @@ class ReceiveCommits {
       int limit = receiveConfig.maxBatchCommits;
       int n = 0;
       for (RevCommit c; (c = walk.next()) != null; ) {
-        if (++n > limit) {
+        if (++n > limit && !skipValidation) {
           logger.atFine().log("Number of new commits exceeds limit of %d", limit);
           reject(
               cmd,
@@ -3062,8 +3061,9 @@ class ReceiveCommits {
           continue;
         }
 
+        BranchCommitValidator validator = commitValidatorFactory.create(projectState, branch, user);
         if (!validator.validCommit(
-            walk.getObjectReader(), cmd, c, false, messages, rejectCommits, null)) {
+            walk.getObjectReader(), cmd, c, false, messages, rejectCommits, null, skipValidation)) {
           break;
         }
       }

java/com/google/gerrit/server/git/validators/CommitValidationListener.java

@@ -35,4 +35,14 @@ public interface CommitValidationListener {
    */
   List<CommitValidationMessage> onCommitReceived(CommitReceivedEvent receiveEvent)
       throws CommitValidationException;
+
+  /**
+   * Whether this validator should validate all commits.
+   *
+   * @return {@code true} if this validator should validate all commits, even when the {@code
+   *     skip-validation} push option was specified.
+   */
+  default boolean shouldValidateAllCommits() {
+    return false;
+  }
 }

java/com/google/gerrit/server/git/validators/CommitValidators.java

@@ -132,7 +132,8 @@ public class CommitValidators {
         SshInfo sshInfo,
         NoteMap rejectCommits,
         RevWalk rw,
-        @Nullable Change change)
+        @Nullable Change change,
+        boolean skipValidation)
         throws IOException {
       PermissionBackend.ForRef perm = forProject.ref(branch.get());
       ProjectState projectState = projectCache.checkedGet(branch.getParentKey());
@@ -153,7 +154,7 @@ public class CommitValidators {
                   change),
               new ConfigValidator(branch, user, rw, allUsers, allProjects),
               new BannedCommitsValidator(rejectCommits),
-              new PluginCommitValidationListener(pluginValidators),
+              new PluginCommitValidationListener(pluginValidators, skipValidation),
               new ExternalIdUpdateListener(allUsers, externalIdsConsistencyChecker),
               new AccountCommitValidator(repoManager, allUsers, accountValidator),
               new GroupCommitValidator(allUsers)));
@@ -474,27 +475,50 @@ public class CommitValidators {
 
   /** Execute commit validation plug-ins */
   public static class PluginCommitValidationListener implements CommitValidationListener {
+    private boolean skipValidation;
     private final PluginSetContext<CommitValidationListener> commitValidationListeners;
 
     public PluginCommitValidationListener(
         final PluginSetContext<CommitValidationListener> commitValidationListeners) {
+      this(commitValidationListeners, false);
+    }
+
+    public PluginCommitValidationListener(
+        final PluginSetContext<CommitValidationListener> commitValidationListeners,
+        boolean skipValidation) {
+      this.skipValidation = skipValidation;
       this.commitValidationListeners = commitValidationListeners;
     }
 
+    private void runValidator(
+        CommitValidationListener validator,
+        List<CommitValidationMessage> messages,
+        CommitReceivedEvent receiveEvent)
+        throws CommitValidationException {
+      if (skipValidation && !validator.shouldValidateAllCommits()) {
+        return;
+      }
+      messages.addAll(validator.onCommitReceived(receiveEvent));
+    }
+
     @Override
     public List<CommitValidationMessage> onCommitReceived(CommitReceivedEvent receiveEvent)
         throws CommitValidationException {
       List<CommitValidationMessage> messages = new ArrayList<>();
       try {
         commitValidationListeners.runEach(
-            l -> messages.addAll(l.onCommitReceived(receiveEvent)),
+            l -> runValidator(l, messages, receiveEvent), CommitValidationException.class);
-            CommitValidationException.class);
       } catch (CommitValidationException e) {
         messages.addAll(e.getMessages());
         throw new CommitValidationException(e.getMessage(), messages);
       }
       return messages;
     }
+
+    @Override
+    public boolean shouldValidateAllCommits() {
+      return commitValidationListeners.stream().anyMatch(v -> v.shouldValidateAllCommits());
+    }
   }
 
   public static class SignedOffByValidator implements CommitValidationListener {

java/com/google/gerrit/server/plugincontext/PluginSetContext.java

@@ -23,6 +23,7 @@ import com.google.gerrit.server.plugincontext.PluginContext.PluginMetrics;
 import com.google.inject.Inject;
 import java.util.Iterator;
 import java.util.SortedSet;
+import java.util.stream.Stream;
 
 /**
  * Context to invoke extensions from a {@link DynamicSet}.
@@ -147,6 +148,10 @@ public class PluginSetContext<T> implements Iterable<PluginSetEntryContext<T>> {
         .forEach(p -> PluginContext.runLogExceptions(pluginMetrics, p, extensionImplConsumer));
   }
 
+  public Stream<T> stream() {
+    return dynamicSet.stream();
+  }
+
   /**
    * Invokes each extension in the set. All exceptions from the plugin extensions except exceptions
    * of the specified type are caught and logged.

javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java

@@ -92,7 +92,6 @@ import com.google.gerrit.server.ChangeMessagesUtil;
 import com.google.gerrit.server.events.CommitReceivedEvent;
 import com.google.gerrit.server.git.receive.NoteDbPushOption;
 import com.google.gerrit.server.git.receive.ReceiveConstants;
-import com.google.gerrit.server.git.validators.CommitValidationException;
 import com.google.gerrit.server.git.validators.CommitValidationListener;
 import com.google.gerrit.server.git.validators.CommitValidationMessage;
 import com.google.gerrit.server.git.validators.CommitValidators.ChangeIdValidator;
@@ -2268,14 +2267,27 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest {
 
   private static class TestValidator implements CommitValidationListener {
     private final AtomicInteger count = new AtomicInteger();
+    private final boolean validateAll;
+
+    TestValidator(boolean validateAll) {
+      this.validateAll = validateAll;
+    }
+
+    TestValidator() {
+      this(false);
+    }
 
     @Override
-    public List<CommitValidationMessage> onCommitReceived(CommitReceivedEvent receiveEvent)
+    public List<CommitValidationMessage> onCommitReceived(CommitReceivedEvent receiveEvent) {
-        throws CommitValidationException {
       count.incrementAndGet();
       return Collections.emptyList();
     }
 
+    @Override
+    public boolean shouldValidateAllCommits() {
+      return validateAll;
+    }
+
     public int count() {
       return count.get();
     }
@@ -2286,6 +2298,7 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest {
     String master = "refs/heads/master";
     TestValidator validator = new TestValidator();
     RegistrationHandle handle = commitValidators.add("test-validator", validator);
+    RegistrationHandle handle2 = null;
 
     try {
       // Validation listener is called on normal push
@@ -2312,8 +2325,25 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest {
       r = push3.to(master);
       r.assertOkStatus();
       assertThat(validator.count()).isEqualTo(1);
+
+      // Validation listener that needs to validate all commits gets called even
+      // when the skip option is used.
+      TestValidator validator2 = new TestValidator(true);
+      handle2 = commitValidators.add("test-validator-2", validator2);
+      PushOneCommit push4 =
+          pushFactory.create(db, admin.getIdent(), testRepo, "change2", "b.txt", "content");
+      push4.setPushOptions(ImmutableList.of(PUSH_OPTION_SKIP_VALIDATION));
+      r = push4.to(master);
+      r.assertOkStatus();
+      // First listener was not called; its count remains the same.
+      assertThat(validator.count()).isEqualTo(1);
+      // Second listener was called.
+      assertThat(validator2.count()).isEqualTo(1);
     } finally {
       handle.remove();
+      if (handle2 != null) {
+        handle2.remove();
+      }
     }
   }