From cc48e65225bea1bc4e55c672c1d89f6b0c26571b Mon Sep 17 00:00:00 2001 From: Gustaf Lundh Date: Fri, 30 Nov 2012 17:11:41 +0100 Subject: [PATCH] Fix: User could get around restrictions by reverting a commit The Gerrit server may enforce several restrictions on the commit message (change-id needed, signed-off-by, etc). The user could get around these restrictions by reverting a commit message using the UI. Now the "Revert" feature is using the validation code refactored out from ReceiveCommits, and correct validation of the commit message is done before Gerrit accepts the new commit. Change-Id: I4700c2d31d4b7e263c793e3430841b29dbd69657 --- .../com/google/gerrit/server/ChangeUtil.java | 38 ++++++++------- .../google/gerrit/server/change/Revert.java | 46 ++++++++++++++----- .../git/validators/CommitValidators.java | 30 ++++++++++++ 3 files changed, 87 insertions(+), 27 deletions(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/ChangeUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/ChangeUtil.java index d20da3c58c..ede8e74d8a 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/ChangeUtil.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/ChangeUtil.java @@ -31,9 +31,9 @@ import com.google.gerrit.server.events.CommitReceivedEvent; import com.google.gerrit.server.extensions.events.GitReferenceUpdated; import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gerrit.server.git.MergeOp; -import com.google.gerrit.server.mail.CommitMessageEditedSender; import com.google.gerrit.server.git.validators.CommitValidationException; import com.google.gerrit.server.git.validators.CommitValidators; +import com.google.gerrit.server.mail.CommitMessageEditedSender; import com.google.gerrit.server.mail.RevertedSender; import com.google.gerrit.server.patch.PatchSetInfoFactory; import com.google.gerrit.server.patch.PatchSetInfoNotAvailableException; @@ -47,7 +47,6 @@ import com.google.gwtorm.server.OrmException; import org.eclipse.jgit.errors.IncorrectObjectTypeException; import org.eclipse.jgit.errors.MissingObjectException; -import org.eclipse.jgit.errors.RepositoryNotFoundException; import org.eclipse.jgit.lib.CommitBuilder; import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.ObjectInserter; @@ -192,14 +191,17 @@ public class ChangeUtil { db.patchSetAncestors().insert(toInsert); } - public static Change.Id revert(final PatchSet.Id patchSetId, - final IdentifiedUser user, final String message, final ReviewDb db, + public static Change.Id revert(final RefControl refControl, + final PatchSet.Id patchSetId, final IdentifiedUser user, + final CommitValidators commitValidators, + final String message, final ReviewDb db, final RevertedSender.Factory revertedSenderFactory, - final ChangeHooks hooks, GitRepositoryManager gitManager, + final ChangeHooks hooks, Repository git, final PatchSetInfoFactory patchSetInfoFactory, - final GitReferenceUpdated replication, PersonIdent myIdent) - throws NoSuchChangeException, EmailException, OrmException, - MissingObjectException, IncorrectObjectTypeException, IOException { + final GitReferenceUpdated replication, PersonIdent myIdent, + String canonicalWebUrl) throws NoSuchChangeException, EmailException, + OrmException, MissingObjectException, IncorrectObjectTypeException, + IOException, InvalidChangeOperationException { final Change.Id changeId = patchSetId.getParentKey(); final PatchSet patch = db.patchSets().get(patchSetId); if (patch == null) { @@ -207,13 +209,6 @@ public class ChangeUtil { } final Change changeToRevert = db.changes().get(changeId); - final Repository git; - try { - git = gitManager.openRepository(changeToRevert.getProject()); - } catch (RepositoryNotFoundException e) { - throw new NoSuchChangeException(changeId, e); - } - final RevWalk revWalk = new RevWalk(git); try { RevCommit commitToRevert = @@ -260,6 +255,18 @@ public class ChangeUtil { ps.setUploader(change.getOwner()); ps.setRevision(new RevId(revertCommit.name())); + CommitReceivedEvent commitReceivedEvent = + new CommitReceivedEvent(new ReceiveCommand(ObjectId.zeroId(), + revertCommit.getId(), ps.getRefName()), refControl + .getProjectControl().getProject(), refControl.getRefName(), + revertCommit, user); + + try { + commitValidators.validateForRevertCommits(commitReceivedEvent); + } catch (CommitValidationException e) { + throw new InvalidChangeOperationException(e.getMessage()); + } + change.setCurrentPatchSet(patchSetInfoFactory.get(revertCommit, ps.getId())); ChangeUtil.updated(change); @@ -305,7 +312,6 @@ public class ChangeUtil { return change.getId(); } finally { revWalk.release(); - git.close(); } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/Revert.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/Revert.java index de7cfd40dc..3614b37ba4 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/Revert.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/Revert.java @@ -17,6 +17,7 @@ package com.google.gerrit.server.change; import com.google.common.base.Strings; import com.google.gerrit.common.ChangeHooks; import com.google.gerrit.extensions.restapi.AuthException; +import com.google.gerrit.extensions.restapi.BadRequestException; import com.google.gerrit.extensions.restapi.ResourceConflictException; import com.google.gerrit.extensions.restapi.RestModifyView; import com.google.gerrit.reviewdb.client.Change; @@ -26,25 +27,34 @@ import com.google.gerrit.server.ChangeUtil; import com.google.gerrit.server.GerritPersonIdent; import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.change.Revert.Input; +import com.google.gerrit.server.config.CanonicalWebUrl; import com.google.gerrit.server.extensions.events.GitReferenceUpdated; import com.google.gerrit.server.git.GitRepositoryManager; +import com.google.gerrit.server.git.validators.CommitValidators; import com.google.gerrit.server.mail.RevertedSender; import com.google.gerrit.server.patch.PatchSetInfoFactory; import com.google.gerrit.server.project.ChangeControl; +import com.google.gerrit.server.project.InvalidChangeOperationException; +import com.google.gerrit.server.ssh.NoSshInfo; import com.google.inject.Inject; import com.google.inject.Provider; import org.eclipse.jgit.lib.PersonIdent; +import org.eclipse.jgit.lib.Repository; + +import javax.annotation.Nullable; public class Revert implements RestModifyView { private final ChangeHooks hooks; private final RevertedSender.Factory revertedSenderFactory; + private final CommitValidators.Factory commitValidatorsFactory; private final Provider dbProvider; private final ChangeJson json; private final GitRepositoryManager gitManager; private final PersonIdent myIdent; private final PatchSetInfoFactory patchSetInfoFactory; private final GitReferenceUpdated replication; + private final String canonicalWebUrl; public static class Input { public String message; @@ -53,20 +63,24 @@ public class Revert implements RestModifyView { @Inject Revert(ChangeHooks hooks, RevertedSender.Factory revertedSenderFactory, + final CommitValidators.Factory commitValidatorsFactory, Provider dbProvider, ChangeJson json, GitRepositoryManager gitManager, final PatchSetInfoFactory patchSetInfoFactory, final GitReferenceUpdated replication, - @GerritPersonIdent final PersonIdent myIdent) { + @GerritPersonIdent final PersonIdent myIdent, + @CanonicalWebUrl @Nullable final String canonicalWebUrl) { this.hooks = hooks; this.revertedSenderFactory = revertedSenderFactory; + this.commitValidatorsFactory = commitValidatorsFactory; this.dbProvider = dbProvider; this.json = json; this.gitManager = gitManager; this.myIdent = myIdent; this.replication = replication; this.patchSetInfoFactory = patchSetInfoFactory; + this.canonicalWebUrl = canonicalWebUrl; } @Override @@ -75,8 +89,7 @@ public class Revert implements RestModifyView { } @Override - public Object apply(ChangeResource req, Input input) - throws Exception { + public Object apply(ChangeResource req, Input input) throws Exception { ChangeControl control = req.getControl(); Change change = req.getChange(); if (!control.canAddPatchSet()) { @@ -85,14 +98,25 @@ public class Revert implements RestModifyView { throw new ResourceConflictException("change is " + status(change)); } - Change.Id revertedChangeId = ChangeUtil.revert( - change.currentPatchSetId(), - (IdentifiedUser) control.getCurrentUser(), - Strings.emptyToNull(input.message), - dbProvider.get(), - revertedSenderFactory, hooks, gitManager, - patchSetInfoFactory, replication, myIdent); - return json.format(revertedChangeId); + final Repository git = gitManager.openRepository(control.getProject().getNameKey()); + try { + CommitValidators commitValidators = + commitValidatorsFactory.create(control.getRefControl(), new NoSshInfo(), git); + + Change.Id revertedChangeId = + ChangeUtil.revert(control.getRefControl(), change.currentPatchSetId(), + (IdentifiedUser) control.getCurrentUser(), + commitValidators, + Strings.emptyToNull(input.message), dbProvider.get(), + revertedSenderFactory, hooks, git, patchSetInfoFactory, + replication, myIdent, canonicalWebUrl); + + return json.format(revertedChangeId); + } catch (InvalidChangeOperationException e) { + throw new BadRequestException(e.getMessage()); + } finally { + git.close(); + } } private static String status(Change change) { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/CommitValidators.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/CommitValidators.java index c626663b87..a53548b325 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/CommitValidators.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/CommitValidators.java @@ -117,6 +117,36 @@ public class CommitValidators { return messages; } + public List validateForRevertCommits( + CommitReceivedEvent receiveEvent) throws CommitValidationException { + + List validators = + new LinkedList(); + + validators.add(new UploadMergesPermissionValidator(refControl)); + validators.add(new AmendedGerritMergeCommitValidationListener( + refControl, gerritIdent)); + validators.add(new AuthorUploaderValidator(refControl, canonicalWebUrl)); + validators.add(new SignedOffByValidator(refControl, canonicalWebUrl)); + validators.add(new ChangeIdValidator(refControl, canonicalWebUrl, sshInfo)); + validators.add(new ConfigValidator(refControl, repo)); + validators.add(new PluginCommitValidationListener(commitValidationListeners)); + + List messages = + new LinkedList(); + + try { + for (CommitValidationListener commitValidator : validators) { + messages.addAll(commitValidator.onCommitReceived(receiveEvent)); + } + } catch (CommitValidationException e) { + // Keep the old messages (and their order) in case of an exception + messages.addAll(e.getMessages()); + throw new CommitValidationException(e.getMessage(), messages); + } + return messages; + } + public static class ChangeIdValidator implements CommitValidationListener { private final RefControl refControl; private final String canonicalWebUrl;