diff --git a/Documentation/config-labels.txt b/Documentation/config-labels.txt index a40c5f3cf2..9cf4e50149 100644 --- a/Documentation/config-labels.txt +++ b/Documentation/config-labels.txt @@ -217,6 +217,19 @@ the lowest possible negative value will not block the change. The label is purely informational and values are not considered when determining whether a change is submittable. +* `PatchSetLock` ++ +The `PatchSetLock` function provides a locking mechanism for patch +sets. This function's values are not considered when determining +whether a change is submittable. When set, no new patchsets can be +created and rebase and abandon are blocked. ++ +This function is designed to allow overlapping locks, so several lock +accounts could lock the same change. ++ +Allowed range of values are 0 (Patch Set Unlocked) to 1 (Patch Set +Locked). + [[label_copyMinScore]] === `label.Label-Name.copyMinScore` diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java index 900d85a818..21fd1a8f50 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java @@ -17,6 +17,7 @@ package com.google.gerrit.acceptance.git; import static com.google.common.truth.Truth.assertThat; import static com.google.common.truth.TruthJUnit.assume; import static com.google.gerrit.acceptance.GitUtil.pushHead; +import static com.google.gerrit.server.group.SystemGroupBackend.ANONYMOUS_USERS; import static java.util.concurrent.TimeUnit.MILLISECONDS; import static java.util.concurrent.TimeUnit.SECONDS; @@ -26,11 +27,19 @@ import com.google.gerrit.acceptance.AbstractDaemonTest; import com.google.gerrit.acceptance.GitUtil; import com.google.gerrit.acceptance.PushOneCommit; import com.google.gerrit.acceptance.TestAccount; +import com.google.gerrit.common.data.LabelType; +import com.google.gerrit.common.data.Permission; +import com.google.gerrit.extensions.api.changes.ReviewInput; import com.google.gerrit.extensions.client.InheritableBoolean; import com.google.gerrit.extensions.common.ChangeInfo; import com.google.gerrit.extensions.common.EditInfo; import com.google.gerrit.extensions.common.LabelInfo; +import com.google.gerrit.reviewdb.client.AccountGroup; import com.google.gerrit.reviewdb.client.Change; +import com.google.gerrit.server.git.MetaDataUpdate; +import com.google.gerrit.server.git.ProjectConfig; +import com.google.gerrit.server.group.SystemGroupBackend; +import com.google.gerrit.server.project.Util; import org.eclipse.jgit.revwalk.RevCommit; import org.joda.time.DateTime; @@ -51,6 +60,7 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest { } private String sshUrl; + private LabelType patchSetLock; @BeforeClass public static void setTimeForTesting() { @@ -73,6 +83,24 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest { @Before public void setUp() throws Exception { sshUrl = sshSession.getUrl(); + ProjectConfig cfg = projectCache.checkedGet(project).getConfig(); + patchSetLock = Util.patchSetLock(); + cfg.getLabelSections().put(patchSetLock.getName(), patchSetLock); + AccountGroup.UUID anonymousUsers = + SystemGroupBackend.getGroup(ANONYMOUS_USERS).getUUID(); + Util.allow(cfg, Permission.forLabel(patchSetLock.getName()), 0, 1, anonymousUsers, + "refs/heads/*"); + saveProjectConfig(cfg); + grant(Permission.LABEL + "Patch-Set-Lock", project, "refs/heads/*"); + } + + private void saveProjectConfig(ProjectConfig cfg) throws Exception { + MetaDataUpdate md = metaDataUpdateFactory.create(project); + try { + cfg.commit(md); + } finally { + md.close(); + } } protected void selectProtocol(Protocol p) throws Exception { @@ -275,6 +303,18 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest { r.assertOkStatus(); } + @Test + public void testPushNewPatchsetToPatchSetLockedChange() throws Exception { + PushOneCommit.Result r = pushTo("refs/for/master"); + r.assertOkStatus(); + PushOneCommit push = pushFactory.create(db, admin.getIdent(), testRepo, + PushOneCommit.SUBJECT, "b.txt", "anotherContent", r.getChangeId()); + revision(r).review(new ReviewInput().label("Patch-Set-Lock", 1)); + r = push.to("refs/for/master"); + r.assertErrorStatus("cannot replace " + r.getChange().change().getChangeId() + + ". Change is patch set locked."); + } + @Test public void testPushForMasterWithApprovals_MissingLabel() throws Exception { PushOneCommit.Result r = pushTo("refs/for/master/%l=Verify"); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/Abandon.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/Abandon.java index f2d40c8e1e..ff1035112f 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/Abandon.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/Abandon.java @@ -82,7 +82,7 @@ public class Abandon implements RestModifyView, throws RestApiException, UpdateException, OrmException { ChangeControl control = req.getControl(); IdentifiedUser caller = control.getUser().asIdentifiedUser(); - if (!control.canAbandon()) { + if (!control.canAbandon(dbProvider.get())) { throw new AuthException("abandon not permitted"); } Change change = abandon(control, input.message, caller.getAccount()); @@ -174,12 +174,18 @@ public class Abandon implements RestModifyView, @Override public UiAction.Description getDescription(ChangeResource resource) { + boolean canAbandon = false; + try { + canAbandon = resource.getControl().canAbandon(dbProvider.get()); + } catch (OrmException e) { + log.error("Cannot check canAbandon status. Assuming false.", e); + } return new UiAction.Description() .setLabel("Abandon") .setTitle("Abandon the change") .setVisible(resource.getChange().getStatus().isOpen() && resource.getChange().getStatus() != Change.Status.DRAFT - && resource.getControl().canAbandon()); + && canAbandon); } private static String status(Change change) { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/Rebase.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/Rebase.java index 60f285f18a..5bcd23cce2 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/Rebase.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/Rebase.java @@ -90,7 +90,7 @@ public class Rebase implements RestModifyView, ObjectInserter oi = repo.newObjectInserter(); BatchUpdate bu = updateFactory.create(dbProvider.get(), change.getProject(), rsrc.getUser(), TimeUtil.nowTs())) { - if (!control.canRebase()) { + if (!control.canRebase(dbProvider.get())) { throw new AuthException("rebase not permitted"); } else if (!change.getStatus().isOpen()) { throw new ResourceConflictException("change is " @@ -209,9 +209,15 @@ public class Rebase implements RestModifyView, public UiAction.Description getDescription(RevisionResource resource) { PatchSet patchSet = resource.getPatchSet(); Branch.NameKey dest = resource.getChange().getDest(); + boolean canRebase = false; + try { + canRebase = resource.getControl().canRebase(dbProvider.get()); + } catch (OrmException e) { + log.error("Cannot check canRebase status. Assuming false.", e); + } boolean visible = resource.getChange().getStatus().isOpen() && resource.isCurrent() - && resource.getControl().canRebase(); + && canRebase; boolean enabled = true; if (visible) { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/Restore.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/Restore.java index 5b0eb6deaf..ce2e81db7c 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/Restore.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/Restore.java @@ -80,7 +80,7 @@ public class Restore implements RestModifyView, public ChangeInfo apply(ChangeResource req, RestoreInput input) throws RestApiException, UpdateException, OrmException { ChangeControl ctl = req.getControl(); - if (!ctl.canRestore()) { + if (!ctl.canRestore(dbProvider.get())) { throw new AuthException("restore not permitted"); } @@ -160,11 +160,17 @@ public class Restore implements RestModifyView, @Override public UiAction.Description getDescription(ChangeResource resource) { + boolean canRestore = false; + try { + canRestore = resource.getControl().canRestore(dbProvider.get()); + } catch (OrmException e) { + log.error("Cannot check canRestore status. Assuming false.", e); + } return new UiAction.Description() .setLabel("Restore") .setTitle("Restore the change") .setVisible(resource.getChange().getStatus() == Status.ABANDONED - && resource.getControl().canRestore()); + && canRestore); } private static String status(Change change) { 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 dc2ed5dffb..7e3845cd72 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 @@ -29,8 +29,8 @@ import com.google.gerrit.reviewdb.client.Change.Status; import com.google.gerrit.server.ChangeUtil; import com.google.gerrit.server.GerritPersonIdent; import com.google.gerrit.server.git.UpdateException; -import com.google.gerrit.server.project.ChangeControl; import com.google.gerrit.server.project.NoSuchChangeException; +import com.google.gerrit.server.project.RefControl; import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; import com.google.inject.Singleton; @@ -59,9 +59,9 @@ public class Revert implements RestModifyView, public ChangeInfo apply(ChangeResource req, RevertInput input) throws IOException, OrmException, RestApiException, UpdateException { - ChangeControl control = req.getControl(); + RefControl refControl = req.getControl().getRefControl(); Change change = req.getChange(); - if (!control.canAddPatchSet()) { + if (!refControl.canUpload()) { throw new AuthException("revert not permitted"); } else if (change.getStatus() != Status.MERGED) { throw new ResourceConflictException("change is " + status(change)); @@ -69,7 +69,7 @@ public class Revert implements RestModifyView, Change.Id revertedChangeId; try { - revertedChangeId = changeUtil.revert(control, + revertedChangeId = changeUtil.revert(req.getControl(), change.currentPatchSetId(), Strings.emptyToNull(input.message), new PersonIdent(myIdent, TimeUtil.nowTs())); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/ProjectConfig.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/ProjectConfig.java index addbc1b973..246647c3d9 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/ProjectConfig.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/ProjectConfig.java @@ -141,7 +141,7 @@ public class ProjectConfig extends VersionedMetaData implements ValidationError. private static final String KEY_CAN_OVERRIDE = "canOverride"; private static final String KEY_Branch = "branch"; private static final Set LABEL_FUNCTIONS = ImmutableSet.of( - "MaxWithBlock", "AnyWithBlock", "MaxNoBlock", "NoBlock", "NoOp"); + "MaxWithBlock", "AnyWithBlock", "MaxNoBlock", "NoBlock", "NoOp", "PatchSetLock"); private static final String PLUGIN = "plugin"; diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java index 5da4ec7e3c..304ebc257e 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java @@ -1966,7 +1966,7 @@ public class ReceiveCommits { } } - boolean validate(boolean autoClose) throws IOException { + boolean validate(boolean autoClose) throws IOException, OrmException { if (!autoClose && inputCommand.getResult() != NOT_ATTEMPTED) { return false; } else if (change == null) { @@ -1989,8 +1989,12 @@ public class ReceiveCommits { } changeCtl = projectControl.controlFor(change); - if (!changeCtl.canAddPatchSet()) { - reject(inputCommand, "cannot replace " + ontoChange); + if (!changeCtl.canAddPatchSet(db)) { + String locked = "."; + if (changeCtl.isPatchSetLocked(db)) { + locked = ". Change is patch set locked."; + } + reject(inputCommand, "cannot replace " + ontoChange + locked); return false; } else if (change.getStatus().isClosed()) { reject(inputCommand, "change " + ontoChange + " closed"); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java index 3ab6ff56ac..8a50114eaa 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java @@ -28,6 +28,7 @@ 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.server.ReviewDb; +import com.google.gerrit.server.ApprovalsUtil; import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.notedb.ChangeNotes; import com.google.gerrit.server.query.change.ChangeData; @@ -103,23 +104,27 @@ public class ChangeControl { private final ChangeData.Factory changeDataFactory; private final RefControl refControl; private final ChangeNotes notes; + private final ApprovalsUtil approvalsUtil; @AssistedInject ChangeControl( ChangeData.Factory changeDataFactory, ChangeNotes.Factory notesFactory, + ApprovalsUtil approvalsUtil, @Assisted RefControl refControl, @Assisted Change change) { - this(changeDataFactory, refControl, + this(changeDataFactory, approvalsUtil, refControl, notesFactory.create(change)); } @AssistedInject ChangeControl( ChangeData.Factory changeDataFactory, + ApprovalsUtil approvalsUtil, @Assisted RefControl refControl, @Assisted ChangeNotes notes) { this.changeDataFactory = changeDataFactory; + this.approvalsUtil = approvalsUtil; this.refControl = refControl; this.notes = notes; } @@ -128,7 +133,7 @@ public class ChangeControl { if (getUser().equals(who)) { return this; } - return new ChangeControl(changeDataFactory, + return new ChangeControl(changeDataFactory, approvalsUtil, getRefControl().forUser(who), notes); } @@ -190,13 +195,13 @@ public class ChangeControl { } /** Can this user abandon this change? */ - public boolean canAbandon() { - return isOwner() // owner (aka creator) of the change can abandon + public boolean canAbandon(ReviewDb db) throws OrmException { + return (isOwner() // owner (aka creator) of the change can abandon || getRefControl().isOwner() // branch owner can abandon || getProjectControl().isOwner() // project owner can abandon || getUser().getCapabilities().canAdministrateServer() // site administers are god || getRefControl().canAbandon() // user can abandon a specific ref - ; + ) && !isPatchSetLocked(db); } /** Can this user publish this draft change or any draft patch set of this change? */ @@ -212,14 +217,14 @@ public class ChangeControl { } /** Can this user rebase this change? */ - public boolean canRebase() { - return isOwner() || getRefControl().canSubmit() - || getRefControl().canRebase(); + public boolean canRebase(ReviewDb db) throws OrmException { + return (isOwner() || getRefControl().canSubmit() + || getRefControl().canRebase()) && !isPatchSetLocked(db); } /** Can this user restore this change? */ - public boolean canRestore() { - return canAbandon() // Anyone who can abandon the change can restore it back + public boolean canRestore(ReviewDb db) throws OrmException { + return canAbandon(db) // Anyone who can abandon the change can restore it back && getRefControl().canUpload(); // as long as you can upload too } @@ -258,8 +263,25 @@ public class ChangeControl { } /** Can this user add a patch set to this change? */ - public boolean canAddPatchSet() { - return getRefControl().canUpload(); + public boolean canAddPatchSet(ReviewDb db) throws OrmException { + return getRefControl().canUpload() && !isPatchSetLocked(db); + } + + /** Is the current patch set locked against state changes? */ + public boolean isPatchSetLocked(ReviewDb db) throws OrmException { + if (getChange().getStatus() == Change.Status.MERGED) { + return false; + } + + for (PatchSetApproval ap : approvalsUtil.byPatchSet(db, this, + getChange().currentPatchSetId())) { + LabelType type = getLabelTypes().byLabel(ap.getLabel()); + if (type != null && ap.getValue() == 1 + && type.getFunctionName().equalsIgnoreCase("PatchSetLock")) { + return true; + } + } + return false; } /** Is this user the owner of the change? */ diff --git a/gerrit-server/src/main/prolog/gerrit_common.pl b/gerrit-server/src/main/prolog/gerrit_common.pl index 9a4e77cd37..b5fa020374 100644 --- a/gerrit-server/src/main/prolog/gerrit_common.pl +++ b/gerrit-server/src/main/prolog/gerrit_common.pl @@ -243,6 +243,7 @@ legacy_submit_rule('AnyWithBlock', Label, Min, Max, T) :- !, any_with_block(Labe legacy_submit_rule('MaxNoBlock', Label, Min, Max, T) :- !, max_no_block(Label, Max, T). legacy_submit_rule('NoBlock', Label, Min, Max, T) :- !, T = may(_). legacy_submit_rule('NoOp', Label, Min, Max, T) :- !, T = may(_). +legacy_submit_rule('PatchSetLock', Label, Min, Max, T) :- !, T = may(_). legacy_submit_rule(Fun, Label, Min, Max, T) :- T = impossible(unsupported(Fun)). %% max_with_block: diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/project/Util.java b/gerrit-server/src/test/java/com/google/gerrit/server/project/Util.java index a67152386a..d60277f4b7 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/project/Util.java +++ b/gerrit-server/src/test/java/com/google/gerrit/server/project/Util.java @@ -96,6 +96,14 @@ public class Util { value(-2, "This shall not be merged")); } + public static final LabelType patchSetLock() { + LabelType label = category("Patch-Set-Lock", + value(1, "Patch Set Locked"), + value(0, "Patch Set Unlocked")); + label.setFunctionName("PatchSetLock"); + return label; + } + public static LabelValue value(int value, String text) { return new LabelValue((short) value, text); }