opendev/gerrit
Code Issues Proposed changes

Allow CommitValidationListener to ignore 'skip validation' push option

Browse Source 
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>
This commit is contained in:
Dariusz Luksza
2013-07-12 15:52:32 +02:00
committed by David Pursehouse
parent 082139a71d
commit 62ef2e5f26
5 changed files with 118 additions and 24 deletions
Download Patch File Download Diff File Expand all files Collapse all files

36
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java
View File

@@ -77,7 +77,6 @@ import com.google.gerrit.server.ChangeMessagesUtil;
import com.google.gerrit.server.events.CommitReceivedEvent;
import com.google.gerrit.server.git.ProjectConfig;
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.group.SystemGroupBackend;
@@ -1965,14 +1964,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)
throws CommitValidationException {
public List<CommitValidationMessage> onCommitReceived(CommitReceivedEvent receiveEvent) {
count.incrementAndGet();
return Collections.emptyList();
}
@Override
public boolean shouldValidateAllCommits() {
return validateAll;
}
public int count() {
return count.get();
}
@@ -1983,6 +1995,7 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest {
String master = "refs/heads/master";
TestValidator validator = new TestValidator();
RegistrationHandle handle = commitValidators.add(validator);
RegistrationHandle handle2 = null;
try {
// Validation listener is called on normal push
@@ -2009,8 +2022,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(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();
}
}
}

6
gerrit-extension-api/src/main/java/com/google/gerrit/extensions/registration/DynamicSet.java
View File

@@ -30,6 +30,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.
@@ -242,6 +244,10 @@ public class DynamicSet<T> implements Iterable<T> {
return new ReloadableHandle(ref, key, item);
}
public Stream<T> stream() {
return StreamSupport.stream(spliterator(), false);
}
private class ReloadableHandle implements ReloadableRegistrationHandle<T> {
private final AtomicReference<Provider<T>> ref;
private final Key<T> key;

56
gerrit-server/src/main/java/com/google/gerrit/server/git/receive/ReceiveCommits.java
View File

@@ -1029,7 +1029,7 @@ class ReceiveCommits {
actualCommands.add(cmd);
}
private void parseUpdate(ReceiveCommand cmd) throws PermissionBackendException {
private void parseUpdate(ReceiveCommand cmd) throws PermissionBackendException, IOException {
logDebug("Updating {}", cmd);
boolean ok;
try {
@@ -1101,7 +1101,7 @@ class ReceiveCommits {
}
}
private void parseRewind(ReceiveCommand cmd) throws PermissionBackendException {
private void parseRewind(ReceiveCommand cmd) throws PermissionBackendException, IOException {
RevCommit newObject;
try {
newObject = rp.getRevWalk().parseCommit(cmd.getNewId());
@@ -2710,26 +2710,33 @@ class ReceiveCommits {
}
private void validateNewCommits(Branch.NameKey branch, ReceiveCommand cmd)
throws PermissionBackendException {
throws PermissionBackendException, IOException {
PermissionBackend.ForRef perm = permissions.ref(branch.get());
if (!RefNames.REFS_CONFIG.equals(cmd.getRefName())
&& !(MagicBranch.isMagicBranch(cmd.getRefName())
|| NEW_PATCHSET_PATTERN.matcher(cmd.getRefName()).matches())
&& pushOptions.containsKey(PUSH_OPTION_SKIP_VALIDATION)) {
RevWalk walk = rp.getRevWalk();
boolean skipValidation =
!RefNames.REFS_CONFIG.equals(cmd.getRefName())
&& !(MagicBranch.isMagicBranch(cmd.getRefName())
|| NEW_PATCHSET_PATTERN.matcher(cmd.getRefName()).matches())
&& pushOptions.containsKey(PUSH_OPTION_SKIP_VALIDATION);
CommitValidators commitValidators =
commitValidatorsFactory.forReceiveCommits(
perm, branch, user, sshInfo, repo, walk, skipValidation);
if (skipValidation) {
try {
perm.check(RefPermission.SKIP_VALIDATION);
if (!Iterables.isEmpty(rejectCommits)) {
throw new AuthException("reject-commits prevents " + PUSH_OPTION_SKIP_VALIDATION);
}
logDebug("Short-circuiting new commit validation");
} catch (AuthException denied) {
reject(cmd, denied.getMessage());
}
return;
if (!commitValidators.hasAllCommitsValidators()) {
logDebug("Short-circuiting new commit validation");
return;
}
}
boolean missingFullName = Strings.isNullOrEmpty(user.getAccount().getFullName());
RevWalk walk = rp.getRevWalk();
walk.reset();
walk.sort(RevSort.NONE);
try {
@@ -2758,7 +2765,7 @@ class ReceiveCommits {
}
if (existing.keySet().contains(c)) {
continue;
} else if (!validCommit(walk, perm, branch, cmd, c)) {
} else if (!validCommit(commitValidators, walk, branch, cmd, c)) {
break;
}
@@ -2786,7 +2793,25 @@ class ReceiveCommits {
ReceiveCommand cmd,
ObjectId id)
throws IOException {
boolean isMerged =
magicBranch != null
&& cmd.getRefName().equals(magicBranch.cmd.getRefName())
&& magicBranch.merged;
CommitValidators validators =
isMerged
? commitValidatorsFactory.forMergedCommits(perm, user.asIdentifiedUser())
: commitValidatorsFactory.forReceiveCommits(
perm, branch, user.asIdentifiedUser(), sshInfo, repo, rw);
return validCommit(validators, rw, branch, cmd, id);
}
private boolean validCommit(
CommitValidators validators,
RevWalk rw,
Branch.NameKey branch,
ReceiveCommand cmd,
ObjectId id)
throws IOException {
if (validCommits.contains(id)) {
return true;
}
@@ -2796,15 +2821,6 @@ class ReceiveCommits {
try (CommitReceivedEvent receiveEvent =
new CommitReceivedEvent(cmd, project, branch.get(), rw.getObjectReader(), c, user)) {
boolean isMerged =
magicBranch != null
&& cmd.getRefName().equals(magicBranch.cmd.getRefName())
&& magicBranch.merged;
CommitValidators validators =
isMerged
? commitValidatorsFactory.forMergedCommits(perm, user.asIdentifiedUser())
: commitValidatorsFactory.forReceiveCommits(
perm, branch, user.asIdentifiedUser(), sshInfo, repo, rw);
for (CommitValidationMessage m : validators.validate(receiveEvent)) {
messages.add(new CommitValidationMessage(messageForCommit(c, m.getMessage()), m.isError()));
}

10
gerrit-server/src/main/java/com/google/gerrit/server/git/validators/CommitValidationListener.java
View File

@@ -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;
}
}

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

@@ -119,6 +119,18 @@ public class CommitValidators {
Repository repo,
RevWalk rw)
throws IOException {
return forReceiveCommits(perm, branch, user, sshInfo, repo, rw, false);
}
public CommitValidators forReceiveCommits(
PermissionBackend.ForRef perm,
Branch.NameKey branch,
IdentifiedUser user,
SshInfo sshInfo,
Repository repo,
RevWalk rw,
boolean skipValidation)
throws IOException {
NoteMap rejectCommits = BanCommit.loadRejectCommitsMap(repo, rw);
ProjectState projectState = projectCache.checkedGet(branch.getParentKey());
return new CommitValidators(
@@ -132,7 +144,7 @@ public class CommitValidators {
projectState, user, canonicalWebUrl, installCommitMsgHookCommand, sshInfo),
new ConfigValidator(branch, user, rw, allUsers),
new BannedCommitsValidator(rejectCommits),
new PluginCommitValidationListener(pluginValidators),
new PluginCommitValidationListener(pluginValidators, skipValidation),
new ExternalIdUpdateListener(allUsers, externalIdsConsistencyChecker),
new AccountCommitValidator(allUsers, accountValidator)));
}
@@ -206,6 +218,10 @@ public class CommitValidators {
return messages;
}
public boolean hasAllCommitsValidators() {
return validators.stream().anyMatch(v -> v.shouldValidateAllCommits());
}
public static class ChangeIdValidator implements CommitValidationListener {
private static final String CHANGE_ID_PREFIX = FooterConstants.CHANGE_ID.getName() + ":";
private static final String MISSING_CHANGE_ID_MSG = "missing Change-Id in message footer";
@@ -444,10 +460,18 @@ public class CommitValidators {
/** Execute commit validation plug-ins */
public static class PluginCommitValidationListener implements CommitValidationListener {
private boolean skipValidation;
private final DynamicSet<CommitValidationListener> commitValidationListeners;
public PluginCommitValidationListener(
final DynamicSet<CommitValidationListener> commitValidationListeners) {
this(commitValidationListeners, false);
}
public PluginCommitValidationListener(
final DynamicSet<CommitValidationListener> commitValidationListeners,
boolean skipValidation) {
this.skipValidation = skipValidation;
this.commitValidationListeners = commitValidationListeners;
}
@@ -457,6 +481,9 @@ public class CommitValidators {
List<CommitValidationMessage> messages = new ArrayList<>();
for (CommitValidationListener validator : commitValidationListeners) {
if (skipValidation && !validator.shouldValidateAllCommits()) {
continue;
}
try {
messages.addAll(validator.onCommitReceived(receiveEvent));
} catch (CommitValidationException e) {
@@ -466,6 +493,11 @@ public class CommitValidators {
}
return messages;
}
@Override
public boolean shouldValidateAllCommits() {
return commitValidationListeners.stream().anyMatch(v -> v.shouldValidateAllCommits());
}
}
public static class SignedOffByValidator implements CommitValidationListener {