opendev/gerrit
Code Issues Proposed changes

Move the 'isPatchSetLocked' check out from the ChangeControl

Browse Source 
PermissionBackend is supposed to only perform permission
related checks. The 'isPatchSetLocked' check is a patch
set status check to decide whether a change is able to
be rebased, abandoned, etc. It's not permission related
and thus should be moved out from the ChangeControl.

This commit moves the 'isPatchSetLocked' method to the
PatchSetUtil class which looks like the best destination
of this method.

After this move, each time the caller tries to check
the REBASE/ABANDON/RESTORE/ADD_PATCH_SET permission, it
must verify the patch set of the change is not locked. If
it's locked the request should be rejected directly without
checking any further permissions. This commit makes sure
every existing usage has done this prerequest check in
advance.

Change-Id: Ic5cef11228f152cb3f51eea0a7aee100332f263a
This commit is contained in:
Changcheng Xiao
2018-03-29 13:33:13 +02:00
parent d4d83c50f1
commit 30325c2b7a
13 changed files with 299 additions and 169 deletions
Download Patch File Download Diff File Expand all files Collapse all files

53
java/com/google/gerrit/server/PatchSetUtil.java
View File

@@ -26,15 +26,22 @@ import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Maps;
import com.google.common.collect.Sets;
import com.google.common.collect.Streams;
import com.google.gerrit.common.data.LabelFunction;
import com.google.gerrit.common.data.LabelType;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
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.RevId;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.notedb.ChangeUpdate;
import com.google.gerrit.server.notedb.NotesMigration;
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.project.ProjectState;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.Provider;
import com.google.inject.Singleton;
import java.io.IOException;
import java.sql.Timestamp;
@@ -48,10 +55,20 @@ import org.eclipse.jgit.revwalk.RevWalk;
@Singleton
public class PatchSetUtil {
private final NotesMigration migration;
private final Provider<ApprovalsUtil> approvalsUtilProvider;
private final ProjectCache projectCache;
private final Provider<ReviewDb> dbProvider;
@Inject
PatchSetUtil(NotesMigration migration) {
PatchSetUtil(
NotesMigration migration,
Provider<ApprovalsUtil> approvalsUtilProvider,
ProjectCache projectCache,
Provider<ReviewDb> dbProvider) {
this.migration = migration;
this.approvalsUtilProvider = approvalsUtilProvider;
this.projectCache = projectCache;
this.dbProvider = dbProvider;
}
public PatchSet current(ReviewDb db, ChangeNotes notes) throws OrmException {
@@ -155,4 +172,38 @@ public class PatchSetUtil {
update.setGroups(groups);
db.patchSets().update(Collections.singleton(ps));
}
/** Check if the current patch set of the change is locked. */
public void checkPatchSetNotLocked(ChangeNotes notes, CurrentUser user)
throws OrmException, IOException, ResourceConflictException {
if (isPatchSetLocked(notes, user)) {
throw new ResourceConflictException(
String.format("The current patch set of change %s is locked", notes.getChangeId()));
}
}
/** Is the current patch set locked against state changes? */
public boolean isPatchSetLocked(ChangeNotes notes, CurrentUser user)
throws OrmException, IOException {
Change change = notes.getChange();
if (change.getStatus() == Change.Status.MERGED) {
return false;
}
ProjectState projectState = projectCache.checkedGet(notes.getProjectName());
checkNotNull(projectState, "Failed to load project %s", notes.getProjectName());
ApprovalsUtil approvalsUtil = approvalsUtilProvider.get();
for (PatchSetApproval ap :
approvalsUtil.byPatchSet(
dbProvider.get(), notes, user, change.currentPatchSetId(), null, null)) {
LabelType type = projectState.getLabelTypes(notes, user).byLabel(ap.getLabel());
if (type != null
&& ap.getValue() == 1
&& type.getFunction() == LabelFunction.PATCH_SET_LOCK) {
return true;
}
}
return false;
}
}

6
java/com/google/gerrit/server/change/PatchSetInserter.java
View File

@@ -311,7 +311,11 @@ public class PatchSetInserter implements BatchUpdateOp {
}
private void validate(RepoContext ctx)
throws AuthException, ResourceConflictException, IOException, PermissionBackendException {
throws AuthException, ResourceConflictException, IOException, PermissionBackendException,
OrmException {
// Not allowed to create a new patch set if the current patch set is locked.
psUtil.checkPatchSetNotLocked(origNotes, ctx.getUser());
if (checkAddPatchSetPermission) {
permissionBackend
.user(ctx.getUser())

5
java/com/google/gerrit/server/edit/ChangeEditModifier.java
View File

@@ -394,7 +394,8 @@ public class ChangeEditModifier {
}
private void assertCanEdit(ChangeNotes notes)
throws AuthException, PermissionBackendException, IOException, ResourceConflictException {
throws AuthException, PermissionBackendException, IOException, ResourceConflictException,
OrmException {
if (!currentUser.get().isIdentifiedUser()) {
throw new AuthException("Authentication required");
}
@@ -406,6 +407,8 @@ public class ChangeEditModifier {
"change %s is %s", c.getChangeId(), c.getStatus().toString().toLowerCase()));
}
// Not allowed to edit if the current patch set is locked.
patchSetUtil.checkPatchSetNotLocked(notes, currentUser.get());
try {
permissionBackend
.currentUser()

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

@@ -2452,6 +2452,13 @@ class ReceiveCommits {
RevCommit newCommit = rp.getRevWalk().parseCommit(newCommitId);
RevCommit priorCommit = revisions.inverse().get(priorPatchSet);
// Not allowed to create a new patch set if the current patch set is locked.
if (psUtil.isPatchSetLocked(notes, user)) {
reject(inputCommand, "cannot add patch set to " + ontoChange + ".");
return false;
}
try {
permissions.change(notes).database(db).check(ChangePermission.ADD_PATCH_SET);
} catch (AuthException no) {

87
java/com/google/gerrit/server/permissions/ChangeControl.java
View File

@@ -21,19 +21,14 @@ import static com.google.gerrit.server.permissions.LabelPermission.ForUser.ON_BE
import com.google.common.collect.Maps;
import com.google.common.collect.Sets;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.common.data.LabelFunction;
import com.google.gerrit.common.data.LabelType;
import com.google.gerrit.common.data.Permission;
import com.google.gerrit.common.data.PermissionRange;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.Change;
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.PatchSetUtil;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.permissions.PermissionBackend.ForChange;
import com.google.gerrit.server.query.change.ChangeData;
@@ -52,19 +47,11 @@ class ChangeControl {
static class Factory {
private final ChangeData.Factory changeDataFactory;
private final ChangeNotes.Factory notesFactory;
private final ApprovalsUtil approvalsUtil;
private final PatchSetUtil patchSetUtil;
@Inject
Factory(
ChangeData.Factory changeDataFactory,
ChangeNotes.Factory notesFactory,
ApprovalsUtil approvalsUtil,
PatchSetUtil patchSetUtil) {
Factory(ChangeData.Factory changeDataFactory, ChangeNotes.Factory notesFactory) {
this.changeDataFactory = changeDataFactory;
this.notesFactory = notesFactory;
this.approvalsUtil = approvalsUtil;
this.patchSetUtil = patchSetUtil;
}
ChangeControl create(
@@ -74,27 +61,19 @@ class ChangeControl {
}
ChangeControl create(RefControl refControl, ChangeNotes notes) {
return new ChangeControl(changeDataFactory, approvalsUtil, refControl, notes, patchSetUtil);
return new ChangeControl(changeDataFactory, refControl, notes);
}
}
private final ChangeData.Factory changeDataFactory;
private final ApprovalsUtil approvalsUtil;
private final RefControl refControl;
private final ChangeNotes notes;
private final PatchSetUtil patchSetUtil;
private ChangeControl(
ChangeData.Factory changeDataFactory,
ApprovalsUtil approvalsUtil,
RefControl refControl,
ChangeNotes notes,
PatchSetUtil patchSetUtil) {
ChangeData.Factory changeDataFactory, RefControl refControl, ChangeNotes notes) {
this.changeDataFactory = changeDataFactory;
this.approvalsUtil = approvalsUtil;
this.refControl = refControl;
this.notes = notes;
this.patchSetUtil = patchSetUtil;
}
ForChange asForChange(@Nullable ChangeData cd, @Nullable Provider<ReviewDb> db) {
@@ -105,8 +84,7 @@ class ChangeControl {
if (getUser().equals(who)) {
return this;
}
return new ChangeControl(
changeDataFactory, approvalsUtil, refControl.forUser(who), notes, patchSetUtil);
return new ChangeControl(changeDataFactory, refControl.forUser(who), notes);
}
private CurrentUser getUser() {
@@ -130,26 +108,24 @@ class ChangeControl {
}
/** Can this user abandon this change? */
private boolean canAbandon(ReviewDb db) throws OrmException {
return (isOwner() // owner (aka creator) of the change can abandon
|| refControl.isOwner() // branch owner can abandon
|| getProjectControl().isOwner() // project owner can abandon
|| refControl.canPerform(Permission.ABANDON) // user can abandon a specific ref
|| getProjectControl().isAdmin())
&& !isPatchSetLocked(db);
private boolean canAbandon() {
return isOwner() // owner (aka creator) of the change can abandon
|| refControl.isOwner() // branch owner can abandon
|| getProjectControl().isOwner() // project owner can abandon
|| refControl.canPerform(Permission.ABANDON) // user can abandon a specific ref
|| getProjectControl().isAdmin();
}
/** Can this user rebase this change? */
private boolean canRebase(ReviewDb db) throws OrmException {
private boolean canRebase() {
return (isOwner() || refControl.canSubmit(isOwner()) || refControl.canRebase())
&& refControl.asForRef().testOrFalse(RefPermission.CREATE_CHANGE)
&& !isPatchSetLocked(db);
&& refControl.asForRef().testOrFalse(RefPermission.CREATE_CHANGE);
}
/** Can this user restore this change? */
private boolean canRestore(ReviewDb db) throws OrmException {
private boolean canRestore() {
// Anyone who can abandon the change can restore it, as long as they can create changes.
return canAbandon(db) && refControl.asForRef().testOrFalse(RefPermission.CREATE_CHANGE);
return canAbandon() && refControl.asForRef().testOrFalse(RefPermission.CREATE_CHANGE);
}
/** The range of permitted values associated with a label permission. */
@@ -158,8 +134,8 @@ class ChangeControl {
}
/** Can this user add a patch set to this change? */
private boolean canAddPatchSet(ReviewDb db) throws OrmException {
if (!(refControl.asForRef().testOrFalse(RefPermission.CREATE_CHANGE)) || isPatchSetLocked(db)) {
private boolean canAddPatchSet() {
if (!refControl.asForRef().testOrFalse(RefPermission.CREATE_CHANGE)) {
return false;
}
if (isOwner()) {
@@ -168,29 +144,6 @@ class ChangeControl {
return refControl.canAddPatchSet();
}
/** Is the current patch set locked against state changes? */
private boolean isPatchSetLocked(ReviewDb db) throws OrmException {
if (getChange().getStatus() == Change.Status.MERGED) {
return false;
}
for (PatchSetApproval ap :
approvalsUtil.byPatchSet(
db, notes, getUser(), getChange().currentPatchSetId(), null, null)) {
LabelType type =
getProjectControl()
.getProjectState()
.getLabelTypes(notes, getUser())
.byLabel(ap.getLabel());
if (type != null
&& ap.getValue() == 1
&& type.getFunction() == LabelFunction.PATCH_SET_LOCK) {
return true;
}
}
return false;
}
/** Is this user the owner of the change? */
private boolean isOwner() {
if (getUser().isIdentifiedUser()) {
@@ -355,12 +308,12 @@ class ChangeControl {
case READ:
return isVisible(db(), changeData());
case ABANDON:
return canAbandon(db());
return canAbandon();
case DELETE:
return (isOwner() && refControl.canPerform(Permission.DELETE_OWN_CHANGES))
|| getProjectControl().isAdmin();
case ADD_PATCH_SET:
return canAddPatchSet(db());
return canAddPatchSet();
case EDIT_ASSIGNEE:
return canEditAssignee();
case EDIT_DESCRIPTION:
@@ -370,9 +323,9 @@ class ChangeControl {
case EDIT_TOPIC_NAME:
return canEditTopicName();
case REBASE:
return canRebase(db());
return canRebase();
case RESTORE:
return canRestore(db());
return canRestore();
case SUBMIT:
return refControl.canSubmit(isOwner());

24
java/com/google/gerrit/server/permissions/ChangePermission.java
View File

@@ -20,15 +20,39 @@ import com.google.gerrit.extensions.api.access.GerritPermission;
public enum ChangePermission implements ChangePermissionOrLabel {
READ,
/**
* The change can't be restored if its current patch set is locked.
*
* <p>Before checking this permission, the caller should first verify the current patch set of the
* change is not locked by calling {@code PatchSetUtil.isPatchSetLocked}.
*/
RESTORE,
DELETE,
/**
* The change can't be abandoned if its current patch set is locked.
*
* <p>Before checking this permission, the caller should first verify the current patch set of the
* change is not locked by calling {@code PatchSetUtil.isPatchSetLocked}.
*/
ABANDON,
EDIT_ASSIGNEE,
EDIT_DESCRIPTION,
EDIT_HASHTAGS,
EDIT_TOPIC_NAME,
REMOVE_REVIEWER,
/**
* A new patch set can't be added if the patch set is locked for the change.
*
* <p>Before checking this permission, the caller should first verify the current patch set of the
* change is not locked by calling {@code PatchSetUtil.isPatchSetLocked}.
*/
ADD_PATCH_SET,
/**
* The change can't be rebased if its current patch set is locked.
*
* <p>Before checking this permission, the caller should first verify the current patch set of the
* change is not locked by calling {@code PatchSetUtil.isPatchSetLocked}.
*/
REBASE,
SUBMIT,
SUBMIT_AS("submit on behalf of other users");

55
java/com/google/gerrit/server/restapi/change/Abandon.java
View File

@@ -14,8 +14,6 @@
package com.google.gerrit.server.restapi.change;
import static com.google.gerrit.extensions.conditions.BooleanCondition.and;
import com.google.common.collect.ImmutableListMultimap;
import com.google.common.collect.ListMultimap;
import com.google.gerrit.common.TimeUtil;
@@ -29,6 +27,7 @@ import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.PatchSetUtil;
import com.google.gerrit.server.account.AccountState;
import com.google.gerrit.server.change.AbandonOp;
import com.google.gerrit.server.change.ChangeJson;
@@ -47,14 +46,19 @@ import com.google.inject.Provider;
import com.google.inject.Singleton;
import java.io.IOException;
import org.eclipse.jgit.errors.ConfigInvalidException;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@Singleton
public class Abandon extends RetryingRestModifyView<ChangeResource, AbandonInput, ChangeInfo>
implements UiAction<ChangeResource> {
private static final Logger log = LoggerFactory.getLogger(Abandon.class);
private final Provider<ReviewDb> dbProvider;
private final ChangeJson.Factory json;
private final AbandonOp.Factory abandonOpFactory;
private final NotifyUtil notifyUtil;
private final PatchSetUtil patchSetUtil;
@Inject
Abandon(
@@ -62,27 +66,32 @@ public class Abandon extends RetryingRestModifyView<ChangeResource, AbandonInput
ChangeJson.Factory json,
RetryHelper retryHelper,
AbandonOp.Factory abandonOpFactory,
NotifyUtil notifyUtil) {
NotifyUtil notifyUtil,
PatchSetUtil patchSetUtil) {
super(retryHelper);
this.dbProvider = dbProvider;
this.json = json;
this.abandonOpFactory = abandonOpFactory;
this.notifyUtil = notifyUtil;
this.patchSetUtil = patchSetUtil;
}
@Override
protected ChangeInfo applyImpl(
BatchUpdate.Factory updateFactory, ChangeResource req, AbandonInput input)
BatchUpdate.Factory updateFactory, ChangeResource rsrc, AbandonInput input)
throws RestApiException, UpdateException, OrmException, PermissionBackendException,
IOException, ConfigInvalidException {
req.permissions().database(dbProvider).check(ChangePermission.ABANDON);
// Not allowed to abandon if the current patch set is locked.
patchSetUtil.checkPatchSetNotLocked(rsrc.getNotes(), rsrc.getUser());
NotifyHandling notify = input.notify == null ? defaultNotify(req.getChange()) : input.notify;
rsrc.permissions().database(dbProvider).check(ChangePermission.ABANDON);
NotifyHandling notify = input.notify == null ? defaultNotify(rsrc.getChange()) : input.notify;
Change change =
abandon(
updateFactory,
req.getNotes(),
req.getUser(),
rsrc.getNotes(),
rsrc.getUser(),
input.message,
notify,
notifyUtil.resolveAccounts(input.notifyDetails));
@@ -135,13 +144,29 @@ public class Abandon extends RetryingRestModifyView<ChangeResource, AbandonInput
@Override
public UiAction.Description getDescription(ChangeResource rsrc) {
UiAction.Description description =
new UiAction.Description()
.setLabel("Abandon")
.setTitle("Abandon the change")
.setVisible(false);
Change change = rsrc.getChange();
return new UiAction.Description()
.setLabel("Abandon")
.setTitle("Abandon the change")
.setVisible(
and(
change.getStatus().isOpen(),
rsrc.permissions().database(dbProvider).testCond(ChangePermission.ABANDON)));
if (!change.getStatus().isOpen()) {
return description;
}
try {
if (patchSetUtil.isPatchSetLocked(rsrc.getNotes(), rsrc.getUser())) {
return description;
}
} catch (OrmException | IOException e) {
log.error(
String.format(
"Failed to check if the current patch set of change %s is locked", change.getId()),
e);
return description;
}
return description.setVisible(rsrc.permissions().testOrFalse(ChangePermission.ABANDON));
}
}

8
java/com/google/gerrit/server/restapi/change/CreateMergePatchSet.java
View File

@@ -50,7 +50,6 @@ import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.permissions.ChangePermission;
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.project.InvalidChangeOperationException;
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.project.ProjectState;
import com.google.gerrit.server.restapi.project.CommitsCollection;
@@ -126,8 +125,11 @@ public class CreateMergePatchSet
@Override
protected Response<ChangeInfo> applyImpl(
BatchUpdate.Factory updateFactory, ChangeResource rsrc, MergePatchSetInput in)
throws OrmException, IOException, InvalidChangeOperationException, RestApiException,
UpdateException, PermissionBackendException {
throws OrmException, IOException, RestApiException, UpdateException,
PermissionBackendException {
// Not allowed to create a new patch set if the current patch set is locked.
psUtil.checkPatchSetNotLocked(rsrc.getNotes(), rsrc.getUser());
rsrc.permissions().database(db).check(ChangePermission.ADD_PATCH_SET);
ProjectState projectState = projectCache.checkedGet(rsrc.getProject());

48
java/com/google/gerrit/server/restapi/change/Move.java
View File

@@ -134,6 +134,9 @@ public class Move extends RetryingRestModifyView<ChangeResource, MoveInput, Chan
throw new ResourceConflictException("Change is already destined for the specified branch");
}
// Not allowed to move if the current patch set is locked.
psUtil.checkPatchSetNotLocked(rsrc.getNotes(), rsrc.getUser());
// Move requires abandoning this change, and creating a new change.
try {
rsrc.permissions().database(dbProvider).check(ABANDON);
@@ -279,24 +282,41 @@ public class Move extends RetryingRestModifyView<ChangeResource, MoveInput, Chan
@Override
public UiAction.Description getDescription(ChangeResource rsrc) {
UiAction.Description description =
new UiAction.Description()
.setLabel("Move Change")
.setTitle("Move change to a different branch")
.setVisible(false);
Change change = rsrc.getChange();
boolean projectStatePermitsWrite = false;
if (!change.getStatus().isOpen()) {
return description;
}
try {
projectStatePermitsWrite = projectCache.checkedGet(rsrc.getProject()).statePermitsWrite();
if (!projectCache.checkedGet(rsrc.getProject()).statePermitsWrite()) {
return description;
}
} catch (IOException e) {
log.error("Failed to check if project state permits write: " + rsrc.getProject(), e);
return description;
}
return new UiAction.Description()
.setLabel("Move Change")
.setTitle("Move change to a different branch")
.setVisible(
and(
change.getStatus().isOpen() && projectStatePermitsWrite,
and(
permissionBackend
.user(rsrc.getUser())
.ref(change.getDest())
.testCond(CREATE_CHANGE),
rsrc.permissions().database(dbProvider).testCond(ABANDON))));
try {
if (psUtil.isPatchSetLocked(rsrc.getNotes(), rsrc.getUser())) {
return description;
}
} catch (OrmException | IOException e) {
log.error(
String.format(
"Failed to check if the current patch set of change %s is locked", change.getId()),
e);
return description;
}
return description.setVisible(
and(
permissionBackend.user(rsrc.getUser()).ref(change.getDest()).testCond(CREATE_CHANGE),
rsrc.permissions().database(dbProvider).testCond(ABANDON)));
}
}

26
java/com/google/gerrit/server/restapi/change/PutMessage.java
View File

@@ -33,7 +33,6 @@ import com.google.gerrit.server.PatchSetUtil;
import com.google.gerrit.server.change.ChangeResource;
import com.google.gerrit.server.change.NotifyUtil;
import com.google.gerrit.server.change.PatchSetInserter;
import com.google.gerrit.server.edit.UnchangedCommitMessageException;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.permissions.ChangePermission;
@@ -68,7 +67,7 @@ public class PutMessage
extends RetryingRestModifyView<ChangeResource, CommitMessageInput, Response<?>> {
private final GitRepositoryManager repositoryManager;
private final Provider<CurrentUser> currentUserProvider;
private final Provider<CurrentUser> userProvider;
private final Provider<ReviewDb> db;
private final TimeZone tz;
private final PatchSetInserter.Factory psInserterFactory;
@@ -81,7 +80,7 @@ public class PutMessage
PutMessage(
RetryHelper retryHelper,
GitRepositoryManager repositoryManager,
Provider<CurrentUser> currentUserProvider,
Provider<CurrentUser> userProvider,
Provider<ReviewDb> db,
PatchSetInserter.Factory psInserterFactory,
PermissionBackend permissionBackend,
@@ -91,7 +90,7 @@ public class PutMessage
ProjectCache projectCache) {
super(retryHelper);
this.repositoryManager = repositoryManager;
this.currentUserProvider = currentUserProvider;
this.userProvider = userProvider;
this.db = db;
this.psInserterFactory = psInserterFactory;
this.tz = gerritIdent.getTimeZone();
@@ -104,8 +103,8 @@ public class PutMessage
@Override
protected Response<String> applyImpl(
BatchUpdate.Factory updateFactory, ChangeResource resource, CommitMessageInput input)
throws IOException, UnchangedCommitMessageException, RestApiException, UpdateException,
PermissionBackendException, OrmException, ConfigInvalidException {
throws IOException, RestApiException, UpdateException, PermissionBackendException,
OrmException, ConfigInvalidException {
PatchSet ps = psUtil.current(db.get(), resource.getNotes());
if (ps == null) {
throw new ResourceConflictException("current revision is missing");
@@ -140,7 +139,7 @@ public class PutMessage
Timestamp ts = TimeUtil.nowTs();
try (BatchUpdate bu =
updateFactory.create(
db.get(), resource.getChange().getProject(), currentUserProvider.get(), ts)) {
db.get(), resource.getChange().getProject(), userProvider.get(), ts)) {
// Ensure that BatchUpdate will update the same repo
bu.setRepository(repository, new RevWalk(objectInserter.newReader()), objectInserter);
@@ -170,8 +169,7 @@ public class PutMessage
builder.setTreeId(basePatchSetCommit.getTree());
builder.setParentIds(basePatchSetCommit.getParents());
builder.setAuthor(basePatchSetCommit.getAuthorIdent());
builder.setCommitter(
currentUserProvider.get().asIdentifiedUser().newCommitterIdent(timestamp, tz));
builder.setCommitter(userProvider.get().asIdentifiedUser().newCommitterIdent(timestamp, tz));
builder.setMessage(commitMessage);
ObjectId newCommitId = objectInserter.insert(builder);
objectInserter.flush();
@@ -179,13 +177,17 @@ public class PutMessage
}
private void ensureCanEditCommitMessage(ChangeNotes changeNotes)
throws AuthException, PermissionBackendException, IOException, ResourceConflictException {
if (!currentUserProvider.get().isIdentifiedUser()) {
throws AuthException, PermissionBackendException, IOException, ResourceConflictException,
OrmException {
if (!userProvider.get().isIdentifiedUser()) {
throw new AuthException("Authentication required");
}
// Not allowed to put message if the current patch set is locked.
psUtil.checkPatchSetNotLocked(changeNotes, userProvider.get());
try {
permissionBackend
.user(currentUserProvider.get())
.user(userProvider.get())
.database(db.get())
.change(changeNotes)
.check(ChangePermission.ADD_PATCH_SET);

90
java/com/google/gerrit/server/restapi/change/Rebase.java
View File

@@ -14,16 +14,12 @@
package com.google.gerrit.server.restapi.change;
import static com.google.gerrit.extensions.conditions.BooleanCondition.and;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Sets;
import com.google.gerrit.common.TimeUtil;
import com.google.gerrit.common.errors.EmailException;
import com.google.gerrit.extensions.api.changes.RebaseInput;
import com.google.gerrit.extensions.client.ListChangesOption;
import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.extensions.conditions.BooleanCondition;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.extensions.restapi.RestApiException;
@@ -81,6 +77,7 @@ public class Rebase extends RetryingRestModifyView<RevisionResource, RebaseInput
private final Provider<ReviewDb> dbProvider;
private final PermissionBackend permissionBackend;
private final ProjectCache projectCache;
private final PatchSetUtil patchSetUtil;
@Inject
public Rebase(
@@ -91,7 +88,8 @@ public class Rebase extends RetryingRestModifyView<RevisionResource, RebaseInput
ChangeJson.Factory json,
Provider<ReviewDb> dbProvider,
PermissionBackend permissionBackend,
ProjectCache projectCache) {
ProjectCache projectCache,
PatchSetUtil patchSetUtil) {
super(retryHelper);
this.repoManager = repoManager;
this.rebaseFactory = rebaseFactory;
@@ -100,13 +98,17 @@ public class Rebase extends RetryingRestModifyView<RevisionResource, RebaseInput
this.dbProvider = dbProvider;
this.permissionBackend = permissionBackend;
this.projectCache = projectCache;
this.patchSetUtil = patchSetUtil;
}
@Override
protected ChangeInfo applyImpl(
BatchUpdate.Factory updateFactory, RevisionResource rsrc, RebaseInput input)
throws EmailException, OrmException, UpdateException, RestApiException, IOException,
NoSuchChangeException, PermissionBackendException {
throws OrmException, UpdateException, RestApiException, IOException,
PermissionBackendException {
// Not allowed to rebase if the current patch set is locked.
patchSetUtil.checkPatchSetNotLocked(rsrc.getNotes(), rsrc.getUser());
rsrc.permissions().database(dbProvider).check(ChangePermission.REBASE);
projectCache.checkedGet(rsrc.getProject()).checkStatePermitsWrite();
@@ -203,40 +205,54 @@ public class Rebase extends RetryingRestModifyView<RevisionResource, RebaseInput
}
@Override
public UiAction.Description getDescription(RevisionResource resource) {
PatchSet patchSet = resource.getPatchSet();
Change change = resource.getChange();
Branch.NameKey dest = change.getDest();
boolean visible = change.getStatus().isOpen() && resource.isCurrent();
boolean enabled = false;
public UiAction.Description getDescription(RevisionResource rsrc) {
UiAction.Description description =
new UiAction.Description()
.setLabel("Rebase")
.setTitle("Rebase onto tip of branch or parent change")
.setVisible(false);
Change change = rsrc.getChange();
if (!(change.getStatus().isOpen() && rsrc.isCurrent())) {
return description;
}
try {
visible &= projectCache.checkedGet(resource.getProject()).statePermitsWrite();
} catch (IOException e) {
log.error("Failed to check if project state permits write: " + resource.getProject(), e);
visible = false;
}
if (visible) {
try (Repository repo = repoManager.openRepository(dest.getParentKey());
RevWalk rw = new RevWalk(repo)) {
visible = hasOneParent(rw, resource.getPatchSet());
if (visible) {
enabled = rebaseUtil.canRebase(patchSet, dest, repo, rw);
}
} catch (IOException e) {
log.error("Failed to check if patch set can be rebased: " + resource.getPatchSet(), e);
visible = false;
if (!projectCache.checkedGet(rsrc.getProject()).statePermitsWrite()) {
return description;
}
} catch (IOException e) {
log.error("Failed to check if project state permits write: " + rsrc.getProject(), e);
return description;
}
BooleanCondition permissionCond =
resource.permissions().database(dbProvider).testCond(ChangePermission.REBASE);
return new UiAction.Description()
.setLabel("Rebase")
.setTitle("Rebase onto tip of branch or parent change")
.setVisible(and(visible, permissionCond))
.setEnabled(and(enabled, permissionCond));
try {
if (patchSetUtil.isPatchSetLocked(rsrc.getNotes(), rsrc.getUser())) {
return description;
}
} catch (OrmException | IOException e) {
log.error(
String.format(
"Failed to check if the current patch set of change %s is locked", change.getId()),
e);
return description;
}
boolean enabled = false;
try (Repository repo = repoManager.openRepository(change.getDest().getParentKey());
RevWalk rw = new RevWalk(repo)) {
if (hasOneParent(rw, rsrc.getPatchSet())) {
enabled = rebaseUtil.canRebase(rsrc.getPatchSet(), change.getDest(), repo, rw);
}
} catch (IOException e) {
log.error("Failed to check if patch set can be rebased: " + rsrc.getPatchSet(), e);
return description;
}
if (rsrc.permissions().database(dbProvider).testOrFalse(ChangePermission.REBASE)) {
return description.setVisible(true).setEnabled(enabled);
}
return description;
}
public static class CurrentRevision
@@ -254,7 +270,7 @@ public class Rebase extends RetryingRestModifyView<RevisionResource, RebaseInput
@Override
protected ChangeInfo applyImpl(
BatchUpdate.Factory updateFactory, ChangeResource rsrc, RebaseInput input)
throws EmailException, OrmException, UpdateException, RestApiException, IOException,
throws OrmException, UpdateException, RestApiException, IOException,
PermissionBackendException {
PatchSet ps = psUtil.current(rebase.dbProvider.get(), rsrc.getNotes());
if (ps == null) {

54
java/com/google/gerrit/server/restapi/change/Restore.java
View File

@@ -14,8 +14,6 @@
package com.google.gerrit.server.restapi.change;
import static com.google.gerrit.extensions.conditions.BooleanCondition.and;
import com.google.common.base.Strings;
import com.google.gerrit.common.TimeUtil;
import com.google.gerrit.extensions.api.changes.RestoreInput;
@@ -90,17 +88,20 @@ public class Restore extends RetryingRestModifyView<ChangeResource, RestoreInput
@Override
protected ChangeInfo applyImpl(
BatchUpdate.Factory updateFactory, ChangeResource req, RestoreInput input)
BatchUpdate.Factory updateFactory, ChangeResource rsrc, RestoreInput input)
throws RestApiException, UpdateException, OrmException, PermissionBackendException,
IOException {
req.permissions().database(dbProvider).check(ChangePermission.RESTORE);
projectCache.checkedGet(req.getProject()).checkStatePermitsWrite();
// Not allowed to restore if the current patch set is locked.
psUtil.checkPatchSetNotLocked(rsrc.getNotes(), rsrc.getUser());
rsrc.permissions().database(dbProvider).check(ChangePermission.RESTORE);
projectCache.checkedGet(rsrc.getProject()).checkStatePermitsWrite();
Op op = new Op(input);
try (BatchUpdate u =
updateFactory.create(
dbProvider.get(), req.getChange().getProject(), req.getUser(), TimeUtil.nowTs())) {
u.addOp(req.getId(), op).execute();
dbProvider.get(), rsrc.getChange().getProject(), rsrc.getUser(), TimeUtil.nowTs())) {
u.addOp(rsrc.getId(), op).execute();
}
return json.noOptions().format(op.change);
}
@@ -161,18 +162,39 @@ public class Restore extends RetryingRestModifyView<ChangeResource, RestoreInput
@Override
public UiAction.Description getDescription(ChangeResource rsrc) {
boolean projectStatePermitsWrite = false;
UiAction.Description description =
new UiAction.Description()
.setLabel("Restore")
.setTitle("Restore the change")
.setVisible(false);
Change change = rsrc.getChange();
if (change.getStatus() != Status.ABANDONED) {
return description;
}
try {
projectStatePermitsWrite = projectCache.checkedGet(rsrc.getProject()).statePermitsWrite();
if (!projectCache.checkedGet(rsrc.getProject()).statePermitsWrite()) {
return description;
}
} catch (IOException e) {
log.error("Failed to check if project state permits write: " + rsrc.getProject(), e);
return description;
}
return new UiAction.Description()
.setLabel("Restore")
.setTitle("Restore the change")
.setVisible(
and(
rsrc.getChange().getStatus() == Status.ABANDONED && projectStatePermitsWrite,
rsrc.permissions().database(dbProvider).testCond(ChangePermission.RESTORE)));
try {
if (psUtil.isPatchSetLocked(rsrc.getNotes(), rsrc.getUser())) {
return description;
}
} catch (OrmException | IOException e) {
log.error(
String.format(
"Failed to check if the current patch set of change %s is locked", change.getId()),
e);
return description;
}
boolean visible = rsrc.permissions().database(dbProvider).testOrFalse(ChangePermission.RESTORE);
return description.setVisible(visible);
}
}

5
javatests/com/google/gerrit/acceptance/rest/change/MoveChangeIT.java
View File

@@ -229,8 +229,9 @@ public class MoveChangeIT extends AbstractDaemonTest {
grant(project, "refs/heads/*", Permission.LABEL + "Patch-Set-Lock");
revision(r).review(new ReviewInput().label("Patch-Set-Lock", 1));
exception.expect(AuthException.class);
exception.expectMessage("move not permitted");
exception.expect(ResourceConflictException.class);
exception.expectMessage(
String.format("The current patch set of change %s is locked", r.getChange().getId()));
move(r.getChangeId(), newBranch.get());
}