Check delete change with PermissionBackend
Deleting the entire change is slightly differently from deleting a
draft patch set. Update only the change deletion path.
Update test assertions to expect the new generic messages thrown
by the PermissionBackend.
Change-Id: If5a83e386304d8f84db37f6c02f9c35cd5ae4266
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 690c3dd2..d9aeb51 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 @@
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 @@
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 @@
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 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 @@
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 da7d7b5..26e6847 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 @@
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 cee2403..e41377e 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 @@
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 d934f6c..6440509 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 @@
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 ad823d4..151ffa1 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 @@
private final Provider<ReviewDb> db;
private final BatchUpdate.Factory updateFactory;
private final Provider<DeleteChangeOp> opProvider;
+ private final Provider<CurrentUser> user;
+ private final PermissionBackend permissionBackend;
private final boolean allowDrafts;
@Inject
@@ -49,16 +55,33 @@
Provider<ReviewDb> db,
BatchUpdate.Factory updateFactory,
Provider<DeleteChangeOp> opProvider,
+ Provider<CurrentUser> 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 @@
@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 992313d..8ab1d2a 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 @@
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.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 @@
private final PatchSetUtil psUtil;
private final StarredChangesUtil starredChangesUtil;
private final DynamicItem<AccountPatchReviewStore> accountPatchReviewStore;
- private final boolean allowDrafts;
private Change.Id id;
@@ -70,12 +67,10 @@
DeleteChangeOp(
PatchSetUtil psUtil,
StarredChangesUtil starredChangesUtil,
- DynamicItem<AccountPatchReviewStore> accountPatchReviewStore,
- @GerritServerConfig Config cfg) {
+ DynamicItem<AccountPatchReviewStore> accountPatchReviewStore) {
this.psUtil = psUtil;
this.starredChangesUtil = starredChangesUtil;
this.accountPatchReviewStore = accountPatchReviewStore;
- this.allowDrafts = allowDrafts(cfg);
}
@Override
@@ -99,8 +94,7 @@
}
private void ensureDeletable(ChangeContext ctx, Change.Id id, Collection<PatchSet> 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 @@
}
}
- 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 a4db8c5..583bc58 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.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 @@
@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 @@
return Response.none();
}
+ private boolean isDeletingOnlyPatchSet(RevisionResource rsrc) throws OrmException {
+ Collection<PatchSet> 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 @@
@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 3e5eba3..1bb1103 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 @@
switch (status) {
case DRAFT:
- return (isOwner() || getRefControl().canDeleteDrafts());
+ return isOwner() || getRefControl().canDeleteDrafts() || isAdmin();
case NEW:
case ABANDONED:
return (isAdmin() || (isOwner() && getRefControl().canDeleteOwnChanges()));
