From 31c83332df68d47b7c00f7047e6249cc9ea1b2d3 Mon Sep 17 00:00:00 2001 From: Alice Kober-Sotzek Date: Wed, 19 Oct 2016 14:23:03 +0200 Subject: [PATCH] Allow deletion of open and abandoned changes If a user includes some sensitive information in a change, admins need to be able to delete it. Previously, only draft changes could be deleted. Deleting merged changes isn't supported because we would need to rewrite history. Feature: Issue 4509 Change-Id: I6a5bf055257b3762271c61fdd82a349e24148be3 --- Documentation/rest-api-changes.txt | 12 +- .../acceptance/api/change/ChangeIT.java | 107 ++++++++- .../acceptance/rest/change/DraftChangeIT.java | 220 ++++++++++++++++-- .../extensions/api/changes/ChangeApi.java | 2 +- .../gerrit/client/change/ChangeConstants.java | 2 +- .../client/change/ChangeConstants.properties | 2 +- .../gerrit/client/change/ChangeScreen.java | 18 +- .../server/api/changes/ChangeApiImpl.java | 10 +- .../server/change/ConsistencyChecker.java | 2 +- ...leteDraftChange.java => DeleteChange.java} | 28 ++- ...DraftChangeOp.java => DeleteChangeOp.java} | 40 ++-- .../server/change/DeleteDraftPatchSet.java | 14 +- .../google/gerrit/server/change/Module.java | 2 +- .../gerrit/server/project/ChangeControl.java | 25 +- 14 files changed, 409 insertions(+), 75 deletions(-) rename gerrit-server/src/main/java/com/google/gerrit/server/change/{DeleteDraftChange.java => DeleteChange.java} (75%) rename gerrit-server/src/main/java/com/google/gerrit/server/change/{DeleteDraftChangeOp.java => DeleteChangeOp.java} (82%) diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt index e5073b2da3..a258b32641 100644 --- a/Documentation/rest-api-changes.txt +++ b/Documentation/rest-api-changes.txt @@ -1767,13 +1767,19 @@ Publishes a draft change. HTTP/1.1 204 No Content ---- -[[delete-draft-change]] -=== Delete Draft Change +[[delete-change]] +=== Delete Change -- 'DELETE /changes/link:#change-id[\{change-id\}]' -- -Deletes a draft change. +Deletes a change. + +New or abandoned changes can only be deleted by administrators. The deletion of +merged changes isn't supported at the moment. Draft changes can only be deleted +by their owner or other users who have the permissions to view and delete +drafts. If the draft workflow is disabled, only administrators with those +permissions may delete draft changes. .Request ---- 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 d995be6f43..8af40987c3 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 @@ -68,6 +68,7 @@ import com.google.gerrit.extensions.common.MergeInput; import com.google.gerrit.extensions.common.MergePatchSetInput; import com.google.gerrit.extensions.common.RevisionInfo; 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.ResourceNotFoundException; import com.google.gerrit.extensions.restapi.UnprocessableEntityException; @@ -398,7 +399,7 @@ public class ChangeIT extends AbstractDaemonTest { } @Test - public void delete() throws Exception { + public void deleteDraftChange() throws Exception { PushOneCommit.Result r = createChange("refs/drafts/master"); assertThat(query(r.getChangeId())).hasSize(1); assertThat(info(r.getChangeId()).status).isEqualTo(ChangeStatus.DRAFT); @@ -408,6 +409,110 @@ public class ChangeIT extends AbstractDaemonTest { assertThat(query(r.getChangeId())).isEmpty(); } + @Test + public void deleteNewChangeAsAdmin() throws Exception { + PushOneCommit.Result changeResult = createChange(); + String changeId = changeResult.getChangeId(); + + gApi.changes() + .id(changeId) + .delete(); + + assertThat(query(changeId)).isEmpty(); + } + + @Test + @TestProjectInput(cloneAs = "user") + public void deleteNewChangeAsNormalUser() throws Exception { + 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)); + gApi.changes() + .id(changeId) + .delete(); + } + + @Test + @TestProjectInput(cloneAs = "user") + public void deleteNewChangeOfAnotherUserAsAdmin() throws Exception { + PushOneCommit.Result changeResult = + pushFactory.create(db, user.getIdent(), testRepo) + .to("refs/for/master"); + changeResult.assertOkStatus(); + String changeId = changeResult.getChangeId(); + + setApiUser(admin); + gApi.changes() + .id(changeId) + .delete(); + + assertThat(query(changeId)).isEmpty(); + } + + @Test + @TestProjectInput(cloneAs = "user") + public void deleteAbandonedChangeAsNormalUser() throws Exception { + 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)); + gApi.changes() + .id(changeId) + .delete(); + } + + @Test + @TestProjectInput(cloneAs = "user") + public void deleteAbandonedChangeOfAnotherUserAsAdmin() throws Exception { + PushOneCommit.Result changeResult = + pushFactory.create(db, user.getIdent(), testRepo) + .to("refs/for/master"); + String changeId = changeResult.getChangeId(); + + gApi.changes() + .id(changeId) + .abandon(); + + gApi.changes() + .id(changeId) + .delete(); + + assertThat(query(changeId)).isEmpty(); + } + + @Test + 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)); + gApi.changes() + .id(changeId) + .delete(); + } + @Test public void rebaseUpToDateChange() throws Exception { PushOneCommit.Result r = createChange(); 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 2a32abe011..0283674cdf 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 @@ -22,20 +22,38 @@ import com.google.gerrit.acceptance.AbstractDaemonTest; import com.google.gerrit.acceptance.PushOneCommit; import com.google.gerrit.acceptance.RestResponse; import com.google.gerrit.acceptance.RestSession; +import com.google.gerrit.acceptance.TestProjectInput; +import com.google.gerrit.common.TimeUtil; +import com.google.gerrit.common.data.Permission; import com.google.gerrit.extensions.api.changes.ReviewInput; import com.google.gerrit.extensions.client.ChangeStatus; +import com.google.gerrit.extensions.client.ListChangesOption; import com.google.gerrit.extensions.client.ReviewerState; import com.google.gerrit.extensions.common.AccountInfo; import com.google.gerrit.extensions.common.ChangeInfo; import com.google.gerrit.extensions.common.LabelInfo; +import com.google.gerrit.extensions.common.RevisionInfo; +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.ResourceNotFoundException; +import com.google.gerrit.extensions.restapi.RestApiException; +import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.PatchSet; +import com.google.gerrit.server.git.BatchUpdate; +import com.google.gerrit.server.git.UpdateException; +import com.google.gerrit.server.notedb.PatchSetState; import com.google.gerrit.testutil.ConfigSuite; +import com.google.gwtorm.server.OrmException; +import com.google.inject.Inject; import org.eclipse.jgit.lib.Config; import org.junit.Test; import java.util.Collection; +import java.util.EnumSet; +import java.util.List; +import java.util.stream.Collectors; public class DraftChangeIT extends AbstractDaemonTest { @ConfigSuite.Config @@ -43,20 +61,8 @@ public class DraftChangeIT extends AbstractDaemonTest { return allowDraftsDisabledConfig(); } - @Test - public void deleteChange() throws Exception { - PushOneCommit.Result result = createChange(); - result.assertOkStatus(); - String changeId = result.getChangeId(); - String triplet = project.get() + "~master~" + changeId; - ChangeInfo c = get(triplet); - assertThat(c.id).isEqualTo(triplet); - assertThat(c.status).isEqualTo(ChangeStatus.NEW); - RestResponse response = deleteChange(changeId, adminRestSession); - assertThat(response.getEntityContent()) - .isEqualTo("Change is not a draft: " + c._number); - response.assertConflict(); - } + @Inject + private BatchUpdate.Factory updateFactory; @Test public void deleteDraftChange() throws Exception { @@ -74,6 +80,104 @@ public class DraftChangeIT extends AbstractDaemonTest { get(triplet); } + @Test + public void deleteDraftChangeOfAnotherUser() throws Exception { + assume().that(isAllowDrafts()).isTrue(); + 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)); + gApi.changes() + .id(changeId) + .delete(); + } + + @Test + @TestProjectInput(cloneAs = "user") + public void deleteDraftChangeWhenDraftsNotAllowedAsNormalUser() + throws Exception { + assume().that(isAllowDrafts()).isFalse(); + + setApiUser(user); + // We can't create a draft change while the draft workflow is disabled. + // For this reason, we create a normal change and modify the database. + PushOneCommit.Result changeResult = + pushFactory.create(db, user.getIdent(), testRepo) + .to("refs/for/master"); + Change.Id id = changeResult.getChange().getId(); + markChangeAsDraft(id); + setDraftStatusOfPatchSetsOfChange(id, true); + + String changeId = changeResult.getChangeId(); + exception.expect(MethodNotAllowedException.class); + exception.expectMessage("Draft workflow is disabled"); + gApi.changes() + .id(changeId) + .delete(); + } + + @Test + @TestProjectInput(cloneAs = "user") + public void deleteDraftChangeWhenDraftsNotAllowedAsAdmin() throws Exception { + assume().that(isAllowDrafts()).isFalse(); + + setApiUser(user); + // We can't create a draft change while the draft workflow is disabled. + // For this reason, we create a normal change and modify the database. + PushOneCommit.Result changeResult = + pushFactory.create(db, user.getIdent(), testRepo) + .to("refs/for/master"); + Change.Id id = changeResult.getChange().getId(); + markChangeAsDraft(id); + setDraftStatusOfPatchSetsOfChange(id, true); + + String changeId = changeResult.getChangeId(); + + // Grant those permissions to admins. + grant(Permission.VIEW_DRAFTS, project, "refs/*"); + grant(Permission.DELETE_DRAFTS, project, "refs/*"); + + try { + setApiUser(admin); + gApi.changes() + .id(changeId) + .delete(); + } finally { + removePermission(Permission.DELETE_DRAFTS, project, "refs/*"); + removePermission(Permission.VIEW_DRAFTS, project, "refs/*"); + } + + setApiUser(user); + assertThat(query(changeId)).isEmpty(); + } + + @Test + public void deleteDraftChangeWithNonDraftPatchSet() throws Exception { + assume().that(isAllowDrafts()).isTrue(); + + PushOneCommit.Result changeResult = createDraftChange(); + Change.Id id = changeResult.getChange().getId(); + setDraftStatusOfPatchSetsOfChange(id, false); + + String changeId = changeResult.getChangeId(); + exception.expect(ResourceConflictException.class); + exception.expectMessage(String.format( + "Cannot delete draft change %s: patch set 1 is not a draft", id)); + gApi.changes() + .id(changeId) + .delete(); + } + @Test public void publishDraftChange() throws Exception { assume().that(isAllowDrafts()).isTrue(); @@ -160,4 +264,92 @@ public class DraftChangeIT extends AbstractDaemonTest { + patchSet.getRevision().get() + "/publish"); } + + private void markChangeAsDraft(Change.Id id) throws OrmException, + RestApiException, UpdateException { + try (BatchUpdate batchUpdate = updateFactory + .create(db, project, atrScope.get().getUser(), TimeUtil.nowTs())) { + batchUpdate.addOp(id, new MarkChangeAsDraftUpdateOp()); + batchUpdate.execute(); + } + + ChangeStatus changeStatus = gApi.changes() + .id(id.get()) + .get() + .status; + assertThat(changeStatus).isEqualTo(ChangeStatus.DRAFT); + } + + private void setDraftStatusOfPatchSetsOfChange(Change.Id id, + boolean draftStatus) throws OrmException, RestApiException, + UpdateException { + try (BatchUpdate batchUpdate = updateFactory + .create(db, project, atrScope.get().getUser(), TimeUtil.nowTs())) { + batchUpdate.addOp(id, new DraftStatusOfPatchSetsUpdateOp(draftStatus)); + batchUpdate.execute(); + } + + Boolean expectedDraftStatus = draftStatus ? Boolean.TRUE : null; + List patchSetDraftStatuses = getPatchSetDraftStatuses(id); + patchSetDraftStatuses.forEach(status -> + assertThat(status).isEqualTo(expectedDraftStatus)); + } + + private List getPatchSetDraftStatuses(Change.Id id) + throws RestApiException { + Collection revisionInfos = gApi.changes() + .id(id.get()) + .get(EnumSet.of(ListChangesOption.ALL_REVISIONS)) + .revisions + .values(); + return revisionInfos.stream() + .map(revisionInfo -> revisionInfo.draft) + .collect(Collectors.toList()); + } + + private class MarkChangeAsDraftUpdateOp extends BatchUpdate.Op { + @Override + public boolean updateChange(BatchUpdate.ChangeContext ctx) + throws Exception { + Change change = ctx.getChange(); + + // Change status in database. + change.setStatus(Change.Status.DRAFT); + + // Change status in NoteDb. + PatchSet.Id currentPatchSetId = change.currentPatchSetId(); + ctx.getUpdate(currentPatchSetId).setStatus(Change.Status.DRAFT); + + return true; + } + } + + private class DraftStatusOfPatchSetsUpdateOp extends BatchUpdate.Op { + private final boolean draftStatus; + + DraftStatusOfPatchSetsUpdateOp(boolean draftStatus) { + this.draftStatus = draftStatus; + } + + @Override + public boolean updateChange(BatchUpdate.ChangeContext ctx) + throws Exception { + Collection patchSets = psUtil.byChange(db, ctx.getNotes()); + + // Change status in database. + patchSets.forEach(patchSet -> patchSet.setDraft(draftStatus)); + db.patchSets().update(patchSets); + + // Change status in NoteDb. + PatchSetState patchSetState = draftStatus ? PatchSetState.DRAFT + : PatchSetState.PUBLISHED; + patchSets.stream() + .map(PatchSet::getId) + .map(ctx::getUpdate) + .forEach(changeUpdate -> + changeUpdate.setPatchSetState(patchSetState)); + + return true; + } + } } diff --git a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/ChangeApi.java b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/ChangeApi.java index bd5dd6978c..629ad975fa 100644 --- a/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/ChangeApi.java +++ b/gerrit-extension-api/src/main/java/com/google/gerrit/extensions/api/changes/ChangeApi.java @@ -112,7 +112,7 @@ public interface ChangeApi { void publish() throws RestApiException; /** - * Deletes a draft change. + * Deletes a change. */ void delete() throws RestApiException; diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/ChangeConstants.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/ChangeConstants.java index 63de3890aa..99f3b9f245 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/ChangeConstants.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/ChangeConstants.java @@ -51,6 +51,6 @@ public interface ChangeConstants extends Constants { String abandoned(); String deleteChangeEdit(); - String deleteDraftChange(); + String deleteChange(); String deleteDraftRevision(); } diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/ChangeConstants.properties b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/ChangeConstants.properties index 5b4f18f519..dd4760d068 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/ChangeConstants.properties +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/ChangeConstants.properties @@ -34,5 +34,5 @@ abandoned = Abandoned deleteChangeEdit = Delete Change Edit?\n\ \n\ All changes made in the edit revision will be lost. -deleteDraftChange = Delete Draft Change? +deleteChange = Delete Change? deleteDraftRevision = Delete Draft Revision? diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/ChangeScreen.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/ChangeScreen.java index 436e0c33bc..fa3855eff4 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/ChangeScreen.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/change/ChangeScreen.java @@ -495,15 +495,13 @@ public class ChangeScreen extends Screen { } private void initChangeAction(ChangeInfo info) { - if (info.status() == Status.DRAFT) { - NativeMap actions = info.hasActions() - ? info.actions() - : NativeMap. create(); - actions.copyKeysIntoChildren("id"); - if (actions.containsKey("/")) { - deleteChange.setVisible(true); - deleteChange.setTitle(actions.get("/").title()); - } + NativeMap actions = info.hasActions() + ? info.actions() + : NativeMap.create(); + actions.copyKeysIntoChildren("id"); + if (actions.containsKey("/")) { + deleteChange.setVisible(true); + deleteChange.setTitle(actions.get("/").title()); } } @@ -679,7 +677,7 @@ public class ChangeScreen extends Screen { @UiHandler("deleteChange") void onDeleteChange(@SuppressWarnings("unused") ClickEvent e) { - if (Window.confirm(Resources.C.deleteDraftChange())) { + if (Window.confirm(Resources.C.deleteChange())) { DraftActions.delete(changeId, publish, deleteRevision, deleteChange); } } 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 31c70d9203..a26516030e 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 @@ -45,7 +45,7 @@ import com.google.gerrit.server.change.ChangeResource; import com.google.gerrit.server.change.Check; import com.google.gerrit.server.change.CreateMergePatchSet; import com.google.gerrit.server.change.DeleteAssignee; -import com.google.gerrit.server.change.DeleteDraftChange; +import com.google.gerrit.server.change.DeleteChange; import com.google.gerrit.server.change.GetAssignee; import com.google.gerrit.server.change.GetHashtags; import com.google.gerrit.server.change.GetPastAssignees; @@ -98,7 +98,7 @@ class ChangeApiImpl implements ChangeApi { private final Provider submittedTogether; private final PublishDraftPatchSet.CurrentRevision publishDraftChange; - private final DeleteDraftChange deleteDraftChange; + private final DeleteChange deleteChange; private final GetTopic getTopic; private final PutTopic putTopic; private final PostReviewers postReviewers; @@ -129,7 +129,7 @@ class ChangeApiImpl implements ChangeApi { CreateMergePatchSet updateByMerge, Provider submittedTogether, PublishDraftPatchSet.CurrentRevision publishDraftChange, - DeleteDraftChange deleteDraftChange, + DeleteChange deleteChange, GetTopic getTopic, PutTopic putTopic, PostReviewers postReviewers, @@ -159,7 +159,7 @@ class ChangeApiImpl implements ChangeApi { this.updateByMerge = updateByMerge; this.submittedTogether = submittedTogether; this.publishDraftChange = publishDraftChange; - this.deleteDraftChange = deleteDraftChange; + this.deleteChange = deleteChange; this.getTopic = getTopic; this.putTopic = putTopic; this.postReviewers = postReviewers; @@ -324,7 +324,7 @@ class ChangeApiImpl implements ChangeApi { @Override public void delete() throws RestApiException { try { - deleteDraftChange.apply(change, null); + deleteChange.apply(change, null); } catch (UpdateException e) { throw new RestApiException("Cannot delete change", e); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ConsistencyChecker.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ConsistencyChecker.java index 30ed82f070..20e5b9d540 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ConsistencyChecker.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ConsistencyChecker.java @@ -658,7 +658,7 @@ public class ConsistencyChecker { public boolean updateChange(ChangeContext ctx) throws OrmException, PatchSetInfoNotAvailableException { // Delete dangling key references. - ReviewDb db = DeleteDraftChangeOp.unwrap(ctx.getDb()); + ReviewDb db = DeleteChangeOp.unwrap(ctx.getDb()); accountPatchReviewStore.get().clearReviewed(psId); db.changeMessages().delete( db.changeMessages().byChange(psId.getParentKey())); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteDraftChange.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteChange.java similarity index 75% rename from gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteDraftChange.java rename to gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteChange.java index a125272e4e..18d7074001 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteDraftChange.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteChange.java @@ -22,10 +22,11 @@ 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.change.DeleteDraftChange.Input; +import com.google.gerrit.server.change.DeleteChange.Input; import com.google.gerrit.server.config.GerritServerConfig; import com.google.gerrit.server.git.BatchUpdate; import com.google.gerrit.server.git.UpdateException; +import com.google.gerrit.server.project.ChangeControl; import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; import com.google.inject.Provider; @@ -34,25 +35,25 @@ import com.google.inject.Singleton; import org.eclipse.jgit.lib.Config; @Singleton -public class DeleteDraftChange implements +public class DeleteChange implements RestModifyView, UiAction { public static class Input { } private final Provider db; private final BatchUpdate.Factory updateFactory; - private final Provider opProvider; + private final Provider opProvider; private final boolean allowDrafts; @Inject - public DeleteDraftChange(Provider db, + public DeleteChange(Provider db, BatchUpdate.Factory updateFactory, - Provider opProvider, + Provider opProvider, @GerritServerConfig Config cfg) { this.db = db; this.updateFactory = updateFactory; this.opProvider = opProvider; - this.allowDrafts = DeleteDraftChangeOp.allowDrafts(cfg); + this.allowDrafts = DeleteChangeOp.allowDrafts(cfg); } @Override @@ -71,14 +72,21 @@ public class DeleteDraftChange implements @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 draft change " + rsrc.getId()) - .setVisible(allowDrafts - && rsrc.getChange().getStatus() == Status.DRAFT - && rsrc.getControl().canDeleteDraft(db.get())); + .setTitle("Delete change " + rsrc.getId()) + .setVisible(visible); } catch (OrmException e) { throw new IllegalStateException(e); } } + + private boolean isActionAllowed(ChangeControl changeControl, + Status status) { + return status != Status.DRAFT || allowDrafts || changeControl.isAdmin(); + } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteDraftChangeOp.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteChangeOp.java similarity index 82% rename from gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteDraftChangeOp.java rename to gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteChangeOp.java index 4ed6a25a34..0e77a50e27 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteDraftChangeOp.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteChangeOp.java @@ -44,7 +44,7 @@ import org.eclipse.jgit.transport.ReceiveCommand; import java.io.IOException; import java.util.Collection; -class DeleteDraftChangeOp extends BatchUpdate.Op { +class DeleteChangeOp extends BatchUpdate.Op { static boolean allowDrafts(Config cfg) { return cfg.getBoolean("change", "allowDrafts", true); } @@ -68,7 +68,7 @@ class DeleteDraftChangeOp extends BatchUpdate.Op { private Change.Id id; @Inject - DeleteDraftChangeOp(PatchSetUtil psUtil, + DeleteChangeOp(PatchSetUtil psUtil, StarredChangesUtil starredChangesUtil, DynamicItem accountPatchReviewStore, @GerritServerConfig Config cfg) { @@ -82,16 +82,18 @@ class DeleteDraftChangeOp extends BatchUpdate.Op { public boolean updateChange(ChangeContext ctx) throws RestApiException, OrmException, IOException, NoSuchChangeException { checkState(ctx.getOrder() == BatchUpdate.Order.DB_BEFORE_REPO, - "must use DeleteDraftChangeOp with DB_BEFORE_REPO"); - checkState(id == null, "cannot reuse DeleteDraftChangeOp"); + "must use DeleteChangeOp with DB_BEFORE_REPO"); + checkState(id == null, "cannot reuse DeleteChangeOp"); id = ctx.getChange().getId(); Collection patchSets = psUtil.byChange(ctx.getDb(), ctx.getNotes()); ensureDeletable(ctx, id, patchSets); - deleteChangeElementsFromDb(ctx, id); + // Cleaning up is only possible as long as the change and its elements are + // still part of the database. cleanUpReferences(ctx, id, patchSets); + deleteChangeElementsFromDb(ctx, id); ctx.deleteChange(); return true; @@ -100,19 +102,25 @@ class DeleteDraftChangeOp extends BatchUpdate.Op { private void ensureDeletable(ChangeContext ctx, Change.Id id, Collection patchSets) throws ResourceConflictException, MethodNotAllowedException, OrmException, AuthException { - if (ctx.getChange().getStatus() != Change.Status.DRAFT) { - throw new ResourceConflictException("Change is not a draft: " + id); + Change.Status status = ctx.getChange().getStatus(); + if (status == Change.Status.MERGED) { + throw new MethodNotAllowedException("Deleting merged change " + id + + " is not allowed"); } - if (!allowDrafts) { - throw new MethodNotAllowedException("Draft workflow is disabled"); + + if (!ctx.getControl().canDelete(ctx.getDb(), status)) { + throw new AuthException("Deleting change " + id + " is not permitted"); } - if (!ctx.getControl().canDeleteDraft(ctx.getDb())) { - throw new AuthException("Not permitted to delete this draft change"); - } - for (PatchSet ps : patchSets) { - if (!ps.isDraft()) { - throw new ResourceConflictException("Cannot delete draft change " + id - + ": patch set " + ps.getPatchSetId() + " is not a draft"); + + 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("Cannot delete draft change " + id + + ": patch set " + ps.getPatchSetId() + " is not a draft"); + } } } } 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 1cd8726313..e473e39583 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 @@ -59,7 +59,7 @@ public class DeleteDraftPatchSet implements RestModifyView deleteChangeOpProvider; + private final Provider deleteChangeOpProvider; private final DynamicItem accountPatchReviewStore; private final boolean allowDrafts; @@ -68,7 +68,7 @@ public class DeleteDraftPatchSet implements RestModifyView deleteChangeOpProvider, + Provider deleteChangeOpProvider, DynamicItem accountPatchReviewStore, @GerritServerConfig Config cfg) { this.db = db; @@ -97,7 +97,7 @@ public class DeleteDraftPatchSet implements RestModifyView patchSetsBeforeDeletion; private PatchSet patchSet; - private DeleteDraftChangeOp deleteChangeOp; + private DeleteChangeOp deleteChangeOp; private Op(PatchSet.Id psId) { this.psId = psId; @@ -116,7 +116,7 @@ public class DeleteDraftPatchSet implements RestModifyView 1); } catch (OrmException e) { throw new IllegalStateException(e); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/Module.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/Module.java index bb760845d1..9ff9833490 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/Module.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/Module.java @@ -68,7 +68,7 @@ public class Module extends RestApiModule { post(CHANGE_KIND, "check").to(Check.class); put(CHANGE_KIND, "topic").to(PutTopic.class); delete(CHANGE_KIND, "topic").to(PutTopic.class); - delete(CHANGE_KIND).to(DeleteDraftChange.class); + delete(CHANGE_KIND).to(DeleteChange.class); post(CHANGE_KIND, "abandon").to(Abandon.class); post(CHANGE_KIND, "hashtags").to(PostHashtags.class); post(CHANGE_KIND, "publish").to(PublishDraftPatchSet.CurrentRevision.class); 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 5cc461fafc..c4d8dcd1f1 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 @@ -261,10 +261,23 @@ public class ChangeControl { && isVisible(db); } - /** Can this user delete this draft change or any draft patch set of this change? */ - public boolean canDeleteDraft(final ReviewDb db) throws OrmException { - return (isOwner() || getRefControl().canDeleteDrafts()) - && isVisible(db); + /** Can this user delete this change or any patch set of this change? */ + public boolean canDelete(ReviewDb db, Change.Status status) + throws OrmException { + if (!isVisible(db)) { + return false; + } + + switch (status) { + case DRAFT: + return (isOwner() || getRefControl().canDeleteDrafts()); + case NEW: + case ABANDONED: + return isAdmin(); + case MERGED: + default: + return false; + } } /** Can this user rebase this change? */ @@ -377,6 +390,10 @@ public class ChangeControl { return false; } + public boolean isAdmin() { + return getUser().getCapabilities().canAdministrateServer(); + } + /** @return true if the user is allowed to remove this reviewer. */ public boolean canRemoveReviewer(PatchSetApproval approval) { return canRemoveReviewer(approval.getAccountId(), approval.getValue());