From da2034f1a357f54237203975e68a47255318e1f0 Mon Sep 17 00:00:00 2001 From: Andrii Shyshkalov Date: Thu, 19 Jan 2017 13:44:16 -0800 Subject: [PATCH] New extension to validate branch updates by submit strategies. Adds new extension point to validate a branch tip update when a change is being submitted by submit strategy. As submit strategies may generate new commits (e.g. Cherry Pick), this listener allows validation of resulting new commit before branch is updated, potentially aborting the submission. Before, only MergeValidationListener.onPreMerge was available to validate changes being submitted. However, onPreMerge was called before submit strategies were run, thus commits generated by submit strategies were previously impossible to validate. Now, OnSubmitValidationListener.preBranchUpdate can be used to abort submission before any refs are updated, even if there are several changes submitted together. Change-Id: I2ab75c7c38eb8ed0c010507b49f40e1b5cebad3b --- Documentation/config-validation.txt | 12 ++ .../rest/change/AbstractSubmit.java | 114 ++++++++++++++++++ .../server/config/GerritGlobalModule.java | 4 + .../google/gerrit/server/git/BatchUpdate.java | 23 ++++ .../gerrit/server/git/MergeOpRepoManager.java | 9 +- .../server/git/strategy/SubmitStrategy.java | 4 + .../OnSubmitValidationListener.java | 85 +++++++++++++ .../git/validators/OnSubmitValidators.java | 53 ++++++++ 8 files changed, 302 insertions(+), 2 deletions(-) create mode 100644 gerrit-server/src/main/java/com/google/gerrit/server/git/validators/OnSubmitValidationListener.java create mode 100644 gerrit-server/src/main/java/com/google/gerrit/server/git/validators/OnSubmitValidators.java diff --git a/Documentation/config-validation.txt b/Documentation/config-validation.txt index cccfe5c1c1..0d0717e174 100644 --- a/Documentation/config-validation.txt +++ b/Documentation/config-validation.txt @@ -45,6 +45,18 @@ are merged to the git repository. If the commit fails the validation, the plugin can throw an exception which will cause the merge to fail. +[[on-submit-validation]] +== On submit validation + + +Plugins implementing the `OnSubmitValidationListener` interface can +perform additional validation checks against ref operations resuling +from execution of submit operation before they are applied to any git +repositories (there could be more than one in case of topic submits). + +Plugin can throw an exception which will cause submit operation to be +aborted. + [[pre-upload-validation]] == Pre-upload validation diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java index 45da3d8ba7..2a39ffd18c 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java @@ -29,6 +29,7 @@ import static org.junit.Assert.fail; import com.google.common.collect.ImmutableList; import com.google.common.collect.Iterables; import com.google.common.collect.Lists; +import com.google.common.collect.Sets; import com.google.gerrit.acceptance.AbstractDaemonTest; import com.google.gerrit.acceptance.NoHttpd; import com.google.gerrit.acceptance.PushOneCommit; @@ -45,6 +46,8 @@ import com.google.gerrit.extensions.client.ListChangesOption; import com.google.gerrit.extensions.client.SubmitType; import com.google.gerrit.extensions.common.ChangeInfo; import com.google.gerrit.extensions.common.LabelInfo; +import com.google.gerrit.extensions.registration.DynamicSet; +import com.google.gerrit.extensions.registration.RegistrationHandle; import com.google.gerrit.extensions.restapi.AuthException; import com.google.gerrit.extensions.restapi.BinaryResult; import com.google.gerrit.extensions.restapi.ResourceConflictException; @@ -56,6 +59,7 @@ import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gerrit.reviewdb.client.PatchSetApproval; import com.google.gerrit.reviewdb.client.Project; +import com.google.gerrit.reviewdb.client.RefNames; import com.google.gerrit.server.ApprovalsUtil; import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.change.RevisionResource; @@ -63,8 +67,10 @@ import com.google.gerrit.server.change.Submit; import com.google.gerrit.server.git.BatchUpdate; import com.google.gerrit.server.git.BatchUpdate.ChangeContext; import com.google.gerrit.server.git.ProjectConfig; +import com.google.gerrit.server.git.validators.OnSubmitValidationListener; import com.google.gerrit.server.notedb.ChangeNotes; import com.google.gerrit.server.project.Util; +import com.google.gerrit.server.validators.ValidationException; import com.google.gerrit.testutil.ConfigSuite; import com.google.gerrit.testutil.TestTimeUtil; import com.google.gwtorm.server.OrmException; @@ -86,9 +92,13 @@ import org.junit.Before; import org.junit.Test; import java.io.ByteArrayOutputStream; +import java.io.IOException; +import java.util.ArrayList; import java.util.HashMap; +import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.concurrent.atomic.AtomicBoolean; import java.util.stream.Collectors; @NoHttpd @@ -110,6 +120,10 @@ public abstract class AbstractSubmit extends AbstractDaemonTest { @Inject private BatchUpdate.Factory updateFactory; + @Inject + private DynamicSet onSubmitValidationListeners; + private RegistrationHandle onSubmitValidatorHandle; + private String systemTimeZone; @Before @@ -129,6 +143,13 @@ public abstract class AbstractSubmit extends AbstractDaemonTest { db.close(); } + @After + public void removeOnSubmitValidator(){ + if (onSubmitValidatorHandle != null){ + onSubmitValidatorHandle.remove(); + } + } + protected abstract SubmitType getSubmitType(); @Test @@ -647,6 +668,94 @@ public abstract class AbstractSubmit extends AbstractDaemonTest { assertThat(getRemoteHead()).isEqualTo(headAfterSubmit); } + @Test + public void submitWithValidation() throws Exception { + AtomicBoolean called = new AtomicBoolean(false); + this.addOnSubmitValidationListener(new OnSubmitValidationListener() { + @Override + public void preBranchUpdate(Arguments args) throws ValidationException { + called.set(true); + HashSet refs = Sets.newHashSet(args.getCommands().keySet()); + assertThat(refs).contains("refs/heads/master"); + refs.remove("refs/heads/master"); + if (!refs.isEmpty()){ + // Some submit strategies need to insert new patchset. + assertThat(refs).hasSize(1); + assertThat(refs.iterator().next()).startsWith(RefNames.REFS_CHANGES); + } + } + }); + + PushOneCommit.Result change = createChange(); + approve(change.getChangeId()); + submit(change.getChangeId()); + assertThat(called.get()).isTrue(); + } + + @Test + public void submitWithValidationMultiRepo() throws Exception { + assume().that(isSubmitWholeTopicEnabled()).isTrue(); + String topic = "test-topic"; + + // Create test projects + TestRepository repoA = + createProjectWithPush("project-a", null, getSubmitType()); + TestRepository repoB = + createProjectWithPush("project-b", null, getSubmitType()); + + // Create changes on project-a + PushOneCommit.Result change1 = + createChange(repoA, "master", "Change 1", "a.txt", "content", topic); + PushOneCommit.Result change2 = + createChange(repoA, "master", "Change 2", "b.txt", "content", topic); + + // Create changes on project-b + PushOneCommit.Result change3 = + createChange(repoB, "master", "Change 3", "a.txt", "content", topic); + PushOneCommit.Result change4 = + createChange(repoB, "master", "Change 4", "b.txt", "content", topic); + + List changes = + Lists.newArrayList(change1, change2, change3, change4); + for (PushOneCommit.Result change : changes) { + approve(change.getChangeId()); + } + + // Construct validator which will throw on a second call. + // Since there are 2 repos, first submit attempt will fail, the second will + // succeed. + List projectsCalled = new ArrayList<>(4); + this.addOnSubmitValidationListener(new OnSubmitValidationListener() { + @Override + public void preBranchUpdate(Arguments args) throws ValidationException { + assertThat(args.getCommands().keySet()).contains("refs/heads/master"); + try (RevWalk rw = args.newRevWalk()) { + rw.parseBody(rw.parseCommit( + args.getCommands().get("refs/heads/master").getNewId())); + } catch (IOException e) { + assertThat(e).isNull(); + } + projectsCalled.add(args.getProject().get()); + if (projectsCalled.size() == 2) { + throw new ValidationException("time to fail"); + } + } + }); + submitWithConflict(change4.getChangeId(), "time to fail"); + assertThat(projectsCalled).containsExactly(name("project-a"), + name("project-b")); + for (PushOneCommit.Result change : changes) { + change.assertChange(Change.Status.NEW, name(topic), admin); + } + + submit(change4.getChangeId()); + assertThat(projectsCalled).containsExactly(name("project-a"), + name("project-b"), name("project-a"), name("project-b")); + for (PushOneCommit.Result change : changes) { + change.assertChange(Change.Status.MERGED, name(topic), admin); + } + } + private void setChangeStatusToNew(PushOneCommit.Result... changes) throws Exception { for (PushOneCommit.Result change : changes) { @@ -881,6 +990,11 @@ public abstract class AbstractSubmit extends AbstractDaemonTest { return getRemoteLog(project, "master"); } + protected void addOnSubmitValidationListener(OnSubmitValidationListener listener){ + assertThat(onSubmitValidatorHandle).isNull(); + onSubmitValidatorHandle = onSubmitValidationListeners.add(listener); + } + private String getLatestDiff(Repository repo) throws Exception { ObjectId oldTreeId = repo.resolve("HEAD~1^{tree}"); ObjectId newTreeId = repo.resolve("HEAD^{tree}"); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritGlobalModule.java b/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritGlobalModule.java index df96a54ee9..6012142b25 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritGlobalModule.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritGlobalModule.java @@ -128,6 +128,8 @@ import com.google.gerrit.server.git.validators.MergeValidators; import com.google.gerrit.server.git.validators.MergeValidators.ProjectConfigValidator; import com.google.gerrit.server.git.validators.RefOperationValidationListener; import com.google.gerrit.server.git.validators.RefOperationValidators; +import com.google.gerrit.server.git.validators.OnSubmitValidationListener; +import com.google.gerrit.server.git.validators.OnSubmitValidators; import com.google.gerrit.server.git.validators.UploadValidationListener; import com.google.gerrit.server.git.validators.UploadValidators; import com.google.gerrit.server.group.GroupInfoCache; @@ -350,6 +352,7 @@ public class GerritGlobalModule extends FactoryModule { DynamicSet.setOf(binder(), CommitValidationListener.class); DynamicSet.setOf(binder(), ChangeMessageModifier.class); DynamicSet.setOf(binder(), RefOperationValidationListener.class); + DynamicSet.setOf(binder(), OnSubmitValidationListener.class); DynamicSet.setOf(binder(), MergeValidationListener.class); DynamicSet.setOf(binder(), ProjectCreationValidationListener.class); DynamicSet.setOf(binder(), GroupCreationValidationListener.class); @@ -391,6 +394,7 @@ public class GerritGlobalModule extends FactoryModule { factory(AbandonOp.Factory.class); factory(RefOperationValidators.Factory.class); + factory(OnSubmitValidators.Factory.class); factory(MergeValidators.Factory.class); factory(ProjectConfigValidator.Factory.class); factory(NotesBranchUtil.Factory.class); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/BatchUpdate.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/BatchUpdate.java index 5de4ff50a9..4b1ffeb72b 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/BatchUpdate.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/BatchUpdate.java @@ -51,6 +51,7 @@ import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.config.AllUsersName; import com.google.gerrit.server.config.GerritServerConfig; import com.google.gerrit.server.extensions.events.GitReferenceUpdated; +import com.google.gerrit.server.git.validators.OnSubmitValidators; import com.google.gerrit.server.index.change.ChangeIndexer; import com.google.gerrit.server.notedb.ChangeNotes; import com.google.gerrit.server.notedb.ChangeUpdate; @@ -532,6 +533,7 @@ public class BatchUpdate implements AutoCloseable { private BatchRefUpdate batchRefUpdate; private boolean closeRepo; private Order order; + private OnSubmitValidators onSubmitValidators; private boolean updateChangesInParallel; private RequestId requestId; @@ -606,6 +608,15 @@ public class BatchUpdate implements AutoCloseable { return this; } + /** + * Add a validation step for intended ref operations, which will be performed + * at the end of {@link RepoOnlyOp#updateRepo(RepoContext)} step. + */ + BatchUpdate setOnSubmitValidators(OnSubmitValidators onSubmitValidators) { + this.onSubmitValidators = onSubmitValidators; + return this; + } + /** * Execute {@link Op#updateChange(ChangeContext)} in parallel for each change. */ @@ -692,6 +703,18 @@ public class BatchUpdate implements AutoCloseable { op.updateRepo(ctx); } + if (onSubmitValidators != null && commands != null + && !commands.isEmpty()) { + // Validation of refs has to take place here and not at the beginning + // executeRefUpdates. Otherwise failing validation in a second + // BatchUpdate object will happen *after* first object's + // executeRefUpdates has finished, hence after first repo's refs have + // been updated, which is too late. + onSubmitValidators.validate(project, + new ReadOnlyRepository(getRepository()), + ctx.getInserter().newReader(), commands.getCommands()); + } + if (inserter != null) { logDebug("Flushing inserter"); inserter.flush(); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOpRepoManager.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOpRepoManager.java index 5196ebe4b8..1244ad39cc 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOpRepoManager.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOpRepoManager.java @@ -22,6 +22,7 @@ import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.git.CodeReviewCommit.CodeReviewRevWalk; +import com.google.gerrit.server.git.validators.OnSubmitValidators; import com.google.gerrit.server.project.NoSuchProjectException; import com.google.gerrit.server.project.ProjectCache; import com.google.gerrit.server.project.ProjectState; @@ -107,7 +108,8 @@ public class MergeOpRepoManager implements AutoCloseable { if (update == null) { update = batchUpdateFactory.create(db, getProjectName(), caller, ts) .setRepository(repo, rw, ins) - .setRequestId(submissionId); + .setRequestId(submissionId) + .setOnSubmitValidators(onSubmitValidatorsFactory.create()); } return update; } @@ -157,6 +159,7 @@ public class MergeOpRepoManager implements AutoCloseable { private final Map openRepos; private final BatchUpdate.Factory batchUpdateFactory; + private final OnSubmitValidators.Factory onSubmitValidatorsFactory; private final GitRepositoryManager repoManager; private final ProjectCache projectCache; @@ -169,10 +172,12 @@ public class MergeOpRepoManager implements AutoCloseable { MergeOpRepoManager( GitRepositoryManager repoManager, ProjectCache projectCache, - BatchUpdate.Factory batchUpdateFactory) { + BatchUpdate.Factory batchUpdateFactory, + OnSubmitValidators.Factory onSubmitValidatorsFactory) { this.repoManager = repoManager; this.projectCache = projectCache; this.batchUpdateFactory = batchUpdateFactory; + this.onSubmitValidatorsFactory = onSubmitValidatorsFactory; openRepos = new HashMap<>(); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategy.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategy.java index e0e2ae7212..692bc98624 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategy.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategy.java @@ -46,6 +46,7 @@ import com.google.gerrit.server.git.MergeTip; import com.google.gerrit.server.git.MergeUtil; import com.google.gerrit.server.git.SubmoduleOp; import com.google.gerrit.server.git.TagCache; +import com.google.gerrit.server.git.validators.OnSubmitValidators; import com.google.gerrit.server.patch.PatchSetInfoFactory; import com.google.gerrit.server.project.ChangeControl; import com.google.gerrit.server.project.ProjectCache; @@ -118,6 +119,7 @@ public abstract class SubmitStrategy { final ProjectCache projectCache; final PersonIdent serverIdent; final RebaseChangeOp.Factory rebaseFactory; + final OnSubmitValidators.Factory onSubmitValidatorsFactory; final TagCache tagCache; final Branch.NameKey destBranch; @@ -158,6 +160,7 @@ public abstract class SubmitStrategy { @GerritPersonIdent PersonIdent serverIdent, ProjectCache projectCache, RebaseChangeOp.Factory rebaseFactory, + OnSubmitValidators.Factory onSubmitValidatorsFactory, TagCache tagCache, @Assisted Branch.NameKey destBranch, @Assisted CommitStatus commits, @@ -212,6 +215,7 @@ public abstract class SubmitStrategy { "project not found: %s", destBranch.getParentKey()); this.mergeSorter = new MergeSorter(rw, alreadyAccepted, canMergeFlag); this.mergeUtil = mergeUtilFactory.create(project); + this.onSubmitValidatorsFactory = onSubmitValidatorsFactory; } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/OnSubmitValidationListener.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/OnSubmitValidationListener.java new file mode 100644 index 0000000000..f2f397c650 --- /dev/null +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/OnSubmitValidationListener.java @@ -0,0 +1,85 @@ +// Copyright (C) 2017 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +package com.google.gerrit.server.git.validators; + +import com.google.gerrit.extensions.annotations.ExtensionPoint; +import com.google.gerrit.reviewdb.client.Project; +import com.google.gerrit.reviewdb.client.Project.NameKey; +import com.google.gerrit.server.validators.ValidationException; + +import org.eclipse.jgit.lib.ObjectReader; +import org.eclipse.jgit.lib.Repository; +import org.eclipse.jgit.revwalk.RevWalk; +import org.eclipse.jgit.transport.ReceiveCommand; + +import java.util.Map; + +/** + * Listener to validate ref updates performed during submit operation. + * + * As submit strategies may generate new commits (e.g. Cherry Pick), this + * listener allows validation of resulting new commit before destination branch + * is updated and new patchset ref is created. + * + * If you only care about validating the change being submitted and not the + * resulting new commit, consider using {@link MergeValidationListener} instead. + */ +@ExtensionPoint +public interface OnSubmitValidationListener { + public class Arguments { + private Project.NameKey project; + private Repository repository; + private ObjectReader objectReader; + private Map commands; + + public Arguments(NameKey project, Repository repository, + ObjectReader objectReader, Map commands) { + this.project = project; + this.repository = repository; + this.objectReader = objectReader; + this.commands = commands; + } + + public Project.NameKey getProject() { + return project; + } + + /** + * @return a read only repository + */ + public Repository getRepository() { + return repository; + } + + public RevWalk newRevWalk() { + return new RevWalk(objectReader); + } + + /** + * @return a map from ref to op on it covering all ref ops to be performed + * on this repository as part of ongoing submit operation. + */ + public Map getCommands(){ + return commands; + } + } + + /** + * Called right before branch is updated with new commit or commits as a + * result of submit. + * + * If ValidationException is thrown, submitting is aborted. + */ + void preBranchUpdate(Arguments args) throws ValidationException; +} diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/OnSubmitValidators.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/OnSubmitValidators.java new file mode 100644 index 0000000000..568c5972ff --- /dev/null +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/OnSubmitValidators.java @@ -0,0 +1,53 @@ +// Copyright (C) 2017 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +package com.google.gerrit.server.git.validators; + +import com.google.gerrit.extensions.registration.DynamicSet; +import com.google.gerrit.reviewdb.client.Project; +import com.google.gerrit.server.git.IntegrationException; +import com.google.gerrit.server.git.validators.OnSubmitValidationListener.Arguments; +import com.google.gerrit.server.validators.ValidationException; +import com.google.inject.Inject; + +import org.eclipse.jgit.lib.ObjectReader; +import org.eclipse.jgit.lib.Repository; +import org.eclipse.jgit.transport.ReceiveCommand; + +import java.util.Map; + +public class OnSubmitValidators { + public interface Factory { + OnSubmitValidators create(); + } + + private final DynamicSet listeners; + + @Inject + OnSubmitValidators(DynamicSet listeners) { + this.listeners = listeners; + } + + public void validate(Project.NameKey project, Repository repo, + ObjectReader objectReader, Map commands) + throws IntegrationException { + try { + for (OnSubmitValidationListener listener : this.listeners) { + listener.preBranchUpdate( + new Arguments(project, repo, objectReader, commands)); + } + } catch (ValidationException e) { + throw new IntegrationException(e.getMessage()); + } + } +}