opendev/gerrit
Code Issues Proposed changes

Merge branch 'stable-2.16' into stable-3.0

Browse Source 
* stable-2.16:
  ElasticVersionTest: Add missing version 7.1 asserts
  ElasticVersionTest: Improve test method ordering
  ElasticVersionTest: Make test method names accurate
  ReceiveCommits: Validate ref operation before commits for non-ff case
  Add test coverage for ref operation validation extension point
  Allow CommitValidationListener to ignore 'skip validation' push option
  Upgrade JGit to 4.11.8.201904181247-r
  CreateProject: Expose createProject method
  ElasticContainer: Switch to Alpine-based images from blacktop
  DeleteVote.soy: Make wording consistent with DeleteVoteHtml.soy
  DeleteVote.soy add missing review URL
  NewChange.soy: add missing closing parenthesis
  Update git submodules
  gerrit.sh: Fix message about JRE
  Allow CommitValidationListener to ignore 'skip validation' push option
  Bazel: Fix lint warning flagged by buildifier

Change-Id: I5ca13ae4495265592ac07381bc97370ec025d3f5
This commit is contained in:
Marco Miller
2019-06-26 16:32:03 -04:00
parent 7797359e10 e7b44a3fbe
commit ff50a286ed
18 changed files with 366 additions and 64 deletions
Download Patch File Download Diff File Expand all files Collapse all files

6
java/com/google/gerrit/extensions/registration/DynamicSet.java
View File

@@ -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.
@@ -284,6 +286,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;

26
java/com/google/gerrit/server/git/receive/BranchCommitValidator.java
View File

@@ -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)) {

34
java/com/google/gerrit/server/git/receive/ReceiveCommits.java
View File

@@ -1298,11 +1298,8 @@ class ReceiveCommits {
}
private void parseRewind(ReceiveCommand cmd) throws PermissionBackendException {
RevCommit newObject;
try {
newObject = receivePack.getRevWalk().parseCommit(cmd.getNewId());
} catch (IncorrectObjectTypeException notCommit) {
newObject = null;
receivePack.getRevWalk().parseCommit(cmd.getNewId());
} catch (IOException err) {
logger.atSevere().withCause(err).log(
"Invalid object %s for %s forced update", cmd.getNewId().name(), cmd.getRefName());
@@ -1311,11 +1308,12 @@ class ReceiveCommits {
}
logger.atFine().log("Rewinding %s", cmd);
if (newObject != null) {
validateRegularPushCommits(new Branch.NameKey(project.getNameKey(), cmd.getRefName()), cmd);
if (cmd.getResult() != NOT_ATTEMPTED) {
return;
}
if (!validRefOperation(cmd)) {
return;
}
validateRegularPushCommits(new Branch.NameKey(project.getNameKey(), cmd.getRefName()), cmd);
if (cmd.getResult() != NOT_ATTEMPTED) {
return;
}
Optional<AuthException> err = checkRefPermission(cmd, RefPermission.FORCE_UPDATE);
@@ -3064,10 +3062,12 @@ class ReceiveCommits {
*/
private void validateRegularPushCommits(Branch.NameKey branch, ReceiveCommand cmd)
throws PermissionBackendException {
if (!RefNames.REFS_CONFIG.equals(cmd.getRefName())
&& !(MagicBranch.isMagicBranch(cmd.getRefName())
|| NEW_PATCHSET_PATTERN.matcher(cmd.getRefName()).matches())
&& pushOptions.containsKey(PUSH_OPTION_SKIP_VALIDATION)) {
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);
if (skipValidation) {
if (projectState.is(BooleanProjectConfig.USE_SIGNED_OFF_BY)) {
reject(cmd, "requireSignedOffBy prevents option " + PUSH_OPTION_SKIP_VALIDATION);
return;
@@ -3082,11 +3082,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);
@@ -3101,7 +3098,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,
@@ -3113,8 +3110,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;
}
}

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

32
java/com/google/gerrit/server/git/validators/CommitValidators.java
View File

@@ -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(projectConfigFactory, 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)));
@@ -476,27 +477,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)),
CommitValidationException.class);
l -> runValidator(l, messages, receiveEvent), 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 {

5
java/com/google/gerrit/server/plugincontext/PluginSetContext.java
View File

@@ -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.