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()); + } + } +}