diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java index 690c3dd2be..d9aeb514d4 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java @@ -557,11 +557,10 @@ public class ChangeIT extends AbstractDaemonTest { PushOneCommit.Result changeResult = pushFactory.create(db, user.getIdent(), testRepo).to("refs/for/master"); String changeId = changeResult.getChangeId(); - Change.Id id = changeResult.getChange().getId(); setApiUser(user); exception.expect(AuthException.class); - exception.expectMessage(String.format("Deleting change %s is not permitted", id)); + exception.expectMessage("delete not permitted"); gApi.changes().id(changeId).delete(); } @@ -605,11 +604,10 @@ public class ChangeIT extends AbstractDaemonTest { try { PushOneCommit.Result changeResult = createChange(); String changeId = changeResult.getChangeId(); - Change.Id id = changeResult.getChange().getId(); setApiUser(user); exception.expect(AuthException.class); - exception.expectMessage(String.format("Deleting change %s is not permitted", id)); + exception.expectMessage("delete not permitted"); gApi.changes().id(changeId).delete(); } finally { removePermission(Permission.DELETE_OWN_CHANGES, project, "refs/*"); @@ -633,13 +631,12 @@ public class ChangeIT extends AbstractDaemonTest { PushOneCommit.Result changeResult = pushFactory.create(db, user.getIdent(), testRepo).to("refs/for/master"); String changeId = changeResult.getChangeId(); - Change.Id id = changeResult.getChange().getId(); setApiUser(user); gApi.changes().id(changeId).abandon(); exception.expect(AuthException.class); - exception.expectMessage(String.format("Deleting change %s is not permitted", id)); + exception.expectMessage("delete not permitted"); gApi.changes().id(changeId).delete(); } @@ -661,12 +658,11 @@ public class ChangeIT extends AbstractDaemonTest { public void deleteMergedChange() throws Exception { PushOneCommit.Result changeResult = createChange(); String changeId = changeResult.getChangeId(); - Change.Id id = changeResult.getChange().getId(); merge(changeResult); exception.expect(MethodNotAllowedException.class); - exception.expectMessage(String.format("Deleting merged change %s is not allowed", id)); + exception.expectMessage("delete not permitted"); gApi.changes().id(changeId).delete(); } @@ -679,13 +675,12 @@ public class ChangeIT extends AbstractDaemonTest { PushOneCommit.Result changeResult = pushFactory.create(db, user.getIdent(), testRepo).to("refs/for/master"); String changeId = changeResult.getChangeId(); - Change.Id id = changeResult.getChange().getId(); merge(changeResult); setApiUser(user); exception.expect(MethodNotAllowedException.class); - exception.expectMessage(String.format("Deleting merged change %s is not allowed", id)); + exception.expectMessage("delete not permitted"); gApi.changes().id(changeId).delete(); } finally { removePermission(Permission.DELETE_OWN_CHANGES, project, "refs/*"); diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/DraftChangeIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/DraftChangeIT.java index da7d7b556f..26e68479dd 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/DraftChangeIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/DraftChangeIT.java @@ -82,14 +82,13 @@ public class DraftChangeIT extends AbstractDaemonTest { PushOneCommit.Result changeResult = createDraftChange(); changeResult.assertOkStatus(); String changeId = changeResult.getChangeId(); - Change.Id id = changeResult.getChange().getId(); // The user needs to be able to see the draft change (which reviewers can). gApi.changes().id(changeId).addReviewer(user.fullName); setApiUser(user); exception.expect(AuthException.class); - exception.expectMessage(String.format("Deleting change %s is not permitted", id)); + exception.expectMessage("delete not permitted"); gApi.changes().id(changeId).delete(); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/api/changes/ChangeApiImpl.java b/gerrit-server/src/main/java/com/google/gerrit/server/api/changes/ChangeApiImpl.java index cee2403a15..e41377e1b1 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/api/changes/ChangeApiImpl.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/api/changes/ChangeApiImpl.java @@ -375,7 +375,7 @@ class ChangeApiImpl implements ChangeApi { public void delete() throws RestApiException { try { deleteChange.apply(change, null); - } catch (UpdateException e) { + } catch (UpdateException | PermissionBackendException e) { throw new RestApiException("Cannot delete change", e); } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/api/changes/RevisionApiImpl.java b/gerrit-server/src/main/java/com/google/gerrit/server/api/changes/RevisionApiImpl.java index d934f6cb2e..6440509e80 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/api/changes/RevisionApiImpl.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/api/changes/RevisionApiImpl.java @@ -262,7 +262,7 @@ class RevisionApiImpl implements RevisionApi { public void delete() throws RestApiException { try { deleteDraft.apply(revision, null); - } catch (UpdateException e) { + } catch (UpdateException | OrmException | PermissionBackendException e) { throw new RestApiException("Cannot delete draft ps", e); } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteChange.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteChange.java index ad823d47f8..151ffa1f71 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteChange.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteChange.java @@ -15,20 +15,24 @@ package com.google.gerrit.server.change; import com.google.gerrit.common.TimeUtil; +import com.google.gerrit.extensions.restapi.AuthException; +import com.google.gerrit.extensions.restapi.MethodNotAllowedException; import com.google.gerrit.extensions.restapi.Response; import com.google.gerrit.extensions.restapi.RestApiException; import com.google.gerrit.extensions.restapi.RestModifyView; import com.google.gerrit.extensions.webui.UiAction; import com.google.gerrit.reviewdb.client.Change; -import com.google.gerrit.reviewdb.client.Change.Status; import com.google.gerrit.reviewdb.server.ReviewDb; +import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.change.DeleteChange.Input; import com.google.gerrit.server.config.GerritServerConfig; -import com.google.gerrit.server.project.ChangeControl; +import com.google.gerrit.server.permissions.ChangePermission; +import com.google.gerrit.server.permissions.GlobalPermission; +import com.google.gerrit.server.permissions.PermissionBackend; +import com.google.gerrit.server.permissions.PermissionBackendException; import com.google.gerrit.server.update.BatchUpdate; import com.google.gerrit.server.update.Order; import com.google.gerrit.server.update.UpdateException; -import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; import com.google.inject.Provider; import com.google.inject.Singleton; @@ -42,6 +46,8 @@ public class DeleteChange private final Provider db; private final BatchUpdate.Factory updateFactory; private final Provider opProvider; + private final Provider user; + private final PermissionBackend permissionBackend; private final boolean allowDrafts; @Inject @@ -49,16 +55,33 @@ public class DeleteChange Provider db, BatchUpdate.Factory updateFactory, Provider opProvider, + Provider user, + PermissionBackend permissionBackend, @GerritServerConfig Config cfg) { this.db = db; this.updateFactory = updateFactory; this.opProvider = opProvider; + this.user = user; + this.permissionBackend = permissionBackend; this.allowDrafts = DeleteChangeOp.allowDrafts(cfg); } @Override public Response apply(ChangeResource rsrc, Input input) - throws RestApiException, UpdateException { + throws RestApiException, UpdateException, PermissionBackendException { + if (rsrc.getChange().getStatus() == Change.Status.MERGED) { + throw new MethodNotAllowedException("delete not permitted"); + } else if (!allowDrafts && rsrc.getChange().getStatus() == Change.Status.DRAFT) { + // If drafts are disabled, only an administrator can delete a draft. + try { + permissionBackend.user(user).check(GlobalPermission.ADMINISTRATE_SERVER); + } catch (AuthException e) { + throw new MethodNotAllowedException("Draft workflow is disabled"); + } + } else { + rsrc.permissions().database(db).check(ChangePermission.DELETE); + } + try (BatchUpdate bu = updateFactory.create(db.get(), rsrc.getProject(), rsrc.getUser(), TimeUtil.nowTs())) { Change.Id id = rsrc.getChange().getId(); @@ -71,21 +94,33 @@ public class DeleteChange @Override public UiAction.Description getDescription(ChangeResource rsrc) { - try { - Change.Status status = rsrc.getChange().getStatus(); - ChangeControl changeControl = rsrc.getControl(); - boolean visible = - isActionAllowed(changeControl, status) && changeControl.canDelete(db.get(), status); - return new UiAction.Description() - .setLabel("Delete") - .setTitle("Delete change " + rsrc.getId()) - .setVisible(visible); - } catch (OrmException e) { - throw new IllegalStateException(e); - } + Change.Status status = rsrc.getChange().getStatus(); + PermissionBackend.ForChange perm = rsrc.permissions().database(db); + return new UiAction.Description() + .setLabel("Delete") + .setTitle("Delete change " + rsrc.getId()) + .setVisible(couldDeleteWhenIn(status) && perm.testOrFalse(ChangePermission.DELETE)); } - private boolean isActionAllowed(ChangeControl changeControl, Status status) { - return status != Status.DRAFT || allowDrafts || changeControl.isAdmin(); + private boolean couldDeleteWhenIn(Change.Status status) { + switch (status) { + case NEW: + case ABANDONED: + // New or abandoned changes can be deleted with the right permissions. + return true; + + case MERGED: + // Merged changes should never be deleted. + return false; + + case DRAFT: + if (allowDrafts) { + // Drafts can only be deleted if the server has drafts enabled. + return true; + } + // If drafts are disabled, only administrators may delete. + return permissionBackend.user(user).testOrFalse(GlobalPermission.ADMINISTRATE_SERVER); + } + return false; } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteChangeOp.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteChangeOp.java index 992313db6c..8ab1d2afc7 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteChangeOp.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteChangeOp.java @@ -17,7 +17,6 @@ package com.google.gerrit.server.change; import static com.google.common.base.Preconditions.checkState; import com.google.gerrit.extensions.registration.DynamicItem; -import com.google.gerrit.extensions.restapi.AuthException; import com.google.gerrit.extensions.restapi.MethodNotAllowedException; import com.google.gerrit.extensions.restapi.ResourceConflictException; import com.google.gerrit.extensions.restapi.RestApiException; @@ -27,7 +26,6 @@ import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.reviewdb.server.ReviewDbUtil; import com.google.gerrit.server.PatchSetUtil; import com.google.gerrit.server.StarredChangesUtil; -import com.google.gerrit.server.config.GerritServerConfig; import com.google.gerrit.server.project.NoSuchChangeException; import com.google.gerrit.server.update.BatchUpdateOp; import com.google.gerrit.server.update.BatchUpdateReviewDb; @@ -62,7 +60,6 @@ class DeleteChangeOp implements BatchUpdateOp { private final PatchSetUtil psUtil; private final StarredChangesUtil starredChangesUtil; private final DynamicItem accountPatchReviewStore; - private final boolean allowDrafts; private Change.Id id; @@ -70,12 +67,10 @@ class DeleteChangeOp implements BatchUpdateOp { DeleteChangeOp( PatchSetUtil psUtil, StarredChangesUtil starredChangesUtil, - DynamicItem accountPatchReviewStore, - @GerritServerConfig Config cfg) { + DynamicItem accountPatchReviewStore) { this.psUtil = psUtil; this.starredChangesUtil = starredChangesUtil; this.accountPatchReviewStore = accountPatchReviewStore; - this.allowDrafts = allowDrafts(cfg); } @Override @@ -99,8 +94,7 @@ class DeleteChangeOp implements BatchUpdateOp { } private void ensureDeletable(ChangeContext ctx, Change.Id id, Collection patchSets) - throws ResourceConflictException, MethodNotAllowedException, OrmException, AuthException, - IOException { + throws ResourceConflictException, MethodNotAllowedException, IOException { Change.Status status = ctx.getChange().getStatus(); if (status == Change.Status.MERGED) { throw new MethodNotAllowedException("Deleting merged change " + id + " is not allowed"); @@ -114,14 +108,7 @@ class DeleteChangeOp implements BatchUpdateOp { } } - if (!ctx.getControl().canDelete(ctx.getDb(), status)) { - throw new AuthException("Deleting change " + id + " is not permitted"); - } - if (status == Change.Status.DRAFT) { - if (!allowDrafts && !ctx.getControl().isAdmin()) { - throw new MethodNotAllowedException("Draft workflow is disabled"); - } for (PatchSet ps : patchSets) { if (!ps.isDraft()) { throw new ResourceConflictException( diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteDraftPatchSet.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteDraftPatchSet.java index a4db8c5ec8..583bc58abf 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteDraftPatchSet.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteDraftPatchSet.java @@ -14,6 +14,7 @@ package com.google.gerrit.server.change; +import com.google.common.collect.Iterables; import com.google.gerrit.common.TimeUtil; import com.google.gerrit.extensions.registration.DynamicItem; import com.google.gerrit.extensions.restapi.AuthException; @@ -32,6 +33,8 @@ import com.google.gerrit.server.change.DeleteDraftPatchSet.Input; import com.google.gerrit.server.config.GerritServerConfig; import com.google.gerrit.server.patch.PatchSetInfoFactory; import com.google.gerrit.server.patch.PatchSetInfoNotAvailableException; +import com.google.gerrit.server.permissions.ChangePermission; +import com.google.gerrit.server.permissions.PermissionBackendException; import com.google.gerrit.server.project.NoSuchChangeException; import com.google.gerrit.server.update.BatchUpdate; import com.google.gerrit.server.update.BatchUpdateOp; @@ -81,7 +84,12 @@ public class DeleteDraftPatchSet @Override public Response apply(RevisionResource rsrc, Input input) - throws RestApiException, UpdateException { + throws RestApiException, UpdateException, OrmException, PermissionBackendException { + if (isDeletingOnlyPatchSet(rsrc)) { + // A change cannot have zero patch sets; the change is deleted instead. + rsrc.permissions().database(db).check(ChangePermission.DELETE); + } + try (BatchUpdate bu = updateFactory.create(db.get(), rsrc.getProject(), rsrc.getUser(), TimeUtil.nowTs())) { bu.setOrder(Order.DB_BEFORE_REPO); @@ -91,6 +99,12 @@ public class DeleteDraftPatchSet return Response.none(); } + private boolean isDeletingOnlyPatchSet(RevisionResource rsrc) throws OrmException { + Collection patchSets = psUtil.byChange(db.get(), rsrc.getNotes()); + return patchSets.size() == 1 + && Iterables.getOnlyElement(patchSets).getId().equals(rsrc.getPatchSet().getId()); + } + private class Op implements BatchUpdateOp { private final PatchSet.Id psId; @@ -183,15 +197,14 @@ public class DeleteDraftPatchSet @Override public UiAction.Description getDescription(RevisionResource rsrc) { try { - int psCount = psUtil.byChange(db.get(), rsrc.getNotes()).size(); return new UiAction.Description() .setLabel("Delete") .setTitle(String.format("Delete draft revision %d", rsrc.getPatchSet().getPatchSetId())) .setVisible( allowDrafts && rsrc.getPatchSet().isDraft() - && rsrc.getControl().canDelete(db.get(), Change.Status.DRAFT) - && psCount > 1); + && psUtil.byChange(db.get(), rsrc.getNotes()).size() > 1 + && rsrc.getControl().canDelete(db.get(), Change.Status.DRAFT)); } catch (OrmException e) { throw new IllegalStateException(e); } 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 3e5eba3a97..1bb110379c 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 @@ -275,7 +275,7 @@ public class ChangeControl { switch (status) { case DRAFT: - return (isOwner() || getRefControl().canDeleteDrafts()); + return isOwner() || getRefControl().canDeleteDrafts() || isAdmin(); case NEW: case ABANDONED: return (isAdmin() || (isOwner() && getRefControl().canDeleteOwnChanges()));